alkstore/tasks/review-004-fixes.md

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

id	name	status	depends_on	scope	risk	impact	level	tags
review-004-fixes	Review gate — review-004 fix batch (recorded-findings dispositions)	completed	sqlite-fix-seam-panic-safety sqlite-fix-watcher-death-subscribe sqlite-fix-listener-bridge-alignment sqlite-fix-scheduler-error-classification sqlite-substrate-safety-comments pg-fix-forwarder-hardening pg-fix-forwarder-diagnosability decide-log-facade-adoption pg-decide-stamp-arithmetic-side pg-fix-bigint-stamp-overflow pg-fix-scheduler-error-classification mem-doc-growth-outbox-notes core-pin-offset-initial-value core-fix-display-log-forging docs-pg-identifier-truncation-note normalize-tokio-dev-deps	broad	low	phase	review	review-004-fixes review-gate

Description

The fix batch's own gate (the review-wave-4-fixes pattern): review 004's sixteen recorded findings were decomposed into sixteen tasks (items 2+3 and 4+6 bundled; item 5 split into the decision task and its implementation; the pg scheduler-classification twin added at the sqlite task's implementation); verify each disposition is real — code-read against the review's failure mechanism, not test-name-trust — and that the fixes didn't damage the postures review 004's inline fixes pinned.

Check, per finding (the review's §"Findings recorded, not fixed" numbering):

1. Seam panic-safety: the abandonment plumbing is real (a panic injected in a seam closure leaves the writer slot grantable and the reader pool intact — the test injects, it doesn't simulate); the design of record in Notes matches what landed.
2. Watcher death + initial-stat: post-death subscribe fails closed (not parks); the identity-unknown state survives the first successful stat; pre-death close-at-death pins green.
3. Bridge alignment: the listen bridge is std-thread shaped; the two bridges' residual difference (if any) named and justified.
4. Scheduler classification: the closed-store arm no longer reports LeadershipLost on either engine — the sqlite fix and the pg twin (pg-fix-scheduler-error-classification, surfaced at the sqlite task's implementation) both verified against the landed ADR-009 §1 arm set; the window-state/annotation record is present.
5. Substrate SAFETY comments: every flagged block carries a concrete invariant; the PROVENANCE delta entry registered (the diff-fidelity posture honored — this is the check that matters most here).
6. Forwarder hardening: the TOCTOU restructure is atomic decide-and-queue (code-read the guard/command ordering); the two hang arms bounded; the preserved postures (deadlock pitfalls, retry core, fanout guard, register-failure unregister, poison-recovery idiom) re-verified green.
7. Log facade: the decision of record exists with user sign-off; the diagnosability arms surface failures in the decided form; secret-free grep re-run.
8. BIGINT stamps: the decided guard at the decided sites; the extreme-stamped-row tests run against the live server; no re-deciding happened (or discrepancies recorded).
9. Mem doc notes: the growth/outbox pins present and anchored.
10. Offset pre-yield pin: the three-way verification evidence exists; the contract text pinned (or the divergence raised).
11. Display forging: the control-char pin; taxonomy untouched.
12. Identifier note: in the release text, dated, no-cap verdict untouched.
13. Tokio dev-deps: the three sets identical; tests green.

No regressions: review 004's inline fixes' pinned postures re-spot-checked where the batch touched them — the tick's drop-+-replenish arm, the tx-form negative seed (all three columns), the register-failure unregister, the poison-recovery idiom, the encode depth limit (127), the sleep caps, the doc corrections.

Records: review 004's recorded-findings items annotated with their dispositions/task pointers (the record reads closed); docs/plans/implementation.md gains a review-rounds entry for this batch; task files audited (all dependencies completed, Notes and Summary filled, no placeholder residue).

Gates: full battery — cargo build, cargo test --workspace (server-less), the live pg column vs the harness server (env per review 003's op note), cargo clippy --all-targets -- -D warnings, cargo fmt --check, the fuzz corpus replay, the wasm32 check, and the doc gate.

Acceptance Criteria

• Every finding's disposition verified by code-read/live run against the review's failure mechanism (the per-finding checks above)
• No regressions in the inline fixes' pinned postures
• Records landed (review-004 annotations, implementation.md review-rounds entry, task-file audit)
• All gates re-run green this session (incl. live pg column, fuzz replay, wasm32, doc gate)

References

• docs/reviews/004-pre-release-general-review.md §"Findings recorded, not fixed" (the sixteen items this gate disposes) and §"Findings fixed inline" (the postures re-spot-checked)
• tasks/review-wave-4-fixes.md (the gate pattern)
• docs/plans/implementation.md §Review rounds so far (the record to extend)

Notes

Review performed 2026-10-11 (opencode, glm-5.3-flash). Gate checklist record — every disposition verified against the review's failure mechanism by code-read, not test-name-trust:

• 1 (seam panic-safety): the armor is real, and the test injects rather than simulates — store/seam_tests.rs passes panicking closures directly into the pub(crate) seams; WriterLease/ ReaderLease (seam.rs) own the connection via Option + Drop, and the unwind arm is Writer::replenish() (path-based fresh bootstrapped open, matching the tick's established arm; failed replenish tolerated with a diagnosability event) and Readers::abandon() (outstanding decrement — the accounting precises out because every connection carries its fresh-open increment for life — with the waiter notify under the pool lock). Post-unwind assertions are behavioral: slot re-acquired, max_readers: 1 pool re-acquires, full store op succeeds. The Notes' design of record matches what landed.
• 2 (watcher death + initial-stat): run_poll_loop sets the dead sign before the identity-replacement fatal return, and subscribe fails closed on it and the taken watcher slot atomically with enrollment under the senders lock (the belt-and-braces second arm recorded in Notes, matching). The identity baseline is Option<(u64, u64)> — a failed startup stat leaves it unknown and the first successful stat captures without driving death. Four new tests (substrate + engine-level fail-closed, the injected-failure aliveness drive unix-gated because non-unix stat_identity cannot fail), and the pre-death close-at-death pins checked green.
• 3 (bridge alignment): listen's bridge is now a named std::thread::Builder thread (alkstore-listen-bridge-{channel}) — the stream bridge's shape exactly, including the spawn-failure unsubscribe-and-fail-closed arm; no residual difference is left unjustified (the Notes name the spawn-failure arm as adopted, not residual). listen_after_watcher_death_fails_closed surfaces identically through the aligned bridge.
• 4 (scheduler classification): both engines' loop-top renew is a three-way match — Ok(false) keeps Err(LeadershipLost) return-before-tick, Err(e) propagates its opaque classification (closed store arrives pre-classified Database, engine-wide posture) — the .unwrap_or(false) flattening gone on both, the two pin tests (closed_store_run_reports_the_closed_classification_not_leadership_ lost, mid_run_lease_loss_still_returns_leadership_lost) present on both engines, and the dated 2026-10-11 ADR-009 §1 annotation of record verified engine-neutral.
• 5 (substrate SAFETY comments): all ten unsafe { ctx. get_connection() } sites carry concrete-invariant comments (the eight review-scoped + the same-class schema.rs:104 notify registration + the test-module probe), and the diff-fidelity posture is honored — PROVENANCE delta D-40 (category hygiene) is registered with the derived rusqlite #643 soundness argument. This was the check that mattered most here; it passes.
• 6 (forwarder hardening): the TOCTOU restructure is real decide-and-queue — ChannelSet::enqueue_register/ enqueue_unregister make the refcount decision, the connected read, the generation-tag load, and the command enqueue one critical section under the registry guard (poison-recovered), with the ack awaited outside the guard (never a std lock across an await); the queue's order is thus the registry transitions' linearization. The two hang arms are bounded by bounded_wait (biased toward the shutdown flip): the verify batch and the serve/replay commands ride LISTENER_QUERY_TIMEOUT (5 s), the failed-verify settle rides LISTEN_SETTLE_TIMEOUT (2 s) with an unsettled reaper aborted (the connection closes with it). Preserved postures re-verified green: the register-failure/subscribe-failure unregisters in place (notify.rs both arms), the poison-recovery idiom throughout, the replay-proofs recorded in the task's Notes (the pin fails on the review's exact assert with the fix reverted). The Notes' thread- preemption window analysis matches the landed guard placement.
• 7 (log facade + diagnosability): ADR-025 exists with the user sign-off recorded in its status (tracing adopted, engine crates only, core dep-free and emit-free); zero library-path eprintln! remain (grep), both engines carry tracing = "0.1"; the two diagnosability arms emit in the decided form (warn on failed reconnect connects with attempt/backoff context, error on the poll-exit endings); the secret-free grep re-run clean (the remaining .ok()s are tests' env plumbing).
• 8 (BIGINT stamps): the decided guard at the decided sites as one owner — resolution.rs::saturating_stamp_expr called at exactly the decision's six sites (queue claim/retention/retry/heartbeat, lock insert/renew) with the ($N::bigint)::numeric chain (the live-caught WrongType refinement recorded in the owner's doc); the decision task (ADR-023 §2 dated annotation, window verified open) and the implementation task both green against the harness server, no re-deciding — the one refinement is recorded, not deviated around.
• 9 (mem doc notes): engine-mem.md carries the new growth-postures section (both postures anchored to stream.rs/queue.rs sites) and the outbox-semantics subsection ("processed means delivered-or- reclaimable"); the debug_assert! declined with a real false-positive rationale recorded (in-closure handle ops would trip it).
• 10 (offset pre-yield pin): the three-way verification evidence exists — the suite row receiver_offset_pre_yield_is_zero (four arms incl. the ADR-023 §2 negative-seed arm and the first-observation composition leg), green on all three columns; the contract text pinned in three places (core-contract.md's streams row + verification backlog, the trait doc — no divergence to raise, so the AC's divergent branch correctly unticked as N/A).
• 11 (Display forging): display_safe (per-char escape_debug) in the four Display arms (InvalidName/ReservedName/InvalidSpec
  • Codec, the same-class neighbor the task's grep found); the escaping test pins zero emitted control bytes; the taxonomy (variants/fields/match sites) byte-identical.
• 12 (identifier note): the note is in the release text — deployment.md (dated, at the pg co-tenancy paragraph) and engine-postgres.md — with the accuracy sharpening recorded (the server rejects ≥ 64-byte pg_notify channels; the collision is on the registration side); the no-cap validation verdict untouched.
• 13 (tokio dev-deps): the three engines' [dev-dependencies] tokio feature sets are identical (rt-multi-thread + macros + time + test-util); tests green.

No regressions (re-spot-checked at the touched sites): the tick's drop-+-replenish disposition intact ((out, conn, poisoned) + the commit/rollback-fault seams); the tx-form negative seed intact (pg save_offset_tx stores raw, no GREATEST; mem's has_offset_row probe distinguishes the absent checkpoint); the sleep caps (MAX_SLEEP_S 30-day cap on all three engines); the sqlite claim stamp's saturating_add (scheduler.rs:346); the register-failure/subscribe- failure unregisters; the poison-recovery lock idiom; encode_payload's 127-container depth limit; the doc corrections.

Records landed this gate: review 004's sixteen Findings recorded, not fixed items annotated with their dispositions and task pointers (the record reads closed); implementation.md's review-rounds entry for the batch; the task-file audit — all sixteen dependencies completed, Notes and Summary filled.

Minor records issues found and fixed inline (comment/metadata only):

• stale placeholder lines (> To be filled …) left above real content in two batch task files (sqlite-fix-watcher-death-subscribe, sqlite-fix-listener-bridge-alignment) — removed; the identical pre-existing residue in three earlier completed tasks (review-wave-3, release-readme, and — noted by review-wave-5 itself as the class's precedent — same shape) cleaned in the same stroke.
• completed-AC checkboxes left unticked on five batch tasks (sqlite-fix-seam-panic-safety, sqlite-substrate-safety-comments, mem-doc-growth-outbox-notes, normalize-tokio-dev-deps, pg-fix- scheduler-error-classification) — ticked after this gate verified each; the one intentionally-conditional divergence AC (core-pin-offset-initial-value's) left unticked as N/A.
• the gate task's own description said "fifteen tasks"; corrected to the honest sixteen (the pg twin added mid-batch).

Summary

Review gate passed: all sixteen dispositions verified against review 004's failure mechanisms by code-read; the inline fixes' pinned postures re-spot-checked with no regressions; records landed; all gates green.

Highlights (full checklist in Notes):

• Findings 1–4: the armor/state-machine changes are structurally what the tasks' Notes of record pin (lease Drop arms, the dead sign, the two bridges' aligned shapes, the twin three-way renew matches) — and every injected-panic / death-injection / interleave test drives the real mechanism, not a simulation.
• Finding 5's PROVENANCE delta D-40 (the diff-fidelity posture's purpose) verified; Findings 6–11's behavioral shapes confirmed (decide-and-queue under the registry guard, bounded_wait at every listener await, the saturating_stamp_expr six-site owner, ADR-025's decided emission form, the three-place offset pin, the display escaping).
• The batch's own honest-record trail held up under audit — every Notes section (design of record, form refinements, declined alternatives like mem's debug_assert!) matches what landed — with three minor record-hygiene defects fixed inline (placeholders, AC checkboxes, the description's task count).

Gates this session: cargo build; server-less cargo test --workspace (core 28+3+1 doc, mem 102+26, sqlite 203+26, pg lib SKIP-posture 135+26+9, all green); the full pg column live vs the harness server (pglo-poc :15432 — 135 lib + 26 suite + 9 schema); cargo clippy --all-targets -- -D warnings; cargo fmt --check; the fuzz corpus replay (fuzz/shared, 6/6); the wasm32 mem check; the doc gate (RUSTDOCFLAGS="-D warnings" cargo doc --no-deps, zero warnings); taskgraph validate (96 tasks).