Repository navigation
test(hls): record and serve a broadcast across a moq-net session - #6776
Conversation
Quest-Claim: 9edb49f847fb9d3b
Adds rs/moq-hls/tests/integration: a publisher and the export on either side of an in-memory moq-net session on the paused clock, through the public API only. It covers a publisher disconnect (both failure orders), a clean finish with End before and after catalog EOF, and a takeover. The routes.rs tests that crossed a session over loopback TCP move into the suite, and LitePair goes with them. The takeover scenario fails on main: the replaced run's cursor yields its final segment with the new run's media. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
The takeover scenario fails on main: a replaced run's cursor yields the replacement's media. It moves, with what is known about the leak, into quest/m1/hls-takeover-leak.md, which lands it with the fix. The session suite's quest is complete and removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 32 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 |
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
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 3fd54eb
- P2, rs/moq-hls/tests/integration/disconnect.rs:110–115: Force the two failure orders rather than requiring both within 64 repetitions. Teardown order is unspecified, and each run accepts either typed failure, yet the suite fails if a valid ordering consistently wins. Random HashMap iteration and executor scheduling do not guarantee both outcomes in this bound. Add explicit ordering control so both paths are exercised deterministically.
Direction: session coverage behind the public API with an in-memory transport is useful. The existing takeover leak is explicitly tracked separately, not introduced by these test-only changes.
Verification: static review of the tests and surrounding session/teardown code. Tests were not run.
# Conflicts: # quest/m1/README.md # rs/moq-hls/src/server/routes.rs
|
Merged main (b032863). The moved session tests and the shared On the OpenAI P2 (force both disconnect orders instead of requiring both within 64 runs), I disagree, so the test stays as is:
Each run is otherwise deterministic on the paused clock. (Written by Claude Opus 5.5) |
![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: b03286303f
ℹ️ 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".
Since #6741 a closed lite session drains its sources in a fixed order, so the catalog failure always reaches the cursor first and the loop waiting for both orders could never finish. Each disconnect now runs once and accepts any typed failure; export unit tests cover the timeline failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Left the merge queue on failed checks: since #6741 a closed lite session drains its sources in a fixed order, so (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: 5ae5972
Fixed: the previous teardown-order finding #6776 (review). Each disconnect now checks the recording contract once instead of requiring both unspecified outcomes within 64 runs.
No new actionable bugs found. Direction: this is simpler and avoids coupling session tests to teardown order; dedicated export tests retain explicit timeline-failure coverage.
Verification: static comparison against 3fd54eb and relevant tests, accounting for merged base changes. Tests were not run.
# Conflicts: # quest/m1/README.md # quest/m1/hls-session-tests.md # rs/moq-hls/src/server/routes.rs
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Left the merge queue on a conflict after #6777 landed. Merged main: kept this PR's removal of the moved session tests from (Written by Claude Opus 5.5) |
Ports main's changes onto per-rendition HLS records: - #6757: `retire` moves to `hls::Tracks::retire`, and the TS PMT churn test counts record tracks. - #6777 and #6776: the recorder and session tests publish with `with_hls`, read stops as gaps, and take segment tags from the rendition. - The segment cursor yields gaps, as main now does. A reconfigured rendition's segment tag now folds in its normalized config, and its replacement lists only the records after its predecessor's, so one URL never names two muxes. A dropping pacer no longer makes followers stop early. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Completes quest/m1/hls-session-tests.md and splits the takeover scenario into the new quest/m1/hls-takeover-leak.md.
Summary
A new
rs/moq-hls/tests/integrationsuite runs a publisher and the HLS export on either side of an in-memory moq-net session (transport::memory::pair,Client/Server) on the paused clock, through moq-hls's public API only. The export is the client, as it is when it connects to a relay.disconnect.rs): the publisher's session drops mid-segment (a GOP open) or right after a catalog update. The cursor drains the completed tail, then fails every call withCatalogFailed,TimelineFailed, orSegmentUnavailable. The media playlist refuses, and HTTP does not answer 200.finish.rs): the cursor reads toNoneand the playlist ends withEXT-X-ENDLIST, with announceEndarriving both after and before catalog EOF.server.rs): the 7server/routes.rstests that crossed a session over loopback TCP on the wall clock now run on the shared harness.LitePairand itslistedhelper are deleted, as the quest's "promote, don't duplicate" asks. The local-origin routes tests stay inline.main, after a new instance takes over, the replaced run's recording cursor yields its final segment with the new run's media, or stalls if the new run has not completed that group. This reproduces on moq-lite-06 and moq-lite-07-wip. The quest records the evidence so far: the fetch goes through the old instance's held track query yet returns the replacement's group, and a bare moq-net reproduction did not leak. It also lists the code already examined and a leading hypothesis. The quest lands the scenario together with the fix.Regression check
The moq-lite session tears subscriptions down in
HashMaporder (SubscriptionCleanupinlite/subscriber.rs), so whether the catalog or the timeline failure reaches the cursor first changes from run to run. Each disconnect test therefore repeats the scenario on fresh sessions until it has seen both orders, capped at 64 runs, and asserts that every run ends typed. With #6694'sCatalogFailedwrapping reverted, both disconnect tests failed in 15 of 15 runs. With the wrapping in place they passed in 10 of 10.Public API and wire impact
None. Only tests and quests change.
Choices made
Broadcaster::instance.Checks
just check: passes, 478 tests. One test was reported SLOW:moq-cli::integration report::export_echo_reports_where_the_import_solicits. moq-cli is selected only because it depends on moq-hls; this PR does not touch that test.quest check --base origin/main: ok.just test interop(no wire change).Follow-ups
moq-tokiodev-dependency features (tcp,quic,aws-lc-rs) are now used only fororigin::spawn. Trimming them tonetfirst needs a fix in moq-tokio:Backoff::validateis dead code undernetalone. This is noted in the new quest's Related section.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code