diff --git a/packages/control-plane/src/sandbox/client.ts b/packages/control-plane/src/sandbox/client.ts index 17ea932fd..0df3e1d70 100644 --- a/packages/control-plane/src/sandbox/client.ts +++ b/packages/control-plane/src/sandbox/client.ts @@ -240,6 +240,18 @@ export interface SnapshotSandboxResponse { error?: string; } +export interface TerminateSandboxRequest { + providerObjectId: string; + sessionId: string; + reason: string; + signal?: AbortSignal; +} + +export interface TerminateSandboxResponse { + success: boolean; + error?: string; +} + export interface SnapshotBuildSandboxRequest { buildId: string; providerSessionId: string; @@ -316,6 +328,7 @@ export class ModalApiError extends Error { export class ModalClient { private createSandboxUrl: string; private snapshotSandboxUrl: string; + private terminateSandboxUrl: string; private snapshotBuildSandboxUrl: string; private restoreSandboxUrl: string; private createImageBuildSandboxUrl: string; @@ -362,6 +375,7 @@ export class ModalClient { const baseUrl = getModalBaseUrl(workspace, environmentWebSuffix); this.createSandboxUrl = `${baseUrl}-api-create-sandbox.modal.run`; this.snapshotSandboxUrl = `${baseUrl}-api-snapshot-sandbox.modal.run`; + this.terminateSandboxUrl = `${baseUrl}-api-terminate-sandbox.modal.run`; this.snapshotBuildSandboxUrl = `${baseUrl}-api-snapshot-build-sandbox.modal.run`; this.restoreSandboxUrl = `${baseUrl}-api-restore-sandbox.modal.run`; this.createImageBuildSandboxUrl = `${baseUrl}-api-create-build-sandbox.modal.run`; @@ -535,7 +549,55 @@ export class ModalClient { } /** - * Trigger a filesystem snapshot for a sandbox object. + * Terminate a sandbox by its Modal object id. + */ + async terminateSandbox( + request: TerminateSandboxRequest, + correlation?: CorrelationContext + ): Promise { + const startTime = Date.now(); + const endpoint = "terminateSandbox"; + let httpStatus: number | undefined; + let outcome: "success" | "error" = "error"; + + try { + const result = await this.postJson( + this.terminateSandboxUrl, + endpoint, + MODAL_CLEANUP_REQUEST_DEADLINE_MS, + { + sandbox_id: request.providerObjectId, + session_id: request.sessionId, + reason: request.reason, + }, + imageBuildOperationModalResponseSchema, + correlation, + request.signal, + (status) => (httpStatus = status) + ); + if (!result.success) { + return { success: false, error: result.error || "Unknown terminate error" }; + } + + outcome = "success"; + return { success: true }; + } finally { + log.info("modal.request", { + event: "modal.request", + endpoint, + session_id: request.sessionId, + sandbox_id: request.providerObjectId, + trace_id: correlation?.trace_id, + request_id: correlation?.request_id, + http_status: httpStatus, + duration_ms: Date.now() - startTime, + outcome, + }); + } + } + + /** + * Take a filesystem snapshot of a running sandbox. */ async snapshotSandbox( request: SnapshotSandboxRequest, diff --git a/packages/control-plane/src/sandbox/lifecycle/manager.test.ts b/packages/control-plane/src/sandbox/lifecycle/manager.test.ts index 4ceebdcc5..bd3e844a6 100644 --- a/packages/control-plane/src/sandbox/lifecycle/manager.test.ts +++ b/packages/control-plane/src/sandbox/lifecycle/manager.test.ts @@ -1045,6 +1045,40 @@ describe("SandboxLifecycleManager", () => { expect(sandbox.last_spawn_error).toContain("temporarily disabled"); }); + it("schedules a retry alarm when the circuit breaker is open", async () => { + const now = Date.now(); + const failureAgeMs = 60_000; + const sandbox = createMockSandbox({ + status: "pending", + spawn_failure_count: 3, + last_spawn_failure: now - failureAgeMs, + }); + const storage = createMockStorage(createMockSession(), sandbox); + const alarmScheduler = createMockAlarmScheduler(); + const manager = new SandboxLifecycleManager( + createMockProvider(), + storage, + createMockBroadcaster(), + createMockWebSocketManager(false), + alarmScheduler, + createMockIdGenerator(), + createTestConfig() + ); + + await manager.spawnSandbox(); + + // Bot-triggered prompts have no next message to retry on: without a + // scheduled retry the open breaker strands them for good. + expect(alarmScheduler.alarms.length).toBeGreaterThan(0); + const last = alarmScheduler.alarms[alarmScheduler.alarms.length - 1]; + expect(last).toBeGreaterThan(now); + // The retry lands once the breaker window has fully passed the oldest + // counted failure. + expect(last).toBeGreaterThanOrEqual( + now + DEFAULT_LIFECYCLE_CONFIG.circuitBreaker.windowMs - failureAgeMs + ); + }); + it("still broadcasts the reason when persisting it throws", async () => { const now = Date.now(); const sandbox = createMockSandbox({ @@ -2261,6 +2295,59 @@ describe("SandboxLifecycleManager", () => { expect(provider.takeSnapshot).not.toHaveBeenCalled(); }); + it("counts a connecting timeout toward the circuit breaker", async () => { + const now = Date.now(); + const sandbox = createMockSandbox({ + status: "connecting" as SandboxStatus, + created_at: now - 130_000, + last_heartbeat: null, + }); + const storage = createMockStorage(createMockSession(), sandbox); + + const manager = new SandboxLifecycleManager( + createMockProvider(), + storage, + createMockBroadcaster(), + createMockWebSocketManager(), + createMockAlarmScheduler(), + createMockIdGenerator(), + createTestConfig() + ); + + await manager.handleAlarm(); + + // The queued prompt is re-driven after this failure, so a repository + // whose boot always exceeds the timeout must eventually stop respawning. + expect(storage.calls).toContain("incrementCircuitBreakerFailure"); + }); + + it("does not reset the circuit breaker until the sandbox connects", async () => { + const now = Date.now(); + const sandbox = createMockSandbox({ + status: "pending" as SandboxStatus, + created_at: now - 60_000, + }); + const storage = createMockStorage(createMockSession(), sandbox); + + const manager = new SandboxLifecycleManager( + createMockProvider(), + storage, + createMockBroadcaster(), + createMockWebSocketManager(false), + createMockAlarmScheduler(), + createMockIdGenerator(), + createTestConfig() + ); + + await manager.spawnSandbox(); + // Spawn initiation is not success: the sandbox has not connected yet, + // and connecting timeouts counted before it must stay counted. + expect(storage.calls).not.toContain("resetCircuitBreaker"); + + manager.onSandboxConnected(); + expect(storage.calls).toContain("resetCircuitBreaker"); + }); + it("does not timeout connecting sandbox within timeout window", async () => { const now = Date.now(); const sandbox = createMockSandbox({ diff --git a/packages/control-plane/src/sandbox/lifecycle/manager.ts b/packages/control-plane/src/sandbox/lifecycle/manager.ts index f697b2711..88086a11e 100644 --- a/packages/control-plane/src/sandbox/lifecycle/manager.ts +++ b/packages/control-plane/src/sandbox/lifecycle/manager.ts @@ -403,6 +403,9 @@ export class SandboxLifecycleManager implements SandboxLifecycle { failure_count: circuitBreakerState.failureCount, wait_time_ms: cbDecision.waitTimeMs || 0, }); + // Bot-triggered prompts have no "next message" to retry on; without a + // scheduled retry the open breaker strands them for good. + await this.alarmScheduler.schedule(now + (cbDecision.waitTimeMs || 0)); this.reportSandboxError( `Sandbox spawning temporarily disabled after ${circuitBreakerState.failureCount} failures. Try again in ${Math.ceil((cbDecision.waitTimeMs || 0) / 1000)} seconds.` ); @@ -642,9 +645,6 @@ export class SandboxLifecycleManager implements SandboxLifecycle { await this.finishProviderStartup(); - // Reset circuit breaker on successful spawn initiation - this.storage.resetCircuitBreaker(); - this.log.info("Sandbox spawn completed", { event: "sandbox.spawn", outcome: "success", @@ -1056,7 +1056,6 @@ export class SandboxLifecycleManager implements SandboxLifecycle { await this.storeAndBroadcastTunnelUrls(result.tunnelUrls); await this.finishProviderStartup(); - this.storage.resetCircuitBreaker(); } catch (error) { const errorMessage = error instanceof Error ? error.message : "Failed to resume sandbox"; this.storage.updateSandboxStatus("failed"); @@ -1300,6 +1299,10 @@ export class SandboxLifecycleManager implements SandboxLifecycle { }); this.storage.updateSandboxStatus("failed"); this.clearSandboxAccessState(); + // Count toward the circuit breaker: the queued prompt is re-driven + // after this failure, and a repository whose boot always exceeds the + // timeout would otherwise respawn in a loop. + this.storage.incrementCircuitBreakerFailure(now); if (this.canStopProviderSandbox()) { try { await this.stopProviderSandbox("connecting_timeout"); @@ -1311,7 +1314,7 @@ export class SandboxLifecycleManager implements SandboxLifecycle { } this.broadcaster.broadcast({ type: "sandbox_status", status: "failed" }); this.reportSandboxError( - "Sandbox failed to connect within the allowed time. It will be retried on your next message." + "Sandbox failed to connect within the allowed time. Retrying with a fresh sandbox." ); return "sandbox_failed"; } @@ -1666,12 +1669,16 @@ export class SandboxLifecycleManager implements SandboxLifecycle { /** * Notify the manager that a sandbox has connected. - * Resets the in-memory spawning flag and clears any stale spawn error. + * Resets the in-memory spawning flag, clears any stale spawn error, and + * closes the spawn circuit breaker: connection is the first point where a + * spawn actually succeeded, so failures counted before it (connecting + * timeouts) stay counted until a sandbox genuinely comes up. * * Called by SessionDO when sandbox WebSocket connects successfully. */ onSandboxConnected(): void { this.isSpawningSandbox = false; this.storage.setLastSpawnError(null, null); + this.storage.resetCircuitBreaker(); } } diff --git a/packages/control-plane/src/sandbox/providers/modal-provider.test.ts b/packages/control-plane/src/sandbox/providers/modal-provider.test.ts index 34b05524e..a059ec529 100644 --- a/packages/control-plane/src/sandbox/providers/modal-provider.test.ts +++ b/packages/control-plane/src/sandbox/providers/modal-provider.test.ts @@ -18,6 +18,8 @@ import type { SnapshotSandboxRequest, SnapshotBuildSandboxRequest, SnapshotSandboxResponse, + TerminateSandboxRequest, + TerminateSandboxResponse, CreateImageBuildSandboxRequest, CreateImageBuildSandboxResponse, StartImageBuildSandboxRequest, @@ -33,6 +35,7 @@ function createMockModalClient( createSandbox: (req: CreateSandboxRequest) => Promise; restoreSandbox: (req: RestoreSandboxRequest) => Promise; snapshotSandbox: (req: SnapshotSandboxRequest) => Promise; + terminateSandbox: (req: TerminateSandboxRequest) => Promise; snapshotBuildSandbox: (req: SnapshotBuildSandboxRequest) => Promise; createImageBuildSandbox: ( req: CreateImageBuildSandboxRequest @@ -63,6 +66,7 @@ function createMockModalClient( imageId: "image-123", }) ), + terminateSandbox: vi.fn(async (): Promise => ({ success: true })), snapshotBuildSandbox: vi.fn( async (): Promise => ({ success: true, @@ -108,6 +112,7 @@ describe("ModalSandboxProvider", () => { expect(provider.name).toBe("modal"); expect(provider.capabilities.supportsSnapshots).toBe(true); expect(provider.capabilities.supportsRestore).toBe(true); + expect(provider.capabilities.supportsExplicitStop).toBe(true); }); }); @@ -505,6 +510,62 @@ describe("ModalSandboxProvider", () => { }); }); + describe("stopSandbox", () => { + it("terminates the provider sandbox by object id", async () => { + const client = createMockModalClient(); + const provider = new ModalSandboxProvider(client); + + const result = await provider.stopSandbox({ + providerObjectId: "mo-1", + sessionId: "session-1", + reason: "connecting_timeout", + }); + + expect(result).toEqual({ success: true }); + expect(client.terminateSandbox).toHaveBeenCalledWith( + { + providerObjectId: "mo-1", + sessionId: "session-1", + reason: "connecting_timeout", + signal: undefined, + }, + undefined + ); + }); + + it("returns the failure message when terminate reports one", async () => { + const client = createMockModalClient({ + terminateSandbox: vi.fn(async () => ({ success: false, error: "sandbox lookup failed" })), + }); + const provider = new ModalSandboxProvider(client); + + const result = await provider.stopSandbox({ + providerObjectId: "mo-1", + sessionId: "session-1", + reason: "connecting_timeout", + }); + + expect(result).toEqual({ success: false, error: "sandbox lookup failed" }); + }); + + it("classifies HTTP 503 from terminate as transient", async () => { + const client = createMockModalClient({ + terminateSandbox: vi.fn(async () => { + throw new ModalApiError("stop failed", 503); + }), + }); + const provider = new ModalSandboxProvider(client); + + await expect( + provider.stopSandbox({ + providerObjectId: "mo-1", + sessionId: "session-1", + reason: "connecting_timeout", + }) + ).rejects.toMatchObject({ errorType: "transient" }); + }); + }); + describe("image builds", () => { it("binds a created image-build sandbox before starting it", async () => { const client = createMockModalClient(); diff --git a/packages/control-plane/src/sandbox/providers/modal-provider.ts b/packages/control-plane/src/sandbox/providers/modal-provider.ts index 32d2129b9..e831b221f 100644 --- a/packages/control-plane/src/sandbox/providers/modal-provider.ts +++ b/packages/control-plane/src/sandbox/providers/modal-provider.ts @@ -21,6 +21,8 @@ import { type RestoreResult, type SnapshotConfig, type SnapshotResult, + type StopConfig, + type StopResult, } from "../provider"; interface StartModalImageBuildConfig { @@ -90,7 +92,7 @@ export class ModalSandboxProvider implements SandboxProvider, ModalImageBuildPro supportsSnapshots: true, supportsRestore: true, supportsPersistentResume: false, - supportsExplicitStop: false, + supportsExplicitStop: true, }; constructor(private readonly client: ModalClient) {} @@ -241,6 +243,33 @@ export class ModalSandboxProvider implements SandboxProvider, ModalImageBuildPro } } + /** + * Stop a sandbox explicitly via Modal's terminate-by-id API. Terminal: Modal + * sandboxes cannot pause, so every stop reason kills the sandbox. + */ + async stopSandbox(config: StopConfig): Promise { + try { + const result = await this.client.terminateSandbox( + { + providerObjectId: config.providerObjectId, + sessionId: config.sessionId, + reason: config.reason, + signal: config.signal, + }, + config.correlation + ); + return result; + } catch (error) { + if (error instanceof ModalApiError) { + throw this.classifyErrorWithStatus(`Stop failed with HTTP ${error.status}`, error.status); + } + if (error instanceof SandboxProviderError) { + throw error; + } + throw this.classifyError("Failed to stop sandbox", error); + } + } + async snapshotImageBuildSandbox(config: SnapshotModalImageBuildConfig): Promise { try { const result = await this.client.snapshotBuildSandbox( diff --git a/packages/control-plane/src/session/alarm/handler.test.ts b/packages/control-plane/src/session/alarm/handler.test.ts index a0666baac..8f216f9fc 100644 --- a/packages/control-plane/src/session/alarm/handler.test.ts +++ b/packages/control-plane/src/session/alarm/handler.test.ts @@ -13,6 +13,7 @@ function createHandler() { failStuckProcessingMessage: vi.fn<() => Promise>().mockResolvedValue(), recoverStopConfirmationTimeout: vi.fn<() => Promise>().mockResolvedValue(), resumeAfterSandboxTermination: vi.fn<() => Promise>().mockResolvedValue(), + processMessageQueue: vi.fn<() => Promise>().mockResolvedValue(), }; const lifecycleManager = { handleAlarm: vi.fn<() => Promise>().mockResolvedValue("no_action"), @@ -120,6 +121,7 @@ describe("createAlarmHandler", () => { failStuckProcessingMessage: vi.fn<() => Promise>().mockResolvedValue(), recoverStopConfirmationTimeout: vi.fn<() => Promise>().mockResolvedValue(), resumeAfterSandboxTermination: vi.fn<() => Promise>().mockResolvedValue(), + processMessageQueue: vi.fn<() => Promise>().mockResolvedValue(), }; const handler = createAlarmHandler({ @@ -160,7 +162,7 @@ describe("createAlarmHandler", () => { expect(lifecycleManager.handleAlarm).toHaveBeenCalledTimes(1); }); - it("fails stuck work without resuming after a connecting timeout", async () => { + it("fails stuck work and resumes after a connecting timeout", async () => { const { handler, repository, messageQueue, lifecycleManager } = createHandler(); repository.getProcessingMessageWithStartedAt.mockReturnValue(null); lifecycleManager.handleAlarm.mockResolvedValue("sandbox_failed"); @@ -168,7 +170,9 @@ describe("createAlarmHandler", () => { await handler.handle(); expect(messageQueue.failStuckProcessingMessage).toHaveBeenCalledOnce(); - expect(messageQueue.resumeAfterSandboxTermination).not.toHaveBeenCalled(); + // Bot-triggered prompts never send the "next message" the old recovery + // waited for, so the alarm itself must re-drive the queued prompt. + expect(messageQueue.resumeAfterSandboxTermination).toHaveBeenCalledOnce(); }); it("fails stuck work and resumes after lifecycle termination", async () => { @@ -181,4 +185,29 @@ describe("createAlarmHandler", () => { expect(messageQueue.failStuckProcessingMessage).toHaveBeenCalledOnce(); expect(messageQueue.resumeAfterSandboxTermination).toHaveBeenCalledOnce(); }); + + it("re-drives the queue when the lifecycle alarm took no action", async () => { + const { handler, repository, messageQueue, lifecycleManager } = createHandler(); + repository.getProcessingMessageWithStartedAt.mockReturnValue(null); + lifecycleManager.handleAlarm.mockResolvedValue("no_action"); + + await handler.handle(); + + // The circuit-breaker retry alarm lands here: the sandbox row is dead and + // nothing else will move a pending bot prompt. + expect(messageQueue.processMessageQueue).toHaveBeenCalledOnce(); + // resumeAfterSandboxTermination would clear a live sandbox's stop marker. + expect(messageQueue.resumeAfterSandboxTermination).not.toHaveBeenCalled(); + }); + + it("does not re-drive the queue when the lifecycle alarm acted", async () => { + const { handler, repository, messageQueue, lifecycleManager } = createHandler(); + repository.getProcessingMessageWithStartedAt.mockReturnValue(null); + lifecycleManager.handleAlarm.mockResolvedValue("sandbox_terminated"); + + await handler.handle(); + + expect(messageQueue.processMessageQueue).not.toHaveBeenCalled(); + expect(messageQueue.resumeAfterSandboxTermination).toHaveBeenCalledOnce(); + }); }); diff --git a/packages/control-plane/src/session/alarm/handler.ts b/packages/control-plane/src/session/alarm/handler.ts index 345b51c23..d7ca79a4b 100644 --- a/packages/control-plane/src/session/alarm/handler.ts +++ b/packages/control-plane/src/session/alarm/handler.ts @@ -12,6 +12,7 @@ export interface AlarmHandlerDeps { | "failStuckProcessingMessage" | "recoverStopConfirmationTimeout" | "resumeAfterSandboxTermination" + | "processMessageQueue" >; lifecycleManager: Pick; alarmScheduler: AlarmScheduler; @@ -69,9 +70,20 @@ export function createAlarmHandler(deps: AlarmHandlerDeps): AlarmHandler { if (lifecycleResult !== "no_action") { await deps.messageQueue.failStuckProcessingMessage(); } - if (lifecycleResult === "sandbox_terminated") { + // A connecting timeout strands bot-triggered prompts: nothing re-drives + // the queue after the sandbox is failed, and automated sessions never + // send the "next message" the recovery assumed. Resume here re-drives + // the pending prompt through the same path an inbound message takes. + if (lifecycleResult === "sandbox_terminated" || lifecycleResult === "sandbox_failed") { await deps.messageQueue.resumeAfterSandboxTermination(); } + // no_action is the circuit-breaker retry alarm: the sandbox row is + // already dead and only the queue can move a pending prompt. Unlike + // resumeAfterSandboxTermination, processMessageQueue leaves any + // stop-confirmation wait intact — the sandbox may still be alive. + if (lifecycleResult === "no_action") { + await deps.messageQueue.processMessageQueue(); + } }, }; } diff --git a/packages/control-plane/test/integration/session-lifecycle-alarm-recovery.test.ts b/packages/control-plane/test/integration/session-lifecycle-alarm-recovery.test.ts index 185b042d3..63490cb81 100644 --- a/packages/control-plane/test/integration/session-lifecycle-alarm-recovery.test.ts +++ b/packages/control-plane/test/integration/session-lifecycle-alarm-recovery.test.ts @@ -6,6 +6,11 @@ import { cleanD1Tables } from "./cleanup"; import { initSession, queryDO, seedMessage, waitForSandboxStatus } from "./helpers"; const CONNECTING_TIMEOUT_BUFFER_MS = 1_000; +const RESPAWN_POLL_INTERVAL_MS = 100; +const RESPAWN_TIMEOUT_MS = 5_000; +// Long enough that a background spawn submitted by the alarm would have +// stamped a fresh sandbox row, without slowing the suite meaningfully. +const SPAWN_QUIET_WINDOW_MS = 750; /** * Park the session's sandbox past the connecting timeout, so the next alarm @@ -64,4 +69,70 @@ describe("SessionDO lifecycle alarm recovery", () => { expect(message?.status).toBe("failed"); expect(message?.error_message).toContain("stuck processing"); }); + + it("re-drives a pending prompt after a connecting timeout instead of stranding it", async () => { + const { stub } = await initSession({ userId: "user-1" }); + await parkSandboxPastConnectingTimeout(stub); + await seedMessage(stub, { + id: "msg-stranded", + authorId: await ownerParticipantId(stub), + content: "Review the PR", + // Bot-triggered prompts arrive once and never send a follow-up message. + source: "github", + status: "pending", + createdAt: Date.now() - 1000, + }); + + const [parked] = await queryDO<{ created_at: number }>(stub, "SELECT created_at FROM sandbox"); + if (!parked) throw new Error("Expected parked sandbox row"); + + await runInDurableObject(stub, (instance: SessionDO) => instance.alarm()); + + // The alarm re-drove the queue: the pending prompt found no sandbox and + // spawned a replacement, which stamps a fresh created_at even though + // Modal is unavailable here and the attempt settles failed. + const deadline = Date.now() + RESPAWN_TIMEOUT_MS; + let respawned = false; + while (Date.now() < deadline) { + const [row] = await queryDO<{ created_at: number }>(stub, "SELECT created_at FROM sandbox"); + if (row && row.created_at > parked.created_at) { + respawned = true; + break; + } + await new Promise((resolve) => setTimeout(resolve, RESPAWN_POLL_INTERVAL_MS)); + } + expect(respawned).toBe(true); + + // The queued prompt survived the sandbox failure and stays pending for + // the replacement sandbox instead of being silently dropped. It only + // reaches a terminal state once a sandbox connects and dispatches it. + const [message] = await queryDO<{ status: string }>( + stub, + "SELECT status FROM messages WHERE id = ?", + "msg-stranded" + ); + expect(message?.status).toBe("pending"); + }); + + it("does not respawn a sandbox when no prompt is queued", async () => { + const { stub } = await initSession({ userId: "user-1" }); + await parkSandboxPastConnectingTimeout(stub); + + const [parked] = await queryDO<{ created_at: number }>(stub, "SELECT created_at FROM sandbox"); + if (!parked) throw new Error("Expected parked sandbox row"); + + await runInDurableObject(stub, (instance: SessionDO) => instance.alarm()); + + // Control for the respawn above: the connecting-timeout alarm itself + // must not spawn — only a queued prompt re-driving the queue does. A + // fresh created_at here would mean some other path moved the respawn, + // and the queue-recovery test would stop proving anything about the queue. + await new Promise((resolve) => setTimeout(resolve, SPAWN_QUIET_WINDOW_MS)); + const [after] = await queryDO<{ created_at: number; status: string }>( + stub, + "SELECT created_at, status FROM sandbox" + ); + expect(after?.created_at).toBe(parked.created_at); + expect(after?.status).toBe("failed"); + }); }); diff --git a/packages/modal-infra/src/sandbox/manager.py b/packages/modal-infra/src/sandbox/manager.py index 14bf682a5..faafd2301 100644 --- a/packages/modal-infra/src/sandbox/manager.py +++ b/packages/modal-infra/src/sandbox/manager.py @@ -136,7 +136,7 @@ def get_logs(self) -> str: async def terminate(self) -> None: """Terminate the sandbox.""" - self.modal_sandbox.terminate() + await self.modal_sandbox.terminate.aio() @dataclass(frozen=True) @@ -605,7 +605,11 @@ async def get_sandbox_by_id(self, sandbox_id: str) -> SandboxHandle | None: sandbox_id: The Modal sandbox ID Returns: - SandboxHandle if found, None otherwise + SandboxHandle if found, None if the sandbox does not exist + + Raises: + modal.exception.Error: any lookup failure other than NotFoundError, + so callers do not mistake a provider error for absence. """ try: modal_sandbox = await modal.Sandbox.from_id.aio(sandbox_id) @@ -615,9 +619,11 @@ async def get_sandbox_by_id(self, sandbox_id: str) -> SandboxHandle | None: status=SandboxStatus.READY, # Assume ready if we can retrieve it created_at=time.time(), ) + except modal.exception.NotFoundError: + return None except Exception as e: log.warn("sandbox.lookup_error", sandbox_id=sandbox_id, exc=e) - return None + raise async def restore_from_snapshot( self, diff --git a/packages/modal-infra/src/web_api.py b/packages/modal-infra/src/web_api.py index e46ddc234..0f99b91ec 100644 --- a/packages/modal-infra/src/web_api.py +++ b/packages/modal-infra/src/web_api.py @@ -39,6 +39,7 @@ configure_logging() log = get_logger("web_api") IMAGE_BUILD_FINALIZATION_GRACE_SECONDS = 10 * 60 +DEFAULT_TERMINATION_REASON = "manual" class _ModalRequestModel(BaseModel): @@ -453,6 +454,97 @@ def api_health() -> dict: return {"success": True, "data": {"status": "healthy", "service": "open-inspect-modal"}} +@app.function(image=function_image, secrets=[internal_api_secret]) +@fastapi_endpoint(method="POST") +async def api_terminate_sandbox( + request: dict, + authorization: str | None = Header(None), + x_trace_id: str | None = Header(None), + x_request_id: str | None = Header(None), + x_session_id: str | None = Header(None), + x_sandbox_id: str | None = Header(None), +) -> dict: + """ + Terminate a sandbox by its Modal object id. + + Used by the control plane when a sandbox is failed without its bridge + connected (e.g. a connecting timeout): the provider sandbox keeps running + until its timeout, so an explicit terminate is the only way it dies. + + POST body: + { + "sandbox_id": "...", # Modal object id + "session_id": "...", + "reason": "connecting_timeout" | ... + } + + A sandbox that no longer exists is success: the caller's goal is that the + sandbox stop existing. + + Errors are reported in-band like every other endpoint in this module: the + response stays HTTP 200 and carries ``success: false``, which the + control-plane client parses from the body. The request log therefore + records the 200 that actually shipped, with ``outcome`` carrying the + error signal. + """ + start_time = time.time() + http_status = 200 + outcome = "success" + sandbox_id = request.get("sandbox_id") + + require_auth(authorization) + + if not sandbox_id: + raise HTTPException(status_code=400, detail="sandbox_id is required") + + try: + from .sandbox.manager import SandboxManager + + session_id = request.get("session_id") + reason = request.get("reason", DEFAULT_TERMINATION_REASON) + + manager = SandboxManager() + + handle = await manager.get_sandbox_by_id(sandbox_id) + if handle is not None: + await handle.terminate() + + return { + "success": True, + "data": { + "sandbox_id": sandbox_id, + "session_id": session_id, + "reason": reason, + "terminated": True, + }, + } + except HTTPException as e: + outcome = "error" + http_status = e.status_code + raise + except Exception as e: + # http_status stays 200: the response above is the in-band error + # envelope, and outcome is the error signal in the log. + outcome = "error" + log.error("api.error", exc=e, endpoint_name="api_terminate_sandbox") + return {"success": False, "error": str(e)} + finally: + duration_ms = int((time.time() - start_time) * 1000) + log.info( + "modal.http_request", + http_method="POST", + http_path="/api_terminate_sandbox", + http_status=http_status, + duration_ms=duration_ms, + outcome=outcome, + endpoint_name="api_terminate_sandbox", + trace_id=x_trace_id, + request_id=x_request_id, + session_id=x_session_id, + sandbox_id=x_sandbox_id or sandbox_id, + ) + + @app.function(image=function_image, secrets=[internal_api_secret]) @fastapi_endpoint(method="POST") async def api_snapshot_sandbox( @@ -506,7 +598,7 @@ async def api_snapshot_sandbox( from .sandbox.manager import SandboxManager session_id = request.get("session_id") - reason = request.get("reason", "manual") + reason = request.get("reason", DEFAULT_TERMINATION_REASON) manager = SandboxManager() diff --git a/packages/modal-infra/tests/test_snapshot_timeout.py b/packages/modal-infra/tests/test_snapshot_timeout.py index 3a0a3cf07..a5626f9eb 100644 --- a/packages/modal-infra/tests/test_snapshot_timeout.py +++ b/packages/modal-infra/tests/test_snapshot_timeout.py @@ -3,6 +3,7 @@ from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock +import modal import pytest from sandbox_runtime.types import SandboxStatus @@ -50,3 +51,41 @@ async def test_get_sandbox_by_id_awaits_async_lookup(monkeypatch): assert handle.modal_sandbox is modal_sandbox from_id.assert_not_called() from_id.aio.assert_awaited_once_with("sandbox-1") + + +@pytest.mark.asyncio +async def test_get_sandbox_by_id_maps_not_found_to_none(monkeypatch): + from_id = _async_method() + from_id.aio = AsyncMock(side_effect=modal.exception.NotFoundError("sandbox gone")) + monkeypatch.setattr("src.sandbox.manager.modal.Sandbox.from_id", from_id) + + handle = await SandboxManager().get_sandbox_by_id("sandbox-gone") + + assert handle is None + + +@pytest.mark.asyncio +async def test_get_sandbox_by_id_raises_other_lookup_errors(monkeypatch): + from_id = _async_method() + from_id.aio = AsyncMock(side_effect=modal.exception.ServiceError("modal unavailable")) + monkeypatch.setattr("src.sandbox.manager.modal.Sandbox.from_id", from_id) + + with pytest.raises(modal.exception.ServiceError): + await SandboxManager().get_sandbox_by_id("sandbox-1") + + +@pytest.mark.asyncio +async def test_terminate_awaits_async_terminate(): + """Handle termination must run the provider terminate RPC.""" + terminate = _async_method() + handle = SandboxHandle( + sandbox_id="sandbox-1", + modal_sandbox=SimpleNamespace(terminate=terminate), + status=SandboxStatus.READY, + created_at=0, + ) + + await handle.terminate() + + terminate.assert_not_called() + terminate.aio.assert_awaited_once() diff --git a/packages/modal-infra/tests/test_web_api_terminate_sandbox.py b/packages/modal-infra/tests/test_web_api_terminate_sandbox.py new file mode 100644 index 000000000..08b13a7c3 --- /dev/null +++ b/packages/modal-infra/tests/test_web_api_terminate_sandbox.py @@ -0,0 +1,107 @@ +"""Tests for the by-id sandbox terminate endpoint.""" + +from types import SimpleNamespace +from unittest.mock import AsyncMock + +import pytest + +from src import web_api + + +async def _call_terminate(request: dict) -> dict: + return await web_api.api_terminate_sandbox.get_raw_f()( + request, + authorization="Bearer test", + x_trace_id=None, + x_request_id=None, + x_session_id=None, + x_sandbox_id=None, + ) + + +@pytest.mark.asyncio +async def test_terminate_terminates_the_sandbox_by_id(monkeypatch): + monkeypatch.setattr(web_api, "require_auth", lambda _authorization: None) + handle = SimpleNamespace(terminate=AsyncMock()) + manager = SimpleNamespace(get_sandbox_by_id=AsyncMock(return_value=handle)) + monkeypatch.setattr("src.sandbox.manager.SandboxManager", lambda: manager) + + result = await _call_terminate( + {"sandbox_id": "mo-1", "session_id": "session-1", "reason": "connecting_timeout"} + ) + + assert result == { + "success": True, + "data": { + "sandbox_id": "mo-1", + "session_id": "session-1", + "reason": "connecting_timeout", + "terminated": True, + }, + } + manager.get_sandbox_by_id.assert_awaited_once_with("mo-1") + handle.terminate.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_terminate_treats_missing_sandbox_as_success(monkeypatch): + monkeypatch.setattr(web_api, "require_auth", lambda _authorization: None) + manager = SimpleNamespace(get_sandbox_by_id=AsyncMock(return_value=None)) + monkeypatch.setattr("src.sandbox.manager.SandboxManager", lambda: manager) + + result = await _call_terminate({"sandbox_id": "mo-gone", "session_id": "session-1"}) + + assert result["success"] is True + assert result["data"]["terminated"] is True + + +@pytest.mark.asyncio +async def test_terminate_reports_lookup_errors_as_failure(monkeypatch): + monkeypatch.setattr(web_api, "require_auth", lambda _authorization: None) + manager = SimpleNamespace( + get_sandbox_by_id=AsyncMock(side_effect=RuntimeError("modal unavailable")) + ) + monkeypatch.setattr("src.sandbox.manager.SandboxManager", lambda: manager) + requests: list[tuple[str, str, dict]] = [] + monkeypatch.setattr( + web_api, + "log", + SimpleNamespace( + error=lambda *a, **k: requests.append(("error", a[0], k)), + info=lambda *a, **k: requests.append(("info", a[0], k)), + ), + ) + + result = await _call_terminate({"sandbox_id": "mo-1", "session_id": "session-1"}) + + assert result["success"] is False + assert "modal unavailable" in result["error"] + # In-band error contract: the response ships HTTP 200 and the request log + # records that 200 with outcome carrying the error signal. + http_logs = [kw for name, _, kw in requests if name == "info" and _ == "modal.http_request"] + assert http_logs and http_logs[0]["http_status"] == 200 + assert http_logs[0]["outcome"] == "error" + + +@pytest.mark.asyncio +async def test_terminate_requires_a_sandbox_id(monkeypatch): + monkeypatch.setattr(web_api, "require_auth", lambda _authorization: None) + + with pytest.raises(web_api.HTTPException) as exc: + await _call_terminate({"session_id": "session-1"}) + + assert exc.value.status_code == 400 + assert exc.value.detail == "sandbox_id is required" + + +@pytest.mark.asyncio +async def test_terminate_runs_after_authentication(monkeypatch): + def reject_auth(_authorization): + raise web_api.HTTPException(status_code=401, detail="Unauthorized") + + monkeypatch.setattr(web_api, "require_auth", reject_auth) + + with pytest.raises(web_api.HTTPException) as exc: + await _call_terminate({"sandbox_id": "mo-1"}) + + assert exc.value.status_code == 401