Repository navigation
fix(net): sample a closing subscription's lag once more - #4451
Conversation
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Outcome: quest implemented and checked locally ( (Written by Claude Sonnet 5.5) |
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: cff4c75e3d
ℹ️ 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".
| /// is gone, so no tick will find this frontier again: record the bytes produced | ||
| /// since the last sample now, at the lag they last had. | ||
| fn drop(&mut self) { | ||
| if let Some((lag, weight)) = self.sample(crate::model::clock::now()) { |
There was a problem hiding this comment.
If the producer has already ended and this is the last subscriber, dropping track::Subscriber destroys its inner field before stats_sub because of their declaration order in model/track.rs; that can destroy the cache track before this destructor runs. Since FrontierState stores only a Weak, sample() then fails the upgrade at lines 1921-1922 and discards the source without recording any bytes produced since the previous tick, so the documented final sample is still lost on this common end-of-track path. Ensure a sample occurs while the subscriber still keeps its source alive, and add a regression case where the producer closes before the subscriber. The repository's handle model makes last-handle teardown an expected closure path.
AGENTS.md reference: rs/AGENTS.md:L13-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5b98e2b at the source rather than by field order: the same drop-order hazard also exists in GroupServe (group drops before delivery), so reordering one struct would not cover it. Each frontier source now holds the track's Production strongly (Arc) and keeps the Weak only for liveness, so the final sample reads a dead track's last bytes regardless of which handle drops last. Regression tests: the_last_subscriber_samples_a_track_its_own_drop_releases and an_in_flight_delivery_samples_a_track_that_already_closed (both fail without the fix).
(Written by Claude Opus 5.5)
|
Recommendation: MERGE Positive stats correctness fix: lag was only sampled on registry ticks, so bytes after the last tick (or a sub that opened and closed between ticks) never hit the histogram. Sampling once more in This is an automated review, not the maintainer's decision |
The frontier held each source only by a Weak to its track, so when the track died first (the subscriber's state dropping before its stats guard, or a group serve dropping its group before its Delivery), the final sample failed the upgrade and discarded the bytes since the last tick. Sources now hold the track's Production strongly and use the Weak only for liveness. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b98e2bdb9
ℹ️ 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((lag, weight)) = self.sample(crate::model::clock::now()) { | ||
| self.counters.publisher.lag.record(lag, weight); |
There was a problem hiding this comment.
When the last producer and subscriber handles disappear concurrently with Registry::report, this record can still be lost from the per-broadcast report. report samples and snapshots TierCounters before its later entries.retain; if this destructor runs after that snapshot but finishes before retain checks Arc::strong_count(counters), the lag is updated here and the counters are then retired and removed without refreshing the already-built Report, so no later tick emits the final sample. The new Production Arc fixes the earlier dead-track case, but this snapshot/prune interleaving is fresh evidence that the final-sample guarantee still fails; retain such late updates for the next report and cover synchronized last-handle teardown.
AGENTS.md reference: rs/AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 13facea. The window was general to Registry::report (any guard's last bump, not only lag, could land between the readout and the prune), so the fix is there: each tier now decides it is done (orphaned entry, counters held only by the map) before its readout, with an acquire fence pairing the last holder's decrement. A bump racing the report then lands in this readout, or the counters survive to the next one, which retires them with it. No deterministic regression test: the old window sits inside one lock scope with no hook to interleave a drop into, so reproducing it would need a thread race; the ordering is now structural instead.
(Written by Claude Opus 5.5)
Registry::report read every entry out, then pruned those only the map still held. A guard whose last bump (such as a frontier's final lag sample) landed between the two was retired without that bump reaching any per-broadcast report. Each tier now decides it is done before its readout, so a racing bump lands in this readout or the next. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Landing summary:
Merging on the maintainer's go-ahead, CI green, Codex thumbs-up on 13facea. (Written by Claude Opus 5.5) |
1 participant
Problem
Counters::sample(rs/moq-net/src/stats.rs) samples lag only onRegistry::reportticks and prunes deadFrontierInnerweak references without sampling them. When an egress subscription closed, the bytes its track produced since the last tick never reached thelaghistogram, and a subscription that opened and closed between two ticks was never sampled.Approach
FrontierInnernow takes one lastsample(now)into the same counters inDrop. The subscription guard and every in-flight groupDeliveryhold a strong reference, so it fires once the last of them is gone.sampleadvances each source'ssampledbytes under the frontier lock, so a tick racing the drop finds nothing left to count again. The frontier holds anArcto itsTierCountersuntil after the sample, so the report's entry pruning cannot discard it first. Each source holds its track'sProductionstrongly (theWeakto the track now only reports liveness), so the final sample still reads a track that died first, whichever of the subscriber's state, its guard, a serve task's group, or itsDeliverydrops last.Registry::reportnow decides a tier's counters are retired before reading them out, so a last bump racing a report (this final sample, or any guard's) lands in that readout or the next instead of being retired unreported.Tests (each fails without the fix): opens and closes between ticks lands its bytes once; closes mid-interval adds exactly the bytes since its last sample; an in-flight
Deliverydefers the final sample to its own drop; the last subscriber, or aDeliveryoutliving it, still samples a track the producer already closed.doc/concept/stats.mdnotes the behaviour. The quest is deleted and its entry removed from the QoS README.Ordering with #4381 (lag-splice, same base): the drop reuses
FrontierInner::sample, so whatever weight that PR defers (unsampled, retired sources) is kept by the final sample automatically, with no separate copy of the logic. The two touch different hunks (this adds animpl Drop, #4381 editssample/unwatch), so they should merge in either order; if #4381 lands first, the existing tests here still apply unchanged. Until #4381 lands, the pending-weight loss it describes (sampletakingunsampledbefore a source suppliesnewest) also applies at drop time.Impact
laghistogram's values change (a closing subscription's last interval is now counted).Alternatives
track::Subscriberfields sostats_subdrops beforeinner: the same hazard exists in the serve tasks (groupdrops beforedelivery), so field order would have to be kept right in every holder.Subscription::drop: misses bytes still owed while aDeliveryoutlives the guard, and does not cover a frontier dropped by another path.Follow-ups
None.
(Written by Claude Sonnet 5.5, updated by Claude Opus 5.5)
🤖 Generated with Claude Code