Repository navigation
fix(relay): shrink the relay's startup and serving stack frames - #7866
Conversation
Quest-Claim: 0f5de5637fe8ab44
Debug builds copy every future into its parent's poll frame several times, so `stack::debug_relay_load_and_run_fit_windows_stack` sat within about 128 KiB of its 1 MiB thread under the workspace's unified features. - `Cluster` keeps its dial template and shared dial client behind `Arc`s, so the handle every session, future, and `Relay` holds is no longer several KiB. - `Relay::run_inner` boxes the cluster and web serving futures it selects over, and `Web::serve` boxes its HTTP and HTTPS servers. The test now needs 416 KiB instead of 896 KiB. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 25 minutes. View limit details
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. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Cluster dial state moved behind
Arc—connect,quic, and the shareddialclient inrs/moq-relay/src/cluster.rsnow live behindArc, so cloning aClusterper accepted session is a refcount bump instead of a multi-KiB deep copy. - Templates still clone before mutating —
peer_clientusesas_deref().cloned()andrun_remote_sessionusesmoq_tokio::Client::clone(shared), so per-dial/per-session state keeps the old independence despite the sharedArc. - Large
select!branches boxed —serve_shared,started.run(), andweb.serve(web_routes)areBox::pinned inrun_inner, andWeb::serveboxes its HTTP and HTTPS server futures. - Quest completed — deletes
quest/m0/nightly/relay-stack.mdand its## Requiredbullet; no dangling references remain.
The Arc sharing is read-only for the templates and every mutation path clones first, so no cross-session state can leak. Boxing preserves cancellation and drop ordering, and adds one allocation each per relay run. cargo check -p moq-relay --lib passes locally. The remaining uncertainty (ARM64 Nightly not yet exercising the test behind the pre-existing coroutines failure) is already stated in the PR body.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head 0d48a57 against base 68c72cc. No actionable correctness findings; shrinking the shared dial state and boxing the larger serving futures is a reasonable direction.
The configuration is cloned before per-peer changes, fixed-port sessions still clone the underlying client before applying session settings, and the boxed branches retain the existing select and cancellation behavior. The 1 MiB stack regression remains unchanged.
Verification: GitHub-only static review of the implementation and existing tests. I did not build, run tests or independently measure the stack, including on ARM64 or Windows. GitHub's Check was still running when inspected.
(Written by review (OpenAI))

Problem
stack::debug_relay_load_and_run_fit_windows_stackrunsRelay::loadandRelay::runon a 1 MiB thread, standing in for a Windows main thread. It overflowed in Nightly's--all-featuresjob on 2026-11-01. moq-dev/moq#7595 then boxed the relay's resolved configuration, and no Nightly has reached this test since; each has stopped earlier atcoroutines. On today'smain, under the workspace's unified features (addsmoq-tokio/webrtc,ring, andsim), the test needs 896 KiB on x86_64. That leaves about 128 KiB of headroom on the 1 MiB thread. Nightly runs on ARM64, and moq-dev/moq#7539 measured the same thin margin (896 KiB overflows, 960 KiB passes).GDB on the overflow shows the cost is debug poll frames, not the futures themselves.
Relay::run_inner's frame was 428 KB andRelay::run's 111 KB, because a debug build lays out each future several times in its parent's frame. Two things multiplied that:run_inner'stokio::select!builds a 72 KB tuple of its branch futures, plus a temporary for each branch.Clusterwas 4.9 KB inline: itsconnectdial template (1.4 KB) and shared dialClient(2.5 KB) sat in every clone.run_inner's frame alone held 11 copies (asClusterand asStarted), andRelay(9.2 KB) carried it into every embedder frame.Approach
Clusterkeepsconnect,quic, anddialbehindArcs. A clone is now a refcount bump instead of a deep copy of the dial template's strings and vectors.run_innerboxes the large futures it selects over (Started::run,Web::serve, the shared accept loop), andWeb::serveboxes its HTTP and HTTPS servers. That is one allocation each per relay run.Smallest passing stack for the test, x86_64 debug,
-p moq-relay --all-features --features moq-tokio/webrtc,moq-tokio/ring,moq-tokio/sim(16 KiB steps):mainselect!branchesArcthe cluster's dial stateArconly, no boxingThe deepest path is now
Relay::load(with rcgen signing under it), not the serving loop.Impact
Cluster::with_connectkeeps its signature.Cluster(once per accepted session) no longer deep-copies the dial template.Alternatives
select!branches. The four small ones (STUN, internal, drain, jemalloc) are under 2.3 KB each, and boxing them did not move the threshold (592 KiB either way, before theArcchange).Validation
just check: 737 Rust tests passed, plus the repository checks.just ci origin/main: passed (737 Rust tests plus the repository checks).cargo clippy -p moq-relay --all-targets --all-features -- -D warnings: clean.-p moq-relay --all-featuresplus the workspace-unifiedmoq-tokiofeatures. I measured the thresholds above by temporarily making the stack size configurable (not committed).origin/mainat 3b7b8fc552:quest checkandquest check --base origin/mainpass, and the stack test passes again under the same feature set.coroutineswill exercise it.Follow-ups
just rs coroutinesstep fails onmain(moq-net compiles coroutines without its async feature: async fn body of model::group::Producer::ready(), run 41656613559). It stops the Features job before the all-features nextest that runs this test, and no quest I found owns it. Suggest a Nightly child for it unless poll-first already covers it.Review
Codex, GPT-6.1 Sol (
gpt-6.1-sol), medium effort, round 1, reviewed 0d48a57878 againstorigin/main. No findings.3b7b8fc552 merges
origin/mainwithout conflicts (the quest merge driver resolved the Nightly## Requiredlist). Its Rust diff againstorigin/mainmatches the reviewed head's byte for byte, and the only change is the Nightly README's link list picking up main's new children, so the merge skips a new review round.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code