Repository navigation
fix(net): backport stop idle viewer sessions from waking every 100ms (#5382) - #5384
Conversation
…5382) The Session handle held its own send-bandwidth consumer, so the sampler never idled, and each open PROBE stream ticked every 100ms reading the transport stats. Together an idle viewer cost its server ~40 driver polls and ~20 path-stats reads per second. The handle now keeps a weak reference, so only held consumers are demand. PROBE serving follows the supervisor's estimate instead of a timer: the supervisor samples on driver polls that already happen (at most once per 100ms) while a PROBE stream is open, and ProbeServe wakes on a change or its age deadline. Reporting rules (thresholds, decay, max age) are unchanged. An idle viewer now costs one max-age refresh per 10s. Ported by hand onto release's tokio-clock runtime and test harness. Co-Authored-By: Claude Opus 5.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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 0a3ec82.
No actionable defects found in the full backport diff and relevant surrounding code. The direction is sound: the session handle no longer creates sampling demand, and PROBE uses estimate changes plus age deadlines. I checked the release-specific Deadline/Clock behavior, Lite versus IETF Session::new wiring, and consumer drop/recreation semantics.
Verification limits: GitHub-only static review; I did not run tests or benchmarks. Passive sampling still relies on driver activity or the max-age deadline, so this does not establish prompt reporting of transport-only changes.
(Written by OpenAI)
![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: 0a3ec82b49
ℹ️ 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".
| .expect("server handshake failed"); | ||
| tokio::spawn(run(driver)); | ||
| session | ||
| (session, spawn_counted(driver)) |
There was a problem hiding this comment.
Because rs/moq-net/benches/session.rs imports this harness and constructs every relay, publisher, and viewer connection through connect_mock, wrapping every server driver here adds an atomic increment to the timed delivery and join paths even though those benchmarks never read the counter. The overhead scales with session count and poll activity, contaminating the benchmark slopes used to evaluate this optimization; make counting opt-in for idle_session and retain the uninstrumented runner by default.
AGENTS.md reference: AGENTS.md:L52-L52
Useful? React with 👍 / 👎.
| #[tokio::test(start_paused = true)] | ||
| async fn idle_viewer_does_not_wake_the_server() { |
There was a problem hiding this comment.
This one-viewer regression test verifies wake and stats-read counts, but it does not measure the CPU cost or sweep the publisher/viewer session axes that motivated the change. The existing benches/session.rs cases exercise active delivery and joins rather than elapsed idle PROBE behavior, so a future per-session regression would not appear as a measured slope; add an idle benchmark across multiple publisher and viewer counts.
AGENTS.md reference: AGENTS.md:L35-L36
Useful? React with 👍 / 👎.
Summary
Backport of #5382 to
release: an idle viewer session no longer wakes its server every 100ms.Sessionhandle held its own send-bandwidth consumer, so the sampler never idled, and each open PROBE stream ticked every 100ms reading the transport stats. An idle viewer cost its server ~40 driver polls and ~20 path-stats reads per second.bandwidth::Weak, so only consumers a caller holds count as demand. PROBE serving follows the supervisor's(send rate, rtt)estimate instead of a timer: while a PROBE stream is open, the supervisor samples on driver polls that already happen (at most once per 100ms), andProbeServewakes on a change or its age deadline. Reporting rules (thresholds, decay, 10s max age) are unchanged.releaselacks main's types:runtime::Deadlineon the tokio clock, noHandlesstruct (the estimate is aSession::newargument), and tests ontokio::test(start_paused = true)instead ofmoq_net_sim. Also drops the now-unusedDeadline::at. Thesession_idlebench is not ported.tests/idle_session.rsfails without the fix onrelease(1200 stats reads over 60s; with it, at most 6).API and wire impact
None.
Session::send_bandwidth()mints a fresh consumer per call; PROBE carries the same messages under the same rules.Checks
0a3ec82b4againstorigin/release1c651615b:just check origin/releasepasses (5083 + 387 + 132 + 78 tests).🤖 Generated with Claude Code
(Written by Claude Opus 5.5)
Need help on this PR? Tag
@codesmith-botwith what you need.