fix(vllm): preserve decode handoff on cancellation

[connorcarpenter15]

Overview:

Part 2 of a 4-PR stack. This layer preserves disaggregated NIXL handoff completion when a client cancels a decode request.

Details:

• Submit decode requests even when client cancellation wins the initial race.
• Keep the decode response stream alive until the first token or terminal transfer signal so the receiver can complete and release transferred KV.
• After cancellation, map premature EOF, gRPC failure, or response-conversion failure to a Cancelled terminal while logging the underlying failure.
• Continue cancelling aggregate and prefill requests by dropping only their per-request gRPC response stream.
• Do not call vLLM Abort or close the shared pooled channel.

The post-cancellation wait intentionally has no arbitrary timeout; safe bounded cleanup requires a protocol-level transfer-completion or cleanup signal.

Stack

1. #12734 — split gRPC services and Control discovery
2. #12736 — preserve decode handoff on cancellation
3. #12735 — deterministic DP and KV routing
4. #12214 — multimodal sidecar requests

Base: #12734

Validation

• cargo fmt --all -- --check
• cargo test -p dynamo-vllm-sidecar — 17 tests passed
• cargo clippy -p dynamo-vllm-sidecar --all-targets -- -D warnings

Where should the reviewer start?

• lib/sidecar/vllm/src/engine.rs for the cancellation race and stream-lifetime behavior.
• lib/sidecar/vllm/src/tests.rs for first-token deferral and premature-EOF regression coverage.

Related Issues

This PR is NOT linked to an issue:

• Confirmed — no related issue

Summary by CodeRabbit

• Bug Fixes

  • Improved cancellation handling for streaming requests.
  • Decode requests now complete necessary handoff and receive the first output token before cancellation takes effect.
  • Premature stream closure and cancellation-related errors now produce a clear cancelled result.
  • Shutdown cancellation remains immediate.

• Documentation

  • Clarified cancellation behavior for aggregate, prefill, and decode requests.

[devin-ai-integration]

Devin Review found 2 potential issues.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

Walkthrough

vLLM generation now separates request cancellation from shutdown. Decode requests remain active until stream setup and first-token or transfer completion, then emit cancelled output. Tests cover delayed cancellation and premature EOF, and the README documents the behavior.

Changes

vLLM request cancellation flow

Layer / File(s)	Summary
Separate request and shutdown cancellation lib/sidecar/vllm/src/engine.rs, lib/sidecar/vllm/README.md	Generation uses independent request and shutdown cancellation paths. Decode cancellation is deferred during stream setup. The README documents stream ownership and cancellation behavior.
Preserve and terminate cancelled decode streams lib/sidecar/vllm/src/engine.rs, lib/sidecar/vllm/src/tests.rs	The response loop tracks first-token and transfer completion. Cancellation-related stream failures produce cancelled output with warnings. Other errors remain propagated.
Validate decode cancellation states lib/sidecar/vllm/src/tests.rs	Tests cover cancellation before headers, while waiting for the first token, after token release, and on premature EOF.

Estimated code review effort: 4 (Complex) | ~45 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes preserving the vLLM decode handoff when cancellation occurs.
Description check	✅ Passed	The description includes the required sections and clearly explains the changes, review focus, related issues status, and validation performed.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

lib/sidecar/vllm/src/tests.rs (1)

926-951: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the remaining cancellation-mapping arms.

decode_cancellation_maps_premature_eof_to_cancelled covers the Ok(None) arm in engine.rs. Two sibling arms remain untested: the Err(status) arm that maps a gRPC failure after cancellation to a cancelled result, and the Err(error) arm that maps a conversion failure after cancellation. Both arms are new and both silently convert a failure into a success value, so a regression there is easy to miss.

The FakeVllm service already supports the required hooks. Add a fail_before_first_token flag that yields Err(Status::internal(...)) after the prompt response, and a flag that emits an invalid response (for example, num_tokens that does not match token_ids.len()) to trigger the conversion error path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/sidecar/vllm/src/tests.rs` around lines 926 - 951, Add tests alongside
decode_cancellation_maps_premature_eof_to_cancelled covering both remaining
cancellation mappings: configure FakeVllm with a fail_before_first_token hook
that returns Err(Status::internal(...)) after the prompt, and with an
invalid-response hook such as mismatched num_tokens and token_ids length to
trigger conversion failure. For each case, stop generation after starting the
stream and assert the terminal result is a successful cancelled output with
FinishReason::Cancelled.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/sidecar/vllm/README.md`:
- Line 70: Update the cancellation behavior paragraph near the decode stream
description to document both release conditions: the stream is retained until
either the first output token or a terminal response containing finish_info.
Also state that premature stream termination, gRPC failures, and
post-cancellation conversion failures produce a cancelled result.

In `@lib/sidecar/vllm/src/engine.rs`:
- Around line 223-242: Update the request-processing loop around
request_cancelled and stream.message() to create one per-request cancellation
deadline before the loop, then include that deadline in the post-cancellation
select. When it expires, emit the cancelled output and exit the loop, while
preserving the existing shutdown and stream-message handling and ensuring
intermediate messages do not restart the deadline.

---

Nitpick comments:
In `@lib/sidecar/vllm/src/tests.rs`:
- Around line 926-951: Add tests alongside
decode_cancellation_maps_premature_eof_to_cancelled covering both remaining
cancellation mappings: configure FakeVllm with a fail_before_first_token hook
that returns Err(Status::internal(...)) after the prompt, and with an
invalid-response hook such as mismatched num_tokens and token_ids length to
trigger conversion failure. For each case, stop generation after starting the
stream and assert the terminal result is a successful cancelled output with
FinishReason::Cancelled.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3b175f35-3496-4ef9-a108-770acd167493

📥 Commits

Reviewing files that changed from the base of the PR and between 50bcfb6 and 5aaaa4f.

📒 Files selected for processing (3)

• lib/sidecar/vllm/README.md
• lib/sidecar/vllm/src/engine.rs
• lib/sidecar/vllm/src/tests.rs

[connorcarpenter15]

/ok to test a67345b

[alec-flowers]

I would have thought cancellation / lifecycle would be handled by unified backend not specific to each framework sidecar

[connorcarpenter15]

Right now cancellation happens by dropping the gRPC response stream, which is owned by the sidecar. Also, to preserve the decode handoff, the sidecar needs to read vLLM-specific fields from transfer params, so that logic cannot live in the unified backend. I think this is the right location for this code until the sidecars are more stable and we can define more clear abstractions.