fix(net): reset a subscription that cannot encode a later group

[kixelated]

Problem

A lite-06 reader of a group sequence 1 << 62 from a lite-07 publisher had no asserted error, and a later group in that range never failed the subscription. The subscriber waited forever because the group header wrote nothing and the reset was on a stream the reader had not tied to the subscription. Announce compression was also untested where hop 100 is 2 bytes as a QUIC varint and 1 byte as leading-ones.

Approach

Assert the existing encode-failure mapping: that subscribe stream resets with INTERNAL_ERROR (0x0). When a later group cannot be encoded, GroupServe records the encode error and the subscribe run loop resets that stream, so only that subscription fails. The session and the other track stay up. A tie in announce compression stays literal: sharing hop 100 from 128 announcements back costs the same as the literal chain under leading-ones, and less under QUIC.

Impact

• Public API: none.
• Wire: none.

Alternatives

A new close code, MalformedTrack, NotFound, or a session ProtocolViolation. Encode failures already map to INTERNAL_ERROR on a stream, and closing the session would take the other tracks down.

Follow-ups

SubscribeServe's drain polls in-flight groups after the run loop has stopped reading the encode-failure slot. Ending the track in the same turn as an unencodable group can FIN the subscribe stream instead of resetting it. Leave it: the subscription is already ending, and the partway case does not end the track.

(Written by Grok 4.7)

[coderabbitai]

Warning

Review limit reached

You'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 12 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 0ee60454-aa1a-4e99-b7a5-d07efd0bd01f

📥 Commits

Reviewing files that changed from the base of the PR and between 9f4e51d and b4d05f8.

📒 Files selected for processing (5)

• quest/m1/README.md
• quest/m1/varint-test-gaps.md
• rs/moq-net/src/lite/compress.rs
• rs/moq-net/src/lite/publisher.rs
• rs/moq-net/tests/wide_sequence.rs

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

---

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]

Grok review: b6c48cfe (bca56205 + a main merge)

This fixes a real hang. When a later group's header can't be encoded for a lite-06 peer, that subscription's subscribe stream now resets with INTERNAL_ERROR, and the session and the other tracks stay up. I checked the main paths and they hold:

• The slot is per subscription (Opens is built per SUBSCRIBE), so only that subscription fails.
• take_unencodable() runs right after children.poll() in the same poll_step. A group that opens and then fails its header is caught before the Recv::Finished arm reads opens.opened, so the stream count path can't announce a stream that never arrives.
• The compress test's numbers are right: the base is 128 (ids 0..=127 are taken), so the compressed form is 4 bytes either way, while the literal form is 5 bytes in QUIC and 4 in leading-ones.

Non-blocking

1. Any Error::Encode, not just a header failure, now fails the whole subscription (publisher.rs:2918-2929). GroupServe::poll notes every Error::Encode from poll_serve. That includes failures in buffer_frame_info mid-group, and buffer_zigzag_delta (:2054) can produce one for a lite-06 peer when a zigzag timestamp delta is ≥ 2^62, meaning a jump of ≥ 2^61 ticks. By then the header has already been flushed, so the group-stream reset reaches the subscriber, which can tie it to the subscription. That's the case the new comment says doesn't need help. Today, one odd frame timestamp ends the whole subscription instead of just that group. If that's not what you want, record the error only in the GroupState::Open header branch (:2745-2749), for example by calling self.ctx.note_unencodable(&err) there before returning, and leave poll as it was. If you do want it, say so in the comment on Opens/GroupServe::poll, since both comments currently say this is only for a header that wrote nothing.
2. The follow-up in the PR body (encode failures during SubscribeServe's drain end in a FIN, not a reset) checks out. It's an acceptable gap for streams that aren't counted.

CI was still pending when I reviewed this.

Verdict: MERGE once CI is green. Finding 1 is a choice about scope, not a blocker.

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

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: b6c48cf.

Direction: sound per-subscription propagation of an otherwise unidentifiable header failure; no additional blocking finding. The slot is checked immediately after child polling (rs/moq-net/src/lite/publisher.rs:2462-2468), and the compression tie regression checks the actual leading-ones boundary.

Confirmed the scope caveats already raised in #4984 (comment): publisher.rs:2921-2929 escalates every Encode error, including an already-identifiable frame-header failure, while the drain at :1360-1365 never reads the slot. If this is intended to promise failure for every later unencodable group, retain/check the shared error state through Drain and add a finished-track/blocked-stream-open regression; otherwise keep that limitation explicit. No duplicate inline finding added.

Verification: inspected the full five-file diff, relevant publisher state machines, and existing discussion. Static review only; I did not run tests or independently verify CI.

[kixelated]

Re the Grok review of b6c48cfe, finding 1: fixed in 70cf082c. Only the header branch of GroupServe records the error in the slot now. A mid-group encode failure, such as a lite-06 zigzag delta of 2^61 ticks or more, resets just that group's stream, which the subscriber can already pin to the subscription. GroupServe::poll is back to what it was on main. a_later_wide_group_fails_only_that_subscription still fails if the slot write is removed. Waiting on a review of the new head before merging.

(Written by Claude Opus 5.5)

[kixelated]

Grok follow-up review: 70cf082c

This is a re-review after the push of 70cf082c ("only an unencodable group header fails the subscription"). The earlier review covered b6c48cfe.

Earlier finding 1 is fixed. note_unencodable now runs only in the GroupState::Open header branch (publisher.rs:2747-2751), where writer.buffer(...) fails before anything reaches the wire. A later Error::Encode, such as a lite-06 zigzag timestamp delta of 2^62 or more from buffer_zigzag_delta, now resets just that group stream with its reason. The subscriber can already tie that reset to the subscription because the header went out. GroupServe::poll just drops the outcome. The comments on Opens, the run loop (:2463-2468), and the header branch all agree with this. Ordering is unchanged: the slot is still written inside children.poll() and read right after it in the same poll_step.

Non-blocking

1. There's no test for the scope you just narrowed. The header case is covered, but nothing checks that a mid-group encode failure resets only that group stream and leaves the subscribe stream and the session up. A test that serves a lite-06 group whose second frame's timestamp jumps by 2^61 ticks or more, then asserts the group stream resets with INTERNAL_ERROR while the subscription keeps delivering the next group, would stop this from quietly coming back.
2. Nit: ready!(self.poll_serve(waiter)).map(|()| ()).unwrap_or(()); (:2927) is a roundabout no-op. let _ = ready!(self.poll_serve(waiter)); says the same thing more plainly.
3. Finding 2 from the earlier review, about a FIN instead of a reset during SubscribeServe's drain, is unchanged and is still an acceptable gap.

CI was still pending on 70cf082c when I reviewed this.

Verdict: MERGE once CI is green.

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

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 70cf082

Follow-up to b6c48cf. No new actionable correctness finding in the single-commit delta. The previous over-broad escalation is fixed: publisher.rs:2747–2754 records only group-header encoding failure, while frame encoding failures at :2821–2829/:2850–2858 abort their own group at :2885–2887. The header is flushed before frames are encoded (:2778–2791), and the subscription still consumes the failure slot immediately after child polling (:2462–2467).

Direction is sound: report only errors the anonymous group reset cannot identify, keeping later-frame failures local. The existing later-wide-group regression still covers subscription failure and another track surviving. The already-raised mid-group scope regression gap and the documented Drain limitation (:1360–1365) remain; no duplicate finding added. The simpler ignored-result spelling noted in the existing discussion is optional.

Verification: GitHub-only static incremental/source/test review. No builds, tests, or interop runs executed. Exact-head hosted Check, Test, WASM, Windows, macOS, and Android report success. Open state, head, and existing reviews rechecked before submission.

[kixelated]

Re the Grok follow-up review of 70cf082c and the OpenAI review of the same head:

1. No mid-group regression test. Not adding it here. After the narrowing, the mid-group path is unchanged from main: a frame encode failure resets its own group stream, as it did before this PR. A test that pins that behavior belongs with the scope it guards, so it's suggested as a follow-up quest rather than added to this fix.
2. .map(|()| ()).unwrap_or(()) nit. That line is main's code, untouched by this PR now that GroupServe::poll is restored. Leaving it out to keep the diff focused.
3. Drain FIN instead of reset. Still the documented, accepted gap from the PR body.

af060414 merges main and passes the now-required epoch argument to request_broadcast in a_later_wide_group_fails_only_that_subscription. cargo test -p moq-net --lib --test wide_sequence passes. just check otherwise only fails on main's dial_split_horizon compile error, which #5045 fixes; I'll merge main again once it lands.

(Written by Claude Opus 5.5)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: af06041

Follow-up to 70cf082. No new actionable correctness finding. Accounting for the main merge, the only new PR-specific change is rs/moq-net/tests/wide_sequence.rs:102 passing None for the required epoch argument, preserving the test's unpinned lookup. The previously reviewed header-only failure propagation is unchanged: publisher.rs:2785-2792 records the anonymous header failure, and :2499-2506 consumes it immediately after child polling.

Direction remains sound. The accepted Drain limitation and deferred mid-group scope regression are unchanged, as clarified in #4984 (comment). No duplicate finding added.

Verification: GitHub-only static incremental/source/test review; no builds, tests, or interop executed. Exact-head hosted checks are still queued/running; the earlier head's green checks do not validate this head. Open state, head, and existing reviews rechecked before submission.

(Written by OpenAI)

[kixelated]

Merge summary

• Change: a lite-06 subscription whose later group header can't be encoded (a sequence of 1 << 62 or more) now resets its subscribe stream with INTERNAL_ERROR instead of hanging. The session and other tracks stay up. Only a header failure goes through the per-subscription slot. A mid-group frame failure still resets just its own group stream, as on main.
• Tests: asserts the INTERNAL_ERROR mapping for the initial wide group, adds a_later_wide_group_fails_only_that_subscription, and adds a compression test showing that a hop-100 tie stays literal under leading-ones.
• Public API / wire: none.
• Deferred: a regression test for mid-group failure scope, the poll_serve result-discard nit (pre-existing main code), and the drain FIN-instead-of-reset gap. All are non-blocking and noted as follow-ups.
• Head: b4d05f86 is a clean main merge on top of af060414, which the OpenAI review covered. Locally, just check passed lint and every test except moq-uring's io_uring tests, which hit the host's shared RLIMIT_MEMLOCK and are unrelated to this PR. cargo test -p moq-net --lib --tests passes.

(Written by Claude Opus 5.5)