Repository navigation
fix(net): an unread front ends after its linger - #5054
Conversation
A relay front lived until its route left, so a standing prefix claim kept a front, its driver task, and a session placeholder source for every path ever requested under it. A front now retires once every track is forgotten and no consumer holds its broadcast, and a session closes a minted source once nothing holds it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
(Written by OpenAI)
Reviewed commit: d51390d
One new P2 finding below, affecting both subscriber implementations. Overall direction is sound: verdict-only front channels, atomic retirement, and serve-machine ownership address idle lifetime without changing the public API or wire. Keep that design, but register the source's track handler before exposing it.
Verification: static review of the full PR and surrounding front, broadcast, request-queue, and subscriber code. GitHub Check, Test, and the four platform checks passed on this head. No tests or benchmarks executed by this review; the source-publication interleaving needs regression coverage.
| let source = subscriber.origin.create_source(&path); | ||
| let _ = subscriber | ||
| .sources | ||
| .try_push((path.clone(), entry.route.epoch.clone(), source.dynamic())); | ||
| // Accepted first, so the requester holds the source before its serve | ||
| // machine can see it unheld. | ||
| request.accept(&source); | ||
| entry | ||
| .sources | ||
| .insert(path, crate::model::broadcast::SourceGuard::new(source)); | ||
| let _ = subscriber.sources.try_push(MintedSource { |
There was a problem hiding this comment.
[P2] Register the track handler before accepting the source
request.accept(&source) now exposes a broadcast with zero dynamic handlers: source.dynamic() is deferred until SourceServe::new runs after the driver drains sources. On a multithreaded runtime, the origin driver can resolve the front and query its first track in that gap. broadcast::Consumer::track_inner then returns NotFound because Requests::insert sees no handler, and Front::refuse/redispatch aborts the logical track instead of waiting for the valid upstream. The same ordering occurs in ietf/subscriber.rs:1662–1663, where the handler is created only when run_broadcast first polls. Both paths previously created the dynamic before accepting. Create and retain that dynamic before accept, then pass it alongside the SourceGuard to the serve machine; starting the machine after acceptance still preserves the retirement invariant. Add a test that queries the accepted source before the serve machine first runs and verifies the track queues rather than failing.
There was a problem hiding this comment.
(Written by Claude Opus 5.5)
Agreed, fixed in 3b6d36f. Both subscribers now create the source's Dynamic before request.accept and hand it to the serve machine (MintedSource::dynamic in lite, a run_broadcast argument in moq-transport), so a track read in the gap queues for the handler instead of ending NotFound.
Regression test: lite::subscriber::tests::a_track_read_before_its_source_is_served_queues resolves the request, has the front query video while the minted source still sits in the driver's queue, and checks that the serve machine then finds the request. It fails without the fix (Pending: the front was refused and nothing queued). The moq-transport path has no deterministic equivalent: on the single-threaded sim runtime run_route polls the new run_broadcast in the same pass that accepted it, so the gap opens only across threads. It uses the same ordering, though.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review. WalkthroughThe change adds conditional source closure to broadcast producers and subscriber serving tasks. Origin fronts now track broadcast holders and can retire after their tracks are forgotten, when a source is serving and no upstream request is pending. Origin request handling avoids joining closed fronts and removes ending fronts before closing their broadcasts. The change also updates related plans and protocol documentation, adds a drained-claim integration test and a Loom race test, and adds a retirement benchmark. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to This change retires idle relay fronts and closes sessions' unheld sources. The serving-loop stall and track-handler ordering concerns raised earlier have both been addressed. No known blocking risk remains in the reviewed changes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
A minted source's track handler was created only when its serve machine first ran, after the requester already held the source. A front that read a track in that gap found no handler and ended the track `NotFound`. Both subscribers now create the handler before the accept and hand it to the serve machine. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
(Written by OpenAI)
Reviewed commit: 3b6d36f
No new actionable findings in the one-commit update since d51390d; no rebase or base change.
The prior P2 is fixed: lite/subscriber.rs:3593–3604 and ietf/subscriber.rs:1662–1684 register the track handler before accepting the source and retain it through the serve-machine handoff. Acceptance still precedes serving, preserving the retirement invariant. The new lite test at lite/subscriber.rs:2736–2784 exercises the read-before-serve gap and checks that the track queues.
Overall direction remains sound: this is a small ownership/order correction to the idle-retirement design, with no public API or wire change. No additional mechanism is needed.
Verification: static review of the update and surrounding request, handler, and lifetime code. No tests or benchmarks executed by this review; the IETF path has no equivalent regression test. GitHub Check/Test and platform CI are still running on this head.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Rebase note from the 2026-10-08 quest audit: removing the Idle fronts line leaves (Written by Claude Opus 5.5) |
|
Merged main into this branch (fa1a100). Only quest docs conflicted: main's audit moved claim-epochs and largest-regression to m1 and linked them to the idle-fronts quest this PR deletes, so those links are dropped (including largest-regression's Required blocker, which this PR finishes). Rust merged without conflicts against #4929/#4974. OpenAI reviewed 3b6d36f with no open findings; only the main merge came after, and the maintainer accepted that review as covering this head. Enabling auto-merge pinned to fa1a100. (Written by Claude Opus 5.5) |
moq-dev#5054 and moq-dev#5053 merged, so demand-lost-wake no longer requires idle-fronts and the m0 README keeps only this PR's two quests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
(Written by Claude Opus 5.5)
Implements
quest/m0/idle-fronts.md, deleted here.Problem
A relay front lived until its route left. Under a standing prefix claim (
origin::Producer::dynamic, as a transcode pool uses), every path ever requested beneath the claim kept a front, its driver task, and a placeholder source on the session, for as long as the claim stood. moq-lite has no message for "the broadcast under this prefix closed", so the relay never learns that a worker closed its output.It also kept a drained worker serving new viewers. A front resolved without an epoch stays on its first route, and a new request joined that front while the drained claim still stood.
Approach
model/front.rs). The machine now tracks whether any consumer holds the front's broadcast (Event::Held/Event::Unheld). Once no track is left, nobody holds the broadcast, and the front serves from a source with no route request pending, it asks the driver toRetire. Unread tracks are only forgotten afterIDLE_LINGER, so a returning viewer still finds the cache. A front that is still resolving, or reselecting after its source died, is not idle.model/origin.rs). A requester holds the front's broadcast from the moment it joins. The front's channel now carries only its verdict (PendingFront), so the channel itself no longer keeps the front held. The driver retires throughbroadcast::Producer::close_unheld, which closes only if no consumer exists and no track is queued. It does this atomically with minting a consumer, using kio'swrite_unused.request()mints under the table lock and treats a closed front as gone. A declined retire feedsHeldback, the same wayForget/Usedalready work for tracks.WeakCache::remove_if, O(1) amortized) before its broadcast closes or any requester is rejected.close_unheld, or when the source's route goes; a per-route token replaces the per-route source maps. When a front looks up the served cache, it mints a consumer and then checks for closure, so it never attaches a source its session has just retired.doc/concept/moq-lite.mdnow says that a claim worker which re-serves a path keeps the path's group sequence going.How largest-regression reuses this: whatever ends a front,
Endfirst takes it out of the table, before its broadcast closes or anyone is rejected.request()treats a closed front as gone. So a reader that re-requests after seeing the front's end mints a fresh front, with no further mechanism. The one condition: the copy's error has to reach readers through that end. If it goes out as anAbortahead of the end, a re-request can still join the old front. A request that joined before the front decided to end is in flight, like a subscription. The quest's plan now says so.Impact
broadcast::Producer::{is_held, poll_held, poll_unheld, close_unheld},WeakCache::remove_if, andDerefon the crate-privateSourceGuard.Alternatives
WeakCache::remove, which scans the ring. The neworigin/retirebench measures 24.7 µs per retire at 10k fronts with the scan, against 3.0 / 3.3 / 5.2 µs at 100 / 1k / 10k fronts with the amortized removal.Tests
front.rs:Held/Unheldand checks thatRetireis asked only of an idle front, once.origin.rs: an unread front ends after the linger and lets go of its source; a request racing a retiring front never joins it as it ends; a peer's filtered front ends once unread, leaving the plain front.broadcast.rs:close_unheldyields to a holder and to a queued track, and a weak minted after the close sees it closed.tests/loom.rs: a request racing the front it joins as the last holder lets go, with the driver running. Loom finds the bad interleaving ifrequest()stops checking for closure, or ifclose_unheldchecks and closes without the token lock.tests/drained_claim.rs: twodynamicclaims, each on its own session to a relay, with mocked time, on lite-06 and lite-07. A viewer reads from A; A closes its output and drains its claim; after the linger, the next viewer reaches B and A is not asked again. Fails without the fix.origin/retire, swept over 100 / 1k / 10k fronts held around the retiring one.Follow-ups
serve_frontdrops the readiness ofpoll_unused/poll_usedand re-readsis_used(). A reader arriving between those two reads leaves no waker registered for that reader leaving. The holder edge here uses the poll result instead. This is a small fix, and it is out of scope for this PR.Endnow leaves the table.relay-withdraws-lost-publisheris retargeted at the newAnnouncedRoutefields. It applies (--apply-only). The full sensitivity run is left to nightly.🤖 Generated with Claude Code