fix(moq-audio): give the publish-then-read tests an age budget - #3441
Merged
Conversation
Four tests have been failing on dev since the last main merge: two assert against shed packets, and two block forever on groups that were skipped before they could be read. The tests came from #3386, written on main where the decode budget was `latency_max: Option<Duration>` and defaulted to `None`, meaning never shed. dev had already replaced that with `max_age: Duration`, whose default is `ZERO` and skips aggressively. The merge reconciled the two APIs and adapted three of the new tests, but missed four more. Each of them publishes a whole track up front and reads it back afterwards, so every packet but the last is already older than a zero budget allows and gets dropped. `a_missing_packet_leaves_a_hole` saw only the second of its two packets, `resampling_does_not_shift_the_first_pts` read a PTS 0.96s into the second of audio it had just written, and the two Opus tests waited on frames that no longer existed. Give those sites the same one-second budget the adapted tests already carry. The default is right for playback; these tests just aren't playing back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
kixelated
enabled auto-merge (squash)
September 5, 2026 06:39
kixelated
disabled auto-merge
September 5, 2026 06:44
kixelated
added a commit
that referenced
this pull request
Sep 5, 2026
dev fixed the moq-audio publish-then-read tests itself (#3441), so its version replaces the copy carried here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
devhas been red since the lastmainmerge (ac69338ad): fourmoq-audiotests fail, two by assertion and two by hanging until the nextest timeout. This blocks everydev-based PR, which is how I found it (while landing #3418).Root cause
The tests arrived with #3386, written against
main, where the decode budget waslatency_max: Option<Duration>and defaulted toNone, meaning never shed.devhad already replaced that withmax_age: Duration, whoseDefaultisDuration::ZEROand, per its own doc comment, "skips aggressively". The merge reconciled the two APIs and adapted three of #3386's tests by addingmax_age: Duration::from_secs(1), but four more sites kept the default.Every one of those four publishes an entire track before it reads anything back, so by the time the consumer looks, all but the newest packet is older than a zero budget permits and gets shed:
a_missing_packet_leaves_a_holesaw only the second of its two packets: 2 frames stamped fromstamps[1]instead of the expected 4.resampling_does_not_shift_the_first_ptsread a first PTS of 1_960_000 instead of 1_000_000, i.e. 0.96 s into the one second of audio it had just written.a_lost_opus_packet_shorter_than_its_neighbour_is_a_gapandopus_pre_skip_does_not_leave_a_timestamp_holewrite each packet as its own group, so theirread()waits forever on groups that were skipped. That is the CI timeout.Nothing is wrong with the shedding default or with the decode logic these tests cover. The tests simply aren't playing back, so they should not be subject to a playback budget.
Fix
Give the four sites the same one-second budget the three already-adapted tests carry:
pcm_gapshelper indecode/consumer.rs, coveringa_missing_packet_leaves_a_holeandmillisecond_stamps_are_not_a_gapa_lost_opus_packet_shorter_than_its_neighbour_is_a_gapandopus_pre_skip_does_not_leave_a_timestamp_holeresampling_does_not_shift_the_first_ptsTest-only; no production code changes.
Test plan
cargo test -p moq-audio: 173 passed, 0 failed, 1 ignored (plus the doc/integration targets), against 172 passed / 1 failed / 2 hung before.cargo test -p moq-audio --lib decode::consumer: 11 passed, previously 2 failed and 2 hung past 60 s.origin/devwith no changes applied, so this is the base and not any one PR.just fixclean.Cross-Package Sync
No row applies: this touches only
moq-audio's own tests. No wire, catalog, FFI, config, or CLI surface moves, so nodrafts/ordoc/update is due.🤖 Generated with Claude Code
(Written by claude-opus-5[1m])