Skip to content

fix(net): an append wakes only the parked reads it expires - #5005

Merged
kixelated merged 5 commits into
mainfrom
quest/m0/expiry-wakes
Oct 7, 2026
Merged

kixelated merged 5 commits into
mainfrom
quest/m0/expiry-wakes

Conversation

@kixelated

@kixelated kixelated commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Completes quest/m0/expiry-wakes.md.

Problem

A group read parked on its subscription's drift budget (GroupExpiry::is_expired, reached from the lite and IETF publishers' GroupServe) registered on every track-state change and on unstamped groups past the edge. Each append woke every parked read. An FFI publisher sending one 2.5 ms Opus frame per group to a subscriber with a 2 s max age parks ~800 serves, so the single runtime thread sat at ~110% CPU and the Python and Go interop publisher rows failed.

Fix

Deadline-indexed wakes, in a new model::expiry::Wakes owned by the track's cache::Track account (the link every group's frame writes already follow back to the track):

  • A parked read with a stamped successor parks on its deadline: the successor's start plus its budget, in floored micros. A frame write reports the timestamp it presented, and wakes only the reads whose deadline it reaches. An atomic earliest lets a write that crosses nothing skip the lock.
  • A read also parks on its own sequence. A visible group landing wakes only the reads between its nearest servable predecessor and itself, which are the reads it becomes the new successor of. Entries below the oldest cached group are pruned.
  • The successor's first frame and abort stay watched per group, as before. Budget and cap changes stay on their per-subscription channels. A replaced track Info wakes everything.
  • The read registers before it resolves the edge, so a write that crosses the deadline is either seen by the judge or wakes it.

A side effect: the edge group's later frames now count. Before, only a group's first frame past the edge woke a parked read, so a read waiting on the edge's own later frames could stay parked.

Tests

  • an_append_wakes_only_the_parked_reads_it_expires: 64 parked reads, one append, and only the 4 it expires plus the newest wake. It fails on main, where all 64 wake.
  • a_parked_read_ignores_first_frames_between_its_successor_and_the_edge is renamed to a_parked_read_wakes_only_once_the_edge_reaches_its_deadline. A new edge that falls short of the deadline no longer wakes the read. The edge's own later frame does.
  • track_parked_read now measures one append past N parked reads and re-polls only the reads it wakes. It is swept over N:
parked main this PR
8 2.36 µs 0.75 µs
64 14.2 µs 0.70 µs
512 167 µs 0.71 µs

Checks: moq-net tests (1550) and its loom models pass. The scoped just check passes except moq-uring worker::tests::dropped_worker_rejects_operations, which failed locally on RLIMIT_MEMLOCK. Other processes on the machine hold that limit (see quest/m1/uring-tests-under-load.md), and this change does not touch moq-uring.

Not verified: the quest's goal that just test interop --all passes the Python and Go publisher rows. The local run was stopped before it finished, so nightly Interop is the check.

Known limits

  • A read capped below the live edge (end_at / set_groups upper bound) is woken by every write past its deadline, because the deadline index does not filter by cap. That is the same order as before for those reads, and it is rare: bounded SUBSCRIBEs, and route handoffs in resume.
  • A rewound timeline whose edge group is then aborted can fall back to a lower group that already passed a read's deadline without a wake. The read is re-judged on its next wake.

Impact

  • Public API: none.
  • Wire: none.

Follow-ups

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

A group read parked on its drift budget registered on every track change,
so each append woke every parked serve. A 2.5 ms-frame publisher with a 2 s
max age parks ~800 serves and pinned its runtime.

Parked reads now index by deadline (successor start plus budget), woken by
the frame write that reaches it, and by sequence, woken by a group landing
above them. Sweep track_parked_read over parked readers.

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

Copy link
Copy Markdown
Collaborator Author

Quest outcome: deadline-indexed wakes landed as the quest decided. One append past 512 parked reads now costs about 0.7 µs, down from 167 µs on main. The mocked-time regression test fails on main and passes here.

Open item: the local interop run (Python/Go publishers) was stopped before it finished, so nightly Interop confirms the rows. The local moq-uring failure is RLIMIT_MEMLOCK exhaustion on the shared host, unrelated to this change. Left as a draft.

(Written by Claude Opus 5.5)

@kixelated
kixelated marked this pull request as ready for review October 7, 2026 15:56
@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 1 minute.

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: eb54642b-5513-47b9-ac32-619758708258
📥 Commits

Reviewing files that changed from the base of the PR and between fc1c09c and 5ed2463.

📒 Files selected for processing (11)
  • quest/m0/README.md
  • quest/m0/expiry-wakes.md
  • quest/m0/held-group-wakes.md
  • rs/moq-net/benches/track.rs
  • rs/moq-net/src/lite/publisher.rs
  • rs/moq-net/src/model/cache.rs
  • rs/moq-net/src/model/expiry.rs
  • rs/moq-net/src/model/group.rs
  • rs/moq-net/src/model/mod.rs
  • rs/moq-net/src/model/track.rs
  • rs/moq-net/tests/route_change.rs

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: ad95ef6d-8717-4b9b-945c-e551232bbf5c
📥 Commits

Reviewing files that changed from the base of the PR and between 409dc31 and fc1c09c.

📒 Files selected for processing (9)
  • quest/m0/README.md
  • quest/m0/expiry-wakes.md
  • quest/m0/held-group-wakes.md
  • rs/moq-net/benches/track.rs
  • rs/moq-net/src/model/cache.rs
  • rs/moq-net/src/model/expiry.rs
  • rs/moq-net/src/model/group.rs
  • rs/moq-net/src/model/mod.rs
  • rs/moq-net/src/model/track.rs
💤 Files with no reviewable changes (1)
  • quest/m0/expiry-wakes.md

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


Walkthrough

The track model adds a wake index for parked group reads, keyed by deadlines and group sequences. Frame presentation and group insertion notify the index, while GroupExpiry registers targeted wake conditions instead of polling the track. The changes also update expiry tests and the parked-read benchmark. Quest documentation now describes held-group wakes and replaces the prior expiry-wakes entry.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to fc1c0

No concrete issue remains that should block merging after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 73.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 6 files. (2 skipped: … 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 and concisely describes the main change: limiting append-triggered wakeups to parked reads whose deadlines are reached.
Description check ✅ Passed The description directly explains the problem, deadline- and sequence-indexed wake implementation, tests, benchmarks, known limits, and validation status.
Full details: Docstring Coverage

Explanation

Docstring coverage is 73.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 6 files. (2 skipped: 2 unsupported.)

✨ 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

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

@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated review of 6073863d (fix(net): an append wakes only the parked reads it expires)

The core change looks correct. I checked the register-before-judge order: the read registers its deadline under the parked lock before it resolves the edge, and the edge is read under the group lock that a writer drops before it calls presented. So the relaxed earliest fast path can't lose a wake. The floored deadline key is never later than the exact reach + budget, every state.stamp site in group.rs (543, 610, 700) now reports to Wakes, and the landing range [below, sequence) still covers out-of-order group arrival. The new tests check the intended wake counts.

Should fix before merge

  1. The quest is deleted, but its moq-net: a group held across a route switch is not woken when its successor gets its first timestamp #4950 half isn't done. quest/m0/expiry-wakes.md folded in moq-net: a group held across a route switch is not woken when its successor gets its first timestamp #4950 and listed it under Closes. Its plan had Recover::poll register a held group in the same index, plus the test a_group_no_route_continues_wakes_when_its_successor_is_stamped. This PR doesn't touch resume.rs. Recover::poll (resume.rs:898) still judges through track::Consumer::poll_stale (track.rs:2689), which only registers on whole-track state changes. So a held group is still woken by the next unrelated append, not by its successor's first stamp or abort. That test doesn't exist, and moq-net: a group held across a route switch is not woken when its successor gets its first timestamp #4950 is still open. The quest's goal sentence ("A group held across a route switch is woken when it goes stale") isn't met either. Once the quest file is gone, nothing tracks this work. Either do the Recover part here, or keep a smaller quest for it, linked to moq-net: a group held across a route switch is not woken when its successor gets its first timestamp #4950, and say so in the PR body.

Non-blocking

  1. Deadline entries are only cleaned up when the edge crosses them (expiry.rs:34, :70). Landings get pruned below oldest, but deadlines doesn't. A read that finishes or drops leaves its WaiterList keyed in the map until the edge passes reach + budget. If a track stalls or its publisher stops, those entries stay for the life of the track. That's bounded by about one budget's worth of groups (around 800 in the 2 s / 2.5 ms audio case), but with a long max age and tiny groups it grows to tens of thousands of map entries per track. A cheap fix: in landed, drop leading deadlines entries whose lists are all dead, or prune them by sequence along with landings.
  2. presented also fires for writes that can never move the edge (group.rs:543/610/700). That includes invisible fetched backfill and fetched head copies. In a normal timeline those timestamps sit below every deadline, so this costs nothing. But after a timestamp rewind, a backfill fetch of old-timeline content (higher timestamps) wakes every parked read on each frame. That's the same N-wide fan-out this PR removes, in a rare case. If it ever matters, the fix is to skip the report for groups created with visible == false.
  3. CI is still queued on this head. The interop Python and Go publisher rows, which were the quest's actual goal, haven't been verified yet (the PR body says so).

Verdict: ITERATE (small). The code is ready once CI is green. What's left is keeping #4950 tracked, or finishing it.

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

The Recover::poll registration folded in from #4950 is not done here, so it
moves to its own quest instead of disappearing with expiry-wakes.md.

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

Copy link
Copy Markdown
Collaborator Author

Replies to the Grok review of 6073863d:

  1. Agreed. fc1c09c moves the moq-net: a group held across a route switch is not woken when its successor gets its first timestamp #4950 half (Recover::poll registering a held group in the same index, plus the a_group_no_route_continues_wakes_when_its_successor_is_stamped test) into its own quest, quest/m0/held-group-wakes.md, which carries moq-net: a group held across a route switch is not woken when its successor gets its first timestamp #4950 under Closes. This PR stays scoped to the GroupServe fan-out.
  2. Not changing. Dead deadline entries are bounded by one budget's worth of distinct deadlines per track and go away when the edge passes them or the track's Info is replaced. Pruning them needs a liveness check on every WaiterList, which costs more than it saves until a profile shows it.
  3. Not changing. It only fires after a timestamp rewind with a backfill fetch in flight, the case the PR body already lists as rare. Skipping invisible groups is the right fix if a profile ever shows it.
  4. CI is green on that head. The Python and Go interop rows are left to nightly Interop, as the PR body says.

(Written by Claude Opus 5.5)

Copy link
Copy Markdown
Collaborator Author

Automated follow-up review of fc1c09c5 (re-review after push, since 6073863d)

The push is one docs-only commit (fc1c09c5, quest files only; no Rust changed). It adds quest/m0/held-group-wakes.md and lists it under Required in quest/m0/README.md.

Earlier findings

  1. Fixed. The moq-net: a group held across a route switch is not woken when its successor gets its first timestamp #4950 half is tracked again. The new quest carries moq-net: a group held across a route switch is not woken when its successor gets its first timestamp #4950 under Closes, the Recover::poll / Consumer::poll_stale plan, both index differences (an entry with no deadline yet, and an entry that moves with the serving route), and the regression test. The PR body no longer claims to close moq-net: a group held across a route switch is not woken when its successor gets its first timestamp #4950, which matches. The claims match the code: Wakes lives on cache::Track, and Recover::poll still only registers on whole-track state through poll_stale, so "today it waits for the next unrelated append" is accurate.
  2. Declined, reasonably. Dead deadline entries stay bounded by about one budget of distinct deadlines per track. One small correction to the reply: wake_all only runs when Info is replaced (install skips the first set), so in practice the cleanup comes from the edge crossing the entries.
  3. Declined, reasonably. Wakes from invisible backfill writes only matter after a rewind. Skipping visible == false groups is still the fix if a profile shows it.
  4. CI: Quest passed on this head. Check, Test, WASM, Android, Windows and macOS are still running. The code is unchanged from 6073863d, which was green. The Python and Go interop rows are still left to nightly Interop.

New, nit

Verdict: MERGE once CI is green on fc1c09c5.

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

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

Copy link
Copy Markdown
Collaborator Author

Merge summary for 85ce7533:

(Written by Claude Opus 5.5)

# Conflicts:
#	rs/moq-net/benches/track.rs
#	rs/moq-net/src/model/track.rs
@kixelated

Copy link
Copy Markdown
Collaborator Author

Merged origin/main into this branch (5ed2463) to clear conflicts with #4917's max_age to max_delay rename. The resolution keeps this PR's GroupExpiry::is_expired and track_parked_read bench, with the new names. It also carries the same three test renames as #5016, since main's moq-net tests don't build without them; whichever lands first, the other merges cleanly. moq-net tests (1552) pass locally.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge summary for 5ed24638:

  • Since the last review (Grok, MERGE on fc1c09c5): 85ce7533 rewords one quest line per the Grok nit, and 5ed24638 merges origin/main. I checked the merge's conflict resolution: main's side of benches/track.rs and model/track.rs was only the feat!: name subscriber staleness max_delay; publisher retention keeps max_age #4917 max_age to max_delay rename, so keeping this PR's GroupExpiry::is_expired and track_parked_read bench with the new names drops nothing from main. The carried test renames in lite/publisher.rs and tests/route_change.rs match fix(net): finish the max_age to max_delay rename in tests #5016 exactly, so either order merges cleanly.
  • No code findings are outstanding. Grok findings 2 and 3 stay declined as documented limits, per the maintainer decision above.
  • Nightly Interop confirms the Python and Go publisher rows after merge.

Auto-merge is enabled on this head.

(Written by Claude Opus 5.5)

@kixelated
kixelated merged commit 9d5cf8d into main Oct 7, 2026
9 checks passed
@kixelated
kixelated deleted the quest/m0/expiry-wakes branch October 7, 2026 19:15
kixelated added a commit to Dryvnt/moq that referenced this pull request Oct 7, 2026
Adapts to moq-dev#5005 (an untimed frame presents nothing, so it wakes no parked read) and the max_age to max_delay rename (moq-dev#4917).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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