alkstore/docs/reviews/003-wave-5-general-review.md

General review — wave 5 (contract suite + engine hardening)

• Reviewer: opencode (glm-5.3-flash)
• Scope: general review of everything wave 5 landed (commit range 250511d..HEAD: nine delivery tasks + the wave-5 review gate), distinct from the gate (tasks/review-wave-5.md, which audited the backlog discharge map, stamps, dispositions, and green-on-both- engines — and passed). Lenses: correctness, security posture, code smells, coverage gaps (cargo-llvm-cov), and the classic dev issues reviews 001/002 hunted. The wave-5 delta is overwhelmingly test-side (properties.rs +2379 lines, both engines' suite targets, three engine-side test additions); the only production-code change is the pg wait_wake lag logging (stream.rs, +13/-1).
• Input: full read of alkstore-contract-suite/src (all 3344 lines of properties.rs, factory, harness, version stamp), both engines' tests/contract_suite.rs executors, the three engine-side diffs (sqlite seam.rs/store.rs/tx.rs, substrate/queue_ops.rs, substrate/watcher.rs; pg stream.rs/stream_tests.rs), the wave-5 task Notes, and the lcov output. No live probes were needed — the harness server was up, so the verification ran the real thing.
• Gates at review time (all re-run this session): cargo build, cargo test --workspace server-less (green, skips clean), the full pg column live (121 lib + 25 suite + 9 schema, vs the harness server), cargo clippy --all-targets -- -D warnings, cargo fmt --check — all green.

Op note for future sessions (harness credentials rediscovery)

The pg tests' server-less skip posture hides which env the harness needs. For the record: ALKSTORE_PG_HOST=localhost ALKSTORE_PG_PORT=15432 ALKSTORE_PG_USER=postgres ALKSTORE_PG_PASSWORD=poc ALKSTORE_PG_DB=blobs (discovered via the POC crate's examples — the dockerized pglo-poc publishes :15432 through docker NAT, so the server's host … 127.0.0.1 trust pg_hba lines do not apply from the test host; the scram rule matches and the real password is required). A bare cargo test -p alkstore-postgres without these silently skips 155 tests — the workspace stays "green" with no signal that the pg column never ran. That is exactly the documented SKIP posture, but reviewers should set the env before believing a pg claim: this review's first baseline run reported zero pg tests executed (25 suite tests "passing" in 0.02 s).

Env-variable boundary (verified — family posture)

The alk* family rule is "nothing important goes in env vars" (alkvault README), with the alkhttp spelling as the wire-facing case ("no handler reads std::env::var"). Verified this session by grep across the whole workspace: every env::var read lives in a test target — the pg gate modules (store/*_tests.rs, tests/schema_tests.rs, tests/contract_suite.rs). Production paths (alkstore/src, both engines' machinery, the contract suite, the constructors) are env-free — the only std::env uses in the other crates are std::env::temp_dir() inside test modules for temp-file paths, not configuration. Configuration reaches production exclusively through PgOpts / SqliteOpts / DSN arguments passed by the caller's own config layer (engine-postgres.md, ADR-008 §6; deployment.md's config table carries no env entry) — the engines never consult the process environment, so no credential (or any setting) can arrive implicitly.

For wave 6 (release readiness / prepublish) this boundary is an explicit re-check item, not a settled fact: re-run the no-env-in-production grep, and pin the boundary in the engine specs / deployment matrix in one sentence ("env reads are harness-only; production configuration is the opts-constructor surface"). Should a future consumer-facing surface ever want env conveniences, the family pattern is caller-side: the app's own config layer reads env (or vault/config file) and hands the store's opts its values.

Verdict

No correctness or security defects found; wave 5's deliverables are sound and the suite rows pin what the ADRs pin, no more and no less. What the review did find is one genuine coverage gap on a wave-4 hardening feature (the pg scheduler's transient-error retry loop has never executed under any test), a set of narrow untested race/failure arms around the forwarder and SQLite rollback paths, and a handful of no-action smells. Nothing here is engine-consumer-facing; the load-bearing findings are of the same "untested failure arm" class that produced review 002's live bugs, so they are recommended for the wave-6 window rather than accepted silently.

---

Finding 1 — MEDIUM: the pg scheduler's transient-error retry loop

is dead code under test (coverage gap, not a demonstrated bug)

Site: alkstore-postgres/src/scheduler.rs:680-704 (tick_with_retries), 171-184 of the same file's uncovered inventory.

pg-fix-scheduler-resilience (the wave-4 fix batch, review 002 Finding 5) added two halves: the tampered-row quarantine and the tick-level transient-error retry (3 retries at 250 ms → 500 ms → 1 s, then a context_error exhaustion arm). The quarantine half is test- pinned (tampered_spec_row_is_quarantined_and_the_runner_survives). The retry half is pinned only at the constants level — the_tick_retry_policy_is_pinned asserts TICK_RETRIES == 3 and the tick_retry_backoff table server-less — while the loop that uses them (the retry, its backoff sleeps, the exhaustion error, and the eprintln diagnostic) has zero coverage: the lcov run shows scheduler.rs 171-184 and 237-260 unexecuted, and no fault seam exists anywhere in the pg scheduler for tick_once.

A grep of the task module doc shows the deferral is half-visible ("the transient-error injection's unit arm"), but the injection itself never landed. The review explicitly flags this not as a bug (the code reads correct — the retry loop is straight-line) but as the exact class that bit review 002: Finding 1 there was an untested reconnect failure arm that turned out to be a permanent-death bug; Finding 2 an untested tx-wake arm that turned out to be F-1's root cause.

Recommendation: a small wave-6 task — a cfg(test) transient- fault seam on the pg tick path (the sqlite-commit-error-arm pattern: one-shot armed flag feeding a fabricated error into the real retry loop), driving (a) one retried tick then success and (b) exhaustion to the typed error shape. Small-honest scope; it retires the last uncovered hardening arm from the wave-4 fix batch.

Finding 2 — LOW: forwarder narrow untested arms (race-shaped; park

or seam in wave 6, acceptably)

Coverage residue in forwarder.rs with genuine (if unlikely) failure shapes behind it:

• forwarder.rs:693-698 — the "connection died before/during the LISTEN" fall-through to the reconnect path. The reconnect-success and kill-listener tests cover reconnect-while-idle and shutdown-while-idle; this arm is death during the LISTEN window.
• forwarder.rs:719-727 — mid-recv-loop stale-command abandonment (command.generation() < generation inside the run loop). This is the race arm of review 002 Finding 4's fix; the generation-top drain half is tested, this arm structurally needs a send-racing- reconnect seam.
• forwarder.rs:330-338 (mid_listen_error ack arm), 444 (forwarder gone), 744-745/762 (shutdown abort/await arms).

All are defensive or need deliberate race machinery to reach. The recommendation is the same shape as Finding 1 in spirit but weaker: a seam for (a) is cheap; the race arm (b) can stay parked with the code-read as its pin — record it either way so the next general review does not re-derive the same inventory.

Finding 3 — LOW: SQLite rollback-failure-on-drop arms untested

alkstore-sqlite/src/tx.rs:439-447 (SqliteTxHandle::drop) and 166-175 (ConnLease::drop): when ROLLBACK itself fails, the connection is dropped, the error is printed, and (the wave-3 fix's property) a fresh connection replenishes the writer slot. The new failed_commit_replenishes_the_writer_slot test covers the commit error arm via the commit-fault seam; the mirror rollback arms remain unexercised. Reachable on real failure (I/O errors mid-rollback), and the replenish-on-failure logic is identical in shape to the commit arm the seam now proves. A rollback-fault arm on the existing seam would cover both in one small test. Minor — the wave-3 fix's live behavior is already proven on the commit path.

Finding 4 — INFO/no-action: pg stream bridge Lagged arm

wait_wake's Lagged(n) arm (stream.rs:644-650) — the arm pg-suite-infra-hardening extended with the lag eprintln — is unreachable in tests without forcing >capacity broadcast lag (F-4's known untestability). The change is logging-only on an existing recovery behavior; the commit's "now true at both bridges" claim is code-read-true, not test-true, which is appropriately disclosed in the task record. No action.

Finding 5 — INFO/no-action: smells (recorded, not churned)

• tests/contract_suite.rs:50-52 (SQLite factory): to_str() .unwrap_or_default() would silently open "" on a non-UTF-8 temp path — unreachable (std::env::temp_dir() base is UTF-8 on all supported targets), but it masks rather than fails. One-line fix whenever the file is next touched; not worth a green-gate-churning commit alone.
• tests/contract_suite.rs:91 (pg factory): fresh_schema(&self) is async fn with no await — needless.
• properties.rs:1683-1685: past_stamp_sleep uses blocking std::thread::sleep inside async rows — safe under the multi-thread test flavors both engines' targets use (and the tolerance-sleep posture is documented), but it would stall a current-thread runtime; note it if the harness ever evolves flavors.
• harness_ready() costs a 3 s timeout per pg suite test only when the server hangs (connection-refused fails fast) — correct posture.

Coverage inventory (cargo-llvm-cov, full workspace, this session)

Crate	Line coverage	Note
alkstore (core)	100%
alkstore-sqlite	95.0%	residue: defensive/closed/rollback-failure arms (Finding 3), watcher/substrate error arms
alkstore-postgres	92.9%	residue: Finding 1 (retry loop), Finding 2 (forwarder arms), stream-bridge defensive arms
alkstore-contract-suite	97.1%	residue: failure-path panic labels (expected — a failing row is the failure)

The residuals are overwhelmingly the family's accepted posture: failure arms unreachable through the contract surface without a seam. Finding 1 is the one exception worth closing; Findings 2–3 are the judgment line between seam-cost and diminishing returns, called explicitly here so the call is recorded rather than implicit.

Cross-checks that came back clean

• The suite rows pin ADR text, not implementation detail — checked for over-pinning (a row that a correct engine could fail): the tolerance postures hold (state outcomes, bounded waits, straddle / proximity tolerances on clock-adjacent values); the two engine asymmetries a row could over-pin (SQLite overtrigger delivery, pg cross-schema LISTEN fanout) are handled with by-construction observables (the wake's channel field, own-schema re-drain) rather than absence pins.
• Engine-specific rows are wired engine-specifically: *_on_sqlite runs only in the SQLite column, *_on_pg only in the pg column — no cross-column mistakes in either executor.
• Derived-queue naming equivalence (__alkstore_outbox:mail): both engines' outbox_backing_queue_name are format-identical and each has its own unit pin; the suite row pins the composed result.
• The N-5 panic probe is honest on both engines: the SQLite panic unwinds through with_tx and the rollback runs in the unwind's drop; the pg detached rollback is waited out by the row's convergence loop; the silence window's placement (before any post-panic write commit) correctly avoids racing SQLite's documented commit-overtrigger.
• Security posture: wave 5 introduces no new consumer surface, no new SQL (the test factories' DDL is parameter-quoted / minted-schema-scoped), no secrets, no dependency additions beyond dev/test tokio features.
• Task hygiene: all nine wave-5 task files carry completed status with filled Notes and Summary; disposition records in the tasks match what the code shows.
• The gate's flake-watch echo: the two unreproducible pg row failures (trim row, lock row) get independent concurrence on the gate's reasoning — the trim row's suspected failure shape (an event beyond the checkpoint in the post-trim absence window) is structurally blocked by the own-schema re-drain argument; the lock row's lapse assertions can only be delayed, never un-lapsed, by slow clocks. Supporting the gate's watch-not-block disposition; the watch flag stands (the release-readiness watch — wave 7 since ADR-024's renumbering).

Recommendation

No changes required to keep wave 5's verdict standing. For wave 6's window (wave 6 at writing — release readiness; renumbered by ADR-024, with Finding 1 since routed to the new wave 6 — the mem engine — window, and Findings 2–3 to wave 7's pre-release pass, per docs/plans/implementation.md's wave-6 section): Finding 1 is a decomposable small task (transient-fault seam + retry/exhaustion pin); Findings 2–3 can ride the same task or be accepted with the code-read as their pin — either way the disposition should be recorded. Findings 4–5 need no work. The env-variable boundary section above folds into the release-readiness pass (wave 7) as a verified-now / re-check-at-prepublish item.