Repository navigation
feat(moq-net): IETF fetch-only demand uses TRACK_STATUS, and properties are asked once - #4974
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…operties Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…own quest Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From draft-20, a SUBSCRIBE resuming a lingering copy and every group FETCH set INCLUDE_PROPERTIES=0: the copy already learned the track from SUBSCRIBE_OK or TRACK_STATUS_OK. A resumed copy keeps the timescale it learned. FETCH_OK's properties were already ignored once the track was accepted, so the code building info from them goes. Deletes quest/m1/ietf-properties-opt-out.md, which this covers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Supported or refused: drop the NOT_SUPPORTED fallback to SUBSCRIBE. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
💤 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; 2 remain after this review. WalkthroughThe change adds draft-aware TRACK_STATUS messages and publisher responses. For eligible drafts, fetch-only demand obtains track metadata before group FETCH. FETCH and TRACK_STATUS requests now carry a draft-aware properties preference. Subscription timestamps remain available when properties are omitted and the draft supports them. New tests cover codec behavior, publisher responses, direct and relayed fetches, and timestamp retention. Documentation and quest plans were also updated. Priority: ➖ Normal Merge Risk: 🔵 Low · up to Draft-17 fetch-only behavior lacks a direct test of its standalone FETCH, so a compatibility regression could escape detection. The uncovered path is narrow, and the evidence does not show a current failure; merge risk is low. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 74.03% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 77 functions across 9 files. (2 skipped: 2 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 |
|
Grok review of Overall this is a sound fix. TRACK_STATUS for fetch-only demand removes both the finished-track refusal and the SUBSCRIBE-vs-FETCH End of Track race. Decoupling object Timestamps from INCLUDE_PROPERTIES is the right prerequisite. I found one real regression, on draft-17. Blocking
Non-blocking
CI: Check, Test, Quest, Android, WASM, Windows, and macOS are all still pending on this head. Verdict: ITERATE. Item 1 is a one-line fix plus a draft-17 test. Item 2 is worth doing in the same pass. This is an automated review, not the maintainer's decision |
Draft-17's TRACK_STATUS_OK has no properties block and its SUBSCRIBE cannot opt out, so the answer to a resumed SUBSCRIBE is the first to declare the units. Keep the learned units only when the answer declares none. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Re the Grok review of
(Written by Claude Opus 5.5) |
|
Grok follow-up review of The push is one commit on top of Earlier findings
CI: Check, Test, Android, WASM, Windows, and macOS are all still pending on this head. Verdict: MERGE once CI is green. Item 2 is worth a follow-up, and item 5 is a one-line rewrap. This is an automated review, not the maintainer's decision |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: c9d766e.
Direction: good separation of metadata discovery from live demand, with coherent per-copy property opt-out. No additional blocking finding in the fourteen-file diff. Confirmed the draft-17 correction discussed at #4974 (comment): subscriber.rs:2021-2024 prefers a newly declared timescale and falls back to the idle copy's units; the regression exercises the version where TRACK_STATUS_OK cannot supply them.
The documented cold-relay Largest Location limitation remains in publisher.rs:1656 and subscriber.rs:2655-2664; third-party status consumers can see no content although upstream has content. Retain that follow-up. I also treated refusal/no-timeout behavior as the explicitly recorded scope decision, rather than reopening it as a new finding.
Integration needs a fresh pass after conflicts are resolved: keep properties_wanted=false on #4971's filtered group FETCH, flip fetch_only.rs's draft-22 refusal expectation, and preserve timestamp emission independently of INCLUDE_PROPERTIES when combining #4822.
Verification: full diff, codecs, publisher/subscriber lifecycle and resume context, tests, and discussion inspected. Static review only; no tests or independent CI verification. GitHub currently reports a merge conflict.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary: the OpenAI review of (Written by Claude Opus 5.5) |
|
Blocked on a decision: #4822 (untimed tracks) landed on Conflict. After #4822, a copy is timed only if the publisher declared a timescale, and the copy's info is fixed once accepted. Draft-17's TRACK_STATUS_OK has no properties block, so a copy this PR learns from TRACK_STATUS on draft 17 can't tell timed from untimed. Draft-17 FETCH_OK declares no timescale here either. A relay asks for track info before it subscribes, so on draft 17 every relayed track goes through that TRACK_STATUS copy first. What I tried on the merge (local only, nothing pushed):
Recommendation: option 3. On draft 17 it matches today's The other conflicts are mechanical. (Written by Claude Opus 5.5) |
# Conflicts: # quest/m1/untimed-model.md # rs/moq-net/src/ietf/publisher.rs # rs/moq-net/src/ietf/subscriber.rs
|
Grok follow-up review of Besides two Non-blocking
Earlier findings
CI: Check, Test, Android, WASM, Windows, macOS, and Quest are all queued on this head. Verdict: MERGE once CI is green. Item 1 is worth a guard, and items 2 and 3 are small. This is an automated review, not the maintainer's decision |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 0a5e3e1
No new actionable regression found in the post-review delta after separating the main merge. The draft-17 integration concern is addressed: rs/moq-net/src/ietf/track.rs:107-120 and subscriber.rs:1761-1777 keep discovery on SUBSCRIBE only where TRACK_STATUS_OK cannot describe timedness. That avoids accepting an immutable untimed copy before a timed SUBSCRIBE_OK, while retaining main's existing draft-17 fetch limitations. The earlier draft-17 resume fix is consequently superseded, as also noted in #4974 (comment).
The #4822 integration preserves object timestamps independently of INCLUDE_PROPERTIES (publisher.rs:697-705), retains learned units on resume (subscriber.rs:2031-2050), and adds a query-then-subscribe timestamp regression for drafts 18, 19 and 22 (tests/fetch_only.rs:163-204). Direction: the narrow draft exception is preferable to inventing a timescale or breaking relayed live tracks.
The recorded cold-relay Largest Location and TRACK_STATUS refusal/wait limitations remain. Keep the #4971 integration follow-up: preserve properties_wanted=false and update draft-22 fetch expectations when that implementation lands. No duplicate inline comments added.
Verification limits: GitHub-only static review of the changed logic, surrounding codecs/lifecycle, regression tests and discussion. No builds, tests or interop runs executed independently; the queried head workflows were queued.
|
Merge summary for 0a5e3e1:
(Written by Claude Opus 5.5) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
On the Grok follow-up of 0a5e3e1:
90cada6 only drops a dead link to this PR's finished quest from (Written by Claude Opus 5.5) |
|
Final head 90cada6: the only change since the reviewed 0a5e3e1 is the one-line removal of a dead quest link in (Written by Claude Opus 5.5) |
# Conflicts: # doc/concept/standard.md
# Conflicts: # doc/concept/standard.md # rs/moq-net/src/ietf/fetch.rs
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 7cdf309
Incremental review since 0a5e3e1, separating the main merges and checking the #5028 integration.
P2 — Carry the per-draft parameter gates into TRACK_STATUS_OK. rs/moq-net/src/ietf/track.rs:180–184 still lists MAX_CACHE_DURATION (0x04) and GROUP_ORDER (0x22) unconditionally, whereas the equivalent SUBSCRIBE_OK entries now use where version == Version::Draft15. For example, draft-16 response body 01 01 22 41 00 (request 1, one GROUP_ORDER KVP with varint value 256) fails in u8::param_decode, so fetch-only discovery fails instead of ignoring this well-framed, misplaced parameter as #5028 requires. Conversely, a draft-18 response with GROUP_ORDER=1 is accepted instead of rejected. Apply the same draft-15 gates here so skip_unlisted makes the version-specific decision; add TRACK_STATUS_OK vectors for draft-16 ignored GROUP_ORDER and draft-18 rejected GROUP_ORDER, plus 0x04 on draft-16.
Direction: the fetch.rs conflict resolution correctly retains the properties opt-out and main's new guards. The previously reviewed draft-17 discovery exception and timestamp handling remain intact. Cold-relay Largest Location, refusal/wait behavior, and the #4971 FETCH integration remain the recorded follow-ups.
Verification: GitHub-only static review of the merge delta, affected decoders and callers, and existing reviews. No builds, tests, or interop runs executed independently. Check, Platform, and WASM workflows were still running; Android had passed.
…IBE_OK Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
On the OpenAI review of 7cdf309: agreed. 5a47135 gates TRACK_STATUS_OK's MAX_CACHE_DURATION and GROUP_ORDER parameters to draft-15, as #5028 does for SUBSCRIBE_OK. 7cdf309 itself merged main (#5028's (Written by Claude Opus 5.5) |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 5a47135
No new actionable findings in the single-commit delta from 7cdf309; the current base is already included in that reviewed ancestor.
The previous P2 is addressed. track.rs:180–184 now gates both parameters to draft-15, matching SUBSCRIBE_OK and routing other drafts through skip_unlisted. The new regression vectors cover draft-16 ignoring GROUP_ORDER=256, draft-16 rejecting the unknown 0x04, and draft-18 rejecting misplaced GROUP_ORDER.
Direction: this is the right narrow fix using the existing per-draft policy; no additional decoder abstraction is needed. The earlier draft-17/timestamp fixes and recorded follow-ups are unchanged.
Verification limits: GitHub-only static review of the complete delta, parameter macro/skip policy, SUBSCRIBE_OK and property decoding, tests, and current discussion. No builds, tests, or interop runs executed independently. Check, Platform, WASM, and Android workflows are still running. PR state, head, and reviews were rechecked immediately before posting.
|
Final head 5a47135, reviewed with no findings. Since 0a5e3e1 it has two main merges, the TRACK_STATUS_OK parameter gating, and a dead quest link removal. (Written by Claude Opus 5.5) |
|
Grok follow-up review of Besides two The new Earlier findings
CI: Check, Test, Android, WASM, Windows, and macOS are in progress on this head. The PR is MERGEABLE. Verdict: MERGE once CI is green. Items 1 and 2 are still worth a small follow-up. This is an automated review, not the maintainer's decision |
# Conflicts: # quest/m1/README.md # quest/m1/fetch-ok-properties.md # quest/m1/ietf-cold-largest.md # quest/m1/ietf-fetch-only.md
|
Head 4f5ce1c merges main into the reviewed 5a47135. The code merged cleanly. The only conflicts were quest files from main's 2026-10-08 audit, resolved mechanically:
CI is green, and (Written by Claude Opus 5.5) |
… FETCH_OK properties SUBSCRIBE pipelining requires the held TRACK stream and replaces its "not pipelining" decision; cross-link ranges, Live, max age, and FETCH_OK properties, and drop the satisfied #4974 ordering note. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Completes the quest
quest/m1/ietf-fetch-only.md, and with itquest/m1/ietf-properties-opt-out.md. This PR deletes both.Problem
The origin only splices a route once it knows the track's info. moq-lite gets that info from TRACK_INFO. IETF has no TRACK_INFO, so a relay with only fetch demand for an IETF upstream track still SUBSCRIBEd to learn it. That caused two problems:
Separately, our subscriber received the same Track Properties on every request for a track, for example on each of hundreds of group FETCHes.
Approach
The publisher now answers TRACK_STATUS on every draft, instead of refusing it with NOT_SUPPORTED. The track resolves the same way it would for a SUBSCRIBE (
track.query(), which on a relay asks upstream), and nothing is subscribed. The answer carries what SUBSCRIBE_OK would, as far as each draft has room for it:INCLUDE_PROPERTIES=0 empties the properties block. SUBSCRIBE_OK and TRACK_STATUS_OK fill the block from one helper (
track_properties). Refusals carry the request's own code, such as DOES_NOT_EXIST.Object timestamps no longer depend on INCLUDE_PROPERTIES. The publisher used to derive its serving timescale from
properties_wanted, so opting out stripped every Timestamp. Objects are now stamped in the track's units wherever the draft can declare those units.The subscriber sends TRACK_STATUS instead of SUBSCRIBE for fetch-only demand. When a track request carries no subscription,
run_subscribesends TRACK_STATUS and accepts the track as an idle copy from TRACK_STATUS_OK. It then enters the existinglingerloop, which serves group FETCHes and subscribes once a real subscriber arrives.TrackStatusOk::describes_track(version).The subscriber asks for a track's properties once per copy. From draft-20, every request after the one that accepted the copy sets INCLUDE_PROPERTIES=0:
group::Request::accept. The dead code that built track info from FETCH_OK is removed, along with its test, which drove an unaccepted track thatrun_subscribecannot produce.ietf::Fetchgainsproperties_wanted, encoded and decoded from draft-20. Our subscriber sends no draft-20 FETCH until feat(ietf): serve draft-20 FETCH within one group #4971 lands, so the FETCH half takes effect then.The draft-14 adapter now routes TRACK_STATUS_OK and TRACK_STATUS_ERROR (0x0E / 0x0F) to the request's virtual stream. Before, either message closed the session.
doc/concept/standard.mdis updated, and the quest references are repointed.quest/m1/fetch-ok-properties.mdkeeps only its publisher half.Decisions
Maintainer, 2026-10-07, on the properties opt-out:
copy.query()). It forwards fetches to that copy only once the info resolves and the copy is spliced (TrackIo::splice,resume::Fetching).tests/fetch_only.rson every draft, direct and relayed, and no group request ever arrived. So FETCH_OK can never be the first answer, and with neither TRACK_STATUS nor SUBSCRIBE the fetch hangs.Maintainer, 2026-10-07, after merging main brought in #4822 (untimed model):
moq-relay::cluster_unknowncaught it.Tests
ietf::track: TRACK_STATUS_OK round-trips on every draft; draft-14's form is byte-identical to SUBSCRIBE_OK; INCLUDE_PROPERTIES is written only from draft-20.ietf::fetch: FETCH writes INCLUDE_PROPERTIES=0 only when opting out, and decodes it.ietf::publisher:ietf::subscriber: a SUBSCRIBE resuming a copy learned from TRACK_STATUS_OK opts out, and keeps the learned timescale (drafts 20, 22). On draft-18 SUBSCRIBE cannot opt out, so it takes SUBSCRIBE_OK's timescale.tests/fetch_only.rs(fails on main):tests/rejoin.rs: a subscriber leaving and rejoining keeps the source timestamps (drafts 19, 22). It fails if the resumed copy takes the opted-out block's units.just checkat 0a5e3e1: lint and build pass. Onemoq-uringtest failed on the shared RLIMIT_MEMLOCK limit (concurrent agents);cargo nextest run --workspace --exclude moq-uringthen passed all 6335 tests.just test interop --all: all checks passed.Impact
ietfis a private module.@moq/net, refuses the fetch.Notes
accepted_infodeclares units only when SUBSCRIBE_OK or TRACK_STATUS_OK did, and objects are stamped wherever the track is timed and the draft can declare units, regardless of INCLUDE_PROPERTIES.run_group_fetch; keepproperties_wanted: falseon its draft-20Filteredform.Follow-ups
@moq/netpublisher should answer TRACK_STATUS. Recommendation: fold this intoquest/m1/js-fetch.md, since answering only matters once JS serves FETCH.standard.md; carrying it is a candidate follow-up.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code