Repository navigation
fix(net): forget spliced tracks unread for the linger, finished ones included - #4361
Conversation
…included A relay's front removed a finished or aborted logical track from its machine, so the idle linger never ran for it and the broadcast kept it (and the source copy's cache) for the life of the front. Tracks now stay in the machine until unread for TRACK_IDLE_LINGER, then are removed from the broadcast. The same covers parked tracks and lookups whose reader left before the front saw them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. WalkthroughTrack assignment now records its time and starts unread tracks in a parked state. Finished and aborted tracks remain available during the linger period. When a track is due for forgetting, the front requests removal and waits for confirmation; if a reader arrives first, the track remains. The origin clears ended source state and uses producer identity and reader state when processing forget requests. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Track forgetting has no established merge-blocking issue; merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new cleanup flow changes when cached tracks are retained, but the inspected reader safeguards and event ordering limit the apparent security risk. Production behavior and rollout coverage remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
MERGEPositive improvement, worth the complexity, and the approach looks right. What this fixes. Finished and aborted tracks used to leave the front machine immediately, so Why this shape. Renaming Complexity. +317/−55 across four files is real, but most of it is the Nits (non-blocking). CI was still pending when this review ran — worth a glance once Check/Test finish. The aborted-track unit test stops at Recommendation: MERGE once CI is green. This is an automated review, not the maintainer's decision |
The demand poll is level-triggered, so a reader that blocked the forget and left before the next poll left the track unread with no linger armed. Also assert the aborted track leaves the machine once forgotten. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3155fc8c74
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| track.state = TrackState::Idle; | ||
| track.ended = true; |
There was a problem hiding this comment.
When a relay churns short-lived tracks, retaining each finished track here for 30 seconds makes self.tracks proportional to the recent completion rate, while every Front::step calls next_deadline(), which linearly scans that entire map. A workload completing R tracks per interval therefore turns subsequent assignments, demand edges, and completions into O(R²) work during the linger window, whereas finished tracks previously left the map immediately. Maintain the next expiry without a full-table scan, and add a churn benchmark that sweeps the track count to expose the slope.
AGENTS.md reference: AGENTS.md:L35-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed the scan is linear, but it predates this PR and is not the dominant term: the driver in origin::run_front also polls every track in tracks on each wake, so both are O(tracks) per step. This PR bounds that table to tracks read within the linger; before it, finished tracks left, but aborted, released, and never-read ones stayed for the life of the front, so the table was unbounded. Fixing only next_deadline would leave the same slope in the driver.
Leaving this for a follow-up that sweeps a churn benchmark over tracks per front and replaces both scans (an expiry index in the machine, per-track wakes in the driver).
(Written by Opus 5.5)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8bfa11cf0
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(io) = tracks.get_mut(&name) | ||
| && !broadcast.forget_spliced(&name, &io.resume) |
There was a problem hiding this comment.
When a cached or finished track is read and fully dropped between driver polls, which is possible because cached reads can complete without yielding to run_front, poll_used only observes the current level and no Used/Unused event refreshes Parked::since. At the old deadline this check sees is_used() == false and forgets the track even if its last reader left moments ago, so the next reader unnecessarily requests the source instead of receiving the promised 30-second cache linger. Preserve a demand generation or otherwise restart the linger after this transient-use race, and cover the concurrent handoff in loom.
AGENTS.md reference: rs/moq-net/AGENTS.md:L14-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, but not new here and not a correctness issue. Parked tracks on main have the same gap: a reader that comes and goes between driver polls does not refresh Parked::since, so Release drops the warm copy at the old deadline. This PR only extends the same linger to finished and aborted tracks. The cost is one extra source request after a sub-poll read, and the next reader still gets correct data.
Closing it properly needs a demand generation on the channel itself (the consumer count in kio is level-only), which is a change to the kio primitive plus a loom model, so it belongs in its own PR. Leaving it as a follow-up.
(Written by Opus 5.5)
SummaryLanded at Changes since review
Codex findings (replied, not fixed)
Edited: this comment first went up with another PR's summary by mistake. (Written by Opus 5.5) |
Problem
A relay's front (
origin::run_front) removed a logical track from its state machine as soon as it finished or aborted. The idle linger (TRACK_IDLE_LINGER, 30s) only runs for tracks in the machine, so a finished track stayed in the broadcast'sspliced.tracksmap, and kept the source copy's cache pinned through its segments, for the life of the front. A long-lived broadcast that churns short-lived track names grew without bound. Aborted tracks were only reclaimed if someone looked the name up again, and tracks that were parked and released, or never read at all, left a stub per name.Found while reviewing how ended and aborted tracks are cached after #4351.
Approach
One rule: a track nobody reads for the linger is forgotten, meaning removed from the broadcast along with everything behind it.
front.rs: finished and aborted tracks stay in the machine with anendedflag, so they are never spliced again but still follow the demand edges. Every unread state goes toParked { since }, including a freshly assigned track, which now starts unread untilUsedarrives. The deadline emitsAction::Forget(replacingRelease), and the driver confirms withEvent::Forgotten. A local source's unread track is forgotten at once, as it was released at once before.origin.rs:FinishandAbortnow drop the track's source handles (TrackIo::end) but keep the entry.Forgetcallsbroadcast::Producer::forget_spliced, which removes the entry only if it has no reader. Lookups hand out readers under the same lock, so a reader arriving after the decision keeps the track. The driver feeds that reader'sUsededge itself, since the level-triggered demand poll would miss a reader gone before the next poll and leave the track with no linger armed.Tests
origin_impl::tests::finished_track_is_forgotten_after_the_linger: a finished track is served from the front within the linger (no new source request), and after the linger the next reader asks the source again. It fails onmain.frontunit tests: a finished track lingers, is not re-spliced on failover, then is forgotten; an aborted track lingers then is forgotten; a reader racing the forget keeps the track; a local unread track is forgotten at once; a never-read assigned track is forgotten. The exhaustive walk test now includesForgotten.just checkpasses.Impact
resume::Producer::is_cloneandbroadcast::Producer::forget_splicedarepub(crate).Follow-ups
next_deadlineand the driver's per-wake poll both scan every track, so each step is O(tracks within the linger). Needs a churn benchmark swept over tracks per front, then an expiry index and per-track wakes.kio), for parked tracks onmainas well. Costs one extra source request; a demand generation inkiowith a loom model would close it.Refs #4351.
🤖 Generated with Claude Code
(Written by Claude Opus 5.5)