280 lines
15 KiB
Markdown
280 lines
15 KiB
Markdown
---
|
||
id: review-004-fixes
|
||
name: Review gate — review-004 fix batch (recorded-findings dispositions)
|
||
status: completed
|
||
depends_on: [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]
|
||
scope: broad
|
||
risk: low
|
||
impact: phase
|
||
level: review
|
||
tags: [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
|
||
|
||
- [x] Every finding's disposition verified by code-read/live run
|
||
against the review's failure mechanism (the per-finding checks
|
||
above)
|
||
- [x] No regressions in the inline fixes' pinned postures
|
||
- [x] Records landed (review-004 annotations, implementation.md
|
||
review-rounds entry, task-file audit)
|
||
- [x] 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). |