Repository navigation
[None][fix] Fail stop executor worlds on unproven KV retirement - #19674
Conversation
109cf30 to
ea584bf
Compare
ea584bf to
904ce91
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #75782 [ run ] triggered by Bot. Commit: |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/TensorRT-LLM/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review. WalkthroughPhysical-ownership transfer sessions now track eligible and published writers separately. Cancellation can trigger sender session-quiescence acknowledgments that contribute to receive retirement evidence. The transceiver qualifies MPI fail-stop handling for unresolved retirement and changes shutdown ordering. ChangesTransfer quiescence and retirement
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant RxSession
participant Receiver
participant SenderWorker
RxSession->>Receiver: Notify cancellation to known sender endpoints
Receiver->>SenderWorker: Send cancellation with request ID and receiver rank
SenderWorker->>Receiver: Send SESSION_QUIESCED after pending work drains
Receiver->>RxSession: Record acknowledgment for a published writer
Suggested reviewers: Merge Risk: 🟡 Moderate · up to Sender memory may grow with every peer cancellation in long-running servers that use this opt-in profile. This should be bounded or explicitly accepted before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
nv-xtf
left a comment
nv-xtf
left a comment
There was a problem hiding this comment.
Thanks for clarifying the tradeoff and adding the boundary tests. LGTM
|
Thanks for the effort! |
|
PR_Github #75782 [ run ] completed with state
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/bot run --disable-fail-fast |
|
PR_Github #75872 [ run ] triggered by Bot. Commit: |
|
PR_Github #75872 [ run ] completed with state
|
This comment has been minimized.
This comment has been minimized.
|
PR_Github #75935 [ run ] triggered by Bot. Commit: |
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #75941 [ run ] triggered by Bot. Commit: |
|
PR_Github #75935 [ run ] completed with state |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
PR_Github #75941 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #75978 [ run ] triggered by Bot. Commit: |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Semantic conflict reviewThe verdict of record is the Latest recorded state: No semantic conflict found (best effort) for head Best-effort AI judgment for the recorded revisions. PASS, FAIL and INCONCLUSIVE may be incomplete or incorrect. PR authors and reviewers should independently verify the evidence and relevant behavior. This semantic review and its status/workflow are advisory, not required merge checks under current repository rules; other merge requirements still apply. Advisory status does not make a confirmed defect safe to ignore.
Processed request and reply comments are minimized to reduce timeline noise; they remain expandable for audit. |
|
PR_Github #75978 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #76011 [ run ] triggered by Bot. Commit: |
|
PR_Github #76011 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #76049 [ run ] triggered by Bot. Commit: |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
PR_Github #76049 [ run ] completed with state |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
allisonlim-nv
left a comment
allisonlim-nv
left a comment
There was a problem hiding this comment.
unblocking from runtime, mainly a disagg review
Summary
Fail-stop when exposed KV-transfer resources remain unproven after their fixed quiescence deadline. Retain KV and transport resources until termination; timeout never authorizes ordinary cleanup or memory reuse.
Builds on merged #19378 and #19673. Rebased onto main
0d3bbd257d; current head0efe12dab0; only this PR’s containment and acknowledgment changes remain.Scope
SESSION_QUIESCEDacknowledgments for cancelled CTX requests, including requests without a send session or with expired metadata. Cancellation permanently fences the immutable request ID; existing KV/AUX accesses must drain before acknowledgment. ACKs follow earlier results on the same worker/socket. Missing-result retirement requires ACKs from every published candidate, without inventing data-transfer success or bypassing CUDA,IN_DOUBT, or fatal-expiry guards.MPI_THREAD_MULTIPLEcommunicator matching executor rank/size, retain unresolved resources, and connect fatal expiry to termination. Fatal logging gets at most a 0.5-second join before unconditional kill dispatch, even if logging blocks or thread setup fails.TRTLLM_ENABLE_FP4_MLA_KV_OWNERSHIP_BRIDGE=1andTRTLLM_DISAGG_NO_RETRY=1on every participant. Mixed-version peers and non-MPI launchers are not qualified.Availability tradeoff: a persistent NIXL error retains sender roots and can cause connected CTX workers to fail-stop, interrupting unrelated requests. MPI abort can terminate the entire MPI job, including other executors in split communicators. Backend fencing or finer-grained quarantine could reduce this impact in a separate change.
Cancellation-fence cost: ownership mode intentionally retains one request-ID fence for the Sender’s lifetime per distinct cancelled request on each addressed CTX rank. Budget roughly 0.1 KB of host metadata per ID per rank (about 100 MB per million IDs, allocator-dependent; not a hard upper bound), not KV/GPU storage. ADP broadcasts multiply this across ranks. Metadata TTL and transfer timeout do not bound late session admission, so TTL/LRU eviction would be unsafe; bounded reclamation needs a separate admission-expiry or epoch contract.
Safety boundary: process termination alone does not prove GPU/RDMA access revocation. Exact-profile multi-rank NIXL behavior, surviving-peer handling, platform fencing, and safe replacement remain qualification gates; automatic replacement safety is not claimed.
Verification
904ce917b7reproduced four behavioral assertion failures for missing no-session/expired-metadata acknowledgment and reopened admission; the active-write safety guard passed. These historical motivators are a subset of the expanded suite.0efe12dab0: full CI #75941 / pipeline #62616 recorded 206 passes on each of CPU x86 and ARM: 32 fail-stop, 29 session-acknowledgment, 81 retirement-deadline, and 64 ownership-regression cases. This includes real two-rank termination/fresh-world and four-rank split-executor MPI tests. The existingunittest/disaggregatedCPU test-list entry collects these modules./bot run --disable-fail-fast. CPU/MPI coverage does not establish GPU/RDMA revocation or safe automatic replacement.Dev Engineer Review
SESSION_QUIESCEDacknowledgments provide evidence that cancelled sender sessions have drained before the receiver retires exposed KV resources. Physical-ownership mode tracks eligible and published writers, handles partial publication, and retains resources when access remains unproven. Non-ownership transfer paths retain their existing behavior.The transceiver enables fail-stop only for the ownership bridge profile. Startup requires initialized, unfinalized MPI with
MPI_THREAD_MULTIPLEand a matching intracommunicator. The fatal path bounds best-effort diagnostics and requests hard termination without ordinary cleanup.propagate_hard_killadds optional diagnostics and communicator arguments.Receiver.send_cancel_to_sendersnow returns a set, andTxSession.cancel_localadds a keyword-only argument; callers and mocks must account for these API changes.The profile remains limited to the stated Python/NIXL generation-first ADP/NVFP4 no-bounce configuration. The supplied objectives warn that MPI abort can affect the whole MPI job and that connected CTX workers may be interrupted. Production GPU/RDMA access revocation and replacement safety are not established. The reported test run passed on a pre-rebase snapshot; current-head test results and production qualification remain pending.
QA Engineer Review
Five test files changed. They cover cancellation acknowledgment ordering, physical-access retirement, deadline handling, MPI qualification and termination, retained resources after failed or unresolved operations, and proxy behavior after worker death. Named tests include
test_session_ack_waits_for_admitted_and_submitting_pieces,test_unproven_fatal_bounds_log_join_and_always_dispatches_kill, andtest_retirement_rejects_unqualified_executor_world. The bounded-polling test file also contains a skipped idle context-consensus fast-path test.No test-list files changed. Of the five changed test files, only
tests/unittest/disaggregated/test_transceiver_bounded_polling.pyappears in an integration CI list (test-db/l0_h100.yml). The other four files do not appear in the inspected integration lists. Coverage verdict: needs follow-up. The author reports 424 passed, one pre-existing skip, and no failures or errors on the tested snapshot. Current-head test results remain pending.Per-File QA Perspective
tensorrt_llm/_torch/disaggregation/native/transfer.py: Cancellation can now produce session-quiescence evidence, and acknowledgments wait for pending results and settlements. Verify acknowledgment ordering, partial publication, and unchanged behavior outside physical-ownership mode.tensorrt_llm/_torch/disaggregation/transceiver.py: The ownership bridge validates and captures an MPI communicator, enables fatal expiry handling, and drains before shutdown. Verify profile gating, communicator checks, shutdown behavior, and resource retention when retirement remains unproven.tensorrt_llm/_torch/pyexecutor/hang_detector.py:propagate_hard_killcan skip diagnostics and use a supplied communicator. Verify both diagnostic modes and MPI behavior when finalized or when no communicator is supplied.tests/unittest/disaggregated/test_kv_transfer_fail_stop.py: Tests qualification, bounded fatal logging, retention, shutdown, proxy behavior, and MPI termination scenarios. It is not listed in the inspected integration CI or manual-QA lists.tests/unittest/disaggregated/test_retirement_deadline.py: Tests deadline boundaries, admission closure, unresolved-owner retention, and blocked backend behavior. It is not listed in the inspected integration CI or manual-QA lists.tests/unittest/disaggregated/test_transceiver_bounded_polling.py: Adds ownership-bridge fail-stop gating coverage and exercises bounded polling and session completion. It is listed intests/integration/test_lists/test-db/l0_h100.yml; no manual-QA listing was found.tests/unittest/disaggregated/test_transfer_ownership_regressions.py: Updates cancellation fixtures and tests ownership retention, retirement evidence, shutdown, and pre-cancelled sender behavior. It is not listed in the inspected integration CI or manual-QA lists.tests/unittest/disaggregated/test_unsubmitted_session.py: Tests pre-session and existing-session cancellation acknowledgments, settlement ordering, unsettled physical access, stale evidence, and deadline expiry. It is not listed in the inspected integration CI or manual-QA lists.