Repository navigation
[Bugfix][Core] Ignore completed duplex append tasks in response gating - #9282
Conversation
Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR appears to belong to: docs/design/module/engine_orchestration.md, docs/design/module/observability.md. Module owners: @tzhouam @fake0fan @lishunyang12 Routing: @tzhouam via module of the changed files, CODEOWNERS; @fake0fan via module of the changed files; @lishunyang12 via module of the changed files @amy-why-3459, 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. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
amy-why-3459
left a comment
There was a problem hiding this comment.
Thanks for the tight fix on the response-bound append gate.
Summary. has_response_bound_append_tasks previously treated any tracked response_bound entry as blocking, including tasks that had already finished but were still waiting for their done callback. That window is real under barge-in: prompt prep can complete in the same loop turn as VAD scoring, so a completed append kept the gate closed and delayed the next turn. Restricting the check to response_bound and not task.done() matches the documented intent (only unfinished response-bound appends hold the gate), and the new parametrized test covers both response_bound values through the sleep(0) callback window.
Looks good.
- The docs note in
fullduplex.mdcorrectly separates this gate from active-response / unplayed-output gates. - The test asserts the task remains in
append_tasksafter completion (still tracked) while the gate query already returns false — that is the race this change is aiming at. cancel_append_tasksafterward staying clear is a useful regression check.
Minor notes (non-blocking).
await asyncio.sleep(0)is the right way to yield for the done callback; worth keeping the comment so future readers do not “simplify” it away.- If other call sites ever assume “tracked ⇒ holds gate,” they should go through this helper rather than inspecting
append_tasksdirectly; today that looks consistent.
No blocking concerns from my side on this head.
| def has_response_bound_append_tasks(self) -> bool: | ||
| return any(meta.response_bound for meta in self.append_tasks.values()) | ||
| # A finished task can remain tracked until its done callback runs. | ||
| return any(meta.response_bound and not task.done() for task, meta in self.append_tasks.items()) |
There was a problem hiding this comment.
Clear and correct: a finished task can linger in append_tasks until the done callback runs, so gating on not task.done() is the right predicate. Matches the barge-in race the test covers.
|
@natureofnature PTAL |
natureofnature
left a comment
There was a problem hiding this comment.
Reviewed the append-task lifecycle, response/commit gates, cancellation paths, and regression test. Ignoring finished tasks is consistent with the separate active-response and playback gates; the new test captures the interval before the cleanup callback runs.
I suggest confirming the existing completed-append regression and Qwen multi-turn VAD test on this merged head before merging. The reported CPU results are for 1501839, and Buildkite #17904 is still pending. Both response-bound values should pass, and the VAD/overlap/queued-append variants should submit the second utterance separately without first-turn audio or errors. A current-head CI result covering these cases is sufficient; no additional GPU test is needed for this change.
【Conclusion: Static review found no blocking code issues in commit 65f7fa7.】
【Action recommended: Confirm the completed-append and multi-turn VAD tests pass on this head, with the second utterance submitted separately, before merging.】
#8483 #9272 #8802 #9248 #9282 #9264 (#9295) Signed-off-by: psv666 <2693925048@qq.com> Signed-off-by: haic0 <149741444+haic0@users.noreply.github.com> Signed-off-by: andyluo7 <andy.luo@amd.com> Signed-off-by: Joshna Medisetty <joshna.medisetty@intel.com> Signed-off-by: Chendi Xue <chendi.xue@intel.com> Signed-off-by: Wenbo Ji <36562829+fusheng-ji@users.noreply.github.com> Signed-off-by: NumberWan <wantszkin2003@gmail.com> Signed-off-by: Sylvan Liu <Sylvan.Liu@amd.com> Signed-off-by: wangqiankun13 <wangqiankun13@huawei.com> Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com> Signed-off-by: Weiming Liao <liaowm5@gmail.com> Co-authored-by: psv666 <150513104+psv666@users.noreply.github.com> Co-authored-by: haic0 <149741444+haic0@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: andyluo7 <andy.luo@amd.com> Co-authored-by: Joshna-Medisetty <joshna.medisetty@intel.com> Co-authored-by: Chendi Xue <chendi.xue@intel.com> Co-authored-by: Wenbo Ji 嵇文博 <36562829+fusheng-ji@users.noreply.github.com> Co-authored-by: dengyunyang <584797741@qq.com> Co-authored-by: NumberWan <wantszkin2003@gmail.com> Co-authored-by: Sylvan Liu <syliu@amd.com> Co-authored-by: wangqiankun13 <wangqiankun13@huawei.com> Co-authored-by: amy-why-3459 <wuhaiyan17@huawei.com> Co-authored-by: Weiming Liao <liaowm5@gmail.com>
Contributor intent
Purpose
Fix #9292
Buildkite #17871 failed
test_large_vad_append_submits_separate_audio_turns[True-barge_in_on_speech-server_vad]: the last submitted prompt still contained the first utterance instead of the second (0 == 16000).A completed append task can remain in
append_tasksuntil its done callback runs. When VAD and prompt preparation finish together, barge-in can cancel the previous response while that completed task is still tracked. The response gate incorrectly remains closed, so the next utterance is acknowledged without starting its response.Count only unfinished response-bound append tasks. Add a regression that completes a real asyncio task before its cleanup callback runs and checks both bound and unbound tasks. Document the gate in
docs/design/fullduplex.md; public configuration and API usage are unchanged.Test Plan
Project test dependencies are required, including
step-audio2==1.0.0(providescosyvoice2). CPU tests with Python 3.12, vLLM 0.32.0, PyTorch 2.13.0+cu130, Transformers 5.19.0. Base:59fedaf594bc4c13442f37668f2c7f33e8db1892.-o addopts=''disables the repository's pytest-xdist default because xdist is not installed locally; the test selection is unchanged.Test Result
Tested revision:
15018393365e1d1fe08f18f2493c3a36225863b7.Regression before the fix: 1 failed, 1 passed; the response-bound case reliably reproduces the stale gate.
Regression and duplex coverage: 1420 passed, 2 skipped, including the original Qwen3-Omni VAD test, using:
All applicable changed-file pre-commit hooks passed, including mypy-3.10, Ruff, test marks, SPDX/import guards, and Markdown lint.
Full Model Executor CI selection: 6044 passed, 6 skipped, 2 failed in 827.33s. Both failures were
test_ragged_dit_two_step_decode_matches_per_row_exact_decode(fp32-cache/bf16-cache) failing at import withModuleNotFoundError: cosyvoice2.Installed the already-declared
step-audio2==1.0.0into an isolated test dependency directory and reran those two cases: 2 passed. No repository dependency changes were needed. The full selection was not rerun after this environment correction.GPU/model-weight E2E validation and a new Buildkite run were not performed locally.