fix(net): resolve a lite fetch only once the publisher answers - #4164
Conversation
|
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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
💤 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; 0 remain after this review. WalkthroughFetch 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 The fetch changes are ready to merge after normal checks; no outstanding issue is established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to 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 Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1✨ Simplify code
🛠️ Fix failing CI checks 💡
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
MERGE Positive improvement: yes. Over moq-lite there is no FETCH_OK, so accepting the Worth the complexity: yes. One extra lite fetch state, Different approach: putting the peek in Tests and docs line up: This is an automated review, not the maintainer's decision |
There was a problem hiding this comment.
💡 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".
| // 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)); |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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>
4d67996 to
b33ce4a
Compare
|
Rebased onto current Landing: over moq-lite, (Written by Grok 4.7) |
Completes the
quest/m1/fetch-missing-groupquest.Problem
HTTP
/fetch/<broadcast>/<track>?group=Nanswered200with an empty, cut-off body for a group the track lacks, andmoq fetch --group Nonly failed on its first frame read.Root cause: moq-lite has no FETCH_OK, and the lite subscriber accepted the
group::Requestas soon as the FETCH was flushed. Sotrack::Consumer::fetch_groupresolved 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 joinedfetch_groupresolves to the error instead of a doomed group. This makesfetch_groupmatch its documented contract.moq-relay/fetch: a relayed miss arrives asError::Stream(StreamError::NotFound), so map it to404alongside the localError::NotFound(it fell through to500once the lookup failed early).moq fetch: only a stale comment dropped; the lookup now fails withgroup N of `<track>` not foundbefore any output.Decision (the quest left it to the implementer): the check lives in
moq-net, not in themoq_relay::fetch_grouphelper. The helper could have peeked the first frame on a cloned consumer, but that would leavefetch_groupmisleading every other caller (FFI, relays chaining FETCH).Public API / wire impact
fetch_groupfor a group the upstream lacks now resolves toErrinstead of a group that aborts on first read; an existing group resolves once its first byte arrives (it could not be read earlier anyway).js/netupdate.Tests
moq-clifetch::tests::a_missing_sequence_failsnow asserts the top-level error is the lookup's "not found" (fails without themoq-netchange) and that HTTP/fetch?group=99is404with an empty body (fails without the fix:200, then500without the relay mapping).moq fetch.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code