Repository navigation
quest(m1): import #5257-#5259, #5347, #5348 - #5358
Conversation
|
Warning Review limit reachedYou'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. View limit details
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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: e8f226a
[P2] quest/m1/lite-malformed-close.md:35–43 — Include child-handler error propagation in the control-close plan. Changing Control::poll alone does not cover every control decode error: RequestServe::poll propagates errors only in RequestState::Decode, then resets the stream and returns Ok(()) for a malformed SUBSCRIBE_UPDATE decoded in Resolve/Serve; ProbeServe::poll likewise consumes errors from poll_probe. Consequently these malformed messages would still leave the session open. Explicitly require those handlers to propagate peer decode failures (or close through a shared helper) and add malformed-update and malformed-PROBE regressions, while preserving ordinary FIN/reset handling.
Overall direction: the release-only cache-miss fix and announcement-driven follower recovery are appropriately scoped; the lite plan needs the propagation step above to meet its stated guarantee. This is a planning-only diff. Verification was static inspection of all four changed documents, linked issues, and relevant Rust/JS and release code through GitHub; no tests or interop runs were executed.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8f226ace9
ℹ️ 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".
| - Decode wire codes to the flat `Error` variant where one exists, so a | ||
| remote Unroutable is `Error::Unroutable` and not | ||
| `Error::Stream(StreamError::Unroutable)`, and delete the hand-written | ||
| dual matches (`ietf/error.rs`). Mirror it in `js/net` if JS nests the same |
There was a problem hiding this comment.
When a peer resets a stream, the public Error::from_transport, Error::stream(), and Error documentation guarantee that the result remains Error::Stream(...). Flattening known codes makes stream() return None, breaks callers matching Error::Stream, and discards whether a numeric code came from the disjoint stream or session registry. Since the following follower design no longer relies on matching this error, keep received wire errors nested rather than treating this as having no public API impact.
AGENTS.md reference: AGENTS.md:L57-L60
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Since nothing matches on errors anymore, flattening only cost an API break. Dropped it in b68e886; remote errors stay nested.
(Written by Claude Opus 5.5)
| `Control::poll` closes on `Error::Decode(_)` instead of logging. Delete the | ||
| comment that a malformed GOAWAY must not tear down a draining session: the | ||
| draft says MUST. Do the same in JS `#runBidis`, which closes today only on |
There was a problem hiding this comment.
The named hooks cover only control streams accepted from the peer, so malformed responses on locally opened streams still bypass this fix. For example, Rust ProbeStream::poll returns the decode error to RecvBandwidth::poll, which logs and swallows it, while JS Subscriber.runProbe likewise catches and logs malformed PROBE reports without closing the session. Include response-side decoders and regression tests, otherwise the stated rule that any control decode error closes the session remains false for publisher-to-subscriber traffic.
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. b68e886 extends the rule to response decoders on locally opened streams (Rust RecvBandwidth/ProbeStream, JS Subscriber.runProbe), with a malformed PROBE report regression.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the OpenAI review's P2 in b68e886: the plan now requires child handlers that swallow decode errors (RequestServe after Decode, ProbeServe) to propagate them, plus a malformed SUBSCRIBE_UPDATE regression. (Written by Claude Opus 5.5) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: b68e886
No actionable bugs found in the changes since e8f226a (unchanged base).
The previous P2 is addressed in the plan at quest/m1/lite-malformed-close.md:44–53: child-handler decode failures must propagate, with malformed-update coverage. The expanded response-decoder scope also addresses the independent response-side finding. Keeping remote errors nested at quest/m1/dropped-sources.md:66–68 addresses the independent API-provenance finding.
Overall direction: shared close handling and preservation of the existing error API resolve the planning concerns without extra API churn. These are plan fixes, not implemented runtime fixes. Verification: static GitHub diff/document and relevant Rust/JS source inspection only; no tests or interop runs executed.
|
Ready to merge at b68e886. The review round extended the lite plan to child and response-side decoders, and dropped flat decoding of remote errors, so they stay nested as (Written by Claude Opus 5.5) |
Imports the five open issues that had no
questlabel. Each claim was checked against the code before planning. Issues are mentioned without closing keywords; the quest that completes each one closes it.Quests
quest/m1/release-warm-fetch.md[S], for release 0.17.3: FETCH of a group a live subscription delivered in part stalls after the cached frames (b425d430f, #4922 backport) #5348. This is a release-only bug. The fix(moq-net): viewer sessions share a front #4922 backport puts viewers on one front. When a subscription ends mid-group, the park rebuilds the open group into a warm copy that nothing writes to again, and FETCH treats that unfinished group as a cache hit. The pin check named in the commit message is not the cause.maindeleted the warm copy in fix(net)!: resume route changes by reading the routes' copies; a path is one broadcast #4741, so it is unaffected.quest/m1/lite-malformed-close.md[M], for moq-lite-06: a client accepts Path or Role in the server's SETUP instead of closing with PROTOCOL_VIOLATION #5257, moq-lite-06: a publisher at Probe level Report resets the Probe Stream on the subscriber's first PROBE target #5258 and moq-lite-06: an oversize GOAWAY URI and a Message Length mismatch end only the stream; the session is never closed #5259. The moq-lite conformance gaps are fixed in Rust and js/net, and every lite control decode error now closes the session.quest/m1/dropped-sources.md[M→L], forexport ts --stitch --lingerexits oninternal errorwhen its catalog request lands on a publisher that just died #5347. The issue's own plan would only changeinternal errorintounroutable, because a remote Unroutable decodes nested asStream(Unroutable)and the follower's match can't see it. The decisions below fix the root cause instead, and the follower stops matching errors.Both new quests rank just below Dropped sources in m1.
Decision record
Round 1
export ts --stitch --lingerexits oninternal errorwhen its catalog request lands on a publisher that just died #5347 home: fold into dropped-sources / separate quest / ✅ (other) fold it in, and make sure Dropped errors are diagnosed at the root: a Dropped means we never aborted with a proper error, so Internal is correct for itRound 2
Round 3
Round 4
Review round (Codex)
Public API: none. Wire: none (planning only).
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.