Skip to content

feat(moq-net): IETF fetch-only demand uses TRACK_STATUS, and properties are asked once - #4974

Merged
kixelated merged 24 commits into
mainfrom
quest/m1/ietf-fetch-only
Oct 8, 2026
Merged

kixelated merged 24 commits into
mainfrom
quest/m1/ietf-fetch-only

Conversation

@kixelated

@kixelated kixelated commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Completes the quest quest/m1/ietf-fetch-only.md, and with it quest/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:

  • A finished upstream track refuses the SUBSCRIBE, so none of its groups could be fetched.
  • The live subscription raced the group FETCHes. End of Track is known only once an upstream FETCH_OK reports it, so a downstream FETCH running to the end could answer before that and leave End of Track unset. This is the moxygen "FETCH with large objects" flake.

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:

    • draft-14: TRACK_STATUS_OK (0x0E), which is the SUBSCRIBE_OK body with Track Alias 0.
    • drafts 15-17: REQUEST_OK with LARGEST_OBJECT, plus GROUP_ORDER on draft-15.
    • draft-18 and later: the same, plus the Track Properties block.

    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_subscribe sends TRACK_STATUS and accepts the track as an idle copy from TRACK_STATUS_OK. It then enters the existing linger loop, which serves group FETCHes and subscribes once a real subscriber arrives.

    • Draft-17 is the exception and still SUBSCRIBEs, gated by TrackStatusOk::describes_track(version).
    • A publisher that refuses TRACK_STATUS, NOT_SUPPORTED included, refuses the fetch-only request. There is no SUBSCRIBE fallback.
    • The wait ends if the demand goes away or the session ends.
  • 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:

    • A resumed SUBSCRIBE (a subscriber returning to a lingering copy) opts out and keeps the timescale the copy already learned, instead of taking the empty block.
    • Every group FETCH opts out. A group FETCH only ever runs on a copy already accepted from SUBSCRIBE_OK or TRACK_STATUS_OK, so FETCH_OK's properties were already ignored by group::Request::accept. The dead code that built track info from FETCH_OK is removed, along with its test, which drove an unaccepted track that run_subscribe cannot produce.
    • ietf::Fetch gains properties_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.md is updated, and the quest references are repointed. quest/m1/fetch-ok-properties.md keeps only its publisher half.

Decisions

Maintainer, 2026-10-07, on the properties opt-out:

  • Backward compatibility with moq-transport peers that refuse TRACK_STATUS or mishandle INCLUDE_PROPERTIES:
    • Keep it as a constraint, and opt out only after a peer answered TRACK_STATUS.
    • ✅ Not a constraint. moq's own libraries prefer moq-lite client to server, and an implementation without INCLUDE_PROPERTIES is at fault.
  • TRACK_STATUS alongside SUBSCRIBE:
    • Send TRACK_STATUS in parallel with every SUBSCRIBE and FETCH (the 2026-10-01 plan).
    • ✅ No. SUBSCRIBE_OK carries the properties. TRACK_STATUS stays for fetch-only demand.
  • Repeated properties:
    • Keep receiving them on every request.
    • ✅ Once the session knows a track's properties, later requests for it send INCLUDE_PROPERTIES=0, if that keeps the code clean. It did: the copy that holds the known properties is the same one every later request is made for.
  • Not sending TRACK_STATUS at all (later on 2026-10-07):
    • Fetch-only demand never reaches the subscriber as a group request before the track's info is known. Every consumer reads through an origin, and the origin front only asks a route's copy for its info (copy.query()). It forwards fetches to that copy only once the info resolves and the copy is spliced (TrackIo::splice, resume::Fetching).
    • A probe that waited for a group request instead of sending TRACK_STATUS timed out tests/fetch_only.rs on 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.
    • ✅ Keep TRACK_STATUS for fetch-only demand (only when nobody subscribes), and keep the INCLUDE_PROPERTIES opt-out after the first answer.
  • NOT_SUPPORTED fallback to SUBSCRIBE:
    • Keep it for publishers that refuse TRACK_STATUS.
    • ✅ Drop it, with its test: supported or refused. A publisher that refuses TRACK_STATUS refuses the fetch-only request.
  • The extra round trip before the first FETCH (TRACK_STATUS, then FETCH):
    • ✅ Out of scope here. A separate m1 quest removes it by pipelining the info request with the FETCH.

Maintainer, 2026-10-07, after merging main brought in #4822 (untimed model):

  • Draft-17 fetch-only demand:
    • Draft-17's REQUEST_OK answer to TRACK_STATUS has no properties block, while its SUBSCRIBE_OK declares TIMESCALE. A copy learned from TRACK_STATUS was accepted untimed, and the next SUBSCRIBE_OK fed stamped frames into it ("frame timestamp doesn't match track timescale"). That broke every relayed draft-17 track, because the origin front asks for a track's info before it forwards a subscription. moq-relay::cluster_unknown caught it.
    • Refusing fetch-only demand on draft-17 does not work for the same reason: upstream, a relayed live subscribe looks like fetch-only demand.
    • ✅ Draft-17 keeps SUBSCRIBE for demand with no subscriber, as before this PR. It is not an interop target, so it keeps the old race and its finished-track refusal.

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:
    • TRACK_STATUS is answered on every draft, with and without the opt-out, and subscribes nothing.
    • A missing broadcast is refused with the draft's code.
    • A draft-20 SUBSCRIBE with INCLUDE_PROPERTIES=0 still stamps its objects. This test fails on main.
  • 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):
    • On drafts 14-16 and 18-19, a finished track is fetched directly and the publisher never sees a subscription. On draft-22 nothing is subscribed either, but its FETCH is still refused until feat(ietf): serve draft-20 FETCH within one group #4971 lands.
    • Through a relay, a downstream fetch never puts a subscription upstream.
    • A SUBSCRIBE after a TRACK_STATUS still delivers timestamped frames (drafts 18, 19, 22).
    • Draft-17 is excluded: it still subscribes.
  • 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 check at 0a5e3e1: lint and build pass. One moq-uring test failed on the shared RLIMIT_MEMLOCK limit (concurrent agents); cargo nextest run --workspace --exclude moq-uring then passed all 6335 tests.
  • just test interop --all: all checks passed.

Impact

  • Public API: none. ietf is a private module.
  • Wire:
    • The Rust publisher answers TRACK_STATUS instead of refusing it.
    • A SUBSCRIBE with INCLUDE_PROPERTIES=0 now gets stamped objects.
    • The Rust subscriber sends TRACK_STATUS before serving fetch-only demand, except on draft-17. A publisher that refuses it, such as today's @moq/net, refuses the fetch.
    • From draft-20, the Rust subscriber sends INCLUDE_PROPERTIES=0 on a resumed SUBSCRIBE and on every group FETCH.
    • No encoding changed, and none of our drafts change.

Notes

  • On draft-16, REQUEST_OK has no properties block, so a fetch-only copy there gets no max cache duration or priority. FETCH_OK cannot fill that in later, since the track's info is fixed once it is accepted.
  • Merged with feat(net)!: carry untimed tracks faithfully #4822 (untimed model): accepted_info declares 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.
  • feat(ietf): serve draft-20 FETCH within one group #4971 will conflict on the group FETCH literal in run_group_fetch; keep properties_wanted: false on its draft-20 Filtered form.

Follow-ups

  • The @moq/net publisher should answer TRACK_STATUS. Recommendation: fold this into quest/m1/js-fetch.md, since answering only matters once JS serves FETCH.
  • A relay with no subscription upstream answers TRACK_STATUS with the Largest Location it has cached, since an idle copy drops the upstream TRACK_STATUS_OK's. Our subscriber ignores it, but third-party clients read it. Documented in standard.md; carrying it is a candidate follow-up.
  • A new m1 quest pipelines the track info request with the first FETCH, removing the TRACK_STATUS round trip a fetch-only request now waits on.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

kixelated and others added 8 commits October 7, 2026 00:03
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>
@kixelated kixelated changed the title feat(moq-net): fetch-only IETF demand learns the track from TRACK_STATUS feat(moq-net): IETF fetch-only demand uses TRACK_STATUS, and properties are asked once Oct 7, 2026
Supported or refused: drop the NOT_SUPPORTED fallback to SUBSCRIBE.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review October 7, 2026 18:35
@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 49bc4aaa-9a46-4981-8dbc-ecae19d93afb
📥 Commits

Reviewing files that changed from the base of the PR and between 5a47135 and 4f5ce1c.

📒 Files selected for processing (4)
  • quest/m1/README.md
  • quest/m1/fetch-ok-properties.md
  • quest/m1/ietf-fetch-only.md
  • quest/m2/pipeline-fetch-info.md
💤 Files with no reviewable changes (2)
  • quest/m1/README.md
  • quest/m1/ietf-fetch-only.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.


Walkthrough

The 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 4f5ce

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main changes: IETF fetch-only demand uses TRACK_STATUS, and repeated track properties are avoided.
Description check ✅ Passed The description is directly related to the changeset. It explains the TRACK_STATUS implementation, fetch-only behavior, properties opt-out, draft-specific behavior, tests, and follow-up work.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

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.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
✨ Simplify code
  • Commit to this branch
  • Create a new PR
  • 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

Comment @coderabbitai help to get the list of available commands.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Grok review of 793ae1b2 (full review: first Grok pass, the PR was a draft until now)

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

  1. A copy learned on draft-17 loses its Timestamps when a subscriber resumes it. See rs/moq-net/src/ietf/subscriber.rs:2022-2025. The resumed path now always takes idle.timescale and ignores the fresh SUBSCRIBE_OK's TIMESCALE. That is only correct when the SUBSCRIBE actually opted out, which happens from draft-20 on (Subscribe::encode gates 0x35 on Filter::is_draft20). On draft-17:
    • TrackStatusOk::has_properties is false, so a fetch-only copy is accepted with timescale: None (subscriber.rs:2663).
    • A subscriber then arrives, linger resumes, and the SUBSCRIBE goes out without the opt-out. The publisher answers with a full block including TIMESCALE (sends_timescale is true for 17), and it stamps every object (publisher.rs:698).
    • The subscriber discards that TIMESCALE and keeps None, so every live object is stamped on arrival instead of with its source time.
    • Before this PR, draft-17 fetch-only demand SUBSCRIBEd and learned the units, so this is a regression. The fix is one line: let timescale = timescale.or(resumed.as_ref().and_then(|idle| idle.timescale));. A fresh declaration wins, and the learned one fills in for an opted-out empty block.
    • tests/rejoin.rs::rejoin_keeps_source_timestamps doesn't catch it, because it only runs drafts 19 and 22, and its first join is a real SUBSCRIBE. A draft-17 case that fetches first and then subscribes would cover it.

Non-blocking

  1. A subscriber that joins while TRACK_STATUS is in flight is rejected along with the fetch. track_status() decides the path from request.subscription() once, before sending. If a live subscriber joins during the wait, three things follow:
    • It pays the full TRACK_STATUS round trip before any SUBSCRIBE.
    • If the publisher refuses TRACK_STATUS (today's @moq/net, or any third-party publisher without it), request.reject(err) at subscriber.rs:2649 fails that subscriber too, even though a SUBSCRIBE would have served it.
    • Since track::Request::subscription() is live, a cheap fix is to check it again on refusal and fall through to Target::Request(request) when someone is now subscribed. That keeps the "no SUBSCRIBE fallback" decision for pure fetch-only demand.
    • The wait also has no timeout. A peer that silently drops TRACK_STATUS holds a joined live subscriber until the session or the demand ends.
  2. A relay's TRACK_STATUS_OK reports no Largest Location for a cold copy. run_track_status_stream answers largest: live_edge(&track).largest (publisher.rs:1656), which is just the local cache. On a relay, a copy accepted from an upstream TRACK_STATUS_OK drops ok.largest (subscriber.rs:2656), so a downstream TRACK_STATUS for a track with content answers "no content". Our own subscriber ignores largest, so moq-to-moq is fine. But Largest Location is the main thing third-party clients send TRACK_STATUS for. It's worth carrying ok.largest on the idle copy, or at least noting the limitation in standard.md.
  3. Opted-out SUBSCRIBEs now get Timestamps in units that were never declared to them. This is intentional, and fine for our subscriber, which learned the units first. A third-party draft-20+ subscriber that opts out without ever sending TRACK_STATUS now receives Timestamps it has no TIMESCALE for, where before it received none. The standard.md paragraph says this. Just flagging it as a wire-behavior change for peers.
  4. Tiny: the new doc/concept/standard.md paragraph has one unwrapped long line ("A publisher that refuses TRACK_STATUS refuses the fetch. The Rust publisher answers..."). Also, "TRACK_STATUS to a JavaScript publisher" sits in a list that otherwise describes what the Rust publisher refuses, which reads oddly.

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
(Written by Grok)

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>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Re the Grok review of 793ae1b2:

  1. Fixed in ba732bb. A resumed copy now keeps its learned units only when SUBSCRIBE_OK declares none: timescale.or(idle.timescale). a_resumed_subscribe_opts_out_of_known_properties gains a draft-17 case (no properties on TRACK_STATUS_OK, units on SUBSCRIBE_OK). It fails without the fix.
  2. Declined. Supported or refused is the maintainer's call here: a publisher that refuses TRACK_STATUS refuses the request, and the TRACK_STATUS round trip is going away in a separate quest that pipelines the info request with the FETCH. No timeout either: the wait already ends when the demand goes away or the session closes, and a timeout would only paper over a peer that never answers.
  3. Documented in doc/concept/standard.md: a relay with no subscription upstream reports the Largest Location it has cached. Carrying the upstream Largest Location on an idle copy is a follow-up, since our subscriber ignores it.
  4. Intentional, as noted.
  5. Rewrapped. The NOT_SUPPORTED list names requests that are refused, and TRACK_STATUS to a JavaScript publisher is one, so that line stays.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Grok follow-up review of ba732bbe (re-review after a push; last Grok review was on 793ae1b2, comment)

The push is one commit on top of 793ae1b2. It fixes the blocking draft-17 regression and adds a test that would have caught it. I found no new issues in it.

Earlier findings

  1. Fixed: a resumed draft-17 copy dropped SUBSCRIBE_OK's TIMESCALE. rs/moq-net/src/ietf/subscriber.rs:2024 is now timescale.or(resumed…idle.timescale), so a fresh declaration wins and the learned units only fill in for an empty, opted-out block. That's right on every draft. On draft-17 and 18–19 the SUBSCRIBE can't opt out, so SUBSCRIBE_OK declares the units. From draft-20 the opted-out block is empty and the learned units are kept. a_resumed_subscribe_opts_out_of_known_properties now runs Draft17, too, with a TRACK_STATUS_OK that has no timescale and a SUBSCRIBE_OK that declares it. It asserts the copy ends up with Some(declared), which fails on the old code. The properties_wanted == !opts_out assert is also sound. The resumed SUBSCRIBE still sets properties_wanted: false (line 1867), but the encoder only writes 0x35 from draft-20 (subscribe.rs:190), so on draft-17 it decodes as the default true.
  2. Still open (non-blocking): a subscriber that joins while TRACK_STATUS is in flight. It still waits out the round trip, it's still rejected with the fetch if the publisher refuses TRACK_STATUS (today's @moq/net, for example), and the wait still has no timeout. Re-checking request.subscription() on refusal and falling through to a SUBSCRIBE would fix the rejection. This is fine as a follow-up.
  3. Addressed in docs: standard.md now says a relay with no upstream subscription reports the Largest Location it has cached. The upstream ok.largest still isn't carried on the idle copy, but the limitation is now stated.
  4. Informational only, no change expected: opted-out third-party subscribers get Timestamps in units declared by TRACK_STATUS.
  5. Partly fixed: the long line from the last review was rewrapped. The new sentence put another unwrapped line in its place, at doc/concept/standard.md:70 ("…reports the Largest Location it has cached. A SUBSCRIBE that sets…"). Also, "TRACK_STATUS to a JavaScript publisher" (line 107) still sits in the list of what the Rust side refuses.

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
(Written by Grok)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge summary: the OpenAI review of c9d766e4 found nothing blocking. Since then only ac1e723a landed, a cargo fmt reflow of one test assertion that was failing Check. The cold-relay Largest Location limitation it notes stays a recorded follow-up. Auto-merge is enabled on ac1e723a.

(Written by Claude Opus 5.5)

@kixelated
kixelated enabled auto-merge (squash) October 7, 2026 23:23
@kixelated

Copy link
Copy Markdown
Collaborator Author

Blocked on a decision: #4822 (untimed tracks) landed on main and conflicts with this PR in a way the merge can't settle mechanically.

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):

  1. Take TRACK_STATUS_OK's timescale as given, so the draft-17 copy is untimed. moq-relay::cluster_unknown every_ingest_version_crosses_a_redundant_mesh fails deterministically for moq-transport-17 with "group closed before a frame arrived". The later SUBSCRIBE_OK declares units, but the copy is already untimed. The test passes on main.
  2. Assume a draft-17 TRACK_STATUS copy is timed. The relay test passes, but fetch-only on draft 17 (fetch_only.rs) fails with NotFound: fetched objects have no units, and the timed copy refuses them. That also contradicts feat(net)!: carry untimed tracks faithfully #4822's rule that a standalone FETCH with no declared units is untimed.
  3. Keep SUBSCRIBE for discovery on draft 17, and use TRACK_STATUS only where TRACK_STATUS_OK can declare timedness (drafts 14-16 and 18+). The relay test passes. The cost is that draft-17 fetch-only demand subscribes again, as it does on main. That flips draft 17's expectation in fetch_only.rs and drops the draft-17 case of a_resumed_subscribe_opts_out_of_known_properties.

Recommendation: option 3. On draft 17 it matches today's main behavior, and draft 17 can't do better without units on the wire. Options 1 and 2 each break something that works on main. All three need a fresh review.

The other conflicts are mechanical. untimed-model.md takes main's deletion. track_properties and accepted_info carry Option<Timescale>. The publisher stamps objects with track.info().timescale.filter(sends_timescale), independent of INCLUDE_PROPERTIES, as the OpenAI review asked.

(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
@kixelated

Copy link
Copy Markdown
Collaborator Author

Grok follow-up review of 0a5e3e19 (re-review after a push; last Grok review was on ba732bbe, comment)

Besides two origin/main merges, the push has four PR commits. 3ad859e3 rewraps the doc and ac1e723a is cargo fmt. e0a40d2d adapts to main's epoch argument and untimed frames: the TRACK_STATUS handler now calls request_broadcast(&msg.track_namespace, None), like the other publisher call sites. 0a5e3e19 is the one that changes behavior: on draft-17, fetch-only demand goes back to SUBSCRIBE, because draft-17's REQUEST_OK has no properties block and now that main has untimed tracks, a copy learned from it would be untimed for good. TrackStatusOk::describes_track (track.rs:110) chooses the right drafts (14–16 and 18+ use TRACK_STATUS, 17 subscribes), and the new a_subscribe_after_track_status_keeps_timestamps test pins the draft-18+ path. No blocking issues.

Non-blocking

  1. The mismatch this push avoids on draft-17 can still happen on drafts 18–19. On resume, subscriber.rs:2033 takes timescale.or(resumed.timescale), so a non-empty SUBSCRIBE_OK wins and its value becomes held.timescale (line 2078). But the copy itself keeps the timed or untimed state it got from TRACK_STATUS_OK. A third-party draft-18/19 publisher could send a REQUEST_OK with no TIMESCALE and a SUBSCRIBE_OK that declares one. Then ingest decodes stamped objects (line 4180) into an untimed track, and create_frame_owned fails each one with TimestampMismatch (model/group.rs:343). On draft-20+ the same publisher fails silently: the resumed SUBSCRIBE opts out, so the track stays untimed and its Timestamps are dropped. A conforming publisher won't do this, but the result is hard to diagnose. Suggested fix: when a resumed copy and the answer disagree on whether the track is timed, keep the copy's state (or abort the copy) and log a warning, instead of letting the answer's value win.
  2. Draft-17's fetch-only path has lost its test coverage. moq-transport-17 was dropped from VERSIONS in tests/fetch_only.rs, and nothing new asserts that draft-17 still subscribes and serves a fetch. A one-line unit test of describes_track for each version, or keeping draft-17 in VERSIONS with a "subscribes" flag on a live track, would catch a later change to has_properties or sends_timescale that quietly flips it.
  3. Docs: standard.md:78 says draft-17 "still subscribes", but not what that costs. On draft-17 a finished track is still unfetchable, and a relay's fetch-only demand puts a live subscription upstream again. A short clause would keep the paragraph's first sentence from overstating the guarantee.

Earlier findings

  1. Fixed earlier, and now moot on draft-17: the resumed draft-17 TIMESCALE problem. Draft-17 never resumes from TRACK_STATUS any more, and the test now covers drafts 18, 20, and 22, with TRACK_STATUS_OK always declaring the units.
  2. Still open (non-blocking): a subscriber that joins while TRACK_STATUS is in flight. It still waits for the round trip with no timeout, and it's still rejected along with the fetch if the publisher refuses TRACK_STATUS. On draft-17 this no longer applies.
  3. Fixed: the unwrapped line at doc/concept/standard.md:70 was rewrapped by 3ad859e3.
  4. Still open (nit): "TRACK_STATUS to a JavaScript publisher" (standard.md:125) still sits in the list of requests the Rust side refuses, and that line is 98 columns.

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
(Written by Grok)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge summary for 0a5e3e1:

  • Merged main, including feat(net)!: carry untimed tracks faithfully #4822 (untimed model), feat(stats)!: publish each group announcement under its own epoch #4904 (epochs) and fix(net): pass the epoch to request_broadcast in dial_split_horizon #5045. Conflicts in ietf/publisher.rs and ietf/subscriber.rs resolved by threading Option<Timescale> through accepted_info, and by stamping objects wherever the track is timed and the draft can declare units, regardless of INCLUDE_PROPERTIES. quest/m1/untimed-model.md was deleted on main, so the deletion is kept.
  • Adapted the new TRACK_STATUS path and tests to request_broadcast(path, epoch) and the optional frame timestamp.
  • Maintainer decision: draft-17 demand with no subscriber still SUBSCRIBEs (TrackStatusOk::describes_track). Its REQUEST_OK has no properties block, so a copy learned from TRACK_STATUS could not know whether it is timed, and that broke relayed draft-17 tracks. Draft-17 is not an interop target.
  • Added a regression test: a SUBSCRIBE after TRACK_STATUS still delivers timestamped frames on drafts 18, 19 and 22.
  • just check (minus the shared-memlock moq-uring tests) and just test interop --all pass locally. The OpenAI review of this head has no findings.

(Written by Claude Opus 5.5)

@kixelated
kixelated enabled auto-merge (squash) October 8, 2026 05:13
@kixelated
kixelated disabled auto-merge October 8, 2026 05:14
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

On the Grok follow-up of 0a5e3e1:

  1. A draft-18/19 publisher whose REQUEST_OK omits TIMESCALE but whose SUBSCRIBE_OK declares one is non-conforming: both answers describe the same track. The TimestampMismatch it hits fails loudly, which matches "supported or refused". Not guarding it here.
  2. Draft-17 is not an interop target, and the maintainer asked not to polish it. Its live path is still covered end to end by moq-relay::cluster_unknown every_ingest_version_crosses_a_redundant_mesh, which is the test that caught this.
  3. "Still subscribes" already names the cost, since the paragraph's first sentence explains what a SUBSCRIBE costs. Left as is.

90cada6 only drops a dead link to this PR's finished quest from quest/m1/ietf-cold-largest.md, which the Quest check flagged after the main merge.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Final head 90cada6: the only change since the reviewed 0a5e3e1 is the one-line removal of a dead quest link in quest/m1/ietf-cold-largest.md, so the review still stands. The maintainer accepted it as a trivial change. CI is green. Enabling auto-merge pinned to this head.

(Written by Claude Opus 5.5)

@kixelated
kixelated enabled auto-merge (squash) October 8, 2026 14:50
@kixelated
kixelated disabled auto-merge October 8, 2026 14:54
# Conflicts:
#	doc/concept/standard.md
#	rs/moq-net/src/ietf/fetch.rs

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@kixelated

Copy link
Copy Markdown
Collaborator Author

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. track_status_ok_parameters_follow_the_draft covers three cases: draft-16 ignores GROUP_ORDER=256, draft-16 rejects 0x04 (unknown there, as on SUBSCRIBE), and draft-18 rejects GROUP_ORDER.

7cdf309 itself merged main (#5028's where guards in fetch.rs, and the summarized standard.md) and kept this PR's INCLUDE_PROPERTIES decode and doc points.

(Written by Claude Opus 5.5)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kixelated

Copy link
Copy Markdown
Collaborator Author

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. just check (minus the shared-memlock moq-uring tests) and just test interop --all pass locally. Enabling auto-merge pinned to this head.

(Written by Claude Opus 5.5)

@kixelated
kixelated enabled auto-merge (squash) October 8, 2026 16:00
@kixelated

Copy link
Copy Markdown
Collaborator Author

Grok follow-up review of 5a471350 (re-review after a push; last Grok review was on 0a5e3e19, comment)

Besides two origin/main merges (which bring in #5028's per-draft parameter gating; the PR's own patches only change where they had to adopt main's where gates in fetch.rs and main's condensed standard.md), the push has two PR commits. 90cada61 only drops a quest link. 5a471350 changes the wire parsing: TrackStatusOk::decode_msg (rs/moq-net/src/ietf/track.rs:180-185) now gates MAX_CACHE_DURATION (0x04) and GROUP_ORDER (0x22) to draft-15, which is exactly how SubscribeOk already reads them (subscribe.rs:287-292). With the gate false, skip_unlisted applies each draft's rule: draft-16 skips 0x22 (it's in DRAFT16_MESSAGE_PARAMS) and rejects 0x04, and draft-17 and later reject both. That matches the encoder, which only ever writes 0x22 on draft-15 (track.rs:148-155). Before this, a draft-16+ answer could carry a bogus GROUP_ORDER that got validated, or even overwrote properties.group_order via the .or(group_order) on line 190. TrackStatusOk is new in this PR (main has no decoder for it), so nothing deployed reads the old, looser form, and stricter parsing breaks no compatibility.

The new track_status_ok_parameters_follow_the_draft test is valid. I checked that each assertion fails for the intended reason. On draft-18 the properties block runs to the end of the message, so [0x01, 0x22, 0x01] has a well-formed empty block, and the error has to come from the gate, not a short read. Draft-16 has no properties block on REQUEST_OK, so the 0x04 case fails in skip_unlisted too. No new issues.

Earlier findings

  1. Still open (non-blocking): on drafts 18–19, a resumed copy keeps the timed or untimed state it learned from TRACK_STATUS_OK, while subscriber.rs lets a SUBSCRIBE_OK TIMESCALE win. A publisher that disagrees between the two still produces TimestampMismatch on every frame.
  2. Still open (non-blocking): draft-17's fetch-only path still has no test. moq-transport-17 is still missing from VERSIONS in tests/fetch_only.rs, and nothing asserts that describes_track(Draft17) is false.
  3. Partly addressed by the merge's rewrite: standard.md:29 now says why draft-17 still subscribes, but not that a finished track is still unfetchable there, or that relay fetch-only demand still opens a live upstream subscription.
  4. Still open (non-blocking): a subscriber that joins while TRACK_STATUS is in flight still waits with no timeout, and it's refused along with the fetch if the publisher refuses TRACK_STATUS.
  5. Resolved: after main's rewrite, the TRACK_STATUS to a JavaScript publisher line sits under "this stack" (standard.md:33), so it's accurate now, and the line-length nit no longer applies.

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
(Written by Grok)

@kixelated
kixelated disabled auto-merge October 8, 2026 16:11
# Conflicts:
#	quest/m1/README.md
#	quest/m1/fetch-ok-properties.md
#	quest/m1/ietf-cold-largest.md
#	quest/m1/ietf-fetch-only.md
@kixelated

Copy link
Copy Markdown
Collaborator Author

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:

  • kept main's deletion of ietf-cold-largest.md
  • kept this PR's deletion of ietf-fetch-only.md
  • dropped the links to the finished quest from fetch-ok-properties.md and m2/pipeline-fetch-info.md

CI is green, and just check (minus the shared-memlock moq-uring tests) passes locally. As with 90cada6, I'm treating this quest-only delta as trivial and re-enabling auto-merge pinned to this head.

(Written by Claude Opus 5.5)

@kixelated
kixelated merged commit d4514ee into main Oct 8, 2026
9 checks passed
@kixelated
kixelated deleted the quest/m1/ietf-fetch-only branch October 8, 2026 16:47
kixelated added a commit that referenced this pull request Oct 8, 2026
… 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant