Skip to content

[PD] Fix disagg accuracy regression: default early-send cached-prefix off - #31239

Closed
Lzy17 wants to merge 1 commit into
sgl-project:mainfrom
Lzy17:fix/disagg-early-send-default-off
Closed

Lzy17 wants to merge 1 commit into
sgl-project:mainfrom
Lzy17:fix/disagg-early-send-default-off

Conversation

@Lzy17

@Lzy17 Lzy17 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Problem

SGLANG_DISAGG_PREFILL_EARLY_SEND_CACHED_PREFIX (introduced in #29316, commit dbe9e3b7) defaults to on. But maybe_send_cached_prefix_chunk only runs on the non-staging KV-transfer path — the guard returns early when enable_staging is set:

if (not envs.SGLANG_DISAGG_PREFILL_EARLY_SEND_CACHED_PREFIX.get()
    or self.enable_staging or req.pending_bootstrap):
    return

Since SGLANG_DISAGG_STAGING_BUFFER defaults to False, the non-staging path is the default path. On it, the early KV send races the ongoing prefill forward, so decode reads torn/partial prefix KV and accuracy drops. The optimization was only validated on the mooncake staging path (which snapshots into a staging buffer and is excluded by the guard) — it was never safe on the path it actually executes on by default.

Repro / bisect

  • Kimi-K2.6 fp8 1P1D, MI355X, GSM8K 1319Q 8-shot: 0.94 → ~0.88 (below the 0.92 gate).
  • git bisect (source overlay on a fixed image, isolating python source from binaries) pinpoints first-bad = dbe9e3b7. Parent e6efe100 = 0.946; dbe9e3b7 = 0.882. Clean separation, not variance.
  • Runtime instrumentation confirms maybe_send_cached_prefix_chunk fires on ~every request: --chunked-prefill-size splits each prompt across scheduler steps, and each steps completed pages become the reqs own self-prefix (chunk cache, present even without radix cache), which is then early-sent.

Fix

Flip the default to False, making the optimization opt-in. This restores GSM8K to 0.944 on the same image/binaries. It reverts the default to the well-validated pre-#29316 behavior; users who validated the optimization on the staging path can still enable it explicitly.

Notes

cc @cctry (author of #29316)


CI States

Latest PR Test (Base): ❌ Run #29378148931
Latest PR Test (Extra): ❌ Run #29378148760

…prefix off

SGLANG_DISAGG_PREFILL_EARLY_SEND_CACHED_PREFIX (added in sgl-project#29316) defaults on,
but maybe_send_cached_prefix_chunk only runs on the non-staging KV-transfer
path (the guard returns early when enable_staging is set). On that path the
early KV send races the ongoing prefill forward, so decode reads torn/partial
prefix KV and accuracy drops.

Repro: Kimi-K2.6 fp8 1P1D (mori backend, non-staging) on MI355X, GSM8K 1319Q
8-shot fell from 0.94 to ~0.88 (below the 0.92 gate). The optimization was
validated on the mooncake staging path, which snapshots into a staging buffer
and is excluded by the guard; it was never safe on the non-staging path it
actually executes on.

Make the optimization opt-in (default False). Bisected to dbe9e3b; flipping
the default restores GSM8K to 0.944 on the same image/binaries.
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@cctry

cctry commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

The optimization was only validated on the mooncake staging path

This is wrong. as shown in the condition, staging case is not supported

The claimed race condition does not make sense to me. All other disaggregation tests passed normally these days with this on. looks like an AMD-specific RDMA bug

@Lzy17

Lzy17 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Scope-mapping experiments (MI355X, 1P1D, BAD image ...20260626, GSM8K 1319Q 8-shot)

Two orthogonal axes tested to confirm the fix does not regress other models/backends.

Model axis (mori backend, identical 1P1D config)

Model early-send ON early-send OFF delta
Kimi-K2.6 fp8 (plain MLA) 0.888 (fail, gate 0.92) 0.944 −0.056
DeepSeek-V4-Flash fp8 (DSA) 0.926 0.926 0.000

Toggle verified applied (OFF run has SGLANG_DISAGG_PREFILL_EARLY_SEND_CACHED_PREFIX=false in the generated launch script; ON run uses the default). So on the same backend, config, and image, early-send corrupts Kimi but has zero effect on DeepSeek-V4.

Implication for this fix: flipping the default to off is neutral for DeepSeek-V4 (0.926 either way) while fixing Kimi (0.888 → 0.944). It does not regress the other MLA model we can test.

Hypothesis (not proven) for why DSA is immune: send_kv_chunks last_chunk path re-sends full-sequence page indices for DSA state (_dsa_payload), re-syncing any torn early-sent pages; plain-MLA has no such full resync, so the early partial KV persists into decode corrupted.

Backend axis

mooncake does not run cleanly on this MI355X fabric regardless of early-send (both ON and OFF fail to produce an accuracy number — dmabuf/transport issues unrelated to this change), and nixl cross-node is unvalidated here, so the backend axis can only be reasoned about from code: maybe_send_cached_prefix_chunk → send_kv_chunk → disagg_kv_sender.send is backend-agnostic, and flipping the default reverts to the pre-#29316 behavior, which is safe for untested backends.

Note on the deeper fix

This PR makes the optimization opt-in (safe, minimal, unblocks the nightly). The underlying race — early RDMA send of prefix pages overlapping the ongoing prefill forward on the non-staging path — is best fixed in the feature itself; @cctry, happy to help validate a corrected version on the Kimi 1P1D repro.

@Lzy17
Lzy17 marked this pull request as ready for review July 15, 2026 00:42
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@cctry cctry left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please fix forward

@Lzy17

Lzy17 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @cctry for the review and for the push to fix forward instead of disabling the optimization — your AMD-RDMA hint was right. I traced it to the mori transfer path posting the RDMA read directly from the KV pool with no forward-completion sync, while nixl/mooncake gather into a staging buffer and synchronize before transferring, so they never race the in-flight prefill forward. I also want to retract my earlier "only validated on the staging path" wording — that was wrong; the guard means staging is excluded, not validated.

Opened a proper fix that keeps early-send enabled and adds the missing forward sync on the mori worker thread: #31368 (Kimi-K2.6 GSM8K 0.888 -> 0.945/0.942, no perf regression). Closing this one in favor of that.

@Lzy17 Lzy17 closed this Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants