[y-null]

Follow-up to #9260, answering @amy-why-3459's review findings.

1. The drain padding never sent a single unit (blocking).
_pad_silence_until_stopped called client.append_audio(...), which exists only on the duplex client in vllm_omni/clients/duplex.py; the runner instantiates RealtimeDuplexClient, which has no such method, so the AttributeError was swallowed by the broad except Exception and the padding task ended before sending anything.

RealtimeDuplexClient.append_silence now sends the same input_audio_buffer.append event shape stream_av_units sends — duration_ms/audio_end_ms included, _media_clock_ms advanced — so the server's step loop can keep stepping on appended silence as it did on the clip. The padding loop catches only (ConnectionClosed, OSError).

Tests: the fake client surface is now guarded by a mirror check against the real class (test_the_fake_client_mirrors_the_real_client_surface), and test_padding_works_against_the_real_client runs the real client with only send stubbed, asserting the append events and the advancing media clock.

2. A completed sample whose close stalls is published, not retried and lost.
close_timeout no longer rides result.error once the reply is done: it stays in metadata, output.error stays empty, and the sample goes through finalize_duplex_eval unchanged. _duplex_retryable_stall returns nothing for response_done=True, so the close cost is the handshake, not a full retry that could lose the sample. (test_completed_sample_with_a_stalled_close_is_not_retried, test_completed_sample_with_a_stalled_close_is_published.)

3. The outer close wait leaves the inner one room.
_close_duplex_session now waits timeout_s + _FORCE_CLOSE_GRACE_S outside; at the same value the outer timer always fired first and force-aborted closes that would still have received session.closed.

4. Inner wait_for leaks are no longer logged as the per-request timeout.
The retry branch compares elapsed time against the per-request deadline: a TimeoutError raised well before the deadline (e.g. the configure handshake) is logged as an inner wait_for leak, the deadline itself keeps its own message. (test_timeout_logging_distinguishes_inner_and_outer_timeouts.)

5. The judge follow-up says what failed to parse.
Any ValueError from parse_judge_json — a prose reply or a malformed value in a present field — now carries the parse error into the follow-up, and the first trace line records it, so the trace shows which reply failed and why. (test_parse_error_reaches_the_retry_and_the_trace.)

Also bounded — the same unbounded session write, earlier in the epilogue:

• the final commit write waits at most _FINAL_COMMIT_TIMEOUT_S; on a stall the commit_timeout is recorded and the padding still starts;
• the input stream waits at most clip duration + _INPUT_SLACK_S; on a stall input_timeout is recorded and the sample still lands with its transcript (input_committed=False).

Both are retried once as transport stalls by the existing retry logic. (test_stalled_commit_is_bounded_and_padding_still_starts, test_stalled_input_is_bounded_and_the_sample_still_lands, test_a_stalled_input_or_commit_write_is_retried_once.)

Test evidence: tests/benchmarks full directory: 875 passed locally (plain pytest run). Two fake servers were updated because the padding is real now: in the lifecycle test's none turn-detection mode the committed clip's reply drains on the first padding unit, so its transcript lands at 0.01 + 1.0s (the padding advances the media clock); in the multiturn test the streamed clip's reply fires once — appended silence no longer re-triggers it.

Follow-up (needs GPU, out of scope here): the 66-sample run quoted in #9260 predates this fix — its padding was a no-op, so its 0/66 drain_timeout does not speak to how the real server reacts to appended silence. A real-client 66-sample run and the full-repo UT will follow separately.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-11-03T06:50:45.170479Z We couldn’t complete this review. Comment @codex review to try again.	4298900	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

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.

[amy-why-3459]

Thanks for the follow-up. I went through the runner/patch/client changes and the new tests. The append_silence fix, narrowing the padding's except, and the outer-close grace all look right to me. A few code-level points:

1. A completed reply with an input_timeout/commit_timeout is now failed and never retried. generate_sample puts input_timeout/commit_timeout into result.error regardless of response_done, so output.success is False for such a sample. But the new early return in _duplex_retryable_stall (if metadata.get("response_done") is True: return None) skips the retry for any completed response. test_stalled_input_is_bounded_and_the_sample_still_lands produces exactly this state (response_done=True, input_timeout set, in result.error), and that sample would end up as a non-retried failure. That doesn't match the description ("Both are retried once as transport stalls"); test_a_stalled_input_or_commit_write_is_retried_once only covers response_done=False. If only close/ack are meant to be advisory, the early return should probably apply only when no input/commit/drain timeout is recorded. Otherwise, please document that a stalled input on a completed reply is a final failure, and add a test for that case.

2. The wire-order claim for a stalled commit isn't covered. The _commit_clip_then_pad docstring says the stalled commit "stays queued ahead of the padding on the same socket". That only holds if RealtimeDuplexClient.send serializes writes FIFO (a lock, or the transport's write queue). The new test actually shows the opposite in the fake: the padding's append_silence calls land while the commit never does. If the server receives padding units before input_audio_buffer.commit, the residual unit the commit is shaped to flush changes, which is the thing the docstring says padding must not do. Could you either not start the padding when commit_timeout is set, or confirm (and test against the real client) that sends are strictly ordered?

3. Cancelling stream_av_units mid-write. asyncio.wait_for(client.stream_av_units(...)) cancels the coroutine on timeout, possibly in the middle of a websocket send. Depending on the websockets version, a cancelled send can leave a partially written frame and the connection unusable, so the follow-up commit and padding may fail with ConnectionClosed instead of flushing the residual unit. It may also leave _media_clock_ms and frame_sink (image_items) in a half-advanced state. It would help to say what state the client is in after this cancel, or to stop at a unit boundary instead.

4. Inner-timeout classification. inner_timeout = elapsed + 0.001 < deadline will raise TypeError if _omni_request_timeout_s() can return None (no per-request timeout) and an inner wait_for leaks a TimeoutError. If None is a valid value, please guard it. The 1 ms margin is also tight relative to event-loop timer resolution and scheduling jitter under load; a relative margin would be safer for the log label.

5. Minor. input_committed = commit_timeout is None records False even if the shielded commit lands a moment later. That's fine as a conservative flag, but a comment noting it would help readers of the metadata. Also, appending str(exc) from parse_judge_json to the judge follow-up is useful, but if that message can embed the raw reply, it duplicates the reply that's already echoed in the follow-up. It may be worth capping its length.

[vllm-omni-review-bot]

This PR appears to belong to: docs/design/module/benchmarking.md.

Module owners: @alex-jw-brooks @Bounty-hunter @NickCao

Routing: @alex-jw-brooks via module of the changed files, CODEOWNERS; @Bounty-hunter via module of the changed files, CODEOWNERS; @NickCao via CODEOWNERS

@y-null, 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.

[y-null]

Real-client numbers for this PR, and one more fix they forced.

What the first real run exposed

With append_silence actually streaming (R1), the MiniCPM-o server answers every padded silence unit with a response.listen. The drain's quiet window counted those listens as completion activity, so each padded step reset stable_since and every drain ran to its timeout — the first run logged one drain with 151 listens, and 37/39 samples recorded a drain_timeout at ~3 min/sample. The no-op padding of the earlier runs never hit this because it never sent anything.

Fixed in 3a58649: in none mode the client sends nothing but silence after the commit, so the drain settle test now counts completion activity without response.listen. Reply events (created/done/delta) still count, and the server-turn path is untouched. The regression test replays the server's per-unit listens and fails on the previous tip; tests/benchmarks is at 876 passed.

End-to-end, same machine, same commands

run	padding	samples	drain timeouts	published	wall time
nightly on ada97391e (reference)	no-op	66	0/66	66	~27 min
this PR before the fix	real	39	37/39	39	~3.3 h projected
this PR + 3a58649	real	66	0/66	66 (all response_done)	~47 min

Scoring is unaffected by the drain change: the pre-fix 39 samples and the post-fix first 39 score 0.846 vs 0.769 mean all-success, within one standard error of a single run (server sampling is temperature 0.8). Artifacts and per-sample metas are retained under ~/duplex_eval_9802/ (responses/ pre-fix, responses_r2/ post-fix).

[amy-why-3459]

Incremental look at 3a58649f only (the one new commit on top of 42989003, which I reviewed earlier). I read the diff; I did not run it.

Excluding response.listen from the drain's quiet-window count makes sense given the measurement (every padded unit acknowledged with a listen), and the new test reproduces that shape with listens paced faster than the settle window. A few points:

1. Stale docstring. _pad_silence_until_stopped still says non-speech units "are expected to produce no response.listen/delta events and not to reset the completion activity window". After this commit that's no longer the assumption the drain relies on; it might be worth rewording it to say the server does acknowledge padded units with listens and that _DRAIN_ACTIVITY is what keeps them from resetting the window, so the next reader doesn't chase the wrong lead.
2. A drain whose only post-commit events are listens. Previously a model that chose to keep listening (no reply to the committed clip) still produced "activity" after the commit; with _DRAIN_ACTIVITY that drain now sees zero activity. Could you double-check that responses_finished (including the settled_silence_is_terminal path) treats "no non-listen activity since the commit" the way you intend, i.e. it settles rather than waiting for the drain timeout, and maybe add a small case for it next to the new test?
3. Scope of the exclusion. _DRAIN_ACTIVITY is applied to the whole post-commit window. If the server ever emits a reply-bearing response.listen (e.g. one that carries state the reply depends on) during the drain, it will no longer count. Not an issue with today's server as far as I can tell, just flagging that the comment ties this to 'none' mode while the helper doesn't check the mode itself.
4. Test strength (minor). test_padding_listens_do_not_reset_the_drain_quiet_window asserts drain_timeout is None, which is the key property. Asserting that at least one response.listen was actually recorded after the commit would make sure the test keeps exercising the exclusion and can't pass just because the fake's listen task never got scheduled before the settle.

[y-null]

Nightly-aligned validation on the followup tip, run through the same entry as the nightly job (tests/dfx/perf/scripts/run_benchmark.py + test_minicpmo_4_5_omni_duplex_eval.json: C8 x 66 CI IDs, soundtrack mix, cards_2, judge in the same run) on L20X. Only the worktree differs: 3a58649f3 vs the nightly's ada97391e.

run	completed	failed	duration (generate)	drain timeouts
nightly r1-r3	66/66 x 3	0	680 / 662 / 715 s	0
this PR (3a58649f3)	66/66	0	399.5 s	0

Published 66, published_with_unfinished_responses 0, failed_diagnostics 0, no artifact errors. Accuracy on the same judge: RTD content 1.933 (>= 1.8), RTD temporal 1.844 (>= 1.4), PR all-success 0.25 (recorded; the L20X lane does not enforce the CUDA PR floor - nightly recorded 0.278-0.306 on the same lane). rc=0 PASSED.

The 66/66 run with real padding, this C8 replay, and the earlier serial run agree: the padding now streams, the drain settles, and the run is faster than the no-op-padding nightly.

[amy-why-3459]

Incremental review of 882c7f3: this commit is ruff format only on tests/benchmarks/duplex/test_omni_duplex_eval_drain_padding.py and tests/benchmarks/patch/test_duplex_eval_retry.py (joined line breaks, removed trailing blank line). No behavior change and nothing new to flag beyond my earlier comments.

[y-null]

Thanks @amy-why-3459 — all four points from the 3a58649 review are addressed in 7c98035, none left open.

1. Stale docstring. _pad_silence_until_stopped now says the server acknowledges every padded unit with a response.listen and that _DRAIN_ACTIVITY is what keeps those acknowledgements from resetting the quiet window.

2. A drain whose only post-commit events are listens. Checked the path: with no response.created after the commit, _residual_unit_decided returns True on the first model-listen decision (finished_open >= open_at_commit is vacuously true, _is_model_listen_decision is what fires), and responses_finished then only needs the settled quiet window — it settles instead of waiting for drain_timeout. Added test_drain_settles_when_the_committed_clip_gets_only_a_listen next to the padding test: the fake answers the commit with a single model-listen and no response, and the test asserts drain_timeout is None.

3. Scope of the exclusion. Added a note next to _DRAIN_ACTIVITY that no runtime mode check is needed because responses_finished is only reachable through the padding path (server_turns false), so the 'none'-mode wording no longer implies a guard that does not exist.

4. Test strength. test_padding_listens_do_not_reset_the_drain_quiet_window now records the listens it sends and asserts at least one landed after the commit, so the test can no longer pass vacuously if the fake's listen task never runs.

python -m pytest tests/benchmarks/duplex/ tests/benchmarks/patch/test_duplex_eval_retry.py: 235 passed.

[amy-why-3459]

Incremental look at 7c98035a only (the one new commit on top of 882c7f38, which I reviewed earlier). I read the diff; I did not run it.

The reworded _pad_silence_until_stopped docstring and the _DRAIN_ACTIVITY comment now match how the drain actually behaves, and the new test_drain_settles_when_the_committed_clip_gets_only_a_listen covers the listen-only case I asked about. Two small test notes:

1. assert sent_listens checks that listens were sent, not that they arrived after the commit. The comment says the exclusion is exercised "only if padding listens actually landed after the commit", but sent_listens is appended to on every send, so if listen_every can start before input_audio_buffer.commit is handled, a listen sent before the commit would satisfy the assertion. If the task is only started on commit, this is fine as is. Otherwise, recording the index or time of the commit in the handler and asserting at least one listen after it would make the assertion match the comment.
2. The listen-only test can't tell "settled because of the listen" apart from "settled on an empty quiet window". drain_timeout is None would also hold if responses_finished settled without ever looking at the listen. If the point is that _residual_unit_decided fires on the model-listen decision, a variant where the fake sends no listen at all, plus an assertion on what that variant does, would pin down that the listen is what drives the decision. That's optional, but it would keep the test meaningful if the settle logic changes later.

Nothing else to flag in this commit.

[vllm-omni-review-bot]

Omni ReviewBot routing record

Assigned Strict on claude-code (mimo-v2.6-pro) under experiment fleet-strict-claudecode-mimo26pro-100-c10-20261015.

[vllm-omni-review-bot]

Omni ReviewBot review status

Review for commit 4c9506fcd8bb6448cf1802e5e48e859e697cd27a ended as blocked (step 'review' (agent.review_diff): [ensemble x3] API Error: Request rejected (429) · quota exhausted).

[amy-why-3459]

Review of 4c9506fcd8bb6448cf1802e5e48e859e697cd27a (static inspection; I did not run tests).

• Fix the completed-response guard in vllm_omni/benchmarks/patch/patch.py:316. The earlier input-timeout finding remains unresolved: test_stalled_input_is_bounded_and_the_sample_still_lands produces response_done=True with input_timeout included in result.error. _duplex_retryable_stall then returns early, so this recoverable transport failure becomes a final failed sample without the promised retry. A completed reply does not establish that the complete input reached the server. Restrict the exemption to advisory close/ack failures, preserving the existing checks for deterministic errors, and add retry coverage for response_done=True combined with input/commit timeouts.

• Propagate or explicitly record unexpected padding-task exceptions. Narrowing the catch in vllm_omni/benchmarks/duplex/omni_duplex_eval_runner.py:612 does not yet make coding errors surface: generate_sample still catches and discards every Exception when awaiting silence_task at lines 854–858. An AttributeError or other programming error therefore still silently stops padding and can appear as a drain timeout. Keep expected transport shutdown best-effort, but expose unexpected failures and cover this through generate_sample, not only by awaiting the padding helper directly.

The client/runner responsibility split is reasonable; the completion guard above is the main correctness issue. The diff-scoped redundancy pass found no additional actionable removal.

Please provide complete repository unit-test evidence for the updated head after these fixes, covering the CPU shards and applicable backend-specific UT jobs, with commands, environment/dependency versions, result counts, and CI or complete-log links. Explain skipped/deselected tests and unavailable jobs; compare base/head in the same environment for any pre-existing failures. The benchmark-directory results and the reported 66-sample runs on earlier revisions do not establish full-suite coverage on this head. Maintainers own admission to the project's Buildkite jobs. Merge readiness remains unestablished until the required evidence is available or maintainers explicitly disposition the gaps.

[y-null]

Both fixes from this review are in 0819c49, together with one find of my own that the requested test evidence surfaced.

1. Completed-response guard (patch.py). _duplex_retryable_stall no longer exempts every finished sample. response_done=True now only exempts the advisory close_timeout of a completed reply; an input_timeout/"commit_timeoutunder a finished reply is retried once like any other transport stall, and theserver_errors/_duplex_reported_errorchecks still run first so deterministic failures keep exactly one attempt. New parametrized testtest_an_input_or_commit_stall_under_a_completed_reply_is_retriedcoversresponse_done=True+ input/commit timeouts end to end through_async_request_duplex_eval`; the existing stalled-close tests still pass unchanged.

2. Padding-task exceptions (runner.py). generate_sample's cleanup no longer swallows every Exception from silence_task: CancelledError and a transport that is already dead (ConnectionClosed/OSError) stay best effort, and anything else now propagates out of generate_sample into the request wrapper's except Exception, so a padding coding error lands in output.error with its traceback instead of looking like a drain timeout. Covered by test_a_padding_coding_error_surfaces_through_the_sample, which breaks append_silence with an AttributeError and asserts the sample fails visibly through generate_sample.

3. What the test evidence surfaced. Running the fullduplex e2e tests on this branch showed 12 failures that the PR's own drain-padding commits introduced and that no CI job had ever run: the three e2e tests predate the padding and counted the padded zeroed units as clip audio (and one expected the reply's media timestamp before the padding advances the clock). Verified pre-existing to this review by stashing the two fixes above and re-running (same 12 failures). 0819c49 scopes those assertions to the wire before the commit (server VAD has no commit and is unchanged) and splits the realtime / as-fast-as-possible timestamp expectation; all 54 tests in that file now pass.

Unit-test evidence on 0819c49 (venv: Python 3.12.13, pytest 9.1.1, pytest-asyncio 1.4.0, ruff 0.14.10):

• python -m pytest tests/benchmarks/ -q → 879 passed, 1 failed. The failure (test_serve_cli.py::test_bench_serve_cli_mocks_http_request, OmniServingCapability has no IMAGE_EDITS) reproduces identically on the base tree 434a0d640 in the same venv — pre-existing.
• python -m pytest tests/buildkite/test_amd_pipeline.py tests/buildkite/test_upload_pipeline.py tests/dfx/perf/tests/test_runner_metadata.py tests/e2e/accuracy/omni_duplex_eval/test_omni_duplex_eval_ci.py tests/e2e/features/fullduplex/test_omni_duplex_eval_cli.py -q → 588 passed, 7 skipped, 99 failed, and the failed-name list diffs identical against the same command on 434a0d640 (environment-bound AMD-pipeline tests; nothing new on this branch, the 12 regressions above fixed).
• pre-commit on the touched files: 12 hooks pass; mypy-3.10 reports only the pre-existing runner.py:560 finding (raise task.exception(), from 434a0d6).
• CI on the previous head (4c9506f) was fully green (Analyze, CodeQL, DCO, build 3.11/3.12, pre-commit, docs); the same set is running on 0819c49.

Gap, stated plainly: I cannot run the repository's full CPU shard locally — pytest tests/ --collect-only -m cpu collects ~34k tests, hits 8 collection errors in this venv, and a full run is beyond this box. Per the review's own framing, the Buildkite CPU/backend shards remain maintainer-owned; the evidence above covers every test that imports the changed modules plus the whole benchmarks tree, with base/head comparisons on the same interpreter.

[amy-why-3459]

Incremental review of 4c9506fcd8..0819c4904d (static inspection of the new commit; I did not run tests).

Both findings from the previous review look addressed:

• _duplex_retryable_stall (vllm_omni/benchmarks/patch/patch.py:316-324) now keeps the server_errors / reported-error checks first, and only exempts a close_timeout under response_done=True. Since _duplex_timeout_reason checks input_timeout/commit_timeout before close_timeout, a completed reply that also has an input or commit stall is retried even when the close also stalled. test_an_input_or_commit_stall_under_a_completed_reply_is_retried covers this, because _session_result defaults to response_done=True.
• The silence_task cleanup in generate_sample (omni_duplex_eval_runner.py:850-863) now swallows only CancelledError and ConnectionClosed/OSError, and test_a_padding_coding_error_surfaces_through_the_sample exercises the full path through async_request_openai_realtime_duplex. Because the session runs inside async with client, the transport still gets torn down when the error propagates and skips the playback ack and close.

Remaining points (non-blocking):

• The new e2e expectation sentence_time = 0.8 if pace == "realtime" else 1.8 (tests/e2e/features/fullduplex/test_omni_duplex_eval_cli.py:381-386) relies on the first padded unit reaching the wire before the reply delta is processed in the as-fast-as-possible pace. That ordering comes from event-loop scheduling between the padding task and the reader rather than anything the test controls, so it could flake on a slower or loaded runner. If the fake server can make the order deterministic (for example, sending the delta only after it receives the first post-commit append), the assertion would stop depending on timing. Otherwise, asserting on the realtime value and accepting either value for the fast pace would be enough.
• A retry now throws away a completed reply's artifacts when that reply had an input or commit stall. If the second attempt then fails (for example, with a server error), the sample ends up recorded as failed even though the first attempt produced a full reply. That might be acceptable for this benchmark, but it would help to state the behavior in the docstring or retry_count metadata, or to add a test that pins it down, so it's an explicit choice.
• Minor: if the drain body raises something other than TimeoutError and the padding task then fails with a coding error, the padding exception replaces the original one, which survives only as __context__. That's an unlikely combination and doesn't need a change. I'm mentioning it only in case the traceback in output.error turns out to be confusing.

The test evidence is noted. I have not independently verified CI on 0819c4904d. The Buildkite CPU and backend shards are still with maintainers, as you said.