Skip to content

fix(relay): shrink the relay's startup and serving stack frames - #7866

Merged
kixelated merged 3 commits into
mainfrom
quest/m0/nightly/relay-stack
Nov 4, 2026
Merged

kixelated merged 3 commits into
mainfrom
quest/m0/nightly/relay-stack

Conversation

@kixelated

@kixelated kixelated commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

stack::debug_relay_load_and_run_fit_windows_stack runs Relay::load and Relay::run on a 1 MiB thread, standing in for a Windows main thread. It overflowed in Nightly's --all-features job 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 at coroutines. On today's main, under the workspace's unified features (adds moq-tokio/webrtc, ring, and sim), 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 and Relay::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's tokio::select! builds a 72 KB tuple of its branch futures, plus a temporary for each branch.
  • Cluster was 4.9 KB inline: its connect dial template (1.4 KB) and shared dial Client (2.5 KB) sat in every clone. run_inner's frame alone held 11 copies (as Cluster and as Started), and Relay (9.2 KB) carried it into every embedder frame.

Approach

  • Cluster keeps connect, quic, and dial behind Arcs. A clone is now a refcount bump instead of a deep copy of the dial template's strings and vectors.
  • run_inner boxes the large futures it selects over (Started::run, Web::serve, the shared accept loop), and Web::serve boxes its HTTP and HTTPS servers. That is one allocation each per relay run.
  • The test keeps its 1 MiB stack, unchanged.

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):

Change Min stack
main 896 KiB
+ box the select! branches 592 KiB
+ Arc the cluster's dial state 416 KiB
Arc only, no boxing 448 KiB

The deepest path is now Relay::load (with rcgen signing under it), not the serving loop.

Impact

  • Public API: none. Cluster::with_connect keeps its signature.
  • Wire: none.
  • Cloning a Cluster (once per accepted session) no longer deep-copies the dial template.

Alternatives

  • Raise the test's stack: ruled out by the quest. It would hide the growth an embedder's main thread pays for.
  • Box all nine 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 the Arc change).
  • Hand-place each future on the heap with no stack temporary. Not possible on stable Rust, and the remaining frames are small next to the embedder's own.

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.
  • The unchanged 1 MiB test passes under -p moq-relay --all-features plus the workspace-unified moq-tokio features. I measured the thresholds above by temporarily making the stack size configurable (not committed).
  • After merging origin/main at 3b7b8fc552: quest check and quest check --base origin/main pass, and the stack test passes again under the same feature set.
  • Not run: an ARM64 build. The next Nightly to get past coroutines will exercise it.

Follow-ups

  • Nightly's just rs coroutines step fails on main (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 against origin/main. No findings.

3b7b8fc552 merges origin/main without conflicts (the quest merge driver resolved the Nightly ## Required list). Its Rust diff against origin/main matches 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

kixelated and others added 2 commits November 4, 2026 06:26
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>
@kixelated kixelated changed the title The relay's startup fits its stack under all features fix(relay): shrink the relay's startup and serving stack frames Nov 4, 2026
@kixelated
kixelated marked this pull request as ready for review November 4, 2026 14:50
@coderabbitai

coderabbitai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4e2f2d2c-4c95-4a17-802f-534c7057b7fd
Commits

Reviewing files that changed from the base of the PR and between 735cc0c and 3b7b8fc.

Files selected for processing: 5
  • quest/m0/nightly/README.md
  • quest/m0/nightly/relay-stack.md
  • rs/moq-relay/src/cluster.rs
  • rs/moq-relay/src/relay.rs
  • rs/moq-relay/src/web.rs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Cluster dial state moved behind Arc — connect, quic, and the shared dial client in rs/moq-relay/src/cluster.rs now live behind Arc, so cloning a Cluster per accepted session is a refcount bump instead of a multi-KiB deep copy.
  • Templates still clone before mutating — peer_client uses as_deref().cloned() and run_remote_session uses moq_tokio::Client::clone(shared), so per-dial/per-session state keeps the old independence despite the shared Arc.
  • Large select! branches boxed — serve_shared, started.run(), and web.serve(web_routes) are Box::pinned in run_inner, and Web::serve boxes its HTTP and HTTPS server futures.
  • Quest completed — deletes quest/m0/nightly/relay-stack.md and its ## Required bullet; 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.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

@kixelated
kixelated added this pull request to the merge queue Nov 4, 2026
Merged via the queue into main with commit 0a00923 Nov 4, 2026
9 of 10 checks passed
@kixelated
kixelated deleted the quest/m0/nightly/relay-stack branch November 4, 2026 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant