A newly bookmarked Series acquires its Cover at creation (#59) (#68)

Closes #59. Part of spec #55, and the ticket that fixes the reported bug #47. Architecture: `docs/adr/0007-backend-hosts-cover-bytes.md`. Does not close #47 or #55. ## What changed A Reader bookmarks a Series nobody holds yet — the exact case in #47 — and within seconds the list shows its artwork instead of a broken image. The first Bookmark to create a Series fires `Store.OnSeriesCreated` after commit, and the new `latest.Acquirer` turns that into **one** series-page fetch that yields both the Latest Chapter and the cover URL. The bytes go through the gated cover fetcher from #57 and are stored content-addressed through #56, so the wire carries an absolute URL on this deployment's own origin — never a third-party address, and never one that 404s. ### Store - Migration `0009_series_cover_address.sql` adds `series.cover_address`. The two facts are now split: `series.cover` is the third-party source address the bytes came from (the acquisition path's dedupe key), `series.cover_address` is the SHA-256 they are stored under. An empty `cover_address` is precisely what "no Cover yet" means, which is the distinction both the API and the UI depend on. - `SetSeriesCover` writes the address only after the bytes are on disk, so the wire can never name an object that is not there. - `CoverWireURL` builds `PUBLIC_BASE_URL + /covers/<sha256>` for every scanned row, and returns `""` for a blank address. - The cover columns are gone from `Upsert`'s `INSERT` and its `DO UPDATE`. A client-supplied cover cannot reach the shared Series row on any path, not just the creation path. - `Open` now rejects a base URL that is not an absolute `http(s)` origin: `PUBLIC_BASE_URL=bookmarks.example.com` would otherwise start cleanly and emit addresses no browser can load. ### Acquisition - `internal/latest/acquire.go`: one fetch, gated by the poller's own `fetchableSeriesURL` (a `series_url` arrives in a client-supplied PUT body, so without the gate a token-holder chooses what the server fetches from its own network position). - Asynchronous and log-and-drop. The Bookmark, its progress and its Latest Chapter are already committed; a Site that is down or a cover that cannot be produced disturbs none of them. - Bounded by a two-slot semaphore. A bulk sync creating N Series would otherwise fire N simultaneous requests from one IP — the traffic shape the poller's stagger exists to avoid. - Cancelled at shutdown (shares the poller's context) and stamps `latest_checked_at`, so the poller does not refetch the same page a tick later. - Browser-backed Sites (kagane, novelfull) are deliberately skipped: their pages only yield a Cloudflare challenge to the TLS client, so the request would be spent for nothing. They arrive in #62. ### Wire and route - `GET /covers/{address}` serves the bytes publicly and uncredentialed with `Cache-Control: public, max-age=604800, immutable`. The address is gated by a `^[0-9a-f]{64}$` pattern and cross-checked against a pure function of itself before any filesystem read, so no request shaped like a traversal reaches disk. - `PUT /bookmarks/{key}` still accepts a `cover` field and discards it, permanently. Rejecting it would break every installed userscript the moment this deploys, and ADR-0004's compatibility argument depends on those scripts continuing to work. The decode site says so in place of a TODO nobody intends to keep. - `store.CoverContentType` canonicalises comix's non-standard `image/jpg` to `image/jpeg`, so one image cannot land under two spellings. This one was found by the live smoke test, not by reading. ### Config `PUBLIC_BASE_URL` is new and required (cover URLs must go out absolute — the userscript renders them on third-party origins, where a relative path resolves against the Site). Documented in `.env.example`, `docker-compose.yml` (`:?` so compose fails too), `DEPLOY.md` and `backend/AGENTS.md`. ## Acceptance criteria All twelve of #59's criteria are met; the checklist on the issue is ticked with the evidence. ## Verification - `go test ./...` green (Docker-backed Postgres suite). - Live smoke against a real backend + Postgres: bookmarking `comix:n8we-dungeons-and-crayons` produced `"cover": "http://127.0.0.1:8099/covers/8ce74d80…"` and `"latest_chapter": "Chapter 81"` within seconds of the PUT; `curl` on that address returned `200`, `Content-Type: image/jpeg`, `Cache-Control: public, max-age=604800, immutable`, and a 280x420 JPEG. That run is what surfaced the `image/jpg` content type. - Mutation-checked the asynchrony test: removing the `go` from `Acquire` turns `TestAcquireDoesNotBlockTheWrite` red. ## Reviewed Both axes of `/code-review` were run against this diff before commit. Their findings that were actionable here are folded in: the concurrency bound, the shutdown tie, the `PUBLIC_BASE_URL` validation, the missing `latest_checked_at` stamp, and a test that could not fail. ## Known sequencing A kagane/novelfull Series created between this deploy and #62 has no cover source at all: the acquisition skips those Sites and `Upsert` no longer persists the userscript-scraped address. This is #59's stated boundary rather than a defect, but it is a user-visible gap on two Sites and should order #62 accordingly. Reviewed-on: #68 Co-authored-by: Sulthan Zaki <sultankiki05@gmail.com> Co-committed-by: Sulthan Zaki <sultankiki05@gmail.com>
2026-08-10 04:07:53 +07:00
parent b6b88bde8a
commit 92eba07da7
18 changed files with 971 additions and 115 deletions
@@ -0,0 +1,239 @@
+package latest
+
+import (
+	"context"
+	"errors"
+	"testing"
+	"time"
+
+	"bookmarkmanager/backend/internal/store"
+)
+
+// The series page carries both facts, which is the whole argument for taking
+// them from one fetch.
+const asuraSeriesAndCoverFixture = asuraSeriesFixture + asuraCoverFixture
+
+const (
+	acquireKey       = "asura:chronicles-of-the-demon-faction-f886a8af"
+	acquireSeriesID  = "chronicles-of-the-demon-faction-f886a8af"
+	acquireSeriesURL = "https://asurascans.com/comics/chronicles-of-the-demon-faction-f886a8af"
+	acquireCoverURL  = "https://cdn.asurascans.com/asura-images/covers/chronicles-of-the-demon-faction.d4dcb8.webp"
+)
+
+// newAcquirer wires an acquirer onto the store's creation hook, which is how
+// main wires it: the write path is what starts an acquisition.
+func newAcquirer(s *store.Store, page *fakeFetcher, covers *fakeBytesCoverFetcher) *Acquirer {
+	a := &Acquirer{Store: s, Fetch: page, Covers: covers}
+	s.OnSeriesCreated = a.Acquire
+	return a
+}
+
+func bookmarkNewSeries(t *testing.T, s *store.Store, seriesURL string) store.Bookmark {
+	t.Helper()
+	stored, err := s.Upsert(s.OwnerID(), store.Bookmark{
+		Key: acquireKey, Site: "asura", SeriesID: acquireSeriesID,
+		Title: "Chronicles of the Demon Faction", SeriesURL: seriesURL,
+		Cover: "https://evil.example/client-supplied.jpg", UpdatedAt: 1000,
+	})
+	if err != nil {
+		t.Fatalf("Upsert: %v", err)
+	}
+	return stored
+}
+
+func readBookmark(t *testing.T, s *store.Store, key string) store.Bookmark {
+	t.Helper()
+	b, ok, err := s.Get(s.OwnerID(), key)
+	if err != nil || !ok {
+		t.Fatalf("Get %q = %v, %v", key, ok, err)
+	}
+	return b
+}
+
+// The reported bug: a Reader bookmarks a Series nobody holds and expects the
+// Cover, not a broken image. Both facts come from the one series-page fetch.
+func TestAcquireFillsChapterAndCoverFromOneFetch(t *testing.T) {
+	s, _ := newTestStore(t)
+	page := &fakeFetcher{body: asuraSeriesAndCoverFixture, status: 200}
+	covers := &fakeBytesCoverFetcher{body: []byte("cover-bytes"), contentType: "image/jpeg"}
+	acq := newAcquirer(s, page, covers)
+
+	// The write itself must not carry the acquisition: it returns before the
+	// Cover exists, and the field is empty until the bytes land.
+	stored := bookmarkNewSeries(t, s, acquireSeriesURL)
+	if stored.Cover != "" {
+		t.Fatalf("Cover on the creating write = %q, want empty", stored.Cover)
+	}
+	acq.Wait()
+
+	if got := page.callCount(); got != 1 {
+		t.Fatalf("series page fetches = %d, want exactly 1", got)
+	}
+	if got := covers.callCount(); got != 1 {
+		t.Fatalf("cover fetches = %d, want 1", got)
+	}
+	got := readBookmark(t, s, acquireKey)
+	if got.LatestChapterNum == nil || *got.LatestChapterNum != 181 {
+		t.Fatalf("LatestChapterNum = %v, want 181", got.LatestChapterNum)
+	}
+	if want := testCoverBaseURL + "/covers/" + store.CoverAddress(acquireCoverURL); got.Cover != want {
+		t.Fatalf("Cover = %q, want the absolute address %q", got.Cover, want)
+	}
+	body, contentType, ok, err := s.CoverByAddress(store.CoverAddress(acquireCoverURL))
+	if err != nil || !ok {
+		t.Fatalf("CoverByAddress = %v, %v", ok, err)
+	}
+	if string(body) != "cover-bytes" || contentType != "image/jpeg" {
+		t.Fatalf("stored cover = (%q, %q), want the fetched bytes", body, contentType)
+	}
+}
+
+// A Series that already exists is not re-acquired: no fetch, and the Cover it
+// already has is left alone.
+func TestAcquireSkipsAnExistingSeries(t *testing.T) {
+	s, _ := newTestStore(t)
+	page := &fakeFetcher{body: asuraSeriesAndCoverFixture, status: 200}
+	covers := &fakeBytesCoverFetcher{body: []byte("cover-bytes"), contentType: "image/jpeg"}
+	acq := newAcquirer(s, page, covers)
+
+	bookmarkNewSeries(t, s, acquireSeriesURL)
+	acq.Wait()
+	bookmarkNewSeries(t, s, acquireSeriesURL)
+	acq.Wait()
+
+	if got := page.callCount(); got != 1 {
+		t.Fatalf("series page fetches = %d, want 1 — an existing series is not re-acquired", got)
+	}
+	if got := covers.callCount(); got != 1 {
+		t.Fatalf("cover fetches = %d, want 1", got)
+	}
+	got := readBookmark(t, s, acquireKey)
+	if want := testCoverBaseURL + "/covers/" + store.CoverAddress(acquireCoverURL); got.Cover != want {
+		t.Fatalf("Cover = %q, want the acquired one %q", got.Cover, want)
+	}
+}
+
+// A Site that is down costs the Cover and nothing else.
+func TestAcquireFailureLeavesTheBookmarkIntact(t *testing.T) {
+	cases := []struct {
+		name   string
+		page   *fakeFetcher
+		covers *fakeBytesCoverFetcher
+		// wantLatest is the chapter that still lands; 0 means none did.
+		wantLatest float64
+	}{
+		{
+			"the series page is unreachable",
+			&fakeFetcher{err: errors.New("connection reset")},
+			&fakeBytesCoverFetcher{body: []byte("cover-bytes"), contentType: "image/jpeg"},
+			0,
+		},
+		{
+			"the series page answers with a challenge",
+			&fakeFetcher{body: challengeFixture, status: 200},
+			&fakeBytesCoverFetcher{body: []byte("cover-bytes"), contentType: "image/jpeg"},
+			0,
+		},
+		{
+			"only the cover bytes fail",
+			&fakeFetcher{body: asuraSeriesAndCoverFixture, status: 200},
+			&fakeBytesCoverFetcher{err: errors.New("403")},
+			181,
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			s, _ := newTestStore(t)
+			acq := newAcquirer(s, tc.page, tc.covers)
+
+			stored := bookmarkNewSeries(t, s, acquireSeriesURL)
+			acq.Wait()
+
+			got := readBookmark(t, s, acquireKey)
+			if got.Cover != "" {
+				t.Fatalf("Cover = %q, want empty rather than an address that 404s", got.Cover)
+			}
+			if got.Title != stored.Title || got.UpdatedAt != stored.UpdatedAt {
+				t.Fatalf("bookmark = %+v, want it untouched by the failed acquisition", got)
+			}
+			if tc.wantLatest == 0 {
+				if got.LatestChapterNum != nil {
+					t.Fatalf("LatestChapterNum = %v, want none captured", *got.LatestChapterNum)
+				}
+				return
+			}
+			if got.LatestChapterNum == nil || *got.LatestChapterNum != tc.wantLatest {
+				t.Fatalf("LatestChapterNum = %v, want %v", got.LatestChapterNum, tc.wantLatest)
+			}
+		})
+	}
+}
+
+// series_url arrives in a client-supplied body, so the acquisition reuses the
+// poller's gate rather than deriving a second one: a non-https scheme, a
+// site the parsers do not know, or a host pinned to another site is refused
+// before the server spends a request from its own network position.
+func TestAcquireRefusesAnUnfetchableSeriesURL(t *testing.T) {
+	for _, seriesURL := range []string{
+		"http://asurascans.com/comics/x",
+		"file:///etc/passwd",
+		"",
+	} {
+		t.Run(seriesURL, func(t *testing.T) {
+			s, _ := newTestStore(t)
+			page := &fakeFetcher{body: asuraSeriesAndCoverFixture, status: 200}
+			acq := newAcquirer(s, page, &fakeBytesCoverFetcher{})
+
+			bookmarkNewSeries(t, s, seriesURL)
+			acq.Wait()
+
+			if got := page.callCount(); got != 0 {
+				t.Fatalf("fetches for %q = %d, want 0", seriesURL, got)
+			}
+		})
+	}
+}
+
+// blockingFetcher stands in for a Site that never answers, so a synchronous
+// acquisition would be visible as a stalled write rather than a slow one.
+type blockingFetcher struct {
+	release <-chan struct{}
+	body    string
+}
+
+func (f *blockingFetcher) Get(ctx context.Context, _ string) (string, int, error) {
+	select {
+	case <-f.release:
+		return f.body, 200, nil
+	case <-ctx.Done():
+		return "", 0, ctx.Err()
+	}
+}
+
+// The Reader's write may not wait on a third-party Site: with the acquisition
+// wedged on an unanswering page, the PUT still returns.
+func TestAcquireDoesNotBlockTheWrite(t *testing.T) {
+	s, _ := newTestStore(t)
+	release := make(chan struct{})
+	acq := &Acquirer{Store: s, Fetch: &blockingFetcher{release: release, body: asuraSeriesAndCoverFixture}}
+	s.OnSeriesCreated = acq.Acquire
+
+	upserted := make(chan error, 1)
+	go func() {
+		_, err := s.Upsert(s.OwnerID(), store.Bookmark{
+			Key: acquireKey, Site: "asura", SeriesID: acquireSeriesID,
+			Title: "Chronicles of the Demon Faction", SeriesURL: acquireSeriesURL, UpdatedAt: 1000,
+		})
+		upserted <- err
+	}()
+	select {
+	case err := <-upserted:
+		if err != nil {
+			t.Fatalf("Upsert: %v", err)
+		}
+	case <-time.After(10 * time.Second):
+		t.Fatal("the creating write blocked on the acquisition")
+	}
+	close(release)
+	acq.Wait()
+}