diff --git a/apps/api/src/handlers/tasks/__tests__/task-stop.test.ts b/apps/api/src/handlers/tasks/__tests__/task-stop.test.ts index 47777c863..34e9d2fac 100644 --- a/apps/api/src/handlers/tasks/__tests__/task-stop.test.ts +++ b/apps/api/src/handlers/tasks/__tests__/task-stop.test.ts @@ -14,6 +14,7 @@ const { mockTxReturning, mockMarkTaskStartParallelCountEndedAt, mockCancelTaskRunDirect, + mockDestroyCanceledTaskRunSandbox, } = vi.hoisted(() => { const mockTxReturning = vi.fn(); const mockTxUpdateWhere = vi.fn(() => ({ returning: mockTxReturning })); @@ -36,11 +37,13 @@ const { mockTxReturning, mockMarkTaskStartParallelCountEndedAt: vi.fn(), mockCancelTaskRunDirect: vi.fn(), + mockDestroyCanceledTaskRunSandbox: vi.fn(() => Promise.resolve('skipped')), }; }); vi.mock('@roomote/sdk/server', () => ({ withSandboxServerRpcClient: mockWithSandboxServerRpcClient, + destroyCanceledTaskRunSandbox: mockDestroyCanceledTaskRunSandbox, })); vi.mock('@roomote/db/server', () => ({ @@ -249,5 +252,11 @@ describe('stopTaskRun', () => { // parallel-count close live in the shared cancelTaskRunDirect helper, // covered by its real-DB tests in packages/db. expect(mockCancelTaskRunDirect).toHaveBeenCalledWith({ runId: 7 }); + // A spawn racing this cancel may have stamped a machine already; the + // teardown helper decides whether anything actually needs destroying. + expect(mockDestroyCanceledTaskRunSandbox).toHaveBeenCalledWith({ + runId: 7, + logPrefix: 'stopTaskRun', + }); }); }); diff --git a/apps/api/src/handlers/tasks/cancelTask.ts b/apps/api/src/handlers/tasks/cancelTask.ts index 38b8dba6c..79cf67b72 100644 --- a/apps/api/src/handlers/tasks/cancelTask.ts +++ b/apps/api/src/handlers/tasks/cancelTask.ts @@ -15,6 +15,7 @@ import { isExitedRunStatus, } from '@roomote/types'; import { captureTaskSettled } from '@roomote/telemetry/server'; +import { destroyCanceledTaskRunSandbox } from '@roomote/sdk/server'; import type { Variables } from '../../types'; import type { McpAuth } from '../mcp/middleware'; @@ -88,6 +89,14 @@ export async function cancelTask( if (canceledRun) { void captureTaskSettled(canceledRun.id, 'canceled'); + + // This cancel writes the terminal state directly (it never reaches + // finishRun), so tear down any attached sandbox here or it keeps + // running against the provider's capacity until its TTL. + await destroyCanceledTaskRunSandbox({ + runId: canceledRun.id, + logPrefix: 'cancelTask', + }); } return c.json({ success: true }); diff --git a/apps/api/src/handlers/tasks/task-stop.ts b/apps/api/src/handlers/tasks/task-stop.ts index 5b5f1a669..50a614e7a 100644 --- a/apps/api/src/handlers/tasks/task-stop.ts +++ b/apps/api/src/handlers/tasks/task-stop.ts @@ -1,5 +1,8 @@ import { TRPCClientError } from '@trpc/client'; -import { withSandboxServerRpcClient } from '@roomote/sdk/server'; +import { + destroyCanceledTaskRunSandbox, + withSandboxServerRpcClient, +} from '@roomote/sdk/server'; import { and, cancelTaskRunDirect, @@ -73,6 +76,14 @@ async function cancelTaskRunBeforeSandbox(runId: number): Promise { const canceled = await cancelTaskRunDirect({ runId }); if (canceled) { void captureTaskSettled(runId, 'canceled'); + + // Usually a no-op (pre-sandbox runs have no machine), but a spawn racing + // this cancel may already have stamped machineId without a reachable + // sandbox server — destroy it rather than leaking it until provider TTL. + await destroyCanceledTaskRunSandbox({ + runId, + logPrefix: 'stopTaskRun', + }); } return canceled; } diff --git a/apps/bullmq/src/scheduled-jobs/__tests__/sleep-check.test.ts b/apps/bullmq/src/scheduled-jobs/__tests__/sleep-check.test.ts index 5e32b0f21..251944b1c 100644 --- a/apps/bullmq/src/scheduled-jobs/__tests__/sleep-check.test.ts +++ b/apps/bullmq/src/scheduled-jobs/__tests__/sleep-check.test.ts @@ -10,6 +10,9 @@ const { mockGetInstanceStatus, mockCreateSnapshot, mockFinishRun, + mockClaimMachineDestroy, + mockClaimRelease, + mockClaimFinish, mockRecordComputeProviderUsage, mockRecordMutation, mockCreateComputeProviderMutationEventRecorder, @@ -74,6 +77,9 @@ const { mockGetInstanceStatus: vi.fn() as AnyMock, mockCreateSnapshot: vi.fn() as AnyMock, mockFinishRun: vi.fn() as AnyMock, + mockClaimMachineDestroy: vi.fn() as AnyMock, + mockClaimRelease: vi.fn() as AnyMock, + mockClaimFinish: vi.fn() as AnyMock, mockRecordComputeProviderUsage: vi.fn() as AnyMock, mockRecordMutation: vi.fn() as AnyMock, mockCreateComputeProviderMutationEventRecorder: vi.fn() as AnyMock, @@ -114,6 +120,7 @@ vi.mock('@roomote/compute-providers', () => ({ vi.mock('@roomote/sdk/server', () => ({ createSnapshot: mockCreateSnapshot, finishRun: mockFinishRun, + claimMachineDestroy: mockClaimMachineDestroy, recordComputeProviderUsage: mockRecordComputeProviderUsage, })); @@ -342,6 +349,12 @@ describe('sleepCheckJob', () => { }); returningFn.mockResolvedValue([]); mockFinishRun.mockResolvedValue(undefined); + mockClaimMachineDestroy.mockImplementation(async () => ({ + outcome: 'claimed', + release: mockClaimRelease, + finish: mockClaimFinish, + })); + mockClaimRelease.mockResolvedValue(undefined); // No stop request persisted on the row unless a test opts in. mockDbQueryTaskRunsFindFirst.mockResolvedValue(undefined); }); @@ -851,6 +864,66 @@ describe('sleepCheckJob', () => { expect(captureBullMqMessageMock).not.toHaveBeenCalled(); }); + it('skips the provider delete when another destroyer holds the teardown claim', async () => { + const mockJob = { + id: 99, + machineId: 'sb-2', + payloadKind: TaskPayloadKind.GithubPrReview, + status: RunStatus.Running, + taskPhase: 'waiting_for_prompt', + vendor: 'modal', + snapshotId: null, + sleepRequestedAt: null, + snapshotRequestedAt: null, + }; + + mockJobQueries({ dueJobs: [mockJob] }); + mockGetInstanceStatus.mockResolvedValue({ + status: 'running', + timeoutRemainingMs: 5 * 60 * 60 * 1_000, + }); + returningFn.mockResolvedValue([{ id: 99 }]); + // A concurrent cancel finalization already owns the teardown. + mockClaimMachineDestroy.mockResolvedValue({ outcome: 'held' }); + + await sleepCheckJob(); + + expect(mockDestroyInstance).not.toHaveBeenCalled(); + expect(mockRecordComputeProviderUsage).not.toHaveBeenCalled(); + // The run still finalizes; only the duplicate provider delete is skipped. + expect(setFn).toHaveBeenCalledWith( + expect.objectContaining({ status: RunStatus.Completed }), + ); + }); + + it('releases the teardown claim when the provider delete fails', async () => { + const mockJob = { + id: 99, + machineId: 'sb-2', + payloadKind: TaskPayloadKind.GithubPrReview, + status: RunStatus.Running, + taskPhase: 'waiting_for_prompt', + vendor: 'modal', + snapshotId: null, + sleepRequestedAt: null, + snapshotRequestedAt: null, + }; + + mockJobQueries({ dueJobs: [mockJob] }); + mockGetInstanceStatus.mockResolvedValue({ + status: 'running', + timeoutRemainingMs: 5 * 60 * 60 * 1_000, + }); + returningFn.mockResolvedValue([{ id: 99 }]); + mockDestroyInstance.mockRejectedValue(new Error('provider down')); + + await sleepCheckJob().catch(() => {}); + + expect(mockClaimRelease).toHaveBeenCalled(); + expect(mockClaimFinish).not.toHaveBeenCalled(); + expect(mockRecordComputeProviderUsage).not.toHaveBeenCalled(); + }); + it('reports provider-timeout backstop shutdowns to Sentry', async () => { const mockJob = { id: 100, diff --git a/apps/bullmq/src/scheduled-jobs/sleep-check.ts b/apps/bullmq/src/scheduled-jobs/sleep-check.ts index b2486fa63..574dc472a 100644 --- a/apps/bullmq/src/scheduled-jobs/sleep-check.ts +++ b/apps/bullmq/src/scheduled-jobs/sleep-check.ts @@ -35,6 +35,7 @@ import { type ComputeProviderClient, } from '@roomote/compute-providers'; import { + claimMachineDestroy, createSnapshot, finishRun, refreshTaskTitleOnCompletion, @@ -1432,6 +1433,24 @@ async function destroyInstanceWithAudit( ); } + // Serialize with the cancel-finalization teardown path: both destroyers + // record their final usage row only after the provider call returns, so the + // redis claim is the only atomic arbiter for a live race on this machine. + // The lease renews until settled below, so a slow provider delete cannot + // outlive it. + const claim = await claimMachineDestroy({ + provider: job.vendor ?? 'docker', + machineId: job.machineId!, + owner: logPrefix, + }); + + if (claim.outcome === 'held') { + console.log( + `[${logPrefix}] Skipping destroyInstance for ${job.machineId}: another destroyer holds the teardown claim`, + ); + return; + } + const recordMutation = createComputeProviderMutationEventRecorder( db, { @@ -1453,6 +1472,10 @@ async function destroyInstanceWithAudit( try { const result = await client.destroyInstance({ instanceId: job.machineId! }); + // Success: stop renewing and let the claim expire naturally — the + // residual TTL keeps guarding against a duplicate delete. + claim.finish(); + await tryRecordComputeProviderUsage({ runId: job.id, lifecycleAction: 'destroy', @@ -1465,6 +1488,9 @@ async function destroyInstanceWithAudit( logPrefix, }); } catch (error) { + // Give the claim back (token-conditional, so a successor that took over + // after a lapsed lease is unaffected) so teardown can be retried. + await claim.release(); await recordMutation({ provider: job.vendor ?? 'docker', operation: 'destroy_instance', diff --git a/apps/controller/src/BaseController.ts b/apps/controller/src/BaseController.ts index 85b98bcb4..85ea15c94 100644 --- a/apps/controller/src/BaseController.ts +++ b/apps/controller/src/BaseController.ts @@ -35,7 +35,7 @@ import { sql, } from '@roomote/db/server'; import { dequeueTaskRun } from '@roomote/cloud-agents/server'; -import { finishRun } from '@roomote/sdk/server'; +import { destroyCanceledTaskRunSandbox, finishRun } from '@roomote/sdk/server'; import { getOrphanedTaskRun } from './orphaned-task-runs'; import { @@ -399,11 +399,52 @@ export abstract class BaseController { sandboxTimeoutMs, provider, ); + + // A cancel that lands mid-provision beats the machine stamp: the run row + // is already terminal, so no later finalize path will tear the fresh + // sandbox down. Re-check once the spawn settled and destroy if so. + await this.destroySandboxIfCanceledDuringSpawn(taskRun.id); } catch (error) { await this.handleSpawnTaskRunError(taskRun, error); } } + private async destroySandboxIfCanceledDuringSpawn( + runId: number, + ): Promise { + try { + const latestRun = await db.query.taskRuns.findFirst({ + where: eq(taskRuns.id, runId), + columns: { + status: true, + canceledAt: true, + }, + }); + + if ( + !latestRun || + (!latestRun.canceledAt && latestRun.status !== RunStatus.Canceled) + ) { + return; + } + + console.warn( + `[BaseController] Task run #${runId} was canceled during spawn; destroying its sandbox`, + ); + + await destroyCanceledTaskRunSandbox({ + runId, + logPrefix: 'BaseController', + }); + } catch (error) { + console.warn( + `[BaseController] Failed post-spawn cancel check for task run #${runId}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } + } + private spawnWorkerInBackground(taskRun: TaskRun): boolean { if (this.inFlightSpawns.has(taskRun.id)) { console.warn( diff --git a/apps/web/src/lib/task-run-errors.test.ts b/apps/web/src/lib/task-run-errors.test.ts index 7fb65356e..1dfbf981b 100644 --- a/apps/web/src/lib/task-run-errors.test.ts +++ b/apps/web/src/lib/task-run-errors.test.ts @@ -25,6 +25,22 @@ stderr -> fatal: 'origin/main' is not a commit and a branch 'main' cannot be cre ); }); + it('explains the compute broker sandbox capacity rejection', () => { + expect( + getTaskRunErrorDisplayMessage('The tenant sandbox limit was reached.'), + ).toBe( + "Roomote couldn't start a new sandbox because this deployment is already running its maximum number of concurrent sandboxes. Wait for an active task to finish, or stop one you no longer need, then try again.", + ); + }); + + it('explains the sandbox capacity rejection when only the broker code is present', () => { + expect( + getTaskRunErrorDisplayMessage( + 'Broker request POST /v1/sandboxes failed with HTTP 429 (sandbox_limit_reached)', + ), + ).toContain('maximum number of concurrent sandboxes'); + }); + it('explains missing Docker worker images instead of only showing docker run', () => { const error = `Failed to run docker run. diff --git a/apps/web/src/lib/task-run-errors.ts b/apps/web/src/lib/task-run-errors.ts index 763f38b18..046078cdb 100644 --- a/apps/web/src/lib/task-run-errors.ts +++ b/apps/web/src/lib/task-run-errors.ts @@ -32,6 +32,11 @@ const DOCKER_RELEASE_ARCHIVE_MISSING = /Docker worker release archive does not exist|Docker provider requires a local worker release archive/i; const DOCKER_FETCH_FAILED_IN_LOGS = /Job\s+<\s*unknown\s*>\s*failed:\s*fetch failed|❌[^\n]*failed:\s*fetch failed/i; +// Roomote Cloud compute broker capacity rejection (code sandbox_limit_reached). +const SANDBOX_LIMIT_REACHED = + /tenant sandbox limit was reached|sandbox_limit_reached/i; +const SANDBOX_LIMIT_REACHED_MESSAGE = + "Roomote couldn't start a new sandbox because this deployment is already running its maximum number of concurrent sandboxes. Wait for an active task to finish, or stop one you no longer need, then try again."; function parseOpenAiAdminErrorBody( body: string, @@ -222,6 +227,10 @@ export function getTaskRunErrorDisplayMessage( return workspacePreparationMessage; } + if (SANDBOX_LIMIT_REACHED.test(stripped)) { + return SANDBOX_LIMIT_REACHED_MESSAGE; + } + // The persisted category is authoritative; text inference covers runs // that failed before error codes existed. const dockerFriendly = diff --git a/apps/web/src/trpc/commands/task-runs/index.test.ts b/apps/web/src/trpc/commands/task-runs/index.test.ts index 115318b38..4d97bb307 100644 --- a/apps/web/src/trpc/commands/task-runs/index.test.ts +++ b/apps/web/src/trpc/commands/task-runs/index.test.ts @@ -24,6 +24,13 @@ vi.mock('@roomote/cloud-agents/server', () => ({ routeTask: vi.fn(), })); +// Short-circuit the sdk server import chain: the command module only needs +// the cancel-teardown helper from it, and loading the real package would pull +// schema tables into this file's hand-rolled @roomote/db/server mock. +vi.mock('@roomote/sdk/server', () => ({ + destroyCanceledTaskRunSandbox: vi.fn(() => Promise.resolve('skipped')), +})); + vi.mock('@roomote/db/server', () => ({ and: vi.fn((...conditions: unknown[]) => ({ type: 'and', conditions })), db: { diff --git a/apps/web/src/trpc/commands/task-runs/index.ts b/apps/web/src/trpc/commands/task-runs/index.ts index 8cf9062b2..a80d20614 100644 --- a/apps/web/src/trpc/commands/task-runs/index.ts +++ b/apps/web/src/trpc/commands/task-runs/index.ts @@ -31,6 +31,7 @@ import { tasks, } from '@roomote/db/server'; import { SlackNotifier } from '@roomote/slack'; +import { destroyCanceledTaskRunSandbox } from '@roomote/sdk/server'; import type { UserAuthSuccess } from '@/types'; import { Env, getArtifactById, getRepositories } from '@/lib/server'; @@ -494,6 +495,14 @@ export async function cancelTaskRunCommand( if (canceledRun) { void captureTaskSettled(canceledRun.id, 'canceled'); + + // This cancel writes the terminal state directly (it never reaches + // finishRun), so tear down any attached sandbox here or it keeps + // running against the provider's capacity until its TTL. + await destroyCanceledTaskRunSandbox({ + runId: canceledRun.id, + logPrefix: 'cancelTaskRunCommand', + }); } } diff --git a/packages/redis/src/index.ts b/packages/redis/src/index.ts index b0a0433a9..f931fb3e1 100644 --- a/packages/redis/src/index.ts +++ b/packages/redis/src/index.ts @@ -17,6 +17,13 @@ export const REDIS_KEYS = { * `${prefix}:${scope}:${channelId}`. */ SLACK_CHANNEL_INFO: 'slack:channel_info', + /** + * Teardown claim for one provider machine keyed as + * `${prefix}:${provider}:${machineId}`. Serializes concurrent destroyers + * (cancel finalization vs sleep-check) so only one issues the provider + * delete. + */ + MACHINE_DESTROY_CLAIM: 'compute:machine-destroy-claim', } as const; /** Positive-cache TTL for successfully fetched GitHub release notes. */ diff --git a/packages/sdk/src/server/index.ts b/packages/sdk/src/server/index.ts index 05373590e..9d4c5bc0d 100644 --- a/packages/sdk/src/server/index.ts +++ b/packages/sdk/src/server/index.ts @@ -13,6 +13,8 @@ export { } from './trpc'; export { finishRun } from './lib/task-runs/finish-run'; +export { destroyCanceledTaskRunSandbox } from './lib/task-runs/destroy-canceled-run-sandbox'; +export { claimMachineDestroy } from './lib/task-runs/machine-destroy-claim'; export { recordLlmUsage, type RecordLlmUsageInput, diff --git a/packages/sdk/src/server/lib/task-runs/__tests__/destroy-canceled-run-sandbox.test.ts b/packages/sdk/src/server/lib/task-runs/__tests__/destroy-canceled-run-sandbox.test.ts new file mode 100644 index 000000000..e59fca768 --- /dev/null +++ b/packages/sdk/src/server/lib/task-runs/__tests__/destroy-canceled-run-sandbox.test.ts @@ -0,0 +1,431 @@ +import { RunStatus } from '@roomote/types'; + +// ── Mocks ──────────────────────────────────────────────────────────────────── + +const mockFindFirstRun = vi.fn(); +const mockRecordMutation = vi.fn().mockResolvedValue(undefined); +const mockCreateComputeProviderMutationEventRecorder = vi + .fn() + .mockReturnValue((...args: unknown[]) => mockRecordMutation(...args)); +const mockResolveComputeProviderEnvValues = vi.fn(); + +/** Rows resolved by the final-usage-record lookup select chain. */ +let finalUsageRows: unknown[] = []; + +function makeSelectChain() { + const chain: Record = {}; + + for (const method of ['from', 'where', 'limit']) { + chain[method] = vi.fn().mockReturnValue(chain); + } + + chain.then = ( + onFulfilled: (value: unknown[]) => unknown, + onRejected?: (reason: unknown) => unknown, + ) => Promise.resolve(finalUsageRows).then(onFulfilled, onRejected); + + return chain; +} + +vi.mock('@roomote/db/server', async () => { + const actual = + await vi.importActual( + '@roomote/db/server', + ); + return { + ...actual, + db: { + query: { + taskRuns: { + findFirst: (...args: unknown[]) => mockFindFirstRun(...args), + }, + }, + select: () => makeSelectChain(), + }, + createComputeProviderMutationEventRecorder: (...args: unknown[]) => + mockCreateComputeProviderMutationEventRecorder(...args), + resolveComputeProviderEnvValues: (...args: unknown[]) => + mockResolveComputeProviderEnvValues(...args), + }; +}); + +const mockRedisSet = vi.fn(); +const mockRedisEval = vi.fn().mockResolvedValue(1); + +vi.mock('@roomote/redis', async () => { + const actual = + await vi.importActual('@roomote/redis'); + return { + ...actual, + getRedis: () => ({ + set: (...args: unknown[]) => mockRedisSet(...args), + eval: (...args: unknown[]) => mockRedisEval(...args), + }), + }; +}); + +const mockDestroyInstance = vi.fn(); +const mockCreateComputeProviderClient = vi.fn().mockReturnValue({ + destroyInstance: (...args: unknown[]) => mockDestroyInstance(...args), +}); + +vi.mock('@roomote/compute-providers', () => ({ + createComputeProviderClient: (...args: unknown[]) => + mockCreateComputeProviderClient(...args), +})); + +const mockRecordComputeProviderUsage = vi.fn().mockResolvedValue(undefined); + +vi.mock('../record-compute-provider-usage', () => ({ + recordComputeProviderUsage: (...args: unknown[]) => + mockRecordComputeProviderUsage(...args), +})); + +import { destroyCanceledTaskRunSandbox } from '../destroy-canceled-run-sandbox'; + +// ── Helpers ────────────────────────────────────────────────────────────────── + +function makeRun(overrides: Record = {}) { + return { + id: 42, + taskId: 'task-42', + status: RunStatus.Canceled, + machineId: 'sb-42', + vendor: 'roomote', + snapshotId: null, + canceledAt: new Date('2026-07-29T12:00:00.000Z'), + ...overrides, + }; +} + +// ── Tests ──────────────────────────────────────────────────────────────────── + +describe('destroyCanceledTaskRunSandbox', () => { + beforeEach(() => { + vi.clearAllMocks(); + finalUsageRows = []; + mockRedisSet.mockResolvedValue('OK'); + mockRedisEval.mockResolvedValue(1); + mockResolveComputeProviderEnvValues.mockResolvedValue({ + ROOMOTE_CLOUD_TOKEN_ID: 'tenant-1', + }); + mockDestroyInstance.mockResolvedValue({ + usageObservation: { + activeCpuDurationMs: 1_000, + networkTransfer: { ingress: 10, egress: 20 }, + }, + }); + mockCreateComputeProviderClient.mockReturnValue({ + destroyInstance: (...args: unknown[]) => mockDestroyInstance(...args), + }); + }); + + it('destroys the machine and records a destroy usage record for a canceled run', async () => { + mockFindFirstRun.mockResolvedValue(makeRun()); + + const result = await destroyCanceledTaskRunSandbox({ + runId: 42, + logPrefix: 'test', + }); + + expect(result).toBe('destroyed'); + expect(mockCreateComputeProviderClient).toHaveBeenCalledWith({ + provider: 'roomote', + envFallback: { ROOMOTE_CLOUD_TOKEN_ID: 'tenant-1' }, + }); + expect(mockDestroyInstance).toHaveBeenCalledWith({ instanceId: 'sb-42' }); + expect(mockRecordComputeProviderUsage).toHaveBeenCalledWith( + expect.objectContaining({ + runId: 42, + lifecycleAction: 'destroy', + completedAt: expect.any(Date), + activeCpuDurationMs: 1_000, + networkIngressBytes: 10, + networkEgressBytes: 20, + details: expect.objectContaining({ + provider: 'roomote', + reason: 'task_run_canceled', + }), + }), + ); + expect(mockRecordMutation).toHaveBeenCalledWith( + expect.objectContaining({ + operation: 'destroy_instance', + eventType: 'started', + instanceId: 'sb-42', + }), + ); + expect(mockRecordMutation).toHaveBeenCalledWith( + expect.objectContaining({ + operation: 'destroy_instance', + eventType: 'completed', + instanceId: 'sb-42', + }), + ); + }); + + it('builds a docker client without env fallback for docker runs', async () => { + mockFindFirstRun.mockResolvedValue(makeRun({ vendor: 'docker' })); + + await destroyCanceledTaskRunSandbox({ runId: 42, logPrefix: 'test' }); + + expect(mockResolveComputeProviderEnvValues).not.toHaveBeenCalled(); + expect(mockCreateComputeProviderClient).toHaveBeenCalledWith({ + provider: 'docker', + }); + }); + + it('treats a canceledAt stamp as canceled even before the status write lands', async () => { + mockFindFirstRun.mockResolvedValue(makeRun({ status: RunStatus.Running })); + + const result = await destroyCanceledTaskRunSandbox({ + runId: 42, + logPrefix: 'test', + }); + + expect(result).toBe('destroyed'); + expect(mockDestroyInstance).toHaveBeenCalled(); + }); + + it('skips runs without a machine', async () => { + mockFindFirstRun.mockResolvedValue(makeRun({ machineId: null })); + + const result = await destroyCanceledTaskRunSandbox({ + runId: 42, + logPrefix: 'test', + }); + + expect(result).toBe('skipped'); + expect(mockDestroyInstance).not.toHaveBeenCalled(); + expect(mockRecordComputeProviderUsage).not.toHaveBeenCalled(); + }); + + it('skips runs preserved via snapshot', async () => { + mockFindFirstRun.mockResolvedValue(makeRun({ snapshotId: 'snap-1' })); + + const result = await destroyCanceledTaskRunSandbox({ + runId: 42, + logPrefix: 'test', + }); + + expect(result).toBe('skipped'); + expect(mockDestroyInstance).not.toHaveBeenCalled(); + }); + + it('skips runs that are not canceled', async () => { + mockFindFirstRun.mockResolvedValue( + makeRun({ status: RunStatus.Running, canceledAt: null }), + ); + + const result = await destroyCanceledTaskRunSandbox({ + runId: 42, + logPrefix: 'test', + }); + + expect(result).toBe('skipped'); + expect(mockDestroyInstance).not.toHaveBeenCalled(); + }); + + it('skips missing runs', async () => { + mockFindFirstRun.mockResolvedValue(undefined); + + const result = await destroyCanceledTaskRunSandbox({ + runId: 42, + logPrefix: 'test', + }); + + expect(result).toBe('skipped'); + expect(mockDestroyInstance).not.toHaveBeenCalled(); + }); + + it('skips when a final usage record already exists (e.g. sleep-check destroyed first)', async () => { + mockFindFirstRun.mockResolvedValue(makeRun()); + finalUsageRows = [{ id: 'roomote:compute:roomote:42:sb-42' }]; + + const result = await destroyCanceledTaskRunSandbox({ + runId: 42, + logPrefix: 'test', + }); + + expect(result).toBe('skipped'); + expect(mockDestroyInstance).not.toHaveBeenCalled(); + expect(mockRecordComputeProviderUsage).not.toHaveBeenCalled(); + }); + + it('claims the machine with a unique token before the provider delete', async () => { + mockFindFirstRun.mockResolvedValue(makeRun()); + + await destroyCanceledTaskRunSandbox({ runId: 42, logPrefix: 'test' }); + + expect(mockRedisSet).toHaveBeenCalledWith( + 'compute:machine-destroy-claim:roomote:sb-42', + expect.stringMatching(/^test:/), + 'EX', + expect.any(Number), + 'NX', + ); + expect(mockRedisSet.mock.invocationCallOrder[0]).toBeLessThan( + mockDestroyInstance.mock.invocationCallOrder[0]!, + ); + }); + + it('lets exactly one of two concurrent callers issue the provider delete', async () => { + mockFindFirstRun.mockResolvedValue(makeRun()); + // First caller wins the SET NX; the second sees the claim held. + mockRedisSet.mockResolvedValueOnce('OK').mockResolvedValueOnce(null); + + const [first, second] = await Promise.all([ + destroyCanceledTaskRunSandbox({ runId: 42, logPrefix: 'finishRun' }), + destroyCanceledTaskRunSandbox({ runId: 42, logPrefix: 'cancelTask' }), + ]); + + expect([first, second].sort()).toEqual(['destroyed', 'skipped']); + expect(mockDestroyInstance).toHaveBeenCalledTimes(1); + expect(mockRecordComputeProviderUsage).toHaveBeenCalledTimes(1); + // The loser must not record a failed destroy mutation. + expect(mockRecordMutation).not.toHaveBeenCalledWith( + expect.objectContaining({ eventType: 'failed' }), + ); + }); + + it('skips when another destroyer already holds the claim', async () => { + mockFindFirstRun.mockResolvedValue(makeRun()); + mockRedisSet.mockResolvedValue(null); + + const result = await destroyCanceledTaskRunSandbox({ + runId: 42, + logPrefix: 'test', + }); + + expect(result).toBe('skipped'); + expect(mockDestroyInstance).not.toHaveBeenCalled(); + expect(mockRecordMutation).not.toHaveBeenCalled(); + }); + + it('conditionally releases its own token when the provider delete fails', async () => { + mockFindFirstRun.mockResolvedValue(makeRun()); + mockDestroyInstance.mockRejectedValue(new Error('provider down')); + + const result = await destroyCanceledTaskRunSandbox({ + runId: 42, + logPrefix: 'test', + }); + + expect(result).toBe('failed'); + // Token-conditional DEL: the release script compares the stored value to + // this caller's token so it can never delete a successor's claim. + const claimedToken = mockRedisSet.mock.calls[0]?.[1]; + expect(mockRedisEval).toHaveBeenCalledWith( + expect.stringContaining('del'), + 1, + 'compute:machine-destroy-claim:roomote:sb-42', + claimedToken, + ); + }); + + it('renews the lease while a slow provider delete is in flight', async () => { + vi.useFakeTimers(); + try { + mockFindFirstRun.mockResolvedValue(makeRun()); + + let resolveDestroy!: (value: unknown) => void; + mockDestroyInstance.mockReturnValue( + new Promise((resolve) => { + resolveDestroy = resolve; + }), + ); + + const pending = destroyCanceledTaskRunSandbox({ + runId: 42, + logPrefix: 'test', + }); + + // Two renewal intervals elapse while destroyInstance is still pending. + await vi.advanceTimersByTimeAsync(11 * 60 * 1_000); + + const claimedToken = mockRedisSet.mock.calls[0]?.[1]; + const renewCalls = mockRedisEval.mock.calls.filter(([script]) => + String(script).includes('expire'), + ); + expect(renewCalls.length).toBeGreaterThanOrEqual(2); + expect(renewCalls[0]).toEqual([ + expect.stringContaining('expire'), + 1, + 'compute:machine-destroy-claim:roomote:sb-42', + claimedToken, + expect.any(String), + ]); + + resolveDestroy({}); + await expect(pending).resolves.toBe('destroyed'); + + // Settling the claim stops renewal. + mockRedisEval.mockClear(); + await vi.advanceTimersByTimeAsync(30 * 60 * 1_000); + expect(mockRedisEval).not.toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); + + it('still destroys when redis is unavailable rather than leaking the machine', async () => { + mockFindFirstRun.mockResolvedValue(makeRun()); + mockRedisSet.mockRejectedValue(new Error('redis unavailable')); + + const result = await destroyCanceledTaskRunSandbox({ + runId: 42, + logPrefix: 'test', + }); + + expect(result).toBe('destroyed'); + expect(mockDestroyInstance).toHaveBeenCalledTimes(1); + }); + + it('records a failed mutation event and reports failure when destroyInstance throws', async () => { + mockFindFirstRun.mockResolvedValue(makeRun()); + mockDestroyInstance.mockRejectedValue(new Error('sandbox not found')); + + const result = await destroyCanceledTaskRunSandbox({ + runId: 42, + logPrefix: 'test', + }); + + expect(result).toBe('failed'); + expect(mockRecordComputeProviderUsage).not.toHaveBeenCalled(); + expect(mockRecordMutation).toHaveBeenCalledWith( + expect.objectContaining({ + operation: 'destroy_instance', + eventType: 'failed', + details: expect.objectContaining({ error: 'sandbox not found' }), + }), + ); + }); + + it('still reports destroyed when the usage record write fails', async () => { + mockFindFirstRun.mockResolvedValue(makeRun()); + mockRecordComputeProviderUsage.mockRejectedValueOnce( + new Error('usage write failed'), + ); + + const result = await destroyCanceledTaskRunSandbox({ + runId: 42, + logPrefix: 'test', + }); + + expect(result).toBe('destroyed'); + expect(mockRecordMutation).toHaveBeenCalledWith( + expect.objectContaining({ eventType: 'completed' }), + ); + }); + + it('never throws when the run lookup fails', async () => { + mockFindFirstRun.mockRejectedValue(new Error('db unavailable')); + + const result = await destroyCanceledTaskRunSandbox({ + runId: 42, + logPrefix: 'test', + }); + + expect(result).toBe('failed'); + }); +}); diff --git a/packages/sdk/src/server/lib/task-runs/__tests__/finish-run.test.ts b/packages/sdk/src/server/lib/task-runs/__tests__/finish-run.test.ts index 0a9680f55..0ab1cd10c 100644 --- a/packages/sdk/src/server/lib/task-runs/__tests__/finish-run.test.ts +++ b/packages/sdk/src/server/lib/task-runs/__tests__/finish-run.test.ts @@ -268,6 +268,13 @@ vi.mock('../notify-source-run-on-settle', () => ({ mockNotifySourceRunOnSettle(...args), })); +const mockDestroyCanceledTaskRunSandbox = vi.fn().mockResolvedValue('skipped'); + +vi.mock('../destroy-canceled-run-sandbox', () => ({ + destroyCanceledTaskRunSandbox: (...args: unknown[]) => + mockDestroyCanceledTaskRunSandbox(...args), +})); + import { finishRun } from '../finish-run'; import { createTaskRunGitHubToken } from '@roomote/github'; import { enqueueTask } from '@roomote/cloud-agents/server'; @@ -442,6 +449,53 @@ describe('finishRun', () => { }, ); + it('tears down the sandbox when a run finishes as canceled', async () => { + mockFindFirstRun.mockResolvedValue( + makeRun({ vendor: 'roomote', machineId: 'sb-42' }), + ); + + await finishRun({ id: 1, status: RunStatus.Canceled }); + + expect(mockDestroyCanceledTaskRunSandbox).toHaveBeenCalledWith({ + runId: 1, + logPrefix: 'finishRun', + }); + }); + + it('tears down the sandbox when a stop-requested failure is normalized to canceled', async () => { + mockFindFirstRun.mockResolvedValue( + makeRun({ + vendor: 'roomote', + machineId: 'sb-42', + cancelRequestedAt: new Date(), + }), + ); + + await finishRun({ + id: 1, + status: RunStatus.Failed, + error: 'sandbox died mid-cancel', + }); + + expect(mockDestroyCanceledTaskRunSandbox).toHaveBeenCalledWith({ + runId: 1, + logPrefix: 'finishRun', + }); + }); + + it.each([RunStatus.Completed, RunStatus.Failed, RunStatus.Idle] as const)( + 'does not tear down the sandbox when a run finishes as %s', + async (status) => { + mockFindFirstRun.mockResolvedValue( + makeRun({ vendor: 'roomote', machineId: 'sb-42' }), + ); + + await finishRun({ id: 1, status }); + + expect(mockDestroyCanceledTaskRunSandbox).not.toHaveBeenCalled(); + }, + ); + it('does not capture a settled event when a run becomes idle', async () => { mockFindFirstRun.mockResolvedValue(makeRun()); diff --git a/packages/sdk/src/server/lib/task-runs/destroy-canceled-run-sandbox.ts b/packages/sdk/src/server/lib/task-runs/destroy-canceled-run-sandbox.ts new file mode 100644 index 000000000..b3ccdb023 --- /dev/null +++ b/packages/sdk/src/server/lib/task-runs/destroy-canceled-run-sandbox.ts @@ -0,0 +1,218 @@ +import { + type ComputeProvider, + RunStatus, + computeProviderUsageFinalLifecycleActions, +} from '@roomote/types'; +import { + and, + computeProviderUsage, + createComputeProviderMutationEventRecorder, + db, + eq, + inArray, + resolveComputeProviderEnvValues, + taskRuns, +} from '@roomote/db/server'; +import { createComputeProviderClient } from '@roomote/compute-providers'; + +import { claimMachineDestroy } from './machine-destroy-claim'; +import { recordComputeProviderUsage } from './record-compute-provider-usage'; + +type DestroyCanceledTaskRunSandboxResult = 'destroyed' | 'skipped' | 'failed'; + +async function createCancelTeardownClient(provider: ComputeProvider) { + // Docker resolves purely from the local daemon; every managed provider + // falls back to the encrypted deployment env vars saved during setup. + return provider === 'docker' + ? createComputeProviderClient({ provider }) + : createComputeProviderClient({ + provider, + envFallback: await resolveComputeProviderEnvValues(provider), + }); +} + +/** + * Best-effort teardown of a canceled run's live sandbox. + * + * Canceled runs never re-enter the sleep/snapshot pipeline (sleep-check only + * sweeps active statuses), so a machine that is still attached when the run + * turns terminal would otherwise keep running — and keep counting against the + * provider's sandbox capacity — until the provider TTL reaps it. Every path + * that finalizes a run as canceled funnels through this helper. + * + * Skips when the run has no machine, was preserved via snapshot, is not + * actually canceled, or already has a final `compute_provider_usage` record + * (a prior destroy/snapshot — e.g. sleep-check destroyed the instance before + * calling finishRun). Never throws: cancel finalization must not fail because + * provider teardown did. + */ +export async function destroyCanceledTaskRunSandbox(params: { + runId: number; + /** Caller tag used for log lines and the recorded audit trail. */ + logPrefix: string; +}): Promise { + const { runId, logPrefix } = params; + + try { + const run = await db.query.taskRuns.findFirst({ + where: eq(taskRuns.id, runId), + columns: { + id: true, + taskId: true, + status: true, + machineId: true, + vendor: true, + snapshotId: true, + canceledAt: true, + }, + }); + + if ( + !run || + !run.machineId || + run.snapshotId != null || + (run.status !== RunStatus.Canceled && run.canceledAt == null) + ) { + return 'skipped'; + } + + // A final lifecycle record means the instance was already torn down (or + // preserved) by another writer — most commonly sleep-check destroying the + // machine right before it finalizes the run as canceled. + const [finalUsageRecord] = await db + .select({ id: computeProviderUsage.id }) + .from(computeProviderUsage) + .where( + and( + eq(computeProviderUsage.runId, runId), + inArray(computeProviderUsage.lifecycleAction, [ + ...computeProviderUsageFinalLifecycleActions, + ]), + ), + ) + .limit(1); + + if (finalUsageRecord) { + return 'skipped'; + } + + const provider: ComputeProvider = run.vendor ?? 'docker'; + + // Build the client (which can throw on a misconfigured provider) before + // taking the claim, so an early failure never leaves an armed lease. + const client = await createCancelTeardownClient(provider); + + const recordMutation = createComputeProviderMutationEventRecorder( + db, + { + runId: run.id, + taskId: run.taskId, + }, + { logPrefix, logger: console }, + ); + + // The usage-record check above is only a fast path; it cannot arbitrate a + // live race because every destroyer records usage after the provider call + // returns. The redis claim is the atomic gate: exactly one caller owns the + // provider delete for this machine, and the lease renews until settled. + const claim = await claimMachineDestroy({ + provider, + machineId: run.machineId, + owner: logPrefix, + }); + + if (claim.outcome === 'held') { + console.log( + `[${logPrefix}] Skipping sandbox teardown for canceled task run #${run.id}: another destroyer holds the claim for ${run.machineId}`, + ); + return 'skipped'; + } + + const details = { + phase: 'destroy_after_cancel', + reason: 'task_run_canceled', + trigger: logPrefix, + }; + + await recordMutation({ + provider, + operation: 'destroy_instance', + eventType: 'started', + instanceId: run.machineId, + message: `Calling destroyInstance for instance ${run.machineId} of canceled task run #${run.id}.`, + details, + }); + + let usageObservation; + + try { + ({ usageObservation } = await client.destroyInstance({ + instanceId: run.machineId, + })); + // Success: stop renewing and let the claim expire naturally — the + // residual TTL keeps guarding against a duplicate delete. + claim.finish(); + } catch (error) { + // Give the claim back (token-conditional, so a successor that took over + // after a lapsed lease is unaffected) so teardown can be retried. + await claim.release(); + await recordMutation({ + provider, + operation: 'destroy_instance', + eventType: 'failed', + instanceId: run.machineId, + message: `destroyInstance failed for instance ${run.machineId} of canceled task run #${run.id}.`, + details: { + ...details, + error: error instanceof Error ? error.message : String(error), + }, + }); + console.warn( + `[${logPrefix}] Failed to destroy sandbox ${run.machineId} for canceled task run #${run.id}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + return 'failed'; + } + + try { + await recordComputeProviderUsage({ + runId: run.id, + lifecycleAction: 'destroy', + completedAt: new Date(), + activeCpuDurationMs: usageObservation?.activeCpuDurationMs, + networkIngressBytes: usageObservation?.networkTransfer?.ingress, + networkEgressBytes: usageObservation?.networkTransfer?.egress, + details: { provider, ...details }, + }); + } catch (error) { + console.warn( + `[${logPrefix}] Failed to record compute provider usage for canceled task run #${run.id}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } + + await recordMutation({ + provider, + operation: 'destroy_instance', + eventType: 'completed', + instanceId: run.machineId, + message: `destroyInstance completed for instance ${run.machineId} of canceled task run #${run.id}.`, + details, + }); + + console.log( + `[${logPrefix}] Destroyed sandbox ${run.machineId} for canceled task run #${run.id}`, + ); + + return 'destroyed'; + } catch (error) { + console.warn( + `[${logPrefix}] Failed to tear down sandbox for canceled task run #${runId}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + return 'failed'; + } +} diff --git a/packages/sdk/src/server/lib/task-runs/finish-run.ts b/packages/sdk/src/server/lib/task-runs/finish-run.ts index 2fc03047b..fb8d95cb4 100644 --- a/packages/sdk/src/server/lib/task-runs/finish-run.ts +++ b/packages/sdk/src/server/lib/task-runs/finish-run.ts @@ -62,6 +62,7 @@ import { readConflictResolutionSummary, } from './conflict-resolution-comments'; import { cleanupSandboxOidcTargetsForTaskRun } from '../sandbox-oidc'; +import { destroyCanceledTaskRunSandbox } from './destroy-canceled-run-sandbox'; import { notifySourceRunOnSettle } from './notify-source-run-on-settle'; import { refreshTaskTitleOnCompletion } from './record-task-message-envelope'; import { getRedis } from '@roomote/redis'; @@ -450,6 +451,15 @@ export const finishRun = async ({ ); } } + + // Canceled runs never re-enter the sleep/snapshot pipeline, so an attached + // machine must be destroyed here or it runs until the provider TTL while + // counting against sandbox capacity. Last on purpose: a worker-driven cancel + // awaits this RPC from inside the sandbox being destroyed, so every other + // side effect completes first. Best-effort — never throws. + if (status === RunStatus.Canceled) { + await destroyCanceledTaskRunSandbox({ runId: id, logPrefix: 'finishRun' }); + } }; /** diff --git a/packages/sdk/src/server/lib/task-runs/index.ts b/packages/sdk/src/server/lib/task-runs/index.ts index 450c1e2ad..f3af25c24 100644 --- a/packages/sdk/src/server/lib/task-runs/index.ts +++ b/packages/sdk/src/server/lib/task-runs/index.ts @@ -5,7 +5,9 @@ export * from './update-runtime-state'; export * from './touch-worker-heartbeat'; export * from './dequeue-task-run'; export * from './dequeue-resume-task-run'; +export * from './destroy-canceled-run-sandbox'; export * from './finish-run'; +export * from './machine-destroy-claim'; export * from './enqueue-snapshot'; export * from './enqueue-sleep'; export * from './revert-pr-commit'; diff --git a/packages/sdk/src/server/lib/task-runs/machine-destroy-claim.ts b/packages/sdk/src/server/lib/task-runs/machine-destroy-claim.ts new file mode 100644 index 000000000..c54f54b07 --- /dev/null +++ b/packages/sdk/src/server/lib/task-runs/machine-destroy-claim.ts @@ -0,0 +1,138 @@ +import { randomUUID } from 'node:crypto'; + +import type { ComputeProvider } from '@roomote/types'; +import { getRedis, REDIS_KEYS } from '@roomote/redis'; + +/** + * Lease for one teardown attempt. Renewed while the provider call is in + * flight, so it only has to outlive a crashed claim holder, not a slow + * provider delete. + */ +const MACHINE_DESTROY_CLAIM_TTL_SECONDS = 15 * 60; + +/** Renew well inside the TTL so one missed tick cannot lose the lease. */ +const MACHINE_DESTROY_CLAIM_RENEW_INTERVAL_MS = 5 * 60 * 1_000; + +/** Delete the claim only when the caller's token still owns it. */ +const RELEASE_IF_OWNED_SCRIPT = `if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("del", KEYS[1]) else return 0 end`; + +/** Extend the lease only when the caller's token still owns it. */ +const RENEW_IF_OWNED_SCRIPT = `if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("expire", KEYS[1], ARGV[2]) else return 0 end`; + +type MachineDestroyClaim = + /** Another caller is already destroying this machine — do not delete. */ + | { outcome: 'held' } + | { + /** + * `claimed`: this caller owns the teardown and must issue the provider + * delete. `unavailable`: redis is unreachable — proceed unserialized + * rather than leak the machine (release/finish are no-ops). + */ + outcome: 'claimed' | 'unavailable'; + /** + * Call when the provider delete failed: stops lease renewal and deletes + * the key only if this caller's token still owns it, so a retry (or a + * concurrent caller that took over after lease expiry) is unaffected. + */ + release: () => Promise; + /** + * Call when the provider delete succeeded: stops lease renewal and + * leaves the key to expire (destroys are permanent, so the residual + * TTL keeps guarding against a duplicate delete from a lagging path). + */ + finish: () => void; + }; + +function buildMachineDestroyClaimKey( + provider: ComputeProvider, + machineId: string, +): string { + return `${REDIS_KEYS.MACHINE_DESTROY_CLAIM}:${provider}:${machineId}`; +} + +const NOOP_CLAIM_HANDLE = { + release: async () => {}, + finish: () => {}, +}; + +/** + * Atomically claim the teardown of one provider machine before calling + * destroyInstance. Cancel finalization and sleep-check can race on the same + * machine; the final `compute_provider_usage` record alone cannot arbitrate + * because both writers record it only after the provider call returns. + * + * Ownership is a unique token: release and renewal are conditional on the + * token so a caller whose lease lapsed can never delete a successor's claim, + * and the lease is renewed in the background until the caller settles it via + * `finish()` (success) or `release()` (failure). + */ +export async function claimMachineDestroy(params: { + provider: ComputeProvider; + machineId: string; + /** Caller tag embedded in the claim token for debugging. */ + owner: string; +}): Promise { + const key = buildMachineDestroyClaimKey(params.provider, params.machineId); + const token = `${params.owner}:${randomUUID()}`; + + try { + const claim = await getRedis().set( + key, + token, + 'EX', + MACHINE_DESTROY_CLAIM_TTL_SECONDS, + 'NX', + ); + + if (claim !== 'OK') { + return { outcome: 'held' }; + } + } catch (error) { + console.warn( + `[claimMachineDestroy] Redis unavailable while claiming ${params.provider} machine ${params.machineId}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + return { outcome: 'unavailable', ...NOOP_CLAIM_HANDLE }; + } + + const renewTimer = setInterval(() => { + getRedis() + .eval( + RENEW_IF_OWNED_SCRIPT, + 1, + key, + token, + String(MACHINE_DESTROY_CLAIM_TTL_SECONDS), + ) + .catch((error: unknown) => { + console.warn( + `[claimMachineDestroy] Failed to renew teardown lease for ${params.provider} machine ${params.machineId}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + }); + }, MACHINE_DESTROY_CLAIM_RENEW_INTERVAL_MS); + + // Never keep the process alive just to renew a teardown lease. + renewTimer.unref?.(); + + return { + outcome: 'claimed', + finish: () => { + clearInterval(renewTimer); + }, + release: async () => { + clearInterval(renewTimer); + try { + await getRedis().eval(RELEASE_IF_OWNED_SCRIPT, 1, key, token); + } catch (error) { + console.warn( + `[claimMachineDestroy] Failed to release teardown claim for ${params.provider} machine ${params.machineId}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } + }, + }; +}