Repository navigation
fix(net): a resumed group keeps the head the replaced route received - #5958
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When a front resumes a group on another route, the replacement copy now adopts frames 0..N-1 the replaced copy held and fetches only N onward, so the serving route caches the group whole, as a cold relay would. A group handed out from a logical track shares a hand-off with its readers. A release that is no cache policy moves the committed frames into it rather than dropping them, so a plain consumer still reads the abort and pins nothing, and the frames go with the last resuming reader. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
![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: 79396ca4f7
ℹ️ 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".
|
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 25 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 |
Nothing bounds how many group consumers a stalled downstream egress holds, so an unbilled hand-off let a peer that forces route changes pin failed groups' heads without limit. A held head is now billed to its track's pool, refused when the pool has no room for it, and dropped by the sweep once the pool's idle window passes, the same bounds as cached content. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Decisions on the three internal choices, now recorded in the description:
(Written by Claude Opus 5.5) |
|
@codex review (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 307d50c8f3
ℹ️ 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".
…e from adoption - Handoff::head takes the group's lock before the salvage, as release does, so it neither deadlocks against a concurrent release nor sees the head in neither place. - A held head registers for expiry only once stored, so a sweep in between cannot forget it. - Adoption counts the open frame's unwritten remainder against the group budget. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edf62b2ec8
ℹ️ 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".
A GroupTooLarge from set_end_object reached commit_abort through abort and was salvaged; abort_too_large now shares commit_abort, which frees it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8a7e7c243
ℹ️ 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".
Every failed group in an expiry-enabled pool registered its hand-off for the sweep, even when recovery adopted the head at once, and the sweep's retain kept the vector's peak allocation. A burst of failed groups could pin memory outside the pool's accounting. The registry now forgets dead hand-offs before it grows, and the sweep gives back what a burst of held heads grew it to. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ab1757490
ℹ️ 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".
A held hand-off head sits outside every track's eviction order, and only an expiry window ever dropped it. In a capacity-only pool, a shrink below the held bills left a stalled resumer keeping them while ordinary groups were evicted. The held registry now lives on the pool, which drops the oldest held heads first whenever a track charges debt over capacity, and on each sweep. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Main's #5958 adds a handoff field to Recover where this branch adds backups; the fetch's Recover literal conflicted and takes both fields. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The branch's held() path continues a failed group from another copy before poll_serving runs, so #5958's lend_head never ran for it and that copy's cache missed the head the failed route received. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolve against main's head hand-off (#5958) and key-logging (#5907): - MIGRATE: keep both Unreleased entries. - resume: the replacement lends the failed copy's head before the compressed-group prefix check. - cache: main's held-head Bill keeps its name; the inflated copy's growable bill becomes cache::Extra. - group: a held Head carries a compressed group's inflated sizes, and a compressed group adopts only a compressed head, with its sizes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Plans and finishes
quest/m1/resume-prefix.md(added and removed in this PR).Problem
After a route change mid-group, a front's reader resumed group S at frame N by fetching N onward from the new route. The new route's cache then held S from N only: the replaced copy's
commit_abortreleased frames 0..N-1 before anything could take them, andRecoverheld no frames. A later reader or fetch of S from frame 0 missed the cache and went upstream again, so a relay that saw the route change behaved differently from one that did not. This affects every group, not only compressed ones.Change
group::Handoff): a group handed out from a logical track shares one hand-off among the readers that can resume it (created on firsthand_out, held weakly by the group). It reads the copy's frames while the copy has them. If the copy is released for any reason other than cache policy (a failed or given-up abort, or a producer dropped unfinished),releasemoves the committed frames into the hand-off instead of dropping them. Old, Timeout, Evicted and GroupTooLarge still free everything.group::Producer::adopt_head, throughtrack::Consumer::adopt_head): beforeRecoverchecks a candidate replacement (delivered by subscription or fetched), it lends the head to the serving copy's cached copies of that group. Each one that starts above the head and is reached by it takes the frames below its start. The adopted frames are billed and checked against the group budget (if they do not fit, nothing is adopted). IETF object fields stay aligned, and the wire header'sfirst_object/first_id/lastcovers the head, including a header that arrives after the adoption.Hook for #5928:
group::Producer::adopt_headis where a group gains frames below its start. A compressed group would rebuild its inflater there from the adopted head, rather than widening resume to frame 0.JS: js/net has no route resume on main (it is in #5836 /
quest/m1/js-group-handover.md), so there is nothing to mirror here yet. That port should carry this contract.Public API and wire impact
None. Everything new is
pub(super). No wire, moq-ffi or binding changes, so the interop matrix was not run.Decisions (settled by the maintainer)
serve()pushes aGroupServeper group with no in-flight cap (rs/moq-net/src/lite/publisher.rs:3733-3754), IETF says "one machine per group, with unlimited concurrency" (rs/moq-net/src/ietf/publisher.rs:4007), the max-delay budget defaults to about 2^53 ms (lite/publisher.rs:90-104), and an aborted group waiting inOpennever expires. So an upstream forcing route changes, plus a downstream withholding credit, could pin failed groups' heads without limit; before this PR, the abort released them. Now, as the second commit:cache::Bill, released on drop). It takes the place of the group's own charge, which the release just cleared.cache::Track::hold).cache::Track::expire_held, registered only after the head is stored, so a concurrent sweep cannot forget it). That is 30 s by default on a relay, the same window that reclaims idle cached groups.Locking:
Handoff::headtakes the group's lock before the salvage, inrelease's order, so it cannot deadlock against a concurrent release or see the head in neither place.Tests
resume:a_failed_route_hands_its_head_to_the_replacement,a_stalled_route_lends_its_head_to_the_replacement(both failed on main: the later fetch from frame 0 was a cache miss), andan_evicted_group_hands_nothing_to_the_replacement.group:an_abort_hands_the_head_to_its_resumers(the frames move only for a non-policy abort, are billed while held, and are freed and unbilled with the last resumer),a_held_head_expires_with_the_pool_windowanda_full_pool_holds_no_head(both failed without the second commit),an_open_frame_reserves_its_remainder_from_adoption(failed without the third commit), andan_adopted_head_keeps_fields_and_the_wire_aligned(fields and wire header, whether the header was set before or after adoption, billing, and reader positions).#[test]s, no wall clock.Benchmark:
origin/relay(tracks by readers through a front, thehand_outpath that now takes the hand-off) against amainbaseline on the same machine shows no regression: every shape measured faster, which is within the noise of the baseline run. Adoption runs only on a route change.Checks:
just checkpasses (exit 0).cargo test -p moq-net --all-featurespasses.quest checkpasses. Not run:just test interop --all(no wire or ffi change),just rs loom.Follow-ups
adopt_headinstead of widening resume to frame 0.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)