feat(net): move wall-clock LRU expiry to the cache pool - #3169
Conversation
Track max_age used to drive two different clocks: the media-timestamp staleness budget and the wall-clock idle scan in evict_expired. The wall-clock half now lives on cache::Pool as an expiry window (default 30s, None to disable), so a track can promise a 5s media window while the pool reclaims anything unaccessed for 30s of real time. Media timestamps stand still during a congestion stall, so a short max_age can no longer age content out of the cache mid-stall; the LRU window is purely about not pinning RAM on content nobody reads. The origin's cache_duration keeps only its media-time role: clamping each track's advertised max_age. The relay's cache-duration knob now also sets the pool expiry, which is what its docs already promised (retained since last written or served from cache). Frame-write settlement now runs the expiry scan too, so a track appending frames to open groups without inserting new ones still ages idle siblings out. Progress on #3161 (the Rust reclamation half). Still open there: idle reclamation with zero writes needs a wakeup the model layer deliberately does not own, and the JS retention prune still keys off maxAge and retains open groups. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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. |
![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: 3afea3d1b8
ℹ️ 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".
| let Some(state) = self.state.upgrade() else { return }; | ||
| if let Ok(mut state) = state.write() { | ||
| state.charge_debt(); | ||
| state.evict_expired(); |
There was a problem hiding this comment.
For a track that keeps adding frames to an existing open group but writes less than 256 KiB after the previous settlement, settle returns before this new eviction scan runs. Consequently, a configured 30-second expiry can retain idle sibling groups for minutes on low-bitrate tracks, and the four-entry scan can make a large backlog take much longer to clear, despite the documented claim that expiry is evaluated when a track writes. Drive expiry from a separate time-based gate rather than the byte-debt threshold, and cover a sub-threshold writer with a regression test.
AGENTS.md reference: AGENTS.md:L141-L145
Useful? React with 👍 / 👎.
| Self { | ||
| used: AtomicU64::new(0), | ||
| capacity: AtomicU64::new(u64::MAX), | ||
| expiry: AtomicU64::new(DEFAULT_EXPIRY.as_millis() as u64), |
There was a problem hiding this comment.
Initializing every pool with a 30-second expiry means Pool::unbounded() now aborts idle non-latest groups, while that public method still promises to create a pool that never evicts. A caller choosing unbounded() specifically to preserve cached history will unexpectedly receive Error::Old unless it discovers and adds with_expiry(None). Either preserve the documented behavior or explicitly redefine the method as byte-unbounded and direct callers to the expiry opt-out.
AGENTS.md reference: AGENTS.md:L115-L117
Useful? React with 👍 / 👎.
1 participant
Progress on #3161, scoped to where the LRU window is configured.
What changed
track::Info::max_ageused to drive two different clocks. It is the media-timestamp staleness budget (is_stale, post-#3099), butevict_expiredalso used it as a wall-clock idle window: a group unaccessed formax_ageof real time was aborted. That conflation means a 5s media window also nukes the cache after a 5s congestion stall, and there was no way to say "5s of media retention, but keep idle content around for 30s in case someone fetches it".The wall-clock half now lives on
cache::Pool:Pool::with_expiry(Option<Duration>)/Pool::expiry(): the LRU window, defaulting to a newcache::DEFAULT_EXPIRY(30s).Nonedisables idle reclamation entirely, leaving only the byte budget.TrackState::evict_expiredreads the window from the pool instead of taking the track'smax_age.max_ageis now purely a media-timestamp promise: a stall (no accesses, no timestamp progress) inside the LRU window can never age content out, so tracks are immune to congestion bursts.cache::Track::settle) now runs the expiry scan alongside debt payment, so a track that only appends frames to open groups still ages its idle siblings out instead of needing a new group insertion.refresh_if_stale) keys off half the pool window instead of halfmax_age, and stays bounded even when expiry is disabled (the stamp also protects against byte-budget eviction).origin::Info::cache_durationkeeps only its media-time role: clamping each track's advertisedmax_age(what subscribers may rely on).cache-durationknob now also sets the pool expiry, which is what its documentation already promised ("retained since it was last written or served from cache by a FETCH"). Unset leaves the 30s default in force, so a long-running relay no longer accumulates unbounded idle history when no knobs are set.Root cause
Issue #3161: after #3099 made subscription expiry timestamp-only, wall-clock reclamation was left keyed to a per-track media property, so it moved with the wrong knob (a long
max_agemeant unbounded idle RAM; a short one meant congestion stalls emptied the cache). The window is a cache policy about RAM, so it belongs to the pool that owns the RAM budget.Tests
pool_expiry_controls_eviction,disabled_pool_expiry_never_reclaims,expiry_configcover the new knob.max_age_does_not_drive_wall_eviction(a 1smax_agetrack survives a 10s wall stall) andorigin_cache_duration_does_not_wall_evict.DEFAULT_MAX_AGEtocache::DEFAULT_EXPIRY.just checkandjust testpass (3365 tests); rustdoc with-D warningsis clean.Cross-package sync
doc/bin/relay/config.mdcache section anddoc/concept/layer/moq-lite.mdupdated (the latter still described the pre-refactor(net)!: make group delivery order a handle, and retire the Ordered wire field #3099 dual-clock subscriber budget).js/netdeliberately not mirrored here: it has no pool to hang the knob on, its prune keys off insertion time rather than last access, and it retains open groups; that is the JS half Retention should reclaim idle open groups now that expiry is timestamp-only #3161 tracks and needs its own design.moq-ffiexposes onlycache_capacity_bytes; an expiry option there would fan out to every binding and is left as an additive follow-up (FFI users get the 30s default automatically).Still open on #3161
Expiry is still evaluated only when a track writes. A fully idle publisher keeps its cache pinned until it resumes or disconnects, and a parked reader on an abandoned open group still waits forever; fixing that needs a wakeup, which the model layer deliberately does not own (
model/clock.rs), so it belongs to a timer-owning layer.🤖 Generated with Claude Code
(Written by Claude Fable 5)