Skip to content

[Bugfix] Retain duplex request IDs until cleanup succeeds - #7708

Open
aayc123 wants to merge 5 commits into
vllm-project:mainfrom
aayc123:codex/issue-7636-cancel-cleanup
Open

aayc123 wants to merge 5 commits into
vllm-project:mainfrom
aayc123:codex/issue-7636-cancel-cleanup

Conversation

@aayc123

@aayc123 aayc123 commented Sep 17, 2026

Copy link
Copy Markdown

Purpose

Addresses issue 8 in #7636 and the cleanup lifecycle concern in:
#7413 (comment)

Keep cancelled-fence request resources until stage cleanup completes so session close or expiry can retry a failed cleanup.

Cancellation now: prepare fence cancellation, await stage_port.cleanup(..., abort=True), then release the cancelled fence’s resources only after cleanup succeeds. The accepted fence still advances before cleanup, so old-epoch outputs remain stale. No new retry mechanism, locking, or session state.

Test Plan

Add a regression test covering failure followed by close cleanup retry. It verifies epoch advances, runtime_signal_failed is emitted, the cancelled request ID remains registered, close retries cleanup with the same request ID and abort=True, and the resource is released after retry.

pytest -q \
  tests/engine/duplex/test_session_runner.py::test_stale_epoch_output_is_dropped_after_barge_in \
  tests/engine/duplex/test_session_runner.py::test_cancel_cleanup_failure_retains_requests_for_close_retry \
  tests/engine/duplex/test_session_runner.py::test_close_emits_session_closed_and_releases_stage_requests

pytest -q tests/engine/duplex -m "core_model and cpu"

vLLM Version: 0.29.0

vLLM-Omni Commit: 8fe5b42

Test Result

  • Targeted cancellation and close tests: 3 passed
  • Full duplex CPU suite: 345 passed, 1 skipped
  • Ruff, formatting, typos, CI markers, SPDX, forbidden-import, and diff checks passed.
  • Pre-existing mypy errors outside this diff: model_channel.py:753, model_channel.py:797, test_session_runner.py:187, test_session_runner.py:1070.
  • CPU-only validation; no GPU or model weights required.

AI Assistance

Used OpenAI Codex to assist with issue analysis, implementation, regression testing. I reviewed and understand the final changes and validated them locally.

Keep cancelled-fence request resources until stage cleanup completes so session close or expiry can retry a failed cleanup. Add a regression test covering failure followed by close cleanup.

Signed-off-by: aayc123 <2803023760@qq.com>
@vllm-omni-review-bot

Copy link
Copy Markdown

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

Module owners: @fake0fan @tzhouam @lishunyang12

Routing: @fake0fan via module of the changed files, semantic router, CODEOWNERS; @tzhouam via module of the changed files, semantic router, CODEOWNERS; @lishunyang12 via module of the changed files

@aayc123, 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.

@vllm-omni-review-bot

vllm-omni-review-bot commented Sep 17, 2026 •

Copy link
Copy Markdown

Omni ReviewBot triage note

Automated triage of commit 23550fb78333 produced:

  • Priority: high. Prompt maintainer attention is suggested.

These are automated triage suggestions only — the final decision belongs to the maintainers.

@aayc123

aayc123 commented Sep 17, 2026

Copy link
Copy Markdown
Author

Self-review:

  • Confirmed the accepted fence advances before cleanup, so stale outputs are still rejected.
  • Cancelled request resources are released only after cleanup succeeds; on failure they remain available for close/expiry retry.
  • No new locking, retry loop, or session state was introduced.
  • Added regression coverage for cleanup failure followed by successful close cleanup.
  • Local duplex CPU suite: 345 passed, 1 skipped.

@hsliuustc0106 hsliuustc0106 added bug Something isn't working core related to core module: cache, scheduler, engine, worker, modelrunner labels Sep 17, 2026
Comment thread vllm_omni/engine/duplex/session/model_channel.py
aayc123 and others added 3 commits September 22, 2026 14:38
Signed-off-by: aayc123 <2803023760@qq.com>
Signed-off-by: aayc123 <115020989+aayc123@users.noreply.github.com>
Signed-off-by: aayc123 <2803023760@qq.com>
@aayc123

aayc123 commented Sep 24, 2026

Copy link
Copy Markdown
Author

@hsliuustc0106 Thanks for the review. I removed the cancel_fence wrapper as suggested and updated the affected call sites to use the explicit prepare_cancel_fence -> cleanup -> release_fence flow. The conflict with main has also been resolved, and all CI checks now pass. Could you please take another look when you have time?

@vllm-omni-review-bot

Copy link
Copy Markdown

Omni ReviewBot: no human activity for 7 days

@aayc123 this pull request has had no human commit, comment or review since 2026-09-24. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state.

To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline.

@vllm-omni-review-bot

Copy link
Copy Markdown
Omni ReviewBot routing record

Assigned Strict on zcode (GLM-5.3-Flash) under experiment fleet-strict-cursor-grok46-zcode-glm53flash-5050-c5-z10-20261002.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working core related to core module: cache, scheduler, engine, worker, modelrunner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants