From aa2159ca06fb439243b55f8dab6eaaeace3cab28 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Sat, 30 May 2026 21:58:15 +0200 Subject: [PATCH] =?UTF-8?q?fix(admin):=20three=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20audit=20no-op,=20404,=20chapter=20priority=20(0.41.?= =?UTF-8?q?1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - admin_safe_set_is_admin: short-circuit when target.is_admin == value, before writing audit. PATCH {is_admin: true} on someone already admin previously wrote a misleading "promote_user" row even though the UPDATE was a no-op. - list_chapters (/admin/mangas/:id/chapters): explicit exists() check on manga_id, returns 404 instead of 200 [] for a typo'd / deleted manga. - ChapterSyncState priority: the Failed branch now requires page_count = 0, so a chapter with pages on disk AND a historical dead job (from a re-download attempt that crashed) stays Synced. The old order contradicted Synced's documented "downloaded at some point" contract. Doc comments updated alongside the SQL. Three new regression tests pin the behaviour. --- backend/Cargo.lock | 2 +- backend/Cargo.toml | 2 +- backend/src/api/admin/mangas.rs | 5 ++++ backend/src/domain/sync_state.rs | 8 +++-- backend/src/repo/admin_view.rs | 13 ++++++-- backend/src/repo/user.rs | 10 +++++++ backend/tests/api_admin_mangas.rs | 50 +++++++++++++++++++++++++++++++ backend/tests/api_admin_users.rs | 31 +++++++++++++++++++ frontend/package.json | 2 +- 9 files changed, 114 insertions(+), 9 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index aa9ea2e..a8a4e95 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1470,7 +1470,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4" [[package]] name = "mangalord" -version = "0.41.0" +version = "0.41.1" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 828c5d9..369d11c 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.41.0" +version = "0.41.1" edition = "2021" default-run = "mangalord" diff --git a/backend/src/api/admin/mangas.rs b/backend/src/api/admin/mangas.rs index 010122d..0095705 100644 --- a/backend/src/api/admin/mangas.rs +++ b/backend/src/api/admin/mangas.rs @@ -77,6 +77,11 @@ async fn list_chapters( _admin: RequireAdmin, Path(manga_id): Path, ) -> AppResult>> { + // Explicit existence check so a typo / deleted manga returns 404 + // rather than a misleading "no chapters" 200. + if !repo::manga::exists(&state.db, manga_id).await? { + return Err(AppError::NotFound); + } let rows = repo::admin_view::list_chapters_with_sync_state(&state.db, manga_id).await?; Ok(Json(rows)) } diff --git a/backend/src/domain/sync_state.rs b/backend/src/domain/sync_state.rs index 2e352c5..8a437b7 100644 --- a/backend/src/domain/sync_state.rs +++ b/backend/src/domain/sync_state.rs @@ -31,10 +31,12 @@ pub enum ChapterSyncState { /// At least one `chapter_sources` row exists AND all of them are /// `dropped_at IS NOT NULL`. Dropped, - /// Most recent `sync_chapter_content` job for this chapter is `dead` - /// (terminal failure). Checked BEFORE `NotDownloaded` so the more + /// `page_count = 0` AND a `dead` `sync_chapter_content` job exists + /// for this chapter. Checked BEFORE `NotDownloaded` so the more /// informative "we tried and it died" state wins over "we never - /// got around to it". + /// got around to it". Does NOT fire when `page_count > 0`, because + /// pages on disk mean the chapter IS synced regardless of historical + /// job failures — see the priority comment in `repo::admin_view`. Failed, /// `page_count = 0` and no in-flight or failed job — the chapter /// row exists but content has never been downloaded. diff --git a/backend/src/repo/admin_view.rs b/backend/src/repo/admin_view.rs index a203563..80da958 100644 --- a/backend/src/repo/admin_view.rs +++ b/backend/src/repo/admin_view.rs @@ -13,8 +13,14 @@ //! Priority order for `ChapterSyncState`: //! 1. `Downloading` — pending/running `sync_chapter_content` for this id //! 2. `Dropped` — chapter has source rows AND all are dropped -//! 3. `Failed` — most recent terminal `sync_chapter_content` is `dead` -//! 4. `NotDownloaded` — `page_count = 0` +//! 3. `Failed` — `page_count = 0` AND a `dead` `sync_chapter_content` +//! row exists for this chapter. Constrained to `page_count = 0` +//! because once pages are on disk the chapter IS synced — a +//! historical dead job (likely from a re-download attempt that +//! crashed) is noise that gets reaped after retention. Surfacing +//! "Failed" when content is present would contradict +//! `ChapterSyncState::Synced`'s "downloaded at some point" contract. +//! 4. `NotDownloaded` — `page_count = 0`, no in-flight, no dead job //! 5. `Synced` — `page_count > 0` //! //! Reminder: `done` jobs are reaped after `CRAWLER_JOB_RETENTION_DAYS`, @@ -181,7 +187,8 @@ pub async fn list_chapters_with_sync_state( WHERE cs.chapter_id = c.id AND cs.dropped_at IS NULL ) THEN 'dropped' - WHEN EXISTS ( + WHEN c.page_count = 0 + AND EXISTS ( SELECT 1 FROM crawler_jobs cj WHERE cj.state = 'dead' AND cj.payload->>'kind' = 'sync_chapter_content' diff --git a/backend/src/repo/user.rs b/backend/src/repo/user.rs index 6ee8900..e80ec1a 100644 --- a/backend/src/repo/user.rs +++ b/backend/src/repo/user.rs @@ -172,6 +172,16 @@ pub async fn admin_safe_set_is_admin( return Err(AppError::NotFound); }; + // No-op: caller asked to set `is_admin` to its current value. Return + // the row as-is without writing an audit entry — otherwise repeated + // PATCH calls (browser retry, double-click) pile misleading + // "promote_user" rows in `admin_audit` for actions that changed + // nothing. + if target.is_admin == value { + tx.commit().await?; + return Ok(target); + } + // Recount inside the lock — this is the authoritative read. if target.is_admin && !value { let admin_count: i64 = diff --git a/backend/tests/api_admin_mangas.rs b/backend/tests/api_admin_mangas.rs index c4746f7..b2d06d2 100644 --- a/backend/tests/api_admin_mangas.rs +++ b/backend/tests/api_admin_mangas.rs @@ -423,6 +423,56 @@ async fn http_list_chapters_returns_per_chapter_state(pool: PgPool) { assert_eq!(items[1]["sync_state"], "not_downloaded"); } +#[sqlx::test(migrations = "./migrations")] +async fn http_list_chapters_returns_404_for_unknown_manga(pool: PgPool) { + // Regression: used to return 200 [] for a non-existent manga, + // which silently rendered "No chapters." for a typo'd / deleted id. + let h = common::harness(pool.clone()); + let (_admin, cookie) = seed_admin(&pool, &h.app).await; + let resp = h + .app + .oneshot(common::get_with_cookie( + &format!("/api/v1/admin/mangas/{}/chapters", Uuid::new_v4()), + &cookie, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::NOT_FOUND); +} + +#[sqlx::test(migrations = "./migrations")] +async fn chapter_state_synced_when_pages_present_even_with_dead_job(pool: PgPool) { + // Regression: the old CASE prioritised the dead-job branch above + // the page_count check, so a chapter with pages on disk AND a + // historical dead job (e.g. from a re-download attempt that + // crashed) flipped to Failed — contradicting Synced's "downloaded + // at some point" contract. + seed_source(&pool).await; + let m = insert_manga(&pool, "M").await; + let c = insert_chapter(&pool, m, 1, 12).await; // pages present + insert_chapter_source(&pool, c, "ckey-1", false).await; + insert_job( + &pool, + json!({ + "kind": "sync_chapter_content", + "source_id": SOURCE_ID, + "chapter_id": c.to_string(), + "source_chapter_key": "ckey-1", + }), + "dead", + ) + .await; + + let rows = repo::admin_view::list_chapters_with_sync_state(&pool, m) + .await + .unwrap(); + assert_eq!( + rows[0].sync_state, + mangalord::domain::ChapterSyncState::Synced, + "pages on disk override historical dead-job noise" + ); +} + #[sqlx::test(migrations = "./migrations")] async fn http_list_mangas_requires_admin(pool: PgPool) { let h = common::harness(pool); diff --git a/backend/tests/api_admin_users.rs b/backend/tests/api_admin_users.rs index 4743228..c350ee9 100644 --- a/backend/tests/api_admin_users.rs +++ b/backend/tests/api_admin_users.rs @@ -330,6 +330,37 @@ async fn promote_writes_audit_row(pool: PgPool) { assert_eq!(target, Some(b.id)); } +#[sqlx::test(migrations = "./migrations")] +async fn redundant_promote_does_not_write_audit_row(pool: PgPool) { + // Regression: PATCH {is_admin: true} on someone already admin used + // to UPDATE (no-op) and still INSERT a misleading "promote_user" + // audit row. Should short-circuit without touching admin_audit. + let h = common::harness(pool.clone()); + let (_a_name, a_cookie, _a_id) = seed_admin(&pool, &h.app).await; + let (b_name, _b_cookie, _b_id) = seed_admin(&pool, &h.app).await; // already admin + + let b = repo::user::find_by_username(&pool, &b_name) + .await + .unwrap() + .unwrap(); + let resp = h + .app + .oneshot(common::patch_json_with_cookie( + &format!("/api/v1/admin/users/{}", b.id), + json!({ "is_admin": true }), + &a_cookie, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + + let (count,): (i64,) = sqlx::query_as("SELECT COUNT(*) FROM admin_audit") + .fetch_one(&pool) + .await + .unwrap(); + assert_eq!(count, 0, "no-op promote must not write audit row"); +} + #[sqlx::test(migrations = "./migrations")] async fn delete_writes_audit_row(pool: PgPool) { let h = common::harness(pool.clone()); diff --git a/frontend/package.json b/frontend/package.json index f678c6a..2e4aeb5 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.41.0", + "version": "0.41.1", "private": true, "type": "module", "scripts": {