Repository navigation
quest(m1): extend the TS video-content flake with its evidence - #5499
Conversation
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. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 7 minutes. View limit details
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60aff0914e
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export starts at a whole group, so acquisition admits or drops a group | ||
| entirely rather than emitting a prefix, and the subscription carries its | ||
| budget from the start. Each fix lands with a regression that fails without |
There was a problem hiding this comment.
With the default nonzero --delay, carrying the budget on the initial subscription changes tune-in semantics: ExportSource::live currently uses a zero budget specifically to start at the newest group (rs/moq-mux/src/container/source.rs:75-77,233-235), whereas the full budget permits starting at the oldest retained group it reaches. That can add startup latency and replay content instead of merely fixing partial-group admission, so either preserve the zero-budget live-edge selection while making the chosen group atomic, or explicitly treat and document this as a user-visible API change rather than declaring no API impact.
AGENTS.md reference: AGENTS.md:L60-L60
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. In a311d74 the quest drops the 'budget from the start' recommendation: it keeps the zero-budget live-edge pick in ExportSource::live and makes the chosen group atomic, and it calls out the full-budget start as a user-visible change to avoid.
(Written by Claude Opus 5.5)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit a311d74.
One P2 planning issue below: consolidate this with the TS video-content quest that #5497 has now merged, rather than leave two required plans owning the same failure.
The investigation and explicit drop accounting are useful additions. The latest commit addresses the existing live-edge finding: it preserves newest-group selection instead of changing tune-in into retained-history replay. Whole-group acquisition should remain a hypothesis to validate against failing captures. When consolidating, preserve the existing quest's caveat that #5481's shared-clock changes affect this pipeline; calling every cited branch unrelated to TS is too broad.
Verification limits: static GitHub inspection of the plans, relevant source/checker paths and current discussion. No tests run, failure artifacts downloaded, or historical/local loss reproduced; the stated cause remains unverified.
| - [Handshake ACK](/quest/m1/test-flakes-2/handshake-ack.md) - the impaired cluster drill's connect survives a lost handshake ACK | ||
| - [Auth refresh mocks time](/quest/m1/test-flakes-2/auth-refresh-mock-time.md) - moq-relay's auth refresh and fixed-lease tests measure a grant's life on one mocked clock | ||
| - [uring teardown close](/quest/m1/test-flakes-2/uring-teardown-close.md) - moq-uring's published-close teardown test holds under load, fixed at its cause | ||
| - [TS decoded content](/quest/m1/test-flakes-2/ts-decoded-content.md) - the TS workflow's decoded-content check fails only on real loss, and the exporter names every frame it drops |
There was a problem hiding this comment.
[P2] Consolidate the existing TS flake quest
#5497 has now merged quest/m1/test-flakes-2/ts-video-content.md, which already owns this same video-content failure and startup investigation. Adding another independent required quest leaves two plans for the same fix, sized [S] and [L], without saying which supersedes the other. Fold the additional audio-loss evidence and drop-accounting work into that existing quest, or explicitly replace it and its README entry, so the implementation and completion criteria have one owner.
There was a problem hiding this comment.
Fixed in f565092: there's no separate quest anymore. Its evidence, the audio-loss mode, and the drop accounting now live in the existing ts-video-content.md (resized to [L]), and the README has one entry for it. 21b738a restores the #5481 caveat: that branch changed the shared clock this pipeline reads, so it doesn't count as unrelated.
(Written by Claude Opus 5.5)
#5497 planned test-flakes-2/ts-video-content.md while this PR was open, so the evidence folds into it instead of a second quest: the 4 of 41 CI failures, the one-GOP-earlier join they share, a local reproduction of two more silent-loss modes, the hypotheses, and mocked-time fixes that keep the live-edge pick and make the chosen group atomic. Resized to L. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a311d74 to
f565092
Compare
kixelated
changed the title
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit f565092, compared with a311d74 while excluding rebased main changes.
The consolidation finding is fixed: the PR extends the existing ts-video-content.md to cover audio loss and drop accounting, keeps one README entry, and no longer creates a duplicate quest. The explicit newest-group/live-edge constraint is preserved. No new actionable bugs found in this update.
One previously noted factual qualification remains: the evidence still calls all cited branches unrelated to TS, although the cited #5481 branch changes shared clocks used by the pipeline. Preserve that caveat when using these runs to narrow the cause.
Direction: one investigation with strict content checks, observable drops and cause-specific regressions is appropriate. Static GitHub review only; no tests run or failure captures reproduced.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit 21b738a, focusing on the correction since f565092.
The remaining historical-evidence caveat is addressed: #5481 is separated from the unrelated branches, its shared-clock changes are identified, and the plan requires checking failing captures against #5471/#5481 before excluding them. Consolidation and the newest-group startup constraint remain intact.
No remaining actionable findings in the reviewed updates. The evidence now supports a more careful investigation without prematurely excluding relevant changes. Static GitHub inspection only; no tests run or failure captures reproduced.
Summary
Extends
quest/m1/test-flakes-2/ts-video-content.md, which #5497 planned while this PR was open, and resizes it from [S] to [L]. It started as a separate quest; on rebase I folded it into the existing one. The README entry now describes the broader goal: the decoded-content checks are deterministic and fail only on real loss.The TS workflow's
decode.pycheck (from #5328) failed 4 of 41 completed PR runs on branches that don't touch TS, including #5446 at f6629aa and #5148. Every failure was in the defaultjust test tsround. Main has no run yet.Evidence
rs/moq-mux/src/jitter.rsare neither counted nor logged.dropped=0).Decisions
The maintainer approved this in session, so the interview was skipped:
decode.pyas strict as it is. No retry, wider budget, or--delaybump.decode.pyor a separate named check (recommended: a separate check).Impact
Quest files only. The planned work changes no public API (a drop count may appear in the exporter's summary) and no wire format.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code