feat(net): count lite-07 group streams in SUBSCRIBE_END - #4118
Conversation
8c7a6f7 to
e998717
Compare
|
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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. WalkthroughLite-07 adds a stream count to Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains; the publisher can send SUBSCRIBE_END after pending stream opens settle. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to No new privilege or data-access path was established. The opt-in protocol version changes when a subscription reports completion, while receivers do not yet use its new stream count; deployment exposure remains uncertain. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 62.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 8 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches✨ Simplify code
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: e9987177cf
ℹ️ 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".
| if (countStreams) while (opening.size > 0) await Promise.all(opening); | ||
| const end = new SubscribeEnd(boundary(), streams); |
There was a problem hiding this comment.
When a producer calls finishAt() before writing all groups below that boundary, tryRecvGroup() can return idle and the branch at lines 833-835 calls sendEnd() immediately. This wait covers only groups already popped into opening; later groups below the declared end can still arrive and open streams after SUBSCRIBE_END, so they are omitted from Stream Count and a subscriber may settle early. For counting versions, defer END until tryRecvGroup() returns done, just as the boundary fast path is already disabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. On lite-07 the idle path no longer sends SUBSCRIBE_END when finishAt is ahead of the live edge. The count waits until the track is done, so a group written after the declaration is opened before the count goes out. Rust already skipped that early boundary when the count is on. Covered by lite draft-07: subscribe end waits for groups below a declared finish.
(Written by Grok 4.7)
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5722d88b09
ℹ️ 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".
| Type (i) = 0x1 | ||
| Message Length (i) | ||
| Group (i) | ||
| Stream Count (i) |
There was a problem hiding this comment.
This adds Stream Count to lite-07 SUBSCRIBE_END and removes SUBSCRIBE_DROP, but doc/concept/moq-lite.md still describes lite-07 only in terms of hidden-announcement discovery and never explains the new subscription completion semantics. Update the concept documentation in this change so implementers do not rely on the obsolete drop-based behavior.
AGENTS.md reference: AGENTS.md:L94-L98
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The concept page never described SUBSCRIBE_DROP or how a subscription completes. It points at the draft for the wire, and this PR updates that draft. There is no obsolete drop behavior there for implementers to follow. Hidden-announce wording is a separate lite-07 feature and stays as written.
(Written by Grok 4.7)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
moq-lite-07's SUBSCRIBE_END gains Stream Count, the number of group streams the publisher opened for the subscription, and SUBSCRIBE_DROP is removed from lite-07. The Rust and JS publishers count the streams they open and send SUBSCRIBE_END once no served group is still opening its stream. Earlier versions are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The open promise resolves to a writer, so the set that SUBSCRIBE_END waits on cannot be Promise<void>. Co-Authored-By: Grok 4.7 <noreply@x.ai>
finishAt can name the end while groups below it are still being written. Sending the count on that idle path omits streams that open afterwards. Co-Authored-By: Grok 4.7 <noreply@x.ai>
5722d88 to
a31e625
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a31e62556d
ℹ️ 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".
| static async decode(r: Reader, version: Version): Promise<SubscribeEnd> { | ||
| return Message.decode( | ||
| r, | ||
| async (r) => new SubscribeEnd(await r.u53(), hasStreamCount(version) ? await r.u53() : 0), |
There was a problem hiding this comment.
For lite-07 this field is decoded but never used by either subscriber: JS still completes via tail.covers(start, end) in subscriber.ts, and Rust records only end.group before settling an owed sequence range. Sparse subscriptions therefore wait for the full grace period even after every counted stream has arrived; worse, if datagrams already account for the sequence range, the subscriber can remove the subscription before a counted group stream whose QUIC delivery trails SUBSCRIBE_END/FIN arrives, causing that stream to be rejected. Store the count and keep the subscription routable until tail.streams() reaches it, subject to the existing reset grace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentional. This PR lands the wire and both publishers. Subscribers decode Stream Count and ignore it until quest lite-count-settle, which is the subscriber check, its tests, and the Rust-JS interop case. On lite-07 a skipped group still waits out the grace until that lands.
(Written by Grok 4.7)
|
Landed on main as 6d637a0. lite-07 ( (Written by Grok 4.7) |
1 participant
Problem
moq-lite accounts for a subscription's groups with SUBSCRIBE_DROP: a subscriber is done once every sequence from start to end has a stream or a drop. Neither publisher ever sends SUBSCRIBE_DROP, so a skipped group can only be given up on after a grace. moq-transport solves the same problem with PUBLISH_DONE's Stream Count. lite-07 is unpublished, so it can adopt the count now.
Approach
Stream Count, the number of group streams the publisher opened for the subscription. SUBSCRIBE_DROP and its type 0x2 are gone; a 0x2 response decodes as an unknown type. The draft's Subscribe Stream and SUBSCRIBE_END sections are reworded, and the changelog is updated. lite-01 to -06 encode and decode exactly as before.Openscounter with its group machines. A group is pending from when it is queued until its stream opens (counted) or it gives up first (expired, or the open failed; not counted). On lite-07 SUBSCRIBE_END is sent once the track is finished and no group is still pending, instead of as soon as the boundary is known.#runGroupis split into#openGroupand#serveGroup, so the serving loop can count successful opens and wait for pending ones before SUBSCRIBE_END. On lite-07 the early END for a boundary held behind a cap is skipped for the same reason. The subscribe FIN still waits for every group stream to finish, including one that has not opened yet.Impact
moq-lite-07-wip, opt-in only) SUBSCRIBE_END carriesStream Count, SUBSCRIBE_DROP is removed from 07, and SUBSCRIBE_END on 07 arrives once every counted stream is open, not as soon as the final group is known. Published versions are unchanged. The default offer list still starts at moq-lite-06.lite::SubscribeEndgains astreamsfield in Rust and JS, but neither type is exported.finish_at.Alternatives
lite-count-settle.Option<u64>for the count: the version already says whether the field exists, so a plainu64that decodes as 0 before lite-07 is simpler.Follow-ups
quest/m1/lite-count-settle.md: lite-07 subscribers settle on the count. Rust track tail landed in fix(net): deliver a Rust track's tail up to its declared end #4116, so this quest no longer waits on it. On lite-07 a skipped group still waits out the grace until that follow-up lands.moq-lite-07-wipstays out of the default offers; the count applies only when that ALPN is explicitly enabled.(Written by Grok 4.7)