alkstore/tasks/review-wave-3.md

id, name, status, depends_on, scope, risk, impact, level, tags

id	name	status	depends_on	scope	risk	impact	level	tags
review-wave-3	Review gate — wave 3 (SQLite engine)	completed	sqlite-engine-integration	moderate	low	phase	review	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

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