Skip to content

[Bugfix][Frontend] Roll back cancelled duplex resume delivery - #7429

Closed
LOGO127 wants to merge 1 commit into
vllm-project:mainfrom
LOGO127:fix/duplex-resume-cancel-main-0911
Closed

LOGO127 wants to merge 1 commit into
vllm-project:mainfrom
LOGO127:fix/duplex-resume-cancel-main-0911

Conversation

@LOGO127

@LOGO127 LOGO127 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Purpose

Treat cancellation during duplex resume activation/replay like the existing failed-delivery rollback, then propagate CancelledError. Without it the rotated, abandoned connection generation remains current and the prior token cannot use the intended retry path.

This is a two-file fix against current main 02aaa34; it does not include PR #7413's framework changes. The same issue was discussed with its author, and a separately validated author-branch adaptation is preserved. This main version uses the current incarnation-based API.

Test Plan and Results

  • Four new deterministic cancellation cases: takeover/reconnect, each during activation or replay. 4 fail on unchanged main and pass after the fix.
  • The attachment registry, lease and public duplex-client files together: 63 passed, 0 failed, 0 skipped.
  • The cases assert cancellation propagation, obsolete attachment removal, released outbound lock, retained replay and one-shot old-token recovery. This preserves existing failed-delivery semantics; it does not claim to redesign grace timers or arbitrary repeated-cancel handling.
  • Applicable changed-file pre-commit checks passed, including mypy-3.10, SPDX and test markers.
python -m pytest tests/entrypoints/openai/test_duplex_session_attachment.py tests/engine/duplex/test_duplex_lease.py tests/clients/test_duplex_client.py -m 'core_model and cpu' --run-level core_model -q

Retained output: 63 passed, 15 warnings in 0.78s (rerun on signed head e07daf3). Python 3.12 CPU environment, actual registry/lease/client code; the local adapter disables NVML discovery only. No GPU or live model run is claimed. Earlier 66-test results against #7413 are not reused as current-main results.

AI assistance: ChatGPT assisted with implementation, tests, and this description.

Signed-off-by: luozijian <luozijian0924@gmail.com>
@LOGO127

LOGO127 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Contributor-side self-review for signed commit e07daf39:

  • Rechecked that cancellation enters the existing guarded failed-delivery cleanup and is then re-raised. The four regressions cover takeover/reconnect during activation/replay, obsolete attachment removal, retry/replay and one-shot recovery.
  • Reran the exact signed main-based tree: 63 passed, 0 failed, 0 skipped across attachment, lease and client tests. The retained four-case baseline fails on unmodified main. Prior 66-test evidence on the framework author branch is not counted here.
  • This is only the two-file main fix, not [Core][Frontend] Unified Full-duplex Framework #7413's framework. No GPU/live-model validation, timer redesign, or arbitrary repeated-cancellation guarantee is claimed. Applicable pre-commit results were rechecked against the unchanged signed content.

AI assistance: ChatGPT assisted with implementation, tests and these review notes.

@vllm-omni-review-bot

Copy link
Copy Markdown

This PR appears to belong to: docs/design/module/profiling.md, docs/design/module/entrypoints.md, docs/design/module/observability.md.

Module owners: @alex-jw-brooks @linyueqian @NickCao

Routing: @alex-jw-brooks via module of the changed files, module named in the PR description, semantic router, CODEOWNERS; @linyueqian via module of the changed files, module named in the PR description, semantic router, CODEOWNERS; @NickCao via module of the changed files, module named in the PR description, semantic router, CODEOWNERS

@LOGO127, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer.

Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment.

@hsliuustc0106 hsliuustc0106 added bug Something isn't working frontend code related to entrypoint labels Sep 11, 2026
@linyueqian

Copy link
Copy Markdown
Collaborator

@LOGO127 this landed on main through the framework PR: #7413 (99ff4f30, merged 2026-09-16) changed the same block in session_attachment.py to except (Exception, asyncio.CancelledError) and routes both failures through a shared _rollback_resume helper, which is why this branch now conflicts there. The production change in this PR is therefore already in place. If any of your four cancellation cases (takeover and reconnect, during activation and during replay) is not covered by the tests that came with #7413, a test-only follow-up against current main would still be welcome; otherwise this can be closed.

LOGO127 commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the pointer. I checked current main after #7413. Its cancellation regression covers the detached reconnect path for both activation and replay delivery, but it does not exercise the attached takeover variants from this PR.

I prepared a minimal test-only follow-up on current main that parameterizes the existing cancellation regression over takeover and reconnect, keeping production code unchanged. git diff --check and Python syntax validation pass locally. The full focused pytest is currently blocked in this workspace because the compatible vllm Python package is not installed, so I am not publishing or claiming a passing runtime result yet.

I’ll keep #7429 as the historical production fix for now and move the remaining value into that test-only follow-up rather than trying to resolve the production conflict here.

@LOGO127

LOGO127 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for clarifying the new framework boundary. I have confirmed the production rollback is already covered by merged #7413. The independent four-case cancellation regression (attached takeover / detached reconnect, each during activation / replay) is now isolated in test-only #8200, so I am closing this older conflicting implementation rather than carrying duplicate production changes.

The current #8200 attachment test file passes 25 supplemental CPU leaf tests and all applicable pre-commit hooks. That is not a full native-package or GPU result; the local installed vLLM/current-Omni API mismatch is recorded separately. #8200 remains a draft pending personal review/DCO completion. No additional production fix is claimed here.

@LOGO127 LOGO127 closed this Oct 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working frontend code related to entrypoint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants