alkstore/tasks/pg-fix-scheduler-resilience.md

id, name, status, depends_on, scope, risk, impact, level, tags

id	name	status	depends_on	scope	risk	impact	level	tags
pg-fix-scheduler-resilience	Scheduler runner resilience — quarantine bad rows, retry transient errors (review 002 Finding 5)	completed		narrow	medium	component	implementation	wave-4-fixes postgres-engine scheduler

Description

Fix the LOW/MEDIUM finding from the wave-4 general review (docs/reviews/002-wave-4-general-review.md §Finding 5): one bad row or one transient error permanently kills run_schedules. Sites: alkstore-postgres/src/scheduler.rs:403 (parse_every_interval(&row.spec)?), :429 (fire enqueue_row(...)?), :519-526 (the in_tx tick ?) — all propagating straight out of the leader loop. The @every grammar is re-parsed from stored text per tick (unvalidated-on-read), so a tampered or future-foreign spec row makes the whole runner exit with InvalidSpec; one transient pool/database error does the same. The consumer's respawn recipe makes this survivable but brittle — schedules never fire without a runner, and the runner dies on any hiccup rather than skipping/retrying. The exiting error also carries no schedule-name context.

Fix shape (the review's, made concrete):

• Bad-row quarantine, not fatality: a stored spec that fails re-parse is quarantined — log (eprintln!, the engine's diagnostic posture) with the schedule name, advance the row's next_fire_at past now (the tick's skip-forward helper — the row must not spin), and continue with the remaining due rows. The tick tx still commits (the quarantine advance is a normal write). Re-validate-on-read at registration is unnecessary (specs are pre-validated at schedule()); quarantine is only the tampered/ foreign-row arm.
• Transient-error retry: a tick that fails with a pool/database error retries N times (pin N — 3 is the natural floor) with a short backoff before giving up and returning Err from the loop (the consumer's respawn recipe remains the last resort, but a single hiccup no longer kills the runner). Leadership renewal continues to own the loss decision — a retry must not mask a lost lease (the renew check at the loop top already runs before each tick).
• Error context: the exiting error's source chain carries the schedule name / tick phase (via the database_error message shape — Database Display is opaque; the chain is the carrier).
• Exit-path leadership honesty: on the Err exit path the leadership lock is not released — the TTL lapse covers it. Say so in the module docs (the review's fine print); opportunistically releasing on the error exit is acceptable if it stays simple, but the documented TTL-lapse posture is the bar.
• Doc-comment fix (Finding 6's scheduler bullet, same file): scheduler.rs:46-53 claims "a newly-registered schedule is noticed by the re-read no later than the next slice (1 s)" — false: the 1 s slices only renew the lease and check the stop token; the soonest is never re-read mid-sleep. An idle runner notices a new schedule up to IDLE_SLEEP_S (60 s) late; an active runner waits for the current soonest deadline. Correct the comment to the honest posture (the 60 s idle-tick floor is already acknowledged two paragraphs down as acceptable — the doc comment above it contradicts it). Implementing the per-slice soonest re-check is optional (a behavior improvement, not required by this task) — if done, the corrected comment describes it.

Tests: a tampered spec row is quarantined (the runner survives, other schedules still fire, the bad row's boundary advanced — behavioral, against the harness server, tampering via a direct SQL UPDATE); the retry policy pinned (unit-level pin of the retry count/backoff shape is the bar; a behavioral transient-error injection is welcome if a deterministic arrangement exists); the doc-comment claim matches the code.

Acceptance Criteria

• A tampered/foreign spec row is quarantined (logged with name, boundary advanced past now, other schedules still fire, runner survives) — pinned by behavioral test
• A transient tick error is retried N times (pinned N) with backoff before the loop returns Err — policy pinned by test
• The exiting error's chain carries schedule-name/tick-phase context
• The error-exit leadership posture (TTL lapse covers the unreleased lock) stated in the module docs
• The scheduler.rs:46-53 doc claim corrected to the honest slice/idle posture
• Existing scheduler tests stay green (clean stop, LeadershipLost, catch-up cap, boundary math)
• cargo test -p alkstore-postgres (harness server), clippy -D warnings, fmt clean; gates green server-less

References

• docs/reviews/002-wave-4-general-review.md §Finding 5 + §Finding 6 (the scheduler doc bullet)
• docs/architecture/decisions/009-scheduler-boundaries.md (the boundary guarantee the quarantine advance must preserve)
• docs/architecture/decisions/010-queue-stamps-and-backoff.md §3
• tasks/pg-engine-scheduler-outbox.md (the scheduler's task of record)
• alkstore-sqlite/src/scheduler.rs (the structural twin — check its error posture for parity notes)

Notes

• Retry budget pinned at N = 3 (TICK_RETRIES), retry beyond the first attempt (4 total); backoff doubling 250 ms → 500 ms → 1 s (tick_retry_backoff, capped at 1 s; the full retry window adds ≤ 1.75 s of stop latency). Every tick error at the loop's tick site is pool/database-shaped (Error::Database), so the retry is unconditional — LeadershipLost can never originate there, and a lost lease stays the top-of-iteration renew's decision (documented explicitly: a mid-retry lapse is caught by the renew on the next iteration, before any tick).
• Quarantine advance mechanism: the task said "the tick's skip-forward helper" — since an unparseable spec yields no interval, the helper runs at the minimum interval (skip_forward(next_fire_at, now, 1)), so the advance is strictly past now and monotone (no within-tick spin). Consequence noted in the module docs: the quarantined row re-enters the due set on every subsequent tick and is re-quarantined (and re-logged) each time — the runner keeps a ≥1 s cadence while the stored spec is broken rather than failing; repairing/unscheduling the row stops the noise.
• Error-context carrier: Database's Display is opaque, so a plain database_error(msg) would drop the driver detail along with losing the source chain. Implemented as context_error: the exiting error is Database → io::Error (context message) → original error → driver chain — the message carries schedule name / tick phase AND the full driver chain is preserved. Wrapped sites: the fire enqueue and the boundary advance inside tick (schedule name + phase), and the loop's retry-exhausted exit (tick phase).
• Exit-path leadership: documentation only (the TTL-lapse posture is the bar); no opportunistic release — keeping the exiting path simple per the task's fine print.
• Per-slice soonest re-check not implemented (the task's optional arm): the module doc now states the honest posture (1 s slices only renew the lease and check the stop token; new schedules noticed at the current soonest deadline, or up to 60 s late when idle).
• No behavioral transient-error injection test: no deterministic arrangement exists that wouldn't depend on timing (server kills race the retry window); the unit-level pin of count/backoff shape is the bar and landed (the_tick_retry_policy_is_pinned, server-less).
• SQLite twin parity: alkstore-sqlite/src/scheduler.rs retains the original propagate-straight-out posture (its substrate tick has the same re-parse-from-storage exposure). The task scoped the fix to the pg engine; if the twins' error postures must agree at wave 5's cross-engine suite, follow up there.

Summary

Landed the scheduler runner resilience fix in alkstore-postgres/src/scheduler.rs (review 002 Finding 5 + the scheduler bullet of Finding 6): (1) bad-row quarantine — a due row whose stored spec fails @every re-parse is logged with its name, has its boundary advanced strictly past now via the skip-forward helper at minimum interval, and the remaining due rows proceed (the tick tx still commits); (2) transient-error retry — the loop's tick (tick_once hoisted from the inline body) retries 3× with a short doubling backoff (250 ms → 1 s cap) before the loop exits Err; (3) error context — schedule-name/tick-phase context rides the exiting error's source chain via the ErrorContext wrapper (source chain preserved, unlike a plain message remap); (4) exit-path leadership honesty — the module docs state the TTL-lapse posture for the unreleased lock; (5) the false scheduler.rs:46-53 doc claim corrected (slices renew the lease/check the stop token only; new schedules noticed at the current soonest deadline or up to 60 s late when idle). Tests: behavioral quarantine pin (tampered_spec_row_ is_quarantined_and_the_runner_survives — tampered via direct SQL UPDATE, runner survives to a clean Ok(()), other schedule fires, bad row never fires, boundary advanced) and the server-less retry-policy pin (the_tick_retry_policy_is_pinned). Verified: full cargo test -p alkstore-postgres green against the harness server (121 lib + 10 contract_suite + 9 schema, including all pre-existing scheduler tests), workspace cargo build/cargo test, cargo clippy --all-targets -- -D warnings, cargo fmt --check all green server-less.