You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(mux): a reset track's arrivals drop the hold behind the old tail - #8513
Nightly's Audio quality job fails just test push-to-talk with native: an utterance did not resume on its head (runs 43041909115 and 42842827718), with 8 native underruns and 8 stalls where it used to have none. It is a regression from moq-dev/moq#8362, not a flake: it reproduces every time locally, the parent commit passes, and that PR's commit fails.
Each back-to-back press resumes one delay after the old tail drains, so the replacement segment's held grows by about 250 ms per press. Once the queue behind it passes the age budget, the consumer evicts media, the hole is too large to fill, and play_audio resets the sink and calls Track::reset. The reset clears the anchor but kept held, so the next arrival anchored the clock that much late. Since #8362, media due past the sink's cap waits in play_audio until it is due, so the player sat out about 1 s while a fresh 300 ms sink drained. It stalled, evicted again, and repeated, losing the heads of the following presses. Before #8362 that media was written right away, which hid the stale hold.
The comfort numbers in the report (median about 32 s, max about 60 s) are correct: they are milliseconds of comfort noise across the 5 s and 60 s mutes.
Approach
Track::reset zeroes the segment's held. Media after the break was not held behind any tail, so its arrivals are the live edge again. A unit test in sync.rs covers it and fails without the fix.
Validation
cargo test -p moq-mux --lib sync: 61 passed, including a_reset_drops_the_hold_behind_a_tail, which fails without the fix.
just test push-to-talk on head ccf28b0, on a heavily loaded host: passes. Every utterance resumes on its head (native: 1 underrun, 6 stalls).
A/B on the same host, with only the one-line fix removed: fails with native: an utterance did not resume on its head and 8 native underruns, which matches the Nightly failure.
An earlier unloaded run by the starter session showed native 0 underruns and 0 stalls.
Caused by
moq-dev/moq#8362 - media due past the sink's cap now waits in play_audio, which exposed the stale held that Track::reset kept
Impact
No public API or wire change. moq_mux::sync::Track::reset now also drops the hold behind a tail. Players that reset after a loss re-anchor at the live edge instead of up to the accumulated hold later.
Back-to-back push-to-talk presses keep their latency [S] - each back-to-back press adds about 250 ms because a resume with no gap still re-buffers one delay. Maintainer pick: ✅ keep as an [S] quest (rejected: drop it). Its Plan recommends continuing the old schedule when no gap separates the presses; it has no open Maintainer: decisions.
Models
Size: S
Implementer: Claude Code, claude-opus-5-5, unknown effort
Implementer: Claude Code, claude-opus-5-5, low effort (iterate, S)
Score: 9/10, The only change since the previous review adds a planning document, leaving the reviewed implementation unchanged and introducing no actionable defects; builds and tests were not run as instructed.
A replacement segment held behind the old tail adds that hold to each
arrival, so it does not drag the schedule ahead. A reset (a hole too large
to fill, or a rewind) kept the hold, so the clock re-anchored that much late
and moq play waited out media that was already live, draining the sink.
Back-to-back push-to-talk presses accumulate a hold, so after the first
eviction the native player stalled repeatedly and lost utterance heads.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No actionable comments were generated in the recent review.
Walkthrough
The quest document describes a plan for keeping push-to-talk delay flat across back-to-back presses and re-buffering after a real gap. In rs/moq-mux/src/sync.rs, Track.reset now clears the segment’s accumulated hold time before resetting the role timeline. A Tokio test checks that the next mapped arrival is due one default delay after arrival.
Merge Risk: Minimal · up to 778fc
The change clears stale hold time on reset so new arrivals re-anchor at the live edge, and it adds a regression test. No actionable merge-blocking risk was found.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the main change: Track::reset drops the hold a track carried behind the old tail, which the PR body and objectives describe as the fix for the stalled push-to-talk utterances. It is sp…
Description check
The description explains the problem (a push-to-talk regression from #8362 causing underruns and stalls), the approach (Track::reset zeroes held, plus a unit test in sync.rs), validation ru…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names the main change: Track::reset drops the hold a track carried behind the old tail, which the PR body and objectives describe as the fix for the stalled push-to-talk utterances. It is specific and matches the changeset (sync.rs reset clears held before re-anchoring the clock). It is a single sentence under 70 characters.
Full details: Description check
Explanation
The description explains the problem (a push-to-talk regression from #8362 causing underruns and stalls), the approach (Track::reset zeroes held, plus a unit test in sync.rs), validation runs, impact, and a follow-up quest. The quest file added in the raw summary matches the follow-up it references. It is clearly related to the changeset.
Finishing Touches
Simplify code
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: d2d7661d-1b20-4dc5-8b66-f7a54a543012
Commits
Reviewing files that changed from the base of the PR and between 7c2b816 and 778fc1b.
Files selected for processing: 2
quest/m1/push-to-talk-resume-latency.md
rs/moq-mux/src/sync.rs
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
✅ No new issues found.
Reviewed changes — the full diff (2 files, 2 commits).
Track::reset zeroes held — rs/moq-mux/src/sync.rs now clears the segment's hold, so arrivals after a break anchor at the live edge instead of that much late. The mapping lock is taken and released before the clock state lock, so there is no new lock nesting.
Regression test a_reset_drops_the_hold_behind_a_tail — builds a held segment behind a tail, resets, and asserts the next arrival is due one DELAY after it arrives.
Quest doc quest/m1/push-to-talk-resume-latency.md — plans the follow-up for the per-press growth. It is documentation only.
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
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.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Problem
Nightly's Audio quality job fails
just test push-to-talkwithnative: an utterance did not resume on its head(runs 43041909115 and 42842827718), with 8 native underruns and 8 stalls where it used to have none. It is a regression from moq-dev/moq#8362, not a flake: it reproduces every time locally, the parent commit passes, and that PR's commit fails.Each back-to-back press resumes one delay after the old tail drains, so the replacement segment's
heldgrows by about 250 ms per press. Once the queue behind it passes the age budget, the consumer evicts media, the hole is too large to fill, andplay_audioresets the sink and callsTrack::reset. The reset clears the anchor but keptheld, so the next arrival anchored the clock that much late. Since #8362, media due past the sink's cap waits inplay_audiountil it is due, so the player sat out about 1 s while a fresh 300 ms sink drained. It stalled, evicted again, and repeated, losing the heads of the following presses. Before #8362 that media was written right away, which hid the stale hold.The comfort numbers in the report (median about 32 s, max about 60 s) are correct: they are milliseconds of comfort noise across the 5 s and 60 s mutes.
Approach
Track::resetzeroes the segment'sheld. Media after the break was not held behind any tail, so its arrivals are the live edge again. A unit test insync.rscovers it and fails without the fix.Validation
cargo test -p moq-mux --lib sync: 61 passed, includinga_reset_drops_the_hold_behind_a_tail, which fails without the fix.just test push-to-talkon headccf28b0, on a heavily loaded host: passes. Every utterance resumes on its head (native: 1 underrun, 6 stalls).native: an utterance did not resume on its headand 8 native underruns, which matches the Nightly failure.Caused by
play_audio, which exposed the staleheldthatTrack::resetkeptImpact
moq_mux::sync::Track::resetnow also drops the hold behind a tail. Players that reset after a loss re-anchor at the live edge instead of up to the accumulated hold later.Alternatives
Follow-ups
Maintainer:decisions.Models
Size: S
Implementer: Claude Code, claude-opus-5-5, unknown effort
Implementer: Claude Code, claude-opus-5-5, low effort (iterate, S)
(Written by Claude Opus 5.5)
Review
Rewritten by
quest reviewafter each round.Head: 778fc1b
Base: origin/main at 36931c3
Reviewer: Codex, gpt-6.1-sol, medium effort
Session:
01a1d154-072b-7382-80b4-752a7c8275ac, round 2Score: 9/10,
The only change since the previous review adds a planning document, leaving the reviewed implementation unchanged and introducing no actionable defects; builds and tests were not run as instructed.Findings: none