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