131 lines
6.6 KiB
Markdown
131 lines
6.6 KiB
Markdown
---
|
|
id: sqlite-fix-scheduler-error-classification
|
|
name: Classify run_schedules' renew-failure arms (review 004 item 9)
|
|
status: completed
|
|
depends_on: []
|
|
scope: narrow
|
|
risk: low
|
|
impact: component
|
|
level: implementation
|
|
tags: [review-004-fixes, sqlite-engine, scheduler]
|
|
---
|
|
|
|
## Description
|
|
|
|
The SQLite `run_schedules` error flattening, per
|
|
`docs/reviews/004-pre-release-general-review.md` §"Findings recorded,
|
|
not fixed" item 9: the renew arm maps *any* renew failure — including
|
|
the closed-store `Database` error — to `LeadershipLost`
|
|
(`scheduler.rs:216`, `lease.renew(LEADER_TTL_S).await.unwrap_or(false)`),
|
|
misdirecting the documented respawn recipe at a dead store (an operator
|
|
respawns a leader against a store that is closed).
|
|
|
|
Decide the right arm set and implement it: closed-store vs
|
|
leadership-lost vs storage failure — what `run_schedules` returns on a
|
|
dead store, and what stays `LeadershipLost`.
|
|
|
|
**Contract-adjacent — check the window state first.** This is an
|
|
error-classification change on a stable surface (what error a public
|
|
scheduler entry point returns is contract-observable). As of the
|
|
decomposition (2026-10-11) no publish tag exists, so the class-1
|
|
window is presumed open — verify, and if open, land with a dated
|
|
ADR-023 §2-style annotation of the classification. If the window has
|
|
closed (the user has published), raise with the user before changing
|
|
the observable error surface, per AGENTS.md's ADR-wins discipline.
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [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`
|
|
- [x] The window state was verified and recorded; a dated annotation
|
|
landed if class-1, or user sign-off if post-publish
|
|
- [x] A test pins the closed-store arm (and the genuine
|
|
leadership-lost arm's unchanged disposition)
|
|
- [x] The documented respawn recipe's text (wherever it lives) matches
|
|
the new classification
|
|
- [x] `cargo test -p alkstore-sqlite`, clippy `-D warnings`, fmt
|
|
clean; workspace gates green server-less
|
|
|
|
## References
|
|
|
|
- docs/reviews/004-pre-release-general-review.md §"Findings recorded,
|
|
not fixed" item 9 (the finding, the "record, don't rush" note this
|
|
task discharges)
|
|
- docs/architecture/core-contract.md (the scheduler error surface)
|
|
- alkstore-sqlite/src/scheduler.rs:205-216 (the flattening site)
|
|
|
|
## Notes
|
|
|
|
- **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
|
|
|
|
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. |