Skip to content

[disagg][mooncake] Keep decode status thread alive on bad messages - #32909

Closed
guptaishaan wants to merge 1 commit into
sgl-project:mainfrom
guptaishaan:fix-32652
Closed

guptaishaan wants to merge 1 commit into
sgl-project:mainfrom
guptaishaan:fix-32652

Conversation

@guptaishaan

@guptaishaan guptaishaan commented Jul 30, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #32652

The decode KV manager binds an unauthenticated zmq.PULL socket, and decode_thread
parses whatever arrives on it with no exception guard. A frame that does not match the
status wire format (for example a single-frame message, which hits bootstrap_room, status, prefill_rank = msg) raises out of the thread and kills it. The process stays up
and /health keeps returning 200, so the router keeps routing to a decode worker whose
control path is dead, and every later prefill status message is dropped, leaving requests
in KVPoll.WaitingForInput until they time out.

Wrap the per-message body of decode_thread in try/except Exception and log with
logger.exception. This is the same shape the mori backend already uses for its decode
status worker. recv_multipart() stays outside the guard on purpose: a dead socket should
still end the thread rather than spin.

Verified on CPU, no GPU involved. Added
test/registered/unit/disaggregation/test_mooncake_decode_thread.py: it binds a real
zmq.PULL socket, runs the real start_decode_thread against it, pushes one malformed
frame followed by a valid Success status for a pending room, and asserts the room reaches
Success. It fails before this change (room stays in WaitingForInput, thread dead) and
passes after. The rest of test/registered/unit/disaggregation/ still passes, and black,
isort and ruff are clean on both files.

Not verified: the full 1P1D Mooncake plus router end-to-end scenario from the report. I do
not have that setup, so the repro is at the thread and socket level. The reporter's exact
trigger is under private disclosure, but every parse in the decode status loop is covered
by the guard. Health checks themselves are unchanged: they still cannot see a dead
receiver thread. The prefill-side bootstrap_thread in the same file has the same
unguarded shape and is left for a follow-up.

Reported by @yyymk, who also filed it privately as GHSA-98f6-8g99-45hr.


CI States

Latest PR Test (Base): ❌ Run #31222996845
Latest PR Test (Extra): ❌ Run #31222998834

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@yyymk

yyymk commented Aug 3, 2026

Copy link
Copy Markdown

Thanks for working on this issue.

The public issue only includes limited details. I removed some technical information to avoid public disclosure. Because of this, the current fix may not fully address the root cause.

I have a private Gist with the full report and PoC. I can share it with you privately if that would help.

I am also happy to review or test the fix with you.

@guptaishaan

Copy link
Copy Markdown
Contributor Author

Thanks, yes, please share it. Since it is still under private disclosure, the GHSA advisory
(GHSA-98f6-8g99-45hr) is probably the better channel than a Gist link in this thread, so nothing
lands in public. Adding me there works, or send the Gist privately if you prefer that.

To save you time, here is exactly what this PR does and does not cover, so you can check it
against your PoC:

  • It only makes decode_thread survive a failure while handling one message. The thread stays
    alive and logs instead of dying.
  • It does not authenticate or restrict the zmq.PULL socket. Any peer that can reach it can
    still send to it.
  • It does not validate message contents. A well formed message with a forged bootstrap_room
    is still accepted and acted on.
  • It does not touch the prefill side bootstrap_thread in the same file, which has the same
    unguarded shape.

So if the redacted part of the report is about who can reach the socket, or about what a
well formed but forged message can do, this patch is not the fix and I would rather rework it
than layer on top of it. If it is about a malformed frame killing the receiver, it should cover
it.

I will hold off on further changes until I have read the report. And yes, testing it against your
PoC would be very helpful.

@yyymk

yyymk commented Aug 5, 2026

Copy link
Copy Markdown

Thanks. I have added you to GHSA-98f6-8g99-45hr as a remediation developer. The full report and PoC are available there.

This PR appears to address the specific thread termination issue in my report.

@ShangmingCai @ByronHsu , could you please review this PR and help move it forward?

The decode KV manager binds an unauthenticated zmq.PULL socket and
decode_thread parses whatever arrives on it with no exception guard. A
frame that does not match the status wire format, for example a single
frame message hitting the `bootstrap_room, status, prefill_rank = msg`
unpack, raises out of the thread and kills it. The process stays up and
/health keeps returning 200, so the router keeps sending work to a
decode worker whose control path is gone, and later prefill status
messages are never processed, leaving requests in WaitingForInput until
they time out.

Wrap the per-message body in try/except Exception and log with
logger.exception, the same shape the mori backend uses for its decode
status worker. recv_multipart() stays outside the guard so a dead socket
still ends the thread instead of spinning.

Add a CPU unit test that binds a real zmq.PULL socket, runs the real
start_decode_thread, pushes one malformed frame followed by a valid
Success status, and asserts the room still reaches Success.
@guptaishaan

Copy link
Copy Markdown
Contributor Author

Thanks for confirming, and for adding me to the advisory.

The branch had fallen out of merge with main, so I rebased it onto current main
(8a22b83). The only conflict was inside decode_thread itself: #30545 dropped the
_chunk_writer_counts bookkeeping from the CHUNK_READY and Success paths. I took
main's version of that code and re-wrapped it, so the change is still only the
try/except Exception around the per-message body plus the unit test. Nothing about
the fix itself changed from the version you looked at.

Re-verified on the rebased branch: black, isort and ruff are clean, the new test still
fails without the guard and passes with it, and test/registered/unit/disaggregation/
passes (130 tests).

The items I listed earlier as out of scope are still out of scope here: authenticating
or restricting the zmq.PULL socket, validating a well formed but forged
bootstrap_room, and the prefill side bootstrap_thread which has the same unguarded
shape. If the advisory calls for any of those, I would rather send them as separate PRs
than grow this one.

@ShangmingCai @ByronHsu one note for whoever picks this up: CI here is red only because
the run-ci label is missing, every failing job is the pr-gate check. It needs the
label to actually run.

@github-actions

Copy link
Copy Markdown
Contributor

Thanks @guptaishaan. Closing this because you have 7 PRs open and none updated in 7 days, over our soft cap of 5 for idle PRs.

Reopen the two or three you most want reviewed - a short active set moves faster than a long idle one.

Some directories moved recently, so an older branch may need retargeting:
sgl-kernel/ -> python/sglang/kernels/aot/, python/sglang/jit_kernel/
-> python/sglang/kernels/jit/, docs/ -> docs/docs/ (.mdx),
bench_serving.py -> benchmark/serving.py, test/srt/ -> test/registered/.

@github-actions github-actions Bot closed this Sep 27, 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.

[Bug] [Security tracking][PD disaggregation] Decode control-path failure can remain invisible to health checks

2 participants