Skip to content

[None][fix] Complete queued context cancellations before activation - #21482

Merged
chienchunhung merged 1 commit into
NVIDIA:mainfrom
chienchunhung:dev/pact-queued-ctx-cancellation
Nov 4, 2026
Merged

chienchunhung merged 1 commit into
NVIDIA:mainfrom
chienchunhung:dev/pact-queued-ctx-cancellation

Conversation

@chienchunhung

@chienchunhung chienchunhung commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Complete cancelled, never-activated CTX-only requests before executor admission. They return a terminal result without waiting for capacity, another model batch, a GEN request, or the transfer timeout.

Scope

  • Queued cancellation preserves client/beam/sequence identities, publishes each terminal result once, and removes queue bookkeeping without execution-resource teardown.
  • Prompt-logprob handling accepts cancelled sequences with no computed logits while preserving cached streaming data and success-path checks.
  • Applies to CTX-only requests in both context-first and generation-first serving. Activated requests, ordinary CONTEXT_AND_GENERATION requests, GEN-only requests, KV connectors, retries, and transfer-retirement policy are unchanged. Ordinary queued serving is a separate follow-up.

Verification

  • 29 added CPU cases: 23 queue/native-response cases and 6 prompt-logprob cases. All passed on x86 and ARM at a0d3a61 in CI #67830; the 301-case executor module also passed on both.
  • Complete source self-review and static checks passed. Current-head full CI #68137 is running; GPU/NIXL retirement and full cancellation-workflow qualification are not claimed.

Notes

This queue-only change does not depend on #21272 or #21291. Keep the shared prompt-logprob guard and tests as one implementation when merging #21291. No public API or configuration change.

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82109 [ run ] triggered by Bot. Commit: 7a3287f Link to invocation

@chienchunhung
chienchunhung marked this pull request as ready for review November 2, 2026 23:45
@chienchunhung
chienchunhung requested review from a team as code owners November 2, 2026 23:45
@coderabbitai

coderabbitai Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The executor now cancels eligible queued context-only requests before admission and publishes cancellation responses. The worker handles final cancelled results without context logits. Tests cover queue behavior, response handling, and prompt-logprob behavior.

Changes

Queued Context Cancellation

Layer / File(s) Summary
Remove and answer queued context requests
tensorrt_llm/_torch/pyexecutor/py_executor.py, tests/unittest/_torch/pyexecutor/test_queued_context_cancellation.py, tests/unittest/_torch/pyexecutor/test_py_executor.py
Before admission, the executor removes eligible cancelled context-only requests from the waiting queue and enqueues cancellation responses under the configured rank rules. Tests cover queue types, response contents, result routes, unrelated state, and rank configurations.
Handle cancellation results without context logits
tensorrt_llm/executor/base_worker.py, tests/unittest/executor/test_base_worker.py
The worker returns None for prompt logprobs when a final result has only cancelled finish reasons and no context logits. Tests check cached prompt logprobs and preserve the assertion for successful results without context logits.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant PyExecutor
  participant WaitingQueue
  participant ResponseQueue
  PyExecutor->>WaitingQueue: Remove matching queued context request
  PyExecutor->>ResponseQueue: Enqueue cancellation responses
Loading
Merge Risk: Minimal · up to a0d3a

Queued cancellations can complete before admission. Investigate the reported CI failure before merging.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 72.73% 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. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title follows the required [None][fix] format and clearly identifies the primary change: completing queued context cancellations before activation.
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.
Description check Passed The description follows the repository template in substance. It has a Summary, a Scope section, and a Verification section that lists the test coverage, with 29 added CPU cases (23 queue/native-respo…
Full details: Description check

Explanation

The description follows the repository template in substance. It has a Summary, a Scope section, and a Verification section that lists the test coverage, with 29 added CPU cases (23 queue/native-response and 6 prompt-logprob) and the CI runs. It clearly states what changes and why: cancelled, never-activated CTX-only requests get a terminal result before admission. It also states what stays unchanged (activated requests, GEN-only, KV connectors, retries, transfer-retirement policy). The template's PR Checklist and title format are not addressed explicitly. The description gives no explicit title in the required [ticket][type] format, and it does not state whether an API label is needed beyond saying no public API or configuration change. Those are non-critical gaps. The description is mostly complete.

Full details: Docstring Coverage

Explanation

Docstring coverage is 72.73% 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 11 functions across 4 files.

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
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 7a3287f and a0d3a61.

Files selected for processing: 3
  • tests/unittest/_torch/pyexecutor/test_py_executor.py
  • tests/unittest/_torch/pyexecutor/test_queued_context_cancellation.py
  • tests/unittest/executor/test_base_worker.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82109 [ run ] completed with state SUCCESS. Commit: 7a3287f
/LLM/main/L0_MergeRequest_PR pipeline #67742 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
@chienchunhung
chienchunhung force-pushed the dev/pact-queued-ctx-cancellation branch from 7a3287f to a0d3a61 Compare November 3, 2026 04:53

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82201 [ run ] triggered by Bot. Commit: a0d3a61 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82201 [ run ] completed with state SUCCESS. Commit: a0d3a61
/LLM/main/L0_MergeRequest_PR pipeline #67830 completed with status: 'UNSTABLE'

CI Report

⚠️ Multi-GPU Label Required:
Multi-GPU tests require the ci: full pre-merge approved label on this PR. Either:

  • Wait for the PR to be fully approved — the label is added automatically once approval is complete. Having unresolved open comments is fine, or
  • If needed, ask a member of NVIDIA/trt-llm-ci-approvers to add the label manually.
    Then re-trigger CI with the same bot command (no rebase needed).

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

Link to invocation

@cascade812 cascade812 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix only covers REQUEST_TYPE_CONTEXT_ONLY. A queued CONTEXT_AND_GENERATION request (normal non-disaggregated serving) that gets cancelled still goes through _handle_canceled_requests. That removes it from the waiting queue without sending any response.
It's an existing gap, not a regression, but it's the same problem in the same place, should we fix it as well?

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements.

@chienchunhung

Copy link
Copy Markdown
Collaborator Author

The fix only covers REQUEST_TYPE_CONTEXT_ONLY. A queued CONTEXT_AND_GENERATION request (normal non-disaggregated serving) that gets cancelled still goes through _handle_canceled_requests. That removes it from the waiting queue without sending any response. It's an existing gap, not a regression, but it's the same problem in the same place, should we fix it as well?

Thanks @cascade812 . This PR targets queued, never-activated CTX-only requests in both context-first and generation-first disaggregated serving. You are right that ordinary CONTEXT_AND_GENERATION requests have the same existing response gap. This will be addressed in follow-up PRs.

@chienchunhung

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82372 [ run ] triggered by Bot. Commit: a0d3a61 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82372 [ run ] completed with state FAILURE. Commit: a0d3a61
/LLM/main/L0_MergeRequest_PR pipeline #67990 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82544 [ run ] triggered by Bot. Commit: a0d3a61 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82544 [ run ] completed with state SUCCESS. Commit: a0d3a61
/LLM/main/L0_MergeRequest_PR pipeline #68137 completed with status: 'SUCCESS'

CI Report

Link to invocation

@chienchunhung
chienchunhung merged commit 20c4047 into NVIDIA:main Nov 4, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants