Skip to content

[PD Disagg] Add bootstrap timeout to NIXL prefill sender - #34688

Closed
ting-hong-shieh wants to merge 1 commit into
sgl-project:mainfrom
ting-hong-shieh:codex/nixl-prefill-bootstrap-timeout
Closed

ting-hong-shieh wants to merge 1 commit into
sgl-project:mainfrom
ting-hong-shieh:codex/nixl-prefill-bootstrap-timeout

Conversation

@ting-hong-shieh

@ting-hong-shieh ting-hong-shieh commented Aug 13, 2026 •

Copy link
Copy Markdown

Motivation

Part of #34510 Step 1. Related to #17845.

CommonKVSender already implements the Prefill bootstrap timeout. Mooncake starts the deadline when its sender is created and checks it while the request is bootstrapping. NIXL does neither, so a request can remain in KVPoll.Bootstrapping indefinitely if Decode destination metadata never arrives.

Changes

  • Record NixlKVSender initialization time.
  • Call _check_bootstrap_timeout() while poll() reports KVPoll.Bootstrapping.
  • Add CPU regression tests for deadline initialization and the timeout failure path.

The existing helper records the failure, changes the room status to KVPoll.Failed, and returns KVPoll.Failed. This patch does not change the common protocol, transport abstraction, or wire format.

Tests

  • python3 -m compileall -q python/sglang/srt/disaggregation/nixl/conn.py test/registered/unit/disaggregation/test_nixl_backend_basic.py
  • isort --check-only python/sglang/srt/disaggregation/nixl/conn.py test/registered/unit/disaggregation/test_nixl_backend_basic.py (isort 7.0.0)
  • black --check python/sglang/srt/disaggregation/nixl/conn.py test/registered/unit/disaggregation/test_nixl_backend_basic.py (Black 26.1.0)
  • ruff check --select=F401,F821,UP037 python/sglang/srt/disaggregation/nixl/conn.py test/registered/unit/disaggregation/test_nixl_backend_basic.py (Ruff 0.15.1)
  • git diff --check HEAD^ HEAD

The focused registered unit-test target was not run locally because this lightweight environment does not include NumPy, PyTorch, pyzmq, or pytest.

Accuracy and performance

Not applicable. This change affects only the Prefill bootstrap control path and does not modify model execution or KV transfer.


CI States

Latest PR Test (Base): ❌ Run #31674506695
Latest PR Test (Extra): ❌ Run #31674506531

@ting-hong-shieh
ting-hong-shieh marked this pull request as ready for review August 13, 2026 06:45

Copy link
Copy Markdown
Author

Hi @ByronHsu @cctry @ShangmingCai, could one of you help coordinate this with #17845 and trigger CI for the current commit?

This implements the #34510 Step 1 NIXL Prefill bootstrap-timeout item using the current NixlKVSender/CommonKVSender structure. It reuses _check_bootstrap_timeout() and adds focused CPU regression tests. #17845 addresses the same gap but is based on older code and has had no updates since January.

The red checks currently shown were created while this PR was a draft; their gate log says PR is draft. Blocking CI., so the test jobs did not run. The PR is now ready for review. Could an authorized maintainer add the run-ci label and rerun CI for commit e39388f?

If continuing #17845 is preferred, I’m happy to close this PR.

Copy link
Copy Markdown
Author

Quick coordination update: #34692 was opened after the comment above and implements the same #34510 Step 1 item.

The production changes overlap: both initialize NixlKVSender.init_time and call _check_bootstrap_timeout() while the sender is in KVPoll.Bootstrapping. This PR also includes focused regression coverage in test_nixl_backend_basic.py; #34692 currently changes conn.py only, with its test kept on a separate fork branch as noted in that PR.

Could the maintainers advise which PR should continue? I’m happy to consolidate the work or close this one if another direction is preferred.

Copy link
Copy Markdown
Author

Closing in favor of #34692, now merged as a82f8e177.

I also compared the test-only rewrite I had prepared with the public rfc-pd-test branch at ca7ad0f50. That test constructs a real NixlKVManager and covers the sender's init_time, the timeout transition to KVPoll.Failed, request_status, and the recorded failure reason. My rewrite covers the same scenario with mocks, so pushing it would duplicate already published work.

No branch update was made after #34692 merged. Thanks to @jambow0320 and @ShangmingCai for carrying the fix through.

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.

1 participant