General review — wave 8 / pre-release (whole workspace)

Reviewer: opencode (glm-5.3-flash)
Scope: a general review of the full workspace as it stands at wave-8 completion (2026-10-11, post review-wave-8, pre-publish) — the same lens family as reviews 001/002/003 (correctness, security, code smells, classic dev issues), applied to all four published crates plus the contract suite and the fuzz sidecar, rather than a single wave's delta. Numbering note: reviews 001 (waves 1–2), 002 (wave 4), and 003 (wave 5) already occupy the first three slots, so this record is 004 by the directory's sequence.
Input: four parallel deep-read agents (one per crate: core; sqlite engine + substrate; postgres; mem) plus a cross-cutting sweep (panics/unsafe/logging/security greps, metadata, README / CHANGELOG spot-checks, fuzz/shared), followed by session-level verification of every finding that drove a change — each fixed item below was re-verified by direct read and, where behavioral, by a live run.
Gates at review time (all run this session, after the fixes): cargo build; cargo test --workspace server-less (green; core 27, sqlite 194, mem 102, pg lib SKIP-posture, suite 25 on the sqlite+mem columns); the full pg column live vs the harness server (pglo-poc: 127 lib + 25 suite + 9 schema, all green — the run that re-verified the pg fixes below through the live server); cargo clippy --all-targets -- -D warnings; cargo fmt --check; the fuzz corpus replay (6/6); the wasm32 check (cargo check --target wasm32-unknown-unknown -p alkstore-mem); the doc gate (RUSTDOCFLAGS="-D warnings" cargo doc --no-deps, zero warnings). Harness env for the record (review 003's op note): ALKSTORE_PG_HOST=localhost ALKSTORE_PG_PORT=15432 ALKSTORE_PG_USER=postgres ALKSTORE_PG_PASSWORD=poc ALKSTORE_PG_DB=blobs.

Findings fixed inline

#	Severity	Where	Finding	Fix
1	HIGH	`alkstore-sqlite/src/scheduler.rs`	A failed tick `COMMIT`/`ROLLBACK` left the writer slot wedged with a stuck-open transaction. The tick ran inside `with_writer`, which releases the connection on both arms; SQLite's documented behavior is that a failed `COMMIT` leaves the transaction active — so the released connection carries a live `BEGIN IMMEDIATE` forever: every later auto-commit op silently executes inside an uncommitted transaction (writes vanish at close) and every later `BEGIN IMMEDIATE` errors. Every other commit site has the drop-+-`reopen`-replenish arm (the tx handle's commit, `ConnLease`, the drop paths — pinned by `sqlite-commit-error-arm` tests); the tick was the one site missing it. Also, the tick's error arm swallowed rollback failure (`let _ = ROLLBACK`).	The tick takes the connection by value now and returns a `(result, conn, poisoned)` disposition: `COMMIT` failure or `ROLLBACK` failure marks the connection poisoned — dropped, not released, slot replenished via a fresh bootstrapped open (the established arm's posture). The `cfg(test)` commit-fault seam reaches the tick (the store's flag is threaded into `run_schedules`), and a new test pins the property: `failed_tick_commit_replenishes_the_writer_slot` (`store/scheduler_tests.rs`) injects a `SQLITE_FULL`-shaped `COMMIT` failure and asserts the error chain, slot grantability, and full store usability after.
2	MED (contract-observable)	`alkstore-postgres/src/tx.rs`, `alkstore-mem/src/tx.rs`	The wave-8 gate's pg negative-seed fix missed the tx seam. The gate fixed pg's auto-commit save (raw seed, `CHECK` dropped) against ADR-023 §2's 2026-10-11 annotation ("a negative is stored as the checkpoint") and pinned `save_offset`'s totality through the direct form — but `save_offset_tx` still clamped (`GREATEST($3, 0)`), so the same consumer's first save landed −5 through `StreamHandle::save_offset` and 0 through `TxHandle::save_offset_tx` depending on entry point, with the tx path's doc still citing the pre-annotation clamp rationale. The suite's new legs did not pin the tx form, which is why the gate's audit read passed. Mem had the same family defect in-tx: `merged_get_offset` reported `max(staged, committed)` against a phantom zero for an absent committed checkpoint, so a staged −5 seed read 0 in-tx but committed as −5 (`save_offset_row` seeds raw) — the in-tx read contradicted the merge outcome; SQLite (uncommitted in-tx SQL) was correct.	Both fixed: pg `save_offset_tx` stores `$3::bigint` raw (comment updated to the annotated posture); mem's merged read now distinguishes an absent committed row (new `has_offset_row` probe) — a staged seed reads raw in-tx, an existing checkpoint rides the monotone max as before. A new suite leg pins the tx form end-to-end (staged −5 → raw in-tx read-back → stored −5 at commit → anchored reads resume forward) in `in_tx_reads_see_own_writes`, stamped with the ADR-023 §2 (annotated 2026-10-11) accumulation; green on all three columns against the live pg server. `core-contract.md`'s numeric-domain verification row names the tx-form leg; core-contract's `save_offset_tx` prose was already uniform ("monotone upsert" — the divergence was pg/mem implementation, not contract text).
3	MED	`alkstore-postgres/src/forwarder.rs`, `src/notify.rs`, `src/stream.rs`	A failed `register` leaked a phantom refcount. `register` increments the channel registry first, then faults on three paths (mid-reconnect, forwarder-gone, ack dropped). `listen()` (`notify.rs`) propagated the error without unregistering — and since no receiver was created, no future `Drop` would ever decrement: the last-subscriber UNLISTEN rule could never fire and the reconnect re-issue kept LISTENing a channel nobody consumes. The subscribe-side failure arm two lines below already unregisters ("the acked LISTEN must not leak") — the register-side arm was the miss. `spawn_bridge` (`stream.rs`) had the same shape.	Both call sites now unregister on the register error before returning it (mirroring the existing subscribe-failure arm), and `register`'s doc is corrected: the failure disposition is the call site's (no receiver exists to drop the reference), replacing the old "the registry entry stays either way" claim that codified the leak.
4	MED	`alkstore-postgres/src/forwarder.rs`	Nine non-test `expect()`s in the fanout/registry/config locks — a panic while holding any of them poisoned the mutex and converted every later `register`/`subscribe`/`shutdown` into a panic in unrelated tasks, despite the workspace's no-`expect`-outside-tests rule (and the crate's own seam docs).	Converted to the mem crate's established poison-recovery idiom (`unwrap_or_else(std::sync::PoisonError::into_inner)`) — degraded-but-alive continuation matching `alkstore-mem/src/state.rs`'s locked-discipline posture. The one remaining pg `expect` (`stream.rs` wake-runtime build, a `OnceLock` fixed-config invariant) is recorded, not converted: its failure is a bug, not a condition, and converting it would change the function's signature for no path.
5	MED	`alkstore/src/payload.rs` (+ `fuzz/shared/src/payload_codec.rs`, engine enqueue paths implicitly)	Deeply-nested payload `Value`s aborted the process at encode time. The doc claimed "Unreachable for `serde_json::Value` in practice", but serialization's recursion is unguarded: a caller-supplied `Value` nested ~10⁵ levels overflows the stack inside `serde_json::to_vec` — an abort, not an `Err` — and `encode_payload` runs at every enqueue/publish/notify site on every engine, so one bad `Value` from one consumer kills the runtime. Worse for the pre-change state: the fuzz invariant explicitly pinned that a value nested past the decode recursion limit encodes successfully and only "decodes loud" later — i.e. storing bytes no reader could ever read back, silent data loss at read time. A scratch probe pinned serde_json's decode boundary empirically: 127 nested containers read back, the 128th nested open fails (`RECURSION_LIMIT` 128 counts the failing open, not the last success).	`encode_payload` now measures nesting iteratively (an explicit-stack walk — a recursive checker would itself overflow on exactly the deep inputs) and rejects anything deeper than 127 containers with typed `Error::Codec`: exact encode/decode symmetry (everything decodable encodes; nothing undecodable stores) and no reachable abort path. Applied pre-publish, so the class-1 window covers it as a failure-set addition within ADR-023 §1's typed-failure pin — flagged here for the record's honesty. The fuzz invariant's encode expectations were corrected (past-limit encode fails typed; a boundary round-trip leg at 127 containers added), and two core tests pin the boundary + no phantom behavior (`value_type_tests.rs`). `payload_as`'s decode-side behavior is unchanged.
6	MED (platform-conditional)	`alkstore-mem/src/scheduler.rs`, `alkstore-sqlite/src/scheduler.rs`, `alkstore-postgres/src/scheduler.rs`	`Instant + Duration` panic / unchecked overflow on far-future boundaries. The three leader loops' sleep deadlines computed `tokio Instant + Duration::from_secs(soonest − now)` with `soonest` a consumer-reachable i64 (a legal `@every 9999999999999999s` interval); on macOS (u64-nano `Instant`, ~584 y) or Windows (~29 k y) the addition panics for waits past the platform's representable span (the mem agent verified the panic class; Linux tolerates it). The sleep is a wake-latency optimization — correctness never rides it. Same family: the sqlite tick's claim stamping did an unchecked `now + s` (`expires_s` consumer i64) — debug panic / release wrap, in the one spot whose neighbors all use `saturating_add`.	All three sleeps cap one slice at `MAX_SLEEP_S` (30 days): a far-future boundary sleeps, ticks (a no-op), and sleeps again — the tick recomputes the soonest every iteration and the renewals continue, so a 30-day-cap is behavior-preserving on any platform. The sqlite claim stamp is `now.saturating_add(s)` matching `resolution.rs`'s own posture.
7	LOW	`alkstore-mem/src/wakes.rs`	Stale module-level `#![allow(dead_code)]` whose own comment said "until the mechanism tasks wire it" — the mechanisms wired the bus in wave 6; the suppression was masking the module's real dead-code signal.	Removed (with the now-covered `#[allow(dead_code)]` on `new()`, which the `Default` impl exercises); compiles clean — the surface is genuinely live.
8	LOW (test hygiene, core)	`alkstore/src/value_type_tests.rs`	Two test smells: `payload_as_undecodable_target_yields_codec` exercised a `map_err` the test itself wrote instead of the function under test (it proved nothing); two serialization asserts compared against `to_vec(...).unwrap_or_default()` — a failure in the reference encode would diff against an empty `Vec` and misdirect.	The undecodable-bytes case now goes through `Job::payload_as` on a real undecodable job; the reference encodes panic with their cause.
9	LOW (doc accuracy)	several	Doc-vs-code mismatches found by the read, all public or contract text: core's `Error::Closed` doc scoped the variant to wake receivers while the stream receiver's `try_recv` produces it too; `validation.rs`'s shared-kind bullet was self-contradictory about the whitespace rule and `NameKind::Local` omitted it; `alkstore-sqlite`'s lib header promised "durability knobs … on `SqliteOpts`" although `SqliteOpts` carries only the watcher cadence and reader-pool size (`synchronous`/busy-timeout are bootstrapped constants); pg `lock.rs` claimed the acquire sequence "auto-committed as a unit" though the statements commit individually (correctness lives in the PK constraint, which the doc itself cited second); mem's tx module doc said the commit-merge is "one guarded operation" while the merge is two sequential guarded sections (queues → streams) on a non-awaiting path, and mem's stream doc said publishes fire "one wake per event" where the tx commit path coalesces per stream.	All reworded to match the code. The pg `lock.rs` and mem merge statements now state the real mechanism honestly; the sqlite lib doc now matches `SqliteOpts`'s actual fields; the `Error::Closed` doc covers both receiver families.

Findings recorded, not fixed (follow-up material)

These are real but either need their own design pass, touch lineage or distributed machinery whose repair exceeds a review fix, or are conscious postures to record. None blocks the release; each names its shape so a future session can decompose without re-deriving.

(Dispositions, 2026-10-11: all sixteen items decomposed into the review-004-fixes fix batch — the sixteen tasks/* files below each carry the per-item decisions of record and verification — and the batch's gate audited them all landed. The record reads closed.)

sqlite seam panic-safety (defense-in-depth) — with_writer/with_reader/blocking_read (alkstore-sqlite/src/seam.rs, substrate/schema.rs) release their slot/pool connection after the closure returns only. A panic inside the closure skips the release: the single writer slot wedges forever (Writer::acquire waits on a condvar with no timeout), and an abandoned reader connection permanently shrinks the pool (Readers.outstanding never decrements on abandonment). The tx seam already owns this class (ConnLease); the auto-commit seams don't. No reachable panic path today (the library is panic-free by the standing gates, and these closures take no consumer code), so this is armor, not exposure — but the fix needs abandonment plumbing on both Writer (drop + path-based replenish) and Readers (an outstanding decrement), which is a deliberate small design task, not an inline patch. Disposition: landed via sqlite-fix-seam-panic-safety (RAII WriterLease/ReaderLease guards, drop-not-release, replenish/ abandon dispositions; injected-panic tests in store/seam_tests.rs).
sqlite post-watcher-death subscribe hang — 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 (enforced only for pre-death subscribers). Needs a dead-store/dead-watcher sign that new subscription paths consult — a state-machine change across store.rs/notify.rs/stream.rs. Disposition: landed via sqlite-fix-watcher-death-subscribe arm (a) — the dead sign set before the fatal exit, fallible subscribe failing closed under the senders lock.
sqlite watcher initial-stat failure self-kill — 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 another watcher-state change — bundle with item 2 if either is taken up. Disposition: bundled with item 2 in sqlite-fix-watcher-death- subscribe arm (b) — the Option<(u64, u64)> identity-unknown baseline; the first successful stat captures without driving death.
pg forwarder register/unregister TOCTOU — the registry refcount and the LISTEN/UNLISTEN command emission are not one critical section (forwarder.rs:131-176 vs 438-482): interleaved register/unregister across tasks can leave a live refcount with the server-side LISTEN cleared (both queued commands share a generation, so no stale check saves it) until the next reconnect re-issue. The fix is a decide-and-queue restructure inside the forwarder — forwarder surgery that should ride its own session with the existing test-seam vocabulary. Disposition: landed via pg-fix-forwarder-hardening (a) — ChannelSet::enqueue_register/enqueue_unregister make the refcount decision and the command enqueue one critical section under the registry guard; replay-proven both ways.
pg server-side BIGINT overflow on consumer stamps — the claim, heartbeat, retry-reschedule, and retention-sweep statements compute server-side sums over consumer-supplied i64 values (queue.rs:386/912/1000/714, lock.rs:91): one row stamped with visibility_timeout_s = i64::MAX (or retention i64::MIN) makes every claim/heartbeat UPDATE fail with "bigint out of range", and the failure is sticky per row. The client-size resolution is saturated (resolution.rs); the row-stamp arithmetic is not. Fix shape: clamp/validate at the enqueue seam (or LEAST/GREATEST guard the server expressions) — but the which side question is contract-adjacent (ADR-023 §2's totality wording) and deserves its own decision, not an inline patch. Disposition: pg-decide-stamp-arithmetic-side decided the server expression owns the guard (result-domain saturation, no input clamp); pg-fix-bigint-stamp-overflow landed it as saturating_stamp_expr at the six sites, pinned live.
pg forwarder hang-shaped arms — two narrow ones: a failed generation-verify/LISTEN batch is treated as connection death and parks on poll_task.await forever even though the session may be alive (forwarder.rs:698-702/803-808); and close()/Drop can leak the listener session past shutdown when the serve loop is awaiting a command on a black-holed connection (the run_command await sits outside the shutdown select, forwarder.rs:781, no timeout on this connection). Both want the same treatment family — timeouts/bounded waits on the listener path — bundle into the forwarder-hardening session (item 4). Disposition: both bundled into pg-fix-forwarder-hardening (b/c) — the bounded_wait primitive bounds every listener-path await (LISTENER_QUERY_TIMEOUT 5 s / LISTEN_SETTLE_TIMEOUT 2 s, shutdown-selected), the failed-verify settle aborts at the bound; the black-hole proxy harness test pins the close-time reclaim.
pg forwarder diagnosability — the reconnect arm's connect failures are swallowed with .ok() and the loop's exit discards the poll error (forwarder.rs:825/855-869): an operator sees nothing before wake loss. Related cross-cutting posture: the family has no log facade and 17 library-path eprintln! sites carry its diagnostics (verified secret-free — no DSN/password material ever printed). A log/tracing adoption decision is a family-level call; recorded rather than made here. Disposition: decide-log-facade-adoption adopted tracing (ADR-025, engine crates only, user sign-off 2026-10-11; all 19 library-path eprintln! sites migrated, substrate sites riding delta D-41) and pg-fix-forwarder-diagnosability surfaced the two arms (warn on failed reconnect connects with attempt/backoff context, error on the poll-exit ending).
sqlite listener-bridge asymmetry — listen() parks one spawn_blocking thread per subscription (notify.rs:98-104, tokio blocking-pool ceiling ~512) while the stream bridge does the identical job on a dedicated std::thread (stream.rs:368-379); past ~500 concurrent listeners the whole engine's seams queue behind parked bridges. Aligning the two bridges (std-thread shape) is a small contained change deferred alongside item 2's subscription-path work. Disposition: landed via sqlite-fix-listener-bridge-alignment — the listen bridge takes the stream bridge's named std-thread shape exactly (including the spawn-failure fail-closed arm).
sqlite run_schedules error flattening — lease.renew(...).await.unwrap_or(false) maps any renew failure (including the closed-store Database error) to LeadershipLost (scheduler.rs:205-211), misdirecting the documented respawn recipe at a dead store. Small, but the right arm set (closed vs lost vs storage failure) is a contract-adjacent error-classification change on a stable surface — record, don't rush. Disposition: sqlite-fix-scheduler-error-classification landed the loss-vs-failure arm set (Ok(false) stays LeadershipLost; a failed renewal propagates its opaque classification) with the dated ADR-009 §1 annotation; the pg twin discovered mid-batch landed via pg-fix-scheduler-error-classification, implementing to match.
mem growth postures (N-1 family) — two perf/posture notes to carry into the mem spec's growth section when next touched: the subscribe path drains the full backlog into the receiver's pending deque before the first yield, and pending grows at publish rate for a stalled consumer (stream.rs:314-331 — the documented "drains to the tail then parks" shape, but unbounded on the consumer side); the claim path is an O(total-live-rows) scan with full-row clones per claim (queue.rs:122-181) — blessed economics for the ephemeral tier, worth one sentence in engine-mem.md if ever revisited. Disposition: landed via mem-doc-growth-outbox-notes arm (a) — the growth postures pinned in engine-mem.md's new growth section, anchored to the sites.
mem outbox outcome swallow — outbox run_once drops the ack/ retry row-refs' booleans and returns Ok(true) even when delivery outlived the visibility window and nothing landed (outbox.rs:104-122). Inherent to the no-heartbeat outbox design (at-least-once reclaim is the recovery), but a debug_assert! or doc sentence pinning the "processed means delivered-or-reclaimable" semantic would prevent future confusion. Disposition: bundled into mem-doc-growth-outbox-notes arm (b) — the "processed means delivered-or-reclaimable" semantic pinned in engine-mem.md's outbox section; the debug_assert! declined with the false-positive rationale recorded in the task.
core offset() initial value — EventReceiver::offset() pins the post-yield value only; the pre-yield return (the seeded checkpoint vs 0) is unpinned in the contract text. A doc-only fix once the three engines' pre-yield values are confirmed equal (not verified this session; left as a named gap). Disposition: landed via core-pin-offset-initial-value — the three engines verified equal (0 on every pre-yield arm, live on all three columns) and the doc-pin branch applied; pinned in core-contract.md, the trait doc, and the suite row receiver_offset_pre_yield_is_zero.
pg identifier truncation cross-channel collision — LISTEN/UNLISTEN interpolate the channel as a quoted identifier (truncated at 63 bytes server-side) while pg_notify carries it as a parameter; two distinct names sharing a 63-byte prefix can silently collapse into one wake channel, with names unbounded by the validation contract (the no-cap "length-independence" verdict is deliberate). The honest cheap fix is a deployment-matrix/ docs note ("names longer than the PG identifier limit may collide"); deferred here because the matrix was just finalized by the gate — fold it into whichever doc pass runs next (or into the publish session's reading). Disposition: landed via docs-pg-identifier-truncation-note — the note in deployment.md (dated) and engine-postgres.md, the mechanism sharpened (the server rejects ≥ 64-byte pg_notify channels; the collision is on the registration side), the no-cap verdict untouched.
core error Display log-forging — InvalidName/
ReservedName / InvalidSpec interpolate raw consumer strings into Display; control characters can forge log lines in naive consumers. Typical-idiom risk, low; recorded. Disposition: landed via core-fix-display-log-forging — the display_safe escape helper in the three variants' Display arms (plus Error::Codec, the one same-class neighbor); taxonomy untouched.
unsafe without // SAFETY in the substrate — the eight unsafe { ctx.get_connection() } UDF blocks (substrate/ops.rs:147-236) carry no justification comments. The pattern is the standard rusqlite UDF shape and provably constrained, but AGENTS.md's own carve-out ("inline comments when a non-obvious safety constraint would otherwise be missed") says this is where they'd live. Not added this session: the substrate is lineage-port code under ADR-012 §3's diff-fidelity posture — comments must ride a PROVENANCE delta. Recorded for whenever the substrate is next touched. Disposition: landed via sqlite-substrate-safety-comments — all ten same-class sites (the eight + schema.rs's notify registration + the test-module probe) carry concrete-invariant // SAFETY comments, registered as delta D-40.
Cosmetic metadata asymmetry — the three engines' dev-dep tokio features disagree (rt-multi-thread+macros+time / test-util / rt+macros); harmless (the contract suite carries the test runtime) but an easy-to-trip asymmetry when copying test code. Recorded, not normalized mid-release. Disposition: landed via normalize-tokio-dev-deps — all three engines normalized to rt-multi-thread + macros + time + test-util, dev-dep metadata only.

False positives caught on verification (for the record)

mem WakeBus::attach check-then-act race (reported as a finding): disproven by read — the closed check happens under the registry guard, and close()'s store(true) is sequenced before its guarded clear(), so the guard's acquire/release supplies the happens-before edge; no interleaving leaks a subscription. No fix needed; noted here so a future audit doesn't re-derive it.
The "tx-path negative seed divergence" was real (fixed, item 2), but the analogous claim against SQLite's tx form was not — its in-tx SQL is already raw-seed-and-monotone; the divergence was pg's clamp and mem's phantom-zero merge read only.

What was checked and clean (summary)

Security: SQL injection — every consumer-supplied string rides bound parameters across all engines (the format!-built SQL interpolates only engine-owned identifiers through the quoting helpers; savepoint/table keys are compile-time constants); env::var confined to test harness plumbing (re-grepped); no fs writes outside tempdirs; no process spawning; no secret/DSN material in any of the 17 library-path eprintln!s.
Core crate: zero panics/unsafe/unwrap outside tests, its three-dependency posture intact, #[non_exhaustive] placement and the hidden-constructor seam exactly per ADR-017, object-safety and validation coverage as pinned. (Post-fix, pre-abort-armor caveat in item 5 above.)
Mem engine: wasm32 subset verified (cargo check clean, tokio sync+time only, no spawn_blocking/fs/thread); all deadline arithmetic saturating; queue state machine, trim semantics, three-way arithmetic, scheduler boundaries/catch-up/leadership re-read clean.
pg engine: injection-clean, in_tx pool accounting exact on every arm, forwarder reconnect invariants sound outside the recorded TOCTOU/hang arms, FOR UPDATE SKIP LOCKED claim semantics and scheduler serialization consistent with sqlite/mem.
Cross-cutting: no todo!/unimplemented!/dbg!/.ok() anywhere; metadata coherent (versions, path-dep bounds, publish fields, exclude = ["fuzz"]); README/CHANGELOG claims spot-read true against code; fuzz/shared targets thin and invariant-correct.

Gates re-run after the fixes (this session)

cargo build, cargo test --workspace (green server-less), the full live pg column (127 + 25 + 9 — including the new in_tx_reads_see_own_writes negative-seed leg against the real server), cargo clippy --all-targets -- -D warnings, cargo fmt --check, the fuzz corpus replay (6/6 — including the corrected encode/invariant), the wasm32 check, and the doc gate — all green.

28 KiB Raw Blame History Unescape Escape