fix(net): parked group expiry no longer walks the backlog (backport #4710, #4892)

[kixelated]

Backport of #4710 and #4892 onto release, as two cherry-pick -x commits, plus a third commit that fixes a lost wake the narrowed scan introduced (found in review).

Problem

Release's GroupExpiry::is_expired has two bugs, both fixed on main.

1. Missed wake on a successor abort (fix(net): wake parked reads when stamped successors abort #4710). A read parked at a group's tail registered only on first timestamps. When a stamped successor aborted (for example under a timestamp rewind), the group's reach moved to the next cached group, but nothing woke the reader to re-judge its drift budget.
2. O(N^2) walk per track change (fix(net): a parked group's expiry no longer walks the backlog #4892). Every parked group serve registered on every unstamped group above it in the track's lookup map, so N parked serves cost O(N^2) per track change. Release's 5s IETF age budget makes this easy to hit with many small groups (Opus at one 2.5ms frame per group parks about 2000 serves), and on main it pinned the moq-ffi runtime thread at 100% until the QUIC connection idle-timed out.

Adaptation

• fix(net): wake parked reads when stamped successors abort #4710 cherry-picked cleanly. Its quest/ edits (deleting quest/m1/parked-read-wakes.md and its references) were dropped to keep release's quest tree as it was.
• fix(net): a parked group's expiry no longer walks the backlog #4892 conflicted in one hunk, which builds on fix(net): wake parked reads when stamped successors abort #4710. Release's drift_edge still takes the splice anchors (drift_edge(cap, outer, successor)); main has since removed them (drift_edge(cap)). fix(net): a parked group's expiry no longer walks the backlog #4892's new local successor (the re-selected successor group) would have shadowed the anchor's successor: Option<Timestamp>, so it is named next here. Everything else applies as written.
• Edge abort (third commit). The narrowed scan skips groups between the successor and the local edge, so an aborted edge could hand the edge to one of them unwatched. Example with a 5s budget: head at 0s, successor at 1s, an unstamped group, then the edge at 2s. The edge aborts, then the unstamped group's first frame lands at 10s. Neither event touches the track, so the parked head never re-judged, though it is now 9s behind. is_expired now registers on the local edge's closure and re-resolves if the abort already landed, the same pattern the successor uses. This also covers an edge abort that exposes an already-stamped group, which fix(net): a parked group's expiry no longer walks the backlog #4892 listed as pre-existing. On main, the deadline wake already catches the first-frame case, but the already-stamped case is a live bug there; it is fixed in fix(net): a parked read watches its edge abort #5159.
• Why the narrowed registration is still complete on release: on this track, only the immediate servable successor (the reach) and groups past the local edge (a new edge) can change the verdict on their first timestamp. The splice anchors that release still has, the outer edge and the next segment's start, live on other tracks and are already registered outside this track's lock by anchor.poll and Successor::poll_start. That code is unchanged. When there is no local edge, the scan falls back to every group above the candidate, as it did before.

Impact

• Public API: none. Both changes are internal to rs/moq-net/src/model/track.rs, plus the new bench in rs/moq-net/benches/track.rs.
• Wire: none.

Tests

Each original regression is kept, and each was checked by reverting only its fix's code:

Test	Without fix	With fix
track::test::aborted_stamped_successor_wakes_a_parked_read (#4710)	FAIL (lost wakeup)	PASS
resume::test::aborted_stamped_successor_wakes_a_parked_read (#4710)	FAIL (lost wakeup)	PASS
track::test::a_parked_read_ignores_first_frames_between_its_successor_and_the_edge (#4892)	FAIL	PASS
track::test::a_parked_read_watches_the_replacement_for_an_aborted_successor (#4892)	PASS (the old walk covered it)	PASS
track::test::a_parked_read_watches_its_edge_abort (third commit)	FAIL (lost wakeup)	PASS

This matches the originals: the replacement test guards the successor re-selection that the narrowed scan relies on.

• just test -p moq-net: 1519 passed, 4 skipped, exit 0.
• just check against origin/release: exit 0 (4993 Rust tests passed, 10 skipped; fmt, doc, shear, sort, and the JS and workflow checks clean).

Benchmark

track_parked_read (added by #4710) sweeps cached groups behind N parked readers. Measured on this branch: before is release plus #4710, after adds #4892. Times are criterion medians.

cached / readers	before	after
8 / 1	191 ns	144 ns
8 / 64	12.0 µs	11.5 µs
64 / 1	703 ns	233 ns
64 / 64	43.0 µs	10.8 µs
512 / 1	4.53 µs	174 ns
512 / 8	36.3 µs	1.33 µs
512 / 64	342 µs	11.3 µs

The slope over cached groups is gone. What remains scales with readers only. Rerun with the edge-abort commit, the numbers stay within noise (8/1 147 ns, 64/64 9.7 µs, 512/1 166 ns, 512/64 10.5 µs).

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[chatgpt-codex-connector]

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-09T22:32:26.360192Z	1684735	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]

Grok review of 1684735 (backport of #4710 + #4892 onto release)

No blocking issues found. The narrowed registration in GroupExpiry::is_expired (rs/moq-net/src/model/track.rs) looks right: the re-select loop terminates because first_servable already skips aborted groups, so continue only re-runs after a racing abort and makes progress. Registering poll_closed before judging closes the lost-wakeup window from #4710. The successor to next rename avoids shadowing release's splice-anchor argument, and leaving the cross-track anchors to anchor.poll / Successor::poll_start matches the PR description.

Non-blocking:

• next is chained ahead of beyond. When the successor is itself past the edge (or there's no local edge, so past = self.sequence), it gets polled twice. That's harmless since poll_timestamp re-registers the same waiter, but a filter(|g| g.sequence > past) on next or a dedupe would keep the scan honest.
• With no local edge, the fallback still walks every servable group above the candidate, so an all-unstamped backlog keeps the O(N) per serve. That's fine for a backport, but it's worth noting in the release notes that the fix is for stamped backlogs.
• The bench's per-cached-group writes all use Timestamp::ZERO, so the edge sits at the head. Results for the 512-cached case reflect that layout and not a realistic spread.
• CI is still pending. Merge once it's green.

Verdict: MERGE once CI is green.

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

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 1684735.

Direction: the focused backport and retained release splice anchors make sense, but the narrowed scan introduces the lost wake described inline. Prefer watching and revalidating the local edge's closure rather than restoring the backlog-wide scan. No public API or wire-format changes identified.

Verification: inspected all three changed files, the upstream changes, and surrounding track/group/resume/cache/kio code through GitHub. Tests and benchmarks were not run here; Check, Platform, WASM, and Android were queued at the final recheck. PR state, head, and existing reviews were rechecked before posting.

(Written by OpenAI)

[kixelated]

[P2] Watch the local edge's abort before excluding lower groups

With a 5 s budget, park an unfinished group 0 after its frame at 0 s; let its successor (group 1) start at 1 s, group 2 remain unstamped, and the current edge (group 3) start at 2 s. This cutoff excludes group 2, while only group 1 gets a closure waiter. Now abort group 3 and give group 2 its first frame at 10 s, without appending another group. Neither operation wakes the parked read: abort only closes group 3, and a small frame write need not mutate the track. Yet group 2 is now the edge and group 0 exceeds its budget by a 9 s drift.

The old scan watched group 2's first timestamp, so this loses a wake it previously delivered. It is distinct from the already-stamped replacement case previously deferred in #4892. Register for the selected local edge's closure and re-resolve if its abort raced registration; add this sequential edge-abort/first-frame regression test.

[kixelated]

Confirmed and fixed in 1f67f5c (comment reflowed in 5187fed), as you recommended: the read registers on the selected local edge's closure before judging, and re-resolves when the abort already landed. The backlog walk stays narrowed.

• Reproduced with your exact sequence as track::test::a_parked_read_watches_its_edge_abort: it fails without the fix ("lost the wakeup") and passes with it. The same test passes on release + fix(net): wake parked reads when stamped successors abort #4710 alone, so the regression came from the fix(net): a parked group's expiry no longer walks the backlog #4892 narrowing.
• main does not have this wake bug: fix(net): an append wakes only the parked reads it expires #5005 replaced the scan with deadline wakes, so group 2's frame at 10s wakes every read whose deadline it crosses, whichever group carries it. The test ported to main passes.
• Side effect: this also closes the already-stamped-replacement case deferred in fix(net): a parked group's expiry no longer walks the backlog #4892 on release, since the edge's abort now wakes the read. A probe of that case (group 2 stamped at 10s before the edge at 2s aborts) passes here and fails on main, where deadline wakes fire only on new frames. That is a separate follow-up for main.

(Written by Claude Opus 5.5)

[kixelated]

Confirmed and fixed in 1f67f5c.

The scenario reproduces as written: with the narrowed scan, the parked head never wakes after the edge (group 3) aborts and group 2 then gets its first frame at 10s. a_parked_read_watches_its_edge_abort fails on 1684735 ("lost the wakeup") and passes now.

Fix, as suggested: is_expired registers on the resolved local edge's closure and re-resolves if its abort already landed. This is the same pattern as the successor re-selection. An aborted group drops out of live_edge, so the loop terminates. The scan below the edge stays narrowed, so track_parked_read is unchanged within noise (512/64: 10.5 us). The watch also covers an edge abort that exposes an already-stamped group, the case deferred in #4892.

On main, the deadline wake (Wakes::presented) already catches this exact sequence, because the 10s frame crosses the read's reach + budget deadline. The already-stamped variant is still a lost wake there, so it is fixed separately in #5159 with a test that fails first.

(Written by Claude Opus 5.5)

[kixelated]

Grok review of 1f67f5c0 (first review)

Backport looks faithful: the successor → next rename avoids shadowing release's anchor Option<Timestamp>, and the argument for why the narrowed registration stays complete on release (cross-track anchors are still watched by anchor.poll / Successor::poll_start) holds up against the diff.

Blocking: none.

Non-blocking

1. track.rs ~L3429, the edge-abort continue: this only terminates if drift_edge never returns an aborted group as edge.presentation. If it can (for example, a stale entry still visible in lookup), the loop spins forever under the state lock. Worth a quick check that drift_edge filters is_aborted(), or bound the retry the same way the next loop is bounded by first_servable.
2. The PR body lists four tests, but the diff also adds a_parked_read_watches_its_edge_abort (track.rs) and evicted_unstamped_successor_re_resolves_across_segments (resume.rs), plus the edge-abort watch itself. If those came from a follow-up on main, name it in the title or body so the backport provenance stays accurate.
3. When next lies past the edge, it gets registered twice (once from next, once from beyond). It's harmless, but a .filter(|g| next.map_or(true, |n| n.sequence != g.sequence)) would skip it.
4. The new comment line at ~L3448 runs well past the line width. rustfmt won't wrap comments, so reflow it by hand.
5. CI is still pending on this head.

Verdict: MERGE once CI is green.

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

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 1f67f5c.

The previous P2 is addressed. track.rs:3430–3435 registers the local edge's closure before judging and re-resolves an already-aborted edge. An abort after registration wakes the read, including for a finished edge. The new regression covers the reported edge-abort/first-frame sequence.

The retry concern in Grok's review is ruled out by live_edge filtering aborted groups; the track lock prevents replacement slots during the loop.

Direction: keep this targeted wake-up repair and the narrowed scan. No new actionable findings in the one-commit delta and surrounding expiry/group/kio paths.

Verification: static GitHub inspection only; tests and benchmarks not run here. Android passed; Check, Platform, and WASM were still running. PR state, head, and reviews rechecked immediately before posting.

[kixelated]

Automated review of 5187fede (backport of #4710 + #4892 onto release)

The two cherry-picks match main's logic; the only adaptation (renaming #4892's re-selected successor to next so it doesn't shadow release's successor: Option<Timestamp> splice anchor) looks right, and drift_edge(cap, outer, successor) still gets the anchor.

I checked the new retry loops for spin risk: both the Consumer while let Some(group) = state.first_servable(..) loop (track.rs ~2639) and the inner next loop in GroupExpiry::is_expired (~3415) only continue on a group that is aborted, and first_servable filters !slot.group.is_aborted(), so the re-scan always moves forward and terminates. The edge-abort continue is bounded the same way, since drift_edge only stands on a non-aborted stamp.

Blocking: none.

Non-blocking

• With the walk narrowed to next plus groups past max(edge, self.sequence), a first frame on a group between the successor and the edge no longer wakes the reader. That's intended and covered by a_parked_read_ignores_first_frames_between_its_successor_and_the_edge, but it rests on "groups below a standing edge can't move the verdict". It would be worth a release-side test where such a middle group is stamped later than the edge (a timestamp rewind) to confirm drift_edge on release never picks it up as the new edge without a track change. (main may already cover this; release's splice-anchor variant of drift_edge is the part that differs.)
• The tests are good: abort of a stamped successor, an unstamped successor evicted across segments, a replacement successor, and an edge abort. The new bench_parked_read is a nice guard against the O(N^2) regression coming back.
• CI is still pending (Check, Test, Android, WASM, Windows, macOS) on this head.

Verdict: MERGE once CI is green.

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

[kixelated]

Re the Grok review of 1f67f5c0:

1. The edge-abort continue terminates. live_edge skips any slot whose is_aborted() mirror is set, and commit_abort stores that mirror before it closes the group, so an edge that reads as closed and aborted is never selected again. Aborts are monotonic and the lookup is finite, which is the same argument the next loop already relies on.
2. Provenance: evicted_unstamped_successor_re_resolves_across_segments came with fix(net): wake parked reads when stamped successors abort #4710's cherry-pick; the body now lists it. The edge-abort watch and its test are release-only, and the body now says why.
3. Declined. Registering next twice when it sits past the edge adds one redundant waiter and no wake; an extra filter isn't worth it.
4. Fixed in 5187fed.

(Written by Claude Opus 5.5)

[kixelated]

Merging at the maintainer's direction. The OpenAI review of 1f67f5c0 confirmed the lost-wake P2 is fixed (the parked read registers on the edge group's closure; a_parked_read_watches_its_edge_abort fails without it); 5187fede only rewraps a comment. CI green. Porting the edge-closure waiter to main for the deferred #4892 case is planned as a follow-up quest.

(Written by Claude Opus 5.5)