diff --git a/alkstore-sqlite/src/scheduler.rs b/alkstore-sqlite/src/scheduler.rs index e445ff5..38e7845 100644 --- a/alkstore-sqlite/src/scheduler.rs +++ b/alkstore-sqlite/src/scheduler.rs @@ -13,8 +13,14 @@ //! - `run_schedules(stop)` — the leader loop (ADR-009 §1): acquire the //! leadership lock `__alkstore_scheduler` (the engine's own lock //! machinery with a derived engine-internal TTL), then loop {renew — -//! on loss return `Err(LeadershipLost)` *before ticking*; tick; sleep -//! until the next due boundary or stop}. The tick's fire enqueues and +//! on a refused renewal return `Err(LeadershipLost)` *before +//! ticking* (the lease expired and re-acquired elsewhere; 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, +//! review 004 item 9 — and any other storage failure; 2026-10-11 +//! classification); tick; sleep until the next due boundary or stop}. +//! The tick's fire enqueues and //! the boundary advance commit in one `BEGIN IMMEDIATE` transaction — //! writer serialization as the row lock on the schedule row (ADR-009 //! §4's layer (b): even a rogue second ticker observing the same @@ -213,12 +219,26 @@ pub(crate) fn run_schedules( 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 with + // the "the store is closed" source — 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); + } } let soonest = { let writer = writer.clone(); diff --git a/alkstore-sqlite/src/store/scheduler_tests.rs b/alkstore-sqlite/src/store/scheduler_tests.rs index 8ed9ec8..e9d9ff8 100644 --- a/alkstore-sqlite/src/store/scheduler_tests.rs +++ b/alkstore-sqlite/src/store/scheduler_tests.rs @@ -599,3 +599,145 @@ async fn failed_tick_commit_replenishes_the_writer_slot() { drop(concrete); cleanup(&dir); } + +/// 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, 2026-10-11 classification). The runner closes mid-run; the +/// next loop-top renewal fails and the classification propagates +/// unchanged. +#[tokio::test(flavor = "multi_thread")] +async fn closed_store_run_reports_the_closed_classification_not_leadership_lost() { + let dir = temp_dir("sched-closed"); + let concrete = open_store(dir.join("store.db").to_str().unwrap(), Default::default()).unwrap(); + + concrete + .schedule("beat", "@every 1s", "closedq", json!(1), Default::default()) + .await + .unwrap(); + + let runner = tokio::spawn(crate::scheduler::run_schedules( + StopToken::new(), + concrete.writer.clone(), + concrete.closed.clone(), + concrete.db_path.to_string_lossy().into_owned(), + concrete.commit_fault.clone(), + )); + + // 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 = { + let conn = rusqlite::Connection::open(dir.join("store.db")).unwrap(); + conn.query_row( + "SELECT count(*) FROM __alkstore_live WHERE queue = 'closedq'", + [], + |r| r.get(0), + ) + .unwrap() + }; + if fired >= 1 { + break; + } + assert!( + tokio::time::Instant::now() < deadline, + "the runner never fired the first boundary" + ); + tokio::time::sleep(Duration::from_millis(100)).await; + } + + concrete.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(concrete); + cleanup(&dir); +} + +/// Acceptance: the genuine mid-run leadership-loss arm keeps its +/// disposition — `Err(LeadershipLost)`. Deleting the lease 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 2026-10-11 classification. +#[tokio::test(flavor = "multi_thread")] +async fn mid_run_lease_loss_still_returns_leadership_lost() { + let (dir, store) = open_test_store("sched-midrun-loss"); + + store + .schedule("beat", "@every 1s", "lossq", json!(1), Default::default()) + .await + .unwrap(); + + let runner_store = + open_store(dir.join("store.db").to_str().unwrap(), Default::default()).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 = { + let conn = rusqlite::Connection::open(dir.join("store.db")).unwrap(); + conn.query_row( + "SELECT count(*) FROM __alkstore_live WHERE queue = 'lossq'", + [], + |r| r.get(0), + ) + .unwrap() + }; + if fired >= 1 { + break; + } + assert!( + tokio::time::Instant::now() < deadline, + "the runner never fired the first boundary" + ); + tokio::time::sleep(Duration::from_millis(100)).await; + } + + // The rogue deletion: the runner's lease row vanishes. + { + let conn = rusqlite::Connection::open(dir.join("store.db")).unwrap(); + conn.execute( + "DELETE FROM __alkstore_locks WHERE name = ?1", + [crate::lock::SCHEDULER_LEADER_LOCK], + ) + .unwrap(); + } + + 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(store); + cleanup(&dir); +} diff --git a/alkstore/src/store.rs b/alkstore/src/store.rs index 8bfaf64..5df89cc 100644 --- a/alkstore/src/store.rs +++ b/alkstore/src/store.rs @@ -127,7 +127,13 @@ pub trait Store: Send + Sync { /// boundaries; sleep until the next boundary or `stop`}. A /// cancelled [`StopToken`] ends the sleep and returns the clean /// `Ok(())`; leadership loss returns `Err(LeadershipLost)` — the - /// caller's recipe is to respawn (ADR-009 §1). + /// caller's recipe is to respawn (ADR-009 §1). A *failed* renewal + /// is not a loss and never returns `LeadershipLost`: a closed + /// store (respawning a leader against a dead store is misdirected + /// — the engine-wide closed-store posture) and storage failure + /// both surface the opaque `Err(Database)` fallback with the + /// engine detail preserved via the source chain (annotated + /// 2026-10-11 — review 004 item 9; ADR-017 §2 class 1). fn run_schedules<'a>(&'a self, stop: StopToken) -> BoxedFuture<'a, Result<()>>; /// Closure-scoped transaction — a wrapper over [`Store::begin_tx`] diff --git a/docs/architecture/core-contract.md b/docs/architecture/core-contract.md index 6e1bc89..07c8434 100644 --- a/docs/architecture/core-contract.md +++ b/docs/architecture/core-contract.md @@ -605,7 +605,15 @@ and `LeadershipLost` (`run_schedules` return on leadership loss) — both from [ADR-009](decisions/009-scheduler-collapse.md) §6 — are part of contract v1's initial text (pre-release additions per [ADR-017](decisions/017-contract-versioning.md) class 1; the -"post-v1 additions" framing an earlier draft used is retired). Pinning rule: a variant exists only when +"post-v1 additions" framing an earlier draft used is retired). +`LeadershipLost` is the *loss* arm only — the lease expired and was +re-acquired elsewhere; a *failed* renewal is not a loss and never +returns `LeadershipLost`: a closed store (respawning a leader +against a dead store is misdirected — the engine-wide closed-store +posture) and storage failure both surface the opaque `Database` +fallback with the engine detail preserved via the source chain +(annotated 2026-10-11 — review 004 item 9; ADR-009 §1's dated +classification note). Pinning rule: a variant exists only when callers can act differently on it. Queue claim "no work" and job/lock boolean results are values, not errors; dead-letter is observable state (`get_job`), not an error. diff --git a/docs/architecture/decisions/009-scheduler-collapse.md b/docs/architecture/decisions/009-scheduler-collapse.md index 2933cd0..1738713 100644 --- a/docs/architecture/decisions/009-scheduler-collapse.md +++ b/docs/architecture/decisions/009-scheduler-collapse.md @@ -97,6 +97,19 @@ store.run_schedules(stop) -> Result<()> // runs until `stop` on it. (One taxonomy note: `LeadershipLost` is a *return-shape* variant of the runner, pinned by the same act-differently rule as ADR-008 §5 — the caller acts on it by respawning.) + *(Annotated 2026-10-11 — review 004 item 9; amend-in-place per + ADR-017 §2 class 1, no publish tag exists.)* The loss arm is the + refused renewal (`renew` → `Ok(false)`): the lease expired and was + re-acquired elsewhere. A *failed* renewal is not a loss and never + returns `LeadershipLost` — the lock semantics were never reached: + a closed store (respawning a leader against a dead store is + misdirected; the engine-wide closed-store posture applies) and + storage failure both surface the opaque `Database` fallback with + the engine detail preserved via the source chain. Engines whose + renew is infallible (`Ok(bool)` only) have no failure arm to + classify; engines whose renew can fail must propagate the `Err` + unchanged (the SQLite engine's pre-annotation flattening — + `unwrap_or(false)` — was the finding). - Schedules do not fire until a consumer runs `run_schedules` — the opt-in is explicit, the no-ambient-timers posture holds, [ADR-002](002-feature-scope.md)'s scheduler evidence is served, and diff --git a/docs/architecture/engine-sqlite.md b/docs/architecture/engine-sqlite.md index 031ed5f..f6cd136 100644 --- a/docs/architecture/engine-sqlite.md +++ b/docs/architecture/engine-sqlite.md @@ -83,7 +83,7 @@ per-contract obligations live in the core spec and are not restated here. | streams | the forked substrate's stream machinery (inherited near-verbatim — the `__alkstore_stream` table's field set is already the contract's shape (its `topic` column carries the contract's `stream` field — the one name delta, ADR-012 §3's fidelity posture), the offset-ASC read path, keyed publish's nullable key column, monotone offset saves ([ADR-015](decisions/015-streams-depth.md)); explicit offset saves through the tx seam; `trim_to` is a `DELETE FROM … WHERE offset <= ?` inside the writer-slot lease) | | queues | owned queue machinery (forked substrate re-derived on [ADR-010](decisions/010-queue-semantics-depth.md) — stamps, no-stranded-rows sweep, dead-visible `get_job`); ADR-003's ride posture superseded on ownership by [ADR-011](decisions/011-sqlite-substrate-fork.md) | | named locks | the substrate's lock machinery (`lock_renew` carries the renew semantics the guarantee row pins; re-acquire does not refresh TTL — inherited deliberately, [ADR-012](decisions/012-forked-substrate-design.md) §4) | -| scheduler / outbox | collapse shape ([ADR-009](decisions/009-scheduler-collapse.md)): owned scheduler storage (`__alkstore_scheduler_tasks`, forked substrate) carrying `@every` specs (cron machinery not ported); the leader loop pattern (TTL lock, heartbeat, exit-before-tick-on-loss); the leadership lock is `__alkstore_scheduler`; outbox = helper over queues with the derived backing-queue name ([ADR-008](decisions/008-contract-v1-pinning.md) §4) | +| scheduler / outbox | collapse shape ([ADR-009](decisions/009-scheduler-collapse.md)): owned scheduler storage (`__alkstore_scheduler_tasks`, forked substrate) carrying `@every` specs (cron machinery not ported); the leader loop pattern (TTL lock, heartbeat, exit-before-tick-on-loss); the leadership lock is `__alkstore_scheduler`; outbox = helper over queues with the derived backing-queue name ([ADR-008](decisions/008-contract-v1-pinning.md) §4). The loss decision is one owner — the loop's top-of-iteration renew: a refused renewal (`Ok(false)`, lease expired and re-acquired elsewhere) exits with `LeadershipLost`; a *failed* renewal is not a loss and propagates its opaque `Database` classification unchanged — the closed-store posture (a dead store must not misdirect the respawn recipe, review 004 item 9, annotated 2026-10-11 per [ADR-009](decisions/009-scheduler-collapse.md) §1) and every other renewal failure | | begin_tx | acquires the writer slot, opens `BEGIN IMMEDIATE`, returns the caller-held handle ([ADR-007](decisions/007-transactional-seam.md)) | | handle ops | each `*_tx` op round-trips `spawn_blocking` to the same connection (thread-affinity note in [ADR-007](decisions/007-transactional-seam.md)) | | commit/rollback | releases the writer slot | diff --git a/docs/architecture/queues.md b/docs/architecture/queues.md index c05de67..62d6b26 100644 --- a/docs/architecture/queues.md +++ b/docs/architecture/queues.md @@ -207,8 +207,13 @@ Collapsed into queues: no `Scheduler` handle, no schedule objects. the ADR; extension path: a consumer-inventory row first). - `run_schedules` is opt-in: schedules never fire without a runner (the no-ambient-timers posture). The runner: leadership lock → - {renew (on loss, return before ticking — `Err(LeadershipLost)`, - distinct from clean `Ok(())` stop) → fire due boundaries → sleep}. + {renew (an actual lease loss — the refused renewal — returns before + ticking with `Err(LeadershipLost)`, distinct from clean `Ok(())` + stop; a *failed* renewal is not a loss — closed store and storage + failure surface the opaque `Err(Database)` classification, a dead + store must not misdirect the respawn recipe, annotated 2026-10-11 + — [ADR-009](decisions/009-scheduler-collapse.md) §1, review 004 + item 9) → fire due boundaries → sleep}. The `stop` parameter is a core-crate cloneable `StopToken` ([ADR-019](decisions/019-mechanism-handle-surfaces.md) §4). Leadership lock: reserved name `__alkstore_scheduler` diff --git a/tasks/sqlite-fix-scheduler-error-classification.md b/tasks/sqlite-fix-scheduler-error-classification.md index 87d3aac..0b645f8 100644 --- a/tasks/sqlite-fix-scheduler-error-classification.md +++ b/tasks/sqlite-fix-scheduler-error-classification.md @@ -1,7 +1,7 @@ --- id: sqlite-fix-scheduler-error-classification name: Classify run_schedules' renew-failure arms (review 004 item 9) -status: pending +status: completed depends_on: [] scope: narrow risk: low @@ -35,17 +35,17 @@ the observable error surface, per AGENTS.md's ADR-wins discipline. ## Acceptance Criteria -- [ ] The arm set is decided and recorded in Notes (which error maps +- [x] The arm set is decided and recorded in Notes (which error maps where, citing the contract text that pins `run_schedules`' error surface); the closed-store arm no longer reports `LeadershipLost` -- [ ] The window state was verified and recorded; a dated annotation +- [x] The window state was verified and recorded; a dated annotation landed if class-1, or user sign-off if post-publish -- [ ] A test pins the closed-store arm (and the genuine +- [x] A test pins the closed-store arm (and the genuine leadership-lost arm's unchanged disposition) -- [ ] The documented respawn recipe's text (wherever it lives) matches +- [x] The documented respawn recipe's text (wherever it lives) matches the new classification -- [ ] `cargo test -p alkstore-sqlite`, clippy `-D warnings`, fmt +- [x] `cargo test -p alkstore-sqlite`, clippy `-D warnings`, fmt clean; workspace gates green server-less ## References @@ -58,8 +58,74 @@ the observable error surface, per AGENTS.md's ADR-wins discipline. ## Notes -> To be filled by implementation agent +- **Window state (verified 2026-10-11)**: `git tag -l` is empty — no + publish tag exists, the class-1 window is open. The classification + landed as a dated ADR-023 §2-style annotation (2026-10-11, + amend-in-place per ADR-017 §2 class 1); post-publish sign-off was + not needed. +- **The arm set (decisions of record)** — citing the contract text + that pins `run_schedules`' error surface (core-contract.md §Errors: + `LeadershipLost` is "`run_schedules` return on leadership loss"; + `Database` is the opaque fallback with engine detail preserved via + the source chain; ADR-009 §1 pins the respawn recipe on the loss): + - `renew` → `Ok(false)` — the genuine loss arm (the lease expired + and was re-acquired elsewhere; exclusion lapses silently at TTL + expiry, ADR-008 §7) — returns `Err(LeadershipLost)`, unchanged, + return-before-tick. The respawn recipe matches on this arm only. + - `renew` → `Err(e)` — the machinery never reached the lock + semantics; not a loss, never `LeadershipLost`. The `Err` + propagates unchanged: the closed-store arm arrives pre-classified + (SqliteLockHandle::renew checks the closed sign first and fails + closed with the engine-wide `Database` posture, source "the + store is closed" — the same classification every other entry + point and the tick's writer-acquire arm already report), and any + other storage failure carries the driver's source chain. + - No new variant minted — ADR-008 §5's act-differently rule holds + (`Closed` stays the wake-receiver variant; closed store is + `Database` engine-wide). +- **Engine notes**: the closed-store entry checks (`run_schedules` + top, lock `renew`/`release`/`try_lock`, tick's `writer.acquire` + via `ok_or_else(closed_store_error)`) already reported `Database`, + so the pre-annotation defect was solely the mid-run + `unwrap_or(false)` flattening. Mem's renew is infallible + (`Ok(bool)`) — no failure arm to classify; the ADR-009 annotation + covers that shape explicitly. +- **Unresolved twin (for a pg follow-up task)**: + `alkstore-postgres/src/scheduler.rs:682` carries the identical + `.unwrap_or(false)` flattening — post-close renew there fails + closed with `Database` (pinned by pg lock_tests) but a mid-run + renewal failure still misreports `LeadershipLost`. Review item 9 + recorded the sqlite finding only; the contract annotation now + applies to both engines, so pg is divergent until a twin task + discharges it. Not fixed here (task scope is narrow/sqlite; pg + tests need the harness server). +- **Docs aligned**: core-contract.md §Errors, ADR-009 §1 (the dated + annotation of record), queues.md's runner bullet, + engine-sqlite.md's scheduler/outbox map row, core + `Store::run_schedules` doc comment, and the sqlite scheduler's + module doc — all state the loss-vs-failure arm set; the respawn + recipe text elsewhere ("the caller's recipe is to respawn", + matching on `LeadershipLost`) stays accurate since the loss arm is + unchanged. ## Summary -> To be filled on completion \ No newline at end of file +The sqlite leader loop's top-of-iteration renew no longer flattens a +failed renewal into `LeadershipLost`: `Ok(false)` keeps the +`Err(LeadershipLost)` respawn arm unchanged, and a failed renewal +propagates its opaque `Database` classification unchanged (closed +store carries the engine-wide "the store is closed" source) — +respawning a leader against a dead store is no longer the documented +outcome. Landed with the dated (2026-10-11) classification +annotation in ADR-009 §1, core-contract.md §Errors, queues.md, +engine-sqlite.md, and the core/sqlite doc comments. Two new +acceptance tests pin the arms: `closed_store_run_reports_the_closed_ +classification_not_leadership_lost` (a mid-run close exits with the +closed `Database` classification, never `LeadershipLost`) and +`mid_run_lease_loss_still_returns_leadership_lost` (a deleted lease +mid-run still returns the loss, the respawn arm unchanged). Verified: +`cargo test -p alkstore-sqlite` (203+25 green, both new pins pass), +workspace `cargo test` green server-less, clippy `-D warnings` clean, +`cargo fmt --check` clean, doc gate (`RUSTDOCFLAGS="-D warnings"` +cargo doc) clean, fuzz corpus replay green. The pg twin +(`scheduler.rs:682`) is recorded in Notes as a follow-up. \ No newline at end of file