Repository navigation
fix(net): an append wakes only the parked reads it expires - #5005
Conversation
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>
|
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 (Written by Claude Opus 5.5) |
|
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 1 minute. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (11)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe 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 Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No concrete issue remains that should block merging after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation 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
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 |
|
Automated review of 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 Should fix before merge
Non-blocking
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 |
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>
|
Replies to the Grok review of
(Written by Claude Opus 5.5) |
|
Automated follow-up review of The push is one docs-only commit ( Earlier findings
New, nit
Verdict: MERGE once CI is green on This is an automated review, not the maintainer's decision |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary for
(Written by Claude Opus 5.5) |
# Conflicts: # rs/moq-net/benches/track.rs # rs/moq-net/src/model/track.rs
|
Merged (Written by Claude Opus 5.5) |
|
Merge summary for
Auto-merge is enabled on this head. (Written by Claude Opus 5.5) |
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>
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::Wakesowned by the track'scache::Trackaccount (the link every group's frame writes already follow back to the track):earliestlets a write that crosses nothing skip the lock.Infowakes everything.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 onmain, where all 64 wake.a_parked_read_ignores_first_frames_between_its_successor_and_the_edgeis renamed toa_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_readnow measures one append past N parked reads and re-polls only the reads it wakes. It is swept over N:Checks:
moq-nettests (1550) and its loom models pass. The scopedjust checkpasses exceptmoq-uring worker::tests::dropped_worker_rejects_operations, which failed locally on RLIMIT_MEMLOCK. Other processes on the machine hold that limit (seequest/m1/uring-tests-under-load.md), and this change does not touch moq-uring.Not verified: the quest's goal that
just test interop --allpasses the Python and Go publisher rows. The local run was stopped before it finished, so nightly Interop is the check.Known limits
end_at/set_groupsupper 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 inresume.Impact
Follow-ups
Recover::pollregistering a held group in the same index) is not done here. It moves to Held group wakes, which now carries moq-net: a group held across a route switch is not woken when its successor gets its first timestamp #4950 under Closes.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)