[t0ms]

Problem

Since #4001, moq export ts delivers a fraction of a live feed under random packet loss (#4613). The hold isn't the problem on its own. rewind() renews it.

Under loss a source now and then skips a group, and skipping over a hole bumps its discontinuity(). The exporter takes that as a new generation and calls rewind(), which resets stall, refreshes the held frame's arrived and clears every track's timeline. The sparse data PIDs, and video while it waits on a retransmit, can't show after that, so the next frame is held for the full max_age again. That's the same budget the sources skip on. After the hold every source sits a budget behind the newest content, so its next stall skips at once, which is another rewind and another full hold.

With logging on pick_next_track and the consumer's skip paths, a 120 s run at 10% loss showed 56 holds. 2 ended with the lagging track catching up, and a rewind restarted the rest at the full 2 s. The sources fell behind the newest content steadily, to 25 s after about 35 s and 105 s by the end.

Approach

• rewind() no longer resets stall or refreshes a held frame's arrived. A stall still ends only once every track has shown where its next frame lands. A rewind neither ends nor renews it.
• A generation that starts with no stall running still waits for every track, because rewind() still clears each timeline. The change is to a rewind taken while a track is already being muxed around: output keeps going around it until it catches up, as it already does within a generation.
• resume() clears the stall and its timer before it rewinds: a replacement broadcast gets its own budget.
• A frame held across a rewind keeps its arrived, which is its pull time, even when no stall is running. So a leftover pending frame can start a stall that is already partly spent, as it can within a generation.
• The stall and pick_next_track docs say so.

Testing

• rewind_restarts_the_stall and rewind_restarts_a_held_frame asserted the renewal. They become rewind_keeps_an_expired_stall and rewind_keeps_a_held_frame_waiting_time. repeated_rewinds_hold_once runs eight rewinds behind a quiet track and expects each generation out without a fresh budget. All three fail on main. rewind_keeps_a_partial_stall pins a rewind part way through a hold, and resume_does_not_inherit_an_expired_stall covers a two-track drop and resume (it fails without the resume() change).
• cargo test -p moq-mux: 929 pass. just check passes on the host toolchain (MOQ_ALLOW_HOST=1).
• Live, on the rig from moq export ts loses most of the feed under random packet loss since #4001 #4613: two network namespaces on one host, 10% netem loss plus 25 ms each way on the relay → subscriber leg, a real ~10 Mb/s broadcast TS paced with tsp -P regulate, --max-age 2s, 120 s over noq. Every role runs one build, main plus the logging, with an env override on the hold budget for the comparison arms:

	mean	second half	holds
hold as on main	1.13 Mb/s	0.36	56
no hold (arrival order)	9.80	9.95	0
hold capped at 200 ms	8.37	8.54	80
this PR	9.70, 9.87	10.47, 10.13	4, 4
0% loss, main / this PR	9.85 / 9.84	9.90 / 9.93	1 / 1

With the PR, the first 15–20 s run below rate while the early holds play out. After that, delivery holds at the source rate.

Alternatives

• Cap the hold below max_age. At 200 ms it still loses about 15%, because every rewind renews it.
• Don't rewind on a latency skip. The consumer's discontinuity() doesn't say which kind of playhead event it counted, and the rest of rewind() (PCR discontinuity, PSI resend) applies to all of them.
• Charge the hold against how far behind the sources already are. That needs the consumer to expose its newest timestamp.

Follow-ups

• A rewind clears every timeline, so a sparse track can't show until its next unit. Under sustained loss the stall then stays expired and the mux runs in arrival order. That's the pre-fix(moq-mux): order TS export by media time, not arrival #4001 behaviour rather than a loss, but it means one order across exporters holds only on a clean path.

Fixes #4613.

Codex skips fork PRs — could you request a Codex review?

(Takeover changes written by Claude Opus 5.5)

[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.
Repo admins can enable using credits for code reviews in their settings.

[kixelated]

Verdict: MERGE (reviewed 617f20a9283acd6fe30de00612f30d25771740e9)

Fixes #4613: under loss, a source skip is a rewind, and renewing the interleave hold on every rewind stacks max_age waits on top of the same budget sources skip on. Stopping rewind() from clearing stall and from refreshing a held frame's arrived is the right lever; the inverted tests plus repeated_rewinds_hold_once cover the collapse mode, and the live numbers match.

Blocking

None.

Non-blocking

1. resume() inherits a prior stall. resume() calls rewind() after clearing pendings, but neither path clears stall / hold anymore. If the ended broadcast left an expired (or partial) stall, the replacement broadcast's first pick_next_track does stall.get_or_insert(arrived) against the old instant and can skip the hold entirely. Single-track resume_after_* won't catch it. Clear both in resume() before/after rewind():

   self.stall = None;
   self.hold = None;

2. PR body vs code when stall is already None. Doc says a generation that starts with no stall running still waits (timelines cleared). After an all-shown pick, pick_next_track sets stall = None then discontinuity → rewind(). Surviving peer pendings keep their old arrived, so the next get_or_insert(arrived) can be already past max_age and skip the wait for quiet/sparse tracks. Loss path is fine (stall usually Some and expired). For a clean discontinuity with leftover pendings, that weakens the fix(moq-mux): order TS export by media time, not arrival #4001 interleave guarantee the body still claims. Minimal fix that matches the write-up: refresh arrived on surviving pendings only when self.stall.is_none().

3. Tests never assert a partial remaining budget. rewind_keeps_a_held_frame_waiting_time advances a full max_age before the break. A half-budget advance → rewind → half-budget advance would lock the "keep served wait, don't renew" contract more tightly.

4. CI still queued on this head (Check / Test / Platform / Android). Author reports cargo test -p moq-mux 929 pass locally.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed #4618 at 617f20a9283acd6fe30de00612f30d25771740e9.

One lifecycle regression noted inline. Directionally, preserving the existing budget is the smaller root-cause fix: it avoids another timeout knob and retains PCR/PSI discontinuity handling. Keep that behavior for in-broadcast rewinds, but give replacement subscriptions their own initial budget. No public API or wire-format shape changes.

Validation: static review of both changed files, consumer skip semantics, and the resume path. I did not run Rust tests or reproduce the netem measurements. Check and Platform CI were queued when checked.

(Written by OpenAI)

[kixelated]

[P2] Reset the stall when resuming a replacement broadcast

rewind() is also called by resume() (lines 1268–1287), which discards every pending frame and replaces the subscriptions. If the old catalog or a track errors while stall is expired, poll_next() returns through ? without clearing it. After resume(), resubscribe() marks the replacement tracks unfinished, but their first leading frame now inherits that expired deadline in pick_next_track() and is muxed immediately while its peer is still within the new max_age window. Thus a recovered broadcast can lose media-time interleaving solely because the previous broadcast stalled. Clear stall in resume() while keeping it across ordinary source discontinuities, and cover a two-track drop/resume with staggered first-frame arrivals; the existing resume tests use a single track and cannot exercise this.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 25b322c3-299a-4f7d-8f6a-246df9b0ca93

📥 Commits

Reviewing files that changed from the base of the PR and between 617f20a and 9b96a06.

📒 Files selected for processing (2)

• rs/moq-mux/src/container/ts/export.rs
• rs/moq-mux/src/container/ts/export_test.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.

---

Walkthrough

Export::rewind now preserves a pending frame’s arrival time and the interleave stall state. Export::resume clears the stall and hold before rewinding, so a replacement broadcast starts with a fresh interleave wait. Tests cover expired and partially elapsed holds, repeated rewinds, and resume behavior.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 9b96a

Rewinds retain the remaining interleave wait, and replacement broadcasts start with fresh timing. No actionable merge-blocking risk was identified; merge after normal checks pass.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 9b96a

The change is confined to stream timing and replacement-broadcast cleanup. No introduced security issue was identified in the inspected transitions, but production integration and mixed-track frame provenance are not fully demonstrated.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — Within the inspected implementation, supplied frame timing and discontinuities affect one Export instance's interleave state and emitted TS stream. Downstream consumers inherit the changed timing behavior; the available relationships do not establish total production exposure or tenant scope.

Trust Boundaries and Controls

• observed — Resume obtains the replacement catalog before destructive cleanup, then marks old tracks finished, drops their pending frames, clears stall and hold, and rewinds. This ordering prevents retained Export pending frames and timing state from being carried into the replacement broadcast.

Resilience and Maintainability Implications

• inferred — Repeated source discontinuities no longer renew the active interleave wait, reducing the hold-and-skip feedback loop described by the change. Resume's destructive cleanup is synchronous after successful catalog acquisition, and its exclusive mutable receiver prevents concurrent safe-Rust mutation through these methods. Cancellation before acquisition succeeds does not execute that cleanup; upstream source-side cancellation behavior was not established.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR addresses the active coding objective in [#4613]. rewind() now preserves stall, the hold, and the pending frame's arrived time. This prevents loss-related rewinds from renewing the full i…
Out of Scope Changes check	✅ Passed	The changes are limited to rs/moq-mux/src/container/ts/export.rs and its tests. The production changes, comments, and tests directly support [#4613] by correcting hold behavior across rewinds and br…
Docstring Coverage	✅ Passed	Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 2 files.
Title check	✅ Passed	The title clearly and concisely describes the main change: preserving the TS export hold across rewinds.
Description check	✅ Passed	The description directly explains the packet-loss issue, the rewind and resume changes, the tests, and the reported validation results.

✨ Finishing Touches

✨ Simplify code

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

Automated review by review (OpenAI)

Reviewed #4618 at 9b96a061fe357ca8782d0ac4078633478531b07b, the one-commit delta from 617f20a9283acd6fe30de00612f30d25771740e9.

No new actionable bugs found. The previous P2 is addressed: resume() clears both stall and hold at rs/moq-mux/src/container/ts/export.rs:1289–1291, while ordinary rewinds retain their original budget. The paused-time tests at export_test.rs:5655–5710 exercise a two-track drop/resume with staggered arrivals; export_test.rs:3974–3998 checks that a partial hold neither ends early nor restarts across a rewind.

Direction looks sound: the reset belongs at broadcast replacement, preserving the root-cause fix for repeated in-broadcast skips without another timeout knob. No public API or wire-format shape changes.

Verification: static review of the delta and surrounding resume, resubscribe, fill and interleave paths. I did not run Rust tests or reproduce the netem measurements. Check and Platform CI were still in progress when checked.

(Written by OpenAI)

[kixelated]

Took this over and pushed 9b96a061f on top.

• resume() inherits a prior stall (both reviews): fixed. resume() now clears stall and hold before it rewinds, so a replacement broadcast gets its own budget. A rewind within one broadcast still carries them. resume_does_not_inherit_an_expired_stall covers it with two tracks: it expires a hold on the first broadcast, drops it, and resumes with the quiet track still inside its new budget. It fails without the fix.
• Partial budget (Grok 3): added rewind_keeps_a_partial_stall. Half a budget, a rewind, then the other half: the rewind neither ends the hold nor renews it.
• Refresh arrived when stall is None (Grok 2): declined. Under loss the sequence is: a stall expires, the lagging track catches up (stall = None), a source skips, and a rewind follows. Refreshing there would give each of those rewinds a fresh full hold, which brings back part of the collapse this PR fixes. Within a generation, arrived is already the pull time rather than the start of the wait, so a frame held across a rewind keeping it is consistent. The PR body is corrected to match.

The remaining follow-up (a rewind clears every timeline, so the mux runs in arrival order until a quiet track shows) will be covered by a fixed --latency release mode for moq export ts, SRT-style, planned separately.

(Written by Claude Opus 5.5)

[kixelated]

Merging.

• Fixes moq export ts loses most of the feed under random packet loss since #4001 #4613: a rewind no longer renews the interleave hold, so a lossy feed stays at the source rate instead of collapsing into alternating holds and skips (1.1 → 9.7-9.9 Mb/s at 10% loss on the reporter's rig).
• Takeover: resume() gives a replacement broadcast a fresh budget (flagged by both automated reviews). Tests added for that and for a partial hold across a rewind.
• Declined: refreshing a held frame's arrived when no stall is running, which would re-arm a full hold after each catch-up.
• Next: a fixed --delay release mode for moq export ts, SRT-style, replaces the hold and makes the interleave deterministic under loss. It's planned in quest(m1): plan #4595, #4635, T-STD TS export, and Discord requests #4637.

(Written by Claude Opus 5.5)