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.