Repository navigation
Conversation
…fault runs a prompt a part Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5zTV27LbaVZFTjnkdmTjJ
… it; the default is prefill Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5zTV27LbaVZFTjnkdmTjJ
…after named rows of a chunk Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5zTV27LbaVZFTjnkdmTjJ
…s each mark as a point in one call Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5zTV27LbaVZFTjnkdmTjJ
…stale bit-neutral claim Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5zTV27LbaVZFTjnkdmTjJ
…l_marked Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5zTV27LbaVZFTjnkdmTjJ
There was a problem hiding this comment.
Verdict: MUST_FIX: valid positions corrupt ring snapshots, and a new comparator duplicates an existing owner.
Must fix
1. Prevent overflow in the ring snapshot index
crates/metal/kernels/delta.metal:918 A public delta_rows call with pos = u32::MAX - 9, nine rows and after = [8] passes the position check at crates/metal/src/kernels/delta.rs:519. Adding the ring length wraps: ring slot 2 copies input row 1 instead of row 8, without a fault.
Fixing the index now prevents callers from restoring incorrect recurrent inputs. Replace the index calculation:
uint back = (last % (uint)BLOOMERY_DELTA_RING_ROWS + (uint)BLOOMERY_DELTA_RING_ROWS - s)
% (uint)BLOOMERY_DELTA_RING_ROWS;The existing delta_rows_snap ring comparison detects this error with the boundary case added:
const SNAP_CASES: [SnapCase; 5] = [
(12, 7, (0, 1), &[3, 9]),
(64, 5, (0, 0), &[0, 40]),
(9, 0, (0, 1), &[1, 8]),
(20, 3, (1, 0), &[12]),
(9, u32::MAX - 9, (0, 1), &[8]),
];2. Reuse the existing bit comparator (R14)
crates/metal/src/bin/gate_metal_qwen35moe_kernels/clauses_rows.rs:427 duplicates crates/metal/src/bin/gate_metal_qwen35moe_kernels/clauses_rows.rs:326: both check equal lengths and compare each float's bits. Only the diagnostic differs.
Reusing the existing owner removes a function before its comparison logic diverges. Delete same_copy and its doc comment. Replace its two calls with:
same_bits(
&format!("{at}: copy {k}'s lane"),
&copies.lane[k * lane_len..(k + 1) * lane_len],
lane,
)?;
same_bits(
&format!("{at}: copy {k}'s ring"),
&copies.ring[k * ring_len..(k + 1) * ring_len],
&end.ring,
)?;In same_bits, replace its diagnostic string with:
"{what}[{i}] is {:?} ({:#010x}), expected {:?} ({:#010x})",Non-blocking
⚠ Identify the timed revision: crates/metal/src/kernels/delta.rs:701 adds one ring-copy launch per delta layer per marked pass. Gate and rotated A/B results are supplied. Record the measured after commit hash instead of “this branch's tree” so the timing evidence identifies a fixed revision.
Checked
Duplicates: compared same_copy with same_bits and checked all SnapCase references.
Complexity: snapshot handling adds validation and a conditional ring-copy dispatch.
Silent failure: traced the position check through snapshot dispatch and indexing.
Tests: delta_rows_snap compares each copied ring with a prefix launch; the added boundary case exercises the overflow.
Gates: no band or tolerance changes in the reviewed snapshot hunks.
Escape catalog: no patterns matched the retained findings.
Notes
Metal gates were not run here; Clippy is unavailable. A host model confirmed the overflow and checked the replacement arithmetic against sequential ring writes in 464,706 slot cases.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
| return; | ||
| } | ||
| uint last = args.pos + snaps.after[k]; | ||
| uint back = (last + (uint)BLOOMERY_DELTA_RING_ROWS - s) % (uint)BLOOMERY_DELTA_RING_ROWS; |
There was a problem hiding this comment.
Reduce the position before adding the ring length; valid positions near u32::MAX otherwise copy the wrong input row.
| uint back = (last + (uint)BLOOMERY_DELTA_RING_ROWS - s) % (uint)BLOOMERY_DELTA_RING_ROWS; | |
| uint back = (last % (uint)BLOOMERY_DELTA_RING_ROWS + (uint)BLOOMERY_DELTA_RING_ROWS - s) | |
| % (uint)BLOOMERY_DELTA_RING_ROWS; |
…, so a chunk at the u32 end does not wrap Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5zTV27LbaVZFTjnkdmTjJ
…2 end and compares with same_bits Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q5zTV27LbaVZFTjnkdmTjJ
There was a problem hiding this comment.
Since the last review:
- Ring snapshot overflow: fixed at crates/metal/kernels/delta.metal:919.
- Duplicate comparator: fixed; both snapshot checks reuse crates/metal/src/bin/gate_metal_qwen35moe_kernels/clauses_rows.rs:466.
- Timed revision: answered; the PR body identifies
0de8a6030.
Verdict: CLEAN: Both earlier must-fix items are resolved, and the incremental changes introduce no blocking defect.
Checked
Duplicates: 0 new functions, types or constants; the duplicate comparator was removed. Dryer reports 0 pairs.
Complexity: crapper's 10 flagged functions have no structural changes in this incremental diff.
Silent failure: 2 paths followed, snapshot validation and comparator errors; neither discards an error.
Tests: in a host model, restoring the old index at delta.metal:919 copies row 1 instead of row 8 into slot 2 in the added boundary case. The fixed index matches sequential ring writes in 2,083,796 slot cases, including empty initial history, one-row chunks and the final legal position.
Gates: one boundary case added; no bands, thresholds or clauses removed.
Escape catalog: no patterns matched.
Notes
Metal gates were not run here. Validation used source review and host arithmetic.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
Filed outside this PR: #795
One Metal prompt call per request, with points at the message starts inside it. Part of #593.
The server cuts a prompt call at the first and the last message start, so a later request can be cut back there and keep the recurrent state. The Metal engine asked for every cut, so chat turn 2 ran as three calls (5, 16 and 4 ids), each paying a pass or steps and a checkpoint. Its doc said such a cut is bit-neutral; since the batched pass it is not (a part of up to 8 ids steps, a longer one passes).
Engine::prefill_marked(ids, media, marks)(serve): the server runs every part of a prompt call through it, with the marks that fall in that part. The default isprefill/prefill_mediaand reads no mark, so every other engine runs as before, byte for byte.SwapEngineforwards text calls.Target::prompt_marked(runtime): the default runs oneprompta part, cut at each mark.Session'sprompt_marked. A mark inside a pass is a copy the pass leaves after the row before it (StateOut::after_rows, up to two a pass), taken as a point once the pass settles. A mark inside a run of steps ends that run. A third mark ends the pass there.gdn_rows_snapwrites the lane's registers after each named row into a copy;gdn_ring_snapwrites the ring a chunk ending at that row would leave, before the ring commit. The pass's own outputs do not move. The copy buffers open on the first pass that takes one.Gates:
delta_rows_snap: each copy equals, bit for bit, the lane and ring of a launch over rows 0..=r, and the copies move no output. FAIL-first: a copy one row late, a ring not copied.q_mark: copies move none of the pass's bits (pick, logits row, every store); the copy after the last row, cut back to, is the pass's end state; a copy's ring slots before the pass are the ring the pass found; a copy is the same beside any other. FAIL-first: a copy one row late, a ring not copied, a point taken from the live ring.q_mark_cut: 1024 ids marked at 700 and 1017, cut back to each and continued twice: the same bits both times, and four teacher-forced tokens withinq_free's band of the stepped run.k_keep:prefill_splitscuts nowhere andprefill_markedkeeps each mark.Gates (M1 Pro, fixture), each rc 0 at this branch's head (after the review fix:
cargo fmt --all --check,check-comments,just mac-metal-lint,just lab-metal,just lab-metal-e2eagain):cargo fmt --all --check,check-unsafe,check-comments,just check-recipes,just mac-metal-lint,just mac-lint(3 warnings, the ratchet's 3),cargo test -p bloomery-serve --lib --test serve -- --include-ignored(367 + 106 passed),cargo test -p bloomery-runtime(289 passed),just lab-metal,just lab-metal-e2e. Output lines:The CUDA engine is unchanged: its binding keeps
prefill_splitsand takes the defaultprefill_marked, so it runs the same calls as before.M4 Pro 48 GB, Qwen3.6-35B-A3B MLX 4-bit, development-loop numbers (not public). Chat through
bloomery-serve-metal -c 8192, greedy, base and after interleaved, twice each:Turn 1 is not settled at two runs (the first base run was an outlier). The first marked pass also opens the copy buffers (about 142 MB) once per process.
The plain prompt path (no marks) is unchanged, interleaved, arm order rotated:
Evidence: base = main e294ea7; after = 0de8a60 (the fix after it changes the ring copy's slot index only near the last u32 position, which no timed run reaches). macOS 27.0.1, Apple M4 Pro 48 GB; model file
Qwen3.6-35B-A3B-mlx4-00001-of-00001.gguf;generate_metal --prompt "$(cat p.txt)" -n 8 --ctx 8192,--pass-times 9,25,64,--prompt "The capital of France is" -n 64; release builds, one process per run. Load (vm.loadavg, 1 min) 2.97–7.98.The code and gates were written with Claude Code rounds and reviewed by the maintainer.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Q5zTV27LbaVZFTjnkdmTjJ