Spec #136: Finished belongs to the Series — owner-owned poll gate, Lifecycle bucket dropped (#163)

Closes #136. Spec #136 end to end: `finished` becomes a fact about the Series, written only by the owner, and the reader-facing Lifecycle bucket is gone. ## What landed - **#157** — `series.finished_at bigint NOT NULL DEFAULT 0` plus the migration whose statement order is load-bearing (seed from the buckets, then flip them); both Lane queries lose the `HAVING COUNT(*) FILTER (WHERE b.status <> 'finished')` clause and gate on `finished_at = 0` instead, with the due-query/eligible-count force asymmetry kept deliberate and commented; `StatusFinished`, its API special-case 400, the web tab and the templates' Finished bucket deleted. - **#158** — owner Finish control on the Series detail page: confirm-gated finish, instant un-finish, admin accent (never ember, nothing is destroyed), `Store.SetSeriesFinished`, the two routes behind the owner gate, and the state displayed on the list row without offering the control there. - **#160** — reader side: derived `finished` bool on the flat Bookmark (`s.finished_at > 0`), rendered as a text-only label in both userscripts and on the web card; read-only inbound by omission from `Upsert`'s explicit `series` column list, same mechanism that already protects `cover`. - **#161** — glossary and the stale Reader-count divergence note catch up. - **#159** — `finished` joins the admin filter vocabulary (predicate `finished_at > 0`, label `Finished`, own aggregate count, figure last in the stats block as informational); the four clock-driven hygiene predicates (stale, never-checked, no-cover, no-chapter) exclude finished Series while unpollable, orphan and sighting-raised deliberately do not. ## Verification `go vet ./...` and `go test ./...` green on the merged branch (Docker-backed, throwaway `postgres:17-alpine` per package). Each ticket also passed a two-axis review (spec + standards) on its own branch before merge. Reviewed-on: #163 Co-authored-by: Sulthan Zaki <sultankiki05@gmail.com> Co-committed-by: Sulthan Zaki <sultankiki05@gmail.com>
2026-08-22 17:31:52 +07:00
parent faa80c41ea
commit 8e4fa6448e
34 changed files with 1225 additions and 241 deletions
@@ -279,6 +279,27 @@ func seedForCheck(t *testing.T, s *Store, key, seriesURL string, checkedAt int64
 	}
 }

+// seedFinished inserts a bookmark (and with it its series) and stamps the
+// series finished — the series-level fact the due gate reads (issue #157).
+func seedFinished(t *testing.T, s *Store, key, seriesURL string) {
+	t.Helper()
+	site, seriesID, ok := strings.Cut(key, ":")
+	if !ok {
+		t.Fatalf("key %q: no ':' separator", key)
+	}
+	if _, err := s.Upsert(s.OwnerID(), Bookmark{
+		Key: key, Site: site, SeriesID: seriesID, SeriesURL: seriesURL,
+		UpdatedAt: 1000,
+	}); err != nil {
+		t.Fatalf("seed %q: %v", key, err)
+	}
+	if _, err := s.db.Exec(
+		`UPDATE series SET finished_at = 1000 WHERE site = $1 AND series_id = $2`,
+		site, seriesID); err != nil {
+		t.Fatalf("seed finish %q: %v", key, err)
+	}
+}
+
 // noCeiling is a Sighting deferral ceiling no Series can reach, for the tests
 // that predate the ceiling and are about rest, ordering or buckets instead.
 const noCeiling = int64(-1)
@@ -486,24 +507,15 @@ func TestUpsertStatusChangeKeepsUpdatedAt(t *testing.T) {
 		t.Fatalf("UpdatedAt = %d, want it frozen at %d", stored.UpdatedAt, first.UpdatedAt)
 	}
 }
-
 // Archiving is the reason to keep polling — the point is to come back to a
-// series that has moved on. A finished series has nothing left to publish.
+// series that has moved on. A finished series has nothing left to publish,
+// whichever Reader marked it (issue #157).
 func TestDueForLatestCheckSkipsFinishedKeepsArchived(t *testing.T) {
 	store := newTestStore(t)
-	for _, tc := range []struct{ key, status string }{
-		{"asura:reading", StatusReading},
-		{"asura:archived", StatusArchived},
-		{"asura:finished", StatusFinished},
-	} {
-		if _, err := store.Upsert(store.OwnerID(), Bookmark{
-			Key: tc.key, Site: "asura", SeriesID: strings.TrimPrefix(tc.key, "asura:"),
-			SeriesURL: "https://asurascans.com/comics/" + tc.key,
-			Status:    tc.status, UpdatedAt: time.Now().UnixMilli(),
-		}); err != nil {
-			t.Fatalf("seed %s: %v", tc.key, err)
-		}
+	for _, key := range []string{"asura:reading", "asura:archived"} {
+		seedForCheck(t, store, key, "https://asurascans.com/comics/"+key, 0)
 	}
+	seedFinished(t, store, "asura:finished", "https://asurascans.com/comics/asura:finished")

 	due, err := store.DueForLatestCheck("asura", time.Now().UnixMilli(), noCeiling)
 	if err != nil {
@@ -522,27 +534,26 @@ func TestDueForLatestCheckSkipsFinishedKeepsArchived(t *testing.T) {
 }

 // The gap's denominator counts every Series the Lane will ever Poll: a
-// finished Series must not make the Lane faster than it needs to be, and
-// another Site's Series must not leak into this Site's count.
+// finished Series must not make the Lane faster than it needs to be, another
+// Site's Series must not leak into this Site's count, and a forced Series is
+// not admitted either — it is due once, not a reason to tighten the pace for
+// every other fetch on the Site (issue #157).
 func TestEligibleSeriesCount(t *testing.T) {
 	store := newTestStore(t)
 	seedForCheck(t, store, "asura:reading", "https://asurascans.com/comics/reading", 0)
 	seedForCheck(t, store, "asura:archived", "https://asurascans.com/comics/archived", 0)
-	if _, err := store.Upsert(store.OwnerID(), Bookmark{
-		Key: "asura:finished", Site: "asura", SeriesID: "finished",
-		SeriesURL: "https://asurascans.com/comics/finished",
-		Status:    StatusFinished, UpdatedAt: 1000,
-	}); err != nil {
-		t.Fatalf("seed finished: %v", err)
-	}
+	seedFinished(t, store, "asura:finished", "https://asurascans.com/comics/asura:finished")
 	seedForCheck(t, store, "demonic:z", "https://demonicscans.org/manga/z", 0)
+	if err := store.ForceSeriesPoll("asura", "archived", 5000); err != nil {
+		t.Fatalf("ForceSeriesPoll: %v", err)
+	}

 	n, err := store.EligibleSeriesCount("asura")
 	if err != nil {
 		t.Fatalf("EligibleSeriesCount: %v", err)
 	}
 	if n != 2 {
-		t.Fatalf("eligible = %d, want 2 (finished excluded, demonic excluded)", n)
+		t.Fatalf("eligible = %d, want 2 (finished and forced excluded, demonic excluded)", n)
 	}
 	n, err = store.EligibleSeriesCount("demonic")
 	if err != nil {
@@ -755,6 +766,133 @@ func TestMigration0008DropsLegacyCoverRows(t *testing.T) {
 	}
 }

+// 0016's three statements are order-dependent: the finished_at seed must see
+// the pre-flip 'finished' buckets, and the flip must come after it. A seed
+// that ran after the flip would read bookmarks that no longer say
+// 'finished', declare nothing finished, and silently resume polling series
+// somebody marked done. This seeds a pre-0016 database the way every
+// deployment looked — finished a bookmark bucket, no finished_at column —
+// runs the migration, and asserts on what it preserved.
+func TestMigration0016SeedsFinishedAtBeforeFlippingBucket(t *testing.T) {
+	url := pgtest.URL(t)
+	db, err := sql.Open("pgx", url)
+	if err != nil {
+		t.Fatalf("open: %v", err)
+	}
+	defer db.Close()
+
+	if err := migrate(db, 15); err != nil {
+		t.Fatalf("migrate to 0015: %v", err)
+	}
+	if err := seedOwner(db, testOwner); err != nil {
+		t.Fatalf("seed owner: %v", err)
+	}
+	// A second reader, so "mixed" can carry two bookmarks on one series.
+	if _, err := db.Exec(
+		`INSERT INTO readers (discord_id, token_sha256) VALUES ('second', '\x01'::bytea)`); err != nil {
+		t.Fatalf("seed second reader: %v", err)
+	}
+
+	owner := func() int64 {
+		t.Helper()
+		var id int64
+		if err := db.QueryRow(
+			`SELECT id FROM readers WHERE discord_id = $1`, testOwner.DiscordID).Scan(&id); err != nil {
+			t.Fatalf("owner id: %v", err)
+		}
+		return id
+	}()
+	seedBookmark := func(readerID int64, site, seriesID, status string) {
+		t.Helper()
+		// The bookmark FK demands the series row; Upsert would create it on
+		// the fly, raw SQL has to spell it out.
+		if _, err := db.Exec(
+			`INSERT INTO series (site, series_id) VALUES ($1, $2) ON CONFLICT DO NOTHING`,
+			site, seriesID); err != nil {
+			t.Fatalf("seed series %s:%s: %v", site, seriesID, err)
+		}
+		if _, err := db.Exec(
+			`INSERT INTO bookmarks (reader_id, site, series_id, status, updated_at)
+			 VALUES ($1, $2, $3, $4, 1000)`, readerID, site, seriesID, status); err != nil {
+			t.Fatalf("seed %s:%s/%s: %v", site, seriesID, status, err)
+		}
+	}
+	// done: the one-bookmark series every finished series looked like.
+	seedBookmark(owner, "asura", "done", "finished")
+	// shared: every reader finished — the multi-reader equivalent.
+	seedBookmark(owner, "asura", "shared", "finished")
+	// mixed: a second reader still reading keeps the series alive.
+	seedBookmark(owner, "asura", "mixed", "finished")
+	seedBookmark(owner+1, "asura", "mixed", "archived")
+	// live: nobody finished it.
+	seedBookmark(owner, "asura", "live", "reading")
+	// orphan: no bookmarks at all, nothing to decide from.
+	if _, err := db.Exec(
+		`INSERT INTO series (site, series_id) VALUES ('asura', 'orphan')`); err != nil {
+		t.Fatalf("seed orphan: %v", err)
+	}
+
+	if err := migrate(db, 0); err != nil {
+		t.Fatalf("migrate 0016: %v", err)
+	}
+
+	finishedAt := func(site, seriesID string) int64 {
+		t.Helper()
+		var at int64
+		if err := db.QueryRow(
+			`SELECT finished_at FROM series WHERE site = $1 AND series_id = $2`,
+			site, seriesID).Scan(&at); err != nil {
+			t.Fatalf("finished_at %s:%s: %v", site, seriesID, err)
+		}
+		return at
+	}
+	status := func(readerID int64, site, seriesID string) string {
+		t.Helper()
+		var s string
+		if err := db.QueryRow(
+			`SELECT status FROM bookmarks WHERE reader_id = $1
+			   AND site = $2 AND series_id = $3`, readerID, site, seriesID).Scan(&s); err != nil {
+			t.Fatalf("status %s:%s: %v", site, seriesID, err)
+		}
+		return s
+	}
+
+	// The seed ran before the flip: had the flip gone first, done's bookmarks
+	// would read archived and nothing would be stamped. The stamp is a real
+	// timestamp, not a sentinel.
+	before := time.Now().Add(-time.Minute).UnixMilli()
+	after := time.Now().Add(time.Minute).UnixMilli()
+	for _, sr := range []struct{ site, seriesID string }{
+		{"asura", "done"}, {"asura", "shared"},
+	} {
+		if at := finishedAt(sr.site, sr.seriesID); at < before || at > after {
+			t.Fatalf("%s:%s finished_at = %d, want now-ish", sr.site, sr.seriesID, at)
+		}
+		// The flip followed the seed: every finished bookmark is archived.
+		if s := status(owner, sr.site, sr.seriesID); s != StatusArchived {
+			t.Fatalf("%s:%s status = %q, want archived after the flip", sr.site, sr.seriesID, s)
+		}
+	}
+	// The seed ignores a series any bookmark keeps alive.
+	if at := finishedAt("asura", "mixed"); at != 0 {
+		t.Fatalf("mixed finished_at = %d, want 0", at)
+	}
+	// The flip is a bucket rewrite, not a series-wide one: the finished
+	// bookmark becomes archived and the sibling rows keep their buckets.
+	if s := status(owner, "asura", "mixed"); s != StatusArchived {
+		t.Fatalf("mixed finished bookmark = %q, want archived", s)
+	}
+	if s := status(owner+1, "asura", "mixed"); s != StatusArchived {
+		t.Fatalf("mixed archived bookmark = %q, want untouched archived", s)
+	}
+	if s := status(owner, "asura", "live"); s != StatusReading {
+		t.Fatalf("live status = %q, want untouched reading", s)
+	}
+	if at := finishedAt("asura", "orphan"); at != 0 {
+		t.Fatalf("orphan finished_at = %d, want 0", at)
+	}
+}
+
 // readSeries reads the series row directly, for asserting on what Upsert
 // actually stored rather than what the joined Bookmark reports.
 func readSeries(t *testing.T, s *Store, site, seriesID string) Series {
@@ -796,6 +934,37 @@ func TestUpsertCreatesSeriesFromClient(t *testing.T) {
 	}
 }

+// The wire's finished flag is derived, never stored: the Upsert's explicit
+// column list does not name finished_at — the same omission that protects
+// cover — so a client echoing a cached flag, true or false, cannot change the
+// Series' retired state (issues #157, #160).
+func TestUpsertCannotWriteSeriesFinished(t *testing.T) {
+	store := newTestStore(t)
+	seedFinished(t, store, "asura:done", "https://asurascans.com/comics/asura:done")
+
+	for _, sent := range []bool{true, false} {
+		stored, err := store.Upsert(store.OwnerID(), Bookmark{
+			Key: "asura:done", Site: "asura", SeriesID: "done",
+			Finished: sent, UpdatedAt: 2000,
+		})
+		if err != nil {
+			t.Fatalf("Upsert(Finished: %v): %v", sent, err)
+		}
+		if !stored.Finished {
+			t.Fatalf("stored.Finished = false after echoing %v, want true", sent)
+		}
+	}
+	var finishedAt int64
+	if err := store.db.QueryRow(
+		`SELECT finished_at FROM series WHERE site = $1 AND series_id = $2`,
+		"asura", "done").Scan(&finishedAt); err != nil {
+		t.Fatalf("read finished_at: %v", err)
+	}
+	if finishedAt != 1000 {
+		t.Fatalf("series.finished_at = %v, want the seeded 1000 untouched", finishedAt)
+	}
+}
+
 // 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.
@@ -1809,19 +1978,12 @@ func TestDueForLatestCheckForcedOverridesSightingDeferral(t *testing.T) {
 	}
 }

-// The finished-only bucket excludes a series whose only bookmarks are
-// finished; a forced request overrides it — the owner asked, so the Lane
-// looks.
-func TestDueForLatestCheckForcedOverridesFinishedBucket(t *testing.T) {
+// The finished flag excludes a series; a forced request overrides it — the
+// owner asked, so the Lane looks.
+func TestDueForLatestCheckForcedOverridesFinishedFlag(t *testing.T) {
 	s := newTestStore(t)
 	seedForCheck(t, s, "asura:reading", "https://asurascans.com/comics/reading", 0)
-	if _, err := s.Upsert(s.OwnerID(), Bookmark{
-		Key: "asura:finished", Site: "asura", SeriesID: "finished",
-		SeriesURL: "https://asurascans.com/comics/finished",
-		Status:    StatusFinished, UpdatedAt: 1000,
-	}); err != nil {
-		t.Fatalf("seed finished: %v", err)
-	}
+	seedFinished(t, s, "asura:finished", "https://asurascans.com/comics/asura:finished")

 	due, err := s.DueForLatestCheck("asura", 1000, noCeiling)
 	if err != nil {