alkstore/docs/reviews/004-pre-release-general-review.md

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 Values 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.

1. 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.
2. 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.
3. 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.
4. 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.
5. 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.
6. 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).
7. 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.
8. 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.
9. 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.
10. 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.
11. 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.
12. 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).
13. 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).
14. 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.
15. 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.
16. 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.

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.