Skip to content

[Bugfix] Bound accepted-token state lookups in GDN/KDA spec decode - #50021

Open
amittell wants to merge 6 commits into
vllm-project:mainfrom
amittell:bugfix/gdn-mtp-spec-decode-index-bounds
Open

[Bugfix] Bound accepted-token state lookups in GDN/KDA spec decode#50021
amittell wants to merge 6 commits into
vllm-project:mainfrom
amittell:bugfix/gdn-mtp-spec-decode-index-bounds

Conversation

@amittell

@amittell amittell commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

On a hybrid GDN model (Qwen3.5 / Qwen3.6) with MTP speculative decoding and prefix caching (--mamba-cache-mode align), the engine dies with CUDA error: unspecified launch failure within 7-10 requests of agent-shaped traffic. The GPU faults, not the runtime: the signature is an SM address exception (Xid 13, ESR 0x404000) or an MMU fault (Xid 31), depending on whether the wild address happens to be mapped.

Validation scope (added 2026-08-19). Everything below was developed and validated with
--mamba-cache-mode align and --enforce-eager. That matters: align is the only branch in
which gpu_model_runner still calls num_accepted_tokens_event.synchronize() (the wait is
skipped when use_async_scheduling and mamba_cache_mode != "align"), and eager mode removes
CUDA-graph stream structure. So this PR addresses accepted-count-derived indices that are
correctly synchronized but out of range. It does not address the separate cross-stream
race reported by @noonghunna under async scheduling with the default (non-align) mamba cache
mode, where the accepted-count value itself arrives unsynchronized — in-kernel bounds cannot
repair a value that arrives wrong. See the discussion in this thread; that path needs stream
ordering, not bounds. Note mamba_cache_mode resolves per-model: with prefix caching and no
explicit flag it is "all" (sync skipped) when model_config.supports_mamba_prefix_caching,
else "align" (sync kept).

This is the crash half of the problems reported around hybrid-Mamba + MTP. It is distinct from the prefix-cache corruption in #43559 (the coordinator-level EAGLE cache-peek gating for Mamba), which is already handled on current main. This PR does not touch that path and does not claim to fix #43559; it fixes a separate GPU fault in two kernels downstream.

Root cause: unchecked accepted-count-derived indices

Speculative decoding produces a per-request accepted-token count. Multiple GPU state consumers turn that count into an array index without bounding it, so a count that is stale, zero, or too large indexes outside its tensor and yields a wild address the GPU then dereferences.

Site 1 fused_recurrent_gated_delta_rule_fwd_kernel (fused_recurrent.py):

i_t = tl.load(num_accepted_tokens + i_n).to(tl.int64) - 1        # unbounded
state_idx = tl.load(ssm_state_indices + i_n * stride_indices_seq + i_t)
if state_idx <= 0: return                                        # positive garbage passes
p_h0 = h0 + state_idx * stride_init_state_token                  # dereferenced

i_t (= count - 1) is unbounded against a stride_indices_seq-column tensor. A zero accepted count gives i_t == -1, a read before this request's row (before the tensor for i_n == 0); a stale or too-large count reads past the row. The state_idx <= 0 guard only rejects non-positive values, so an out-of-range read that returns a garbage positive int flows into the address math and faults the SM.

Site 2 _copy_mamba_state_block (mamba_utils.py): the block-table columns derive from the same count and index the per-request block-table row unbounded; the loaded block id becomes state_base_addr + block_id * state_block_stride, which is then read and written.

The two sites fire under different loads. A light decode load exercises only Site 1; a heavy cache-transition load (long prefills, A->B->A prefix reuse) also drives Site 2. Both must be bounded.

The fix

Both loads are masked to the valid range, so an out-of-range index falls into the existing invalid-state path instead of producing an address. No stream-ordering change, no device sync, no measurable throughput cost.

Test plan

RTX 5090 (sm_120), Qwen3.6-27B NVFP4, TP=1, --enforce-eager --max-model-len 16384, MTP 3 (qwen3_5_mtp), prefix caching on, align mode.

build crash (Xid) corruption (A->B->A probe)
stock dies at 7-10 requests, Xid every run 0/N reproduced (already fixed on main)
Site 1 fix alone survives light load; still crashes on the first heavy probe (Xid 31) 0/N reproduced
both fixes 68 heavy probes + a 39-minute soak, 0 new Xid 0/68 reproduced

The A->B->A probe issues an agent-shaped sequence (long prefill, prefix reuse, 20 tool schemas) and reports whether a poisoned prefix reappears; 68/68 returned a clean verdict, which also confirms the out-of-range early-return does not drop a needed state copy. Warm throughput and MTP acceptance length (3.98-4.00 of 4) are unchanged.

Note: a separate, still-open livelock (#49203)

Independently of this crash, the same stack can occasionally hang: engine alive, /v1/models answering, but the in-flight request stuck at 0% GPU util with no Xid. It is rare and timing-variable (seen once, then not across the 68 probes here) and matches open issue #49203. This PR does not address it and does not claim to; the 68/68 clean-verdict result rules out this change as a cause.

Follow-up bounds audit

A follow-up audit expanded the same fail-closed rule to the remaining consumers in this path:

  • Both FLA wrappers now zero rejected output deterministically instead of returning with new_empty storage visible downstream.
  • _causal_conv1d_update_kernel now bounds the accepted-count offset before state address math; invalid active rows produce zero output and leave state unchanged.
  • GPU regressions cover too-small/too-large accepted counts, NULL block IDs, source/destination columns crossing the block-table row, and temporal-bias overflow.

Validation after this follow-up: full pre-commit passed; on an RTX 5090, 186 FLA/causal-conv kernel tests and all 21 fused Mamba postprocess tests passed (207 total).

Kimi K3 KDA expansion

The same accepted-count-derived state selection pattern also existed in Kimi K3 KDA fused recurrent decode, in both the NVIDIA and AMD vendored kernels. This PR now masks that initial state-index load to the request row, zeroes all invalid-count output tokens, preserves state on invalid counts, and releases NVIDIA PDL dependents before the new empty/invalid early returns.

Additional validation on mini-beast RTX 5090:

  • Patched KDA invalid-count test: 4/4 passed across NVIDIA and AMD implementations (num_accepted 0 and 4).
  • Existing KDA spec-decode correctness plus the new invalid-count target: 12/12 passed.
  • Negative control at previous PR head e7f66b199 with only the new test added: 4/4 failed on old KDA for NVIDIA and AMD, proving the regression is non-vacuous.

AI assistance disclosure

OpenAI Codex assisted with the follow-up bounds audit, implementation, and regression-test generation. The submitter owns the conclusions and final review.

Mamba2 selective-state expansion

The Mamba2 selective-state-update kernel also derives its initial state-slot lookup from num_accepted_tokens - 1. It already clamps the lower side, but had no upper row bound. This update preserves that zero-count behavior and makes an oversized count fail closed: it writes zero output and returns before reading or writing state.

RTX 5090 red/green regression: a count one past a three-column state row made the prior source select an adjacent row and emit nonzero output; the new kernel passes by emitting zero output and preserving the complete state tensor. Pre-commit passes for both changed files.

Copilot AI review requested due to automatic review settings July 27, 2026 17:29
@amittell
amittell requested a review from njhill as a code owner July 27, 2026 17:29

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@mergify mergify Bot added the bug Something isn't working label Jul 27, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dd1fd0389c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread vllm/v1/worker/mamba_utils.py Outdated
Comment thread vllm/third_party/flash_linear_attention/ops/fused_recurrent.py
Comment thread vllm/third_party/flash_linear_attention/ops/fused_recurrent.py

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the hybrid GDN + MTP speculative decode path against CUDA illegal memory accesses by bounding two indices derived from num_accepted_tokens, ensuring out-of-range acceptance counts don’t turn into out-of-bounds reads and wild pointer dereferences in downstream Triton kernels.

Changes:

  • Mask the ssm_state_indices load in fused_recurrent_gated_delta_rule_fwd_kernel so invalid num_accepted_tokens values fall into the existing “invalid state” early-return path.
  • Mask block-table column loads in _copy_mamba_state_block to prevent out-of-range accepted-token-derived columns from reading outside a request’s block-table row.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
vllm/v1/worker/mamba_utils.py Adds masked block-table column loads in _copy_mamba_state_block to prevent out-of-range accepted-token-derived indexing from producing illegal addresses during state copies.
vllm/third_party/flash_linear_attention/ops/fused_recurrent.py Masks the initial-state index lookup derived from num_accepted_tokens - 1 to avoid out-of-bounds reads and subsequent invalid state dereferences.
Comments suppressed due to low confidence (3)

vllm/v1/worker/mamba_utils.py:102

  • Same sentinel issue as above: block ID 0 (NULL_BLOCK_ID) should be treated as invalid. Otherwise, an in-range but unallocated src_col can cause reads from the reserved padding block 0.
        src_block_id = tl.load(
            block_table_base + src_col, mask=src_col_ok, other=-1
        ).to(tl.int64)
        if src_block_id < 0:
            return

vllm/v1/worker/mamba_utils.py:129

  • Same sentinel issue as above: block ID 0 (NULL_BLOCK_ID) should be treated as invalid to avoid copying from the reserved padding block.
        src_block_id = tl.load(
            block_table_base + src_col, mask=src_col_ok, other=-1
        ).to(tl.int64)
        if src_block_id < 0:
            return

vllm/v1/worker/mamba_utils.py:150

  • Same sentinel issue as above in the temporal-state path: block ID 0 (NULL_BLOCK_ID) should be treated as invalid. Otherwise an in-range but unallocated tmp_col can copy from the reserved padding block.
    actual_src_block_id = tl.load(
        block_table_base + tmp_col, mask=tmp_col_ok, other=-1
    ).to(tl.int64)
    if actual_src_block_id < 0:
        return

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread vllm/v1/worker/mamba_utils.py Outdated
Comment thread vllm/v1/worker/mamba_utils.py Outdated
@amittell

Copy link
Copy Markdown
Contributor Author

Thanks for the review — pushed 09edaaa addressing the substantive findings.

Bound the sigmoid-gating kernel (Codex P1). Correct and important: the Qwen3.5/Qwen3.6 GDN decode path calls fused_sigmoid_gating_delta_rule_update (qwen_gdn_linear_attn.py:1377,1404), not fused_recurrent_gated_delta_rule (that one is the OLMo path). fused_sigmoid_gating.py has the identical unbounded ssm_state_indices load, so the original PR left Qwen's actual kernel unpatched. Applied the same row-bounded mask there.

Reject NULL_BLOCK_ID in the copy (Codex P1 + Copilot). Right on both counts — NULL_BLOCK_ID is 0, not negative. Changed all four block-id guards in _copy_mamba_state_block from < 0 to <= 0, so a stale-but-in-range column landing on an unallocated (0) slot is rejected instead of copying block 0. This also matches the FLA kernels' own state_idx <= 0 convention. Fixed the comment that wrongly called 0 "negative".

Initialize outputs when rejecting (Codex P1). The if state_idx <= 0: return early-exit is pre-existing upstream behavior for the legitimate NULL-state case — the mask only routes out-of-range indices into that same existing path instead of faulting, so the output-write semantics are unchanged from main (the caller already tolerates unwritten positions for NULL states). I left this as-is rather than add output-zeroing, which would change behavior beyond the bug; happy to revisit if a maintainer prefers explicit init.

The masking change was validated end-to-end on an RTX PRO 6000 (SM120, driver 610) with Qwen3.6-27B-NVFP4, MTP-3, prefix caching, fp8 KV, full PIECEWISE cudagraph: 0 GPU faults across a long-prompt sweep to 32k tokens, 0 cache poisoning, 149-158 t/s warm.

Copy link
Copy Markdown

Independent GPU red/green confirmation

I independently exercised the exact current-source bounds changes on RTX 5090/SM120 with Qwen3.6 hybrid GDN + MTP n=3.

Five deterministic GPU regressions failed before the bounded-index changes and passed with all three files mounted:

  • accepted-token row bound in fused recurrent;
  • the corresponding fused sigmoid-gating/GDN bound;
  • Mamba source block-table row bound;
  • destination row bound;
  • temporal-bias source-row bound.

This is independent of the FlashInfer/XQA MTP routing issue in #49010: fixing the attention route restored long-context recall, but it does not remove this wild-address/adjacent-state class. I therefore consider this PR required for a production-safe hybrid-GDN MTP stack, not a workaround for the attention bug.

@amittell

amittell commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

The contributor-side checks and requested GPU evidence are complete. The remaining pre-run-check failure is the repository admission gate: it requires a maintainer-applied verified, ready, or ready-run-all-tests label (the workflow reports my merged-PR count as 1). Could a maintainer please apply the appropriate gate label so the full test workflow can run?

justtestingthingsx pushed a commit to meandmyboiclaude/vllm that referenced this pull request Aug 7, 2026
justtestingthingsx pushed a commit to meandmyboiclaude/vllm that referenced this pull request Aug 7, 2026
…bounded block-table load in the SD-conv branch)
@mergify

mergify Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @amittell.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@amittell

amittell commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 71171a880 after one final non-duplicative consumer audit. Mamba2 selective state update already clamps a zero accepted count, but its upper accepted-count-derived state-row index was unbounded. This commit preserves the established zero behavior and makes an oversized count fail closed (zero output; no state read/write).

RTX 5090 red/green proof: with a three-column state row and num_accepted=4, the prior head selected an allocated adjacent row and emitted nonzero output; the patched head passes by zeroing output and preserving the complete state tensor. Pre-commit passes on the two changed files.

This intentionally does not duplicate #51508: that PR handles stale zero-count state semantics at the metadata layer; this is the missing upper-row bound in the Mamba2 kernel.

Comment thread vllm/model_executor/layers/mamba/ops/mamba_ssm.py Outdated
@mergify

mergify Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @amittell.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Aug 17, 2026
…e path

On a hybrid GDN model (Qwen3.5/Qwen3.6) with MTP speculative decoding and
prefix caching (--mamba-cache-mode align), the engine dies with "CUDA error:
unspecified launch failure" within 7-10 requests of agent-shaped traffic. The
GPU faults (Xid 13 SM address exception, or Xid 31 MMU fault), not the runtime.

Speculative decoding produces a per-request accepted-token count. Two kernels
turn that count into an array index without bounding it, so a stale, zero, or
too-large count indexes outside its tensor and yields a wild address that the
GPU then dereferences.

Site 1, fused_recurrent_gated_delta_rule_fwd_kernel: i_t (num_accepted_tokens
minus 1) is unbounded against a stride_indices_seq-column tensor; i_t == -1
reads before the row. The existing state_idx <= 0 guard still passes a garbage
positive index into h0 + state_idx * stride_init_state_token.

Site 2, _copy_mamba_state_block: the block-table columns derive from the same
count and index the per-request block-table row unbounded; the loaded block id
becomes state_base_addr + block_id * state_block_stride, then read and written.

Both loads are masked to the valid range so an out-of-range index falls into
the existing invalid-state path instead of producing an address. No
stream-ordering change, no device sync, no measurable throughput cost.

Signed-off-by: Alex Mittell <mittell@me.com>
Address review feedback:

- fused_sigmoid_gating.py: the Qwen3.5/Qwen3.6 GDN decode path calls
  fused_sigmoid_gating_delta_rule_update, not fused_recurrent_gated_delta_rule,
  and its kernel has the identical unbounded ssm_state_indices load. Apply the
  same row-bounded mask so the Qwen path is actually covered.

- mamba_utils.py: NULL_BLOCK_ID is 0, not negative. The out-of-range early
  return used ``< 0`` (only the -1 sentinel), so a stale-but-in-range column
  landing on an unallocated (0) slot would still copy block 0. Change all four
  block-id guards to ``<= 0`` so both the sentinel and NULL_BLOCK_ID are
  rejected, matching the FLA kernels' own ``state_idx <= 0`` convention. Fix the
  comment that wrongly described 0 as negative.

Signed-off-by: Alex Mittell <mittell@me.com>

Signed-off-by: Alex Mittell <amittell@gmail.com>
Zero rejected kernel output deterministically, fail closed before causal-conv state address math, and exercise block-table row bounds with GPU regressions.

Assisted-by: OpenAI Codex
Signed-off-by: Alex Mittell <mittell@me.com>
Mask Kimi K3 KDA's accepted-token-derived state-index load to the request row for both vendored NVIDIA and AMD recurrent kernels. Invalid accepted counts now fail closed by zeroing output and preserving state instead of selecting an adjacent state row.

Add a shared KDA regression that covers zero and too-large accepted counts for both implementations. The test keeps old out-of-row reads inside allocated storage so failures are deterministic semantic regressions.

Assisted-by: OpenAI Codex
Signed-off-by: Alex Mittell <mittell@me.com>
Assisted-by: OpenAI Codex
Signed-off-by: Alex Mittell <mittell@me.com>
…ment stride

The 71171a880 bound compared init_token_idx against stride(1) of the
state-indices tensor -- 1 for a contiguous [batch, T] layout -- so every
accepted count > 1 was wrongly failed closed (zero output, no state
update), and a non-contiguous layout with stride(1) > stride(0) would have
reopened the out-of-bounds lookup. The invalid-count regression could not
catch this: an oversized count fails closed under both the wrong and the
correct bound.

Bound against stride(0) (elements per batch row; == T when contiguous),
matching fused_recurrent.py's i_t < stride_indices_seq pattern, and add
the discriminating valid-side regression: num_accepted=2 in a 3-column row
must produce real output and a state update.

RTX 5090 evidence: the new test fails on 71171a880 exactly as predicted
(all-zero output) and passes here; the invalid-count test passes on both
(demonstrating it was non-discriminating); the full
tests/kernels/mamba/test_mamba_ssm.py file passes 288/288 (18 skipped).

Found by the depthfirst review bot on the PR diff.

Assisted-by: Claude (Anthropic)
Signed-off-by: Alex Mittell <mittell@me.com>
@amittell
amittell force-pushed the bugfix/gdn-mtp-spec-decode-index-bounds branch from a303cf1 to 9a198c0 Compare August 17, 2026 13:04
@mergify mergify Bot removed the needs-rebase label Aug 17, 2026
ch2lab added a commit to ch2lab/vllm that referenced this pull request Aug 18, 2026
…DA state indices

Replaces the local vllm-project#48475-style clamp with the upstream bounds fix
(PR vllm-project#50021, open): masked row-bounded loads for ssm_state_indices and
block-table columns, fail-closed zero output for invalid counts, so
stale/too-large accepted counts can no longer dereference wild state
addresses (Xid 13/31). Clamps kept as defense in depth in FLA kernels.
ch2lab added a commit to ch2lab/vllm that referenced this pull request Aug 18, 2026
…tale zero-accept rows

Builder-level fix (open PR vllm-project#51508): rows whose async-scheduling step was
discarded get their whole spec_state_indices row set to NULL_BLOCK_ID,
so FLA/conv kernels skip them entirely (no initial-state read, no
final-state write) instead of advancing state for a dead request. Kernel
clamps retained as defense in depth (already applied via vllm-project#50021 port).
ch2lab added a commit to ch2lab/vllm that referenced this pull request Aug 18, 2026
Drop the local vllm-project#48475-style clamps in fused_recurrent/fused_sigmoid_gating
so a zero num_accepted_tokens falls through the row-masked load into the
invalid-state path (zeroed output, state untouched) per PR vllm-project#50021, instead
of silently reading slot 0. Adapt the PR vllm-project#51508 defense test that assumed
clamp equivalence, and point the GDN builder fixture at the local
Qwen3.5-0.8B checkpoint for offline runs.
@noonghunna

Copy link
Copy Markdown

We maintain a serving stack for hybrid-GDN Qwen models on consumer GPUs and have multiple field reports of this crash class. We built a fast reproducer and ran a controlled matrix against v0.27.1, including this PR's current head. Three results that seem important:

1. This PR's head does not stop the crash. We applied the five runtime-file changes at 9a198c0 onto vllm/vllm-openai:v0.27.1 (three files are byte-identical to the PR base outside its hunks and were used verbatim; the fused_recurrent.py/mamba_utils.py hunks were ported onto the v0.27.1 copies; all five verified loaded). The engine died twice under the reproducer with the same signature — Xid 31 MMU faults of type VIRT_WRITE, at the identical virtual address both times — including once from a 100% recent-acceptance state, so the zero/stale accepted-count precondition the bounds target is not the (only) faulting path.

2. The crash is gated on async scheduling — it looks like a cross-stream race, not an index bug. Matched single-variable A/B, identical client and config (MTP n=4, --enable-prefix-caching, fp8 KV, TP=2 on 2× RTX 3090, driver 610.57.04):

async scheduling outcome (5-10 min agent-shaped soak, growing multi-turn conversation)
on (default) dies at 6.4k-12.9k generated tokens — 5/5 runs (illegal access surfacing in gdn_attn.py build() / synchronize_input_prep event frames, always on a cache-hit resume step with spec tokens scheduled)
--no-async-scheduling survives to 33k+ generated, full arc incl. a 35k-ctx full-history prefill
on + CUDA_LAUNCH_BLOCKING=1 survives full arc

Drafter-off and prefix-caching-off arms also never fault. Our reading: step N+1's metadata build consumes num_accepted_tokens (device tensor written by step N's rejection sampler) via the boolean gather at gdn_attn.py:326, and h2d's spec_sequence_masks at :207, while overlapped with step N's execution — a stale/garbage value there is dereferenced downstream as a state-block address. In-kernel bounds can't repair a value that arrives wrong.

3. Separately, we found a second, distinct fault while isolating this one — crossing sequence position 32,768 with the MTP drafter active permanently kills draft acceptance engine-wide (reproduces with async scheduling disabled, so it is not this PR's bug). Filed with full evidence and reproducer as #52873.

Reproducer below (stdlib-only; point at an OpenAI-compatible endpoint; min_tokens: 400 standardizes exposure — note min_p-style params are rejected under spec decode but min_tokens is accepted). Happy to run candidate fixes or instrumented builds — this turns "hours of agent traffic" into a ~10-minute red/green, and the position-32768 half into a deterministic boundary test.

Reproducer
#!/usr/bin/env python3
"""Fast reproducer: CUDA illegal memory access (Xid 31) on Qwen3-Next-family
hybrid GDN + MTP spec decode + prefix caching, vLLM v0.27.1.

Shape: a single growing multi-turn conversation (agent-style). Every turn is a
prefix-cache-hit resume with speculative tokens scheduled. On 2x RTX 3090 (TP=2,
fp8 KV, MTP n=4) the engine dies with `gdn_attn.py build -> illegal memory access`
within 6k-13k generated tokens (~5-10 min), reproduced 4/4 runs. With the drafter
off (SPEC_N=0) the same traffic runs indefinitely.

Usage: python3 gdn-mtp-apc-repro.py [base_url] [model]
"""
import json, sys, time, urllib.request

BASE = sys.argv[1] if len(sys.argv) > 1 else "http://localhost:8095"
MODEL = sys.argv[2] if len(sys.argv) > 2 else "qwen3.8-27b"
SYS = ("You are a senior systems engineer pair-programming with the user on a large "
       "C++ and Python codebase for GPU inference. Be concrete and show code. ") * 40

TOPICS = [
    "Refactor this CUDA kernel launcher to support streams. Write the full code.",
    "Design a block-paged KV cache allocator; write the C++ header with comments.",
    "Write a Python asyncio scheduler for batched LLM requests with timeouts.",
    "Explain then implement speculative-decoding token verification in PyTorch.",
    "Write unit tests for a ring-buffer class; include edge cases and comments.",
    "Port this concept to Triton: fused RMSNorm + residual add. Full kernel please.",
    "Debug: our TP=2 all-reduce hangs on PCIe. List hypotheses, then a bisect plan.",
    "Write a bash script that soaks an OpenAI endpoint and logs TPS per request.",
]

def chat(messages, max_tokens=1000):
    req = urllib.request.Request(BASE + "/v1/chat/completions",
        data=json.dumps({"model": MODEL, "messages": messages, "max_tokens": max_tokens,
                         "temperature": 0.7, "top_p": 0.95,
                         "chat_template_kwargs": {"enable_thinking": False}}).encode(),
        headers={"Content-Type": "application/json"})
    with urllib.request.urlopen(req, timeout=600) as r:
        out = json.loads(r.read().decode())
    ch = out["choices"][0]["message"]["content"] or ""
    return ch, out.get("usage", {}).get("completion_tokens", 0)

msgs = [{"role": "system", "content": SYS}]
total, turns = 0, 0
while total < 220_000:
    msgs.append({"role": "user", "content": f"[turn {turns}] {TOPICS[turns % len(TOPICS)]} "
                 f"Focus on a module named engine_{turns} with a {5 + turns % 7}-stage pipeline."})
    try:
        content, comp = chat(msgs)
    except Exception as e:
        print(f"FAILURE after {total} generated tokens: {e}")
        print("check: docker logs <container> | grep -m5 'illegal memory' ; dmesg | grep Xid")
        sys.exit(2)
    if len(content.strip()) < 20:
        msgs.pop(); turns += 1; continue   # keep the history well-formed
    msgs.append({"role": "assistant", "content": content})
    total += comp; turns += 1
    if turns % 10 == 0:
        print(f"generated={total} turns={turns}", flush=True)
print("no failure up to 220k generated tokens")

@amittell

Copy link
Copy Markdown
Contributor Author

Thank you — the matched single-variable A/B plus the CUDA_LAUNCH_BLOCKING=1 arm is what turns "probably a race" into "a race", and I think you are right on all three points. Two things I can add: the exact gate that produces it, and why this PR's head was never going to move your needle.

The accepted-count sync is conditional, and prefix caching turns it off

vllm/v1/worker/gpu_model_runner.py (v0.27.1, ~L2107):

# Sync num_accepted_tokens from CPU (set by
# _update_states_after_model_execute for hybrid models).
# Skipped under async scheduling (non-align): the CPU copy races with
# the in-flight D2H copy and with input-batch row moves.
needs_cpu_accepted_counts = self.num_accepted_tokens_event is not None and not (
    self.use_async_scheduling and self.cache_config.mamba_cache_mode != "align"
)
if needs_cpu_accepted_counts:
    assert self.num_accepted_tokens_event is not None
    self.num_accepted_tokens_event.synchronize()

num_accepted_tokens_event is recorded immediately after num_accepted_tokens.gpu is written (L1586 → L1607 / L1613), and the synchronize() above is the only consumer of it. It is skipped whenever use_async_scheduling and mamba_cache_mode != "align". CacheConfig.mamba_cache_mode defaults to "none" and resolves to "all" — not "align" — once prefix caching is enabled, so async + APC + MTP (your config) lands squarely in the skip branch: the event is recorded and never waited on.

gdn_attn.py build() then consumes that tensor anyway — the boolean gather you flagged at L326, and a non_blocking=True copy into the persistent self.num_accepted_tokens buffer at L460. That is exactly the shape you inferred from the outside, and the comment in that gate is upstream's own: this code already knows there is a race in the area and resolves it by dropping the CPU-side path, not by making the GPU-side read stream-safe. Which is why, as you put it, in-kernel bounds cannot repair a value that arrives wrong. Agreed — and that is a fair criticism of bounds-as-a-fix for your failure.

Falsifiable in one flag: --mamba-cache-mode align

If that gate is the mechanism, align should survive your reproducer with async scheduling left on, because it is the one branch that keeps the synchronize(). That distinguishes "async scheduling is unsafe for hybrid GDN" from "this specific sync gate is unsafe", and it is a single flag on your existing rig.

Our data at that config point, using your reproducer verbatim:

config result
Qwen3.8-27B hybrid GDN, MTP-3, APC, --mamba-cache-mode align, async scheduling auto-on, TP=1, v0.26.0 16,918 generated tokens, 0 faults, engine healthy — straight through the 6.4k–12.9k window where you died 5/5
Same recipe in production (NVFP4, RTX 6000 Blackwell, TP=1, align, async on) no illegal memory access in the unit's journal history; no Xid in dmesg on that host

Caveat I will flag myself: both of our arms are TP=1 on Blackwell, so this is consistent with the gate being the variable, not proof of it — TP=2 rank synchronization is its own timing regime, and your 2×3090 result may well have a second contributing factor. I have a non-align arm (mamba_cache_mode="all", everything else identical, same node class) staging now and will post it either way; if it faults where the align arm did not, the gate is confirmed independently of arch and TP.

Why this PR's head did not help you

The PR was developed and validated entirely in align mode with --enforce-eager — see the repro line and the test plan in the description. Both sidestep the async race: align keeps the sync, and eager mode removes the CUDA-graph stream structure. So it targets a different defect — accepted-count-derived indices that are correctly synchronized but still out of range (zero count → i_t == -1 reads before the row; stale or oversized count reads past it), which is deterministic and reproduces with async scheduling off. Its negative control (test_invalid_block_table_lookup_does_not_copy_state) still fails 4/4 against pristine main for us, so the defect it bounds is live independent of yours.

So I read your result as "there are at least two bugs here", not "the bounds are wrong". That said, your run shows how easily the PR's scope reads as broader than it is, so I will make the description state the align + eager validation scope explicitly rather than leaving align as a parenthetical, and reference this comment for the async/non-align path.

What a fix for your half probably looks like

Not more bounds: the GPU-side consumer needs to be ordered against the producer. Either the metadata build waits on num_accepted_tokens_event unconditionally (the current gate makes the wait conditional on a cache mode that has nothing to do with whether the GPU tensor is safe to read), or the L326/L460 reads move onto the stream that wrote them so the ordering is implicit rather than host-mediated. Happy to test a candidate patch of either shape.

On #52873

The position-32,768 acceptance collapse reproducing with async scheduling disabled does read as independent, and a hard boundary at exactly 2^15 is a strong hint on its own. We have hybrid-GDN capacity across Blackwell (RTX 6000, GB10, 5090) and can run that as a deterministic boundary sweep if a second data point on different silicon would help.

@amittell

Copy link
Copy Markdown
Contributor Author

Correction to my previous comment, and it sharpens the predicate rather than weakening it.

I wrote that mamba_cache_mode "resolves to "all" — not "align" — once prefix caching is enabled". That is wrong as stated. The actual resolution (vllm/model_executor/models/config.py, identical in v0.26.0 and v0.27.1) is per-model:

if cache_config.enable_prefix_caching:
    if cache_config.mamba_cache_mode == "none":
        cache_config.mamba_cache_mode = (
            "all" if model_config.supports_mamba_prefix_caching else "align"
        )

So with prefix caching on and no explicit flag, a model that supports mamba prefix caching gets "all" (→ use_async_scheduling and mamba_cache_mode != "align" is true → num_accepted_tokens_event.synchronize() is skipped), and one that does not gets "align" (→ the sync is kept).

How I caught it: I claimed our fleet was a clean data point at the same config as yours. It isn't. Our Qwen3.8-27B logs

Mamba cache mode is set to 'align' for Qwen3_5ForConditionalGeneration by default
when prefix caching is enabled

i.e. it auto-selects align and lands in the sync-kept branch even though I never passed the flag in that arm. So my "16,918 tokens, 0 faults" run was an align run either way, and both of my arms were the same arm. That number therefore says nothing about non-align; please discount it as an A/B. It remains a data point that async + MTP + APC is stable in the align branch on Blackwell/TP=1, which is all I should have claimed.

The upshot for your report is a tighter exposure predicate than I gave:

async scheduling on and supports_mamba_prefix_caching for the model and MTP/Eagle-class spec decode (so the event exists) and no explicit --mamba-cache-mode align

which fits Qwen3-Next-family + APC landing in "all", and fits --no-async-scheduling and CUDA_LAUNCH_BLOCKING=1 both making it disappear. The mechanism I described — the event being recorded and never waited on, while gdn_attn.py reads that tensor at L326 and non_blocking=True copies it at L460 — is unchanged; only my account of who lands in the skip branch was sloppy.

--mamba-cache-mode align therefore remains the one-flag test/workaround worth trying on your rig, and now with a clear reason it should work: it forces the branch that keeps the synchronize, without giving up async scheduling. The cost is that align caches mamba state only at step boundaries, so on a model that supports "all" you would be trading some prefix-cache reuse for the sync — worth knowing before anyone ships it as a fix rather than a diagnostic.

I have the genuine non-align arm running now (same node, same model, everything identical, but --mamba-cache-mode all forced so it enters the skip branch), and will report it whichever way it falls. If it faults where the align arm did not, that isolates the gate on hardware and a TP setting quite different from yours.

@amittell

Copy link
Copy Markdown
Contributor Author

Non-align arm, as promised — and it did not reproduce, which is evidence against the gate being sufficient on its own. Reporting it because I said I would either way.

Single variable against my earlier align run: same node, same model, same flags, but --mamba-cache-mode all forced so the engine genuinely enters the skip branch. Resolved args confirm it ('mamba_cache_mode': 'all', enable_prefix_caching: True, speculative_config {'method': 'qwen3_5_mtp', 'num_speculative_tokens': 3}), and the engine logged Asynchronous scheduling is enabled., so needs_cpu_accepted_counts is False and num_accepted_tokens_event.synchronize() is never called.

arm resolved mode result
A align (auto-selected) 16,918 generated, 0 faults
B all (forced) → sync skipped 22,034 generated over 108 turns / 1,250 s, 0 faults, engine healthy

So on Qwen3.8-27B, TP=1, single Blackwell (GB10), v0.26.0, MTP-3, APC, your reproducer verbatim: the missing synchronize is not sufficient to produce the fault. That is a real strike against my "this gate is the mechanism" framing, and you should weight it accordingly.

What I think survives, and what does not:

  • Survives: the unsynchronized read itself is not in doubt — the event is recorded and, in this branch, never waited on, while gdn_attn.py consumes that tensor at L326 and non_blocking=True copies it at L460. That is a genuine ordering hazard in the code regardless of whether it fires here.
  • Does not survive: my implication that entering the skip branch is what turns it into your Xid. Something else is required, and the most structurally different variable left is TP=2. Rank synchronization inserts collectives into exactly the window where step N's sampler write and step N+1's metadata build overlap, which changes both the timing and which stream the write retires on. The other unexamined deltas are v0.27.1 vs v0.26.0, Ampere vs Blackwell, MTP n=4 vs n=3, and fp8 KV vs default.

Given that, the --mamba-cache-mode align test on your rig is now more informative than before, not less — it discriminates cleanly:

  • if align fixes it on 2×3090/TP=2, the gate is load-bearing and my negative result just means TP=1/Blackwell doesn't lose the race;
  • if align does not fix it, the gate is a red herring and the fault is somewhere else in the overlap — worth knowing before anyone spends time on stream ordering there.

Honest limits on our side: we cannot currently match your rig. Our Blackwell pair is both GPUs in production, and the GB10s are one GPU per node, so a TP=2 arm would need multi-node Ray rather than a same-box tensor-parallel split — not a like-for-like substitute for two 3090s on one host. If you can run the one-flag align arm, that is the cheapest discriminator available to either of us. If it would help, I can also rerun arm B under v0.27.1 to remove the version delta, since that is the one variable I can change cheaply.

@noonghunna

Copy link
Copy Markdown

This is the missing piece, and it resolves your arm B cleanly: our config runs align, not the skip branch. Every one of our boots logs

Mamba cache mode is set to 'align' for Qwen3_5ForConditionalGeneration by default when prefix caching is enabled

Qwen3.8-27B reports supports_mamba_prefix_caching = False, so it auto-selects align and lands in the sync-kept branch — needs_cpu_accepted_counts is True and num_accepted_tokens_event.synchronize() does run for us. So we were never in your arm-B path, and your "missing sync is not sufficient" result and our crash are not in tension: they're different branches.

Which means, for our failure, align is not a fix — we crash in align, with the synchronize present. The one-flag test you proposed is effectively already run: we're in align by default and we die 5/5. So the predicate isn't "the sync is skipped"; for us it's "the sync runs too late."

Where "too late" is

In align, the synchronize lives inside _prepare_inputs (~L2116), and _prepare_inputs's own block-table copies to GPU (L2136 num_accepted_tokens, L2267 num_decode_draft_tokens) come after it — so those are already ordered. The thing that is not ordered is _update_states(scheduler_output), which runs earlier in the same synchronize_input_prep block, before that synchronize. Under async scheduling it mutates the persistent batch / block tables while the previous step's postprocess_mamba_align_gpu is still in flight reading them — a write-after-read on the state buffers, upstream of the L326 gather you already flagged.

The fix we shipped, and what it buys

Two lines: hoist the wait to the top of synchronize_input_prep, before _update_states:

    @contextmanager
    def synchronize_input_prep(self):
        if self.prepare_inputs_event is None:
            yield
            return
        self.prepare_inputs_event.synchronize()
        # order this step's _update_states/_prepare_inputs after the previous
        # step's spec-decode postprocess, which records this event
        if self.num_accepted_tokens_event is not None:
            self.num_accepted_tokens_event.synchronize()
        try:
            yield
        finally:
            self.prepare_inputs_event.record()

This is your "wait unconditionally" shape, with the extra point that in align the wait already exists — it's the ordering relative to _update_states that's wrong, not the presence of the wait. It relocates a synchronize align already performed a few lines later, so it's TPS-neutral by construction (no net GPU stall), and it's a no-op without spec decode (event is None) and without async scheduling (the method early-returns).

Result on the rig you can't reproduce on (2×3090, TP=2, Ampere, v0.27.1, MTP n=4, fp8 KV, APC, async on): 3/3 boots clean to 33–36k generated tokens each, straight through the 6.4–13k window where we died 5/5 unpatched, across the ctx-32k crossing and a compaction; bench 79.1 narr / 108.5 code / 1756 prefill tok/s, i.e. ≥ baseline. We've shipped it as a downstream install-script overlay on our stack.

Honest limits

We have not isolated TP=2 as a necessary co-factor — the early-sync fix closes the window regardless of why the window is wide enough to fire, so "align-late-sync race" and "TP=2 timing widens it" are not separated by our data. Your arm B says the skip branch alone doesn't fault on TP=1/Blackwell; our result says the align branch does fault on TP=2/Ampere and the early hoist fixes it. Both can be true. Update — we tried the TP=1 arm to isolate the collective, and it's memory-blocked on our hardware. The 18.2 GiB AutoRound-INT4 weights plus fixed cudagraph/GDN-state scratch don't fit one 24 GB Ampere card: OOM at an identical 2.37 GiB shortfall across --max-model-len 132K / 100K / 75K, and unchanged with --max-num-batched-tokens cut 8192→2048 — so it's neither context- nor profiling-batch-driven, it's a fixed allocation. The only lever that fits is --enforce-eager, which removes the CUDA-graph stream structure and therefore suppresses the race (same as CUDA_LAUNCH_BLOCKING=1), so any TP=1 boot we can produce gives an uninterpretable no-crash. We can't generate a clean TP=1-with-cudagraphs point here, so your arm B (TP=1 + cudagraphs + Blackwell, no fault) stands as the reference TP=1 data point — which reads as evidence that TP=2 (or Ampere) is the co-factor rather than the align late-sync alone. Still glad to test any candidate patch of this shape you'd prefer for the PR.

On #52873

Yes please to the Blackwell boundary sweep — a second-silicon data point is exactly what that one needs. Note the boundary is not a fixed position: three async-on boots here collapsed at ctx ~21,025 while an async-off boot collapsed at ~32,570, reproducible within a config but shifting with it (details on #52873). So a sweep that brackets a fixed 2^15 may miss it; varying the config (async on/off, TP, depth) and watching where the per-window acceptance flips to 0 is the more informative shape.

Copy link
Copy Markdown

Follow-up stress validation with the accepted-token bounds applied

Following my earlier red/green confirmation, I kept the relevant #50021 hunks in the final MTP-3 integration stack and ran a materially larger async/concurrent workload. This is additional soak evidence, not a new isolated A/B.

Environment: RTX 5090, Qwen3.8-27B NVFP4, NVFP4 KV, GDN/MTP-3, async scheduling, up to eight sequences.

  • Two 900-request structured hammers: zero HTTP/FSM/grammar/server errors; approximately 84% MTP acceptance
  • Raw c8 decode: 744.1 tok/s
  • Forced eight-active replay: 857.4 tok/s, 71.9% acceptance, zero errors
  • Real parent + seven-child soak: 58 requests, 1.46M prompt tokens, 33.2K generated tokens, 71.1% acceptance
  • KV peaked at 81%; no accepted-token state OOB, CUDA fault, corruption signature, or preemption was observed

The longer soak adds coverage for mixed prompt lengths, async structured output, prefix caching, and divergent agent histories on top of the original targeted red/green result.

Disclosure: AI-assisted analysis and comment posting; the runs and measurements were produced and verified by me on the hardware described.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working k3 kimi nvidia v1

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

[Bug]: Accuracy drops ~20% when --enable-prefix-caching is used together with MTP speculative decoding (Qwen3.6 35B-A3B)

4 participants