alkstore/tasks/review-wave-3.md

---
id: review-wave-3
name: Review gate — wave 3 (SQLite engine)
status: completed
depends_on: [sqlite-engine-integration]
scope: moderate
risk: low
impact: phase
level: review
tags: [wave-3, review]
---

## Description

Review the SQLite engine before wave 4 (or 5) decomposes. Primary
lens per the plan's review gates: **engine-vs-contract conformance** —
the engine crate now carries the first real consumer surface, so this
is the gate that checks the code against the pinned contract text
line by line (the wave-1 gate's discipline, now against an
implementation that must *honor* the surface, not just declare it).

Check:

- **Contract conformance**: every trait method's behavior against
  core-contract.md's per-mechanism text — validation at entry points
  (auto-commit AND tx paths), error arms (`Database`-only receivers,
  `Codec` at payload seams, `LeadershipLost` vs clean stop), value
  shapes (`Job`/`StreamEvent` field-for-field), the no-work-is-a-value
  vocabulary (`None`/`false`/empty-`Vec` where pinned, never errors).
- **ADR-023 follow-through**: the extent guards actually sit at the
  trait-impl entry (not substrate-side patches); duration guards on
  both lock call sites; plain-path open holds; `poll_interval` wired;
  `encode_payload` consumed fallibly at every call site.
- **Seam integrity**: drop = rollback under panic; writer-slot lease
  released on every path (commit, rollback, drop, error);
  `spawn_blocking` used consistently (no blocking call inside async
  context, no connection moved across await).
- **Substrate boundary**: still contract-blind (no `alkstore::` under
  `src/substrate/`); the lint removal didn't leave dead ported code
  behind; any new divergence registered in PROVENANCE.md.
- **Backoff curve**: the engine-side formula matches ADR-010 §3's
  pinned range and cap exactly (wave 4 re-derives it; wave 5 pins
  equivalence — this gate checks the one implementation against the
  text).
- **Backlog column**: the engine's suite rows present, stamped, green.
- **Family standards**: no panics, no unwrap/expect outside tests, no
  inline comments (doc comments fine), module-per-file.
- **Gates**: full workspace build/test/clippy/fmt.

## Acceptance Criteria

- [ ] Contract conformance spot-checked against the spec text (not
      just the tests)
- [ ] ADR-023's five backlog rows verified implemented as decided
- [ ] Seam/lease/rollback paths verified (code-read, not test-name-trust)
- [ ] Substrate boundary + provenance register still clean
- [ ] All gates green
- [ ] Findings recorded; wave 4/5 decomposition may proceed

## References

- docs/plans/implementation.md (Review gates)
- docs/architecture/core-contract.md
- docs/architecture/engine-sqlite.md
- docs/architecture/decisions/023-fourth-review-round.md
- docs/reviews/001-waves-1-2-general-review.md (the posture this gate inherits)

## Notes

> To be filled by implementation agent

Two findings, both fixed inline (2026-10-08). Neither needed a new
session or scope decision:

- **W-1 (boundary) — `alkstore::Error` referenced inside
  `src/substrate/mod.rs`**: `open_writer_connection` (the tx handle's
  `reopen` helper) lived in the substrate subtree and mapped into the
  contract taxonomy there — a violation of ADR-012 §2's
  contract-blind boundary ("no `alkstore` core types", as the
  substrate's own mod.rs doc text states; the wave-3 review
  checklist's grep for `alkstore::` under `src/substrate/` caught
  it). The machinery files (`schema.rs`/`ops.rs`/`queue_ops.rs`/
  `watcher.rs`) were always clean — the violation was a wave-3
  engine-layer helper that had been placed in the wrong file. Fixed:
  the helper moved to `alkstore-sqlite/src/seam.rs` (the substrate's
  one existing mapping site; identical body). No register entry —
  nothing lineage-side changed.
- **W-2 (seam defect) — the writer slot stranded on three error
  arms**: the task checklist's "writer-slot lease released on every
  path" read found `Writer::acquire`d connections **dropped without
  `release()`/replenish** on (a) `seam.rs::with_writer`'s op-error
  arm, (b) `tx.rs::begin`'s failed `BEGIN IMMEDIATE` arm, and (c)
  `tx.rs::commit`'s failed `COMMIT` arm. `Writer` holds exactly one
  connection and has no self-replenish, so the first failed
  auto-commit op (e.g. a busy timeout expiry on any notify/enqueue/
  claim) would park *every* subsequent writer op forever on the
  condvar — a store-wide livelock from one transient storage failure.
  Fixed: (a) releases unconditionally on both arms (the failed op
  didn't corrupt the connection — a refused statement leaves it
  healthy; the release is safe); (b) releases on the begin-error arm
  (the no-transaction connection is reusable); (c) drops the failed
  connection (its state is unknowable — exactly the Drop path's
  posture) and replenishes the slot via the existing `reopen`
  closure, the same machinery the cancellation-path `ConnLease` and
  the failed-rollback Drop already use. Regression tests added:
  `failed_notify_releases_the_writer_slot` (notify_tests — trigger-
  induced storage failure, then the slot proves grantable),
  `failed_begin_releases_the_writer_slot` (tx_tests — SQLITE_BUSY
  begin at a shortened busy timeout, then `begin_tx` succeeds).

Verified clean otherwise — spot-check record:

- Contract conformance (code-read against the spec text, per the
  acceptance criterion): entry-point validation on every name-bearing
  method, auto-commit AND tx paths (tx.rs does all eleven
  `validate_shared_name`/`validate_local_name` at entry, before any
  round trip); `InvalidName` for empty/whitespace-only, `ReservedName`
  for prefixed, matching core's `validate_name`. Duration guards on
  **both** lock call sites (`try_lock`, `SqliteLockHandle::renew` —
  `ttl <= 0` → opaque `Database`, detail in the source chain, the
  substrate guard's message preserved there). Extent guards at
  trait-impl entry on `claim_batch` (`n <= 0` → empty `Vec`), both
  auto-commit stream reads, both tx stream reads, and
  `EventReceiver::read_since` — never reaching the substrate's
  `LIMIT ?`. Boundary args unguarded-and-total (`trim_to` passes
  through; a negative horizon deletes nothing idempotently).
  `Error::Closed` on already-consumed handles; `Codec` at every
  payload/decode seam; `recv()`'s `Err` remapped to `Database`-only
  (`database_only`, stream.rs). No-work vocabulary held:
  `Ok(None)`, `Ok(false)`, empty `Vec` where pinned.
- Value shapes: `Job::from_row` field set matches ADR-021 §2's struct
  (claimed_at included); `StreamEvent::from_row` matches ADR-015 §3
  (the `topic` column carries `stream`, the one registered delta);
  dead-visible `get_job` carries `last_error`/`died_at`.
- ADR-023 follow-through: all five rows present in
  `alkstore-contract-suite/src/properties.rs` with their contract
  stamps and green in `alkstore-sqlite/tests/contract_suite.rs` (10
  tests); extent guards/duration guards/`poll_interval`/plain-path
  open/`encode_payload` fallibility verified at their sites (see
  above); `encode_payload` consumed fallibly at all eight call sites,
  no silent fallback.
- Backoff curve (queue.rs `backoff_delay_s`): equal-jitter over
  `[base·2^(a−1)/2, base·2^(a−1)]` integerized inclusive, capped at
  3600 s, attempt index = the row's post-claim count — matches
  ADR-010 §3's pinned form exactly.
- Scheduler: tick fires + boundary advance in one `BEGIN IMMEDIATE`
  (writer serialization as the row lock); leadership loss returns
  `Err(LeadershipLost)` **before** ticking (top-of-loop renew, and
  exit-before-tick also holds at start — a refused acquire is
  `LeadershipLost`); the sleep renews the lease across itself; stop
  token releases the lock cleanly; 64-cap catch-up + skip-forward
  inherited (D-12).
- Seam integrity code-read: drop = rollback issues `ROLLBACK` and
  releases the slot (failed rollback → connection dropped + slot
  replenished via `reopen` — the same posture commit now uses);
  panic-through-drop reaches the same path; the mid-op-future
  cancellation path (ConnLease Drop) replenishes; `spawn_blocking`
  used consistently — the only blocking-in-async exception is
  `EventReceiver::save_offset`, a sync *trait signature* taking the
  slot in-line, documented at the site (ADR-007's honest lease
  posture; not a violation).
- Substrate boundary: no `alkstore::` reference remains under
  `src/substrate/` (post-W-1-fix grep; also `grep -r "allow("`
  clean — the wave-3 lint removals hold); PROVENANCE.md's
  D-29..D-36 entries cover the wave-3 deltas (verified against the
  code: URI flag dropped, `open_conn_bootstrapped`/D-30,
  `ack_batch` worker-less/D-31, the five cuts D-32..D-36 all
  genuinely absent); no unregistered divergence found in the
  lineage-adjacent reads.
- Family standards: no panics/unwrap/expect in library code (only
  `#[cfg(test)]` modules); inline `//` comments exist only at the
  correctness-constraint sites the AGENTS.md exception names
  (backoff curve's range pin, the extent guard, the leader loop's
  one-owner-of-the-loss-decision note, `run_once`'s retry-string
  posture — all non-obvious behavior notes, acceptable);
  module-per-file holds.
- One doc-vs-code nit, absorbed in the W-2 fix rather than a
  finding: `seam.rs`'s `with_writer` doc comment said "release on
  both arms" — the code did not do that; now it does.

Deferred (documented, not fixed — each is small but not
inline-clean, or already owned by a later wave):

- **`Readers::acquire`'s `close()` path can drop a busy reader
  connection while held** (schema.rs — `close()` clears the pool
  but does not track outstanding acquisitions; a reader acquired
  before `close()` and released after it is discarded by
  `release()`'s closed check — correct, no leak; but a reader
  connection *still in use* across store-close keeps running on a
  closed file until its op completes — harmless in practice, the op
  finishes on an open connection that is then dropped). Noted for
  release readiness's close-path review (now wave 7); no consumer-visible defect.
- **Coverage of the commit-error arm**: no test induces a failing
  `COMMIT` (needs SQLITE_FULL/IOERR-class injection; the code fix
  is pinned by code-read + the shared `reopen` machinery being
  exercised by the cancellation tests). Recorded here; natural
  home is wave 5's suite hardening.

## Summary

> What landed, verified how:

**Review gate passed: 2 findings, both fixed inline; all gates green.**

- **Fixed — substrate boundary violation**: `open_writer_connection`
  moved from `src/substrate/mod.rs` (which referenced
  `alkstore::Error`, breaking ADR-012 §2's contract-blind boundary) to
  `src/seam.rs`, the engine's one mapping site. Substrate grep for
  `alkstore::` now clean.
- **Fixed — writer-slot stranding on error arms** (the
  store-wide-livelock defect): `with_writer` now releases the slot on
  the op-error arm; `begin` releases on a failed `BEGIN IMMEDIATE`;
  `commit` replenishes via the existing `reopen` closure on a failed
  `COMMIT`. Two regression tests pin the property end-to-end
  (`failed_notify_releases_the_writer_slot`,
  `failed_begin_releases_the_writer_slot`).
- **Verified clean**: contract conformance (entry-point validation,
  error arms, value shapes, no-work vocabulary — full code-read
  record in Notes), ADR-023's five follow-through items + backlog
  rows (10 tests, stamped, green), backoff curve matches ADR-010 §3,
  scheduler tick atomicity + exit-before-tick-on-loss, seam
  integrity (drop = rollback incl. panic path, cancellation
  replenish), substrate boundary + PROVENANCE D-29..D-36 accurate,
  family standards.
- **Gates**: `cargo build`, `cargo test` (25 core + 188 sqlite + 10
  suite + 3 harness), `cargo clippy --all-targets -- -D warnings`,
  `cargo fmt --check` — all green after the fixes.
- **Recorded for later waves**: the reader-pool close-path note and
  the commit-error-arm coverage gap (Notes; wave 5/6 natural homes).

Wave 4/5 decomposition may proceed.