Wave-3 review gate: contract conformance clean; two findings fixed inline — substrate boundary move (open_writer_connection → seam.rs), writer-slot release on with_writer/begin/commit error arms + regression tests (task review-wave-3)

tasks/review-wave-3.md

@@ -1,7 +1,7 @@
 ---
 id: review-wave-3
 name: Review gate — wave 3 (SQLite engine)
-status: pending
+status: completed
 depends_on: [sqlite-engine-integration]
 scope: moderate
 risk: low
@@ -69,6 +69,163 @@ Check:
 
 > 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
+  wave 6's close-path review; 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
 
-> To be filled on completion
+> 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.