fix(net): a parked group's expiry no longer walks the backlog

[kixelated]

Problem

Interop has been red on main since #4162 (f539a520a, tested directly: its parent passes and it fails). python -> js and go -> js time out waiting for rendered video. The C++ publisher also goes through moq-ffi, so it fails the same way.

#4162 raised the browser's audio subscription max age to AUTO_MAX_AGE (2s). These publishers send Opus at 2.5ms frames, one frame per group, so a 2s subscription parks about 800 group serves on the publisher. On every poll, each parked serve's GroupExpiry::is_expired walked every group above it in the track's lookup map. That is O(N^2) work per track change. The single moq-ffi runtime thread then pinned at 100% CPU inside that walk (confirmed with gdb backtraces), the QUIC connection idle-timed out, and the browser received no frames. The "internal error" rows reported in CI are probably the same stall, since the stalled session takes down the publisher's other rows.

Approach

Only two kinds of unstamped group can change a parked group's verdict once they get their first timestamp: the immediate servable successor, which bounds the reach, and any group past the live edge, which becomes the new edge. Groups in between change neither. is_expired now registers on the successor and on the groups past the edge, not on everything above the candidate. With a backlog, the work per poll is now O(log N) rather than O(N).

Because the replacement for an aborted successor now sits in the skipped range, the successor is re-selected whenever poll_closed finds it already aborted (a group abort never touches the track, so nothing else would wake the read). Found by the Codex review.

Impact

• No public API or wire change. Change is internal to rs/moq-net/src/model/track.rs.

Tests

• New a_parked_read_ignores_first_frames_between_its_successor_and_the_edge. It fails on main, where the old walk registered on the middle group, and passes here. It includes a control showing that a first frame past the edge still wakes the read.

• New a_parked_read_watches_the_replacement_for_an_aborted_successor: after the successor aborts, the unstamped replacement's first frame still wakes the read and ends the stale head. It fails if the successor arm is dropped from the registration loop. The abort-before-registration window itself cannot be hit deterministically without an interleaving hook.

• The existing track_parked_read bench already sweeps cached groups behind a parked reader, and the slope is gone:

  bench	main	this PR
  cached_8/1	170 ns	125 ns
  cached_512/1	4.5 µs	139 ns
  cached_512/64	288 µs	8.8 µs

• just test interop --all, run locally on latest main:

  • Before this PR: python -> js and go -> js FAIL, and js -> rust FAIL. Every other row passes.
  • With this PR: 31/32 rows pass. Only js -> rust still fails (re-run on 0cf5d75ae, same result: cmaf: rendition audio changed its codec configuration).

• just check passes.

Follow-ups

• js -> rust is a separate regression from fix(moq-mux)!: fMP4 export keeps every track it declared in moov #4883 (7fcd8a701). It also fails on main (3 of 4 runs), so it is out of scope here. The browser publishes its Opus rendition first without description, and the fMP4 export declares a dOps with a guessed pre-skip. The catalog then gains the encoder's OpusHead, describe() sees a different sample entry, and it fails with TrackChanged.
• The remaining per-poll cost is O(log N) per parked serve, but every track change still wakes all N parked serves. A deadline-based wake (only when the edge crosses the group's budget) would remove that fan-out if a larger backlog ever needs it.
• Pre-existing: an abort of the edge group itself is not observed by a parked read (aborts never touch the track). Same class as the successor race, out of scope here.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 8e80f7a9-0a2d-4223-8131-e4350eb883d2

📥 Commits

Reviewing files that changed from the base of the PR and between 0cf5d75 and 714d874.

📒 Files selected for processing (1)

• rs/moq-net/src/model/track.rs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

---

Walkthrough

GroupExpiry::is_expired retries successor selection when the selected group is closed and aborted. It watches the candidate, the selected successor, and eligible groups beyond the candidate or live-edge sequence for timestamp changes. Tests check wakeups when a successor is aborted or a group beyond the edge receives its first timestamp. They also check that a first timestamp between the successor and the edge does not wake a parked read.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 714d8

The parked-read change has no identified issue that needs resolution before merge; normal checks still apply.

Architecture Summary

Architecture risk: 🔵 Low · up to 714d8

The change affects 1 system.

Changed systems: rs

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — rs (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in rs/moq-net/src/model/track.rs: GroupExpiry::is_expired now repeatedly selects the immediate servable successor and retries if it is already closed and aborted, so the replacement is selected and watched before the staleness check.
• observed — Modified behavior in rs/moq-net/src/model/track.rs: The timestamp-watching strategy now targets the candidate, its successor, and groups past the edge instead of the candidate and every unstamped servable group above it.
• observed — Modified behavior in rs/moq-net/src/model/track.rs: The code derives the first sequence past the candidate or live edge, then watches eligible visible, non-aborted groups in that range together with the selected successor. Groups between the successor and edge are no longer watched.
• observed — Modified behavior in rs/moq-net/src/model/track.rs: Adds a test that a parked read is not woken by a first timestamp between its successor and edge, but is woken when a group past the edge gains its first timestamp.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the main change: preventing a parked group's expiry check from walking the backlog.
Description check	✅ Passed	The description explains the performance problem, the implementation approach, and the tests related to the changeset.

• Fix all pre-merge checks with AI

✨ Finishing Touches

✨ Simplify code

• Commit to this branch
• Create a new PR

• Autopilot · 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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

Adversarial review (Codex, via peer-review)

Verdict: needs-attention (one finding), against a1c8ebc7b.

[high] Revalidate the successor after registering for abort (rs/moq-net/src/model/track.rs, GroupExpiry::is_expired)
Group 0 parked, group 1 stamped, group 2 unstamped, group 3 the stamped edge. If group 1 aborts after first_servable selects it but before poll_closed, poll_closed returns Ready without registering. is_stale then sees group 2 as the successor (unstamped, so not stale), but the registration loop reuses the cached group 1 and starts past the edge, so group 2's first timestamp is never watched. The old full scan registered on group 2 in this interleaving.

Triage: Confirmed. commit_abort only closes the group state; it never touches the track, so nothing else wakes the parked read until an unrelated track change. The failure direction is safe (a stale group is delivered rather than skipped), but it is a real lost wakeup the old walk did not have. Fixed in 0cf5d75ae: the successor is re-selected whenever poll_closed reports it already aborted, so the successor used for both the abort and first-timestamp registrations is the one is_stale judges against. This also closes the pre-existing gap where the replacement successor's own abort went unwatched. No deterministic regression test: the window sits between two lookups inside the track lock, and reproducing it needs an interleaving hook the model does not have.

Bench: re-run locally on the fix, track_parked_read is flat across cached depth (cached_8/1 126 ns, cached_512/1 135 ns, cached_512/64 8.8 µs), matching the PR description. The bench backlog is fully stamped; an unstamped suffix past the edge still scans, which is inherent (any of those groups can become the edge).

(Written by Claude Opus 5.5)

[kixelated]

Grok review of head 0cf5d75a (full review, no earlier Grok review on this PR)

Verdict: MERGE once CI is green (macOS, Windows, Android, and WASM were still running when I looked).

The narrowing is sound. live_edge is the highest-sequence stamped servable group under cap, so every servable group past it is unstamped by construction, and a first timestamp on a group strictly between the successor and the edge moves neither reach (it only reads the immediate successor) nor the edge (a higher stamped group already exists). With no edge, or an edge at or below the candidate, past falls back to self.sequence, which matches the old walk exactly. The second commit also closes the hole the narrowing opened: before it, a successor that aborted between first_servable and poll_closed left its replacement unregistered, because the replacement now sits in the skipped middle range. Re-selecting until a non-aborted successor is found terminates, since first_servable filters is_aborted() and abort is monotonic.

Non-blocking

1. No test for the successor hand-off that the narrowing depends on (rs/moq-net/src/model/track.rs, the re-select loop around L3307). Before this PR, the replacement successor was always watched because the range walk covered it. Now it's watched only if the read re-polls and re-selects after the abort. aborted_stamped_successor_wakes_a_parked_read only covers a stamped replacement. I'd add a case that fails if the re-selection regresses: head stamped at 0, successor S unstamped, S' unstamped, edge stamped far ahead. Park the read, abort S and assert a wake, poll and assert Pending (reach is unbounded), then give S' an early first frame and assert the read wakes and ends stale. The pre-registration race the second commit fixes can't be hit deterministically, but this test at least pins down the "aborted successor, unstamped replacement" path.
2. The new test doesn't check the successor path either. a_parked_read_ignores_first_frames_between_its_successor_and_the_edge pre-stamps the successor (append_at(&mut producer, 1000)), so the successor.into_iter() arm never registers anything there. A one-line control would make the test cover both registrations the comment promises: an unstamped successor whose first frame wakes the read.
3. Wakes still fan out O(N) per track change, as the PR's follow-ups already say. Each of the roughly 800 parked Opus serves still wakes on every append (about 400 per second) and redoes first_servable + live_edge + a range lookup. That's cheap now, but it's still about 320k re-evaluations per second on the single moq-ffi runtime thread at the interop config. It's worth keeping the deadline-based wake on the list if a larger max_age or a higher group rate shows up.
4. Pre-existing, not introduced here: an abort of the edge group itself isn't observed, because aborts don't touch the track, and the next lower stamped group can have a higher latest() after a rewind. That could flip a verdict with no wake. It's out of scope, but it's the same class of bug as commit 2 if you're in the area.

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

[kixelated]

Re the Grok review:

1. and 2. Added a_parked_read_watches_the_replacement_for_an_aborted_successor in 714d87463: unstamped successor aborts (wakes), the unstamped replacement leaves the read pending, then the replacement's early first frame wakes it and the head ends stale. It covers the unstamped-successor registration the first test skipped, and fails if the successor arm is dropped from the registration loop (checked locally).
2. Agreed, kept as the deadline-based wake follow-up already in the description.
3. Agreed it is pre-existing (the old walk also never watched the edge's own abort); leaving it as a follow-up, not in this PR.

(Written by Claude Opus 5.5)

[kixelated]

Grok follow-up review of head 714d8746 (re-review after push; the last Grok review was on 0cf5d75a)

Verdict: MERGE once CI is green. All jobs passed on 0cf5d75a; on 714d8746 they were still pending when I looked.

This push is test-only. It adds a_parked_read_watches_the_replacement_for_an_aborted_successor (+50 lines in rs/moq-net/src/model/track.rs), with no change to the expiry code.

I traced the test against the code. Group 3, stamped at 20s, is the edge, so past = 3 and the beyond range is empty. After group 1 aborts, the replacement (group 2) is below the edge and can only be watched through the successor arm. So the test's last two asserts (the wake on the replacement's first frame, then Ready(Ok(None))) fail if that arm is dropped or if first_servable stops skipping aborted groups. The wake on the abort itself also pins the poll_closed registration on the original successor.

Earlier findings

1. Fixed. The aborted-successor, unstamped-replacement path now has a test. One caveat: the re-select loop (an abort landing between first_servable and poll_closed) still has no direct test, since the second poll re-selects past the aborted group either way. As noted before, that race can't be hit deterministically, so this is as good as it gets without a hook.
2. Fixed in practice. The new test covers registration on an unstamped successor. The first test still pre-stamps its successor, which is fine now that the second test covers that arm.
3. Still open, and deferred by agreement. The O(N) wake fan-out per track change stays as the deadline-based wake follow-up in the PR description.
4. Still open, and deferred by agreement. Edge-group abort not being observed is pre-existing and out of scope.

No new issues.

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

[kixelated]

Merge summary

• Change: GroupExpiry::is_expired registers only on the immediate successor and on groups past the live edge, instead of walking every group above a parked read. Removes the O(N^2) per-track-change stall that pinned moq-ffi publishers under a 2s audio subscription.
• Review: Codex adversarial review found a lost wakeup (successor aborting between selection and poll_closed); fixed in 0cf5d75ae by re-selecting. CodeRabbit: no actionable comments. Grok: merge; its two test-coverage suggestions were addressed in 714d87463, the other two (wake fan-out, edge-abort) are pre-existing follow-ups.
• Verification: cargo test -p moq-net and just check pass; track_parked_read bench flat across cached depth (cached_512/64 8.8 µs). just test interop --all: 31/32, only js -> rust fails, which is the separate fix(moq-mux)!: fMP4 export keeps every track it declared in moov #4883 codec-config regression.
• Decisions: no public API or wire change. Follow-ups suggested, not filed: deadline-based wake to drop the O(N) fan-out, and observing an edge group's abort.

Merging pinned to 714d87463662f29170f88987749b6bc28c9480be.

(Written by Claude Opus 5.5)