[y-null]

Split out of #9207 on review feedback: this changes how every Omni-DuplexEval run scores failures, so it gets its own review instead of riding along with the Code2Wav fix.

Extended on 2026-11-01 with the other two ways a 66-sample run loses samples: a reply that freezes when the input ends, and a judge reply that carries no score at all. The branch is rebased onto current main, which the judge retry depends on (#9390 rejects an incomplete judge reply; this PR re-asks for one instead of failing the sample).

Problem

A duplex sample writes its artifacts only after the session tears down. Two writes on that path have no deadline of their own — the closing acknowledge_playback() and the send({"type": "session.close"}) inside close_session() — and both ride the socket the session already used. When that socket stops draining, the sample hung until the benchmark's per-request timeout (900 s), was cancelled with defer_artifacts=True, and therefore wrote no artifacts at all. The accuracy pass counted it as skipped, and the C8 guard failed with assert completed == num_prompt while nothing in the log explained why.

Measured on a C8 replay of the MiniCPM-o 4.5 duplex case (Ascend 910B), with the graph-capture stall of #9207 already fixed and 0 graph captures during the replay: 7 of 66 samples lost, then 1 of 66 once the ack was bounded — the remaining one was the close.

Changes

1. Bound the closing ack (_FINAL_ACK_TIMEOUT_S, 20 s) and keep it advisory — it only checkpoints how much audio was played, so the sample still writes its artifacts and records ack_timeout.
2. Bound the session close (_FINAL_CLOSE_TIMEOUT_S): run it as a task behind one deadline, and on timeout abort the transport and close it without a handshake (_force_close_transport). close_session() awaits an unbounded send before its own timeout starts, and the client's exit path awaits the same handshake, so a stuck write would otherwise still hang teardown. The failure is recorded as close_timeout.
3. Join the cancelled ack loop with a deadline instead of waiting on it.
4. Retry only a stall that came alone. Any failure the session itself reported — a server_errors entry, or anything left in result.error once the recorded timeout strings are stripped — gets exactly one attempt, so a deterministic failure cannot be retried into a pass.
5. Make retries visible: per-attempt latency (the failed attempt does not inflate the reported latency), retry_count next to the deferred artifacts, and duplex_retried_samples / duplex_retry_count in the run summary.
6. Name the reason in output.error (no completed response) so an unsuccessful sample is diagnosable instead of silently missing.

Problem: a reply that freezes at end of input

The server's step loop is driven by the frames the client appends, and this client stops appending the moment the clip ends. A reply still open at end of input therefore froze mid-utterance: the audio and transcript deltas stopped together and response.done never arrived. This hit 1-3 of 66 samples per run and was reproducible in 6 of 6 attempts on two of them, so it was not jitter.

Evidence that the reply was still being generated rather than lost: every stalled request ended with OmniTiming total=<drain window>, i.e. the engine only stopped when the client gave up.

Problem: a judge reply with no score fields

Sample 512 (RTD_interaction_relation) answers the judge's re-read request with conversational text (C cuts the pineapple). Its 512.judge.jsonl records that raw reply and no 512.json is written, so the run reports evaluated 65 < 66, status="partial", and the accuracy assertion fails — deterministically, on every run.

Changes (continued)

7. Pad the drain with silence in the session's own chunk period (_pad_silence_until_stopped), so the step loop keeps running while the tail reply finishes.
8. Size the drain window from the input, input duration + 120 s (_DRAIN_SLACK_S), because a 37 s soundtrack asked for a summary answers for close to a minute while the window used to stop at input + 20 s.
9. Ask the judge again when it replies without score fields, once per judgement, by echoing its own reply back with the format instruction.

Tests

tests/benchmarks/patch/test_duplex_eval_retry.py, tests/benchmarks/duplex/test_omni_duplex_eval_close_stall.py, tests/benchmarks/duplex/test_omni_duplex_eval_drain_padding.py, tests/benchmarks/duplex/test_omni_duplex_eval_judge_retry.py (+ judge/serving suites) — 318 passed in tests/benchmarks/{duplex,patch}/ (1 pre-existing failure: torchcodec missing in the local container):

• latency covers only the attempt that succeeded
• a timeout folded into result.error is retried once; a server error plus a drain timeout is a single attempt with the error preserved
• no retry beyond the last attempt; a clean sample records no retry
• a close that never returns is torn down, and the sample still produces a result and its artifacts
• a close that ignores cancellation is torn down rather than awaited
• the drain padding starts only after the commit and stops when the drain ends
• cancelling the sample while the final ack is joined propagates the cancellation
• the judge retry records its own failure (truncated reply, or still no score fields)
• run-level tally aggregates per-sample counts

Verified end to end on the 66-sample duplex run (Ascend 910B, temperature 0): completed 66/66, published 66, artifact_errors [], accuracy evaluated 66 / failed 0, status completed, and the accuracy floors the config enforces (rtd.mean_content_score >= 1.8, rtd.mean_avg_temporal_score >= 1.4, pr.mean_all_success >= 0.28) all pass — with 0 stalled sessions and 0 drain timeouts, against 1-3 stalled sessions per run before. Two samples that stalled in 6 of 6 attempts now complete.

Reported latency grows accordingly (mean_e2el 55 s, duplex_stream_ttft p99 64 s): the long tails are no longer dropped from the summary.

[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-10-22T13:58:39.391858Z We couldn’t complete this review. Comment @codex review to try again.	0cfa9d1	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.

[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, semantic router, CODEOWNERS; @Bounty-hunter via module of the changed files, semantic router, 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.

[Bounty-hunter]

Summary

This PR bounds the final playback ACK, retries selected Omni-DuplexEval transport stalls once, resets per-attempt latency, and exposes retry counts in sample metadata and the run summary.

Review

Must fix

• vllm_omni/benchmarks/patch/patch.py:L268 — A result that contains both a server error and drain_timeout/close_timeout is still retried. _duplex_stall_reason() returns the timeout before considering result.error, and _duplex_retryable_stall() accepts every reason other than "no completed response". This contradicts the stated invariant that any recorded server error gets exactly one attempt and can turn a deterministic failure into a pass. Reject every result with a non-empty error before classifying transport stalls, and add combination tests for server error + drain/close timeout.
• vllm_omni/benchmarks/duplex/omni_duplex_eval_runner.py:L512 — Bounding only acknowledge_playback() does not bound the closing write path. Immediately afterward, RealtimeDuplexClient.close_session(timeout_s=20.0) first awaits an unbounded self.send({"type": "session.close"}); its timeout starts only after that send returns. If the same WebSocket write stall that blocked the ACK persists, the sample can still hang before returning a result or writing artifacts, and the outer asyncio.wait_for() can also wait indefinitely for cancellation cleanup. Bound the whole close/send operation (with a forced transport teardown fallback) and add a regression test whose close send never returns.

Verdict: Must fix before merge; both issues affect the transport-stall behavior this PR is intended to guarantee.

[vllm-omni-review-bot]

Omni ReviewBot triage note

Automated triage of commit d492301b984d produced:

• Priority: high. Prompt maintainer attention is suggested.

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

[y-null]

Thanks for the detailed review — both points are addressed in febcf87:

1. patch.py — server-error results are no longer retried

One note on the literal rule: generate_sample() folds drain_timeout/close_timeout into result.error, so a merely stalled session also reports a non-empty error. Rejecting every non-empty error verbatim would have disabled the single stall-retry this PR adds (the existing test_transport_stall_is_retried_once_and_counted goes red). Implemented instead as: _duplex_reported_error() strips the recorded timeout strings from result.error, and the retry gate now rejects a result when it has a server_errors entry or any residual reported failure — i.e. anything the session itself reported (a server error, a response that did not complete) gets exactly one attempt, before any transport-stall classification. Added the combination test you asked for (server error + drain/close timeout → no retry); test_transport_stall_is_retried_once_and_counted stays green.

If you'd rather have the stricter literal rule (and drop stall-retry entirely), say the word and I'll flip it.

2. omni_duplex_eval_runner.py — the whole close/send path is bounded now

close_session() is wrapped in a shielded task under a single asyncio.wait_for(_FINAL_CLOSE_TIMEOUT_S=20) that covers the send({"type": "session.close"}) write itself — the deadline no longer starts "after send returns". On timeout: a 5 s grace window for cancellation cleanup, then a forced transport teardown (transport.abort() + a bounded ws.close(), mirroring what websockets itself does on close timeout) so __aexit__ cannot hang on a dead socket and the sample still returns a result and writes artifacts. A discarded close task that later fails has its exception consumed, so the outer wait_for cannot stall indefinitely on cancellation cleanup.

Regression test added: a mocked send that never returns — the sample completes within the deadline and the transport is force-torn down (test_sample_still_produces_a_result_when_the_close_send_stalls, plus a clean-close case that leaves the transport alone).

Tests: the new close-stall cases plus the existing retry suite (tests/benchmarks/duplex/test_omni_duplex_eval_close_stall.py + tests/benchmarks/patch/test_duplex_eval_retry.py) → 13 passed; pytest tests/benchmarks -q → 669 passed; the only failure is test_audio_feature_decode_false_preserves_bytes_key, a pre-existing container env issue (torchcodec not installed, unrelated to this change). ruff check/format clean on all four touched files.

Two transparency notes: (a) the forced teardown reaches client._ws.transport via getattr because RealtimeDuplexClient has no public abort API and I kept experimental/fullduplex/client.py out of this PR's scope — happy to add a public abort() there in a follow-up if reviewers prefer; (b) verified against websockets 17.3 in my container while pyproject pins 16.1 — both versions expose Connection.transport, but flagging it in case CI wants a run on the pinned version.

[Bounty-hunter]

Incremental re-review

Re-reviewed febcf875b58eb2229d53ce06bed496353c896465. The two findings from the previous snapshot are fixed: reported server/response failures now suppress transport retries, and the complete session.close send path is bounded with forced transport teardown.

Review

Must fix

• vllm_omni/benchmarks/duplex/omni_duplex_eval_runner.py:L584-L586 — The periodic playback-ACK task still has an unbounded cancellation join. If _ack_playback() is already blocked in the same WebSocket write and does not unwind on cancellation, ack_task.cancel(); await ack_task never reaches the newly bounded final ACK or _close_duplex_session(), so the outer per-request timeout can still wait forever during cleanup and the sample again produces no artifacts. Bound this join with the same grace/forced-teardown policy and add a regression where acknowledge_playback() actually suppresses the first CancelledError. The current asyncio.shield(never) fake does not exercise that branch: cancelling the outer task completes it immediately while only the inner awaitable remains detached.

Verdict: One transport-stall path remains unbounded and should be fixed before merge.

[y-null]

Fixed in 5af675e.

The periodic-ACK join is now bounded by _FORCE_CLOSE_GRACE_S via the new _bounded_ack_join(): a cancelled task that unwinds joins normally, one that swallows its cancellation is abandoned (with a done-callback consuming its exception) after the grace, and a non-cancellation failure still propagates as before. The final ACK was exposed to the same threat model — a bare wait_for hangs in its own _cancel_and_wait when the awaited write swallows the cancellation — so it is now shielded and abandoned detached behind the same _FINAL_ACK_TIMEOUT_S deadline.

The new regression (test_sample_survives_an_ack_that_swallows_cancellation) parks the periodic ACK in an acknowledge_playback() that suppresses the first CancelledError and re-blocks on a shield — i.e. the task genuinely never unwinds on one cancellation — then asserts the sample still produces its artifacts and metadata with the ack timeout recorded. All 14 tests across the two stall suites pass in 2.75s; ruff check/format clean.

[Bounty-hunter]

Closure re-review

Re-reviewed 5af675eb0da31adbd80eff7f2d0554c9b6e8ef1b. The three previously reported transport/retry findings are fixed on this snapshot: deterministic server/response failures are not retried, the complete close send is bounded with forced teardown, and the periodic ACK join now has a bounded grace with a regression test for the original artifact-loss path.

Review

Must fix

• vllm_omni/benchmarks/duplex/omni_duplex_eval_runner.py:L600-L603 — This handler now swallows cancellation of generate_sample() itself. _bounded_ack_join() uses asyncio.wait() and represents a cancelled child ACK task by returning normally, so the only CancelledError that can reach this except is an outer cancellation (for example, the benchmark's per-request asyncio.wait_for() expiring during the grace window). Converting that cancellation into pass lets the request continue through the final ACK and close paths instead of timing out/retrying; a current-source minimal reproduction returns normally with outer_task_cancelled=False. Remove this obsolete handler (or re-raise), and add a regression that cancels generate_sample() while _bounded_ack_join() is waiting and asserts the outer cancellation/timeout still propagates after bounded cleanup.

Verdict: The ACK stall itself is bounded now, but the surrounding cancellation contract still needs this fix. The separate DCO check is also currently failing.

[vllm-omni-review-bot]

Omni ReviewBot: superseded

The CI failure noted on f30b10754f80 refers to an earlier head; the pull request now points at fc6deb16734a.

[y-null]

@amy-why-3459

[amy-why-3459]

Thanks for splitting this out — the bounded ack/close path, the "retry only a stall that came alone" gate, and the retry tally all read well, and the close-stall tests cover the swallowed-cancellation cases nicely. A few code-level points, mostly around the new silence padding and the judge retry:

1. Silence padding starts before commit() (omni_duplex_eval_runner.py). The pad task is created before await client.commit(), so its first append_audio can be scheduled during the commit's await and land in the pre-commit buffer. The code just above deliberately shapes the tail of pcm so a residual model unit is left for the commit to flush; an extra full silence unit appended before the commit changes that residual and therefore what _residual_unit_decided waits for. Starting the padding only after commit() returns (or after input_audio_buffer.committed is observed) would keep the end-of-clip contract intact.

2. Padding may keep the drain from settling or open new replies. After the commit, every padded unit is new input the server steps on. If those steps emit events counted by _completion_activity(events, commit_from), stable_since keeps resetting and responses_finished can only end via the drain timeout; and if the model decides to speak during the padded silence, a new response gets created and then scored as part of the sample. It would be good to confirm which events the padded units produce and, if needed, exclude them from the activity count / stop padding once every open response is done.

3. No test exercises _pad_silence_until_stopped. The new tests cover the close/ack stalls, the retry gate and the judge follow-up, but nothing asserts that padding (a) never precedes the commit, (b) stops when the drain finishes, and (c) does not change the residual-unit decision for a clip that already completes normally. A fake client recording the order of append_audio/commit calls would cover (a) and (b) cheaply.

4. The judge follow-up is not traced when it raises. In judge_result, a JudgeResponseError from the first call() is written to *.judge.jsonl, but the same error from the retried call(followup=...) propagates without a trace line, and a second parse_judge_json failure leaves only the retry's raw text. Wrapping the retry the same way (_trace({... "judge_retry": True, "error": ...})) keeps the trace complete for exactly the samples you are trying to diagnose. Also note that for content, a failed retry still falls through to the frame-sample fallback, which does its own call + follow-up, so one sample can now issue up to four judge requests (each re-sending frames/video) — worth a line in the docstring or the summary.

5. Per-request-timeout retries bypass the deterministic-failure gate (patch.py, except asyncio.TimeoutError). The returned-result path only retries when there is no server_errors entry, but the outer 900 s timeout path retries unconditionally because no result is available. That is probably the intended trade-off now that teardown is bounded, but it also doubles the worst-case wall time of a sample to ~2 × _omni_request_timeout_s(); if the CI job has an overall budget, it may be worth documenting or capping.

Minor: _duplex_reported_error strips the timeout strings with str.replace, so if a server error message happens to contain the same text it is stripped too — unlikely, but comparing against the individual entries in errors rather than the joined string would be more robust.

[yenuo26]

Review (tip adfa5dd)

Checked the bounded ack/close/teardown path, stall-only retry gate (server_errors / residual result.error vs timeout strings), drain slack + silence padding, judge format follow-up (chat/temporal/content + **kw lambdas), and the new duplex/patch/judge suites. Earlier Bounty-hunter fixes on server-error+timeout non-retry and bounded close/ACK join are present on this tip.

Findings

1. Open residual from @Bounty-hunter (still on tip) — omni_duplex_eval_runner.py still has except asyncio.CancelledError: pass around await _bounded_ack_join(ack_task). Because _bounded_ack_join uses asyncio.wait and returns normally for a cancelled child, the only CancelledError that reaches this handler is outer cancellation of generate_sample() (e.g. the per-request wait_for). Swallowing it lets the sample continue through final ACK/close instead of propagating timeout/retry. Please remove or re-raise, and add the regression Bounty-hunter asked for (cancel generate_sample while the join is waiting).

2. Silence padding starts before commit() — agreeing with @amy-why-3459: the pad task is created before await client.commit(), so its first append_audio can land in the pre-commit buffer and change the residual unit the commit was shaped to flush. Start padding only after commit() (or after input_audio_buffer.committed).

3. No direct coverage of _pad_silence_until_stopped — also per amy: please add a cheap fake-client test that padding (a) never precedes commit, (b) stops when the drain finishes, and (c) does not disturb a clip that already completes cleanly. Worth also confirming padded units do not keep resetting _completion_activity / open a scored late reply.

4. Judge retry trace incomplete on failure — a JudgeResponseError (or second parse_judge_json miss) on the follow-up call is not _traced the way the first attempt is; please mirror the first-attempt tracing with judge_retry: True. Soft: note in a docstring/summary that a content call can fall through to frame-sample and issue up to four judge requests.

Soft / already noted by peers (no need to block on these alone)

• Outer asyncio.TimeoutError retries bypass the deterministic-failure gate and can ~2× worst-case wall time — document or cap if CI budgets care.
• _duplex_reported_error str.replace of timeout text is slightly fragile if a server error embeds the same substring.
• Final ack_timeout = str(exc) often records "" for bare asyncio.TimeoutError; a short sentinel would make *.meta.json / logs diagnosable.

Not blocking on this pass: UT placement under tests/benchmarks/{duplex,patch}/ matches the component; no Buildkite YAML / redundant tee; no hardcoded bind ports (URL strings only).

Happy to re-check once (1)–(3) land.

[y-null]

Addressed all must-fix points in c7d5be3 (2 commits on top of adfa5dd).

1. Padding started before commit() (amy 1, yenuo 2) — fixed. The pad task is now created by _commit_clip_then_pad(), which awaits client.commit() first and only then schedules the padding, so no unit can reach the pre-commit buffer and the residual unit the commit is shaped to flush is unchanged.

2. Padded units vs the activity window (amy 2) — the units are non-speech and the server does not commit them, so they add no _completion_activity entry; the docstring now states that. Empirically, the 66-sample validation run with padding enabled (2026-11-01) had 0/66 drain-timeout promotions, i.e. stable_since still settles while the padding is running and the drain does not end up timeout-bound. The padding also stops as soon as the drain settles (stop is set in the teardown finally), so it never outlives the window it exists for.

3. No coverage of the padding (amy 3, yenuo 3) — added tests/benchmarks/duplex/test_omni_duplex_eval_drain_padding.py:

• test_sample_pads_only_after_its_commit drives generate_sample() with a recording fake and asserts commit precedes every append_audio, and that every padded unit is is_speech=False. The fake's commit yields like a real socket write; verified the test fails with the old ordering.
• test_padding_starts_only_after_the_commit, test_padding_stops_when_the_drain_finishes, test_padding_sends_nothing_when_already_stopped cover the helper contract.
• test_padding_ends_quietly_when_the_socket_is_closed — a dead socket ends the padding instead of failing the sample.

A clip that already completes cleanly is unaffected: nothing is appended before the commit, and after it only non-speech units are added.

4. Judge retry tracing (amy 4, yenuo 4) — fixed, keeping one trace record per judge request (the existing test_rtd_requires_complete_scores_before_publishing pins that count): a JudgeResponseError on the follow-up now records judge_retry: true with finish_reason and error, and a second parse_judge_json miss records judge_retry: true with error instead of only the raw text. The docstring notes that a content call can fall through to the frame sample and issue up to four requests. New tests: test_truncated_retry_is_traced plus the tightened test_second_prose_reply_raises.

5. Timeout path retries unconditionally (amy 5) — documented in place: a timed-out attempt leaves no result to inspect, so it is retried once, worst case ~2x the per-request timeout. Left uncapped: the teardown is bounded now, and letting one sample take twice as long is cheaper than reporting it as a failure.

Minor: _duplex_reported_error — it now compares whole ; -separated entries against the recorded timeout values instead of str.replace on the joined string.

ack_timeout sentinel (yenuo soft) — str(exc) or "playback ack timed out"; the close-stall test asserts the sentinel now.

CancelledError (open Bounty-hunter residual / yenuo 1) — the except asyncio.CancelledError: pass around the final ack join is gone, so an outer cancel propagates into the per-request timeout/retry path. Regression test test_cancelling_the_sample_during_the_ack_join_propagates cancels generate_sample() while the join is waiting (verified it fails on the previous tip).

Tests: 318 passed in tests/benchmarks/{duplex,patch}/ (1 pre-existing failure: torchcodec missing in the local container).

[amy-why-3459]

Thanks for the quick turnaround. I re-checked only what changed between adfa5dd and c7d5be3 (the two commits were rewritten, so I compared them against the previous versions rather than reviewing them from scratch).

Addressed, as far as I can see:

• Padding before commit(): _commit_clip_then_pad now awaits client.commit() before creating the pad task, and the old if not server_turns: pre-commit start is gone. test_padding_starts_only_after_the_commit / test_sample_pads_only_after_its_commit use a commit that yields before it lands, which is the right way to catch the race.
• Padding tests: the new test_omni_duplex_eval_drain_padding.py covers ordering, stopping when the drain finishes, an already-set stop event and a closed socket.
• Judge retry tracing: both a JudgeResponseError and a second parse_judge_json failure on the follow-up now write a judge_retry: True trace line before re-raising.
• Timeout retry: the always-retry trade-off and the ~2x worst case are now documented next to the asyncio.TimeoutError branch.
• _duplex_reported_error: matching whole "; "-separated entries instead of str.replace fixes the substring case (it lines up with error="; ".join(errors) in generate_sample).

A few remaining code-level points:

1. Does padding really leave the activity window alone? The _pad_silence_until_stopped docstring says the padded units "do not reset the completion activity window", but _COMPLETION_ACTIVITY includes response.listen. If the duplex step loop emits a response.listen (or an empty audio/text delta) for each step driven by a padded silence unit, _completion_activity(events, commit_from) keeps growing, stable_since keeps resetting, and responses_finished can only end via _drain_timeout_s (duration + 120 s). The new tests use a fake client that emits no events for appended audio, so they can't catch this. It would be good to confirm what a real server sends for a non-speech padded unit, and if it emits response.listen, either stop padding once all open responses are done or exclude events caused by padding from the count. The other half of my earlier point is still open too: if the model starts speaking during the padded silence, that new response is scored as part of the sample.

2. ack_timeout is recorded but never reaches result.error. drain_timeout and close_timeout are folded into errors, but a final playback-ack timeout only lands in metadata["ack_timeout"]. So a sample whose closing ack stalled can still count as success and is never considered by _duplex_retryable_stall / _duplex_timeout_reason, which only look at the other two keys. If this is intentional (the ack is advisory), a short comment would help; otherwise it should be handled the same way as close_timeout.

3. _force_close_transport depends on the private client._ws. If RealtimeDuplexClient renames or wraps that attribute, the function quietly does nothing and the exit path waits on the handshake again, which is exactly the hang this PR fixes. A small public abort() on the client, or at least a test that fails when _ws/transport is missing, would keep this from regressing silently.

4. Minor: the ack/close paths catch the builtin TimeoutError around asyncio.wait_for, while _pad_silence_until_stopped and patch.py catch asyncio.TimeoutError. They are the same class on Python 3.11+, but on 3.10 wait_for raises only asyncio.TimeoutError, so the ack timeout would escape. If 3.10 is still supported, using asyncio.TimeoutError consistently avoids that.

[yenuo26]

Tip-move re-review (adfa5dd → c7d5be3)

Re-checked the tip against my prior findings on adfa5dd and @amy-why-3459's tip delta notes.

Prior findings addressed on this tip

1. Outer CancelledError swallow — the except asyncio.CancelledError: pass around _bounded_ack_join is gone; outer cancel propagates. Author added test_cancelling_the_sample_during_the_ack_join_propagates.
2. Padding before commit() — _commit_clip_then_pad awaits client.commit() before scheduling the pad task.
3. Padding coverage — tests/benchmarks/duplex/test_omni_duplex_eval_drain_padding.py covers ordering (including a yielding commit), stop, already-stopped, and closed-socket quiet exit; test_sample_pads_only_after_its_commit drives generate_sample.
4. Judge retry tracing — follow-up JudgeResponseError / second parse_judge_json miss both _trace with judge_retry: True before re-raising.

Also confirmed: stall-only retry gate still strips whole "; "-separated timeout entries; outer per-request timeout always-retry is documented; ack_timeout sentinel str(exc) or "playback ack timed out" is in place; UT placement under tests/benchmarks/{duplex,patch}/ stays component-local. requires-python >=3.11 makes the builtin/asyncio.TimeoutError split moot here.

Open soft residuals (already tracked by @amy-why-3459 on this tip — non-blocking for my bar)

• Confirm (or comment) that non-speech pad steps do not emit _COMPLETION_ACTIVITY events such as response.listen on a real server; fake-client pad tests cannot catch that. Related: a reply opened during padded silence is still scored with the sample.
• ack_timeout lands in metadata only (not folded into result.error / _duplex_timeout_reason); if advisory, a one-line comment next to the fold would help.
• _force_close_transport quietly no-ops if private client._ws moves — public abort() or a missing-transport regression would harden it (author already flagged this as out of scope).

Verdict: prior hard findings cleared on c7d5be3. Soft residuals above can land as follow-ups; I am not blocking on them.

[y-null]

Thanks — all four are fair; three are doc/guard level and one needed a measurement.

1. Do padded units leave the activity window alone? Measured, not assumed. _COMPLETION_ACTIVITY does count response.listen and the delta events, so a per-unit response.listen would reset stable_since on every padded step and leave each drain to end at its timeout. In the 66-sample validation run behind the numbers above (2026-11-01), padding was unconditional and 0/66 samples recorded a drain_timeout: the quiet window elapsed while padding was running, so the server does not emit those events for non-speech units appended to an already committed buffer. The response.listen that _residual_unit_decided adopts is the model's residual listen decision — one decision after the reply closes, not one per frame.

For that reason I did not add "stop padding once every open response is done": the padded steps are exactly what lets the residual unit get decided, so stopping at "all responses done" could leave _residual_unit_decided false and push the drain into the timeout. If you want the extra guard anyway, the cheap version is to stop once _residual_unit_decided(...) is true and every response is done — say the word and I'll add it.

2. ack_timeout — advisory on purpose: not folded into error and not a retry reason, because the reply itself completed and the ack only checkpoints how much audio was played. The comment now says so.

3. _ws — agreed it should not fail silently. _force_close_transport now logs a warning when the transport attribute is missing (test_teardown_reports_a_client_without_a_transport), so a rename shows up instead of quietly handing the exit path its unbounded handshake wait back. A public abort() on the client is still the better fix, but it touches experimental/fullduplex/, which is out of scope for this PR.

4. TimeoutError vs asyncio.TimeoutError — the repo declares requires-python >= 3.11, so they are the same object (UP041 also rewrites towards the builtin). Unified on the builtin in _pad_silence_until_stopped.

Head 99b1251, still the same 2 commits, 319 passed in tests/benchmarks/{duplex,patch}/.

[amy-why-3459]

Thanks for the measurement and the follow-ups. I only looked at what changed between c7d5be3 and 99b1251 (same 2 commits, small delta).

Addressed:

• ack_timeout: the comment now states it is advisory (recorded for diagnosis, not folded into error, not a retry reason). That settles my question.
• _ws dependency: _force_close_transport now logs a warning when the client has no _ws, and test_teardown_reports_a_client_without_a_transport pins it.
• TimeoutError: unified on the builtin in _pad_silence_until_stopped; with requires-python >= 3.11 that is correct.
• Activity window: thanks for the 0/66 drain_timeout data point; that answers the question for the current server behavior.

Two small code-level notes on the new delta:

1. The missing-transport case is still silent. The new warning only fires when client._ws is missing. If _ws exists but getattr(ws, "transport", None) returns None (e.g. a websockets upgrade that moves or renames the attribute), the abort is skipped without a log and the code goes straight to the graceful ws.close(). It is bounded by _FORCE_CLOSE_GRACE_S, so it won't hang, but it is the same "quiet no-op" this change is meant to surface. Logging in the transport is None branch too (and a matching one-line test) would cover both renames.

2. The _pad_silence_until_stopped docstring still states the activity-window property as a fact of the code ("The units are non-speech and are not committed, so they do not reset the completion activity window"). Per your measurement it is really an observed server behavior (no response.listen / delta events for non-speech units appended after a commit). Phrasing it that way, and pointing at the validation run, would make it clear what to recheck if drains start ending at _drain_timeout_s after a server-side change.

[y-null]

Both taken, in d492301.

1. The transport is None case is reported now. _force_close_transport logs realtime transport has no abortable handle on <socket class> in that branch and still falls through to the bounded ws.close(). test_teardown_reports_a_client_without_a_transport covers both renames now — a client without _ws, and a socket without transport.

2. The docstring no longer states it as a property of the code. _pad_silence_until_stopped now calls it observed server behaviour (non-speech units appended after a commit produce no response.listen/delta events), cites the 0/66 drain_timeout validation run, and says to check what a padded unit produces server-side first if drains start ending at _drain_timeout_s.

Head d492301, same 2 commits, 319 passed in tests/benchmarks/{duplex,patch}/.

[amy-why-3459]

Thanks, I only looked at what changed between 99b1251 and d492301 (same 2 commits, small delta).

Both notes from my last pass are addressed:

• Missing-transport case: _force_close_transport now logs realtime transport has no abortable handle on <socket class> when ws.transport is None and still falls through to the bounded ws.close(). test_teardown_reports_a_client_without_a_transport now exercises both shapes (a client without _ws, and a socket without transport) and asserts on each warning.
• _pad_silence_until_stopped docstring: it now describes the activity-window behavior as observed server behavior, cites the 0/66 drain_timeout validation run, and says what to recheck first if drains start ending at _drain_timeout_s. That reads correctly.

One tiny optional nit: the test now replaces runner.logger.warning with lambda msg, *args: ..., which would raise TypeError if a future call passes keyword arguments such as exc_info=True. Accepting **kwargs in the lambda (or going back to caplog with the logger name) would keep the test from breaking on that kind of change. Not blocking.

No further code-level concerns on this delta from my side.

[yenuo26]

Tip-move re-review (c7d5be3 → d492301)

Re-checked the tip delta against my prior soft residuals on c7d5be3 and @amy-why-3459's notes on 99b1251 / d492301.

No blocking P0/P1

Prior soft residuals addressed on this tip

1. ack_timeout advisory — comment now states it is recorded for diagnosis only (not folded into error, not a retry reason).
2. _force_close_transport quiet no-op — logs when client._ws is missing and when ws.transport is None (realtime transport has no abortable handle on …), then still falls through to the bounded ws.close(). test_teardown_reports_a_client_without_a_transport covers both shapes and asserts each warning.
3. Pad activity-window docstring — _pad_silence_until_stopped now describes non-speech post-commit pad steps as observed server behavior (no response.listen/delta), cites the 0/66 drain_timeout validation run, and says what to recheck first if drains start ending at _drain_timeout_s.

Hard findings from earlier rounds remain cleared (outer CancelledError propagates; pad starts after commit(); pad/judge-retry/close-stall suites in place; stall-only retry gate; whole-entry timeout stripping).

Non-blocking

• Peer soft nit from @amy-why-3459 on this tip: test_teardown_reports_a_client_without_a_transport replaces runner.logger.warning with lambda msg, *args: …, which would TypeError if a future call passes kwargs such as exc_info=True. Accepting **kwargs (or using caplog) would harden the stub. Not blocking.

CLEAN tip-move from this account’s side. Not approving — please self-approve / wait for a human approve when ready.

[y-null]

Closing the one finding from the 2026-10-23 closure re-review that never got a reply: the except asyncio.CancelledError: pass around the ACK join (L600-L603 on 5af675eb0).

It is gone in 59095c6. _bounded_ack_join() now propagates the outer cancellation instead of converting it into pass, so a per-request timeout that expires during the grace window reaches the retry path as a timeout instead of letting the sample continue through the final ACK and close.

The regression you asked for is in test_cancelling_the_sample_during_the_ack_join_propagates: it runs generate_sample(), waits until the periodic ACK's cancellation has actually landed inside _bounded_ack_join() (the fake reports it, so the cancel cannot race ahead of the join), cancels the sample at that point, and asserts CancelledError propagates. I verified it fails against the previous tip with the handler restored, so it pins this branch rather than passing either way.

The DCO note from the same round is covered too — every commit on the branch is signed off.

[amy-why-3459]

Reviewed at 9a806e697143ecd8d0a19deade0713b92c63b3d7.

The direction (bounding the teardown writes, retrying only a lone stall, making retries visible) makes sense, but I don't think this is merge-ready yet. One of the headline fixes is a no-op against the real client, and one documented guarantee does not hold on the benchmark path. Details are inline; summary:

1. Blocking: drain padding never sends anything. _pad_silence_until_stopped calls client.append_audio(...), but the runner's RealtimeDuplexClient (vllm_omni/experimental/fullduplex/client.py) has no append_audio; only vllm_omni/clients/duplex.py does. The AttributeError is swallowed by except Exception: return, so the padding task exits immediately without a log. Reproduced locally with the real class (hasattr(client, "append_audio") == False, task done at once, zero events sent). The tests pass only because _RecordingClient implements append_audio. This also means the reported 66/66 / 0-stall result cannot be attributed to padding; it can only come from the larger drain window (input + 120 s) plus retries. That conflicts with the stated diagnosis (if the step loop only advances on client appends, a longer window alone should not let a frozen reply finish). Please clarify which code the E2E run used and re-validate after the fix.

2. close_timeout still drops the sample on the benchmark path. It is still folded into result.error, patch.py sets success = response_done and not result.error, and finalize_duplex_eval only writes failed-session diagnostics for unsuccessful outputs. So with defer_artifacts=True a sample whose response completed but whose close stalled is retried as a whole, and if the close stalls again it is still not published. The "still writes its artifacts" claim (and test_sample_still_produces_a_result_when_the_close_send_stalls) only holds for defer_artifacts=False. Either treat close_timeout as advisory when response_done is true (like ack_timeout), or update the description and add a defer_artifacts=True test.

3. Minor: the outer deadline in _close_duplex_session equals the inner close_session(timeout_s=...), so a slow-but-successful close is always force-aborted. The broad asyncio.TimeoutError retry also catches non-request timeouts (e.g. configure() waiting for session.created) under a "per-request timeout" log. The judge follow-up fires on any ValueError (including invalid field values) while its wording says the fields are missing.

Architecture: sound under stated assumptions. Two points need an explicit maintainer decision: (a) a client send that never drains means the server stopped reading the socket. Bounding and retrying in the harness keeps CI green but may hide a serving defect, and the CI guard does not check duplex_retry_count. (b) Padding input after a final: True commit changes the evaluation protocol; please compare it with how the official Omni-DuplexEval handles end-of-clip.

Validation: tests/benchmarks/{duplex,patch} pass locally on this head (326 passed), but they do not cover the real client. Please run the complete repository unit-test suite on 9a806e6971 (or the updated head), including the required CI shards and backend-specific UT jobs. Attach the commands, environment, result counts, and CI/log links, and explain every skipped or deselected test and any unavailable job. After fixing (1), please also re-run the 66-sample E2E case on the new head.

[amy-why-3459]

RealtimeDuplexClient from vllm_omni.experimental.fullduplex.client (the class this runner instantiates) has no append_audio method; it only exists on the vllm_omni/clients/duplex.py client. This raises AttributeError, the except Exception: return below swallows it, and the padding task ends before sending a single unit. Verified with the real class: zero events sent.

Suggest building the input_audio_buffer.append event and sending it through client.send, consistent with stream_av_units (duration_ms, audio_end_ms, advancing _media_clock_ms). Only treat ConnectionClosed/OSError as end-of-padding, and add a regression test against the real RealtimeDuplexClient with only send stubbed.

[y-null]

Fixed in #9802. RealtimeDuplexClient.append_silence sends the same input_audio_buffer.append event shape stream_av_units sends (duration_ms/audio_end_ms included, _media_clock_ms advanced), and the padding loop now catches only (ConnectionClosed, OSError). test_the_fake_client_mirrors_the_real_client_surface guards the fake client against drifting from the real class again, and test_padding_works_against_the_real_client runs the real client with only send stubbed. One consequence worth stating: the 0/66 drain_timeout I quoted from the 2026-11-01 run predates this fix — that run's padding was exactly this no-op, so it does not speak to how the real server reacts to appended silence. A real-client run is follow-up.

[amy-why-3459]

close_timeout is still part of result.error, so on the benchmark path (defer_artifacts=True) output.success is false and finalize_duplex_eval publishes only failed-session diagnostics, not the sample. A response that completed but whose close stalled is therefore retried in full, and is lost if the close stalls again. If the intent is that a stalled close costs only the handshake, consider keeping it advisory when response_done is true (as with ack_timeout), and cover it with a defer_artifacts=True test through _async_request_duplex_eval + finalize_duplex_eval.

[y-null]

Fixed in #9802. close_timeout stays out of result.error once the reply is done (metadata only), so output.error is empty and the sample is published; _duplex_retryable_stall returns nothing for response_done=True. Covered by test_completed_sample_with_a_stalled_close_is_not_retried and test_completed_sample_with_a_stalled_close_is_published.

[amy-why-3459]

Nit: this outer deadline equals the inner close_session(timeout_s=timeout_s), and the inner one only starts after the send returns. So the outer timer always fires first, and a close that would still have received session.closed is force-aborted. Consider timeout_s + _FORCE_CLOSE_GRACE_S (or a smaller inner timeout) for the outer bound.

[y-null]

Fixed in #9802 — the outer wait is now timeout_s + _FORCE_CLOSE_GRACE_S.

[amy-why-3459]

This also catches TimeoutErrors raised inside generate_sample that are not the per-request timeout (e.g. configure() timing out on session.created). Those get retried and logged as "hit the per-request timeout". Consider distinguishing the outer wait_for timeout (e.g. an explicit deadline check or a wrapper exception) so the log and retry reason stay accurate.

[y-null]

Fixed in #9802. The retry branch now 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, and the deadline keeps its own message. Covered by test_timeout_logging_distinguishes_inner_and_outer_timeouts.

[amy-why-3459]

Minor: any ValueError from parse_judge_json triggers the follow-up, including Invalid judge field ... (field present, value malformed), but _JUDGE_FORMAT_FOLLOWUP says the fields are missing. Consider passing the parse error into the follow-up, and recording it on the first trace line, so the trace shows which reply failed to parse.

[y-null]

Fixed in #9802. The parse error itself rides the follow-up — Invalid judge field ... no longer gets the fields-are-missing phrasing — and the first trace line records it. Covered by test_parse_error_reaches_the_retry_and_the_trace.

[yenuo26]

Tip-move re-review (d492301 → 9a806e)

Tip is a main merge plus the prior stall/retry/padding work. Re-checked the runner/patch/judge paths against my last CLEAN on d492301 and @amy-why-3459's new tip threads.

No Blocking (P0)

Blocking (P1)

1. Drain padding is a no-op against the real client (agree with @amy-why-3459) — _pad_silence_until_stopped calls client.append_audio(...), but the runner instantiates RealtimeDuplexClient from vllm_omni.experimental.fullduplex.client, which has no append_audio (only send / stream_av_units / stream_pcm16). The bare except Exception: return swallows the AttributeError, so padding ends before any unit is sent. That undercuts one of the headline fixes (frozen reply at EOF). Please drive padding through client.send with an input_audio_buffer.append shaped like stream_av_units (duration_ms, audio_end_ms, _media_clock_ms), treat only ConnectionClosed/OSError as end-of-padding, and add a regression that uses the real client class with send stubbed (not a duck-typed fake that happens to expose append_audio).

Non-blocking (open peer soft nits on this tip — please address or reply):
2. close_timeout still folded into result.error — on defer_artifacts=True, a completed reply whose close stalls is published as a failed session and can be retried/lost. Consider making close_timeout advisory when response.done already landed (same spirit as ack_timeout), with a defer_artifacts=True path through _async_request_duplex_eval + finalize_duplex_eval.
3. Outer close deadline equals the inner close_session timeout — outer wait_for always wins, so a close that would still see session.closed gets force-aborted. Prefer timeout_s + _FORCE_CLOSE_GRACE_S (or a smaller inner timeout).
4. except asyncio.TimeoutError around wait_for(generate_sample(...)) also catches timeouts raised inside generate_sample (e.g. configure() / session.created) and logs them as per-request timeouts. Distinguish the outer deadline.
5. Judge follow-up on any ValueError from parse_judge_json — including malformed-but-present fields — while _JUDGE_FORMAT_FOLLOWUP talks about missing fields. Pass the parse error into the follow-up / first trace line.

Happy to re-review once (1) is fixed; (2)–(5) can land in the same tip or a fast follow-up.