From b9dd75684e82efcf235273b62b52a6faa2fa53ce Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Mon, 22 Jun 2026 21:34:39 +0200 Subject: [PATCH] fix(correctness): pagination tiebreakers, dedup race, duration gating (0.87.6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three medium correctness fixes: 1. **Pagination ORDER BY missing tiebreakers** — four admin list queries sorted by a timestamp or `chapters.number` with no stable secondary sort. Add `id` tiebreakers across `repo::admin_view`, `repo::admin_audit`, `repo::bookmark`. 2. **`enqueue_pages` NOT EXISTS read-then-insert race** — concurrent admin clicks could land duplicate analyze_page jobs. Migration 0031 adds a partial unique index mirroring 0014; query relies on `ON CONFLICT DO NOTHING`. 3. **`record_duration` overwrote a prior done row's duration on a non-terminal failed retry of a force-re-analyze.** Gate the call on "did this attempt actually write a row?". Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/Cargo.lock | 2 +- backend/Cargo.toml | 2 +- .../0031_crawler_jobs_analyze_page_dedup.sql | 17 +++++++++++++++++ backend/src/analysis/daemon.rs | 18 ++++++++++++++---- backend/src/repo/admin_audit.rs | 6 +++++- backend/src/repo/admin_view.rs | 13 +++++++++++-- backend/src/repo/bookmark.rs | 6 +++++- backend/src/repo/page_analysis.rs | 14 +++++++++----- frontend/package.json | 2 +- 9 files changed, 64 insertions(+), 16 deletions(-) create mode 100644 backend/migrations/0031_crawler_jobs_analyze_page_dedup.sql diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 9b9f403..8eb1807 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1517,7 +1517,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4" [[package]] name = "mangalord" -version = "0.87.5" +version = "0.87.6" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index f5b9909..915c140 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.87.5" +version = "0.87.6" edition = "2021" default-run = "mangalord" diff --git a/backend/migrations/0031_crawler_jobs_analyze_page_dedup.sql b/backend/migrations/0031_crawler_jobs_analyze_page_dedup.sql new file mode 100644 index 0000000..0023338 --- /dev/null +++ b/backend/migrations/0031_crawler_jobs_analyze_page_dedup.sql @@ -0,0 +1,17 @@ +-- Dedup analyze_page jobs in flight (mirrors 0014 for sync_chapter_content). +-- +-- Without this, `repo::page_analysis::enqueue_pages`' `NOT EXISTS(... pending +-- | running ...)` read-then-insert race let concurrent admin POSTs land +-- duplicate jobs for the same page — visible as duplicate "now analyzing" +-- SSE events and inflated `pending` counters in the admin dashboard, even +-- though the worker's `skip-if-done` net (analysis/daemon.rs:210-217) +-- prevented the actual duplicate vision call. +-- +-- The partial unique index lets enqueue_pages drop the NOT EXISTS subquery +-- and rely on `ON CONFLICT DO NOTHING` instead. At most one (pending|running) +-- job per page_id can exist; the slot frees the moment the job transitions +-- to done/failed/dead, so a force re-analyze still re-enqueues cleanly. +CREATE UNIQUE INDEX crawler_jobs_analyze_page_dedup_idx + ON crawler_jobs ((payload->>'page_id')) + WHERE state IN ('pending', 'running') + AND payload->>'kind' = 'analyze_page'; diff --git a/backend/src/analysis/daemon.rs b/backend/src/analysis/daemon.rs index 9cdc0ef..03cc16e 100644 --- a/backend/src/analysis/daemon.rs +++ b/backend/src/analysis/daemon.rs @@ -313,6 +313,17 @@ impl WorkerContext { self.events.publish(event); } + // Stamp how long the vision dispatch took — but ONLY when a row + // was actually written this attempt. Otherwise a non-terminal + // failure on a force-re-analyze (page already has a `done` row) + // would overwrite the prior duration with the duration of a + // failed retry that wrote nothing new. + let wrote_row = match &result { + Ok(()) => true, + Err(_) if lease.attempts >= lease.max_attempts => true, // mark_failed wrote below + Err(_) => false, + }; + match result { Ok(()) => { let _ = jobs::ack_done(&self.pool, lease.id).await; @@ -341,10 +352,9 @@ impl WorkerContext { } } - // Stamp how long the vision dispatch took. Best-effort and ordered - // after the row is written (persist on success / mark_failed on - // terminal failure); a transient failure with no row updates nothing. - let _ = repo::page_analysis::record_duration(&self.pool, page_id, elapsed_ms).await; + if wrote_row { + let _ = repo::page_analysis::record_duration(&self.pool, page_id, elapsed_ms).await; + } } } diff --git a/backend/src/repo/admin_audit.rs b/backend/src/repo/admin_audit.rs index 87498b1..757f18c 100644 --- a/backend/src/repo/admin_audit.rs +++ b/backend/src/repo/admin_audit.rs @@ -60,7 +60,11 @@ pub async fn list( AND ($2::text IS NULL OR a.target_kind = $2) AND ($3::uuid IS NULL OR a.actor_user_id = $3) AND ($4::timestamptz IS NULL OR a.at >= $4) - ORDER BY a.at DESC + -- `id` tiebreaker for stable pagination: a single admin action + -- can write multiple audit rows in the same statement_timestamp + -- (e.g. bulk requeue) and ties on the timestamp would otherwise + -- skip or repeat rows across pages. + ORDER BY a.at DESC, a.id DESC LIMIT $5 OFFSET $6 "#, ) diff --git a/backend/src/repo/admin_view.rs b/backend/src/repo/admin_view.rs index 1064462..ca79957 100644 --- a/backend/src/repo/admin_view.rs +++ b/backend/src/repo/admin_view.rs @@ -239,7 +239,11 @@ pub async fn list_mangas_with_sync_state( ) SELECT * FROM classified WHERE ($2::text IS NULL OR sync_state = $2) - ORDER BY updated_at DESC + -- `id` tiebreaker stabilises pagination across mangas that share an + -- `updated_at` (multi-row crawl-tick bulk updates land within the + -- same statement_timestamp() and tie on Postgres' default ts + -- resolution). Without it, page boundaries can skip or repeat rows. + ORDER BY updated_at DESC, id DESC LIMIT $3 OFFSET $4 "#, case = MANGA_SYNC_STATE_CASE @@ -332,7 +336,12 @@ pub async fn list_chapters_with_sync_state( WHERE cs.chapter_id = c.id AND cs.dropped_at IS NULL) AS latest_seen_at FROM chapters c WHERE c.manga_id = $1 - ORDER BY c.number ASC + -- `id` tiebreaker: migration 0013 dropped the (manga_id, number) + -- UNIQUE constraint to allow variants ("Ch.14: PH" / "Ch.14: + -- Official") and non-numeric entries (all parse to 0). Without a + -- tiebreaker, pagination over those tied rows can skip or repeat + -- depending on Postgres' chosen sort order. + ORDER BY c.number ASC, c.id ASC LIMIT $2 OFFSET $3 "#, ) diff --git a/backend/src/repo/bookmark.rs b/backend/src/repo/bookmark.rs index 5d4b9f8..7e242fc 100644 --- a/backend/src/repo/bookmark.rs +++ b/backend/src/repo/bookmark.rs @@ -63,7 +63,11 @@ pub async fn list_for_user( INNER JOIN mangas m ON m.id = b.manga_id LEFT JOIN chapters c ON c.id = b.chapter_id WHERE b.user_id = $1 - ORDER BY b.created_at DESC + -- `manga_id` tiebreaker: the (user_id, manga_id) PK guarantees no + -- two bookmarks share both, so this is a deterministic ordering. + -- Without it, multiple bookmarks added in the same `created_at` + -- tick (timestamptz resolution: 1µs) can skip/repeat across pages. + ORDER BY b.created_at DESC, b.manga_id DESC LIMIT $2 OFFSET $3 "#, ) diff --git a/backend/src/repo/page_analysis.rs b/backend/src/repo/page_analysis.rs index 420e8b6..78077d4 100644 --- a/backend/src/repo/page_analysis.rs +++ b/backend/src/repo/page_analysis.rs @@ -86,6 +86,14 @@ pub async fn enqueue_pages( } ReenqueueScope::Chapter(_) => "AND p.chapter_id = $2", }; + // The `NOT EXISTS(... pending|running ...)` pre-check used to live in + // this query, but it races with concurrent admin clicks (two requests + // both see "no in-flight job" and both INSERT). The partial unique + // index `crawler_jobs_analyze_page_dedup_idx` (migration 0031) covers + // (payload->>'page_id') WHERE state IN ('pending', 'running') AND + // kind = 'analyze_page', so `ON CONFLICT DO NOTHING` is now the + // atomic primitive — duplicates can't land and concurrent enqueue + // calls are both observably correct. let sql = format!( r#" INSERT INTO crawler_jobs (payload) @@ -95,11 +103,7 @@ pub async fn enqueue_pages( SELECT 1 FROM page_analysis pa WHERE pa.page_id = p.id AND pa.status = 'done')) {scope_clause} - AND NOT EXISTS ( - SELECT 1 FROM crawler_jobs j - WHERE j.payload->>'kind' = 'analyze_page' - AND j.payload->>'page_id' = p.id::text - AND j.state IN ('pending', 'running')) + ON CONFLICT DO NOTHING "# ); let query = sqlx::query(&sql).bind(only_unanalyzed); diff --git a/frontend/package.json b/frontend/package.json index 6d3764f..7106776 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.87.5", + "version": "0.87.6", "private": true, "type": "module", "scripts": {