fix(worker): stop nested Tokio runtime crash on Cloudflare worker - #333
Conversation
HttpCloudflareBridge always built a private Runtime. When the worker constructed a temporary Cloudflare provider during async `run` registration (capability report), that nested Runtime was dropped while still on the outer #[tokio::main] stack and panicked: Cannot drop a runtime in a context where blocking is not allowed That crash-looped sandboxwich-cloudflare-worker in production after the command-capability registration path landed. When already inside a Tokio runtime, reuse the current handle via block_in_place instead of owning a nested Runtime. Keep an owned Runtime only for sync/test callers outside async. Regression test: construct+drop provider inside multi-thread runtime.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| &self, | ||
| future: impl std::future::Future<Output = anyhow::Result<T>>, | ||
| ) -> anyhow::Result<T> { | ||
| self.runtime.block_on(future) | ||
| if let Some(runtime) = &self.owned_runtime { | ||
| return runtime.block_on(future); | ||
| } | ||
| // Already inside the worker's multi-thread runtime: drive the future on | ||
| // the current handle. `block_in_place` keeps the outer runtime responsive | ||
| // while the sync SandboxProvider trait blocks on bridge I/O. | ||
| tokio::task::block_in_place(|| tokio::runtime::Handle::current().block_on(future)) |
There was a problem hiding this comment.
📝 Info: Runtime strategy is decided at construction time, not at call time
owned_runtime is decided once in HttpCloudflareBridge::new from Handle::try_current(), but the decision is consumed later in block_on on possibly different threads. Two consequences worth noting:
- If a bridge is built inside a Tokio context and any later bridge call happens on a thread with no Tokio context (plain
std::thread, a rayon pool, aDroprunning after runtime shutdown),tokio::runtime::Handle::current()inside the closure panics, whereas the old owned-runtime design worked from any thread. I audited the worker and found no such path today (crates/sandboxwich-worker/src/main.rsuses onlytokio::task::spawn_blocking, which keeps a handle set, and nostd::thread::spawnaround provider calls), so I did not flag it as a bug — but the contract of this shared provider type has changed for future callers. - Deciding at call time (
Handle::try_current()insideblock_on, falling back to a lazily-created owned runtime) would be robust against both directions and would remove the construction-context coupling.
I also verified the production hot path: provider methods run under tokio::task::spawn_blocking (crates/sandboxwich-worker/src/main.rs:3189, :2831), where block_in_place sees NotEntered/no scheduler context and simply runs the closure, and Handle::block_on from a blocking-pool thread is legal — so no panic there.
(Refers to lines 144-172)
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Already inside the worker's multi-thread runtime: drive the future on | ||
| // the current handle. `block_in_place` keeps the outer runtime responsive | ||
| // while the sync SandboxProvider trait blocks on bridge I/O. | ||
| tokio::task::block_in_place(|| tokio::runtime::Handle::current().block_on(future)) |
There was a problem hiding this comment.
🔍 block_in_place path is fatal under a current-thread runtime
tokio::task::block_in_place panics with "can call blocking only when running on the multi-threaded runtime" when invoked from inside a current-thread runtime's block_on. The worker binary uses default #[tokio::main] (multi-thread, crates/sandboxwich-worker/src/main.rs:997), so production is safe today, but the failure mode is silent for future callers: a #[tokio::test] (current-thread by default, per the AGENTS.md preference for #[tokio::test] on async-adjacent tests) or any current-thread-flavored entrypoint that constructs CloudflareSandboxProvider and then performs a bridge call would panic instead of doing I/O. Existing tests avoid this only because they use FakeBridge (for_test/for_test_with_replay_ledger) or plain #[test]. Consider falling back to an owned runtime when RuntimeFlavor::CurrentThread is detected, or documenting the multi-thread requirement on CloudflareSandboxProvider::new.
Was this helpful? React with 👍 or 👎 to provide feedback.
| #[test] | ||
| fn cloudflare_provider_construct_and_drop_inside_async_runtime_does_not_panic() { | ||
| let runtime = tokio::runtime::Builder::new_multi_thread() | ||
| .enable_all() | ||
| .worker_threads(2) | ||
| .build() | ||
| .expect("test runtime"); | ||
| runtime.block_on(async { |
There was a problem hiding this comment.
📝 Info: Regression test uses a hand-built runtime rather than #[tokio::test]
AGENTS.md asks to prefer #[tokio::test] for new async-adjacent tests. This one uses #[test] plus a manually built multi-thread runtime, which is defensible because the test needs to own and explicitly drop the runtime after the nested work to assert the no-nested-runtime property. #[tokio::test(flavor = "multi_thread")] would cover the provider construct/drop-inside-async part but not the explicit outer-runtime drop assertion, so the deviation looks intentional; noting it so the reviewer can confirm.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Production
sandboxwich-cloudflare-workercrash-loops immediately on start with:Root cause
HttpCloudflareBridgealways created a privatetokio::runtime::Runtime. During asyncrun, registration builds a temporary Cloudflare provider for the capability report (registration_provider_capability_report). That nested Runtime is dropped while still on the outer#[tokio::main]stack → panic (exit 101).Introduced when capability registration started constructing the provider at startup (command ledger path).
Fix
Runtime; drive bridge I/O withblock_in_place+ currentHandle.Test
cargo test -p sandboxwich-worker cloudflare_Deploy follow-up
After merge + containers publish, promote the new
sandboxwich-workerdigest into deploy for sandboxwich-cloudflare-worker (and cohort) so production recovers.