Skip to content

feat(tokio)!: own the worker split group - #3673

Merged
kixelated merged 5 commits into
devfrom
quest/m1/api-worker-ownership
Sep 17, 2026
Merged

kixelated merged 5 commits into
devfrom
quest/m1/api-worker-ownership

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Problem: Workers::split hands out owned Servers while Spawners only borrow the group, so dropping one Server or letting one accept loop return resizes the reuseport array and misroutes live sessions on healthy workers. Relay callers get this right by convention; embedders get no guarantee (#2964).

Approach: split(self) now consumes the bound Workers and returns an owning worker::Group. The group retains every member socket (Server::retain endpoint clones) until serving stops, owns every worker thread and join, and ends serving when the first Spawner::serve future completes, panics, or is cancelled (shared shutdown watched by every worker loop, pre-poll aborts included). Spawner::run is preserved for auxiliary tasks that must not end the group. Group keeps explicit shutdown plus Drop (never self-joins), local_addr/certificates/len, members (once-only), and finished (owner termination signal). Relay and worker/relay tests migrate to serve; new Linux worker regressions cover dropped-handle socket retention plus completion/cancel/panic/explicit-shutdown/drop-owner teardown.

Public API (breaking, targets dev): Workers::split(&mut self) -> Vec<(Server, Spawner)> becomes Workers::split(self) -> worker::Group; new worker::Group::{members, finished, shutdown, local_addr, certificates, len}; new Spawner::serve(server, FnOnce(Server) -> Future) with group termination; Spawner::run unchanged in shape (auxiliary only). Wire: no format change.

Alternatives: keep the documented contract and rely on callers; hide servers entirely behind a group serve callback (rejected: callback parameter, and the once-only builder already covers it); wait for map-based reuseport selection.

(Written by Muse Spark)

@kixelated
kixelated force-pushed the quest/m1/api-worker-ownership branch from 87c7896 to fe6ce19 Compare September 14, 2026 00:45
@kixelated
kixelated marked this pull request as ready for review September 14, 2026 23:19
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-17T11:59:33.545610Z edd9dd7 New commits
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fe6ce19b4c

ℹ️ 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".

Comment thread rs/moq-tokio/src/worker/group.rs Outdated
Comment thread rs/moq-tokio/src/worker/group.rs
kixelated and others added 5 commits September 17, 2026 04:29
Co-authored-by: muse-spark-1.3-contributor <noreply@opencode.ai>
Co-authored-by: muse-spark-1.3-contributor <noreply@opencode.ai>
Co-authored-by: muse-spark-1.3-contributor <noreply@opencode.ai>
Subscribe to the shared Notify before reading the shutdown flag so a
cancel cannot drop its wakeup. Drop a self-join handle instead of
forgetting it so the thread detaches.

Co-Authored-By: grok-4.6 <noreply@x.ai>
@kixelated
kixelated force-pushed the quest/m1/api-worker-ownership branch from fe6ce19 to edd9dd7 Compare September 17, 2026 11:54

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: edd9dd7158

ℹ️ 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".

Comment thread rs/moq-relay/src/relay.rs
@kixelated
kixelated enabled auto-merge (squash) September 17, 2026 12:11

Copy link
Copy Markdown
Collaborator Author

Rebased onto origin/dev and enabled squash auto-merge.

  • Quest list conflicts: dropped the completed worker-ownership quest, kept origin/dev's remaining gates. Relay embedding is already gone on dev (feat(relay)!: own listeners and workers for embedders #3638), so runtime_workers stays on relay.run().
  • Codex P1: stopped() subscribes to Notify before loading the shutdown flag.
  • Codex P2 (detach): drop the self-join JoinHandle instead of mem::forget.
  • Codex P2 (relay error race): declined. Sibling teardown panics the serve handle the same way a real serving panic does, so the relay cannot skip it without changing JoinHandle<F::Output>. The group still stops; a follow-up can surface teardown as JoinError::cancelled.

(written by grok-4.6)

@kixelated
kixelated merged commit 9058145 into dev Sep 17, 2026
3 checks passed
@kixelated
kixelated deleted the quest/m1/api-worker-ownership branch September 17, 2026 12:13
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