warriors/erga/audit/2026-10-10-blake-batch-owner/reviews/code.txt

# Fresh Independent CODE Review — Blake benchmark batch owner

**Verdict: APPROVE**

I inspected the complete candidate bodies (`lib.rs`, `batch.rs`, `import_memory.rs`, `batch/tests.rs`, `tests/batch.rs`, `main.rs`, spec paragraph, lock delta) against the originals, the provider APIs, the four unchanged shaders, the pinned SDK/XNU/Apple primaries, and the 42 raw streams. I found no blocker. Details below are what I actually verified, then optional notes.

## Safety / lifecycle verified against source

**Field drop order.** `struct Storage { buffer: Buffer, block: Block }` — declaration order governs drop, so the MTLBuffer import releases before the backing `Block` unmaps. Both fields private; no path detaches an import from its owner. On partial construction failure in `allocate` (second `plan.open()?` or second `initialize`), the already-published `input` Storage drops buffer-then-block before the Block. Relative native destructor order is source-proof, not traced — correctly labeled.

**Fallible construction order.** `page_size → layout → Block::plan → capacity(plan.allocation_size, max_buffer_length) → open → (size==planned && size>=logical) → capacity → validate → zero full actual extent → wrap`. Device max is rejected *before* native allocation, and zeroing precedes both import and any Rust view. `write_bytes(0, block.size())` covers padding, so no typed view ever reads uninit. Failure anywhere leaves no published owner.

**Arithmetic.** `layout` rejects `count==0`, `page==0` (via `checked_sub(1)→None`), `page==usize::MAX`, `count*32` overflow, and `request > isize::MAX`; `request ≥ logical` holds for any `page>0`. Hand-checked against the three asserted pairs (1→(32,16384), 513→(16416,32768), u32::MAX logical). Views use exactly `count` 32-byte records (alignment 1) over the initialized logical prefix; padding is excluded.

**VM/ABI proof.** `RegionInfo` field offsets computed by hand from SDK 26.4.1 `vm_region.h` under `pragma pack(4)` give size 48 / align 4 / `is_submap` at 32 — and the three `const _: ()` asserts enforce it at compile time. FFI signature matches `mach_vm_region_recurse` exactly (`mach_port_t=u32`, `mach_vm_address_t/size_t=u64`, `natural_t*`, `int*`, `mach_msg_type_number_t*`, `kern_return_t=int`); `count=12` equals `VM_REGION_SUBMAP_SHORT_INFO_COUNT_64`, which is also XNU's minimum and the value it writes back. `depth=u32::MAX` plus the `is_submap!=0` rejection reaches a leaf per the pinned XNU body; `protection & 3 != 3` requires RW; `contained()` rejects both the "address unmapped → next region returned" case (`start > pointer`) and any region-crossing extent, with checked end. 8-byte `ulong` alignment is checked explicitly and implied by page alignment; shader offsets are multiples of 32. Correctly stated as a current-mapping compatibility check, not physical contiguity or pinning.

**Grid/index proof.** `grid = count/hpt + u32::from(!count.is_multiple_of(hpt))`; `hpt∈{1,2}` from a sealed `Variant` matched to the compiled pipeline (private field, copy-only getter, no setter). No `count+hpt-1`. At `count=u32::MAX`, hpt=2: grid=2^31, max gid=2^31−1, so `idxA=2^32−2`, `idxB=2^32−1` — no `uint` wrap. `Encoder::launch` routes to `dispatchThreads:threadsPerThreadgroup:`, i.e. nonuniform, so there is **no** whole-group surplus lane that could wrap and overwrite a previous row. Bound is arithmetic, not inferred from memory size. Odd-partner `has_b` guards unchanged; `count=1` (grid 1, B suppressed) and 63/65 exercise it for real.

**Dispatch window.** `validate` (failed-state → device identity via `as_raw()` → per-dispatch mapping/capacity recheck) then `check_width` (0, >pipeline, >device.width, device height/depth==0) — all before any encoding, so pre-submit errors leave bytes and `failed` untouched (test asserts this for width 0 and max+1, then dispatches successfully again). The only fallible calls (`commands()`, `encoder()`) precede `failed = true`; between `failed=true`, `submit()`, `wait()` there is nothing. `status()`/`error()` are evaluated left-to-right after `wait()`; `completed` clears `failed` only on `STATUS_COMPLETED && error.is_none()`, and any formatting happens with `failed` still true. `command` is declared before `encoder`, so encoder drops first, both after `wait()` and both before the exclusive `&mut Batch` borrow ends — imports and Blocks outlive them.

**Escape and auto traits.** No getter returns `Buffer`, `Block`, `handle()`, `address()`, IOSurface ID, or a caller-settable count; `iosurface_id` is gone and the `batch` module is private with only `pub use batch::Batch`. `Buffer` and `&Gpu` carry no `unsafe impl Send/Sync`, so `Batch` inherits `!Send`/`!Sync` with no assertion added — pinned by real nightly E0277 witnesses. E0499/E0502 pin that no input or output view survives across `&mut` dispatch; E0505 pins the factory lifetime. `Encoder::push` copies the 4-byte count inline (Apple ≤4 KB primary), so no new buffer or provider API.

**Cross-factory case.** Passing a batch to a *different* `GpuBlake2b` on the same device is sound: the batch carries no variant assumption, and the dispatching factory's `hpt`/grid always match its own compiled pipeline. Different device → rejected by identity. Mismatch branch is unexercised on a single-device host; that is stated, not claimed measured.

**CLI.** `bench_once`/`bench_stable`/`verify_variant` return `Result`; the old `.ok()?` that turned an allocation failure into "unsupported tg width" is gone. The unsupported-width branch stays distinct (`Ok(None)` → `-`). Allocation/completion errors propagate to `run()` → stderr + exit 1; verification failure still exits 1 before any sweep. All loop/count/trial constants are byte-identical to the original (4/8/16 iters, 3 warmups, 8 iters × trials, 4096/64/seed 1234, both batch/width tables, `0xCAFE_BABE_DEAD_BEEF`). `fill_inputs` produces the same xorshift sequence in the same order.

**Lock.** The diff removes exactly one line: `libc` from `unimem 0.2.0`'s dependency list. `unimem/Cargo.toml` at 9cbd4807 has `libc` only under `[dev-dependencies]`, and the lock omits unimem's other dev-deps — so the edge is genuinely obsolete. All 21 records show `lock_before == lock_after`, i.e. cargo accepted the lock without wanting to update it.

## Evidence cross-checks

Receipt counts reconcile with the raw streams: 21 records / 42 streams, 12 exit-0 / 9 nonzero (I counted the table). Debug and release each show 9 lib + 1 integration runtime tests and 6 doctests; nightly shows the same 6 compile-fail cases with error codes. `cli-verify` shows 4 variants × 4096. `miner-tests` shows 5 + 2 = 7. `difftest` shows 512 nonces byte-exact on M4 Max. semver stdout shows exactly the 6 claimed lint classes, and the `doc_hidden` finding is honestly explained as private-ization rather than an added attribute. The fmt comparison is internally consistent (parent 165 − 5 changed = 160; candidate 160 − 0 = 160; identical bytes). The integration test's BoringSSL literal is embedded with its license, matches the pinned blob/line range, and is compared over all 32 bytes for all 4 variants at counts 1,2,3,4,63,64,65 with two fills — a real independent witness, and `reference::known_vector_abc` is correctly recorded as a non-witness and left unmodified.

## Preserved limits (correctly stated, not blockers)

RTable `3×E0133 + 1×E0596` at lib 58/80/107 remain, induced by Honey PR14's raw-contract repair and newly exposed here; mine-bench owners and the old chime multiply-overflow runtime failures stay masked, not fixed. No phase, rung/kelvin, generic provider, native `Image`, physical/pinning-policy, performance, or package/release closure. Named `buildbench` and `packaging/bundle.nu` unrun. Completion-fault matrix is simulated through the real private helper; device mismatch and relative drop order are source proof. Construction now zeros both actual backings and dispatch adds checks, so prior timings are explicitly not comparable — no measured table was touched.

## Optional notes (no action required to merge)

1. `cmd_bench_all` keeps a bare `{ ... }` block left over from the removed `if let Ok(g)`. Harmless orphan; fold only if you touch the file anyway.
2. `BenchError`'s `Display` delegates and `source()` is unconditional `Some`, so a reporter that prints the error and then walks the chain duplicates the text. Consider a short prefix if that ever shows up in a receipt.
3. `Batch::allocate`'s `count == 0` check duplicates `layout`'s; cosmetic.
4. `pub(crate) failed` is writable from anywhere in the crate (tests use this deliberately). A `pub(crate)` setter would narrow it, but there is no public hazard.
5. The ownership paragraph landed in `specs/README.md` §5 (a resources section); §2's invariant table may read better. Within doctrine either way.
6. For the commit: receipts belong under `<repo-root>/audit/` and the three task artifacts under `.claude/tasks/blake-batch-owner/`; this change removes public API and edits `specs/`, so per `cyberia/dev.md` § merge policy it carries the `decision` label and cannot auto-merge on green gates.