Skip to content

fix(net): resolve a lite fetch only once the publisher answers - #4164

Merged
kixelated merged 1 commit into
mainfrom
quest/m1/fetch-missing-group
Sep 26, 2026
Merged

kixelated merged 1 commit into
mainfrom
quest/m1/fetch-missing-group

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Completes the quest/m1/fetch-missing-group quest.

Problem

HTTP /fetch/<broadcast>/<track>?group=N answered 200 with an empty, cut-off body for a group the track lacks, and moq fetch --group N only failed on its first frame read.

Root cause: moq-lite has no FETCH_OK, and the lite subscriber accepted the group::Request as soon as the FETCH was flushed. So track::Consumer::fetch_group resolved to a group before the publisher answered, and the publisher's refusal (a stream reset) only surfaced as an aborted group.

Fix

  • moq-net (lite subscriber): after flushing FETCH, wait for the first byte of the response (or a FIN, for an empty group) before accepting. A reset now rejects the request, so every joined fetch_group resolves to the error instead of a doomed group. This makes fetch_group match its documented contract.
  • moq-relay /fetch: a relayed miss arrives as Error::Stream(StreamError::NotFound), so map it to 404 alongside the local Error::NotFound (it fell through to 500 once the lookup failed early).
  • moq fetch: only a stale comment dropped; the lookup now fails with group N of `<track>` not found before any output.

Decision (the quest left it to the implementer): the check lives in moq-net, not in the moq_relay::fetch_group helper. The helper could have peeked the first frame on a cloned consumer, but that would leave fetch_group misleading every other caller (FFI, relays chaining FETCH).

Public API / wire impact

  • No new or changed public items. Behavior only: over moq-lite, fetch_group for a group the upstream lacks now resolves to Err instead of a group that aborts on first read; an existing group resolves once its first byte arrives (it could not be read earlier anyway).
  • No wire change, so no draft or js/net update.

Tests

  • moq-cli fetch::tests::a_missing_sequence_fails now asserts the top-level error is the lookup's "not found" (fails without the moq-net change) and that HTTP /fetch?group=99 is 404 with an empty body (fails without the fix: 200, then 500 without the relay mapping).
  • Existing-group tests now assert the HTTP status too, and still compare bytes against moq fetch.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 77c6af8e-a3ff-46ad-8eff-a7e2bee3f13e

📥 Commits

Reviewing files that changed from the base of the PR and between be5404d and b33ce4a.

📒 Files selected for processing (11)
  • doc/bin/cli.md
  • doc/bin/relay/http.md
  • quest/m1/README.md
  • quest/m1/fetch-missing-group.md
  • rs/moq-cli/src/fetch.rs
  • rs/moq-net/src/coding/reader.rs
  • rs/moq-net/src/lite/subscriber.rs
  • rs/moq-net/src/model/group.rs
  • rs/moq-net/src/model/track.rs
  • rs/moq-relay/src/fetch.rs
  • rs/moq-relay/src/web.rs
💤 Files with no reviewable changes (2)
  • quest/m1/README.md
  • quest/m1/fetch-missing-group.md

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


Walkthrough

Fetch handling now waits for a publisher response before accepting a requested group and rejects the request if the response check fails. Relay HTTP fetch maps local and upstream not-found errors to 404. CLI tests check successful 200 responses and missing-sequence results, including a 404 response with an empty body. Fetch documentation describes missing-group behavior, and the missing-group quest entry and planning document were removed.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to b33ce

The fetch changes are ready to merge after normal checks; no outstanding issue is established.

Security Architecture Review

Security architecture risk: 🔵 Low · up to b33ce

The change improves missing-group handling without an identified new security exposure. A publisher that never answers can still delay a fetch, and the available evidence does not establish a system-wide limit on that wait.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — An HTTP caller can select a group within an admitted broadcast, and its publisher controls whether the upstream response supplies data, finishes, resets, or remains open. The examined path does not expose a new broadcast or bypass admission.

Security Findings and Attack Paths

  • inferred — A non-answering publisher can keep an upstream fetch pending, including after an HTTP handler times out. The base implementation could also retain in-flight ingestion for a non-answer, so this review does not attribute a new resource-exhaustion finding to the PR.

Trust Boundaries and Controls

  • observed — A publisher reset now rejects the pending shared request rather than producing a group that fails on first read. HTTP admission and token-derived broadcast scoping remain ahead of that result.

Resilience and Maintainability Implications

  • observed — The HTTP handler bounds its wait to 30 seconds, but that deadline does not itself retire the separately owned upstream fetch task. An external session idle limit was not established.

Hardening Proposals

  • proposed — Consider a bounded upstream answer lifetime or cancellation policy tied to remaining fetch demand, accounting for callers other than HTTP.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: lite fetch requests now resolve only after the publisher answers.
Description check ✅ Passed The description directly explains the missing-group problem, the lite subscriber fix, relay 404 mapping, CLI behavior, API impact, and tests.
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 7 files. (2 skipped: 2 …
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.
✨ Finishing Touches 💡 1
✨ Simplify code
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

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.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-09-25T22:59:21.092769Z b33ce4a New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@kixelated

Copy link
Copy Markdown
Collaborator Author

MERGE

Positive improvement: yes. Over moq-lite there is no FETCH_OK, so accepting the group::Request as soon as FETCH flushed made every joined fetch_group resolve before the publisher answered. A missing group then looked like a live group that only failed on first frame read (HTTP 200 with a cut-off body; moq fetch failing mid-stream). Waiting for the first byte or a FIN, and rejecting on reset, makes fetch_group match its documented contract.

Worth the complexity: yes. One extra lite fetch state, poll_has_more exposed as pub(crate), and mapping upstream Error::Stream(StreamError::NotFound) to HTTP 404 alongside local Error::NotFound. That is proportional to the bug.

Different approach: putting the peek in moq_relay::fetch_group would have fixed the HTTP path only and left FFI and other fetch_group callers with the old lie. Keeping the check in moq-net is the right call. Empty groups still work because a FIN (poll_has_more → Ok(false)) counts as an answer and still accepts.

Tests and docs line up: a_missing_sequence_fails now asserts the lookup error and HTTP 404 with an empty body; existing-group cases assert status too. Quest and doc updates match the behavior change. No public API or wire change.

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4d679964ac

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +3539 to +3543
// Lite has no FETCH_OK: a publisher without the group resets the
// stream instead. Accepting before the first byte (or a FIN, for an
// empty group) would resolve every joined `fetch_group` to a group
// that only fails on its first read, so wait for the answer.
let answered = ready!(stream.reader.poll_has_more(&mut cx));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Gate TypeScript fetches on the publisher answer

For a TypeScript/browser consumer fetching a missing lite group, js/net/src/lite/subscriber.ts:706-767 still creates and returns a group mirror immediately after sending FETCH; its background #runFetchResponse only closes that group when the reset arrives. Consequently, fetchGroup() continues to resolve and the failure appears on the first frame read, which is exactly the behavior this patch fixes only in Rust. Apply the equivalent first-byte/FIN gate and missing-group regression test in js/net so the APIs remain mirrored.

AGENTS.md reference: AGENTS.md:L94-L98

Useful? React with 👍 / 👎.

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.

Leaving the JS gate out of this PR. It is the Rust side: lite fetch_group waits for the publisher's first byte or FIN, and HTTP /fetch maps an upstream miss to 404. The browser gap is the separate quest landed in #4175 (quest/m1/js-fetch-answer.md), not this diff.

(Written by Grok 4.7)

moq-lite has no FETCH_OK, and the subscriber accepted a group request as
soon as FETCH was flushed, so `fetch_group` resolved to a group that only
failed on its first read. Wait for the first byte (or a FIN) of the
response before accepting; a reset now rejects the fetch.

The relay's HTTP /fetch maps the relayed StreamError::NotFound to 404, and
`moq fetch` fails its lookup before writing anything.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated force-pushed the quest/m1/fetch-missing-group branch from 4d67996 to b33ce4a Compare September 25, 2026 22:57

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main (be5404df7). The only conflict was quest/m1/README.md: this drops the completed Rust fetch quest and keeps the JS fetch-answer quest from #4175. The earlier Check/Test failure was moq-ffi E0283 on config.into(), already fixed on main by #4157, so #4171 is not in this branch.

Landing: over moq-lite, fetch_group resolves only after the publisher's first byte or a FIN. A reset rejects the fetch, so a missing group is an error instead of a group that fails on the first read. HTTP /fetch maps that upstream StreamError::NotFound to 404, and moq fetch fails the lookup before writing anything.

(Written by Grok 4.7)

@kixelated
kixelated enabled auto-merge (squash) September 25, 2026 23:04
@kixelated
kixelated merged commit ad48f03 into main Sep 26, 2026
6 of 7 checks passed
@kixelated
kixelated deleted the quest/m1/fetch-missing-group branch September 26, 2026 00:31
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