refactor!: kill direct tokio sync coupling in hot paths - #2394
Merged
Conversation
Bevy-prep PR 1: rig's hot paths no longer touch tokio primitives, so a
tokio runtime is not required for streaming pause/resume, tool-server
registration, or the copilot/chatgpt auth caches.
- streaming: PauseControl drops its tokio::sync::watch channel for one
Arc<PauseState { AtomicBool, futures::task::AtomicWaker }>. poll_next
integrates directly via the register-then-recheck AtomicWaker protocol,
deleting the boxed resume_wait future; both #2258 H7 invariants (no
busy re-wake, no lost resume race) are preserved and pinned by the
existing tests. PauseControl is now Clone.
- tool server: ToolServerHandle's tokio RwLock becomes std::sync::RwLock
(no guard ever crossed an await; all eight sites are clone-under-lock
or sync mutations). Poisoning is recovered via PoisonError::into_inner
in one pair of private accessors. Registration-only methods de-async:
add_tool, add_dynamic_tool, add_portable_dynamic_tool, append_toolset,
remove_tool (MIGRATING.md entry included). rmcp.rs and discord_bot.rs
keep tokio locks: the rmcp feature pulls tokio via the SDK regardless.
- auth caches: the copilot/chatgpt Mutex<()> becomes
async_lock::Mutex<PlatformAuthenticator> — the lock now wraps the
state it serializes (token/key caches) instead of guarding code, and
async-lock works on both native and wasm halves.
- device-flow sleeps: tokio::time::sleep only compiled via feature
unification (rig-core builds tokio without "time"); both call sites
now use a new wasm_compat::sleep built on futures_timer, next to the
existing timeout helper.
Residual non-test tokio in rig-core is the openai realtime websocket
(feature-gated, moves out in the transport-crate split) and test-only
code. Verification: cargo check/clippy --workspace --all-features
--all-targets clean; rig-core (1799) and rig-agent (598) tests pass;
wasm32-unknown-unknown check (CI's command) passes.
…nc-lock The futures crate is already a dependency and its async mutex is a drop-in for this slow, low-contention path (token refresh), so the PR now adds zero new direct dependencies.
This was referenced Aug 21, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bevy-prep PR 1: rig's hot paths no longer touch tokio primitives, so a tokio runtime is not required for streaming pause/resume, tool-server registration, or the copilot/chatgpt auth caches. Bevy ships its own task pools (
bevy_tasks) and Bevy users won't have a tokio runtime; tokio'ssyncprimitives were runtime-agnostic in practice, but they kept a direct tokio coupling in paths that don't need it. Adds zero new direct dependencies: every replacement primitive comes fromfutures, which was already a dependency.Changes
Streaming pause/resume (
rig-core/src/streaming/mod.rs).PauseControldrops itstokio::sync::watchchannel for a single sharedArc<PauseState { paused: AtomicBool, waker: futures::task::AtomicWaker }>, and is nowClone. The stream is the only consumer (poll_nexttakesPin<&mut Self>), which is exactly the single-waiter shapeAtomicWakeris built for — a watch channel is multi-waiter machinery this call site never needed.poll_nextintegrates directly via the register-then-recheckAtomicWakerprotocol, which deletes the boxedresume_waitfuture and itsResumeWaittype aliases entirely. Both #2258 invariants are preserved and remain pinned by the existing tests: no lost wakeup (resumeclears the flag before waking; poll registers its waker before re-reading the flag) and no busy re-wake while paused (H7).Tool server (
rig-agent/src/tool/server.rs).ToolServerHandle'stokio::sync::RwLockbecomesstd::sync::RwLock: all eight lock sites are clone-under-lock or sync mutations inside sync closures — no guard ever crossed an await. Poisoning is recovered viaPoisonError::into_innerin one pair of private accessors (the short sync critical sections can't leave the registry logically torn). The registration-only methods de-async —add_tool,add_dynamic_tool,add_portable_dynamic_tool,append_toolset,remove_tool— breaking change, MIGRATING.md entry included; execution/snapshot paths stay async. The deadlock-regression test that wrappedadd_toolintokio::time::timeoutnow calls it directly: with a sync method, a lock held across tool execution would hang the test harness, which is the same pin.Auth caches (
providers/copilot/auth,providers/chatgpt/auth). TheArc<tokio::sync::Mutex<()>>unit locks (held across awaits — network calls and device-flow polling — so a std mutex is not an option) becomeArc<futures::lock::Mutex<platform::PlatformAuthenticator>>: the lock now wraps the token/key cache state it actually serializes instead of guarding code, andfutures::lockworks on both the native and wasm halves. (futures::lock::Mutexoverasync_lock::Mutex: equivalent for this slow, low-contention refresh path, and it avoids a new direct dependency.)Device-flow sleeps.
tokio::time::sleepin the copilot/chatgpt device flows only compiled via feature unification — rig-core builds tokio without thetimefeature. Both sites now use a newwasm_compat::sleephelper built onfutures_timer::Delay, next to the existingwasm_compat::timeout.Deliberately out of scope
rig-core'stokiodependency line stays: reqwest pulls tokio transitively regardless, and the feature-gated openai realtime websocket still needs it. Removing it is the tail end of a planned transport-crate split.tool/rmcp.rsandintegrations/discord_bot.rskeep their tokio locks — thermcpfeature pulls tokio via the rmcp SDK anyway (rmcp.rshas a realtokio::spawn).Verification
cargo check --workspace --all-features --all-targetsandcargo clippy(same flags): clean.cargo test -p rig-core --all-features(1799 passed) and-p rig-agent --all-features(598 passed), zero failures; the Canonical stream grammar: mandatory identity, one accumulator, decode-then-validate, and a wire-conformance corpus #2258 pause/resume tests and the tool-server concurrency tests pass unchanged.cargo check -p rig-core --all-features --target wasm32-unknown-unknown: clean.tokio::sync/tokio::timegrep over rig-core/rig-agent: only test code,rmcp-gated code, and the websocket module.