Repository navigation
fix(inference): AUT-2539 reply to a KV handoff import that fails before admission - #8255
Open
svcnemo-autobot wants to merge 3 commits into
Open
svcnemo-autobot wants to merge 3 commits into
svcnemo-autobot wants to merge 3 commits into
Conversation
A decode-side KV handoff import that fails (peer failure or local transfer error) before _finalize_kv_handoff_import() never reaches add_request(). The except branch in _admit_pending_kv_imports() released or quarantined its blocks and failed the future, but the engine dispatch drops that future, so no ENGINE_REPLY was sent. The coordinator frees routing and pending counts only on ENGINE_REPLY, so the request hung forever. When the failed import is not in self.requests, register the same FAILED placeholder _fail_submission() uses for rejected submissions. Every MP rank applies the broadcast completion decision to mirrored state, and only the MP coordinator sends the reply. Release-once and quarantine behavior are unchanged. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
svcnvidia-nemo-ci
marked this pull request as draft
October 15, 2026 14:38
Contributor
|
This PR has been automatically converted to draft because all PRs must start as drafts. When you are ready for review, click Ready for Review to begin the review process. This will:
See the contribution guide for more details. |
Contributor
|
/ok to test 49fcf50 |
Bring in main 3914483 so CI's main-based Testmon baseline matches the branch base. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
Contributor
|
/ok to test ba41ebb |
Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
Contributor
|
/ok to test 9dc92d4 |
Contributor
|
/review |
svcnvidia-nemo-ci
left a comment
svcnvidia-nemo-ci
left a comment
Contributor
There was a problem hiding this comment.
LGTM. The pre-admission import failure now reaches the existing immediate failure-reply path. The regression assertions cover exactly one FAILED reply and retain the release/quarantine checks. Static review only; tests were not executed.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex
This branch was successfully deployed
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
3 participants
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.
Background
A review of recent inference changes on
mainfound a hang in disaggregated decode. The decode side imports a request's KV cache before it admits the request. An import can fail before_finalize_kv_handoff_import()runs, either because a peer failed or because the local transfer raised. The request then never reachesadd_request()._admit_pending_kv_imports()releases or quarantines the blocks and fails the import future, but the engine dispatch drops that future. So noENGINE_REPLYis ever sent. The coordinator frees a request's routing entry and pending count only onENGINE_REPLY, so the client request hangs forever.What changed
inference_state_handoff.py: in theexceptbranch, an import that is not inself.requestsnow calls_fail_submission(). This is the same FAILED-placeholder path already used for rejected submissions. Every MP rank applies the same broadcast completion decision to mirrored state, so all ranks register the same placeholder. Only the MP coordinator sends the reply.dynamic_engine.py: the_fail_submissiondocstring now names this caller.test_disagg_handoff_lifecycle.py:_fail_submission,_handle_failed_requestand_complete_request, with only the coordinator socket mocked. It asserts exactly one FAILED reply for the request.Details
Release-once and quarantine behavior are unchanged. A failure after
add_request()still goes through the existing request path. There are no dependency or lock changes.The branch was based on
mainata54dd737and now mergesmainatee6baa2e. CI on the previous head (which mergedmainat3914483f, run 38816895124) timed out in an NCCL barrier intest_checkpointing.py::test_checkpoint_load_without_microbatch_calculator(torch_distcases).ee6baa2eis #8263, "test(checkpointing): seed CPU initialization in checkpoint load test". Againstmainatee6baa2e, this branch still changes only the three files above.Tested
unit:tests/unit_tests/inference/**/*.pyon 1×8 H100 at49fcf50f(before the merges), CI image fora54dd737(sha256:5c82eeb5…): 3674 passed, 37 skipped, 9 deselected, 0 failed, exit code 0. All 37 tests intest_disagg_handoff_lifecycle.pypassed, including the two changed failure tests.tests/unit_tests/**/*.pybucket on 8×H100, on this head merged with themaintip (28d5dd15), imagemegatron-lm@sha256:7b4e22a3…: 3424 passed, 142 skipped, 0 failed; then 24 of 24 passed in the second session; exit code 0. All 16test_checkpoint_load_without_microbatch_calculatorcases passed, including everytorch_distcase.