fix(ArcGraph): Tensor::arange in the captured V4 forward is a dangling host pointer (4 call sites) - #190
fix(ArcGraph): Tensor::arange in the captured V4 forward is a dangling host pointer (4 call sites)#190heydryft wants to merge 4 commits into
Conversation
…OST pointer
V4 decode capture reached "forward RECORDED" and then SIGSEGV'd on its very
first cuGraphLaunch, with a device-side assert from candle's index_select:
ids[id_i] < src_dim_size (T = __nv_bfloat16, I = unsigned int)
That dtype pair is the bf16 cos table indexed by U32 positions, i.e.
`self.cos.index_select(&positions, 0)` in
`DeepSeekV2RotaryEmbedding::forward_at_positions` — NOT the TD-MoE
`tid2eid.index_select` (tid2eid is i64, so it cannot produce T=bf16), and not
the embedding (its ids come from the already-address-stable graph-mode buffer).
`positions` was built one line earlier by `compressed_kv_from_rows` with
`Tensor::arange(0u32, t_c, dev)`. `arange` materialises a transient host `Vec`
and uploads it via `CudaDevice::clone_htod` -> an ASYNC `cuMemcpyHtoDAsync`.
Under `cuStreamBeginCapture` that copy is not executed, it is RECORDED: the
graph stores the HOST POINTER and re-reads it on the first launch and on every
replay. The `Vec` is freed when the expression returns, so the graph copied
freed host memory into `positions` and handed garbage indices to index_select.
This is not an address-staleness bug — it fires on the first launch, before any
replay — which is why the static input_ids buffer did not prevent it. candle
already documents this exact mechanism and works around it in
`CudaDevice::htod_info` (it leaks the host source while capturing); nothing
protects `clone_htod`, which is the path `arange` takes.
Fix: serve the strided compressed positions from a per-(ratio, device) table
built once, outside capture, and take a zero-copy `narrow` view each step. The
values are a pure function of (ratio, capacity) and never change. This also
removes a host round trip from every compressor layer of every decode step.
Keyed per ratio because V4 interleaves CSA and HCA compression ratios; a
single-slot cache would thrash and reintroduce the copy on every layer.
A sweep of every host->device constructor reachable from the B=1 decode forward
found three more `Tensor::arange` calls with the same defect as the compressor
positions, all in `dsv4_attention`, all unconditional on every layer of every
step:
:610 kp = arange(raw_base, raw_base + t_k) absolute key positions
:613 qp = arange(q0, q0 + t_q) query positions (ONE element
at decode -- a whole clone_htod
for 4 bytes, 43x per token)
:628 bp = arange(0, t_c) compressed-block indices
Each is a contiguous integer ramp cast to F32, so all three are views of a
single cached device ramp: `layers::positions_f32(start, len, dev)` narrows
`[0.0, 1.0, 2.0, …]`. Capacity grows by powers of two from 8192, so a typical
run allocates the ramp once during warmup and never again -- rebuilding inside
capture is precisely the hazard being removed.
Fixing only the compressor site would not have been enough: any one of these
would have re-armed the same dangling-host-pointer crash on the first launch.
Also removes three H2D copies per layer per token from the decode hot path
(129 per token at 43 layers), independent of graph capture.
|
Recording a decision on the two ArcGraph branches this one relates to, so they are not left as an implicit 'maybe later'.
Owner: whoever finishes |
|
🚫 UNVERIFIED — NOT MERGEABLE. Do not read the check list on this PR as a pass. Every CI run on this PR is in The standing rule is that This PR has never had a real CI verdict. Treat it as red until it does. I am re-triggering CI staggered (not all six at once — simultaneous dispatch is what exhausted the runners). Once this PR shows a genuine To re-trigger by hand: |
|
Cause identified, and CI re-triggered — this one CAN reach a genuine verdict. The single That is This PR targets The UNVERIFIED note stands until this shows a genuine |
Code Metrics Report━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Language Files Lines Code Comments Blanks ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ C Header 5 305 210 52 43 CSS 2 1181 1036 34 111 CUDA 74 25734 18232 4703 2799 Dockerfile 1 39 22 8 9 JavaScript 16 3546 2676 482 388 Jinja2 7 694 656 5 33 JSON 74 4600 4597 0 3 Makefile 1 6 5 0 1 Metal Shading Lan| 33 12224 9431 1142 1651 PowerShell 1 300 227 30 43 Python 145 15139 12482 811 1846 Shell 40 9948 6637 2655 656 Plain Text 4 3801 0 2479 1322 TOML 33 1498 1294 54 150 YAML 3 25 23 2 0 ───────────────────────────────────────────────────────────────────────────────── HTML 4 2687 2604 43 40 |- CSS 2 543 479 37 27 |- JavaScript 1 1233 1215 12 6 (Total) 4463 4298 92 73 ───────────────────────────────────────────────────────────────────────────────── Jupyter Notebooks 4 122 83 23 16 |- Markdown 1 60 30 22 8 |- Python 1 122 113 1 8 (Total) 304 226 46 32 ───────────────────────────────────────────────────────────────────────────────── Markdown 204 45330 0 35183 10147 |- BASH 72 1655 1203 331 121 |- C 3 17 17 0 0 |- CUDA 2 84 56 16 12 |- JSON 18 708 708 0 0 |- PowerShell 1 1 1 0 0 |- Python 23 1008 787 113 108 |- Rust 66 2051 1716 77 258 |- TOML 6 207 164 0 43 |- YAML 5 41 36 5 0 (Total) 51102 4688 35725 10689 ───────────────────────────────────────────────────────────────────────────────── Rust 673 331228 285444 16819 28965 |- Markdown 491 29538 471 25464 3603 (Total) 360766 285915 42283 32568 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Total 1324 495675 352655 90603 52417 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ |
Same root cause as #181, and the identical fix: `.typos.toml` already exempted the bare Rust identifier `arange` (candle's/numpy's `Tensor::arange`) under [type.rust.extend-identifiers], but that only matches WHOLE identifiers — the word inside `matches_the_arange_expression_it_replaced` still tripped the gate. Add the word-level exemption rather than renaming, because `arange` is a real API name, not a misspelling of 'arrange'. This branch and #181 both carry those test functions, so both hit it. Whichever lands first makes the other's copy redundant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two prior attempts (a push to the branch and a close/reopen of the PR) fired only the pull_request_target Analysis workflow; Continuous integration and CUDA compile check never dispatched for 489134b, leaving the PR displaying only 'comment' — an absence that reads as a pass. Empty commit to force a synchronize on a fresh sha. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🔴 UNVERIFIED note STANDS — and the reason is a trap worth naming, because nothing in the repo currently guards it. This PR displays Root cause: I confirmed this is a mechanism, not a fluke — three separate attempts to force dispatch all produced only
Zero 🔑 This is the same failure shape as the stacked-PR incident that motivated the Why it conflicts: it did not when opened. The seven merges I landed this session moved What it needs: a merge from master resolving those two files. I am not doing it here. Both are V4 model files on the capture path, and the fix this PR carries is itself about dangling host pointers during capture — resolving that conflict without auditing the interaction is precisely the kind of guess that should not be made on a queue-clearing pass, and there is no GPU to check it against. Its value is unchanged and still worth landing: 4 |
Retargeted at the integration branchBase changed: The queue is being restructured to the shape the owner asked for: one PR open against Two things had to land on
This PR was not closed and is not considered stale. An audit of the queue found the overwhelming majority of it to be real work that was never merged, not noise. What you need to do: rebase onto Retargeted, not closed — the supersession claim does not holdThis PR was put forward for closure as superseded by
So closing this would have discarded real, unmerged work — the four-call-site fix the title is about. Retargeted at
|
Rebased onto
|
| here | on release/openrouter-ready |
|---|---|
2743a4400 Tensor::arange in the captured forward is a dangling HOST pointer |
c1d2b27f4, same title |
fb58fc63c the other three arange calls in the captured V4 forward |
64fe1d678, same title |
On the positions_f32 question. It is true that git grep positions_f32 origin/master returns nothing, and that Tensor::arange still appears at
dsv4_attention.rs:372, 391, 396, 888, 896, 950. But neither fact means the fix
is missing — the helper was deliberately deleted, by this lane, in
6fcb87f47 on the integration branch:
layers::positions_f32is REMOVED, with itsIOTA_F32cache. #206
(02edd31cc) deleted two of its three call sites outright — att_q == 1the
raw-window mask is provably all-ones, so the kp/qp chain that fed it is gone —
and this rebase drops the third swap in favour of master's spelling. That left
apub fnwith ZERO callers workspace-wide, and two tests exercising nothing
but itself, which is a vacuous test by the repo's own definition.
So the capture path got a stronger fix than this PR's, not a weaker one.
#206 (a7b0890d9, the decode raw-window mask is all ones — 467
launches/token) does not merely move the two arange calls off the host, it
deletes them: at t_q == 1 — every decode step, i.e. the captured forward —
raw_valid is now Tensor::ones(..) and the compressed-branch block indices
come from layers::compress_positions(t_c, 1, dev), a zero-copy view of a table
built once outside capture.
The surviving Tensor::arange calls are not in the captured forward. Lines
888/896/950 all sit on the t_q > 1 arm (let decode_row = t_q == 1;), and
372/391/396 are in ragged_union_mask. Merging this PR would take the
positions_f32 side of those hunks and thereby revert #206's identity, revert
the graph_positions handling that keeps fixed-capacity graph decode correct,
and fail to compile besides — qp is now Option<Tensor>, and this branch's
code does &qp + 1.0.
What is left after rebase: five lines of .typos.toml adding arange to
[default.extend-words]. 6fcb87f47 chose the opposite fix on purpose —
renaming the test to matches_the_strided_range_expression_it_replaced, "which
is the precedent set in 745c871dd and e4eb59dfb" — and no identifier in the
tree currently contains arange inside a longer snake_case name, so the entry
would be dead on arrival.
Prefill-path arange in ragged_union_mask (3 calls) and on the t_q > 1
attention arm (3 calls) is still real, but it is not a capture-correctness
issue and is not what this PR patched. Worth a separate change if the prefill
budget wants it.
(No behaviour was changed on the integration branch by this analysis; the
branch was rebased locally only, to measure the residue.)
|
Superseded — see the analysis above. Rebases to an empty diff plus a typos allow-list entry the integration branch rejected. |
📌 Correcting the note that sent me here — so this does not get re-openedThis PR was handed to me with the instruction: "Its Both observations are true. The conclusion drawn from them is backwards, and The reasoning error is worth naming precisely, because it will recur: absence
And the surviving Merging this would have (a) reverted #206's all-ones identity, (b) reverted the This is the second time today a "missing from master" claim was wrong in this Rebased onto Staying closed. The genuine residue — six prefill-path |
1 participant
Standalone bug fix, orphaned in this session with no PR. Opening it because it is small, self-contained, and fixes a real correctness bug on the capture path.
Tensor::arangeinside the captured V4 forward allocates on the host and hands the kernel a pointer that is dangling by replay time. Four call sites acrosslayers.rs,models/deepseek4.rs, andmodels/dsv4_attention.rs.This is the same bug class that CLAUDE.md already warns about — "Never use
Tensor::{from_vec,arange}in hot loops" — but the capture path makes it worse than a sync: the pointer is not just slow, it is invalid on every replay after the first.Blast radius
.cuadded or modified.88d86a2— identical to master. Candle-neutral.Relationship to the rest of the ArcGraph work
arcgraph/capture-finalstrictly contains these two commits, so if that branch lands first this becomes redundant. Butcapture-finalalso carries a candle rev bump to9211966(master is on88d86a2), 41 commits, and is the base of live in-flight work — so it is a much bigger decision. This fix does not need to wait for it, and it is worth landing on its own.mistralrs-coreis outside the scoped Clippy lane CI gates on, so this code needs the by-hand check rather than relying on the lint job.🤖 Generated with Claude Code