Skip to content

Fix disaggregated prefill cache leak after abort - #36926

Closed
kflansburg wants to merge 2 commits into
sgl-project:mainfrom
kflansburg:kflansburg/fix-disagg-prefill-abort-cache-leak
Closed

kflansburg wants to merge 2 commits into
sgl-project:mainfrom
kflansburg:kflansburg/fix-disagg-prefill-abort-cache-leak

Conversation

@kflansburg

@kflansburg kflansburg commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

An aborted disaggregated prefill request can lose scheduler ownership when an overlapped middle or final chunk result arrives after AbortReq or bootstrap failure. The result path currently drops aborted middle chunks and does not check final chunks before caching and enqueueing them.

For hybrid Mamba models this leaves one request allocation unaccounted for: one full prefill chunk plus the request Mamba state and overlap buffers. The observed production invariant failure was a 16,384-token full-KV deficit and three missing Mamba states.

Modifications

  • Treat abort as terminal before optimistic-bootstrap retry.
  • Retire aborted final results and abandoned middle results through one idempotent cleanup helper.
  • Release metadata, HiCache request state, KV pages, and Mamba state without inserting the aborted request into radix cache.
  • Defer cleanup while a later overlapped chunk is still running.
  • Ignore delayed results when bootstrap-failure cleanup already released the request.
  • Stream the terminal abort exactly once from the result path that owns cleanup.

Accuracy Tests

No model-output behavior changes for successful requests.

  • 9 focused CPU tests passed, including delayed middle result, delayed final result, already-cleaned bootstrap failure, and unified-memory move-gate coverage.
  • All file-scoped pre-commit hooks passed.

GPU reproduction on the original GLM-5.3-Flash B300 deployment is still pending.

Speed Tests and Profiling

Not applicable. The new checks only run for completed disaggregated-prefill results; no model-forward path changes.

Checklist

  • Format code with pre-commit.
  • Add unit tests.
  • Update documentation (not needed for this lifecycle bug fix).
  • Provide accuracy and speed benchmarks (not applicable; production GPU reproduction remains pending).
  • Follow the SGLang code style guidance.

CI States

Latest PR Test (Base): ❌ Run #33272687086
Latest PR Test (Extra): ❌ Run #33272686834
Latest PR Test (AMD ROCm 7.2): ⏳ Run #33272687029

if not owns_resources:
return False

req.disagg_kv_sender.abort()

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.

Normal abort cleanup now works, but abort() can still raise before metadata, HiCache, KV, and Mamba release. It may be worth making transport notification best effort so every local cleanup step still completes.

@rwang5203

Copy link
Copy Markdown
Collaborator

The cleanup closes the reproduced ownership gap, but it and PR 36724 both modify the disaggregated prefill result loop. Combining them currently conflicts around terminal row collection and streaming. Try using a joint regression during conflict resolution, which can cover embedding failures and ordinary aborts while proving each terminal request streams exactly once.

@mickqian

Copy link
Copy Markdown
Collaborator

After #36958, req.kv is non-optional, so both req.kv is not None checks are now always true. Please rebase and key KV ownership on req.is_holding_kv (plus the separate Mamba/metadata ownership), while keeping transport abort best-effort so it cannot bypass local cleanup.

@kflansburg
kflansburg force-pushed the kflansburg/fix-disagg-prefill-abort-cache-leak branch from 65b1f67 to 9cb30a4 Compare August 29, 2026 20:08
@kflansburg

Copy link
Copy Markdown
Contributor Author

Rebased onto current main and addressed the #36958 follow-ups: KV ownership now uses req.is_holding_kv, and sender abort is best-effort so local metadata, HiCache, KV, and Mamba cleanup still completes. The joint exact-once terminal-streaming regression with #36724 is deferred until this lands and #36724 is rebased.

@mickqian
mickqian marked this pull request as ready for review September 6, 2026 08:10
@mickqian

mickqian commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

/tag-and-rerun-ci

@github-actions github-actions Bot added the run-ci CI: run the baseline test suite on this PR label Sep 6, 2026
@mickqian

mickqian commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

covered in #36988, thanks

@mickqian mickqian closed this Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-ci CI: run the baseline test suite on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants