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