fabi/Mangalord
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(jobs): per-lease generation token closes the ack-from-dead-lease race (0.87.26)

Browse Source 
0.87.20 narrowed but did not close the force-analyze race:
ack_done / ack_failed / renew matched only on (id, state='running'),
so a worker A whose lease was released mid-flight (force-analyze,
reclaim_orphaned) could still come back later — once a successor B
re-leased the same id and put it back to state='running' — and
clobber B's lease. A's stale result became the row's `done` payload;
B's in-flight work was silently lost. The pre-existing test even
documented this hole: "We can't make ack_done's lease_id distinguish
A from B today".

Add `lease_generation BIGINT NOT NULL DEFAULT 0` to crawler_jobs
(migration 0032). It is bumped by every lease, release / release_unowned,
and reclaim_orphaned, so lease identity is `(id, generation)` rather
than just `id`. Each Lease struct carries its generation. ack_done /
ack_failed / renew / release match on `(id, generation, state='running')`
so a stale ack from a prior generation finds zero rows and falls
back to the existing warn-and-skip branch.

Split the release surface in two:
  * release(lease_id, lease_generation) — owner-aware path for
    workers (graceful shutdown, session-expired), matches the
    caller's specific lease.
  * release_unowned(lease_id) — id-only path for force-analyze and
    ops tools that drop a running lease without holding a Lease
    struct; unconditionally bumps generation so any pending ack
    from the in-flight original becomes a no-op.

Force-analyze in repo::page_analysis now uses release_unowned; the
test in api_admin_analysis still passes unchanged (it only asserts
the steady-state outcome). The pre-existing `ack_done_no_ops_when_
lease_was_stolen` test is strengthened: it now mints both A and B
leases, asserts their generations differ, and verifies A's stale
ack does NOT clobber B's running row — the assertion the old test
explicitly couldn't make.

Five new tests in tests/crawler_jobs.rs pin every facet:
  * lease_assigns_strictly_increasing_generation_per_release
  * ack_done_from_dead_lease_after_release_unowned_is_a_no_op
  * ack_failed_from_dead_lease_after_release_unowned_is_a_no_op
  * renew_from_dead_lease_is_a_no_op
  * release_unowned_bumps_generation_even_when_lease_is_held

Mutation-confirmed: relaxing the ack_done guard to
`lease_generation >= $2` (bind 0) makes the race test fail with
state=done — the exact prior-bug shape.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-06-24 20:05:55 +02:00
parent ae708a72d0
commit 93b7e451bf
10 changed files with 419 additions and 82 deletions
Download Patch File Download Diff File Expand all files Collapse all files

290
backend/tests/crawler_jobs.rs
View File

@@ -209,7 +209,7 @@ async fn renew_extends_leased_until_while_running(pool: PgPool) {
.await
.unwrap();
let still_owned = jobs::renew(&pool, id, Duration::from_secs(120))
let still_owned = jobs::renew(&pool, id, leases[0].lease_generation, Duration::from_secs(120))
.await
.unwrap();
assert!(still_owned, "renew on a running job returns true");
@@ -240,9 +240,11 @@ async fn renew_is_noop_once_job_no_longer_running(pool: PgPool) {
.await
.unwrap();
// Job completes — heartbeat should now see it's no longer ours.
jobs::ack_done(&pool, leases[0].id).await.unwrap();
jobs::ack_done(&pool, leases[0].id, leases[0].lease_generation)
.await
.unwrap();
let still_owned = jobs::renew(&pool, id, Duration::from_secs(120))
let still_owned = jobs::renew(&pool, id, leases[0].lease_generation, Duration::from_secs(120))
.await
.unwrap();
assert!(!still_owned, "renew on a non-running job returns false");
@@ -431,7 +433,8 @@ async fn reclaim_orphaned_resets_only_expired_running_jobs(pool: PgPool) {
.await
.unwrap();
// Leave `pending` pending by returning it (release refunds its attempt).
jobs::release(&pool, pending).await.unwrap();
// Test only has the id, not a Lease, so use the unowned variant.
jobs::release_unowned(&pool, pending).await.unwrap();
assert_eq!(job_state(&pool, pending).await, "pending");
// Orphan: rewind its lease into the past (worker crashed). Live: keep
@@ -482,7 +485,9 @@ async fn ack_done_transitions_state_and_clears_lease(pool: PgPool) {
let leases = jobs::lease(&pool, None, 1, Duration::from_secs(60))
.await
.unwrap();
jobs::ack_done(&pool, leases[0].id).await.unwrap();
jobs::ack_done(&pool, leases[0].id, leases[0].lease_generation)
.await
.unwrap();
assert_eq!(job_state(&pool, id).await, "done");
let leased_until: Option<chrono::DateTime<chrono::Utc>> =
@@ -507,9 +512,16 @@ async fn ack_failed_under_max_returns_to_pending_with_future_schedule(pool: PgPo
.await
.unwrap();
let lease = &leases[0];
jobs::ack_failed(&pool, lease.id, "boom", lease.attempts, lease.max_attempts)
.await
.unwrap();
jobs::ack_failed(
&pool,
lease.id,
"boom",
lease.attempts,
lease.max_attempts,
lease.lease_generation,
)
.await
.unwrap();
assert_eq!(job_state(&pool, id).await, "pending");
@@ -537,9 +549,16 @@ async fn ack_failed_at_max_marks_dead(pool: PgPool) {
.await
.unwrap();
let lease = &leases[0];
jobs::ack_failed(&pool, lease.id, "final boom", lease.max_attempts, lease.max_attempts)
.await
.unwrap();
jobs::ack_failed(
&pool,
lease.id,
"final boom",
lease.max_attempts,
lease.max_attempts,
lease.lease_generation,
)
.await
.unwrap();
assert_eq!(job_state(&pool, id).await, "dead");
let last_error: Option<String> =
@@ -564,7 +583,7 @@ async fn ack_done_no_ops_when_lease_was_stolen(pool: PgPool) {
_ => unreachable!(),
};
// Worker A grabs the lease, but its lease expires immediately.
let _a_leases = jobs::lease(&pool, None, 1, Duration::from_secs(60))
let a_leases = jobs::lease(&pool, None, 1, Duration::from_secs(60))
.await
.unwrap();
sqlx::query("UPDATE crawler_jobs SET leased_until = now() - interval '1 minute' WHERE id = $1")
@@ -578,19 +597,26 @@ async fn ack_done_no_ops_when_lease_was_stolen(pool: PgPool) {
.unwrap();
assert_eq!(b_leases.len(), 1);
assert_eq!(b_leases[0].attempts, 2, "re-lease bumps attempts");
assert_ne!(
a_leases[0].lease_generation, b_leases[0].lease_generation,
"re-lease must bump lease_generation so A's late ack can be distinguished"
);
// Worker A's late ack_done — guarded by `state = 'running'` + lease_id
// but in the simplest implementation the guard is state-only. Either
// way, the job stays 'running' with worker B's progress intact.
jobs::ack_done(&pool, id).await.unwrap();
// Worker B is still working; until B acks, the job remains 'running'
// with its leased_until in the future and attempts == 2.
// (We can't make ack_done's lease_id distinguish A from B today —
// both share the same `id` — so the strongest current guarantee is
// that a late ack_done doesn't fire when state is already 'done',
// exercised below.)
// Finalize: worker B acks done.
jobs::ack_done(&pool, b_leases[0].id).await.unwrap();
// Worker A's late ack_done — now scoped to A's lease_generation by
// migration 0032. The row stays `running` with B's progress
// intact; A's stale ack matches nothing and is a no-op.
jobs::ack_done(&pool, a_leases[0].id, a_leases[0].lease_generation)
.await
.unwrap();
assert_eq!(
job_state(&pool, id).await,
"running",
"A's stale ack_done must NOT clobber B's running lease"
);
// Finalize: worker B acks done with its own generation.
jobs::ack_done(&pool, b_leases[0].id, b_leases[0].lease_generation)
.await
.unwrap();
assert_eq!(job_state(&pool, id).await, "done");
assert_eq!(job_attempts(&pool, id).await, 2);
}
@@ -611,11 +637,13 @@ async fn ack_failed_no_ops_when_state_is_not_running(pool: PgPool) {
let leases = jobs::lease(&pool, None, 1, Duration::from_secs(60))
.await
.unwrap();
jobs::ack_done(&pool, leases[0].id).await.unwrap();
jobs::ack_done(&pool, leases[0].id, leases[0].lease_generation)
.await
.unwrap();
assert_eq!(job_state(&pool, id).await, "done");
// Late ack_failed arrives. Must be a no-op.
jobs::ack_failed(&pool, leases[0].id, "late", 1, 5)
jobs::ack_failed(&pool, leases[0].id, "late", 1, 5, leases[0].lease_generation)
.await
.unwrap();
assert_eq!(
@@ -640,11 +668,15 @@ async fn release_no_ops_when_state_is_not_running(pool: PgPool) {
let leases = jobs::lease(&pool, None, 1, Duration::from_secs(60))
.await
.unwrap();
jobs::ack_done(&pool, leases[0].id).await.unwrap();
jobs::ack_done(&pool, leases[0].id, leases[0].lease_generation)
.await
.unwrap();
let attempts_before = job_attempts(&pool, id).await;
// Late release arrives.
jobs::release(&pool, leases[0].id).await.unwrap();
jobs::release(&pool, leases[0].id, leases[0].lease_generation)
.await
.unwrap();
assert_eq!(
job_state(&pool, id).await,
"done",
@@ -670,7 +702,9 @@ async fn release_returns_to_pending_and_undoes_attempt_increment(pool: PgPool) {
.await
.unwrap();
assert_eq!(leases[0].attempts, 1);
jobs::release(&pool, leases[0].id).await.unwrap();
jobs::release(&pool, leases[0].id, leases[0].lease_generation)
.await
.unwrap();
assert_eq!(job_state(&pool, id).await, "pending");
assert_eq!(job_attempts(&pool, id).await, 0);
@@ -974,3 +1008,201 @@ async fn migration_0031_prededup_demotes_duplicate_analyze_page_rows(pool: PgPoo
.await
.expect("post-dedup index creation must succeed");
}
// ---------------------------------------------------------------------------
// Lease-generation race (0.87.26). Closes the gap that 0.87.20 narrowed:
// `ack_done` / `ack_failed` matched on `(id, state='running')` only, so a
// late ack from a dead lease (id same, generation different) could clobber
// the successor's running row. The new `lease_generation` column scopes
// the guard to the *specific* lease the worker was holding.
// ---------------------------------------------------------------------------
#[sqlx::test(migrations = "./migrations")]
async fn lease_assigns_strictly_increasing_generation_per_release(pool: PgPool) {
// Each lease (the very first one, AND every re-lease after release /
// expiry / reclaim) must carry a strictly-increasing generation so
// downstream guards can distinguish "the lease that was leased" from
// "the lease that's currently leased."
let id = match jobs::enqueue(&pool, &chapter_content_payload(Uuid::new_v4()))
.await
.unwrap()
{
EnqueueResult::Inserted(id) => id,
_ => unreachable!(),
};
let a = jobs::lease(&pool, None, 1, Duration::from_secs(60)).await.unwrap();
assert_eq!(a.len(), 1);
assert!(a[0].lease_generation >= 1, "first lease must carry a positive generation");
jobs::release_unowned(&pool, id).await.unwrap();
let b = jobs::lease(&pool, None, 1, Duration::from_secs(60)).await.unwrap();
assert_eq!(b.len(), 1);
assert!(
b[0].lease_generation > a[0].lease_generation,
"successor lease must carry a strictly-higher generation: a={}, b={}",
a[0].lease_generation,
b[0].lease_generation
);
}
#[sqlx::test(migrations = "./migrations")]
async fn ack_done_from_dead_lease_after_release_unowned_is_a_no_op(pool: PgPool) {
// The race that motivated the lease_generation column. A worker A
// holds a lease. An external actor (force-analyze, reclaim) calls
// `release_unowned` on the row, putting it back to `pending`. A
// worker B re-leases the same id. THEN A's dispatch finally returns
// and acks done. Before this fix, A's `ack_done` matched
// `state='running'` (set by B) and clobbered B's lease — A's stale
// result became the row's `done` payload, B's still-in-flight work
// was lost. Now A's `ack_done(id, generation=A)` finds nothing
// because B's lease bumped the generation, so the row stays
// `running` for B.
let id = match jobs::enqueue(&pool, &chapter_content_payload(Uuid::new_v4()))
.await
.unwrap()
{
EnqueueResult::Inserted(id) => id,
_ => unreachable!(),
};
let a = jobs::lease(&pool, None, 1, Duration::from_secs(300))
.await
.unwrap()[0]
.clone();
assert_eq!(a.id, id);
// External release: e.g. force-analyze flipping payload.force = true
// and dropping the in-flight lease so the new payload is picked up.
jobs::release_unowned(&pool, id).await.unwrap();
assert_eq!(job_state(&pool, id).await, "pending");
let b = jobs::lease(&pool, None, 1, Duration::from_secs(300))
.await
.unwrap()[0]
.clone();
assert_eq!(b.id, id);
assert_ne!(
a.lease_generation, b.lease_generation,
"successor must hold a different lease_generation"
);
assert_eq!(job_state(&pool, id).await, "running");
// A's stale ack arrives. MUST be a no-op — the row stays running
// with B's progress intact.
jobs::ack_done(&pool, a.id, a.lease_generation).await.unwrap();
assert_eq!(
job_state(&pool, id).await,
"running",
"stale ack_done from a dead lease must not clobber the successor's lease"
);
// B finishes normally with its own generation.
jobs::ack_done(&pool, b.id, b.lease_generation).await.unwrap();
assert_eq!(job_state(&pool, id).await, "done");
}
#[sqlx::test(migrations = "./migrations")]
async fn ack_failed_from_dead_lease_after_release_unowned_is_a_no_op(pool: PgPool) {
// Mirror of the ack_done case for ack_failed. A late ack_failed
// from A would otherwise either (a) flip B's running row back to
// `pending` with a stale error and a stale backoff, or (b) at
// max_attempts, dead-letter B's in-flight work.
let id = match jobs::enqueue(&pool, &chapter_content_payload(Uuid::new_v4()))
.await
.unwrap()
{
EnqueueResult::Inserted(id) => id,
_ => unreachable!(),
};
let a = jobs::lease(&pool, None, 1, Duration::from_secs(300))
.await
.unwrap()[0]
.clone();
jobs::release_unowned(&pool, id).await.unwrap();
let b = jobs::lease(&pool, None, 1, Duration::from_secs(300))
.await
.unwrap()[0]
.clone();
assert_eq!(b.id, id);
// A's stale ack_failed arrives.
jobs::ack_failed(&pool, a.id, "A late error", a.attempts, a.max_attempts, a.lease_generation)
.await
.unwrap();
assert_eq!(
job_state(&pool, id).await,
"running",
"stale ack_failed from a dead lease must not clobber the successor"
);
}
#[sqlx::test(migrations = "./migrations")]
async fn renew_from_dead_lease_is_a_no_op(pool: PgPool) {
// A's heartbeat must not extend B's lease window after A was
// released and B re-leased. Without the generation check, an A-side
// renew would push B's `leased_until` further into the future,
// delaying reclaim of a B-side crash.
let id = match jobs::enqueue(&pool, &chapter_content_payload(Uuid::new_v4()))
.await
.unwrap()
{
EnqueueResult::Inserted(id) => id,
_ => unreachable!(),
};
let a = jobs::lease(&pool, None, 1, Duration::from_secs(60))
.await
.unwrap()[0]
.clone();
jobs::release_unowned(&pool, id).await.unwrap();
let _b = jobs::lease(&pool, None, 1, Duration::from_secs(60))
.await
.unwrap();
// A's heartbeat arrives. Should report not-owned and skip.
let still_owned = jobs::renew(&pool, a.id, a.lease_generation, Duration::from_secs(900))
.await
.unwrap();
assert!(
!still_owned,
"renew must report not-owned when the lease_generation no longer matches"
);
}
#[sqlx::test(migrations = "./migrations")]
async fn release_unowned_bumps_generation_even_when_lease_is_held(pool: PgPool) {
// Belt-and-braces: release_unowned exists precisely for cases
// where the caller has no `Lease` in hand (force-analyze, ops
// tools). It must bump the generation unconditionally so any
// ack from the in-flight lease is invalidated.
let id = match jobs::enqueue(&pool, &chapter_content_payload(Uuid::new_v4()))
.await
.unwrap()
{
EnqueueResult::Inserted(id) => id,
_ => unreachable!(),
};
let a = jobs::lease(&pool, None, 1, Duration::from_secs(300))
.await
.unwrap()[0]
.clone();
let gen_before: i64 = sqlx::query_scalar(
"SELECT lease_generation FROM crawler_jobs WHERE id = $1",
)
.bind(id)
.fetch_one(&pool)
.await
.unwrap();
assert_eq!(gen_before, a.lease_generation);
jobs::release_unowned(&pool, id).await.unwrap();
let gen_after: i64 = sqlx::query_scalar(
"SELECT lease_generation FROM crawler_jobs WHERE id = $1",
)
.bind(id)
.fetch_one(&pool)
.await
.unwrap();
assert!(
gen_after > gen_before,
"release_unowned must bump lease_generation: before={gen_before}, after={gen_after}"
);
}