From bc64a1d894819273c5f4e6de2db8fe7b4a76e8a2 Mon Sep 17 00:00:00 2001 From: Sulthan Zaki Date: Sat, 22 Aug 2026 09:55:01 +0700 Subject: [PATCH] backend: remove orphan series from the admin (ticket #155) (*Store).RemoveSeries deletes one series row by (site, series_id) via a plain parameterized DELETE; a bookmarks_series_fk violation outside 23503 is translated into the ErrSeriesHasBookmarks sentinel so no driver type escapes the store. The caller reads the row's cover address before the delete and reclaims it after: ReclaimCover's guard cannot pass while a series row still points at the address. POST /admin/series/{key}/remove answers the list row with the removed row's fragment plus the heading re-rendered out of band at the fresh count (HX-Reswap: delete removes the row through the same button that swaps the refusal back in), and navigates from the detail page to the No-Readers list (HX-Redirect for htmx, a 303 for plain clients). A removal that races a fresh Bookmark is a refusal, not a 500: the row re-renders at its new count with the fact spelled out. The control renders only at zero Reader count on both surfaces, gated by hx-confirm with the brief's copy. --- backend/internal/store/store.go | 32 +++ backend/internal/store/store_test.go | 109 ++++++++ backend/internal/web/admin.go | 1 + backend/internal/web/admin_series.go | 169 +++++++++++- backend/internal/web/admin_series_detail.go | 5 + backend/internal/web/static/admin.css | 7 + .../internal/web/templates/series-detail.html | 5 + .../internal/web/templates/series-list.html | 16 +- backend/web_test.go | 248 +++++++++++++++++- 9 files changed, 580 insertions(+), 12 deletions(-) diff --git a/backend/internal/store/store.go b/backend/internal/store/store.go index b403f6d..ba57a0c 100644 --- a/backend/internal/store/store.go +++ b/backend/internal/store/store.go @@ -16,6 +16,7 @@ import ( "strconv" "strings" + "github.com/jackc/pgx/v5/pgconn" _ "github.com/jackc/pgx/v5/stdlib" ) @@ -1082,6 +1083,37 @@ func (s *Store) Delete(readerID int64, key string) error { return nil } +// pgForeignKeyViolation is the SQLSTATE the driver surfaces when a Bookmark +// row refuses a Series delete (bookmarks_series_fk). pgconn exports no named +// constant for it, so the store names it here. +const pgForeignKeyViolation = "23503" + +// ErrSeriesHasBookmarks is RemoveSeries' refusal: a Reader still holds the +// Series, so the owner's removal must not reach past that record. The +// delete is the check — no NOT EXISTS pre-check that can race the insert — +// and the driver's foreign-key violation is translated here so no driver +// type escapes the store (issue #155). +var ErrSeriesHasBookmarks = errors.New("series has bookmarks") + +// RemoveSeries deletes one Series row by (site, series_id). It is refused +// while any Bookmark references the row; deleting an absent key is not an +// error, matching Delete. The caller owns the stranded Cover: read the row's +// cover_address before the delete and call ReclaimCover after it — the +// helper's guard cannot pass while the series row still points at the +// address, so the order is the sequence, not a preference. +func (s *Store) RemoveSeries(site, seriesID string) error { + if _, err := s.db.Exec( + `DELETE FROM series WHERE site = $1 AND series_id = $2`, + site, seriesID); err != nil { + var pgErr *pgconn.PgError + if errors.As(err, &pgErr) && pgErr.Code == pgForeignKeyViolation { + return ErrSeriesHasBookmarks + } + return fmt.Errorf("remove series %s:%s: %w", site, seriesID, err) + } + return nil +} + // RecordLanePass appends one pass and prunes every older row in the same // transaction. retainBefore is supplied by the poller's clock. func (s *Store) RecordLanePass(p LanePass, retainBefore int64) error { diff --git a/backend/internal/store/store_test.go b/backend/internal/store/store_test.go index db286b9..4cb05c5 100644 --- a/backend/internal/store/store_test.go +++ b/backend/internal/store/store_test.go @@ -2340,3 +2340,112 @@ func TestReclaimCoverFailedUnlinkKeepsRow(t *testing.T) { t.Fatal("covers row after failed unlink is gone; want it left for a retry") } } + +// RemoveSeries is the orphan removal (#155): one Series, one delete, refused +// by the database while any Bookmark points at it. The store translates the +// foreign-key violation into its own sentinel so no driver type escapes, and +// the caller reaps the stranded Cover through ReclaimCover. +func TestRemoveSeriesRemovesOrphanAndReclaimsCover(t *testing.T) { + store := newTestStore(t) + seedForCheck(t, store, "asura:solo", "https://asurascans.com/comics/solo", 0) + if err := store.Delete(store.OwnerID(), "asura:solo"); err != nil { + t.Fatalf("orphan the series: %v", err) + } + 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) + } + addr := CoverAddressForBytes([]byte("old-art")) + + if err := store.RemoveSeries("asura", "solo"); err != nil { + t.Fatalf("RemoveSeries: %v", err) + } + // The caller's sequence: the row is deleted first, then the address is + // reclaimed — the guard cannot pass while the row still points at it. + if err := store.ReclaimCover(addr); err != nil { + t.Fatalf("ReclaimCover: %v", err) + } + var one int + if err := store.db.QueryRow(`SELECT 1 FROM series WHERE site = $1 AND series_id = $2`, "asura", "solo").Scan(&one); err != sql.ErrNoRows { + t.Fatalf("series row after remove = %v, want sql.ErrNoRows", err) + } + if _, _, ok, err := store.CoverByAddress(addr); err != nil || ok { + t.Fatalf("covers row after remove = found %v err %v, want gone", ok, err) + } + if _, err := os.Stat(coverShardPath(t, store, addr)); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("sharded file after remove = %v, want fs.ErrNotExist", err) + } +} + +// The refusal is the whole point of the sentinel: a Series a Reader still +// holds is not removed, its row is untouched and its Cover keeps serving. +func TestRemoveSeriesRefusedWhileBookmarked(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/kept.jpg", []byte("kept-art"), "image/jpeg"); err != nil { + t.Fatalf("put cover: %v", err) + } + addr := CoverAddressForBytes([]byte("kept-art")) + + if err := store.RemoveSeries("asura", "solo"); !errors.Is(err, ErrSeriesHasBookmarks) { + t.Fatalf("RemoveSeries on a bookmarked series = %v, want ErrSeriesHasBookmarks", err) + } + var held int + if err := store.db.QueryRow(`SELECT 1 FROM series WHERE site = $1 AND series_id = $2`, "asura", "solo").Scan(&held); err != nil { + t.Fatal("series row after refusal is gone; want it untouched") + } + if body, _, ok, err := store.CoverByAddress(addr); err != nil || !ok || string(body) != "kept-art" { + t.Fatalf("cover after refusal = found %v err %v, want still served", ok, err) + } +} + +// A Series sharing its Cover address with a second Series is removed while +// the artwork stays readable through CoverByAddress: the series row stops +// referencing the address first, so ReclaimCover's guard passes for this +// caller without touching the shared bytes (ADR-0014). +func TestRemoveSeriesSparesSharedCover(t *testing.T) { + store := newTestStore(t) + seedForCheck(t, store, "asura:solo", "https://asurascans.com/comics/solo", 0) + seedForCheck(t, store, "asura:second", "https://asurascans.com/comics/second", 0) + if err := store.Delete(store.OwnerID(), "asura:solo"); err != nil { + t.Fatalf("orphan solo: %v", err) + } + if err := store.Delete(store.OwnerID(), "asura:second"); err != nil { + t.Fatalf("orphan second: %v", err) + } + const src = "https://cdn.example/covers/shared.jpg" + if err := store.SetSeriesCover("asura", "solo", src, []byte("shared-art"), "image/jpeg"); err != nil { + t.Fatalf("put cover on solo: %v", err) + } + if err := store.SetSeriesCover("asura", "second", src, []byte("shared-art"), "image/jpeg"); err != nil { + t.Fatalf("put cover on second: %v", err) + } + addr := CoverAddressForBytes([]byte("shared-art")) + + if err := store.RemoveSeries("asura", "solo"); err != nil { + t.Fatalf("RemoveSeries: %v", err) + } + // The guard spares the shared bytes even though this caller reclaims. + if err := store.ReclaimCover(addr); err != nil { + t.Fatalf("ReclaimCover over a shared address: %v", err) + } + if body, _, ok, err := store.CoverByAddress(addr); err != nil || !ok || string(body) != "shared-art" { + t.Fatalf("shared bytes after remove = found %v err %v, want still served", ok, err) + } + if _, err := os.Stat(coverShardPath(t, store, addr)); err != nil { + t.Fatalf("sharded file after remove: %v, want present", err) + } + var one int + if err := store.db.QueryRow(`SELECT 1 FROM series WHERE site = $1 AND series_id = $2`, "asura", "second").Scan(&one); err != nil { + t.Fatal("the second series row vanished with the first") + } +} + +// Deleting an absent key removes nothing and is not an error, matching the +// Delete precedent — the handler's own lookups turn the absent case into the +// 404 before the store ever sees it. +func TestRemoveSeriesMissingKeyIsCleanNoOp(t *testing.T) { + store := newTestStore(t) + if err := store.RemoveSeries("asura", "ghost"); err != nil { + t.Fatalf("RemoveSeries on a missing key = %v, want nil", err) + } +} diff --git a/backend/internal/web/admin.go b/backend/internal/web/admin.go index cad99c5..c3f6aff 100644 --- a/backend/internal/web/admin.go +++ b/backend/internal/web/admin.go @@ -49,6 +49,7 @@ func (h *Handler) adminRoutes() []adminRoute { {"POST /admin/series/{key}/poll", h.adminSeriesPoll}, {"POST /admin/series/{key}/latest", h.adminSeriesCorrectLatest}, {"POST /admin/series/{key}/series-url", h.adminSeriesSetURL}, + {"POST /admin/series/{key}/remove", h.adminSeriesRemove}, {"POST /admin/lanes/{site}/pause", h.adminLanePause}, {"POST /admin/lanes/{site}/resume", h.adminLaneResume}, {"GET /ui/admin/lanes", h.uiLanes}, diff --git a/backend/internal/web/admin_series.go b/backend/internal/web/admin_series.go index d94c255..e0cfc35 100644 --- a/backend/internal/web/admin_series.go +++ b/backend/internal/web/admin_series.go @@ -1,6 +1,7 @@ package web import ( + "errors" "fmt" "log" "math" @@ -48,7 +49,6 @@ var seriesFilterOrder = []string{ } // seriesListView is the Series list page's data. The template renders strings -// and flags, and every judgement about what a value means is made here. type seriesListView struct { Filters []seriesFilterOption Sites []string @@ -57,6 +57,9 @@ type seriesListView struct { FilterLabel string Rows []seriesRowView Total int + // OOB marks the out-of-band copy of the heading the removal answer + // carries; on the page itself it is false (issue #155). + OOB bool // KindBoth / KindManga / KindNovel are the Library segment links, and // PrevHref / NextHref the pager's, all carrying the active filter, Site // and Kind so narrowing never drops state. @@ -101,6 +104,12 @@ type seriesRowView struct { CanPoll bool Pending bool Requested string // "requested 3m ago", rendered only while pending + // CanRemove is the Remove control's visibility: only a Series no Reader + // holds can be removed, so the owner is never offered a button that the + // database will always refuse (issue #155). RemovalRefused marks the one + // raced answer: the row stays and says a fresh Bookmark caught the press. + CanRemove bool + RemovalRefused bool } // adminSeries renders the filterable, bookmarkable Series list: filter, Site, @@ -279,6 +288,163 @@ func (h *Handler) adminSeriesSetURL(w http.ResponseWriter, r *http.Request) { h.render(w, http.StatusOK, "series-detail-meta", h.seriesDetailView(a)) } +// seriesListHeadView is the list heading's data. The template renders it +// inline at the top of the Series list and out of band in the removal answer +// (OOB true, like the chrome partials' OOB flag): the count and the filter +// label are one fact (issue #155). +type seriesListHeadView struct { + Total int + FilterLabel string + OOB bool +} + +// adminSeriesRemove is the orphan removal: one Series, one delete, refused by +// the database while any Bookmark exists (translated by the store, never a +// driver error on the page). The owner gate is the route's, not this +// handler's; the body is capped like the API path caps its bodies; the key is +// validated here — a malformed key is a 400 and an unknown one a 404. +// +// The Cover is read from the row before the delete and reclaimed after it: +// ReclaimCover's guard cannot pass while a series row still points at the +// address, so the order is the sequence, not a preference. A reclamation +// failure is not a removal failure — the row is gone and the covers row +// survives for a retry; the handler logs and answers success, because the +// failure has no user-facing surface. +// +// Two callers, one handler, branched on HX-Target like adminSeriesPoll. The +// detail page's remove answers with a navigation — to the No-Readers list on +// success, back to the detail page when a fresh Bookmark raced the press, +// where the new count is visible. The list row's answers with the removed +// row's fragment and the heading re-rendered with the fresh count out of +// band; HX-Reswap deletes the row through the same button that swaps the +// refusal back in, and the count query runs over the press's own filter +// state, so the heading describes the list the owner is looking at. +func (h *Handler) adminSeriesRemove(w http.ResponseWriter, r *http.Request) { + site, seriesID, ok := strings.Cut(r.PathValue("key"), ":") + if !ok || site == "" || seriesID == "" { + http.Error(w, "bad series key", http.StatusBadRequest) + return + } + r.Body = http.MaxBytesReader(w, r.Body, 1<<16) + if err := r.ParseForm(); err != nil { + http.Error(w, "invalid form", http.StatusBadRequest) + return + } + // The row's Cover address is read before the delete because the delete is + // what makes it reclaimable. + a, found, err := h.adminSeriesByKey(site, seriesID) + if err != nil { + log.Printf("series remove %s: %v", site+":"+seriesID, err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + if !found { + http.NotFound(w, r) + return + } + cover := a.CoverAddress + if err := h.store.RemoveSeries(site, seriesID); err != nil { + if errors.Is(err, store.ErrSeriesHasBookmarks) { + // A Bookmark landed between the owner's read and the press: the + // row stays, answered at its new count with the fact spelled + // out — never a 500, and never a deleted row. + if r.Header.Get("HX-Target") == "detail-meta" { + seriesRemoveNavigation(w, r, "/admin/series/"+site+":"+seriesID) + return + } + fresh, found, err := h.adminSeriesByKey(site, seriesID) + if err != nil { + log.Printf("series remove %s: %v", site+":"+seriesID, err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + if !found { + // A second press removed it while this one was refused; the + // row has nothing left to say. + http.NotFound(w, r) + return + } + band := 0 + if r.PostFormValue("band") == "1" { + band = 1 + } + row := seriesRow(fresh, band, time.Now()) + row.RemovalRefused = true + h.render(w, http.StatusOK, "series-row", row) + return + } + log.Printf("series remove %s: %v", site+":"+seriesID, err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + if err := h.store.ReclaimCover(cover); err != nil { + log.Printf("series remove %s: reclaim cover: %v", site+":"+seriesID, err) + } + if r.Header.Get("HX-Target") == "detail-meta" { + seriesRemoveNavigation(w, r, "/admin/series?filter="+store.SeriesFilterNoReaders) + return + } + // The list answer: the removed row's fragment, plus the heading + // re-rendered with the fresh count. HX-Reswap deletes the row through the + // same button that swaps the refusal back in. The count query failing + // does not undo the removal — log it and answer the row alone. + w.Header().Set("HX-Reswap", "delete") + band := 0 + if r.PostFormValue("band") == "1" { + band = 1 + } + h.render(w, http.StatusOK, "series-row", seriesRow(a, band, time.Now())) + if head, err := h.seriesListHeadView(r); err != nil { + log.Printf("series remove %s: %v", site+":"+seriesID, err) + } else { + h.render(w, http.StatusOK, "series-list-head", head) + } +} + +// seriesListHeadView is the list heading with the count as it stands after a +// removal: the same filter, Site and Kind the press's row carried (the list +// row's button hx-includes the filterbar), so the figure describes the list +// the owner is looking at — the All filter and an unknown one stay the +// absent case. The count is the store's window total, one query. +func (h *Handler) seriesListHeadView(r *http.Request) (seriesListHeadView, error) { + filter := r.PostFormValue("filter") + if _, ok := seriesFilterLabels[filter]; !ok { + filter = store.SeriesFilterAll + } + site := r.PostFormValue("site") + kind := r.PostFormValue("kind") + if kind != store.KindManga && kind != store.KindNovel { + kind = "" + } + data, err := h.store.SeriesPage(store.SeriesFilter{ + Site: site, + Kind: kind, + Name: filter, + Cutoff: time.Now().Add(-ownerWindow).UnixMilli(), + Page: 1, + }) + if err != nil { + return seriesListHeadView{}, err + } + return seriesListHeadView{ + Total: data.Total, + FilterLabel: seriesFilterLabels[filter], + OOB: true, + }, nil +} + +// seriesRemoveNavigation answers a removal from the detail page. htmx gets a +// full navigation (HX-Redirect): a bare 303 would be followed by the request +// and the landing page swapped into the press's target, so the header is the +// redirect htmx can see; plain clients get the 303 the ticket names. +func seriesRemoveNavigation(w http.ResponseWriter, r *http.Request, to string) { + if r.Header.Get("HX-Request") != "" { + w.Header().Set("HX-Redirect", to) + return + } + http.Redirect(w, r, to, http.StatusSeeOther) +} + // seriesListView assembles one Series list view from the request's query // string. An unknown filter value is the absent All case, never an error: the // select's options are not the only way this URL can be reached. @@ -403,6 +569,7 @@ func seriesRow(a store.AdminSeries, i int, now time.Time) seriesRowView { Readers: a.ReaderCount, Band: i%2 == 1, CanPoll: canPoll, + CanRemove: a.ReaderCount == 0, Pending: pending, Requested: requested, } diff --git a/backend/internal/web/admin_series_detail.go b/backend/internal/web/admin_series_detail.go index 4166f9a..2f5e889 100644 --- a/backend/internal/web/admin_series_detail.go +++ b/backend/internal/web/admin_series_detail.go @@ -52,6 +52,10 @@ type seriesDetailView struct { CanPoll bool Pending bool Requested string + // CanRemove is the Remove control's visibility (issue #155): only a + // Series no Reader holds can be removed, so the owner is never offered a + // button the database will always refuse. + CanRemove bool } // adminSeriesDetail renders one Series' page, keyed by the composite @@ -122,6 +126,7 @@ func (h *Handler) seriesDetailView(a store.AdminSeries) seriesDetailView { Orphan: a.ReaderCount == 0, SightingRaised: a.RaisedByReader, CanPoll: canPoll, + CanRemove: a.ReaderCount == 0, Pending: pending, Requested: requested, } diff --git a/backend/internal/web/static/admin.css b/backend/internal/web/static/admin.css index dd4f23e..6eabb79 100644 --- a/backend/internal/web/static/admin.css +++ b/backend/internal/web/static/admin.css @@ -653,6 +653,13 @@ margin-left: auto; } +.admin-sheet .tbl.series .row-msg { + grid-column: 1 / -1; + margin-top: 6px; + color: var(--danger-soft); + font: 400 13px/1.4 var(--font-body); +} + .admin-sheet .detail-back { display: inline-block; margin: 18px 0 0; diff --git a/backend/internal/web/templates/series-detail.html b/backend/internal/web/templates/series-detail.html index 71f63c8..1bc2ba5 100644 --- a/backend/internal/web/templates/series-detail.html +++ b/backend/internal/web/templates/series-detail.html @@ -34,6 +34,11 @@
Check now
{{end}} +{{if .CanRemove}} +
+
+
+{{end}} {{end}} {{/* series-detail-meta is the meta line, and the answer a Check now or diff --git a/backend/internal/web/templates/series-list.html b/backend/internal/web/templates/series-list.html index c2cce35..0d51ff2 100644 --- a/backend/internal/web/templates/series-list.html +++ b/backend/internal/web/templates/series-list.html @@ -4,7 +4,7 @@ that can be bookmarked: the two selects submit the GET form, and the Library segment links and the pager preserve the filter and Site. */}} {{define "series-list"}} -
+