From 0c2663a23d65709e81c74b7ba4ca33621c9c3a14 Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Wed, 29 Jul 2026 22:43:35 -0400 Subject: [PATCH 1/4] [Fix] Destroy compute sandboxes when task runs are canceled Canceled runs leave the sleep-check candidate set and no cancel writer ever called destroyInstance, so a canceled run's sandbox kept running (and counting against the provider's concurrent-sandbox capacity) until the provider TTL reaped it. Adds a shared destroyCanceledTaskRunSandbox helper that tears down the machine when the run is canceled, has a machineId, was not snapshotted, and has no final compute_provider_usage record yet, recording the usual destroy mutation events and usage row. Wired into finishRun(Canceled), the direct-cancel writers (API cancel handler, web cancel command, pre-sandbox stop), and a post-spawn controller check that covers a cancel racing machine provisioning. Also maps the compute broker's sandbox capacity rejection to actionable copy in the web app instead of the raw broker error. --- .../tasks/__tests__/task-stop.test.ts | 9 + apps/api/src/handlers/tasks/cancelTask.ts | 9 + apps/api/src/handlers/tasks/task-stop.ts | 13 +- apps/controller/src/BaseController.ts | 43 ++- apps/web/src/lib/task-run-errors.test.ts | 16 + apps/web/src/lib/task-run-errors.ts | 9 + apps/web/src/trpc/commands/task-runs/index.ts | 9 + packages/sdk/src/server/index.ts | 1 + .../destroy-canceled-run-sandbox.test.ts | 285 ++++++++++++++++++ .../task-runs/__tests__/finish-run.test.ts | 54 ++++ .../task-runs/destroy-canceled-run-sandbox.ts | 191 ++++++++++++ .../src/server/lib/task-runs/finish-run.ts | 10 + .../sdk/src/server/lib/task-runs/index.ts | 1 + 13 files changed, 648 insertions(+), 2 deletions(-) create mode 100644 packages/sdk/src/server/lib/task-runs/__tests__/destroy-canceled-run-sandbox.test.ts create mode 100644 packages/sdk/src/server/lib/task-runs/destroy-canceled-run-sandbox.ts 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/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.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/sdk/src/server/index.ts b/packages/sdk/src/server/index.ts index 05373590e..16e656819 100644 --- a/packages/sdk/src/server/index.ts +++ b/packages/sdk/src/server/index.ts @@ -13,6 +13,7 @@ export { } from './trpc'; export { finishRun } from './lib/task-runs/finish-run'; +export { destroyCanceledTaskRunSandbox } from './lib/task-runs/destroy-canceled-run-sandbox'; 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..423030ced --- /dev/null +++ b/packages/sdk/src/server/lib/task-runs/__tests__/destroy-canceled-run-sandbox.test.ts @@ -0,0 +1,285 @@ +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 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 = []; + 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('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..0df894a4a --- /dev/null +++ b/packages/sdk/src/server/lib/task-runs/destroy-canceled-run-sandbox.ts @@ -0,0 +1,191 @@ +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 { 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'; + const client = await createCancelTeardownClient(provider); + + const recordMutation = createComputeProviderMutationEventRecorder( + db, + { + runId: run.id, + taskId: run.taskId, + }, + { logPrefix, logger: console }, + ); + + 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, + })); + } catch (error) { + 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..d24f98fff 100644 --- a/packages/sdk/src/server/lib/task-runs/index.ts +++ b/packages/sdk/src/server/lib/task-runs/index.ts @@ -5,6 +5,7 @@ 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 './enqueue-snapshot'; export * from './enqueue-sleep'; From 7be2c76d80ed248e6d3ee7c181ec7a250bbc6aeb Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Wed, 29 Jul 2026 23:01:34 -0400 Subject: [PATCH 2/4] Serialize sandbox teardown with a machine-destroy claim Review feedback: the final-usage lookup alone cannot arbitrate a live race between cancel finalization and sleep-check because both record the destroy usage row only after destroyInstance returns, so both could issue the provider delete and the loser recorded a false failed mutation. Both destroyers now take an atomic redis SET NX claim keyed by provider + machineId before the provider call, skip when the claim is held, and release it when the delete fails so retries can re-claim. Redis outages fall back to unserialized teardown rather than leaking the machine. Also mocks @roomote/sdk/server in the web task-runs command test so the suite no longer pulls schema tables into its hand-rolled db mock (the CI test failure). --- .../__tests__/sleep-check.test.ts | 70 ++++++++++++++ apps/bullmq/src/scheduled-jobs/sleep-check.ts | 25 +++++ .../src/trpc/commands/task-runs/index.test.ts | 7 ++ packages/redis/src/index.ts | 7 ++ packages/sdk/src/server/index.ts | 4 + .../destroy-canceled-run-sandbox.test.ts | 95 +++++++++++++++++++ .../task-runs/destroy-canceled-run-sandbox.ts | 29 ++++++ .../sdk/src/server/lib/task-runs/index.ts | 1 + .../lib/task-runs/machine-destroy-claim.ts | 77 +++++++++++++++ 9 files changed, 315 insertions(+) create mode 100644 packages/sdk/src/server/lib/task-runs/machine-destroy-claim.ts 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..bc1a8ae1c 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,8 @@ const { mockGetInstanceStatus, mockCreateSnapshot, mockFinishRun, + mockClaimMachineDestroy, + mockReleaseMachineDestroyClaim, mockRecordComputeProviderUsage, mockRecordMutation, mockCreateComputeProviderMutationEventRecorder, @@ -74,6 +76,8 @@ const { mockGetInstanceStatus: vi.fn() as AnyMock, mockCreateSnapshot: vi.fn() as AnyMock, mockFinishRun: vi.fn() as AnyMock, + mockClaimMachineDestroy: vi.fn() as AnyMock, + mockReleaseMachineDestroyClaim: vi.fn() as AnyMock, mockRecordComputeProviderUsage: vi.fn() as AnyMock, mockRecordMutation: vi.fn() as AnyMock, mockCreateComputeProviderMutationEventRecorder: vi.fn() as AnyMock, @@ -114,6 +118,8 @@ vi.mock('@roomote/compute-providers', () => ({ vi.mock('@roomote/sdk/server', () => ({ createSnapshot: mockCreateSnapshot, finishRun: mockFinishRun, + claimMachineDestroy: mockClaimMachineDestroy, + releaseMachineDestroyClaim: mockReleaseMachineDestroyClaim, recordComputeProviderUsage: mockRecordComputeProviderUsage, })); @@ -342,6 +348,8 @@ describe('sleepCheckJob', () => { }); returningFn.mockResolvedValue([]); mockFinishRun.mockResolvedValue(undefined); + mockClaimMachineDestroy.mockResolvedValue('claimed'); + mockReleaseMachineDestroyClaim.mockResolvedValue(undefined); // No stop request persisted on the row unless a test opts in. mockDbQueryTaskRunsFindFirst.mockResolvedValue(undefined); }); @@ -851,6 +859,68 @@ 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('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(mockReleaseMachineDestroyClaim).toHaveBeenCalledWith({ + provider: 'modal', + machineId: 'sb-2', + }); + 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..201d78fc5 100644 --- a/apps/bullmq/src/scheduled-jobs/sleep-check.ts +++ b/apps/bullmq/src/scheduled-jobs/sleep-check.ts @@ -35,9 +35,11 @@ import { type ComputeProviderClient, } from '@roomote/compute-providers'; import { + claimMachineDestroy, createSnapshot, finishRun, refreshTaskTitleOnCompletion, + releaseMachineDestroyClaim, } from '@roomote/sdk/server'; import { tryRecordComputeProviderUsage } from '../compute-provider-usage'; @@ -1432,6 +1434,22 @@ 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. + const claim = await claimMachineDestroy({ + provider: job.vendor ?? 'docker', + machineId: job.machineId!, + owner: logPrefix, + }); + + if (claim === 'held') { + console.log( + `[${logPrefix}] Skipping destroyInstance for ${job.machineId}: another destroyer holds the teardown claim`, + ); + return; + } + const recordMutation = createComputeProviderMutationEventRecorder( db, { @@ -1465,6 +1483,13 @@ async function destroyInstanceWithAudit( logPrefix, }); } catch (error) { + // Give the claim back so a later sweep or cancel finalization can retry. + if (claim === 'claimed') { + await releaseMachineDestroyClaim({ + provider: job.vendor ?? 'docker', + machineId: job.machineId!, + }); + } await recordMutation({ provider: job.vendor ?? 'docker', operation: 'destroy_instance', 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/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 16e656819..3ca5617d4 100644 --- a/packages/sdk/src/server/index.ts +++ b/packages/sdk/src/server/index.ts @@ -14,6 +14,10 @@ export { export { finishRun } from './lib/task-runs/finish-run'; export { destroyCanceledTaskRunSandbox } from './lib/task-runs/destroy-canceled-run-sandbox'; +export { + claimMachineDestroy, + releaseMachineDestroyClaim, +} 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 index 423030ced..f58c6023f 100644 --- 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 @@ -49,6 +49,21 @@ vi.mock('@roomote/db/server', async () => { }; }); +const mockRedisSet = vi.fn(); +const mockRedisDel = vi.fn().mockResolvedValue(1); + +vi.mock('@roomote/redis', async () => { + const actual = + await vi.importActual('@roomote/redis'); + return { + ...actual, + getRedis: () => ({ + set: (...args: unknown[]) => mockRedisSet(...args), + del: (...args: unknown[]) => mockRedisDel(...args), + }), + }; +}); + const mockDestroyInstance = vi.fn(); const mockCreateComputeProviderClient = vi.fn().mockReturnValue({ destroyInstance: (...args: unknown[]) => mockDestroyInstance(...args), @@ -89,6 +104,8 @@ describe('destroyCanceledTaskRunSandbox', () => { beforeEach(() => { vi.clearAllMocks(); finalUsageRows = []; + mockRedisSet.mockResolvedValue('OK'); + mockRedisDel.mockResolvedValue(1); mockResolveComputeProviderEnvValues.mockResolvedValue({ ROOMOTE_CLOUD_TOKEN_ID: 'tenant-1', }); @@ -235,6 +252,84 @@ describe('destroyCanceledTaskRunSandbox', () => { expect(mockRecordComputeProviderUsage).not.toHaveBeenCalled(); }); + it('claims the machine before the provider delete', async () => { + mockFindFirstRun.mockResolvedValue(makeRun()); + + await destroyCanceledTaskRunSandbox({ runId: 42, logPrefix: 'test' }); + + expect(mockRedisSet).toHaveBeenCalledWith( + 'compute:machine-destroy-claim:roomote:sb-42', + '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('releases the claim 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'); + expect(mockRedisDel).toHaveBeenCalledWith( + 'compute:machine-destroy-claim:roomote:sb-42', + ); + }); + + 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')); 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 index 0df894a4a..aa9d031d2 100644 --- 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 @@ -15,6 +15,10 @@ import { } from '@roomote/db/server'; import { createComputeProviderClient } from '@roomote/compute-providers'; +import { + claimMachineDestroy, + releaseMachineDestroyClaim, +} from './machine-destroy-claim'; import { recordComputeProviderUsage } from './record-compute-provider-usage'; type DestroyCanceledTaskRunSandboxResult = 'destroyed' | 'skipped' | 'failed'; @@ -96,6 +100,24 @@ export async function destroyCanceledTaskRunSandbox(params: { } const provider: ComputeProvider = run.vendor ?? 'docker'; + + // 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. + const claim = await claimMachineDestroy({ + provider, + machineId: run.machineId, + owner: logPrefix, + }); + + if (claim === 'held') { + console.log( + `[${logPrefix}] Skipping sandbox teardown for canceled task run #${run.id}: another destroyer holds the claim for ${run.machineId}`, + ); + return 'skipped'; + } + const client = await createCancelTeardownClient(provider); const recordMutation = createComputeProviderMutationEventRecorder( @@ -129,6 +151,13 @@ export async function destroyCanceledTaskRunSandbox(params: { instanceId: run.machineId, })); } catch (error) { + // Give the claim back so sleep-check or a later cancel path can retry. + if (claim === 'claimed') { + await releaseMachineDestroyClaim({ + provider, + machineId: run.machineId, + }); + } await recordMutation({ provider, operation: 'destroy_instance', diff --git a/packages/sdk/src/server/lib/task-runs/index.ts b/packages/sdk/src/server/lib/task-runs/index.ts index d24f98fff..f3af25c24 100644 --- a/packages/sdk/src/server/lib/task-runs/index.ts +++ b/packages/sdk/src/server/lib/task-runs/index.ts @@ -7,6 +7,7 @@ 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..471869ea3 --- /dev/null +++ b/packages/sdk/src/server/lib/task-runs/machine-destroy-claim.ts @@ -0,0 +1,77 @@ +import type { ComputeProvider } from '@roomote/types'; +import { getRedis, REDIS_KEYS } from '@roomote/redis'; + +/** + * Long enough to outlive any realistic provider destroy call, short enough + * that a crashed claim holder does not block teardown retries for long. + */ +const MACHINE_DESTROY_CLAIM_TTL_SECONDS = 15 * 60; + +export type MachineDestroyClaimOutcome = + /** This caller owns the teardown and must issue the provider delete. */ + | 'claimed' + /** Another caller is already destroying this machine — do not delete. */ + | 'held' + /** Redis is unreachable; proceed unserialized rather than leak the machine. */ + | 'unavailable'; + +function buildMachineDestroyClaimKey( + provider: ComputeProvider, + machineId: string, +): string { + return `${REDIS_KEYS.MACHINE_DESTROY_CLAIM}:${provider}:${machineId}`; +} + +/** + * 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. + * + * The claim is left to expire after a successful destroy (destroys are + * permanent) and must be released via releaseMachineDestroyClaim when the + * provider call fails, so a later attempt can retry. + */ +export async function claimMachineDestroy(params: { + provider: ComputeProvider; + machineId: string; + /** Caller tag stored as the claim value for debugging. */ + owner: string; +}): Promise { + try { + const claim = await getRedis().set( + buildMachineDestroyClaimKey(params.provider, params.machineId), + params.owner, + 'EX', + MACHINE_DESTROY_CLAIM_TTL_SECONDS, + 'NX', + ); + + return claim === 'OK' ? 'claimed' : 'held'; + } catch (error) { + console.warn( + `[claimMachineDestroy] Redis unavailable while claiming ${params.provider} machine ${params.machineId}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + return 'unavailable'; + } +} + +/** Best-effort release after a failed destroy so retries can re-claim. */ +export async function releaseMachineDestroyClaim(params: { + provider: ComputeProvider; + machineId: string; +}): Promise { + try { + await getRedis().del( + buildMachineDestroyClaimKey(params.provider, params.machineId), + ); + } catch (error) { + console.warn( + `[releaseMachineDestroyClaim] Failed to release claim for ${params.provider} machine ${params.machineId}: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } +} From c13a025bb59c794e27544df08b14043ed4626c63 Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Wed, 29 Jul 2026 23:04:48 -0400 Subject: [PATCH 3/4] Keep MachineDestroyClaimOutcome internal to satisfy knip --- packages/sdk/src/server/lib/task-runs/machine-destroy-claim.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 471869ea3..ace9a8ba5 100644 --- a/packages/sdk/src/server/lib/task-runs/machine-destroy-claim.ts +++ b/packages/sdk/src/server/lib/task-runs/machine-destroy-claim.ts @@ -7,7 +7,7 @@ import { getRedis, REDIS_KEYS } from '@roomote/redis'; */ const MACHINE_DESTROY_CLAIM_TTL_SECONDS = 15 * 60; -export type MachineDestroyClaimOutcome = +type MachineDestroyClaimOutcome = /** This caller owns the teardown and must issue the provider delete. */ | 'claimed' /** Another caller is already destroying this machine — do not delete. */ From 4a47ac0db1bdb9e9840dc2a67fbc9533ce4b30a7 Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Wed, 29 Jul 2026 23:16:09 -0400 Subject: [PATCH 4/4] Own the teardown claim with a token and renew its lease Review feedback: a slow destroyInstance could outlive the fixed 15-minute lease, letting a later cancel/sweep acquire the key and issue a second delete while the first was still in flight; and the failure path's unconditional DEL could remove that successor's claim. The claim now stores a unique per-caller token, release and renewal are Lua-conditional on that token, and the lease renews every 5 minutes in the background until the caller settles it (finish on success, release on failure). The renewal timer is unref'd and the client is built before claiming so an early failure never leaves an armed lease. --- .../__tests__/sleep-check.test.ts | 23 ++-- apps/bullmq/src/scheduled-jobs/sleep-check.ts | 19 +-- packages/sdk/src/server/index.ts | 5 +- .../destroy-canceled-run-sandbox.test.ts | 65 ++++++++- .../task-runs/destroy-canceled-run-sandbox.ts | 46 +++---- .../lib/task-runs/machine-destroy-claim.ts | 129 +++++++++++++----- 6 files changed, 199 insertions(+), 88 deletions(-) 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 bc1a8ae1c..251944b1c 100644 --- a/apps/bullmq/src/scheduled-jobs/__tests__/sleep-check.test.ts +++ b/apps/bullmq/src/scheduled-jobs/__tests__/sleep-check.test.ts @@ -11,7 +11,8 @@ const { mockCreateSnapshot, mockFinishRun, mockClaimMachineDestroy, - mockReleaseMachineDestroyClaim, + mockClaimRelease, + mockClaimFinish, mockRecordComputeProviderUsage, mockRecordMutation, mockCreateComputeProviderMutationEventRecorder, @@ -77,7 +78,8 @@ const { mockCreateSnapshot: vi.fn() as AnyMock, mockFinishRun: vi.fn() as AnyMock, mockClaimMachineDestroy: vi.fn() as AnyMock, - mockReleaseMachineDestroyClaim: 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, @@ -119,7 +121,6 @@ vi.mock('@roomote/sdk/server', () => ({ createSnapshot: mockCreateSnapshot, finishRun: mockFinishRun, claimMachineDestroy: mockClaimMachineDestroy, - releaseMachineDestroyClaim: mockReleaseMachineDestroyClaim, recordComputeProviderUsage: mockRecordComputeProviderUsage, })); @@ -348,8 +349,12 @@ describe('sleepCheckJob', () => { }); returningFn.mockResolvedValue([]); mockFinishRun.mockResolvedValue(undefined); - mockClaimMachineDestroy.mockResolvedValue('claimed'); - mockReleaseMachineDestroyClaim.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); }); @@ -879,7 +884,7 @@ describe('sleepCheckJob', () => { }); returningFn.mockResolvedValue([{ id: 99 }]); // A concurrent cancel finalization already owns the teardown. - mockClaimMachineDestroy.mockResolvedValue('held'); + mockClaimMachineDestroy.mockResolvedValue({ outcome: 'held' }); await sleepCheckJob(); @@ -914,10 +919,8 @@ describe('sleepCheckJob', () => { await sleepCheckJob().catch(() => {}); - expect(mockReleaseMachineDestroyClaim).toHaveBeenCalledWith({ - provider: 'modal', - machineId: 'sb-2', - }); + expect(mockClaimRelease).toHaveBeenCalled(); + expect(mockClaimFinish).not.toHaveBeenCalled(); expect(mockRecordComputeProviderUsage).not.toHaveBeenCalled(); }); diff --git a/apps/bullmq/src/scheduled-jobs/sleep-check.ts b/apps/bullmq/src/scheduled-jobs/sleep-check.ts index 201d78fc5..574dc472a 100644 --- a/apps/bullmq/src/scheduled-jobs/sleep-check.ts +++ b/apps/bullmq/src/scheduled-jobs/sleep-check.ts @@ -39,7 +39,6 @@ import { createSnapshot, finishRun, refreshTaskTitleOnCompletion, - releaseMachineDestroyClaim, } from '@roomote/sdk/server'; import { tryRecordComputeProviderUsage } from '../compute-provider-usage'; @@ -1437,13 +1436,15 @@ 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 === 'held') { + if (claim.outcome === 'held') { console.log( `[${logPrefix}] Skipping destroyInstance for ${job.machineId}: another destroyer holds the teardown claim`, ); @@ -1471,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', @@ -1483,13 +1488,9 @@ async function destroyInstanceWithAudit( logPrefix, }); } catch (error) { - // Give the claim back so a later sweep or cancel finalization can retry. - if (claim === 'claimed') { - await releaseMachineDestroyClaim({ - provider: job.vendor ?? 'docker', - machineId: job.machineId!, - }); - } + // 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/packages/sdk/src/server/index.ts b/packages/sdk/src/server/index.ts index 3ca5617d4..9d4c5bc0d 100644 --- a/packages/sdk/src/server/index.ts +++ b/packages/sdk/src/server/index.ts @@ -14,10 +14,7 @@ export { export { finishRun } from './lib/task-runs/finish-run'; export { destroyCanceledTaskRunSandbox } from './lib/task-runs/destroy-canceled-run-sandbox'; -export { - claimMachineDestroy, - releaseMachineDestroyClaim, -} from './lib/task-runs/machine-destroy-claim'; +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 index f58c6023f..e59fca768 100644 --- 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 @@ -50,7 +50,7 @@ vi.mock('@roomote/db/server', async () => { }); const mockRedisSet = vi.fn(); -const mockRedisDel = vi.fn().mockResolvedValue(1); +const mockRedisEval = vi.fn().mockResolvedValue(1); vi.mock('@roomote/redis', async () => { const actual = @@ -59,7 +59,7 @@ vi.mock('@roomote/redis', async () => { ...actual, getRedis: () => ({ set: (...args: unknown[]) => mockRedisSet(...args), - del: (...args: unknown[]) => mockRedisDel(...args), + eval: (...args: unknown[]) => mockRedisEval(...args), }), }; }); @@ -105,7 +105,7 @@ describe('destroyCanceledTaskRunSandbox', () => { vi.clearAllMocks(); finalUsageRows = []; mockRedisSet.mockResolvedValue('OK'); - mockRedisDel.mockResolvedValue(1); + mockRedisEval.mockResolvedValue(1); mockResolveComputeProviderEnvValues.mockResolvedValue({ ROOMOTE_CLOUD_TOKEN_ID: 'tenant-1', }); @@ -252,14 +252,14 @@ describe('destroyCanceledTaskRunSandbox', () => { expect(mockRecordComputeProviderUsage).not.toHaveBeenCalled(); }); - it('claims the machine before the provider delete', async () => { + 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', - 'test', + expect.stringMatching(/^test:/), 'EX', expect.any(Number), 'NX', @@ -302,7 +302,7 @@ describe('destroyCanceledTaskRunSandbox', () => { expect(mockRecordMutation).not.toHaveBeenCalled(); }); - it('releases the claim when the provider delete fails', async () => { + it('conditionally releases its own token when the provider delete fails', async () => { mockFindFirstRun.mockResolvedValue(makeRun()); mockDestroyInstance.mockRejectedValue(new Error('provider down')); @@ -312,11 +312,62 @@ describe('destroyCanceledTaskRunSandbox', () => { }); expect(result).toBe('failed'); - expect(mockRedisDel).toHaveBeenCalledWith( + // 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')); 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 index aa9d031d2..b3ccdb023 100644 --- 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 @@ -15,10 +15,7 @@ import { } from '@roomote/db/server'; import { createComputeProviderClient } from '@roomote/compute-providers'; -import { - claimMachineDestroy, - releaseMachineDestroyClaim, -} from './machine-destroy-claim'; +import { claimMachineDestroy } from './machine-destroy-claim'; import { recordComputeProviderUsage } from './record-compute-provider-usage'; type DestroyCanceledTaskRunSandboxResult = 'destroyed' | 'skipped' | 'failed'; @@ -101,34 +98,36 @@ export async function destroyCanceledTaskRunSandbox(params: { 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. + // provider delete for this machine, and the lease renews until settled. const claim = await claimMachineDestroy({ provider, machineId: run.machineId, owner: logPrefix, }); - if (claim === 'held') { + 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 client = await createCancelTeardownClient(provider); - - const recordMutation = createComputeProviderMutationEventRecorder( - db, - { - runId: run.id, - taskId: run.taskId, - }, - { logPrefix, logger: console }, - ); - const details = { phase: 'destroy_after_cancel', reason: 'task_run_canceled', @@ -150,14 +149,13 @@ export async function destroyCanceledTaskRunSandbox(params: { ({ 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 so sleep-check or a later cancel path can retry. - if (claim === 'claimed') { - await releaseMachineDestroyClaim({ - provider, - machineId: run.machineId, - }); - } + // 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', 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 index ace9a8ba5..c54f54b07 100644 --- a/packages/sdk/src/server/lib/task-runs/machine-destroy-claim.ts +++ b/packages/sdk/src/server/lib/task-runs/machine-destroy-claim.ts @@ -1,19 +1,47 @@ +import { randomUUID } from 'node:crypto'; + import type { ComputeProvider } from '@roomote/types'; import { getRedis, REDIS_KEYS } from '@roomote/redis'; /** - * Long enough to outlive any realistic provider destroy call, short enough - * that a crashed claim holder does not block teardown retries for long. + * 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; -type MachineDestroyClaimOutcome = - /** This caller owns the teardown and must issue the provider delete. */ - | 'claimed' +/** 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. */ - | 'held' - /** Redis is unreachable; proceed unserialized rather than leak the machine. */ - | 'unavailable'; + | { 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, @@ -22,56 +50,89 @@ function buildMachineDestroyClaimKey( 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. * - * The claim is left to expire after a successful destroy (destroys are - * permanent) and must be released via releaseMachineDestroyClaim when the - * provider call fails, so a later attempt can retry. + * 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 stored as the claim value for debugging. */ + /** Caller tag embedded in the claim token for debugging. */ owner: string; -}): Promise { +}): Promise { + const key = buildMachineDestroyClaimKey(params.provider, params.machineId); + const token = `${params.owner}:${randomUUID()}`; + try { const claim = await getRedis().set( - buildMachineDestroyClaimKey(params.provider, params.machineId), - params.owner, + key, + token, 'EX', MACHINE_DESTROY_CLAIM_TTL_SECONDS, 'NX', ); - return claim === 'OK' ? 'claimed' : 'held'; + 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 'unavailable'; + return { outcome: 'unavailable', ...NOOP_CLAIM_HANDLE }; } -} -/** Best-effort release after a failed destroy so retries can re-claim. */ -export async function releaseMachineDestroyClaim(params: { - provider: ComputeProvider; - machineId: string; -}): Promise { - try { - await getRedis().del( - buildMachineDestroyClaimKey(params.provider, params.machineId), - ); - } catch (error) { - console.warn( - `[releaseMachineDestroyClaim] Failed to release claim for ${params.provider} machine ${params.machineId}: ${ - error instanceof Error ? error.message : String(error) - }`, - ); - } + 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) + }`, + ); + } + }, + }; }