diff --git a/backend/internal/latest/poller.go b/backend/internal/latest/poller.go index b10ba02..1c6ae7c 100644 --- a/backend/internal/latest/poller.go +++ b/backend/internal/latest/poller.go @@ -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) } } diff --git a/backend/internal/latest/poller_test.go b/backend/internal/latest/poller_test.go index 550160a..301225b 100644 --- a/backend/internal/latest/poller_test.go +++ b/backend/internal/latest/poller_test.go @@ -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) + } +} diff --git a/backend/internal/store/store.go b/backend/internal/store/store.go index 4321818..b403f6d 100644 --- a/backend/internal/store/store.go +++ b/backend/internal/store/store.go @@ -746,6 +746,42 @@ func (s *Store) putCover(sourceURL string, body []byte, contentType string) (str return address, nil } +// ReclaimCover permanently removes a Cover nothing references: the sharded +// file first, the covers row last. A blank address is a no-op, and so is any +// address a Series row still points at — byte-identical artwork is one row by +// construction (ADR-0014), so reclaiming one Series' stranded bytes must not +// blank another's. The file goes first because the covers row is the handle: +// an interrupted run stays findable in SQL — covers rows unreferenced by any +// series cover_address — and re-running finishes the job, whereas deleting +// the row first would leave a file nothing names. A concurrent Forced Poll +// repointing a live Series at this address between the guard and the unlink +// is the repairable case: the missing file reads as ok=false and the next +// pass re-installs it. Failures are returned, never logged here — the caller +// logs and carries on — and a failed unlink leaves the row in place for a +// retry. A whole-table sweep, if ever wanted, is one SQL query over covers, +// not a tree walk and not this function. +func (s *Store) ReclaimCover(address string) error { + if address == "" { + return nil + } + var referenced int + err := s.db.QueryRow(`SELECT 1 FROM series WHERE cover_address = $1 LIMIT 1`, address).Scan(&referenced) + if err == nil { + return nil + } + if !errors.Is(err, sql.ErrNoRows) { + return fmt.Errorf("guard reclaim of cover %q: %w", address, err) + } + coverPath := filepath.Join(s.coverDir, filepath.FromSlash(coverRelativePath(address))) + if err := os.Remove(coverPath); err != nil && !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("remove cover file %q: %w", address, err) + } + if _, err := s.db.Exec(`DELETE FROM covers WHERE address = $1`, address); err != nil { + return fmt.Errorf("delete cover row %q: %w", address, err) + } + return nil +} + // GetCover returns the immutable object a source URL's own hash names. Rows // written before byte addressing (ADR-0014) are the only ones that ever reach // it; it hashes the URL, so a byte-addressed Cover is invisible to it. Missing @@ -825,9 +861,10 @@ func (s *Store) SetSeriesCover(site, seriesID, sourceURL string, body []byte, co // ("" if it had none) and current the address of the bytes just stored; both // are read and written in one transaction, so a concurrent replacement reports // the exact displacement. previous == current means the Site served identical -// artwork, an honest no-op; otherwise previous is stranded — its bytes stay -// served under the covers table (ADR-0014), the row just no longer points at -// them. +// artwork, an honest no-op; otherwise previous is stranded — the row no +// longer points at it, and reclaiming its bytes is the caller's separate act +// (the poller's replace path calls ReclaimCover on it). This write itself +// removes nothing. func (s *Store) ReplaceSeriesCover(site, seriesID, sourceURL string, body []byte, contentType string) (previous, current string, err error) { current, err = s.putCover(sourceURL, body, contentType) if err != nil { diff --git a/backend/internal/store/store_test.go b/backend/internal/store/store_test.go index a14be84..db286b9 100644 --- a/backend/internal/store/store_test.go +++ b/backend/internal/store/store_test.go @@ -5,6 +5,8 @@ import ( "crypto/sha256" "database/sql" "encoding/hex" + "errors" + "io/fs" "os" "path/filepath" "strconv" @@ -2029,6 +2031,7 @@ func (s *Store) latestCorrectedAt(t *testing.T, site, seriesID string) int64 { // num2 boxes a chapter number for the Bookmark fields that take a pointer. func num2(f float64) *float64 { return &f } + // --- Cover addressing (ADR-0014): the address is the bytes' SHA-256 --- // The address is what makes a re-art visible at all, so the same bytes must @@ -2094,8 +2097,8 @@ func TestReplaceSeriesCover(t *testing.T) { sr.Cover != "https://cdn.asurascans.com/covers/solo-rebrand.webp" { t.Fatalf("series after replacement = %+v, want the new address and source URL", sr) } - // The replaced bytes stay served under their old address; nothing reclaims - // them in this ticket (the forced-poll wave does). + // The replaced bytes stay served under their old address: ReplaceSeriesCover + // itself reclaims nothing, reclamation is the caller's separate act (#154). if _, _, ok, err := store.CoverByAddress(CoverAddressForBytes([]byte("first-art"))); err != nil || !ok { t.Fatalf("superseded bytes = found %v, err %v, want still served", ok, err) } @@ -2179,3 +2182,161 @@ func TestSetSeriesURLWritesWhereUpsertIgnores(t *testing.T) { t.Fatalf("stored URL = %q, want %q", sr.SeriesURL, repair) } } + +// --- Cover byte reclamation (issue #154): one guarded helper, file first --- + +// coverShardPath is the on-disk location of one address's bytes, built the +// same way getCoverByAddress reads them. +func coverShardPath(t *testing.T, s *Store, address string) string { + t.Helper() + return filepath.Join(s.coverDir, filepath.FromSlash(coverRelativePath(address))) +} + +// ReclaimCover removes a Cover nothing references: the row alone is not the +// point — the sharded file must be gone too, because the file is the reclaimed +// disk space. +func TestReclaimCoverRemovesUnreferencedBytes(t *testing.T) { + store := newTestStore(t) + seedForCheck(t, store, "asura:solo", "https://asurascans.com/comics/solo", 0) + if err := store.SetSeriesCover("asura", "solo", "https://cdn.example/covers/old.jpg", []byte("old-art"), "image/jpeg"); err != nil { + t.Fatalf("put cover: %v", err) + } + old := CoverAddressForBytes([]byte("old-art")) + if _, _, err := store.ReplaceSeriesCover("asura", "solo", "https://cdn.example/covers/new.jpg", []byte("new-art"), "image/jpeg"); err != nil { + t.Fatalf("replace cover: %v", err) + } + + if err := store.ReclaimCover(old); err != nil { + t.Fatalf("ReclaimCover: %v", err) + } + if _, err := os.Stat(coverShardPath(t, store, old)); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("sharded path after reclaim = %v, want fs.ErrNotExist", err) + } + if _, _, ok, err := store.CoverByAddress(old); err != nil || ok { + t.Fatalf("covers row after reclaim = found %v err %v, want gone", ok, err) + } + // The live Cover survives the reclamation of the stranded one. + if body, _, ok, err := store.CoverByAddress(CoverAddressForBytes([]byte("new-art"))); err != nil || !ok || string(body) != "new-art" { + t.Fatalf("new bytes after reclaim = found %v err %v, want still served", ok, err) + } +} + +// The guard is the whole design: byte-identical artwork is one covers row by +// construction (ADR-0014), so a second Series pointing at the address must +// keep the bytes — reclaiming one Series' stranded artwork may not blank +// another's. +func TestReclaimCoverSparesReferencedAddress(t *testing.T) { + store := newTestStore(t) + seedForCheck(t, store, "asura:solo", "https://asurascans.com/comics/solo", 0) + const src = "https://cdn.example/covers/shared.jpg" + addr := CoverAddressForBytes([]byte("shared-art")) + if err := store.SetSeriesCover("asura", "solo", src, []byte("shared-art"), "image/jpeg"); err != nil { + t.Fatalf("put cover: %v", err) + } + + // One Series pointing at the address is enough for the guard. + if err := store.ReclaimCover(addr); err != nil { + t.Fatalf("ReclaimCover on a referenced address: %v", err) + } + if body, _, ok, err := store.CoverByAddress(addr); err != nil || !ok || string(body) != "shared-art" { + t.Fatalf("bytes after no-op = found %v err %v, want still served", ok, err) + } + if _, err := os.Stat(coverShardPath(t, store, addr)); err != nil { + t.Fatalf("sharded file after no-op: %v, want present", err) + } + + // A second Series serving identical bytes shares the row by construction. + seedForCheck(t, store, "asura:second", "https://asurascans.com/comics/second", 0) + if err := store.SetSeriesCover("asura", "second", src, []byte("shared-art"), "image/jpeg"); err != nil { + t.Fatalf("share cover: %v", err) + } + if err := store.ReclaimCover(addr); err != nil { + t.Fatalf("ReclaimCover on a shared address: %v", err) + } + if body, _, ok, err := store.CoverByAddress(addr); err != nil || !ok || string(body) != "shared-art" { + t.Fatalf("shared bytes after no-op = found %v err %v, want still served", ok, err) + } + if _, err := os.Stat(coverShardPath(t, store, addr)); err != nil { + t.Fatalf("sharded file after shared no-op: %v, want present", err) + } +} + +// A blank address is the wire value for "no Cover" (ADR-0007), never a +// reclaimable one. +func TestReclaimCoverBlankAddressIsNoOp(t *testing.T) { + store := newTestStore(t) + if err := store.ReclaimCover(""); err != nil { + t.Fatalf("ReclaimCover(\"\") = %v, want nil", err) + } +} + +// An interrupted reclamation is the state the file-first order exists for: +// the row is the handle, so the unreferenced-covers query finds the torn +// Cover and re-running ReclaimCover finishes the job — a missing file is +// "already gone", which counts as success. +func TestReclaimCoverInterruptedRunIsFindableAndFinishes(t *testing.T) { + store := newTestStore(t) + seedForCheck(t, store, "asura:solo", "https://asurascans.com/comics/solo", 0) + if err := store.SetSeriesCover("asura", "solo", "https://cdn.example/covers/torn.jpg", []byte("torn-art"), "image/jpeg"); err != nil { + t.Fatalf("put cover: %v", err) + } + torn := CoverAddressForBytes([]byte("torn-art")) + if _, _, err := store.ReplaceSeriesCover("asura", "solo", "https://cdn.example/covers/new.jpg", []byte("new-art"), "image/jpeg"); err != nil { + t.Fatalf("replace cover: %v", err) + } + if err := os.Remove(coverShardPath(t, store, torn)); err != nil { + t.Fatalf("unlink mid-reclamation: %v", err) + } + + var found string + err := store.db.QueryRow(` + SELECT address FROM covers c + WHERE NOT EXISTS (SELECT 1 FROM series s WHERE s.cover_address = c.address) + LIMIT 1`).Scan(&found) + if err != nil || found != torn { + t.Fatalf("unreferenced-covers query = (%q, %v), want the torn row %q", found, err, torn) + } + + if err := store.ReclaimCover(torn); err != nil { + t.Fatalf("re-run over a missing file: %v", err) + } + if _, _, ok, err := store.CoverByAddress(torn); err != nil || ok { + t.Fatalf("row after re-run = found %v err %v, want gone", ok, err) + } +} + +// A failed file removal is the one state that is not self-cleaning: the +// covers row must survive so a retry can finish the job, and the store +// returns the error rather than logging — each caller logs and carries on, +// so the failure has no user-facing surface. +func TestReclaimCoverFailedUnlinkKeepsRow(t *testing.T) { + store := newTestStore(t) + seedForCheck(t, store, "asura:solo", "https://asurascans.com/comics/solo", 0) + if err := store.SetSeriesCover("asura", "solo", "https://cdn.example/covers/stuck.jpg", []byte("stuck-art"), "image/jpeg"); err != nil { + t.Fatalf("put cover: %v", err) + } + stuck := CoverAddressForBytes([]byte("stuck-art")) + if _, _, err := store.ReplaceSeriesCover("asura", "solo", "https://cdn.example/covers/other.jpg", []byte("other-art"), "image/jpeg"); err != nil { + t.Fatalf("replace cover: %v", err) + } + // Make the unlink fail: the sharded path becomes a non-empty directory, + // which os.Remove refuses. + shard := coverShardPath(t, store, stuck) + if err := os.Remove(shard); err != nil { + t.Fatalf("clear file: %v", err) + } + if err := os.Mkdir(shard, 0o755); err != nil { + t.Fatalf("replace file with dir: %v", err) + } + if err := os.WriteFile(filepath.Join(shard, "blob"), []byte("x"), 0o644); err != nil { + t.Fatalf("fill dir: %v", err) + } + + if err := store.ReclaimCover(stuck); err == nil { + t.Fatal("ReclaimCover over an unremovable file = nil, want the error") + } + var one int + if err := store.db.QueryRow(`SELECT 1 FROM covers WHERE address = $1`, stuck).Scan(&one); err != nil { + t.Fatal("covers row after failed unlink is gone; want it left for a retry") + } +}