feat(cover): acquire a Series Cover at creation (#59)

A Reader who bookmarks a Series nobody holds yet no longer waits out the poll queue for its artwork: the first Bookmark to create a Series fires Store.OnSeriesCreated, and latest.Acquirer turns that into a single series-page fetch yielding both the Latest Chapter and the cover URL. The bytes are fetched through the gated cover fetcher and stored content-addressed, so the wire carries an absolute URL on this deployment's own origin (ADR-0007) - never a third-party address and never one that 404s. - series.cover_address (migration 0009) splits the third-party source address the bytes came from (series.cover) from the content address they are stored under. A blank cover_address is what "no Cover yet" means, so the wire field is empty until real bytes exist. - GET /covers/{address} serves the bytes publicly and uncredentialed, immutable-cached; the address is gated by a 64-hex pattern and cross-checked against a pure function of itself before any filesystem read. - Client-sent cover values are decoded and discarded permanently: the cover columns are absent from Upsert's INSERT and its DO UPDATE, so no request value can reach the shared Series row (extends ADR-0003's "ignored after creation" to "ignored always", keeps ADR-0004's flat wire so installed userscripts keep working). - Acquisition is asynchronous and log-and-drop: the Reader's write neither blocks on nor fails with a third-party Site. It is bounded by a two-slot semaphore, cancelled at shutdown, and stamps latest_checked_at so the poller does not refetch the same page a tick later. - store.CoverContentType canonicalises comix's non-standard "image/jpg" to "image/jpeg", so one image cannot land under two spellings. - PUBLIC_BASE_URL is a new required setting; Open rejects anything that is not an absolute http(s) origin, since a bare hostname would start cleanly and emit addresses no browser can load. Verified against a live backend: bookmarking a comix series produced a 280x420 JPEG served from /covers/<sha256> with the immutable cache header, and the web UI card renders that address.
2026-08-10 02:31:13 +07:00
parent b6b88bde8a
commit 64c27fe896
18 changed files with 971 additions and 115 deletions
@@ -21,9 +21,12 @@ func TestMain(m *testing.M) { os.Exit(pgtest.Main(m)) }
 // reader register one (see secondReader).
 var testOwner = Owner{DiscordID: "test-owner", TokenHash: sha256.Sum256([]byte("owner-token-hash"))}

+// testCoverBaseURL is the public origin every stored cover URL is built from.
+const testCoverBaseURL = "https://bookmarks.test"
+
 func newTestStore(t *testing.T) *Store {
 	t.Helper()
-	store, err := Open(pgtest.URL(t), testOwner, t.TempDir())
+	store, err := Open(pgtest.URL(t), testOwner, t.TempDir(), testCoverBaseURL)
 	if err != nil {
 		t.Fatalf("Open: %v", err)
 	}
@@ -49,7 +52,7 @@ func secondReader(t *testing.T, s *Store) int64 {
 func TestOpenIsIdempotent(t *testing.T) {
 	url := pgtest.URL(t)
 	coverDir := t.TempDir()
-	first, err := Open(url, testOwner, coverDir)
+	first, err := Open(url, testOwner, coverDir, testCoverBaseURL)
 	if err != nil {
 		t.Fatalf("Open: %v", err)
 	}
@@ -60,7 +63,7 @@ func TestOpenIsIdempotent(t *testing.T) {
 	}
 	first.Close()

-	second, err := Open(url, testOwner, coverDir)
+	second, err := Open(url, testOwner, coverDir, testCoverBaseURL)
 	if err != nil {
 		t.Fatalf("reopen: %v", err)
 	}
@@ -113,7 +116,7 @@ func TestReaderTokenInfo(t *testing.T) {
 func TestRotateTokenInvalidatesOldAndSurvivesRestart(t *testing.T) {
 	url := pgtest.URL(t)
 	coverDir := t.TempDir()
-	store, err := Open(url, testOwner, coverDir)
+	store, err := Open(url, testOwner, coverDir, testCoverBaseURL)
 	if err != nil {
 		t.Fatalf("Open: %v", err)
 	}
@@ -145,7 +148,7 @@ func TestRotateTokenInvalidatesOldAndSurvivesRestart(t *testing.T) {
 	}
 	store.Close()

-	reopened, err := Open(url, testOwner, coverDir)
+	reopened, err := Open(url, testOwner, coverDir, testCoverBaseURL)
 	if err != nil {
 		t.Fatalf("reopen: %v", err)
 	}
@@ -547,32 +550,39 @@ func TestDisplayChapter(t *testing.T) {
 	}
 }

+// CoverURL reads the source address for the kagane branch and the wire value
+// otherwise, so both are set the way scanBookmark sets them.
 func TestCoverURL(t *testing.T) {
 	cases := []struct {
-		name  string
-		cover string
-		want  string
+		name        string
+		coverSource string
+		cover       string
+		want        string
 	}{
 		{
 			"kagane routes through the proxy",
 			"https://kagane.to/api/v2/image/019fe11a-84c3-7fc3-a84b-88787374b617/compressed",
+			"https://bookmarks.test/covers/" + CoverAddress("kagane"),
 			"/img/kagane/019fe11a-84c3-7fc3-a84b-88787374b617",
 		},
 		{
-			"another site is served as stored",
-			"https://gg.asuracomic.net/storage/media/1/conversions/cover.webp",
+			"another site is served from our own origin",
 			"https://gg.asuracomic.net/storage/media/1/conversions/cover.webp",
+			"https://bookmarks.test/covers/" + CoverAddress("asura"),
+			"https://bookmarks.test/covers/" + CoverAddress("asura"),
 		},
 		{
 			"a lookalike host is not rewritten",
 			"https://evil.example/api/v2/image/019fe11a-84c3-7fc3-a84b-88787374b617/compressed",
-			"https://evil.example/api/v2/image/019fe11a-84c3-7fc3-a84b-88787374b617/compressed",
+			"https://bookmarks.test/covers/" + CoverAddress("evil"),
+			"https://bookmarks.test/covers/" + CoverAddress("evil"),
 		},
-		{"no cover stays empty", "", ""},
+		{"no cover stays empty", "", "", ""},
 	}
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			if got := (Bookmark{Cover: tc.cover}).CoverURL(); got != tc.want {
+			b := Bookmark{CoverSource: tc.coverSource, Cover: tc.cover}
+			if got := b.CoverURL(); got != tc.want {
 				t.Errorf("CoverURL() = %q, want %q", got, tc.want)
 			}
 		})
@@ -672,7 +682,7 @@ func TestMigration0002BackfillsExistingBookmarks(t *testing.T) {
 	// Bring it current through the production path: Open runs the schema to
 	// 0003, seeds the owner, then applies 0004 which attaches this row. 0002
 	// must have backfilled the series row, not lost data.
-	st, err := Open(url, testOwner, t.TempDir())
+	st, err := Open(url, testOwner, t.TempDir(), testCoverBaseURL)
 	if err != nil {
 		t.Fatalf("Open after migrate: %v", err)
 	}
@@ -756,39 +766,143 @@ func readSeries(t *testing.T, s *Store, site, seriesID string) Series {
 	return sr
 }

-// The first PUT for a series creates its row from the client's title, cover
-// and URL — there is no other source for them (ADR-0003).
+// The first PUT for a series creates its row from the client's title and URL —
+// there is no other source for them (ADR-0003). The Cover is not among them:
+// it is acquired server-side, so a client-supplied one is dropped even on a
+// brand-new row (ADR-0007).
 func TestUpsertCreatesSeriesFromClient(t *testing.T) {
 	store := newTestStore(t)
-	if _, err := store.Upsert(store.OwnerID(), Bookmark{
+	stored, err := store.Upsert(store.OwnerID(), Bookmark{
 		Key: "asura:solo", Site: "asura", SeriesID: "solo",
 		Title: "Solo Leveling", SeriesURL: "https://asurascans.com/comics/solo",
 		Cover: "https://asurascans.com/covers/solo.jpg", Kind: KindManga,
 		UpdatedAt: 1000,
-	}); err != nil {
+	})
+	if err != nil {
 		t.Fatalf("Upsert: %v", err)
 	}
+	if stored.Cover != "" {
+		t.Fatalf("Cover = %q, want empty — a client cover is never stored", stored.Cover)
+	}

 	sr := readSeries(t, store, "asura", "solo")
-	if sr.Title != "Solo Leveling" || sr.SeriesURL != "https://asurascans.com/comics/solo" ||
-		sr.Cover != "https://asurascans.com/covers/solo.jpg" {
-		t.Fatalf("series = %+v, want client title/url/cover stored", sr)
+	if sr.Title != "Solo Leveling" || sr.SeriesURL != "https://asurascans.com/comics/solo" {
+		t.Fatalf("series = %+v, want client title/url stored", sr)
+	}
+	if sr.Cover != "" {
+		t.Fatalf("series cover = %q, want empty", sr.Cover)
 	}
 }

-// A PUT naming an existing series must not overwrite its title, cover or URL:
-// the row is shared, and those values are scraped page content (ADR-0003).
+// The hook is what starts creation-time acquisition, so it must fire exactly
+// once per Series — on the PUT that created it, and on no later one, whichever
+// Reader sends it.
+func TestOnSeriesCreatedFiresOnceForANewSeries(t *testing.T) {
+	store := newTestStore(t)
+	var created []Series
+	store.OnSeriesCreated = func(sr Series) { created = append(created, sr) }
+
+	b := Bookmark{
+		Key: "comix:solo", Site: "comix", SeriesID: "solo", Title: "Solo Leveling",
+		SeriesURL: "https://comix.to/series/solo", Kind: KindManga, UpdatedAt: 1000,
+	}
+	if _, err := store.Upsert(store.OwnerID(), b); err != nil {
+		t.Fatalf("Upsert: %v", err)
+	}
+	b.LastChapterNum = 12
+	b.UpdatedAt = 2000
+	if _, err := store.Upsert(store.OwnerID(), b); err != nil {
+		t.Fatalf("second Upsert: %v", err)
+	}
+	if _, err := store.Upsert(secondReader(t, store), b); err != nil {
+		t.Fatalf("second reader Upsert: %v", err)
+	}
+
+	if len(created) != 1 {
+		t.Fatalf("hook fired %d times, want 1: %+v", len(created), created)
+	}
+	if created[0].Site != "comix" || created[0].SeriesID != "solo" ||
+		created[0].SeriesURL != "https://comix.to/series/solo" {
+		t.Fatalf("hook got %+v, want the created series' identity and URL", created[0])
+	}
+}
+
+// Acquisition at creation and the poll both write covers, and whichever
+// arrives second must leave the first one alone: a Cover is replaced by
+// nothing short of the series row being rebuilt.
+func TestSetSeriesCoverDoesNotOverwrite(t *testing.T) {
+	store := newTestStore(t)
+	if _, err := store.Upsert(store.OwnerID(), Bookmark{
+		Key: "asura:solo", Site: "asura", SeriesID: "solo", UpdatedAt: 1000,
+	}); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+	first := "https://asurascans.com/covers/first.jpg"
+	if err := store.SetSeriesCover("asura", "solo", first, []byte("first"), "image/jpeg"); err != nil {
+		t.Fatalf("SetSeriesCover: %v", err)
+	}
+	if err := store.SetSeriesCover("asura", "solo", "https://asurascans.com/covers/second.jpg",
+		[]byte("second"), "image/jpeg"); err != nil {
+		t.Fatalf("second SetSeriesCover: %v", err)
+	}
+
+	got, ok, err := store.Get(store.OwnerID(), "asura:solo")
+	if err != nil || !ok {
+		t.Fatalf("Get = %v, %v", ok, err)
+	}
+	if want := "https://bookmarks.test/covers/" + CoverAddress(first); got.Cover != want {
+		t.Fatalf("Cover = %q, want the first one %q", got.Cover, want)
+	}
+}
+
+// The address comes straight off a public request path, so anything that is
+// not a stored address must be a miss rather than a filesystem lookup.
+func TestCoverByAddress(t *testing.T) {
+	store := newTestStore(t)
+	if _, err := store.Upsert(store.OwnerID(), Bookmark{
+		Key: "asura:solo", Site: "asura", SeriesID: "solo", UpdatedAt: 1000,
+	}); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+	source := "https://asurascans.com/covers/solo.jpg"
+	if err := store.SetSeriesCover("asura", "solo", source, []byte("bytes"), "image/jpeg"); err != nil {
+		t.Fatalf("SetSeriesCover: %v", err)
+	}
+
+	body, contentType, ok, err := store.CoverByAddress(CoverAddress(source))
+	if err != nil || !ok {
+		t.Fatalf("CoverByAddress = %v, %v", ok, err)
+	}
+	if string(body) != "bytes" || contentType != "image/jpeg" {
+		t.Fatalf("CoverByAddress = %q, %q, want the stored bytes", body, contentType)
+	}
+
+	for _, address := range []string{"", "../../etc/passwd", "ZZ" + CoverAddress(source)[2:],
+		CoverAddress("never stored")} {
+		_, _, ok, err := store.CoverByAddress(address)
+		if err != nil || ok {
+			t.Fatalf("CoverByAddress(%q) = %v, %v, want a clean miss", address, ok, err)
+		}
+	}
+}
+
+// A PUT naming an existing series must not overwrite its title or URL: the row
+// is shared, and those values are scraped page content (ADR-0003). An acquired
+// Cover is likewise untouched by any client.
 func TestUpsertExistingSeriesIgnoresClientTitleCoverURL(t *testing.T) {
 	store := newTestStore(t)
 	base := Bookmark{
 		Key: "asura:solo", Site: "asura", SeriesID: "solo",
 		Title: "Solo Leveling", SeriesURL: "https://asurascans.com/comics/solo",
-		Cover: "https://asurascans.com/covers/solo.jpg", LastChapterNum: 10,
-		UpdatedAt: 1000,
+		LastChapterNum: 10, UpdatedAt: 1000,
 	}
 	if _, err := store.Upsert(store.OwnerID(), base); err != nil {
 		t.Fatalf("seed: %v", err)
 	}
+	acquired := "https://asurascans.com/covers/solo.jpg"
+	if err := store.SetSeriesCover("asura", "solo", acquired, []byte("bytes"), "image/jpeg"); err != nil {
+		t.Fatalf("SetSeriesCover: %v", err)
+	}

 	// Same series, hostile/compromised values, real progress advance.
 	base.Title = "Scraped Rename"
@@ -799,8 +913,9 @@ func TestUpsertExistingSeriesIgnoresClientTitleCoverURL(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Upsert: %v", err)
 	}
+	wantCover := "https://bookmarks.test/covers/" + CoverAddress(acquired)
 	if got.Title != "Solo Leveling" || got.SeriesURL != "https://asurascans.com/comics/solo" ||
-		got.Cover != "https://asurascans.com/covers/solo.jpg" {
+		got.Cover != wantCover {
 		t.Fatalf("stored = %+v, want original title/url/cover kept", got)
 	}
 	if got.LastChapterNum != 11 {
@@ -836,16 +951,19 @@ func TestUpsertExistingSeriesAcceptsKindAndLatest(t *testing.T) {
 }

 // Deleting the last bookmark must leave the series row behind, so a later
-// re-bookmark shows title and cover immediately instead of waiting for a poll.
+// re-bookmark shows title and cover immediately instead of re-acquiring them.
 func TestDeleteKeepsSeriesRow(t *testing.T) {
 	store := newTestStore(t)
 	if _, err := store.Upsert(store.OwnerID(), Bookmark{
 		Key: "asura:solo", Site: "asura", SeriesID: "solo",
-		Title: "Solo Leveling", Cover: "https://asurascans.com/covers/solo.jpg",
-		UpdatedAt: 1000,
+		Title: "Solo Leveling", UpdatedAt: 1000,
 	}); err != nil {
 		t.Fatalf("seed: %v", err)
 	}
+	acquired := "https://asurascans.com/covers/solo.jpg"
+	if err := store.SetSeriesCover("asura", "solo", acquired, []byte("bytes"), "image/jpeg"); err != nil {
+		t.Fatalf("SetSeriesCover: %v", err)
+	}
 	if err := store.Delete(store.OwnerID(), "asura:solo"); err != nil {
 		t.Fatalf("Delete: %v", err)
 	}
@@ -863,7 +981,8 @@ func TestDeleteKeepsSeriesRow(t *testing.T) {
 	if err != nil {
 		t.Fatalf("re-upsert: %v", err)
 	}
-	if stored.Title != "Solo Leveling" || stored.Cover != "https://asurascans.com/covers/solo.jpg" {
+	wantCover := "https://bookmarks.test/covers/" + CoverAddress(acquired)
+	if stored.Title != "Solo Leveling" || stored.Cover != wantCover {
 		t.Fatalf("re-bookmark = %+v, want title/cover from the surviving series row", stored)
 	}
 }
@@ -940,14 +1059,14 @@ func TestDueForLatestCheckExcludesOrphanSeries(t *testing.T) {
 func TestSeedOwnerIdempotentAndRefreshesTokenHash(t *testing.T) {
 	url := pgtest.URL(t)
 	coverDir := t.TempDir()
-	first, err := Open(url, Owner{DiscordID: "owner", TokenHash: sha256.Sum256([]byte("hash-v1"))}, coverDir)
+	first, err := Open(url, Owner{DiscordID: "owner", TokenHash: sha256.Sum256([]byte("hash-v1"))}, coverDir, testCoverBaseURL)
 	if err != nil {
 		t.Fatalf("Open: %v", err)
 	}
 	ownerID := first.OwnerID()
 	first.Close()

-	second, err := Open(url, Owner{DiscordID: "owner", TokenHash: sha256.Sum256([]byte("hash-v2"))}, coverDir)
+	second, err := Open(url, Owner{DiscordID: "owner", TokenHash: sha256.Sum256([]byte("hash-v2"))}, coverDir, testCoverBaseURL)
 	if err != nil {
 		t.Fatalf("reopen: %v", err)
 	}
@@ -1004,7 +1123,7 @@ func TestMigration0004AttachesBookmarksToOwner(t *testing.T) {
 		t.Fatalf("migrate to 0002: %v", err)
 	}

-	st, err := Open(url, testOwner, t.TempDir())
+	st, err := Open(url, testOwner, t.TempDir(), testCoverBaseURL)
 	if err != nil {
 		t.Fatalf("Open: %v", err)
 	}
@@ -1281,7 +1400,7 @@ func TestTwoReadersShareOneSeriesWithIndependentProgress(t *testing.T) {
 func TestKaganeCoverPersistsAcrossReopen(t *testing.T) {
 	url := pgtest.URL(t)
 	coverDir := t.TempDir()
-	first, err := Open(url, testOwner, coverDir)
+	first, err := Open(url, testOwner, coverDir, testCoverBaseURL)
 	if err != nil {
 		t.Fatalf("Open: %v", err)
 	}
@@ -1293,7 +1412,7 @@ func TestKaganeCoverPersistsAcrossReopen(t *testing.T) {
 		t.Fatalf("close first store: %v", err)
 	}

-	second, err := Open(url, testOwner, coverDir)
+	second, err := Open(url, testOwner, coverDir, testCoverBaseURL)
 	if err != nil {
 		t.Fatalf("reopen: %v", err)
 	}
@@ -1308,15 +1427,26 @@ func TestKaganeCoverPersistsAcrossReopen(t *testing.T) {
 }

 func TestOpenRequiresCoverDirectory(t *testing.T) {
-	if _, err := Open(pgtest.URL(t), testOwner, ""); err == nil || !strings.Contains(err.Error(), "cover directory is required") {
+	if _, err := Open(pgtest.URL(t), testOwner, "", testCoverBaseURL); err == nil || !strings.Contains(err.Error(), "cover directory is required") {
 		t.Fatalf("Open without cover directory = %v, want required-directory error", err)
 	}
 }

+// A base URL without a scheme reads like a hostname and starts cleanly, but
+// every Cover it puts on the wire is an address no browser can resolve.
+func TestOpenRequiresAbsoluteCoverBaseURL(t *testing.T) {
+	for _, base := range []string{"", "bookmarks.test", "https://", "ftp://bookmarks.test"} {
+		if _, err := Open(pgtest.URL(t), testOwner, t.TempDir(), base); err == nil ||
+			!strings.Contains(err.Error(), "absolute http(s) origin") {
+			t.Fatalf("Open with base %q = %v, want absolute-origin error", base, err)
+		}
+	}
+}
+
 func TestKaganeCoverIsContentAddressedOnFilesystem(t *testing.T) {
 	url := pgtest.URL(t)
 	coverDir := t.TempDir()
-	first, err := Open(url, testOwner, coverDir)
+	first, err := Open(url, testOwner, coverDir, testCoverBaseURL)
 	if err != nil {
 		t.Fatalf("Open: %v", err)
 	}