quest: import #4951, #4950, #4945, #4847; decline #4926

[kixelated]

Imports the open issues without the quest label. Each report's claims were checked against main (0574f5b) before deciding.

Changes

• moq-net: draft-22 LOCATION_FILTER should drop the Length and carry the Location Filter Type #4847 → new m0 Draft-22 LOCATION_FILTER [S]. Confirmed against the draft-22 text. JS has the same bug.
• moq-net: a group held across a route switch is not woken when its successor gets its first timestamp #4950 → folded into m0 Expiry wakes. Confirmed: the reporter's test fails on main.
• hang: a consumer whose first group dies empty parks its cursor on a group that never comes #4951 → new m1 JS startup hole [S]. Confirmed, and found a second path to the same stall that the reporter's fix misses.
• relay: a publisher replacing a killed one is sent the old session's subscriptions before it has published, and moq import ts exits with "rendition is not published" #4945 → attached to m1 Enabled flag, whose open PR feat(hang)!: one enabled flag replaces stalled #4915 deletes publish_stalled. That function makes the importer exit when a track is demanded before its first keyframe. The relay half (old subscriptions resumed onto the new publisher, and the catalog groups regressing) is fixed by feat(net)!: carry publisher epochs on routes #4942's epochs.
• Design: an object-level IETF API (subgroup, object ID, properties, status) and a relay mode that forwards them unchanged #4926 → declined: the model is the product. New m1 IETF object gaps [S] (resized from XS in review: closing the session on overflow needs a new route from the data stream to teardown) makes refusing a gapped object ID consistent and loud, and makes an overflowing ID close the session.

Decisions

#4926 scope: should moq-net carry moq-transport object identity?

• ✅ Out of scope: the model is the product, so close as won't fix (Recommended)
• Opaque properties only
• Full object identity
• Relay passthrough mode

#4951 fix: where should the startup stall be fixed?

• ✅ Promotion guard, m1 [S] (Recommended)
• Reporter's finally fix, m1 [S]
• Guard fix, m0

#4950 plan: how to plan the missed wake for the held group?

• Standalone m1 [S] (Recommended)
• ✅ Fold into expiry-wakes
• Standalone in m0

#4945: how to track it?

• ✅ Attach to catalog-enabled (Recommended)
• Close now as covered
• Separate XS quest

#4847: priority?

• ✅ m0 [S], Rust + JS (Recommended)
• m1 [S]
• Rust-only now

#4926 gaps: track the object-ID refusal follow-up?

• ✅ m1 [XS] quest (Recommended)
• m0 [XS]
• Don't track

Recover wake: how does the deadline index cover Recover's held group?

• ✅ Same index, both callers (Recommended)
• Shared per-reader helper

Public API: none. Wire: none (quest files only).

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[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 9 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: 59bdea67-c8fb-4695-88d2-906254e676d2

📥 Commits

Reviewing files that changed from the base of the PR and between 8ecd2ca and 70503e0.

📒 Files selected for processing (7)

• quest/m0/README.md
• quest/m0/expiry-wakes.md
• quest/m0/ietf-location-filter-22.md
• quest/m1/README.md
• quest/m1/catalog-enabled.md
• quest/m1/ietf-object-gaps.md
• quest/m1/js-startup-hole.md

• 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 of 8fdd4f7b (quest files only; checked against main at 8ecd2cad and the draft-22 text)

The claims mostly hold up. I confirmed the draft-22 LOCATION_FILTER figure and type table (§9.20.9, changelog A.1), the recv_group swallow of gapped object IDs, the publish_stalled → track.modify() path in rs/moq-mux/src/codec/video.rs, the Recover::poll → poll_stale wiring, and both JS startup-hole paths through consumer.ts. A few plan details are off or understate the work. None of them block merging a quest.

Non-blocking

1. ietf-object-gaps.md: closing the session on overflow has no existing path. Uni-stream errors never reach the session. session.rs:870-876 runs recv_group and passes its result to stop_on_error (session.rs:927-938), which only aborts the stream, even for an is_protocol_violation error. So "return the BoundsExceeded instead of swallowing it" gets you STOP_SENDING, not PROTOCOL_VIOLATION. The implementer needs a new route from the subgroup task to session teardown, like the one run_subscribe_namespace uses at session.rs:281. It would help to say that in the Facts list, and maybe to re-check the [XS] size. The overflow case is also Rust-only: JS Frame.decode (object.ts:315-318) rejects any non-zero delta before adding anything, so JS has no overflow to close on. Either scope the overflow test to Rust or say what JS should do with a huge delta.
2. ietf-location-filter-22.md: wrong file for the FILL_PARAMETERS scope. It isn't in parameters.rs. It's impl Param for Fill in rs/moq-net/src/ietf/filter.rs (Fill::param_encode/param_decode, around lines 548-600). The claim that every 0x21 use goes through Param for Filter is still correct.
3. ietf-location-filter-22.md: the JS fix isn't "the same exception the Range Filters have." That exception forces a Length onto ids whose parity says varint (parameters.ts:228, :617-621). Draft-22 needs the opposite: 0x21 is odd but carries no Length, so the decoder has to read the type and then that type's fields straight from the stream. Today the value is stored as raw bytes and decoded later by filter.decode(Uint8Array), so the storage changes too. decodeInline in filter.ts:230 is the existing pattern to follow. encodeFill (filter.ts:318-331) also hard-codes encodeLengthPrefixed for 0x21, alongside the FILL_ALLOWED entry the quest already names.
4. expiry-wakes.md (moq-net: a group held across a route switch is not woken when its successor gets its first timestamp #4950 fold-in): "re-keys the entry" skips two differences. The planned index is a per-track map ordered by reach plus budget. Recover's held group (a) has no reach yet, since the bug is exactly that the successor is unstamped, and (b) is judged against the serving copy (resume.rs:879, track.rs:2680), which is a different track from the one holding the group and changes on every route switch. The plan should say that an entry can wait on its successor's first stamp with no deadline yet, and that Recover registers on the serving copy's index and moves its entry when the route changes. Otherwise "one mechanism" quietly picks up both cases during implementation.
5. ietf-location-filter-22.md tests: fetch.rs (around line 913) decodes the length-prefixed 21 03 04 00 02 on Draft20/21/22 in one loop. Draft22 needs to be split out of that loop, not just get new bytes. The draft changelog also cites feat(moq-net): add OriginProducer::dynamic + infallible OriginConsumer::request_broadcast #1913 alongside moq-net: track announcement byte usage in stats #1953, if you want the reference to be complete.

js-startup-hole.md and the catalog-enabled.md addition match the code. Fixing it in the promotion guard really does cover both paths, the reporter's finally path and the next() done-branch += 1, and the open question about #presentedEnd versus #deliveredGroup is a real one (#checkMaxAge drops groups without #recordPresented). CI is still queued, which doesn't matter for a quest-only change.

Verdict: MERGE

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

[kixelated]

I agree with all five of the Grok review's findings and addressed them in 4cfbb2f:

1. ietf-object-gaps.md: added that stop_on_error only aborts the stream, so the overflow close needs a new route from the data stream to session teardown. Resized it to [S]. The overflow case is now Rust-only, since JS refuses any non-zero delta before it could overflow.
2. and 3. ietf-location-filter-22.md: the FILL scope now points to Param for Fill in filter.rs. The JS plan now describes the real change: 0x21 is read inline from the stream (following decodeInline) instead of being stored as raw bytes, which is the opposite of the Range Filters' exception. encodeFill is added too.
3. expiry-wakes.md: an index entry can now wait on its successor's first stamp with no deadline yet. Recover's entry lives in the serving copy's index and moves when the route changes.
4. The fetch.rs loop that Draft22 must leave is now called out, and the changelog reference adds moq-transport#1913.

(Written by Claude Opus 5.5)

[kixelated]

Grok follow-up review of 4cfbb2f8 (re-review after the push "quest: address review of the issue import"; compared against the reviewed 8fdd4f7b, checked against main)

All five findings from the earlier review are fixed, and the new text matches the code:

1. Fixed. ietf-object-gaps.md now says stop_on_error (session.rs:927) only aborts the stream and that the overflow close needs a new route like run_subscribe_namespace's (session.rs:278-281). It was resized to [S] and scopes the overflow case to Rust, since JS refuses any non-zero delta. The m1 README line says the same.
2. Fixed. The FILL_PARAMETERS scope is now attributed to Param for Fill in filter.rs (line 549), which does call Filter::param_decode for 0x21.
3. Fixed. The JS plan now describes the inline-read change, the raw-bytes storage change, decodeInline (filter.ts:230) as the pattern, and encodeFill's hard-coded encodeLengthPrefixed (filter.ts:328).
4. Fixed. expiry-wakes.md now says an entry can wait for its successor's first stamp with no deadline yet, and that Recover's entry lives in the serving copy's index and moves when the route changes.
5. Fixed. The fetch.rs loop over Draft20/21/22 (line 920) is named as something Draft22 has to leave, and feat(moq-net): add OriginProducer::dynamic + infallible OriginConsumer::request_broadcast #1913 is cited.

One tiny nit: the Goal in ietf-object-gaps.md still says an overflowing object ID "closes the session with PROTOCOL_VIOLATION" right after "in Rust and JS". Adding "(Rust)" there would match the README and Tests lines. CI (Check, Test) is still pending, which doesn't matter for a quest-only change.

Verdict: MERGE

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

[kixelated]

Merge summary

This imports the open issues without the quest label. Each decision was confirmed with the maintainer and is listed in the description.

• moq-net: draft-22 LOCATION_FILTER should drop the Length and carry the Location Filter Type #4847 becomes the m0 quest ietf-location-filter-22.
• moq-net: a group held across a route switch is not woken when its successor gets its first timestamp #4950 folds into m0 expiry-wakes, with one deadline index for both callers.
• hang: a consumer whose first group dies empty parks its cursor on a group that never comes #4951 becomes the m1 quest js-startup-hole, fixed in the promotion guard.
• relay: a publisher replacing a killed one is sent the old session's subscriptions before it has published, and moq import ts exits with "rendition is not published" #4945 attaches to m1 catalog-enabled. Its relay half is fixed by feat(net)!: carry publisher epochs on routes #4942.
• Design: an object-level IETF API (subgroup, object ID, properties, status) and a relay mode that forwards them unchanged #4926 is declined and closed. The m1 quest ietf-object-gaps makes its refusals loud.

Both Grok reviews' findings are addressed. The follow-up review's nit (the overflow close is Rust-only in the Goal) is fixed in 70503e0.

(Written by Claude Opus 5.5)

[kixelated]

Automated review: quest import of #4951, #4950, #4945, #4847; decline of #4926

Reviewed head: 70503e0c2b965b3ffa2755bfb79857e7c62bd6fb (quest files only). I checked the claims against main (8ecd2cad) and against the draft-ietf-moq-transport-22 text.

Almost everything checks out:

• Draft-22 §9.20.9 lists exactly the six Location Filter Types the quest names, §3.4 still says a "zero-length" filter inside FILL_PARAMETERS means the whole track, the A.1 changelog's only wire change is LOCATION_FILTER (feat(moq-net): add OriginProducer::dynamic + infallible OriginConsumer::request_broadcast #1913, moq-net: track announcement byte usage in stats #1953), and §11.3.1 does require PROTOCOL_VIOLATION when an Object ID would exceed 2^64 - 1.
• Our draft-20+ Next Object goes out as 21 02 00 00, which a draft-22 peer reads as type 0x02, an absolute start at {0, 0}. That matches the Goal.
• In Rust, next_object_id, the generic Err arm in recv_group, stop_on_error being stream-only, and run_subscribe_namespace's protocol-violation route all match main. So do JS Frame.decode and handleGroup.
• Recover::poll, Consumer::poll_stale (track.rs:2680), and drifted match. So do publish_stalled and track.modify() in moq-mux and the consumer.ts promotion guard. I walked both startup-hole paths through the code, and both stall the way the quest describes.

No blocking issues. Non-blocking findings, most important first:

1. quest/m1/ietf-object-gaps.md: the JS overflow reasoning doesn't hold. "JS refuses any non-zero delta before adding it, so it has no overflow to close on" describes JS's arithmetic, not the wire rule. §11.3.1 makes an overflowing ID a session-level PROTOCOL_VIOLATION whether or not the receiver does the addition. So the same input (for example, a second object with Object ID Delta 2^64 - 1) would close the session in Rust and only stop the stream in JS. That contradicts the Goal's "refusals must be consistent." Either have JS check prior + delta + 1 > 2^64 - 1 before the gap refusal, or say plainly that JS diverges on purpose. A related test detail: the Rust overflow can only happen on draft-17+, where the leading-ones varint carries the full u64. The QUIC form tops out at 2^62 - 1 (coding/varint.rs), so a delta that large can't be sent there. The overflow test needs a draft-17+ version.

2. quest/m0/ietf-location-filter-22.md: some test pointers are off.

   • fetch.rs doesn't contain 21 03 04 00 02. The pinned bytes are 0x01, 0x03, 0x04, 0x00, 0x02, because the id is delta-encoded from 0x20 (fetch.rs:914). Grepping for the quoted bytes finds nothing.
   • I couldn't find a Draft22 LOCATION_FILTER byte pin in subscribe.rs. Its Draft20 to Draft22 loops don't carry 0x21.
   • One pin that isn't listed: publisher.rs legal_requests_we_do_not_serve_are_refused_per_request sends 0x17, 0x01, 0x01 // LOCATION_FILTER (0x21): from one group back on Draft20 to Draft22. It happens to decode the same on draft-22 (type 0x01, StartGroup 1), so it keeps passing. But on Draft22 it would no longer test the length-inferred form its comment describes. Worth a mention, so nobody assumes it covers the new encoding.
   • encodeFill and FILL_ALLOWED live in js/net/src/ietf/filter.ts, not parameters.ts, which the paragraph implies.

3. quest/m0/expiry-wakes.md, minor. The moq-net: a group held across a route switch is not woken when its successor gets its first timestamp #4950 reporter attached a small tested patch: one shared TrackState registration used by both GroupExpiry::is_expired and Consumer::poll_stale, passing 20 of 20. The Plan deliberately chooses the index instead, but folding Recover in adds two new cases to an m0 Required quest: entries with no deadline yet, and entries that move across tracks on a route change. A sentence noting the reporter's patch as a reference for the implementer, or as a fallback if the index slips, would help.

CI: Check and Test were pending at review time. The PR only touches quest files, so I don't expect them to matter.

Verdict: MERGE

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