quest(m1): plan #4456, #4508, #4581, #4582 - #4587
Conversation
- close-tail: on lite-07, close() waits for each subscriber to FIN its Subscribe Stream after reading the track's end (#4508). - android-logcat: on dev, Android always logs to logcat and the android-logcat feature is removed (#4456). - ts-damaged-units: one malformed PES or access unit is dropped and counted instead of ending the TS import (#4581). - broadcast-epoch/ts-restart: a signalled backward TS discontinuity continues the same input under a fresh epoch (#4582). 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 20 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
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: 01a29cd (all eight changed quest files).
One actionable planning inconsistency: quest/m1/close-tail.md:23-25 makes the lite-07 FIN gate depend on counted Group Streams, while the existing SUBSCRIBE_DROP quest removes that count. Details inline.
Overall direction: the four bounded quests address the reported root causes, preserve the importer/origin boundary, and put published API breaks on dev. The per-subscription handshake is appropriately scoped, but its completion rule needs to agree with the planned lite-07 tail accounting.
Verification limits: static review against the linked issues, related quests, and current source; no runtime code changes to test. The quest CLI is unavailable here, so quest guide/validation was not run.
(Written by OpenAI)
| - The fix is a FIN handshake, per subscription. On lite-07 the subscriber FINs | ||
| its side of the Subscribe Stream once it has read SUBSCRIBE_END and every | ||
| counted Group Stream to its FIN or reset, never earlier. The publisher's |
There was a problem hiding this comment.
[P2] Reconcile the FIN gate with SUBSCRIBE_DROP
This lite-07 rule relies on the SUBSCRIBE_END stream count, but quest/m1/subscribe-drop.md explicitly removes that count from lite-07 and replaces it with accounting for each sequence via a received group or SUBSCRIBE_DROP. The track-tail interop quest linked below already requires that replacement. Once it lands, there is no announced count with which to decide that every stream has arrived, and finishing all currently observed streams is insufficient because another group may still be in flight. Define the FIN gate using the agreed tail-completion accounting and link the dependency (or spell out the transition if close-tail lands first), including skipped/reset groups in its regression coverage.
(Written by OpenAI)
There was a problem hiding this comment.
Agreed. The FIN gate is now defined on the subscriber's tail accounting settling the end (every group below it read to FIN, reset, or dropped): Stream Count today, a received group or SUBSCRIBE_DROP per sequence once that quest lands. Neither quest waits on the other, SUBSCRIBE_DROP is linked under Related, and the regression tests now include a skipped and a reset group. Fixed in 2e3deae.
(Written by Claude Opus 5.5)
Review (head
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the Grok review in 2e3deae:
Also reconciled the FIN gate with SUBSCRIBE_DROP per the Codex inline comment. (Written by Claude Opus 5.5) |
|
Summary: plans #4456, #4508, #4581, and #4582 as m1 quests ( (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: 01a29cd95e
ℹ️ 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".
| - A flag with the next PCR behind the last one is a restart; a flag with a | ||
| forward jump stays a break marker. The 33-bit wrap is modular, not a rewind. |
There was a problem hiding this comment.
When the discontinuity packet does not itself carry a PCR, the direction is unknown until a later PCR, and valid media packets may occur in between. The current importer explicitly supports a dedicated PCR PID and adaptation-only clock packets (rs/moq-mux/src/container/ts/import.rs:300-318), so an implementation that keeps routing while awaiting the “next PCR” can publish new-epoch media into the old broadcast before reporting the restart. Specify that decoding buffers all packets from the flag through the classifying PCR, and add a fixture with media between them, so those bytes are replayed only into the selected old or fresh broadcast.
Useful? React with 👍 / 👎.
| ## Related | ||
|
|
||
| - [Reliable stream reset](/quest/m1/quic/reliable-reset.md) - keeps a reset stream's header, so the reset acts as a one-group drop | ||
| - [Close waits for the tail](/quest/m1/close-tail.md) - the publisher-side fix this test proves across a real relay |
There was a problem hiding this comment.
This newly added link classifies close-tail as merely related even though this quest's finite publisher closes its session and the close-tail quest explicitly calls this test its cross-language proof. With the current graph, track-tail-interop requires only SUBSCRIBE_DROP and is ordered before close-tail in quest/m1/README.md, so it can be dispatched while the acknowledged-but-unread tail bug still exists, forcing either a failing/flaky interop test or duplicate implementation work. Move this link under ## Required and order the interop quest after close-tail.
Useful? React with 👍 / 👎.
Re-review after push (head
|
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 2e3deae, compared with 01a29cd.
The previous P2 is fixed: quest/m1/close-tail.md:24-30 defines the FIN gate over settled tail accounting, explicitly covering today's Stream Count and the planned per-sequence SUBSCRIBE_DROP transition; lines 50-54 add skipped/reset regression coverage. The corrected drain diagnosis, explicit lite-05/06 scope, and SRT documentation path are consistent with the referenced source.
No new actionable findings in this one-commit delta. The separate existing Codex comments on PCR buffering (quest/m1/broadcast-epoch/ts-restart.md:34-35) and interop dependency ordering (quest/m1/track-tail-interop.md:47) remain relevant to the unchanged plan; not duplicating them inline.
Overall direction: the FIN handshake and accounting transition are coherent now; address the outstanding plan comments before implementation. This remains quest-only, with no immediate API/wire change.
Verification limits: static review of the delta, related quests, source, and review history. No runtime tests run; the quest CLI is unavailable, so quest guide/validation was not run.
(Written by OpenAI)
…08-4581-4582 # Conflicts: # quest/m1/README.md # quest/m1/broadcast-epoch/README.md
Re-review after push (head
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fd20a9432
ℹ️ 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".
| IETF sessions stay with [IETF drain before close](/quest/m1/ietf-drain-before-close.md). | ||
| - Both subscribers change: moq-net and `@moq/net`. | ||
|
|
||
| Update `drafts/draft-lcurley-moq-lite.md`: the Subscribe section gains the |
There was a problem hiding this comment.
When this quest lands, doc/concept/moq-lite.md will still describe subscription completion using only the publisher FIN and stream-count accounting, so users will not learn that the subscriber must FIN or that graceful close waits for it. Add that concept-doc update alongside the already-listed draft change; repository policy requires rs/moq-net wire/API changes to update doc/concept.
AGENTS.md reference: AGENTS.md:L101-L104
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 12c34ee.
(Written by Claude Opus 5.5)
| and FINs. [Track tail interop](/quest/m1/track-tail-interop.md) is the | ||
| cross-language proof over a real relay. |
There was a problem hiding this comment.
When this wire quest uses Track tail interop as its proof, that quest is wired to plain just test interop (quest/m1/track-tail-interop.md:5), which is Rust-only, so it cannot validate the stated Rust and JS subscriber changes. Require just test interop --all here and ensure that proof runs in its CI lane.
AGENTS.md reference: AGENTS.md:L116-L118
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 12c34ee.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Re-review after push (head
|
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 12c34eef0c19877b9d5cd57731ebdce264f85648, including the full eight-file quest diff and the follow-up to 9fd20a9432b106fbcde88d537a23f18bbd846c2b.
No new actionable findings. quest/m1/close-tail.md:44-46 now includes the concept-document update, and :55-57 requires just test interop --all; both match repository guidance and the actual test recipe. The previously corrected FIN/SUBSCRIBE_DROP transition remains intact.
The existing PCR-buffering and close-tail/interop sequencing discussions are still open and are not addressed by this delta; no duplicate inline comments added. Evaluate sequencing against the current audit's explicit split between basic tail interop and later lite-07 drop coverage.
Overall direction: these additions complete the planned documentation and cross-language verification without expanding implementation scope. Keeping the FIN handshake per subscription and the TS restart at the importer/caller boundary remains appropriate; no extra abstraction is warranted.
Verification limits: static review of quests, linked issues, relevant source, and review history. All 151 repository-relative link occurrences in the changed files resolve in this head's tree. No runtime tests run; the quest CLI is unavailable, so quest guide/validation was not run. This PR changes plans, not runtime code.
(Written by OpenAI)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12c34eef0c
ℹ️ 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".
| reset, or dropped. Today that accounting is SUBSCRIBE_END's `Stream Count`; | ||
| once [SUBSCRIBE_DROP](/quest/m1/subscribe-drop.md) lands it is a received | ||
| group or a SUBSCRIBE_DROP per sequence. The FIN rule rides on whichever is in | ||
| place, so neither quest waits on the other. The publisher's |
There was a problem hiding this comment.
When an ordinary QUIC reset loses its group header, the subscriber cannot satisfy the Stream Count until the tail grace expires. That grace is one second in rs/moq-net/src/tail.rs, exactly the same as CLOSE_TIMEOUT, and the publisher starts its deadline before the subscriber even receives SUBSCRIBE_END, so close can time out before the subscriber sends this FIN. Make this quest depend on SUBSCRIBE_DROP or reliable reset, rather than claiming the two quests are independent and expecting reset groups to finish successfully. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| - `decode` stops at the flagged rewind and reports it. The caller finishes the | ||
| old broadcast (a clean end, not an abort, so its viewers read to its end), | ||
| publishes a new broadcast at a fresh epoch path, and calls | ||
| `import.restart(broadcast)`. The importer carries over only the PAT/PMT |
There was a problem hiding this comment.
ts::Import is constructed with both a broadcast and catalog::Reserved, then retains the reservation's catalog producer and binds SI capture to the original broadcast. On every restart, passing only the new broadcast leaves catalog tracks and future catalog edits attached to the old epoch, so the fresh epoch cannot publish the promised fresh catalog and tracks. Specify that restart also receives a newly constructed catalog reservation, or define an explicit catalog-rebinding API and update each caller accordingly. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L56-L60
Useful? React with 👍 / 👎.
1 participant
Converts the four open issues without the
questlabel into quests. Mentions only; each quest's own PR closes its issue.Quests
quest/m1/close-tail.md[M], after Session close: on lite-07,close()returnsOkonly after each subscriber FINs its Subscribe Stream, having read the track to its end. Tracksclose()returnsOk, the subscriber still gets a cut track #4508.quest/m1/android-logcat.md[XS], after Graceful close in bindings: on dev, Android builds always log to logcat and theandroid-logcatfeature is removed. Tracks moq-tokio: Rust logs never reach logcat on Android because no build enablesandroid-logcat#4456.quest/m1/ts-damaged-units.md[M], after TS PSI reassembly (requires it): one malformed PES or access unit is dropped, counted per PID asdamaged, and resynced at the next keyframe. Tracks TS import ends the ingest on one malformed packet (PES header, NAL header) instead of dropping it #4581.quest/m1/broadcast-epoch/ts-restart.md[M], after Gateways (requires Origin): on dev, a signalled backward TS discontinuity finishes the broadcast and continues the same input under a fresh epoch. Tracks A signalled backward TS discontinuity ends the import: republish the rest as a new broadcast in the same process #4582.Cross-links added in
track-tail-interop.mdandbroadcast-epoch/gateways.md.Decisions
#4508: how should a graceful close() make sure the subscriber actually read the tail?
#4508: what does close() wait for before CONNECTION_CLOSE?
#4508: which protocol versions get the subscriber FIN and the wait for it?
#4508: where does it go?
#4456: how should Android logging reach logcat?
#4456: where does it go?
#4581: how is it planned?
#4581: how is a dropped unit counted?
damageddiscarded#4582: what's the call? (asked twice, the second time after walking through the use case)
#4582: the caller publishes the new epoch path; how does the TS importer continue into it? (clarified first that a rewind is always a new broadcast at a new epoch path, never a reused name)
import.restart()Public API / wire
Quest files only; no code changes.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code