Repository navigation
[TRTLLM-17105][fix] Report a bounce failure only after the receiver has answered every sent chunk - #21191
[TRTLLM-17105][fix] Report a bounce failure only after the receiver has answered every sent chunk#21191chuangz0 wants to merge 3 commits into
Conversation
|
/bot run --disable-fail-fast |
|
PR_Github #81179 [ run ] triggered by Bot. Commit: |
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Quiescence result and status contract cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceResult.h, cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceResult.cpp, cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceTransferStatus.h, cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/transferAgent.cpp, cpp/include/tensorrt_llm/executor/transferAgent.h |
BounceResult tracks unanswered chunks and active source reads. FailedRequest resolves when chunks are answered or its deadline expires. BounceTransferStatus uses result quiescence to report completion. |
Receiver DATA and NACK ordering cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceMessage.h, cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceTransport.cpp |
The receiver reserves scattering regions before decoding DATA, drops duplicate DATA without an answer, and NACKs malformed or stale DATA. Scatter processing synchronizes the stream before a NACK. The bounce wire version is now 3, with documented chunk-answer guarantees. |
Failed-request answer tracking and resolution cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceTransport.h, cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceTransport.cpp, cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceConfig.h |
The sender retains failed requests while awaiting answers for sent chunks. It matches ACKs and NACKs, applies timeout-based resolution, defers cancellation while writes remain in flight, and resolves requests during shutdown. |
Failure-path validation and test wiring cpp/tests/unit_tests/executor/bounce/bounceFailedRequestTest.cpp, cpp/tests/unit_tests/executor/bounce/bounceTransportFailureTest.cpp, cpp/tests/unit_tests/executor/bounce/bounceAgentE2ETest.cpp, cpp/tests/unit_tests/executor/bounce/bounceTestNixlNode.h, cpp/tests/unit_tests/executor/bounce/CMakeLists.txt, cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/CMakeLists.txt |
Tests cover event ordering, answer matching, timeout and shutdown behavior, DATA/NACK handling, and destination contents. End-to-end tests check successful completion and failure status. Shared agent configuration and test targets are updated. |
Priority:
Estimated code review effort: 4 (Complex) | ~60 minutes
Sequence Diagram(s)
sequenceDiagram
participant BounceSender
participant Receiver
participant FailedRequest
participant BounceTransferStatus
BounceSender->>Receiver: Send chunk DATA
Receiver->>BounceSender: Return ACK or NACK
BounceSender->>FailedRequest: Record matching answer
FailedRequest->>BounceTransferStatus: Resolve failure with outstanding-access result
Suggested reviewers: mikeiovine, tabrizian
Merge Risk: 
Minimal · up to cc80f
-
The change makes bounce failures report completion only after the receiver has answered every sent chunk, or after the answer deadline expires. No concrete merge-blocking defect was found. The commit message and PR description disagree about the kBounceVersion bump, and the description should be corrected. The CI failures were reported without specific details.
Pre-merge checks: 
4 · 
1
-
Failed checks: 1 warning Passed checks: 4 passedFull details: Docstring Coverage
-
Explanation
Docstring coverage is 50.93% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 108 functions across 13 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
The bounce gtests wire agents with loadRemoteAgent(AgentDesc) and never use the NIXL listen thread. With it on, NixlTransferAgent locks /tmp/trtllm_nixl_port.lock to pick a port. On a shared machine that file may belong to another user (root is locked out too under fs.protected_regular=2), so the agent fails to build and every transport test SKIPs with "NIXL agent/backend unavailable", hiding regressions. Build every bounce test agent with useListenThread=false, through one shared bounce_test::makeAgentConfig that makeNode and the agent E2E test both use. Production is unchanged: the legacy C++ transceiver still uses the listen thread, and the Python transceiver already passes use_listen_thread=False. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
…as answered every sent chunk Only the receiver's scatter of a received DATA writes the destination, and the receiver answers (ACK or NACK) after that scatter's last write. A failed request used to resolve kFAILURE at once, even while chunks whose DATA was already sent were queued or running on the receiver, so a caller that reused the destination could have it overwritten. Contract: BounceResult gains `quiesced`. A failed request whose sent chunks are not all answered now waits for those answers: it posts, gathers and consumes credits no more, releases its staging and sends its cancel as before, and resolves FAILURE with quiesced=true once the last answer arrives. Answers match by (peer, rid, chunk, region), and failRequest is idempotent, so a later forgetPeer keeps the first reason and the answer deadline. The receiver drains the exec stream before NACKing a failed launch, so a NACK never precedes a write. In-doubt: with a sent chunk still unanswered, a receiver silent for requestTimeoutMs after the failure or its last answer (the dead-peer assumption the lease and quarantine already make), a kNoProgressTimeout, or shutdown resolves the request with quiesced=false and a WARNING. BounceTransferStatus moves to a header-only file; isCompleted() is true on SUCCESS and on a quiesced FAILURE, and the status string marks an in-doubt one. The bounce reactor gtests get a 300 s per-test ctest TIMEOUT, so a hang fails fast under its own name instead of consuming the 2700 s stage budget. The success path is unchanged: no new messages, locks, clock reads or allocations. No wire or Python change. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
|
PR_Github #81179 [ run ] completed with state Resource maintenance in LLM/main/L0_Test-x86_64-Single-GPU #9591: L40S-PyTorch-2 is temporarily paused due to Kubernetes agent provisioning and node issues.
|
6f065bd to
5ffdef9
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #81284 [ run ] triggered by Bot. Commit: |
|
PR_Github #81284 [ run ] completed with state Resource maintenance in LLM/main/L0_Test-x86_64-Single-GPU #9624: L40S-PyTorch-2 is temporarily paused due to Kubernetes agent provisioning and node issues.
|
…isCompleted()
A failed bounce request now records what it may still touch in caller
memory: the sent chunks the receiver has not answered (it may still
scatter them into the destination) and the gathers still running (they
may still read the source). The record lives in a pure core,
bounce/BounceResult.{h,cpp}, and only shrinks once the result is set.
- wait() still returns only after the receiver has answered every sent
chunk, at most request_timeout_ms after the failure or after the last
answer, and at once after kNoProgressTimeout and at shutdown.
- isCompleted() is true once the record is empty and never turns back
to false. Answers arriving after an in-doubt result still count, and
an orphaned gather finishing completes it.
- Failed requests move from the active table to mFailed;
settleFailedRequests, at the end of every IO tick, is the only place
that resolves them and drops their entries.
- The receiver NACKs a DATA it will not scatter unless a scatter still
holds its region. A failed scatter launch syncs the stream before its
NACK. kBounceVersion goes to 3, and BounceSender rejects a
non-positive request_timeout_ms.
- Tests: a host-only test runs FailedRequest through every event order
and checks the contract after each event. Integration tests cover the
reactor around it.
Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
|
/bot run |
|
PR_Github #81425 [ run ] triggered by Bot. Commit: |
|
PR_Github #81425 [ run ] completed with state
|
Dev Engineer Review
The sender now retains failed requests while sent chunks remain unanswered. Matching ACKs and NACKs reduce the outstanding set. A failed result resolves after the configured answer wait or immediately on shutdown. If answers remain outstanding, the result does not prove that the destination is quiescent. Running gathers can also keep source memory in use after failure.
BounceTransferStatus::isCompleted()now requires both a ready result and quiescence. Callers must usewait()to read the transfer outcome;isCompleted()alone does not indicate success. The receiver synchronizes its stream before it NACKs a failed scatter.The wire protocol version changes from 2 to 3. Peers must use compatible versions. This conflicts with the PR description’s claim that the version is unchanged. The sender also rejects non-positive request timeouts. Ownership-mode concerns remain out of scope. Review finding counts are unavailable.
QA Engineer Review
The changes add and modify bounce tests, helpers, and test build configuration. The tests cover failure resolution, sent-chunk answer matching, timeout and shutdown paths, duplicate or malformed messages, receiver behavior, and end-to-end transfer status. The H100 CI list includes the C++ bounce unit-test selector. The QA lists include a related Python bounce integration test, but no manual-QA entry for the changed C++ unit tests was found.
The supplied PR description reports that five bounce gtest binaries passed with zero skips. The supplied CI reports include two failures, but give no specific test failures. The L40S-PyTorch-2 stage was paused, reducing coverage. Coverage verdict: needs follow-up. The reported CI failures need investigation; the available evidence does not identify their causes.
Per-File QA Perspective
cpp/include/tensorrt_llm/executor/transferAgent.h— Updates theisCompleted()contract. Verify that callers distinguish completion from transfer success.cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceTransferStatus.h— Adds future-backed status behavior and requires quiescence for completion. Verify timeout, invalid-future, and in-doubt failure handling.cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceTransport.cpp— Changes failure tracking, ACK/NACK handling, timeout resolution, and scatter-failure synchronization. Verify outstanding chunks and gathers are retained until they can no longer access memory.cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceTransport.h— Adds failed-request tracking and count accessors. Verify the declarations and lifecycle match the implementation.cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/transferAgent.cpp— Uses the extractedBounceTransferStatus. Verify the submit path preserves expected caller-visible status behavior.cpp/tests/unit_tests/executor/bounce/bounceAgentE2ETest.cpp— Covers successful completion and receiver shutdown before progress. The C++ bounce selector is listed in H100 CI; no corresponding manual-QA entry was found.cpp/tests/unit_tests/executor/bounce/bounceTestNixlNode.h— Changes shared test-agent configuration by disabling the listen thread. Verify tests using this helper do not require listening behavior.cpp/tests/unit_tests/executor/bounce/bounceTransportFailureTest.cpp— Adds transport failure-path tests for message validation, answer matching, timeouts, peer removal, and shutdown. The C++ bounce selector is listed in H100 CI; no corresponding manual-QA entry was found.cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceConfig.h— Documents timeout validation and its role in failed-request answer waits. Verify the configuration layer and sender enforce the documented positive-timeout requirement.cpp/tests/unit_tests/executor/bounce/CMakeLists.txt— Adds the failed-request test target and changes reactor test timeout settings. Verify the new target is included in the intended test build and CI selector.cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceMessage.h— ChangeskBounceVersionto 3 and clarifies NACK semantics. Verify peer-version mismatch handling and compatibility expectations.cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceResult.cpp— Implements answer tracking, quiescence, and failure resolution. Verify deadline refresh and unanswered-chunk behavior at timeout.cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/BounceResult.h— Adds failure reasons, outstanding-access tracking, and the failed-request API. Verify result lifetime and source-read tracking across asynchronous work.cpp/tensorrt_llm/executor/cache_transmission/nixl_utils/bounce/CMakeLists.txt— AddsBounceResult.cppto the bounce core library. Verify the library and its consumers link the new implementation.cpp/tests/unit_tests/executor/bounce/bounceFailedRequestTest.cpp— Adds host-side model and event-order tests for failed-request resolution and status. The C++ bounce selector is listed in H100 CI; no corresponding manual-QA entry was found.Description
The C++ transfer-agent bounce moves a KV write in chunks. For each chunk, the sender gathers it from the source KV into staging, RDMA-writes it into the receiver's arena, and sends DATA. The receiver then scatters the chunk into the destination KV pages and answers ACK (after
cudaStreamSynchronize) or NACK.On success the sender resolves only after every ACK. On failure (write or gather failure, receiver NACK,
forgetPeer, no-progress timeout, shutdown) it resolvedkFAILUREimmediately, while chunks whose DATA was already out could still be queued or running on the receiver. A late scatter could therefore write pages the caller had already reused. Separately,isCompleted()was false on every failure, so the ownership bridge could never prove that a failed bounce transfer had stopped touching memory.This PR provides the C++ contract the ownership bridge needs before it admits bounce.
Contract
wait()is the logical outcome. On failure it returns once the receiver has answered (ACK or NACK) every chunk whose DATA was sent, orrequest_timeout_msafter the failure or after the receiver's last answer. The answer wait restarts on each answer, so a live but slow receiver can stretch it to at most N ×request_timeout_msafter the failure, where N is the number of sent chunks unanswered at the failure. It returns at once afterkNoProgressTimeoutand at shutdown. A failure resolved before every answer arrived is in-doubt.isCompleted()is true once the request no longer reads its source and no longer writes its destination, and it never turns back to false. After a failure it can turn true later thanwait()returns: a late answer, or an orphaned gather finishing, completes it.TransferStatus::isCompletedstates this as one backend-independent contract about the caller's source and destination memory. Mooncake is the documented exception, and a NIXL status reads false afterrelease()or once its agent is gone.Design
The safety-relevant decisions live in a small pure core,
bounce/BounceResult.{h,cpp}, with no CUDA, NIXL, zmq or threads:OutstandingAccessis what a failed request may still do to caller memory: its unanswered sent chunks, matched by chunk index and region, and its running gathers, each held as aSourceReadthat ends when the orphaned gather is dropped. It is recorded before the result is set and only shrinks afterwards, which keepsisCompleted()monotone.BounceResultis{state, reason, outstanding}.stateandreasonnever change.FailedRequesthas two phases.weak_ptrto the record, so it is worth keeping only while a sent chunk is unanswered and the result's holders or a running gather keep the record alive.answerWaitFor(trigger, requestTimeout)is 0 forkNoProgressTimeoutandrequest_timeout_msotherwise. Shutdown resolves unconditionally.BounceSenderkeeps two tables:mRequestsholds only active requests; active-path handlers never see a failed one.failRequesttakes the request out ofmRequests, records its unanswered sent chunks, and retires its chunks: in-flight writes and gathers become orphans as before. It defers the cancel while a write is still landing, and moves the request tomFailed.answerFailedRequest, which only applies the answer.settleFailedRequestsis the only place that resolves a failed request and drops its entry, apart fromfailAllat shutdown.checkTimeoutsruns it at the end of every IO tick, after every caller of the two functions above.failAllwaits (bounded) for outbound writes, moves the active requests intomFailed, and resolves everything.Receiver:
onDataclaims the region first. If a scatter still holds the region, the DATA is dropped without an answer, because that scatter may be writing this very chunk. Otherwise, a DATA that does not decode, or whose region the flow no longer holds, is NACKed, so its sender stops waiting for that chunk.kBounceVersionis bumped to 3, because the sender now relies on these NACK semantics. A v2/v3 pair fails the handshake and falls back to standard NIXL; mixed versions are not supported.request_timeout_msmust be positive:BounceSenderrejects a non-positive one, as the config layer already did.Cost
main. There is no new allocation, clock read, lock, map operation per DATA/ACK, or message.BounceResultcarries one nullshared_ptr, and the per-tick sweep ofmFailedis an empty loop.wait()returns after the receiver's remaining answers, typically milliseconds, or within the bound above for a silent receiver.mFailedwhile a sent chunk is unanswered and the result is held or a running orphaned gather keeps the record. Each costs a weak-pointer lock and a count read per IO tick.failedRequestCount()exposes the table size and is used by the tests.Scope and known limits
native/ownership.py), which pollsis_completed()before releasing pages. That mode still refuses bounce (kv_cache_bounce_size_mb == 0in_validate_fp4_mla_bridge_profile). Lifting that gate, with a bridge + bounce regression, is a follow-up PR. There is no Python change here.wait()hold-back closes the context-side failed-chunk race for callers that trustwait(), except for an in-doubt result. Generation-side cancel and multi-writer fan-in still free pages without waiting for other writers. The Python bounce avoided this with its retained bounce region, so this PR alone does not make removing the Python bounce ([None][chore] Remove the Python KV bounce buffer; kv_cache_bounce_size_mb selects the C++ agent bounce #20049) safe outside ownership mode.lastProgress, which keeps the success path free of clock reads. A no-progress timeout right after a late DATA therefore resolves in-doubt, and a late answer still completesisCompleted().failRequestcan break the promise.forget()/shutdown without answering. Those requests stay in-doubt, which is conservative.noweach caller passes tofailRequest, the in-doubt log line, or therequest_timeout_ms > 0check.Test infrastructure
useListenThread=false). They already wire agents withloadRemoteAgent(name, getLocalAgentDesc()). Before this change, every test agent took the shared/tmp/trtllm_nixl_port.lock. Once another user owned that file (and for root underfs.protected_regular=2), agent creation failed and every transport test was silently SKIPPED.TIMEOUT, so a hang fails under its own name.Test Coverage
bounceFailedRequestTest(new, host-only, no GPU, about 0.3 s). It runsFailedRequestthrough every order of the events that can reach a failed request, for 0–2 sent chunks, with and without a running gather, and with both a normal and a zero answer wait. The events are:After every event, a model checks that:
isCompleted()holds exactly when the request is resolved and nothing is outstanding, and never reverts;The file also tests
answerWaitForfor every reason, the status contract over hand-built results, andSourceRead.bounceTransportFailureTestchecks the reactor around the core:FailureWaitsForRunningScatter,FailureWaitsForQueuedScatters,NackWaitsOnlyForOtherSentChunksAndDefersCancelUntilWriteEnds,OnlyMatchingAnswersEndTheWait,AnswerWaitStartsAtTheFailure;SilentReceiverResolvesInDoubtAfterTimeoutSinceLastAnswer,AbandonWithSentChunkResolvesInDoubtWithinOneTimeout,LateAnswerCompletesAnInDoubtFailureAfterItsLastWrite, which uses a real receiver;StatusCompletesOnlyOnceOrphanedGatherFinishes, and recycling viaFailureRecyclesAGatheredChunkStillWaitingForItsCreditandFailedRequestWithUnpostedChunksStaysInert;forgetPeerand shutdown:ForgetPeerFailsOnlyActiveRequestsAndLaterAnswersStillCount,ShutdownResolvesUnansweredRequestsInDoubtWithTheirOwnReason;RefusedDataIsNacked,RefusedDataGetsNoNackWhileItsRegionScatters,DuplicateDataProducesOneScatterAndAck.Ordering. Events are ordered with exec-context holds, a stream gate, scripted writes and a zero-size-WANT → NACK reactor barrier. The tests sleep only where elapsed time is what they measure.
bounceAgentE2ETestaddsFailureWithNothingSentIsCompletedand anisCompleted()check on SUCCESS.Seeded bugs. Each of the following fails at least one test:
failAllignoring the abandon cause;Local results (RTX A6000,
CUDA_MODULE_LOADING=LAZY):bounceFailedRequestTest4,bounceTransportFailureTest32,bounceTransportTest7,bounceAgentE2ETest7, all passing with 0 skipped. CI: pending/bot run; the executor_bounce pass/skip counts will be added here.PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.🤖 Generated with Claude Code