fix: keep Cloudflare worker on the existing Tokio runtime - #332
fix: keep Cloudflare worker on the existing Tokio runtime#332haasonsaas wants to merge 1 commit into
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| fn block_on<T>( | ||
| &self, | ||
| future: impl std::future::Future<Output = anyhow::Result<T>>, | ||
| ) -> anyhow::Result<T> { | ||
| match self { | ||
| Self::Current(handle) => handle.block_on(future), | ||
| Self::Owned(runtime) => runtime.block_on(future), | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Reusing the ambient handle can stall bridge calls if the host runtime is single-threaded
BridgeRuntime::Current stores the ambient Handle and later calls Handle::block_on from whichever thread invokes the synchronous provider methods (in production these are tokio::task::spawn_blocking threads, see crates/sandboxwich-worker/src/main.rs:3189 and :2831). On a multi-thread runtime this is the same pattern already used for the reporter (reporter_runtime.block_on(...) at crates/sandboxwich-worker/src/main.rs:3204) and is safe. However, Handle::block_on does not drive the IO/timer drivers of a current_thread runtime; if the provider is ever constructed inside a current-thread runtime (e.g. a plain #[tokio::test], or an embedder that uses #[tokio::main(flavor = "current_thread")]) and a bridge HTTP call is then issued from a blocking thread while the main thread is not itself inside Runtime::block_on, the request will never make progress and will hang until the reqwest timeout. The worker's own entrypoint is the default multi-thread #[tokio::main] (crates/sandboxwich-worker/src/main.rs:997), so this is not reachable today, but it is a new constraint the type imposes on future callers; the new regression test happens to use flavor = "current_thread" and performs no IO, so it does not exercise it.
Was this helpful? React with 👍 or 👎 to provide feedback.
| impl BridgeRuntime { | ||
| fn new() -> anyhow::Result<Self> { | ||
| match tokio::runtime::Handle::try_current() { | ||
| Ok(handle) => Ok(Self::Current(handle)), | ||
| Err(_) => Ok(Self::Owned(Arc::new( | ||
| tokio::runtime::Runtime::new() | ||
| .context("failed to create Cloudflare Bridge runtime")?, | ||
| ))), | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: Handle is captured at construction, tying the provider's usable lifetime to that runtime
BridgeRuntime::new resolves Handle::try_current() once, at provider construction. The provider is Clone and stored in an Arc for the process lifetime (crates/sandboxwich-worker/src/main.rs:1420, :2396), so any later use after the capturing runtime has shut down (or from a thread whose lifetime outlives it) would panic inside Handle::block_on rather than falling back to an owned runtime. An alternative that re-checks Handle::try_current() at each block_on call — the pattern already used in run_kubectl_command_with_stdin (crates/sandboxwich-worker/src/provider.rs:6190-6199) — would avoid both the captured-handle staleness and the divergence between two runtime-selection strategies in the same crate.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn new(config: CloudflareConfig) -> anyhow::Result<Self> { | ||
| let runtime = Arc::new( | ||
| tokio::runtime::Runtime::new().context("failed to create Cloudflare Bridge runtime")?, | ||
| ); | ||
| let runtime = BridgeRuntime::new()?; | ||
| let client = reqwest::Client::builder() | ||
| .timeout(config.request_timeout) | ||
| .build() |
There was a problem hiding this comment.
📝 Info: Side effect: reqwest client resources are now registered on the same runtime that drives them
Previously, when the provider was built inside async main, the reqwest::Client was created in the ambient runtime's context but every request was driven by the nested owned runtime; connection pool timers/IO registered on one runtime and polled by another is a known source of stalled requests and "IO driver has terminated" errors. Unifying on one runtime removes that latent mismatch in addition to fixing the drop panic — worth noting because it also changes where bridge HTTP work is scheduled (shared worker runtime rather than a dedicated one), so bridge latency now competes with the worker's other async work.
(Refers to lines 162-173)
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Closing as superseded by #333. #333 already merged the production fix for the nested Tokio runtime panic on the Cloudflare worker and is live. This branch conflicts with main and uses a captured-handle approach that has the current-thread/stale-runtime concerns called out in review. Not needed for the current managed-home / runner-host promotion path. |
Pull request was closed
1 participant
Summary
Incident evidence
The production worker exited 101 at
tokio-1.53.1/src/runtime/blocking/shutdown.rs:51because provider capability registration constructed and dropped a nested runtime from asyncmain.Verification
cargo test -p sandboxwich-worker cloudflare_provider_drop_is_async_runtime_safe -- --nocapture(1 passed)cargo test --workspace(667 passed, 0 failed)rustup run 1.95.0 cargo clippy --workspace --all-targets --all-features -- -D warnings(passed)cargo fmt --all -- --check(passed)git diff --check(passed)