Cover byte reclamation: one guarded helper, file first, covers row last (#154)

2026-08-22 09:34:02 +07:00
parent e1d61534bb
commit a4ea80dcc2
4 changed files with 345 additions and 7 deletions
@@ -158,8 +158,8 @@ func (p *Poller) storeCover(ctx context.Context, sr store.Series, sourceURL stri
 	// The forced write replaces whether or not a Cover exists, and the row
 	// then tells the three outcomes apart: a blank filled, identical artwork
 	// re-served — an honest no-op — or a replacement whose previous address
-	// is stranded: its bytes stay served under the covers table (ADR-0014),
-	// the row just no longer points at them.
+	// is stranded and reclaimed below. A failed reclaim is logged and the
+	// stranded bytes stay served until a later call reclaims them.
 	previous, current, err := p.Store.ReplaceSeriesCover(sr.Site, sr.SeriesID, sourceURL, bytes, contentType)
 	if err != nil {
 		log.Printf("latest poll %q: persist cover: %v", sr.Key(), err)
@@ -171,6 +171,9 @@ func (p *Poller) storeCover(ctx context.Context, sr store.Series, sourceURL stri
 	case previous == current:
 		log.Printf("latest poll %q: cover unchanged, the site re-serves the same bytes", sr.Key())
 	default:
+		if err := p.Store.ReclaimCover(previous); err != nil {
+			log.Printf("latest poll %q: reclaim cover %s: %v", sr.Key(), previous, err)
+		}
 		log.Printf("latest poll %q: cover replaced %s -> %s", sr.Key(), previous, current)
 	}
 }
@@ -6,6 +6,7 @@ import (
 	"database/sql"
 	"errors"
 	"fmt"
+	"io/fs"
 	"log"
 	"os"
 	"path/filepath"
@@ -2558,3 +2559,139 @@ func TestRunOnceForcedPassWithoutCoverFetcherStillPolls(t *testing.T) {
 		t.Fatalf("missing skipped-cover log; log:\n%s", logs.String())
 	}
 }
+
+// A forced replacement strands the previous address, and the poller reclaims
+// it from the stranded branch: the old sharded file and covers row are both
+// gone once the pass lands while the new bytes read back (issue #154). The
+// identical-bytes no-op that follows reclaims nothing: previous == current
+// there, and a reclamation would delete the Cover the pass just wrote.
+func TestRunOnceForcedPassReclaimsSupersededCover(t *testing.T) {
+	coverDir := t.TempDir()
+	url := pgtest.URL(t)
+	s, err := store.Open(url, testOwner, coverDir, testCoverBaseURL)
+	if err != nil {
+		t.Fatalf("Open: %v", err)
+	}
+	t.Cleanup(func() { s.Close() })
+	const (
+		key       = "asura:chronicles-of-the-demon-faction-f886a8af"
+		seriesID  = "chronicles-of-the-demon-faction-f886a8af"
+		seriesURL = "https://asurascans.com/comics/chronicles-of-the-demon-faction-f886a8af"
+		first     = "https://cdn.example/covers/first.jpg"
+	)
+	if _, err := s.Upsert(s.OwnerID(), store.Bookmark{
+		Key: key, Site: "asura", SeriesID: seriesID, SeriesURL: seriesURL, UpdatedAt: 1000,
+	}); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+	if err := s.SetSeriesCover("asura", seriesID, first, []byte("first"), "image/jpeg"); err != nil {
+		t.Fatalf("seed cover: %v", err)
+	}
+	stale := store.CoverAddressForBytes([]byte("first"))
+	stalePath := filepath.Join(coverDir, filepath.FromSlash(stale[:2]+"/"+stale[2:4]+"/"+stale))
+
+	at := time.UnixMilli(5_000_000)
+	if err := s.ForceSeriesPoll("asura", seriesID, at.Add(time.Hour).UnixMilli()); err != nil {
+		t.Fatalf("ForceSeriesPoll: %v", err)
+	}
+	p := &Poller{
+		Store: s, Fetch: &fakeFetcher{body: asuraSeriesFixture + asuraCoverFixture, status: 200},
+		CoverBytesFetch: &fakeBytesCoverFetcher{body: []byte("second"), contentType: "image/jpeg"},
+		Now:             func() time.Time { return at },
+	}
+	p.runOnce(context.Background())
+	p.waitCovers()
+
+	// The stranded address is gone on disk and in SQL; the new one reads back.
+	if _, err := os.Stat(stalePath); !errors.Is(err, fs.ErrNotExist) {
+		t.Fatalf("stale sharded file after replacement = %v, want fs.ErrNotExist", err)
+	}
+	if _, _, ok, err := s.CoverByAddress(stale); err != nil || ok {
+		t.Fatalf("stale bytes after replacement = found %v err %v, want reclaimed", ok, err)
+	}
+	if body, _, ok, err := s.CoverByAddress(store.CoverAddressForBytes([]byte("second"))); err != nil || !ok || string(body) != "second" {
+		t.Fatalf("new bytes after replacement = found %v err %v, want served", ok, err)
+	}
+
+	// The identical-bytes pass is an honest no-op and reclaims nothing.
+	if err := s.ForceSeriesPoll("asura", seriesID, at.Add(2*time.Hour).UnixMilli()); err != nil {
+		t.Fatalf("ForceSeriesPoll: %v", err)
+	}
+	p.runOnce(context.Background())
+	p.waitCovers()
+	current := store.CoverAddressForBytes([]byte("second"))
+	currentPath := filepath.Join(coverDir, filepath.FromSlash(current[:2]+"/"+current[2:4]+"/"+current))
+	if _, err := os.Stat(currentPath); err != nil {
+		t.Fatalf("live sharded file after no-op pass = %v, want present", err)
+	}
+	if body, _, ok, err := s.CoverByAddress(current); err != nil || !ok || string(body) != "second" {
+		t.Fatalf("bytes after no-op pass = found %v err %v, want still served", ok, err)
+	}
+}
+
+// A reclamation that fails must not fail the Poll: the failure is logged
+// against the Series and the replacement still lands, so the stranded bytes
+// stay reachable for a retry and the owner's act succeeded (issue #154).
+func TestRunOnceForcedPassReclaimFailureDoesNotFailPoll(t *testing.T) {
+	coverDir := t.TempDir()
+	url := pgtest.URL(t)
+	s, err := store.Open(url, testOwner, coverDir, testCoverBaseURL)
+	if err != nil {
+		t.Fatalf("Open: %v", err)
+	}
+	t.Cleanup(func() { s.Close() })
+	const (
+		key       = "asura:chronicles-of-the-fallen-f886a8af"
+		seriesID  = "chronicles-of-the-fallen-f886a8af"
+		seriesURL = "https://asurascans.com/comics/chronicles-of-the-fallen-f886a8af"
+		first     = "https://cdn.example/covers/first.jpg"
+	)
+	if _, err := s.Upsert(s.OwnerID(), store.Bookmark{
+		Key: key, Site: "asura", SeriesID: seriesID, SeriesURL: seriesURL, UpdatedAt: 1000,
+	}); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+	if err := s.SetSeriesCover("asura", seriesID, first, []byte("first"), "image/jpeg"); err != nil {
+		t.Fatalf("seed cover: %v", err)
+	}
+	// Make the stranded file unremovable: a non-empty directory in its place.
+	stale := store.CoverAddressForBytes([]byte("first"))
+	stalePath := filepath.Join(coverDir, filepath.FromSlash(stale[:2]+"/"+stale[2:4]+"/"+stale))
+	if err := os.Remove(stalePath); err != nil {
+		t.Fatalf("clear file: %v", err)
+	}
+	if err := os.Mkdir(stalePath, 0o755); err != nil {
+		t.Fatalf("replace file with dir: %v", err)
+	}
+	if err := os.WriteFile(filepath.Join(stalePath, "blob"), []byte("x"), 0o644); err != nil {
+		t.Fatalf("fill dir: %v", err)
+	}
+
+	at := time.UnixMilli(5_000_000)
+	if err := s.ForceSeriesPoll("asura", seriesID, at.Add(time.Hour).UnixMilli()); err != nil {
+		t.Fatalf("ForceSeriesPoll: %v", err)
+	}
+	var logs strings.Builder
+	prev := log.Writer()
+	log.SetOutput(&logs)
+	t.Cleanup(func() { log.SetOutput(prev) })
+	p := &Poller{
+		Store: s, Fetch: &fakeFetcher{body: asuraSeriesFixture + asuraCoverFixture, status: 200},
+		CoverBytesFetch: &fakeBytesCoverFetcher{body: []byte("second"), contentType: "image/jpeg"},
+		Now:             func() time.Time { return at },
+	}
+	p.runOnce(context.Background())
+	p.waitCovers()
+
+	logged := logs.String()
+	if !strings.Contains(logged, "reclaim cover") {
+		t.Fatalf("failed reclamation not logged; log:\n%s", logged)
+	}
+	if !strings.Contains(logged, "cover replaced") {
+		t.Fatalf("replacement not reported after a failed reclaim; log:\n%s", logged)
+	}
+	got := readBookmark(t, s, key)
+	if want := testCoverBaseURL + "/covers/" + store.CoverAddressForBytes([]byte("second")); got.Cover != want {
+		t.Fatalf("Cover = %q, want the replacement %q", got.Cover, want)
+	}
+}