112 lines
5.2 KiB
Markdown
112 lines
5.2 KiB
Markdown
---
|
|
id: pg-fix-scheduler-resilience
|
|
name: Scheduler runner resilience — quarantine bad rows, retry transient errors (review 002 Finding 5)
|
|
status: pending
|
|
depends_on: []
|
|
scope: narrow
|
|
risk: medium
|
|
impact: component
|
|
level: implementation
|
|
tags: [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
|
|
|
|
> To be filled by implementation agent
|
|
|
|
## Summary
|
|
|
|
> To be filled on completion |