-
Notifications
You must be signed in to change notification settings - Fork 653
Reclaim Modal sandboxes whose create call was interrupted #2406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -155,7 +155,32 @@ async def start(self) -> None: | |
| except ( | ||
| Exception | ||
| ) as e: # provisioning failure is one rollout's problem, not the eval's | ||
| await self._adopt_orphan() | ||
| raise SandboxError(f"modal sandbox provisioning failed: {e}") from e | ||
| except BaseException: # cancellation, which is not an Exception | ||
| await self._adopt_orphan() | ||
| raise | ||
|
|
||
| async def _adopt_orphan(self) -> None: | ||
| """Reclaim a sandbox whose `create` did not live long enough to hand back a handle. | ||
|
|
||
| Modal commits the sandbox and schedules it before `Sandbox.create` responds, so a | ||
| create that does not return — Ctrl-C, a cancelled rollout, a connection dropped on | ||
| the reply — still boots and bills a sandbox, about a second after the caller stopped | ||
| waiting for it. `_sandbox` was never assigned, so neither `teardown` nor the atexit | ||
| backstop can see it and it runs to its 24h maximum lifetime. The name is ours and | ||
| unique per rollout, so the sandbox is still addressable: claim it here and the | ||
| owner's `stop` disposes of it like any other. Best effort — this runs while `start` | ||
| is already unwinding, usually from a cancellation, hence the shield. | ||
| """ | ||
| if self._sandbox is not None: | ||
| return | ||
| import modal | ||
|
|
||
| with contextlib.suppress(Exception): | ||
| self._sandbox = await asyncio.shield( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] Keep the recovered handle across repeated cancellation A bare |
||
| modal.Sandbox.from_name.aio(_APP_NAME, self.name) | ||
| ) | ||
|
|
||
| async def expose(self, port: int) -> str | None: | ||
| # Publish a server hosted IN the sandbox: Modal forwards `port` (named via | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Don't adopt a pre-existing sandbox after a duplicate-name error
Modal documents that
Sandbox.createraisesAlreadyExistsErrorwhen the requested name already exists. This broadexcept Exceptionthen callsfrom_name, attaches to that already-running sandbox, and the owner'sfinally/abort path callsstop(), whose Modal teardown terminates the adopted handle. Becausemake_runtimeandprovision_runtimeaccept caller-supplied names, a duplicate-name create can therefore terminate a sandbox this runtime did not create. Please keep ownership of the in-flight create directly—for example, perform the create andself._sandboxassignment inside a coroutine passed to the existingrun_shieldedhelper—instead of recovering by name after every exception.