From 608104ab137663d77fdb149305cdfe10ab55a2e7 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Sun, 11 Oct 2026 12:31:48 +0000 Subject: [PATCH] =?UTF-8?q?pg:=20run=5Fschedules'=20failed-renewal=20arm?= =?UTF-8?q?=20classifies=20as=20Database,=20never=20LeadershipLost=20(revi?= =?UTF-8?q?ew=20004=20item=209's=20pg=20twin)=20=E2=80=94=20a=20lost=20lea?= =?UTF-8?q?se=20(Ok(false))=20keeps=20the=20respawn=20arm=20unchanged;=20a?= =?UTF-8?q?=20failed=20renewal=20(closed=20store=20/=20storage=20failure)?= =?UTF-8?q?=20propagates=20its=20opaque=20classification,=20matching=20the?= =?UTF-8?q?=20sqlite=20task's=202026-10-11=20classification=20landed=20eng?= =?UTF-8?q?ine-neutral=20in=20ADR-009=20=C2=A71=20/=20core-contract=20?= =?UTF-8?q?=C2=A7Errors=20(cited,=20not=20re-decided);=20module=20doc=20al?= =?UTF-8?q?igned;=20two=20tests=20pin=20both=20arms=20on=20the=20harness?= =?UTF-8?q?=20server=20(the=20sqlite=20twin's=20pin=20shapes);=20task=20->?= =?UTF-8?q?=20completed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- alkstore-postgres/src/scheduler.rs | 37 +++- .../src/store/scheduler_tests.rs | 168 ++++++++++++++++++ .../pg-fix-scheduler-error-classification.md | 54 +++++- 3 files changed, 249 insertions(+), 10 deletions(-) diff --git a/alkstore-postgres/src/scheduler.rs b/alkstore-postgres/src/scheduler.rs index a07255d..f215154 100644 --- a/alkstore-postgres/src/scheduler.rs +++ b/alkstore-postgres/src/scheduler.rs @@ -35,7 +35,16 @@ //! its own lease by drift; a failed mid-sleep renewal is not acted //! on at the sleep site — the loop's top-of-iteration renew owns //! the loss decision). Acquire-time refusal is `Err(LeadershipLost)` -//! — the same matchable loss a mid-run loser returns. The tick: +//! — the same matchable loss a mid-run loser returns. A mid-run +//! top-of-iteration renewal's arms (the SQLite twin's shape, the +//! 2026-10-11 classification, review 004 item 9): a *refused* +//! renewal (`renew` → `Ok(false)` — the lease expired and was +//! re-acquired elsewhere) is the loss, `Err(LeadershipLost)` +//! return-before-tick (the respawn arm); a *failed* renewal is not +//! a loss and propagates its opaque `Database` classification +//! unchanged (the closed-store posture — respawning a leader +//! against a dead store is misdirected — and any other storage +//! failure). The tick: //! one pool-checked-out transaction (the [`crate::queue::in_tx`] //! frame), the schedule row's `SELECT … FOR UPDATE` inside the tick //! tx as the row lock that serializes rogue second tickers (ADR-009 @@ -679,12 +688,26 @@ pub(crate) fn run_schedules(stop: StopToken, ctx: EngineCtx) -> BoxedFuture<'sta let _ = lease.release().await; return Ok(()); } - let renewed = lease.renew(LEADER_TTL_S).await.unwrap_or(false); - if !renewed { - // The lock was lost (expired and re-acquired elsewhere - // — exclusion lapses silently at TTL expiry). Return - // before ticking: never fire on a stolen lease. - return Err(Error::LeadershipLost); + let renewed = lease.renew(LEADER_TTL_S).await; + match renewed { + Ok(true) => {} + Ok(false) => { + // The lock was lost (expired and re-acquired + // elsewhere — exclusion lapses silently at TTL + // expiry). Return before ticking: never fire on a + // stolen lease. + return Err(Error::LeadershipLost); + } + Err(e) => { + // A failed renewal is not a leadership loss: the + // lock semantics were never reached. The closed- + // store arm (the engine-wide `Database` posture — + // respawning a leader against a dead store is + // misdirected, review 004 item 9) and every other + // renewal failure propagate their opaque + // classification unchanged. + return Err(e); + } } #[cfg(test)] let soonest = tick_with_retries(&pool, &schema, Some(&tick_fault)).await?; diff --git a/alkstore-postgres/src/store/scheduler_tests.rs b/alkstore-postgres/src/store/scheduler_tests.rs index 003729d..a55fb0d 100644 --- a/alkstore-postgres/src/store/scheduler_tests.rs +++ b/alkstore-postgres/src/store/scheduler_tests.rs @@ -1713,3 +1713,171 @@ async fn tick_retry_exhaustion_exits_typed_and_leaves_the_lock_to_lapse() { drop_schema(&fx.admin, &fx.schema).await; } + +/// Acceptance: the closed-store renewal arm reports the engine-wide +/// closed-store `Database` classification — never `LeadershipLost` +/// (review 004 item 9: flattening every renew `Err` into the loss arm +/// misdirected the documented respawn recipe at a dead store — an +/// operator respawns a leader against a store that is closed; the +/// lock semantics were never reached when the machinery itself +/// errors, the 2026-10-11 classification). The runner runs on the +/// fixture store's own closed flag and pool (the `engine_ctx` seam); +/// the runner closes mid-run — the next loop-top renewal sees the +/// closed flag and the classification propagates unchanged. The +/// SQLite twin's pin shape (`closed_store_run_reports_the_closed_ +/// classification_not_leadership_lost`). +#[tokio::test(flavor = "multi_thread")] +async fn closed_store_run_reports_the_closed_classification_not_leadership_lost() { + let Some(fx) = Fixture::open(&instance_namer("so_cls")).await else { + eprintln!("skip: no harness server"); + return; + }; + + fx.store + .schedule("beat", "@every 1s", "closedq", json!(1), Default::default()) + .await + .unwrap(); + + let ctx = fx.store.engine_ctx(); + let runner = + tokio::spawn(async move { crate::scheduler::run_schedules(StopToken::new(), ctx).await }); + + // Cross the renew arm at least once: the loop is demonstrably + // mid-run (one boundary fired) before the store closes. + let deadline = tokio::time::Instant::now() + Duration::from_secs(20); + loop { + let fired: i64 = fx + .admin + .query_one( + &format!( + "SELECT count(*) FROM {} WHERE queue = 'closedq'", + crate::schema::QualifiedTable { + schema: &fx.schema, + name: crate::schema::tables::JOB, + } + ), + &[], + ) + .await + .unwrap() + .get(0); + if fired >= 1 { + break; + } + assert!( + tokio::time::Instant::now() < deadline, + "the runner never fired the first boundary" + ); + tokio::time::sleep(Duration::from_millis(200)).await; + } + + fx.store.close(); + + // The exit is prompt (the sleep is at most one 1 s boundary wide; + // the next loop-top renewal sees the closed store). + let outcome = tokio::time::timeout(Duration::from_secs(10), runner) + .await + .expect("the closed-store exit must terminate promptly") + .unwrap(); + match outcome { + Err(Error::Database(source)) => { + assert!( + source.to_string().contains("closed"), + "the closed-store arm carries the closed source chain: {source}" + ); + } + Err(Error::LeadershipLost) => { + panic!("a closed store must never report LeadershipLost (review 004 item 9)") + } + other => { + panic!("the closed-store arm reports the closed Database classification, got {other:?}") + } + } + + drop_schema(&fx.admin, &fx.schema).await; +} + +/// Acceptance: the genuine mid-run leadership-loss arm keeps its +/// disposition — `Err(LeadershipLost)`. Deleting the leadership lock +/// row while the runner runs (the expired-and-re-acquired-elsewhere +/// shape — exclusion lapses silently at TTL expiry, ADR-008 §7) makes +/// the next loop-top renewal report the refused-lease outcome and the +/// loop returns the loss before any tick; the respawn recipe's arm is +/// unchanged by the classification. The SQLite twin's pin shape +/// (`mid_run_lease_loss_still_returns_leadership_lost`). +#[tokio::test(flavor = "multi_thread")] +async fn mid_run_lease_loss_still_returns_leadership_lost() { + let Some(fx) = Fixture::open(&instance_namer("so_lss")).await else { + eprintln!("skip: no harness server"); + return; + }; + + fx.store + .schedule("beat", "@every 1s", "lossq", json!(1), Default::default()) + .await + .unwrap(); + + let runner_store = open_store(&harness_dsn().unwrap(), test_opts(&fx.schema)) + .await + .unwrap(); + let runner = tokio::spawn(async move { runner_store.run_schedules(StopToken::new()).await }); + + // Demonstrably mid-run: leadership held, one boundary fired. + let deadline = tokio::time::Instant::now() + Duration::from_secs(20); + loop { + let fired: i64 = fx + .admin + .query_one( + &format!( + "SELECT count(*) FROM {} WHERE queue = 'lossq'", + crate::schema::QualifiedTable { + schema: &fx.schema, + name: crate::schema::tables::JOB, + } + ), + &[], + ) + .await + .unwrap() + .get(0); + if fired >= 1 { + break; + } + assert!( + tokio::time::Instant::now() < deadline, + "the runner never fired the first boundary" + ); + tokio::time::sleep(Duration::from_millis(200)).await; + } + + // The rogue deletion: the runner's lease row vanishes. + let deleted = fx + .admin + .execute( + &format!( + "DELETE FROM {} WHERE name = $1", + crate::schema::QualifiedTable { + schema: &fx.schema, + name: crate::schema::tables::LOCKS, + } + ), + &[&crate::scheduler::SCHEDULER_LEADER_LOCK], + ) + .await + .unwrap(); + assert_eq!( + deleted, 1, + "the leadership lock row must exist to be deleted mid-run" + ); + + let outcome = tokio::time::timeout(Duration::from_secs(10), runner) + .await + .expect("the lease-loss exit must terminate promptly") + .unwrap(); + assert!( + matches!(outcome, Err(Error::LeadershipLost)), + "a deleted lease mid-run must return LeadershipLost: {outcome:?}" + ); + + drop_schema(&fx.admin, &fx.schema).await; +} diff --git a/tasks/pg-fix-scheduler-error-classification.md b/tasks/pg-fix-scheduler-error-classification.md index 3f9c55f..31a9fd5 100644 --- a/tasks/pg-fix-scheduler-error-classification.md +++ b/tasks/pg-fix-scheduler-error-classification.md @@ -1,7 +1,7 @@ --- id: pg-fix-scheduler-error-classification name: Classify run_schedules' renew-failure arms on pg (the sqlite twin, review 004 item 9) -status: pending +status: completed depends_on: [] scope: narrow risk: low @@ -77,8 +77,56 @@ a mid-run close exits with the closed `Database` classification, never ## Notes -> To be filled by implementation agent +- **No discrepancy with the decisions of record**: the sqlite task's + arm set implemented verbatim — `Ok(false)` keeps + `Err(LeadershipLost)` return-before-tick (the respawn arm), + `Err(e)` propagates unchanged (no new variant, no re-deciding; the + `Closed`-stays-wake-receiver posture untouched). +- **Harness server (was the recorded blocker)**: the `pglo-poc` + container was up on :15432; the standard env + (`ALKSTORE_PG_HOST/PORT/USER/PASSWORD/DB=…/15432/postgres/poc/blobs`) + ran the whole pg suite live — nothing was held back to the + server-less skip posture. +- **Engine note (how the closed arm arrives)**: pg's closed-store + classification is *proactive* — `PgLockHandle::renew` checks the + store's shared `closed` flag before any SQL (locked.rs closed-sign + first, the sqlite twin's shape; pinned by pg lock_tests) — so the + test drives it through the fixture store's own `EngineCtx` + (`run_schedules` runs on `fx.store.engine_ctx()`, sharing the + closed flag and pool; `close()` then flips the runner's flag), not + through a second store instance the way the sqlite pin's + same-file DB works. The genuine-loss arm drives through the + runner-store pattern (second `open_store` on the same dsn/schema) + with an admin-client `DELETE` of the lock row — the twin's rogue + deletion shape. +- **Doc alignment**: the pg scheduler's module doc got the arm-set + sentence (the pg-specific text this criterion calls out) — the + SQLite twin's wording (`Ok(false)` = the loss / `Err(e)` = + opaque-propagate, review 004 item 9). ADR-009 §1 and + core-contract.md §Errors verified engine-neutral as landed + (2026-10-11, dated annotation; no engine names) — cited, not + touched; engine-postgres.md's scheduler row doesn't describe the + renew arm, so it needs nothing. ## Summary -> To be filled on completion \ No newline at end of file +The pg leader loop's top-of-iteration renew no longer flattens a +failed renewal into `LeadershipLost` (`scheduler.rs` — the +`unwrap_or(false)` the sqlite task recorded): `Ok(false)` keeps the +`Err(LeadershipLost)` respawn arm unchanged, and a failed renewal +propagates its opaque `Database` classification unchanged (the +closed-store arm arrives pre-classified — the engine-wide "the store +is closed" source, the proactive closed-sign posture) — respawning a +leader against a dead store is no longer the pg outcome either; the +sqlite/pg twins now classify identically. Two acceptance tests pin +the arms against the harness server, modeled on the sqlite pair: +`closed_store_run_reports_the_closed_classification_not_leadership_ +lost` (the runner runs on the fixture's own EngineCtx and the store +closes mid-run — the exit is the closed `Database` classification, +never `LeadershipLost`) and `mid_run_lease_loss_still_returns_ +leadership_lost` (a deleted lease row mid-run still returns the +loss, the respawn arm unchanged). Verified: full +`cargo test -p alkstore-postgres` green on the live harness server +(133 lib + 25 contract suite + 9 schema tests), workspace gates green +server-less, clippy `-D warnings` clean, `cargo fmt --check` clean, +doc gate clean, fuzz corpus replay (6/6) green. \ No newline at end of file