alkblobs/tasks/review-wave-2.md

---
id: review-wave-2
name: Wave 2 review gate — facade, dispatch, GC, and the first columns
status: completed
depends_on: [suite-column-local]
scope: narrow
risk: low
impact: phase
level: review
tags: [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

- [x] Every gate check above read and verified against the cited ADR
      text (not from memory of it)
- [x] Named design points each decided, recorded, and consistent with
      the specs
- [x] Findings fixed inline or recorded as wave-3 amendments with
      owners
- [x] All gates re-run green after any fixes
- [x] 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.