Files
alkstore/tasks/sqlite-fix-watcher-death-subscribe.md
T

145 lines
7.3 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
---
id: sqlite-fix-watcher-death-subscribe
name: Dead-watcher subscribe hang + initial-stat self-kill (review 004 items 2+3)
status: completed
depends_on: []
scope: moderate
risk: medium
impact: component
level: implementation
tags: [review-004-fixes, sqlite-engine, watcher]
---
## Description
Two bundled watcher-state defects in the SQLite engine, per
`docs/reviews/004-pre-release-general-review.md` §"Findings recorded,
not fixed" items 2 and 3 (the review says bundle them — both are
watcher-state changes).
**(a) Post-watcher-death subscribe hang (item 2).** After the watcher
thread exits (db-file replacement), `subscribe()` still enrolls new
senders with no poller alive (`substrate/watcher.rs:391-396`): a
`listen()` or stream `subscribe()` on the dead store parks forever,
contradicting engine-sqlite.md's "on watcher death every subscriber
closes" posture — which is enforced only for *pre-death* subscribers.
Fix shape: a dead-store/dead-watcher sign that the new subscription
paths consult — a state-machine change across `store.rs`/`notify.rs`/
`stream.rs`. New subscriptions on a dead store must fail closed (a
typed error, the established failure vocabulary) rather than park.
**(b) Initial-stat failure self-kill (item 3).** An identity-`stat`
failure at watcher start seeds `(0, 0)` (`watcher.rs:164-170`); the
first *successful* later stat mismatches it and drives the death path,
spuriously closing every subscriber on recovery from a transient stat
hiccup. Rare, but the fix — an identity-unknown state instead of a
false identity — is the same watcher-state family: the watcher treats
identity as unknown until the first successful stat, and that first
successful stat must not trigger the death path.
**Test shapes**: (a) kill the watcher (the db-file replacement seam or
a `cfg(test)` death injection), then `listen()`/`subscribe()` — assert
the typed failure, not a park; the pre-death subscribers' close-at-
death pins stay green. (b) inject an initial stat failure, let the
first successful stat land, assert the watcher survives and
subscribers stay open.
## Acceptance Criteria
- [x] Subscribing (listen or stream) after watcher death fails closed
with a typed error instead of parking — pinned by test
- [x] Pre-death subscribers still close at watcher death — the
existing pins green unchanged
- [x] An initial identity-stat failure no longer seeds a false
identity: the first successful stat does not trigger the death
path and subscribers stay open — pinned by injected-failure test
- [x] engine-sqlite.md's watcher-death posture text covers the
new-subscriber arm (and the identity-unknown state if it changes
observable behavior)
- [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" items 2–3 (the findings, the fix shapes, the bundling)
- docs/architecture/engine-sqlite.md (the watcher-death posture)
- alkstore-sqlite/src/substrate/watcher.rs (both sites),
store.rs / notify.rs / stream.rs (the subscription paths)
## Notes
Decisions of record the description didn't pin:
- The dead sign lives on `SharedUpdateWatcher` as an
`Arc<AtomicBool>`, set by `run_poll_loop` itself just before the
watcher-fatal return — it is set *before* the death-guard's senders
clear (which happens after the loop returns), so the guard-side
ordering is fixed by program order.
- `SharedUpdateWatcher::subscribe` became fallible
(`Result<_, String>`) and checks **two** closed arms atomically with
the enrollment under the senders lock: the dead sign *and* the
taken watcher slot (the post-`close` enroll-onto-empty-watcher race
of the same shape). Failing closed on the taken slot is new
belt-and-braces beyond the finding's shape; the store's `closed`
flag already guarded the normal path. Rationale for checking under
the lock: a check-then-enroll done outside the lock could interleave
between the death and the guard's clear and enroll a ghost.
- The substrate stays contract-blind: the failure is a `String`
naming the death, mapped into the taxonomy engine-side —
`Error::Database` (the opaque fallback with the source chain), the
same shape the engine's closed-store checks use. `Error::Closed`
was rejected: the contract docs scope that variant to *receiver*
closed arms (`try_recv`/`recv_timeout`), not entry points.
- The initial-identity fix is an in-place re-derivation of the same
code (`Option<(u64, u64)>` baseline vs. the `(0, 0)` seed); the
death-log message keeps its precise diagnostic (now printing the
captured baseline).
- The injected-failure test for (b) is the substrate-level drive
(`run_poll_loop` with the db file absent at start — the initial
stat genuinely fails; on non-unix `stat_identity` never fails, so
the scenario cannot arise and the test is unix-gated like the other
file-replacement drives); no `cfg(test)` injection seam was added to
`stat_identity` itself. Aliveness after the first successful stat is
proven by wake delivery relative to a pre-insert snapshot (failing
identity stats fire `on_change`, so absolute wake counts are not a
clean liveness signal).
- Also updated: engine-sqlite.md's watcher-failure posture bullet
(both arms), notify.rs/stream.rs/store.rs module docs, open_tests'
three direct `watcher.subscribe()` call sites (now `.unwrap()`d),
and one PROVENANCE.md row (D-39 — the register wants divergence to
the ported watcher recorded even though the task text didn't name it).
## Summary
Both defects landed with tests:
- (a) Post-watcher-death subscribe hang: the poll loop sets the dead
sign before exiting through the identity-replacement death path;
`SharedUpdateWatcher::subscribe` fails closed on it (and on a taken
watcher slot) atomically with enrollment under the senders lock;
`notify.rs::listen` and `stream.rs::spawn_bridge` map the failure to
`Error::Database` with the death named in the source chain. Pinned
by four tests: substrate-level
(`subscribe_fails_closed_after_watcher_death`, unix file-replacement
drive) and engine-level (`listen_after_watcher_death_fails_closed`,
`subscribe_after_watcher_death_fails_closed` — same drive through
the store surface, error-shape + no-enrollment assertions). The
pre-death subscriber close-at-death pins
(`watcher_death_closes_the_receiver_terminally`,
`watcher_death_closes_the_subscription_terminally`, and the
substrate death-signal tests) are green unchanged — the death-signal
substrate test also now pins the dead sign being set.
- (b) Initial-stat self-kill: the loop tracks the identity baseline as
`Option<(u64, u64)>`; a failed startup stat leaves it unknown, the
first successful stat captures it without driving death, and a later
genuine mismatch still kills. Pinned by
`initial_stat_failure_does_not_seed_a_false_identity`
(`run_poll_loop` drive: absent file → unknown baseline → file
created → no death + wake delivery lands → replacement → death
fires; unix-gated).
- Verified: `cargo build`, `cargo test` (workspace, all green),
`cargo test -p alkstore-sqlite` (201 tests), fuzz gate
(`cargo test --manifest-path fuzz/shared/Cargo.toml`), clippy
`-D warnings` clean, `cargo fmt --check` clean, rustdoc
`-D warnings` doc gate clean.