Bound Modal dispatch calls with configured deadlines in the Cloud Run gateway - #1634
Bound Modal dispatch calls with configured deadlines in the Cloud Run gateway#1634hua7450 wants to merge 2 commits into
Conversation
The Cloud Run gateway never passed timeout_seconds into call_modal_worker_dispatch, so a Modal control-plane call that black-holed blocked its executor thread forever: future.cancel() cannot stop a running thread, and the semaphore permit is only released on completion. During the 2026-07-21 incident each hung dispatch permanently leaked one of the 32 executor slots until the pool saturated and every request 503'd without Modal being attempted. Wire the resolved request/probe/canary timeouts into the default call_modal_worker, probe_modal_worker, and probe_modal_canary paths so the Modal SDK call itself (spawn + get(timeout)) gives up and frees its thread instead of waiting indefinitely. Fixes #1633 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
anth-volk
left a comment
anth-volk
left a comment
There was a problem hiding this comment.
Requesting changes because the new timeout path still does not provide a hard bound on the executor thread, which is the core guarantee this incident fix needs to establish. I also left inline comments on the spawn() semantic change, production-shaped and route-level coverage gaps, and duplicated test infrastructure.
Please also update the PR description before re-review: it still describes synchronous spawn().get() and three tests, while the current head uses a process-local async runner and adds four tests. The CI checkbox is also stale now that all checks are green.
I ran the focused failover/dispatch tests locally (57 passed), formatting and git diff --check passed, and all current GitHub checks are green.
| except BaseException: | ||
| coroutine.close() | ||
| raise | ||
| return future.result() |
There was a problem hiding this comment.
Blocking: this synchronous wait is still unbounded. asyncio.timeout() is cooperative: it cancels the coroutine and waits for cancellation cleanup. Modal's .aio bridge similarly shields its inner work and waits for underlying cancellation. If the Modal synchronization loop or transport is wedged and cannot finish cancellation, this thread remains here forever and its gateway semaphore permit is never released—the original pool-exhaustion failure mode. Please give this future.result() boundary its own hard deadline (with bounded grace), cancel/abandon the task when that expires, and test it with cancellation-resistant work.
| timeout_seconds: float, | ||
| ) -> Any: | ||
| async with asyncio.timeout(timeout_seconds): | ||
| function_call = await function.spawn.aio(*args, **kwargs) |
There was a problem hiding this comment.
This changes normal gateway calls from remote() semantics to spawn()+get(). In the locked Modal 1.4.2 client, remote.aio() can use the function's input-plane route when Modal supplies one, whereas spawn.aio() always uses the control-plane FunctionMap path implicated in this incident; it also changes retry/result-retention semantics. The live handles do not advertise an input-plane URL today, so this is latent rather than an immediate production regression, but Modal can enable that metadata independently. Could we preserve the existing invocation behavior with await function.remote.aio(*args, **kwargs) inside the overall timeout instead?
More broadly, we should strongly prefer Modal's canonical, documented invocation patterns here and avoid composing lower-level Modal primitives into a custom call lifecycle unless Modal explicitly recommends it. AI-generated implementations are particularly prone to combining individually supported APIs in ways that look reasonable but are noncanonical and subtly change transport, retry, cancellation, or result-lifecycle behavior. Please use the simplest canonical Modal operation that satisfies the requirement—ideally remote.aio() under the deadline—and verify any deviation against Modal's documentation or with Modal support.
| }, | ||
| timeout_seconds=0.01, | ||
| ), | ||
| timeout_seconds=0.5, |
There was a problem hiding this comment.
This test gives the inner call 10 ms but the outer executor 500 ms, while production passes the same resolved timeout to both layers. That extra 490 ms makes the semaphore-release assertion much easier than the real configuration. With equal deadlines, the outer wait usually wins the race and the permit is still held when the 503 is returned (in a local 100-run reproduction, 95 permits were not immediately reacquirable, although cooperative cancellation released them shortly afterward). Please test identical inner/outer deadlines, a cancellation-resistant coroutine, and a hang in get.aio() as well as spawn.aio().
| def _probe_canary_with_deadline() -> None: | ||
| probe_modal_canary(timeout_seconds=canary_timeout) | ||
|
|
||
| call_modal = modal_request or _call_modal_with_deadline |
There was a problem hiding this comment.
The new tests call call_modal_worker, probe_modal_worker, and probe_modal_canary directly with explicit timeouts, but the test _client() always injects all three Modal callables. As a result, no route-level test exercises these default wrappers or proves that the resolved request/probe/canary configuration reaches the SDK. Please add coverage using the default callables (with the Modal seam mocked) so a wiring or environment-resolution regression here fails a test.
| } | ||
|
|
||
|
|
||
| def _install_fake_modal_with_spawn( |
There was a problem hiding this comment.
This introduces a second Modal async-call fake even though this PR also extends tests/fixtures/modal_fakes.py with the same SyncAsyncCallable/spawn/get surface. Per the repository testing guidance, reusable mocks should live in tests/fixtures/ and test files should focus on behavior. Please extend the shared fake with configurable spawn/get hangs and reuse it here rather than maintaining two subtly different Modal models.
2 participants
Fixes #1633
Summary
During the 2026-07-21 production outage, Modal control-plane dispatches from the gateway black-holed: each hung call blocked its executor thread past the 90s request timeout,
future.cancel()could not stop the running thread, and the semaphore permit (released only on completion) never came back. 32 hung dispatches exhaustedMODAL_EXECUTOR_MAX_WORKERSand the gateway then 503'd every request instantly (modal_executor_saturated) without attempting Modal. See #1633 for the full incident analysis.This PR passes the gateway's already-resolved deadlines into the Modal SDK calls themselves, so a hung dispatch frees its thread when the deadline expires instead of leaking it until process restart:
call_modal_worker,probe_modal_worker, and_call_modal_worker_dispatchaccepttimeout_secondsand forward it tocall_modal_worker_dispatch, which already supports bounding viaspawn(payload).get(timeout=...)(previously the gateway never passed it, so the dispatch used unbounded.remote()).probe_modal_canaryacceptstimeout_secondsand usesspawn().get(timeout=...)when set.create_gateway_appbinds the resolvedrequest_timeout/probe_timeout/canary_timeoutinto the default callables. Injected fakes (modal_request=,modal_health_probe=,modal_canary_probe=) are unaffected.All new parameters are keyword-only with
Nonedefaults, preserving existing call sites and test fakes.Scope notes
_run_modal_operationwait, so client-facing 503 timing is unchanged; the fix is that the executor thread now also unblocks.spawn) can still block a thread; the circuit-breaker and connection-recycling follow-ups in Gateway leaks Modal dispatch threads on hung control-plane calls; circuit breaker never opened during 2026-07-21 outage #1633 cover that residual risk.libs/household-common/gateway.py(the Modal-hosted gateway) still dispatches without a deadline; out of scope here, noted in Gateway leaks Modal dispatch threads on hung control-plane calls; circuit breaker never opened during 2026-07-21 outage #1633.Test plan
spawn().get(timeout=...)fakes)make format-checkpasses (193 files)make testpasses: 714 passed, 1 skipped🤖 Generated with Claude Code