Repository navigation
fix(disagg): mori dropped decode-side abort notifications - #35965
Closed
ShangmingCai wants to merge 1 commit into
Closed
ShangmingCai wants to merge 1 commit into
ShangmingCai wants to merge 1 commit into
Conversation
Every backend inherits `CommonKVReceiver.abort()`, which calls
`_send_abort_notification()` and sends an unguarded 4-frame message:
[b"ABORT", room, decode_ip, decode_port]
The three backends read that socket differently:
- nixl checks WATERMARK / STAGING_RSP / _handle_abort_notification *before*
its `assert msg[0] == GUARD`, so ABORT is handled.
- mooncake has no guard and compares the decoded frame, `room == "ABORT"`.
- mori validated the MORI_GUARD frame *first*, so every ABORT hit
`_validate_message`, logged "Received malformed bootstrap message" and was
dropped.
So on mori the prefill side never learned that decode had aborted: the room
stayed in `request_status` until the bootstrap/waiting timeout fired instead
of being released promptly, and each abort produced a log line blaming
malformed or foreign traffic for sglang's own message.
Fixes it the way nixl already does -- intercept the notification ahead of the
guard check -- rather than adding MORI_GUARD to the sender. Changing the wire
format would break rolling upgrades in both directions: an old prefill would
reject a newly guarded abort, and a new prefill would reject an unguarded one
from an old decode.
`_handle_abort_notification` marks the room Failed only when it is known and
not already Success, taking `transfer_lock` for the read-and-flip because
that is how mori guards `request_status` elsewhere. `record_failure` is
called outside that lock so the two locks never nest. Unknown rooms cannot
be resurrected: `update_status` ignores a Failed transition for a room that
is absent.
Deliberately not implemented: the deferred-decode-KV-release ack. nixl's
version depends on `_staging_outstanding` and `_maybe_ack_drained_abort`,
neither of which mori has, and acking before a mori transfer has drained
could free decode pages while a write is still in flight. With this change
decode falls back to its release timeout, which is the documented behavior
when no ack arrives. A mori owner with hardware should add the ack path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ShangmingCai
requested review from
Duyi-Wang,
HaiShaw,
billishyahao and
kkHuang-amd
as code owners
August 22, 2026 08:02
Collaborator
Author
|
/rerun-test test/registered/amd/disaggregation/test_mori_transfer_engine_e2e.py |
Contributor
|
Results for ⛔ |
Collaborator
|
Hi @ShangmingCai Shangming, Thanks for good catch! Actually there is another similar fix which is in review queue inside amd #29133 . cc previous reviewer and author @HaiShaw @Duyi-Wang @maning00 for their input |
Collaborator
Author
|
Included in #29133 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Mori's prefill side has been silently dropping every decode-side abort notification.
All backends inherit
CommonKVReceiver.abort(), which calls_send_abort_notification()and sends an unguarded 4-frame message:The three backends read that socket differently:
GUARD = b"NixlMsgGuard"WATERMARK/STAGING_RSP/_handle_abort_notificationbefore itsassert msg[0] == GUARD— handledroom == "ABORT"— handledMORI_GUARD = b"MoriMsgGuard"_validate_messagechecks the guard first, so ABORT is rejected — droppedMori's own sends put
MORI_GUARDin frame 0 (lines 657, 1686, 1745), but the inherited abort path never does. So on mori every abort producedand was discarded. Two consequences:
request_statusuntil the bootstrap/waiting timeout fired instead of being released promptly.Nixl's ordering shows this was a known hazard — its abort check is deliberately placed above its guard assert. Mori never got the same treatment.
Modifications
1 file, +47 / -0. Adds
MoriKVManager._handle_abort_notificationand calls it in_start_bootstrap_threadahead of_validate_message.Why the receive side rather than adding MORI_GUARD to the sender
Adding the guard to
_send_abort_notificationwould break rolling upgrades in both directions: an old prefill would reject a newly guarded abort, and a new prefill would reject an unguarded one from an old decode. Intercepting on the receive side keeps the wire format frozen and matches what nixl already does.Correctness details
check_status()indexesrequest_statusdirectly and raisesKeyErrorfor an unknown room, so the active check short-circuits on membership first — same as nixl.request_statuswithtransfer_lockat four other sites, so the read-and-flip takes that lock.record_failureis called outside it, sotransfer_lockandfailure_locknever nest.update_statusdeliberately ignores aFailedtransition for an absent room, so a late abort cannot pollute a future request that reuses the samebootstrap_room.Deliberately not implemented: the deferred-KV-release ack
Nixl's handler also registers a deferred ack target and may send
ABORT_ACK. That path depends on_staging_outstandingand_maybe_ack_drained_abort; mori has neither, and no staging at all. Acking before a mori transfer has drained could free decode pages while a write is still in flight — exactly the hazard nixl's own comment warns about.With this change, decode falls back to its release timeout, which is the documented behavior when no ack arrives. That is strictly better than today (the abort is dropped entirely), but a mori owner with hardware should add the ack path as a follow-up.
Accuracy Tests
Not applicable — no model output, kernel, or forward path is touched. This is a control-plane message handler.
Speed Tests and Profiling
Not applicable. One byte-compare per bootstrap message on an already-ZMQ-bound thread.
Verification performed
The handler's logic was exercised against a stand-in manager reproducing mori's
transfer_lockandCommonKVManager.update_status's exact semantics — 16 assertions, all passing:Failed, reason recordedSuccessroomrequest_statusstays empty (no resurrection)Pinned
ruff 0.15.1 --select=F401,F821,UP037(the pre-commit config) passes, plus pinnedblack 26.1.0andisort 7.0.0.What CI will and will not cover here
test/registered/amd/disaggregation/test_mori_transfer_engine_e2e.pylaunches real prefill+decode servers on 8xMI35x. It has no abort coverage, so it will not validate the fix — but it is meaningful regression cover for the risky part of this change: every transfer-info message now passes through_handle_abort_notificationbefore_validate_message, so a mistake in dispatch ordering or return value would break bothtest_generate_smokeandtest_generate_smoke_tp_mismatch.No mori hardware was available to me, so the end-to-end abort behavior is unverified. Please run the AMD disaggregation stage before merging.
Why there is no unit test in this PR
I wanted to add
test/registered/unit/disaggregation/test_mori_abort_notification.pymodelled on the existingtest_nixl_deferred_kv_release.py, which unit-tests_handle_abort_notificationon CPU CI viaNixlKVManager.__new__(cls).That is not currently possible for mori.
nixl/conn.pyimportsnixl._apilazily inside a method, so the module imports fine without NIXL installed.mori/conn.pyimports eagerly at module level:so
import sglang.srt.disaggregation.mori.connfails without the mori package. That is very likely why mori has 0 unit test files today while nixl has 3 — and why this bug survived: the sharedabort()reaches all three backends, nixl proved its handler with unit tests, mooncake happens to parse it, and mori was never checked.A CPU unit test would need
patch.dictstubs for all ten names. Making mori's imports lazy (as nixl does) would be the cleaner fix and would unlock unit testing for the whole backend. Happy to do either as a follow-up — say which you prefer.Checklist
🤖 Generated with Claude Code
CI States
Latest PR Test (Base): ❌ Run #32561230455
Latest PR Test (Extra): ❌ Run #32561230376
Latest PR Test (AMD ROCm 7.2): ❌ Run #32561230429