Skip to content

[Model] Add JoyAI speech input to unified duplex sessions - #8931

Merged
natureofnature merged 18 commits into
vllm-project:mainfrom
natureofnature:feat/joyai-duplex-asr-20261014
Oct 16, 2026
Merged

natureofnature merged 18 commits into
vllm-project:mainfrom
natureofnature:feat/joyai-duplex-asr-20261014

Conversation

@natureofnature

@natureofnature natureofnature commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Contributor intent

These answers follow the speech-input work agreed in #8803.

1. What outcome do you want to achieve?

Let JoyAI unified duplex sessions accept spoken instructions alongside camera observations.

2. What is the highest-leverage variable for that outcome?

Reuse the existing ASR pipeline and Session input lifecycle.

3. How will this work move that variable?

Route committed speech through ASR into JoyAI, retain its transcript in conversation history, and expose microphone input in the shared WebUI.

4. Whose customer or user needs does this address, and what are those needs?

JoyAI interactive-video users who need to give spoken instructions and continue observing the scene afterward.

Purpose

Follow-up to #8823 and the speech-input task in #8803.

The current JoyAI Session profile accepts text and visual observations. This adds an opt-in four-stage ASR profile while retaining the existing three-stage camera profile.

  • Speech commits enter Qwen3-ASR, then JoyAI and the existing TTS stages. Text/camera observations enter JoyAI directly.
  • Deferred images stay on the orchestrator's raw prompt until the JoyAI handoff. The ASR EngineCore wire carries its own input and metadata, matching the existing ordinary-engine path.
  • The comprehension stage supplies the tokenizer and context budget. ASR transcripts update the exact committed audio item through the Session mailbox, so subsequent camera observations retain the spoken instruction.
  • Transcript-only ASR output remains user input; AURA regression coverage checks that it is not projected as assistant speech.
  • ASR writeback is bound to the original committed audio part. Each audio version receives only its own request's transcript, including when a public item ID is deleted and reused.
  • The shared WebUI gains opt-in push-to-talk and server-VAD speech modes. In VAD mode, observations accompany speech commits; push-to-talk retains camera cadence outside recording.
  • The VAD profile passes the public Session validator. Empty or muted PTT holds leave camera observations available, and rejected commits release only their own pending UI state.
  • Accepted quiet-PCM commits carry their client event ID through the no-response acknowledgement, allowing the UI to finish that hold while preserving other pending or active replies.
  • VAD speech-start and server cancellation clear queued local playback through the shared interrupt handler, including when generation has already completed. Response IDs keep late cancellation or audio from affecting a newer reply.
  • Idle camera input retains the latest frame and one audio chunk of pre-roll. Speech keeps the full utterance through commit; silent camera commits enter JoyAI directly and release their reserved bytes.

Updated contracts and usage: docs/design/fullduplex.md, recipes/JD/JoyAI-VL-Interaction.md, and examples/online_serving/realtime_web/README.md.

Test Plan

vLLM Version: 0.31.0; PyTorch 2.13.0+cu130; Transformers 5.14.1.

vLLM-Omni Commit: d60174fbd71acb95e9bdc642689b7710a43ae990, based on 1b6c6c50.

CPU tests ran inside a remote Docker container with CUDA_VISIBLE_DEVICES= and this checkout on PYTHONPATH:

python -m pytest -q tests/engine/duplex tests/entrypoints/duplex \
  tests/model_executor/models/joyai_vl_interaction \
  tests/model_executor/models/aura_omni \
  tests/model_executor/stage_input_processors/test_joyai_vl_interaction.py \
  tests/engine/test_duplex_omni_engine.py tests/engine/test_duplex_orchestrator.py \
  tests/engine/test_orchestrator.py \
  tests/examples/test_realtime_web_profile_validation.py \
  tests/examples/test_realtime_web_profiles.py \
  tests/examples/test_minicpmo_realtime_web_server.py \
  -m "core_model and cpu" --run-level=core_model
node --test tests/examples/test_realtime_web_profiles.cjs

Test Result

Real-model validation ran in remote Docker on two shared H800 80 GB GPUs, with JoyAI on one card and ASR/TTS/Code2Wav on the other. Test-only placement used one sequence per stage and bounded memory budgets; production context lengths and compiled/CUDA Graph execution were retained.

Case Result
Two camera observations, speech output and session admission reuse Passed
Quiet PCM commit followed by speech in the same session Commit identity preserved; ASR transcript and spoken response matched the Seed-TTS input sentence
Server VAD with paced PCM and no manual commit Real pinned Silero ONNX detected speech start/stop, committed input and produced transcription plus audio
Spoken instruction and camera together, followed by camera-only turns ASR ran once; red, blue and green observations produced Red, Blue, Green and PCM16 audio

These runs use the shared DuplexClient, real model weights and the existing visual E2E helper. The instruction-retention input was synthesized with ffmpeg Flite. Shared-card elapsed times are diagnostic rather than performance results.

  • Current-head Python suite: 766 passed, 1 skipped, 20 deselected (49.36 s), including the added shared-orchestrator regressions. Node is available, so browser-profile-to-public-validator checks run. The skipped case checks default Silero discovery; the GPU VAD E2E explicitly supplied the pinned ONNX artifact and passed.
  • Audio-plus-camera fails on 532aeb35 because deferred PIL images enter the upstream MessagePack encoder. The new real-codec regression fails with that exact error on the old head and passes after the boundary filter, retaining the original image for asr_to_joyai. Dict, already serialized, and absent metadata also pass.
  • The quiet-PCM backend regression fails on 95846229 because the no-response acknowledgement loses the client's commit ID. Five browser cases fail on the same head. They pass with this fix, including two pending commits, both acknowledgement orders, mixed rejection/silence, and another pending or active response.
  • The new public delete/recreate-audio regression fails on fb4b17bf: the old ASR transcript overwrites the recreated audio item. It passes with this fix, including acceptance of the new audio's own transcript.
  • The VAD and idle-camera regressions produce 5 failed, 3 passed on 0fc12c7c and 8 passed with the fix. They cover public configuration validation, bounded silence, subsequent speech acceptance, visual entry, reservation accounting, and rollback.
  • Current-head browser tests: 64 passed, including quiet/empty/muted PTT, rejected commits, actual playback interruption during/after generation, played-cursor acknowledgement, and late old-response fencing. The six playback cases fail on d1c53402 with 23,872 queued frames remaining and pass with this fix.
  • Ruff check and format passed. Full pre-commit could not start because the test container has no Git executable.
  • Combined speech + summary-memory snapshot 7019a1a4: 703 passed, 1 skipped, 20 deselected (115.32 s), plus 64 browser tests passed. This separate integration run uses the shared Session/Aura/JoyAI suite documented in [Model] Add bounded same-stage summary memory for JoyAI duplex #8932, rather than this PR's standalone head. All five new backend cases pass again after test-only formatting.

Validation remaining: manual microphone interaction in a browser. GPU tests above cover the real model pipeline; CPU tests separately cover lifecycle races and malformed input with controlled stage fixtures.

Example:
Screenshot from 2026-10-15 06-54-00
Screenshot from 2026-10-15 06-55-05

Screenshot from 2026-10-15 06-55-28

Development used Codex for implementation, tests, documentation, and review.

Signed-off-by: natureofnature <wzliu@connect.hku.hk>
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
Signed-off-by: natureofnature <wzliu@connect.hku.hk>
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@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: @lishunyang12 @tzhouam @fake0fan

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

@natureofnature, 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 Oct 14, 2026 •

Copy link
Copy Markdown

Omni ReviewBot triage note

Automated triage of commit 8ae79eb683f8 produced:

  • Priority: high. Prompt maintainer attention is suggested.

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

Signed-off-by: natureofnature <wzliu@connect.hku.hk>

Copy link
Copy Markdown
Collaborator Author

Self-review at 0fc12c7

Reviewed with Codex assistance: ASR versus camera/text entry routing, JoyAI tokenizer/context-budget ownership, transcript retention and cancellation fencing, exact committed-audio identity, and microphone/playback state handling. The delete/recreate-audio fix is present; the issues below are separate.

1. [P1] The advertised server-VAD profile cannot initialize

The new profile sends interrupt_response: false, but the public duplex validator rejects that value. session.update invokes this validator, and the browser rejects the handshake before session.updated.

Reproduced by generating the actual JS profile payload and executing the exact Python validator: audio.input.turn_detection.interrupt_response=false is unsupported. Thus joyai-turn --speech-input --vad cannot start. Align the profile with supported server behavior, or implement non-interrupting VAD, and add a profile-to-protocol validation test. The current browser test mocks a successful session update.

2. [P2] Empty PTT leaves camera observations blocked

Start JoyAI speech mode, mute, then press/release Hold to talk. No PCM is uploaded, but release calls submitTurn unconditionally, setting the busy/turn-submitted flags and arming the 120-second watchdog. The server returns input_audio_buffer_empty; the nonfatal error handler logs it without releasing those flags. Camera observations stop, then the watchdog disconnects the session.

Reproduced with the unchanged app/profile source in the Node test harness, including the error and timeout callbacks. A new nonempty speech turn can recover before the timeout, but camera observations alone cannot. Only commit when that PTT turn uploaded input, and unwind pending-turn state when a commit is rejected.

3. [P2] Silent camera/audio input fills the pending PCM buffer

Independently reachable through the supported API using server_vad with interrupt_response: true; the advertised UI currently encounters issue 1 first. The runner preserves silent appends carrying vision, and the new JoyAI buffer unconditionally appends their PCM. Without speech, no VAD commit clears that audio.

A CPU source-level probe executing the real runner append method and buffer with deterministic silent-VAD fixtures retained 60 seconds of silent PCM after 60 seconds of supplied chunks. With a small fixed frame, 262.4 seconds of supplied chunks reached the default 16 MiB limit and produced input_backpressure. These are fixture-input durations, not measured GPU/browser timings. Once full, the reserve failure returns before VAD commit scheduling, also rejecting subsequent speech. Keep the latest frame and bounded audio pre-roll instead of retaining all idle PCM. The shared vision exception predates this PR, but JoyAI previously rejected audio; this combination is newly enabled here.

Validation limits

The 41 browser unit tests pass. Additional Node and exact-source CPU probes reproduced the issues above with fixture transport/VAD boundaries. The full Python suite was not rerun in this review environment, which lacks pytest/vLLM/PyTorch. Real-model GPU speech → ASR → JoyAI → TTS and physical microphone/camera validation remain outstanding.

Signed-off-by: natureofnature <wzliu@connect.hku.hk>

Copy link
Copy Markdown
Collaborator Author

Self-review of d1c5340271ee43c4e7f640f006e48904107c3a41, with Codex assistance. The earlier configuration, empty-PTT, and idle-audio-buffer findings are fixed. One additional issue remains:

[P2] Clear local playback when VAD interrupts a reply. With joyai-turn --speech-input --vad, speech cancels the server response, but the browser keeps playing its queued audio. JoyAI's event mapper falls through for input_audio_buffer.speech_started and output_audio_buffer.cleared; a cancelled response.done follows the normal drain path. None reaches the shared interrupt handler that clears playback.

Please map these events to the shared interrupt action in VAD mode, preserving response-ID fencing. Cover both interruption during generation and speech starting after generation has completed but audio is still queued; Qwen's profile handles both cases.

Independent validation: all 50 existing browser tests pass. Additional Node probes using the unchanged app and actual playback-worklet source, with the server's interruption event sequences, retain 23,872 queued frames and produce nonzero audio after interruption in both cases, with zero clear messages. This is a source-level browser harness reproduction; physical microphone/browser and real-model GPU end-to-end tests were not run.

Signed-off-by: natureofnature <wzliu@connect.hku.hk>

natureofnature commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator Author

Follow-up at 532aeb35079a1817be1ed415189ff5bf5c4db05d (2026-10-15)

The idle quiet-PTT case below is fixed. One P2 residual ordering case remains: a quiet PTT hold made after the previous response.done, but before its queued audio finishes playing.

With Camera off, submitTurn resets the older reply's responseComplete and arms a new watchdog. The backend drops quiet overlapping input and returns a correlated input_audio_buffer_empty error. settleCommitWithoutResponse removes the pending commit but leaves the old response state unchanged because currentResponseId still belongs to that reply. After playback drains, responseComplete is still false; no second response.done arrives to repair it. Camera observations remain blocked and the watchdog disconnects.

Reproduced using the public decoder/overlap/commit/ACK source, serialized server events, unchanged app, and actual playback worklet: the old 1,000 ms of audio fully drains and is acknowledged, yet turnSubmitted remains true with no pending commits; camera submits zero frames and the watchdog ends the session. The new overlap tests cover the inverse order, with the old completion arriving after the quiet hold.

Please preserve the prior reply's terminal/playback state and settle the rejected hold's watchdog without clearing a genuinely active reply. Add the response.done → quiet commit → rejection → playback drain order to the tests. The 64 existing browser tests pass; this extra regression fails. Model/GPU and physical-browser validation were not run.


Previous review (idle case now fixed)

Full self-review of 95846229aea73ee8a2171b6d4bc8abc883f84a47, with Codex assistance. The previous playback issue is fixed: both prior reproductions now clear queued audio and produce silence. All 57 browser tests pass; the full patch, ASR/history ownership, cancellation and shared routing were also checked.

[P2] Finish quiet PTT turns that intentionally produce no response. Start --speech-input without enabling Camera, then hold/release without speaking. Nonempty quiet PCM makes the UI submit and mark itself busy. The public codec classifies it as non-speech; the backend accepts the silent commit without creating a response. It sends input_audio_buffer.committed with event.no_response: true, followed by response.listen.

JoyAI's mapper ignores both. The error-recovery path cannot help because this commit succeeded. turnSubmitted and the watchdog remain set: enabling Camera afterward cannot submit observations, and the watchdog disconnects after 120 seconds.

Reproduced with exact public decoder/runner/projector source and serialized wire events fed into unchanged app.js, including low-amplitude nonzero PCM. Please complete accepted no-response turns without disturbing another response's ownership, and add a quiet-PCM/no-camera regression. The existing muted/zero-sample tests do not cover this case. These are fixture-based tests; full GPU and physical microphone validation remain outstanding.

@hsliuustc0106 hsliuustc0106 added core related to core module: cache, scheduler, engine, worker, modelrunner frontend code related to entrypoint labels Oct 14, 2026

Copy link
Copy Markdown
Collaborator Author

[P2] Preserve retained speech across a camera-only commit

Source review at 3700fb6: the ASR profile can silently discard already-acknowledged speech through this public API sequence in a manual-turn session (auto_response: false, audio.input.turn_detection: null):

  1. input_audio_buffer.append with non-silent base64 PCM16 audio, format: "pcm16", and sample_rate_hz: 16000.
  2. input_audio_buffer.commit with final: true, create_response: false.
  3. input_audio_buffer.append with video_frames: [frame] and no audio.
  4. input_audio_buffer.commit with final: true, create_response: true.

The first commit retains the normalized PCM. The second commit merges the retained payload with the frame-only payload, but merge_audio_payloads returns the second argument whenever either payload lacks format: "pcm_f32le". The PCM disappears, and entry-stage selection bypasses ASR. The speech item remains an untranscribed placeholder.

Please update the latest camera frame without dropping pending speech or transferring its transcript ownership to the camera commit. Add a regression for this sequence that asserts ASR receives the original PCM and writes back to the original speech item; cover the same merge while speech is deferred behind an active response.

This finding is based on tracing the pinned public commit and routing paths; I did not execute the regression or model pipeline.

Keep retained normalized audio and its transcript item when a later
camera-only commit updates the observation, including deferred turns.
Bind incoming item IDs before merging committed payloads.

Use the current ASR transcript for the JoyAI query; an empty transcript
falls back to scene observation instead of repeating an earlier question.

Signed-off-by: natureofnature <wzliu@connect.hku.hk>
@natureofnature natureofnature added the ready label to trigger buildkite CI label Oct 15, 2026
@natureofnature

Copy link
Copy Markdown
Collaborator Author

@ShuoleiWang PTAL

@natureofnature

Copy link
Copy Markdown
Collaborator Author

@kunkunblueberry PTAL

@amy-why-3459 amy-why-3459 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.

Looked at the delta since 3700fb6. Dropping the context["query"] fallback in asr_to_joyai fixes the empty-transcript case I raised earlier, and binding realtime_item_id before the merge reads cleaner than patching it afterwards. One question on the new camera-only branch in the shared merge helper below.

# item that owns its eventual transcript.
return {**first, "video_frames": second["video_frames"]}
if first.get("format") != "pcm_f32le" or second.get("format") != "pcm_f32le":
return second

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.

Two things on this branch:

  1. It replaces first["video_frames"] instead of merging them, while the audio+audio path below concatenates both frame lists. If the retained speech payload already carried frames (e.g. the user asks "what is this?" while holding something up, and a camera-only commit follows before the deferred turn is submitted), those frames are gone from the turn that actually answers the question. Should this be [*first.get("video_frames", []), *second["video_frames"]] (or keep the latest N), to match the other path?

  2. This lives in the shared overlap_policy, so it also changes behaviour for other models that allow video-only commits: before, a camera-only commit after retained speech fell through to return second and replaced it. The new test only covers JoyAI. Could you add a regression for at least one other duplex model, or gate this on a capability, so the semantic change is explicit?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 7da541f. The camera-only merge now concatenates the retained and incoming frames in capture order, preserving the original speech audio and item ownership. Model-specific visual-window limits remain in the plugins.

Expanded the JoyAI regressions to include frames attached to the retained speech in both idle and deferred cases. Added an AURA runner regression using its actual plugin with manual commits, covering audio/frame preservation, transcript ownership, and byte accounting. The JoyAI idle/deferred scenarios also pass on H800 with real ASR and generated audio; the AURA coverage is a regression test, not a real-weight E2E run.

# eviction policy as direct observations only once it is known.
joyai_input["prompt"], images = render_joyai_session_prompt(
cached_processor_from_config(target_model_config),
system=context["system"],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My understanding is that a client can send additional_information={"joyai_prompt_context": {}}, causing context["system"] to raise KeyError. On the regular Chat API path, it looks like this exception would shut down the shared orchestrator.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 7da541f. The bridge now validates joyai_prompt_context before indexing it, including the required system and history fields. Malformed context, prompt-budget, and image-token metadata raise a request-local OmniClientError (400).

Added regression tests for empty/malformed contexts and for both the legacy and event-driven orchestrators. They verify that the invalid request is rejected, the orchestrator stays alive, and the next valid request completes. The related suite passes all 937 tests.

Reject malformed session prompt metadata with request-local client errors.
Keep retained video frames in capture order across camera-only commits.

Cover legacy and event-driven request isolation, subsequent valid requests,
and JoyAI/AURA retained audio, video and transcript ownership.

Signed-off-by: natureofnature <wzliu@connect.hku.hk>
Signed-off-by: natureofnature <wzliu@connect.hku.hk>

Copy link
Copy Markdown
Collaborator Author

CI follow-up: bcd0186 addresses the MiniCPM MRv2 timeout in Buildkite #17320.

On H800, I reproduced the second turn returning response.listen while the test waited for response.created. The test had reduced the 5.47-second response-required fixture to a 1.4-second active slice. Sending the complete fixture produced replies on both turns, so this test now uses the full audio and still requires two completed responses per session, real audio, session isolation, and zero errors.

Ran the updated test body, including all assertions, twice at 1/2/4 concurrent sessions against the real MRv2 server: all six cases passed, with 14 sessions and 28 completed audio responses. Applicable pre-commit hooks passed. These are H800 results; an H100 Buildkite rerun has not yet been verified.

@natureofnature natureofnature added ready label to trigger buildkite CI and removed ready label to trigger buildkite CI labels Oct 15, 2026
Queue playback and drain markers per response. Acknowledge an older
response's cursor after a newer response starts without finishing the
new turn or combining playback durations.

Cover overlapping PTT and clearing queued responses in the real
playback-worklet tests.

Signed-off-by: natureofnature <wzliu@connect.hku.hk>
Keep PCM boundaries per committed item and transcribe earlier items
through ASR before the final item continues to JoyAI and TTS. Carry the
original audio-part identities through continuations and advance their
request fences while keeping one assistant response.

Cover two/three commits, empty ASR, interleaved commits, cancellation,
submission races and failures, and item deletion/recreation.

Signed-off-by: natureofnature <wzliu@connect.hku.hk>

@amy-why-3459 amy-why-3459 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.

Went through 7da541f…e97bd16e since 33b0853. The camera-only merge now concatenates frames in capture order and keeps the retained speech / transcript owner, and the AURA runner case makes the shared overlap_policy change explicit — that answers the earlier question. Context validation raising a request-local OmniClientError is a good fix for the orchestrator-kill path Shuolei flagged.

The committed_audio_segments split plus item-bound input_audio_parts looks like the right ownership model for batched speech, and the playback worklet / playbackDrained ACK changes for overlapping responses are clear. One race on the continuation path is inline.

if isinstance(item, StageOutput):
await self.model.on_stage_output_item(item)
if self.model.is_input_transcription(item.request_id) and not await self._wait_for_append_tail():
return

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.

If _wait_for_append_tail() returns False here (a later chained append failed or was cancelled while this ASR segment had already finished), we drop the StageOutput without calling on_stage_output_item. That leaves ModelChannel._input_transcriptions[request_id] populated, skips stage cleanup / complete_model_turn, and never starts the remaining segment(s) — so a multi-commit batch can stall silently after a successful ASR completion.

The cancel and ASR-submission-failure tests cover the deliberate abort paths; this is the "tail failed underneath a finished transcription" case. Could we still run the transcription cleanup (pop the continuation, close the stream, cleanup the stage request) and either surface an error or explicitly abandon the remaining payload when the wait fails? A regression that fails the append_tail after the first segment's ASR output arrives would pin it.

Release the transcription stage request when a queued ASR output waits
for an append that fails or is cancelled. Abandon the remaining batch
and advance the request fence without changing the failed response event.

Cover output-before-ack failure and cancellation with real orchestrator
cleanup, plus successful submission of the next user turn.

Signed-off-by: natureofnature <wzliu@connect.hku.hk>
@natureofnature

Copy link
Copy Markdown
Collaborator Author

@amy-why-3459 PTAL

@amy-why-3459 amy-why-3459 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.

Looked at the incremental change in 1be43c5 (ASR cleanup after append-ack failure). The new test covers the leak nicely for the finished-segment case; two concerns about the abandon/cleanup path below.

if isinstance(item, StageOutput):
await self.model.on_stage_output_item(item)
if self.model.is_input_transcription(item.request_id) and not await self._wait_for_append_tail():
await self.model.finish_input_transcription(item, abandon=True)

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.

I think the abort=True that this ends up requesting (abandon and not segment_finished in finish_input_transcription) is a no-op for the failure this commit targets. By the time _wait_for_append_tail() returns False, the submit compensation in model_channel.py (except BaseException: await self._ctx.stage_port.cleanup([request_id]), ~L369-371) has already run a non-abort cleanup, which goes through _cleanup_request_ids -> _release_request_bindings. StagePool.abort_requests then finds no live binding (replica_id is None -> continue), so an ASR request the engine already accepted keeps running when its segment isn't finished yet.

Suggest aborting in the submit compensation itself when the engine may have accepted the request (at least when remaining_transcription_input is not None), rather than relying on the later abandon cleanup. It would also help to parametrize the new test with segment_finished=False and assert the abort actually reached the stage client; the current test only exercises segment_finished=True, so the abort branch is untested.

self._ctx.plugin.data_plane.close_stream(item.request_id)
# A failed submission may already have dropped the continuation. Its
# queued ASR output still owns stage cleanup unless cancel/close did it.
if (item.stage_id, item.request_id) in session.request_resources:

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.

Two things about this guard:

  1. In the ack-failure path it doesn't dedupe anything: the compensation cleanup at ~L371 doesn't pop request_resources[(stage_id, request_id)], so this always runs a second cleanup there (harmless, since _cleanup_request_ids is pop-based, but the comment suggests otherwise).
  2. On the normal success path this changes behavior: cleanup used to be unconditional, but request_resources is also pruned without stage cleanup elsewhere (concurrent-turn rebinding at ~L272/L282, the drain path at ~L1200). If the ASR request's entry is dropped there, a finished transcription would now skip stage_port.cleanup, leaving request_states / the session-manager registration behind.

Since cleanup is idempotent, could we keep it unconditional on the success path and only apply the guard (if needed at all) when abandon=True?

Abort while the replica binding is available when sending fails or is
cancelled and when committing accepted duplex input fails. Keep routing
and processor state if abort fails so owner cleanup can retry.

Finalize ASR stage cleanup even if its local session resource was pruned.
Cover partial and finished ASR, send and commit failures, cancellation,
abort retry, and successful submission of the next user turn.

Validation: 794 related regression tests and all changed-file pre-commit
hooks pass. Correct two existing StagePool typing issues exposed by mypy.

Signed-off-by: natureofnature <wzliu@connect.hku.hk>

@amy-why-3459 amy-why-3459 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.

Thanks for the quick turnaround. Looked at 41411d7 only (delta since 1be43c5). Aborting inside submit_initial while the binding is still live fixes the ineffective-abort issue from my last round, and dropping the request_resources guard in finish_input_transcription covers the pruned-binding case (e.g. the concurrent-turn branch in _append_via_data_plane that pops bindings without aborting). The new parametrized tests cover partial/finished ASR and both failure points nicely.

Two small follow-ups inline on the new submit_initial compensation; neither is blocking.

# Sending can fail or be cancelled after the engine accepted the
# request. Abort while its replica binding is still available.
try:
await self.abort_requests([request_id])

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.

On the CancelledError path this abort RPC is awaited inside a task that has already been cancelled, and every caller still holds _stage_input_submission(stage_id) here (duplex submit() and the regular add-request path), so later submissions to this stage wait on it. With a healthy engine that's fine. But if a second cancellation lands while it's awaited (e.g. shutdown cancelling again), the new CancelledError escapes the except Exception below: the warning is skipped and the original submission_error is replaced (only kept as __context__). State is still retained, same as the abort-failure branch, so it's safe. Consider await asyncio.shield(self.abort_requests([request_id])), or catching BaseException around the abort just to log and then re-raise the original, so this path stays observable.

try:
await self.abort_requests([request_id])
except Exception as abort_error:
# Keep routing and processor state so owner cleanup can retry.

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.

Side effect worth pinning with a test: before this change submit_initial always released the binding before re-raising, so in Orchestrator._dispatch_or_fail_request the EngineDeadError branch's dead_replica = pool.get_bound_replica_id(...) was always None for an add_request failure and _handle_dead_replica never ran from this path. Now, when the abort also fails on the dead client, the binding is kept and that branch will evict the replica. I think that's the better behavior, but it's an untested change in the non-duplex path. Could you add a case in tests/engine/test_orchestrator.py where both add_request_async and abort_requests_async raise EngineDeadError, asserting the replica is evicted and the request is failed exactly once?

Preserve the original submission error when abort receives a second
cancellation, and keep the failed abort observable and retryable.

Evict a dead replica before request cleanup so another failed abort cannot
escape the dispatch guard. Preserve CFG parent attribution and release
retained output processor mappings without an RPC to the evicted engine.

Add regressions for repeated cancellation, submission plus abort engine
death, ordinary and CFG requests, and subsequent routing with or without
a surviving replica. Exercise real output processor state cleanup.

Validation: 866 related CPU regression tests and changed-file pre-commit
checks pass.

Signed-off-by: natureofnature <wzliu@connect.hku.hk>

@amy-why-3459 amy-why-3459 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.

Thanks for the quick follow-up. Looked at 8ae79eb only (delta since 41411d7).

Catching CancelledError on the compensating abort so the original submission error is preserved, plus the new cancel/retry tests, closes the second-cancel gap from my last round. Evicting before _fail_request_dead_stage (and still committing OP state when the client slot is already None) fixes the retained-binding / dead-engine abort loop; the EngineDeadError × survivor/CFG regressions look solid.

One small non-blocking note inline.

# request. Abort while its replica binding is still available.
try:
await self.abort_requests([request_id])
except (Exception, asyncio.CancelledError) as abort_error:

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.

When the compensating abort_requests itself raises EngineDeadError but the original submission error was something else (e.g. CancelledError / RuntimeError), we log and re-raise the original — so _dispatch_or_fail_request's dead-replica path never runs and eviction waits for the poll loop. Also, if abort_requests_async raises, commit_aborted_request_state below it is skipped.

Worth promoting abort's EngineDeadError (or calling the same eviction helper) so a dead slot discovered during compensation is removed immediately. Not blocking given the poll path.

@natureofnature natureofnature added ready label to trigger buildkite CI and removed ready label to trigger buildkite CI labels Oct 16, 2026
@natureofnature
natureofnature enabled auto-merge (squash) October 16, 2026 03:40
@natureofnature
natureofnature merged commit 5a5083a into vllm-project:main Oct 16, 2026
8 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core related to core module: cache, scheduler, engine, worker, modelrunner frontend code related to entrypoint ready label to trigger buildkite CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants