[psv666]

Contributor intent

1. What outcome do you want to achieve?

Fix #8892 so Qwen3-Omni audio-in-video tests receive valid audio-bearing video fixtures.

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

Whether generated and cached test videos actually contain a decodable audio stream.

3. How will this work move that variable?

Propagate audio generation/muxing failures and regenerate cached videos missing the requested audio stream.

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

Qwen3-Omni users and CI maintainers need audio-in-video requests and their test coverage to work reliably.

Purpose

Refs #8892.

When TTS generation or FFmpeg muxing failed, generate_synthetic_video(..., embed_audio=True) silently cached a video-only MP4. Reusing that entry after repairing the dependency reproduced PyAV's No audio stream found. and TorchCodec's unknown-best-audio-stream error.

Generation/muxing failures now propagate before cache publication. Cache reads regenerate missing-audio, corrupt, or incomplete MP4s: requested audio is decoded and checked against its declared sample duration, and video must decode to the requested frame count. Completed bytes are closed in a unique same-directory staging file, then published with os.replace. Exclusive creation preserves normal 0666 & ~umask permissions; failed replacement preserves the old entry and removes the staging file. Valid audio-bearing and video-only caches remain reusable.

Regressions cover dependency failures, legacy video-only cache repair, real AAC decoding, valid reuse, empty/invalid media with and without audio, independently truncated audio/video payloads, atomic publication, failed-publication cleanup, and shared/restrictive permission masks. CPU tests substitute only TTS; MP4/AAC encoding and decoding are real. The helper docstring documents the behavior; no runtime API, configuration, or active production design contract changes.

The media regression suite in tests/helpers/tests/test_synthetic_video.py also protects video frame-count validation. It isolates cached-video frame-count validation in both audio modes: the video-only case retains a physical payload truncation; the with-audio case puts a fully encoded five-frame MP4 under the six-frame cache key and explicitly verifies complete audio before invoking cache repair. The renamed incomplete-video test checks both preconditions and the restored six-frame result. This addresses test isolation, with no newly demonstrated runtime defect. tests/helpers/media.py is unchanged from f7b36f8f (and byte-identical to a385408e7).

Main has been integrated through ad6fc16f5, including the upstream Duplex CI repair in #9151. The overlap with this branch's earlier CI repair was resolved by retaining the upstream Duplex test unchanged. The final PR diff against main contains only tests/helpers/media.py and tests/helpers/tests/test_synthetic_video.py; it has no Duplex, MiniCPM E2E, or kernel delta.

Test Plan

Current revision: a7f51ee85488cabd21d76bf9899dd8439de7c7ab; merge parents ac6c5fd8e and main ad6fc16f5. Final comparison base: ad6fc16f5. The two media files are byte-identical to their validated 4d2214126 versions.

Current CPU environment: Python 3.12.13; vLLM 0.32.0; PyTorch 2.13.0+cu130; PyAV 18.0.0; imageio 2.37.4; imageio-ffmpeg 0.6.0; NumPy 2.3.5; pytest 9.1.1. Development/media dependencies and FFmpeg are required; the CPU regression suite does not require a host TTS installation or model weights. Commands below run from the checkout and omit this host's GPU reservation wrapper.

# Validate merge preservation and the media regressions together
uv run --no-sync pytest tests/engine/duplex tests/engine/test_duplex_orchestrator.py \
  tests/helpers/tests/test_media.py tests/helpers/tests/test_media_pydub_removal.py \
  tests/helpers/tests/test_synthetic_video.py \
  -m 'core_model and cpu' --run-level core_model -o addopts='' -q

uv run --no-sync pytest tests/helpers/tests/test_synthetic_video.py \
  -m 'core_model and cpu' --run-level core_model -o addopts='' -q

uv run --no-sync pytest \
  tests/helpers/tests/test_media.py \
  tests/helpers/tests/test_media_pydub_removal.py \
  tests/helpers/tests/test_synthetic_video.py \
  -m 'core_model and cpu' --run-level core_model -o addopts='' -q

uv run --no-sync pre-commit run --files \
  tests/helpers/media.py tests/helpers/tests/test_synthetic_video.py

Test Result

• At a7f51ee85: 1367 related Duplex/orchestrator/media CPU tests pass, including all 51 media cases and both retained-PCM cases from the upstream conflict file. The conflict file equals [CI] Fix MiniCPM-o duplex tests broken after #8971 #9151 byte-for-byte; the final two-file PR diff preserves the existing media implementation and tests unchanged.
• Counterfactual checks against the adopted upstream test still catch its original defects: moving item-ID stamping onto a copy after retain fails the with-ID case (1 failed, 1 passed); independently removing consumed-audio cleanup fails both cases (2 failed). Each mutation ran in a separate checkout, leaving the committed merge unchanged.
• Full author precheck-pr, applicable local pre-commit hooks, and a separate fresh-context review-pr completed without actionable findings. The independent reviewer ran 21 targeted cases on its own frozen final snapshot, verified imported source identities, and checked unchanged fingerprints before/after validation. Matching-head Buildkite validation remains separate from local CPU evidence.
• Incoming main's MiniCPM E2E and Qwen-Image files are retained exactly. No new model-serving or kernel result is claimed by this merge validation.

Earlier media validation (actual revisions retained):

• At 4d2214126: 19 fixture regressions pass within 51 related media tests. Full precheck-pr author self-check and a separate fresh-context reviewer executing review-pr on the frozen final commit completed; no actionable findings. The independent reviewer ran the 51-test media group in its own detached snapshot and verified unchanged source/index fingerprints.
• Reproduced the review concern at 01893cc0: adding the missing audio-completeness precondition failed for the with-audio fixture (5120 decoded samples versus 9216 declared), while video-only passed. The old sample could be rejected before video frame-count validation. This establishes a fixture-isolation gap, not a runtime regression.
• Mutation validation against the final test bytes: removing only the frame-count condition makes both incomplete-video cases fail (2 failed, 17 passed); the same mutation against 01893cc0 failed only video-only (1 failed, 18 passed). Disabling only audio sample completeness still fails the separate truncated-audio regression (1 failed, 18 passed). Bypassing cached-video RuntimeError recovery still fails the two video-only empty/invalid-header cases (2 failed, 17 passed). Each mutation was applied independently in an isolated checkout; the unmodified helper passes all 19 cases.
• All applicable local pre-commit hooks and git diff --check pass. No serving or model run was repeated for this test-only follow-up; the earlier results below retain their actual revisions and environments.

Earlier fixture/model validation:

• Original audio-failure regressions: 5 failed, 2 passed before the original fix; 7 passed afterward. Corruption/atomic-publication cases added later: 5 failed, 7 passed before their fix. Permission tests reproduced 2 failures before the permission correction. Disabling only audio sample completeness makes the independent audio-tail regression fail.
• At a385408e7: 47 media tests and 8/8 real-TTS/AAC repair/cache checks passed (128/300 frames, PyAV and TorchCodec). Environment: vLLM 0.31.0, PyTorch 2.13.0+cu129, TorchCodec 0.15.0, PyAV 18.0.0, torchaudio 2.11.0+cu129, pyttsx3 2.99.
• At 7408f57d, on two NVIDIA L20X GPUs per run: Qwen3 Instruct audio-in-video E2E 6 passed across default/async-chunk configurations. Qwen2.5 boundaries 2 passed with explicit stage KV budgets of 8 GiB/2 GiB after the unmodified deployment OOMed in startup warmup before any request. Thinking's original case generated valid MP4/AAC, then failed HTTP 400 because the client defaults to text+audio output while Thinking supports text only. Supplementary validation with explicit text output passed both the original video request and a use_audio_in_video=True request. This does not turn the original Thinking case into a pass. Earlier author validation summary.
• The issue reporter independently tested a385408e7 on two RTX PRO 6000 Blackwell GPUs with TorchCodec 0.17.0: Qwen3 online 11 passed, 14 skipped; online expansion 26 passed; offline 4 passed; fixture tests 15 passed; Qwen2.5 boundaries 2 passed. They reported the Thinking failure as expected. Reporter validation and full environment.

The initial dependency failure in the reporter's environment remains unidentified. TTS/FFmpeg are still required for real audio generation; the helper exposes their failures instead of caching misleading input.

AI assistance: Codex assisted with diagnosis, the fix, regression tests, validation, author self-check, independent agent review, and writing.

[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-19T02:13:20.098453Z	bde3834	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 matches CODEOWNERS paths: /tests/.

Code owners: @NickCao @yenuo26

Routing: @NickCao via CODEOWNERS; @yenuo26 via CODEOWNERS

@psv666, 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]

Omni ReviewBot triage note

Automated triage of commit ac6c5fd8e922 produced:

• Priority: high. Prompt maintainer attention is suggested.

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

[psv666]

@yenuo26 @amy-why-3459 @natureofnature PTAL

[natureofnature]

Reviewed both changed files and the audio-in-video callers. TTS/mux failures now escape before the MP4 cache write, and legacy video-only entries are regenerated without changing valid-cache reuse. The reported 39 passing media tests on this head cover the fixture change, including real AAC decoding; the pre-rebase TTS and serving runs provide additional coverage. I haven't rerun tests locally.

【Conclusion: Found no blocking issues in commit bde3834.】
【Action required: None.】

[amy-why-3459]

Thanks, failing loudly instead of caching a video-only MP4 under the ea1 key makes sense, and the regression tests cover the main failure modes well.

One robustness point inline on the cache check. Also note that embed_audio=True is used by test_qwen2_5_omni_expansion.py and test_qwen3_omni_thinking.py as well as test_qwen3_omni_expansion.py, so those will now hard-fail on a TTS/FFmpeg problem too. It would be good to confirm their audio-in-video cases still pass, since the test plan only lists the Qwen3-Omni expansion run.

[amy-why-3459]

Thanks for the follow-up. Looked at 7408f57 only: treating probe/decode errors and short frame counts as a cache miss, plus the same-directory temp file and os.replace, address the partial-write case. Two small points inline, one on the cache file permissions and one on what the truncated-audio test actually exercises.

[psv666]

Author self-review completed at a385408: full precheck-pr plus a separate read-only review-pr pass on a detached snapshot, including affected callers and simplification. The final source fingerprint stayed unchanged across 47 passing media tests. Final real TTS/AAC repair and cache-reuse checks also passed 8/8 through PyAV and TorchCodec. The PR body has the reproduction commands and exact revisions.

Additional caller verification at 7408f57 (before the final permission/test-only follow-up):

• Qwen2.5 audio-in-video boundaries: 2 passed, streaming and non-streaming. The default startup first OOMed before any request; the successful run used explicit 8 GiB/2 GiB stage KV budgets.
• Qwen3 Instruct: 6 passed across default/async-chunk configurations.
• Thinking: the unmodified case generated its MP4/AAC successfully, then failed HTTP 400 because the unchanged client defaults to text+audio output while this model supports text only. Supplementary validation with explicit text output passed both the original video request and a request with use_audio_in_video=True. The original case remains reported as failed; fixture generation and the supported embedded-audio path were verified.

All three inline concerns are addressed with regression evidence, including umask handling and the isolated audio-tail mutation check.

AI assistance: Codex assisted with the fixes, tests, validation, self-review, and this response.

[amy-why-3459]

Looked at a385408 only. Both points are addressed. Path.open("xb") goes through os.open(O_CREAT|O_EXCL, 0o666), so the published cache now gets 0666 & ~umask like the old write_bytes, and the uuid name keeps concurrent writers from colliding before os.replace. The umask-parametrized test pins that. The corruption case now cuts at an audio packet past the last video packet and asserts six decodable frames first, so it really exercises the sample-count check. No further comments from me on this delta.

[amy-why-3459]

@yenuo26 PTAL

[yenuo26]

Reviewed f7b36f8f statically (both files plus the embed_audio=True callers). Failing loudly before publication and the PyAV sample-count gate address #8892's video-only ea1 cache; CI images ship espeak-ng, pyttsx3, av and imageio[ffmpeg], so removing the silent fallback should not break CI on missing deps.

One non-blocking test gap inline: the new video-frame-count / decode-failure branch is not exercised by any test. I haven't run the tests locally.

[amy-why-3459]

Looked at 01893cc only (skipped the merge from main). The added video-only cases are useful: empty and invalid-header now go through the real _np_array_from_mp4_bytes failure path, and the truncated video-only case is the one that actually exercises the len(cached_frames) == num_frames guard. One note inline on the with-audio truncated case.

[amy-why-3459]

Looked at 4d22141 only (test-only change). Swapping in a complete five-frame MP4 under the six-frame cache key for the with-audio case addresses my earlier note: the added precondition (samples >= duration * time_base * sample_rate) matches the runtime gate in media.py (integer samples vs. the ceil there are equivalent), so that parametrization now has to reach the len(cached_frames) == num_frames check. The video-only case still exercises physical truncation. No further comments on this delta.

[yenuo26]

LGTM

[vllm-omni-review-bot]

Omni ReviewBot: superseded

The CI failure noted on 847331e3f972 refers to an earlier head; the pull request now points at ac6c5fd8e922.

[psv666]

The Buildkite #17634 failure is inherited from main and is unrelated to this PR's synthetic-video/cache changes.

Both failing parameterizations are tests/engine/duplex/test_auto_response_commit_consume.py::test_auto_response_commit_consumes_retained_pcm, reporting AttributeError: 'types.SimpleNamespace' object has no attribute '_reserve_commit_prefixes'. At the failed revision 847331e3f, the test and production runner are byte-for-byte identical to main parent 7d7b8eabb. #8828 added the test; #8971 subsequently added the commit-prefix helper calls. Its NVIDIA CI #17623 passed on c32b7872, which did not yet contain that new test. This is an integration gap between the two changes; the originating PR did run CI.

Added the narrow test-harness compatibility fix in ac6c5fd8e to unblock this branch: use real bound DuplexSessionRunner commit helpers and the real AURA buffer instead of the incomplete runner/buffer/reservation fakes. The model-submission seam and original assertions for retained identity, [[1.0], [2.0]], and cleared consumed state are preserved. No production Duplex or media code changes in this follow-up.

Local validation at ac6c5fd8e: 2 failed before, 2 passed after; 1367 related Duplex/orchestrator/media CPU cases pass, including all 51 existing media cases. Restoring the original copy-after-retain bug still fails the with-ID case; removing consumed-audio cleanup still fails both cases. Full author precheck and applicable pre-commit hooks pass; a separate fresh-context reviewer ran 41 targeted tests and completed review-pr without actionable findings. Matching-head Buildkite validation remains separate from these local results.

AI assistance: Codex assisted with diagnosis, the test-harness repair, validation, independent agent review, and writing.

[amy-why-3459]

Looked at the delta since my last pass (ac6c5fd8, the duplex test-harness fix; the 847331e3 merge-from-main is skipped). Switching to the real AURA buffer and the real bound commit helpers is a better seam than the old _Buffer/_Reservation fakes, and the original assertions are preserved.

Two notes:

• Since this test is red on main itself (the #8828 / #8971 integration gap you described), it would be good to land the same fix on main as well, or to track it there; otherwise other open PRs stay red until they pick it up, and a different fix on main would conflict with this one.
• One inline note on how much of the commit path the __new__-built runner actually covers.