alkblobs/tasks/review-wave-2.md

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

id	name	status	depends_on	scope	risk	impact	level	tags
review-wave-2	Wave 2 review gate — facade, dispatch, GC, and the first columns	completed	suite-column-local	narrow	low	phase	review	wave-2 review

Description

The wave-2 review gate, before wave 3 decomposes. Wave 2 made the store work end-to-end dependency-free; the gate reads the implementation against the pinned ADR text line by line (the wave-1 gate's method).

Gate checks:

• Facade + dispatch + GC vs ADR-003/005/012:
  • the three numbered unknown-length rules (ADR-003 §Dispatch policy) each read against their tests — mid-stream flush with no restart requirement; verification over the eventual stored artifact as a whole (buffer form and staged-bytes form); stream error leaves no entry and no stage file;
  • the delete-window ordering invariants (ADR-005 §Sweep): pin-before-publish on both put paths and both tiers; arbitration under the pin/liveness lock at delete time; the test-asserted property (a visible entry is never deleted while liveness protects it; an in-flight put is never deleted by a sweep started before it committed);
  • abort-as-data (ADR-012 §1): sweep() returns Ok(SweepReport) on every abort; GcRefuse is the direct-delete refusal only; the full protection set per ADR-012 §6.4;
  • dispatch as a pure function of length (no migration); fall-through general rule per ADR-012 §6.1; constructor modes per ADR-011 §4 / ADR-012 §6.2/§6.3; kv-only put-time rejection.
• Stage-hygiene exact-count tests: present and exact on both engines (mem's degenerate stage; local's real stage form incl. the Drop-cancellation story per the wave-1 review's observation).
• The suite's first columns green on mem + local: rows ADR-stamped, traceable to the backlog, the factory binding recorded (factory.rs + README if corrected).
• The dispatch-coverage shape exercised: the kv = mem + large = local column's dispatch rows (pure-function routing, fall-through, threshold boundaries).
• Named design points audited as resolved (each recorded in its task's Notes): engine representation (enum vs generic); the public stream type; list return shape; verification-on-get posture; liveness-source shape; Pin registry back-reference + lock granularity; put stream parameter shape; len-mismatch error.
• Family conventions: no comments in code (doc comments on public API fine), thiserror, tokio-only async, spawn_blocking for blocking file work, no unwrap/expect outside tests, naming, module structure.
• Gates green: cargo test, cargo test --features mem, cargo test --all-features, cargo clippy --all-targets -- -D warnings, cargo clippy --all-features --all-targets -- -D warnings, cargo fmt --check.

Findings are fixed inline where cheap, or recorded as wave-3 amendments where they shape the next decomposition. The gate's completion is what unblocks wave 3's decomposition.

Acceptance Criteria

• Every gate check above read and verified against the cited ADR text (not from memory of it)
• Named design points each decided, recorded, and consistent with the specs
• Findings fixed inline or recorded as wave-3 amendments with owners
• All gates re-run green after any fixes
• Wave 3 decomposition unblocked (recorded in this task's Summary)

References

• docs/plans/implementation.md (§Wave 2, §Review gates)
• docs/architecture/decisions/003-backend-contract-and-dispatch.md
• docs/architecture/decisions/005-pooled-cas-and-gc-mechanism.md
• docs/architecture/decisions/011-vocabulary-tiers-engines-and-fleet-state.md
• docs/architecture/decisions/012-pre-decomposition-consistency-rulings.md
• docs/architecture/store-api.md
• tasks/review-wave-1.md (the gate method; the observations wave 2 owned)

Notes

Filled by the review agent (2026-10-11). Every gate check was read against the cited ADR/spec text (the files, not memory): ADR-003, ADR-005, ADR-006, ADR-010, ADR-011, ADR-012, ADR-013 §2, store-api.md, and backends-and-dispatch citations. Findings below; everything else passed.

• The three numbered unknown-length rules vs ADR-003 §Dispatch policy — each read against its tests, all faithful:
  1. Mid-stream flush, no restart requirement — mid_stream_overflow_flushes_into_a_fresh_stage_and_the_stream_continues (the buffered prefix flushes into the fresh stage's first appends and the stream continues appending into it; nothing migrates or duplicates); strictly-past-the-threshold overflow with unknown_length_exactly_at_threshold_stays_kv pinning the boundary reading on the unknown path.
  2. Verification over the eventual stored artifact as a whole — unknown_length_put_round_trips_on_both_tiers pins rule 2 by key equality (only a preamble + whole-artifact derivation yields the canonical address), the staged form reading back over preamble + staged bytes through the StagingCursor: ReadCursor seam (the wave-1 design-led extension, now driven verbatim), the buffer form over the complete buffer — the kv commit's whole stored value. The known-length path's one streaming pass hashes the same input.
  3. Stream error before commit — stream_errors_before_commit_leave_no_entry_and_no_stage_file over all four failure forms (flushed staged, buffered kv, known-length large, known-length kv): no entry, zero stage files.
• The delete-window ordering invariants vs ADR-005 §Sweep — verified: mark → arbitrate-at-delete-time (per candidate, not once upfront) → batched commit (~100/batch, SWEEP_DELETE_BATCH); arbitration runs under the one arbitration lock (pins ∨ delete markers ∞ sources ∨ — direct path only — protect callbacks), the engine delete runs outside the lock behind the in-flight marker, and the marker release notifies blocked pin mints (ADR-005's case (a) delete-then-recover verbatim — raced in sweep_racing_a_pinned_put_never_deletes_the_protected_entry and conversion_raced_against_direct_delete_never_deletes); pin-before-publish holds on both put paths and both tiers (pin_before_publish_holds_on_both_put_paths_and_both_tiers, put_publishing_after_a_sweep_started_is_never_deleted); the test-asserted property is covered from both sides plus the no-torn-reads window observation. Sweeps consult the protect callback pre-sweep only (mark), the direct path consults all three sources at delete time — exactly the ADR-005/012 §6.4 split.
• Abort-as-data vs ADR-012 §1 — sweep() returns Ok(SweepReport) on every deliberate no-op; all three causes (NoLivenessSources, ProtectFailed, SweeperLock) exercised as report data with nothing deleted; GcRefuse occurs only on the direct path, decided at the facade (GcHome::arbitrate answers as data — the altitude discipline holds in code too), with the full protection set (pinned / source-live / protect-named / failed protect refused conservatively) per ADR-012 §6.4; GcAbortCause is #[non_exhaustive] with exactly three variants (structurally pinned, in-crate exhaustive match tested). Operational failures alone carry Err (malformed pool list through the sweep's own mark, tested) — the error half ADR-012 §1 reserves.
• Dispatch, modes, and kv-only rejection vs ADR-003/011/012 — route is pure (at-or-below → kv, pinned boundary values including degenerate tunings and u64::MAX); tier_for per mode; read ops never consult it (fall-through is consult order — test-asserted with a planted cross-tier decoy); kv-only's two put-time rejection points (known-length immediate via an Unreadable stream; unknown length at mid-stream overflow) with zero residue, the at-threshold legs landing; mem-only's threshold-free posture. The ADR-012 §6.3 dispatch-coverage shape is exercised both as a constructor test and as the local column's dispatch rows.
• Stage-hygiene exact-count tests vs store-api.md invariant 3 / POC #3 A5 — present and exact on both engines: local's exact-count rows over all three failure forms (discard, failed commit, Drop cancellation — 3N left zero stage files and zero committed) plus the success form (exactly N), mirrored on disk through the local column's stage_hygiene_exact_counts_on_disk_over_both_put_paths and through sweep-outcome counts (the INV-5 posture) everywhere else; mem's degenerate stage form covered including injected commit-failure self-discard. The wave-1 cancellation-residue observation is owned: drop_cancellation_leaves_zero_residue covers local's Drop story.
• The suite's first columns — green on mem + local; every row ADR-stamped (Contract stamp: inventory, 13 + 7 stamps) and traced to the backlog (the in-code traceability gate passes); the factory binding recorded in factory.rs + README (the Store associated type collapsed into the concrete facade — the ruling made upstream in store-construction and verified here); the factory contract's isolation and idempotent-teardown shapes structurally exercised, including the panicking-row path on a medium-backed factory. The mem-gating of the local column and the columns-beyond-the-reference- set shape (concrete-factory rows observing the medium) are recorded conventions, not ad-hoc deviations (README §Columns beyond the reference set; suite-column-local's amended criterion).
• The dispatch-coverage shape — the kv = mem + large = local column's dispatch rows exist and pull their weight: pure-function routing with the on-disk placement oracle, threshold boundaries at the tuned and shipped-default thresholds with the constructor-tuned re-route leg, and fall-through per op (get/stat/has/read_range) with the mirror-position leg and both-tier miss shapes.
• Named design points (each cross-checked from its recording to the spec text): engine representation (concrete Store + enum dispatch; the generic rejected — recorded with its reasons); public stream type (Blob over the crate-internal AnyCursor); list return shape (Vec<Key>, sorted, materialized — with the sweep consuming crates-internally); verification-on-get (kv re-derives and never falls through past a mismatch; large streams metadata+handle, integrity on the put path's whole-blob pass + slice digests; read_range carries no canonical check — read against ADR-006's text, consistent with its Context's caller-carried-digest posture); liveness-source shape (live-shared, two sync verbs; mark-time outside the lock, delete-time probes under it); Pin registry back-reference (Arc<RegistryCore> on the guard, registry outlives the store when guards live) + lock granularity (one short-held parking_lot arbitration lock; one-directional lock order; the enable-before-check Notify recipe); put stream parameter shape (generic impl AsyncRead + Unpin + Send, no boxing); len-mismatch error (Verification, zero residue, both tiers). The staged-put seam's key-at-commit amendment was audited: no ADR text pins the key's arrival point (ADR-003/010 pin handout/commit/discard mechanics), the amended shape is consistent with all four documents' mechanics, and the amendment carries operator sign-off — accepted.
• Family conventions — clean. No unwrap/expect/panic! outside #[cfg(test)] modules (audited every occurrence); thiserror error family (no #[non_exhaustive] on Error, correctly); tokio-only async; all local file work in spawn_blocking (the blocking helper audited end-to-end); parking_lot short-held locks with the one-directional order; the few inline // comments all carry non-obvious safety/accounting constraints (the rule-2 forms, the vanished-candidate accounting, the conservative-refuse branch) — accepted under the wave-1 gate's exception; naming and one-module-per-file re-export shape clean. The #![allow(dead_code)] scope notes follow the house pattern and now more accurately match their remaining scope (see the inline fixes).
• Inline fixes applied (review-of-record):
  1. src/facade.rs — the stale #[allow(dead_code)] + stale store-put-paths task-point comment on Batch::add removed: the call sites landed (put_in_batch), the attribute outlived its scope.
  2. src/dispatch.rs — the module-level #![allow(dead_code)] removed: route/Tier have their in-tree drivers now; clippy is clean without it in all three feature shapes. The module doc's scope note updated to match.
  3. src/backend.rs — attempted the same shrink and reverted: the trait's name (and kv put in default builds) still have no in-tree caller, so the allow is load-bearing; its scope note now records what is actually dead (reporting names, the kv whole-value put until wave 3).
  4. src/mem.rs — the stale module-doc sentence ("Clone is the constructor until dispatch names the engine") updated to the landed state (the facade's mem constructors construct it).
  5. tasks/gc-protocol.md — two stray "> To be filled" placeholder lines removed above its filled Notes/Summary content.
• No wave-3 amendments arise from this gate. The one amendment wave 2 itself carried (the staged-put key-at-commit handout shape, store-put-paths Notes) was audited as consistent, not amended further.

Summary

Wave-2 review gate complete: every gate check was read against the cited ADR text line by line and verified — the three numbered unknown-length rules each test-backed (flush/no-restart, whole-artifact verification over both forms, failure hygiene across all four paths), the delete-window ordering invariants (arbitrate-at-delete-time under the one arbitration lock, pin-before-publish on both put paths and both tiers, dedup refcounts, the raced conversion ordering, the blocked-mint delete-then-recover), abort-as-data with exactly three causes and the GcRefuse/full-protection-set direct path, dispatch as a pure length function with the fall-through consult order and the three constructor modes and kv-only's two put-time rejection points, stage-hygiene exact counts on both engines including the Drop cancellation story, both suite columns green and ADR-stamped with the factory binding recorded, and the dispatch-coverage shape exercised at the constructor and in the local column's dispatch rows. All eight named design points are recorded and consistent with the specs; the staged-put key-at-commit amendment was audited and accepted (operator sign-off in place). Findings were minor and fixed inline: a stale Batch::add dead-code allow, an unnecessary module allow in dispatch.rs (clippy-verified removable), a stale mem doc line, the backend.rs allow's scope note corrected (it stays, load-bearing), and two placeholder strays in tasks/gc-protocol.md. No wave-3 amendments arise. All gates re-run green after the fixes: cargo test (42 lib + 4 git-interop), cargo test --features mem and cargo test --all-features (97 lib + 37 conformance + 4 git-interop), cargo clippy --all-targets -- -D warnings and --all-features --all-targets green, cargo fmt --check green. Wave 3 decomposition is unblocked — sqlite kv engine decomposes against the contract surface as reviewed.