Repository navigation
fix(runtime): cancel TCP request readers while the input buffer is full - #15003
bojiang-li wants to merge 2 commits into
Conversation
Signed-off-by: bojiang-li <327132355+bojiang-li@users.noreply.github.com>
|
👋 Hi bojiang-li! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. WalkthroughThe TCP request reader now allows local kill and stop signals to interrupt blocked sends. Tests cover full-channel behavior for local and remote cancellation, receiver closure, and normal draining. ChangesTCP request cancellation handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The cancellation change is mergeable based on the available evidence. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Signed-off-by: bojiang-li <327132355+bojiang-li@users.noreply.github.com>
Summary
When a bidirectional engine retains its request input but stops consuming it,
handle_request_readercan block inbytes_tx.send(data).await. Its outer cancellation branches are no longer polled, so a local context kill or stop leaves the TCP reader and its channel senders alive until the consumer drains or drops the receiver.Poll the existing kill/stop futures while forwarding each input frame. Either local signal exits the reader and drops the pending frame, matching the existing request-reader cancellation behavior. Receiver drop, FIFO delivery, upstream cancellation accounting, and wire protocol remain unchanged. Unlike local context cancellation, a Stop/Kill frame behind input data on the same socket still follows that data in wire order.
The production caller is
Ingress<ManyIn<T>, ManyOut<U>>::parse_and_build_request: a capacity-8 decoded-input forwarder can retain the capacity-64 transport receiver while the engine stops consuming. This change closes the transport reader's senders; it does not claim to cancel every higher-layer forwarder or release GPU resources.Five real loopback-TCP tests use a read-completion notification and a full capacity-1 channel to establish the blocked send without sleeps. They cover local kill/stop, consumer drop, healthy drain, and ordered remote kill. Existing control-message tests distinguish remote Stop and Kill context state and cancellation accounting. The notification wrapper exists only in the test module; production reuses the existing cancellation futures.
Related: #15001 fixes the opposite, response-receive direction, where graceful Stop must preserve pending response data. This request-input path already exits on either local Stop or Kill. Reviewed #9991, #11149, #9674, #14849 and #13786; none covers this full request-channel wait.
Validation
cargo test -p dynamo-runtime --lib pipeline::network::tcp::client::tests -- --nocapture: 40 passed, including all five retained full-channel cases and both direct Stop/Kill control tests.cargo fmt --all -- --check,cargo clippy -p dynamo-runtime --lib --tests -- -D warnings -A unused-variables, changed-file pre-commit, andgit diff --check: passed. The Clippy allowance is for the existing macOS-onlyjoin_reuseportunused parameter inquic_response.rs.client.rsis byte-identical to the previous PR head.aef7484f8066edbd809c68ac06a9d0873c12eab0. The runtime library reports 805 passed, 0 failed, 2 ignored; all five retained full-channel tests and the existing Stop/Kill control-message tests explicitly pass. The Pre Merge run also completes doc tests, additional feature checks, benchmark compilation, Clippy, and pre-commit. The separate full GPU/deployment pipelines have not been triggered for this head; they require maintainer approval.AI-assisted investigation and implementation. CPU validation on macOS only; no GPU or end-to-end model-serving claim. The initial sandbox run failed before TCP setup because loopback listening was denied; the results above come from the permitted local-network run.
Summary by CodeRabbit
Bug Fixes
Tests