Skip to content

quest: plan the second release backport batch and the parked-read bench budget - #5166

Merged
kixelated merged 2 commits into
mainfrom
plan/release-followup-quests
Oct 10, 2026
Merged

kixelated merged 2 commits into
mainfrom
plan/release-followup-quests

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Follow-up quests from the 2026-10-09 triage of fixes on main that might need backporting to release (first batch: #5120 through #5135, plus #5164).

Quests

Ranked in m1 next to the IETF and benchmark quests. quest check passes.

API / wire impact

None; planning only.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

…ch budget

Follow-ups from the 2026-10-09 release backport triage: a questline for the
smaller release-only fixes left out of the first batch, a quest for the
track_parked_read bench outliving its budget, and a note that the IETF
request-header fix needs a release backport too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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 35 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e0f60b16-f062-40cf-a503-7af3261c0da0

📥 Commits

Reviewing files that changed from the base of the PR and between 0bddd90 and 5e04a33.


📒 Files selected for processing (10)
  • quest/m1/README.md
  • quest/m1/ietf-dispatch-headers.md
  • quest/m1/parked-read-bench-budget.md
  • quest/m1/release-backports/README.md
  • quest/m1/release-backports/catalog-floor.md
  • quest/m1/release-backports/fetch-fin.md
  • quest/m1/release-backports/js-fin.md
  • quest/m1/release-backports/namespace-fill.md
  • quest/m1/release-backports/request-params.md
  • quest/m1/release-backports/setup-repeat.md

  • Autofix · 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.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 10, 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 ✅ Completed 2026-10-10T02:05:53.993831Z 37194f1 PR opened
ℹ️ 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

Automated review of 37194f1

Planning-only quest PR. I spot-checked the claims against main and release. The bench math holds: track_parked_read uses a 3600 s max_delay and advances 2500 µs per append (rs/moq-net/benches/track.rs:424,434,469), and 3600 s / 2.5 ms ≈ 1.44M. #5133, #5125, #5032 and #4940 are merged as described.

Non-blocking

  1. release-backports/README.md says most children can't cherry-pick because release lacks "fix(net): keep IETF requests alive after FIN #4658's request_stream.rs", and request-params.md says the same about fix(net): accept each draft's message parameters #5028. But rs/moq-net/src/ietf/request_stream.rs already exists on release, since fix(net): keep IETF subscriptions alive after a request FIN (backport #4658) #5133 brought it over with fin_cancels. fetch-fin.md even relies on that. Please reword so implementers don't write around a file that's already there.
  2. The README says the first batch is "fix(js): drop a headless subgroup before FIRST_OBJECT (backport #5019) #5120 through fix(net): a capped IETF stream never claims END_OF_GROUP (backport #5077) #5135, plus fix(net): follow-ups to #5133 (deferred FIN on drop, reprioritize open group streams) #5164", which "covered every crash, leak, and interop break", but fix(net): follow-ups to #5133 (deferred FIN on drop, reprioritize open group streams) #5164 is still open. Consider saying "pending fix(net): follow-ups to #5133 (deferred FIN on drop, reprioritize open group streams) #5164", or adding a check that it lands.
  3. fetch-fin.md says "check whether main's FETCH path already behaves this way; if not, land it there first". That makes it a main fix, not just a backport. It may deserve its own main-side note so it isn't buried in a release questline.

No broken links in the added entries, as far as I checked.

Verdict: MERGE

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: 37194f14e1

ℹ️ 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 thread quest/m1/release-backports/README.md Outdated
Comment on lines +19 to +21
The `release` interop matrix only negotiates moq-lite, so IETF changes need
unit tests or `just test bare-fin`-style coverage rather than `just test
interop`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep the required full interop run

For these wire-facing backports, the “rather than” wording directs implementers to substitute unit or bare-fin coverage for the full interop matrix. Keep the focused regression coverage, but also require just test interop --all, which test/justfile:30-42 identifies as the cross-language matrix and the repository explicitly requires for wire changes.

AGENTS.md reference: AGENTS.md:L116-L116

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.

Agreed. Fixed in 5e04a33: the README keeps the unit or bare-fin coverage (the release interop matrix only negotiates moq-lite) and also requires just test interop --all.

(Written by Claude Opus 5.5)

…is release-only

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Merging (picked by the maintainer). Changes in 5e04a33, addressing the review notes:

  • release-backports/README.md and request-params.md no longer say release lacks request_stream.rs. It has been there since fix(net): keep IETF subscriptions alive after a request FIN (backport #4658) #5133 (fin_cancels, poll_cancel), so implementers should extend it. The README's main-only list now names decode_params! syntax instead.
  • The README says the first batch (including fix(net): follow-ups to #5133 (deferred FIN on drop, reprioritize open group streams) #5164) is still landing in part, rather than "covered".
  • fetch-fin.md: main already routes its FETCH paths through request_stream::poll_cancel, while release's run_fetch_stream still watches reader.poll_closed directly. So this is release-only, and the "land it on main first" step is gone. No main-side quest needed.
  • Codex P2: the README keeps unit or bare-fin coverage and also requires just test interop --all.

Checked for overlap with the in-flight backports #5164 (deferred FIN on drop, priority updates), #5124 (TS damaged units), and #5132 (viewer fronts). None of the children duplicate them, so nothing was trimmed.

(Written by Claude Opus 5.5)

@kixelated
kixelated enabled auto-merge (squash) October 10, 2026 12:45
@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated review of head 5e04a33f

Planning-only quest PR. I spot-checked the main claims against the code and they hold up:

  • release has rs/moq-net/src/ietf/request_stream.rs with fin_cancels and poll_cancel, and release's run_fetch_stream paths still poll stream.reader.poll_closed directly (publisher.rs around lines 1390 and 1499), so fetch-fin.md is accurate.
  • track_parked_read on main uses a 3600 s with_max_delay, and 3600 s ÷ 2.5 ms comes to 1.44M iterations, which matches parked-read-bench-budget.md.
  • The links in both READMEs point at files this PR adds.

Non-blocking:

  • release-backports/README.md says the first batch "covers every crash, leak, and interop break", but request-params.md describes moqx on d16 redialing in a loop, which is an interop break. Consider softening that sentence.
  • js-fin.md asks for a @moq/net bump with /bump on release. Please confirm that command works on the release branch, or say how release bumps are done.
  • CI (Check and Test) is still pending.

Verdict: MERGE

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

@kixelated
kixelated merged commit 7c53fa9 into main Oct 10, 2026
3 checks passed
@kixelated
kixelated deleted the plan/release-followup-quests branch October 10, 2026 12:52
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