diff --git a/apps/sim/lib/execution/isolated-vm.test.ts b/apps/sim/lib/execution/isolated-vm.test.ts index 2d77e438c8f..717f9ebaa2e 100644 --- a/apps/sim/lib/execution/isolated-vm.test.ts +++ b/apps/sim/lib/execution/isolated-vm.test.ts @@ -654,7 +654,57 @@ describe('isolated-vm scheduler', () => { expect(result.error?.message).toContain('Too many concurrent') }) + it('releases the lease when abort races an undetermined acquisition', async () => { + const scripts: string[] = [] + let markLeaseRequested!: () => void + const leaseRequested = new Promise((resolve) => { + markLeaseRequested = resolve + }) + const { executeInIsolatedVM, spawnMock } = await loadExecutionModule({ + envOverrides: { + REDIS_URL: 'redis://localhost:6379', + IVM_LEASE_REDIS_DEADLINE_MS: '5', + }, + spawns: [() => createReadyProc('unused')], + redisEvalImpl: (...args: unknown[]) => { + const script = String(args[0] ?? '') + scripts.push(script) + // Never settles, so the deadline decides and Redis may still register + // the lease id afterwards. + if (script.includes('ZREMRANGEBYSCORE')) { + markLeaseRequested() + return new Promise(() => {}) + } + return 1 + }, + }) + const controller = new AbortController() + + const execution = executeInIsolatedVM( + { + code: 'return "unused"', + params: {}, + envVars: {}, + contextVariables: {}, + timeoutMs: 100, + requestId: 'req-abort-undetermined', + ownerKey: 'user:cancelled-undetermined', + }, + { signal: controller.signal } + ) + await leaseRequested + controller.abort(new DOMException('user', 'AbortError')) + + await expect(execution).resolves.toMatchObject({ + termination: 'cancelled', + error: { name: 'AbortError' }, + }) + expect(scripts.some((script) => script.includes("'ZREM'"))).toBe(true) + expect(spawnMock).not.toHaveBeenCalled() + }) + it('reports cancellation when abort races a rejected distributed lease', async () => { + const scripts: string[] = [] let resolveLease!: (value: number) => void let markLeaseRequested!: () => void const leaseResult = new Promise((resolve) => { @@ -670,6 +720,7 @@ describe('isolated-vm scheduler', () => { spawns: [() => createReadyProc('unused')], redisEvalImpl: (...args: unknown[]) => { const script = String(args[0] ?? '') + scripts.push(script) if (script.includes('ZREMRANGEBYSCORE')) { markLeaseRequested() return leaseResult @@ -699,6 +750,8 @@ describe('isolated-vm scheduler', () => { termination: 'cancelled', error: { name: 'AbortError' }, }) + // Redis answered before its ZADD, so there is nothing to remove. + expect(scripts.some((script) => script.includes("'ZREM'"))).toBe(false) expect(spawnMock).not.toHaveBeenCalled() }) diff --git a/apps/sim/lib/execution/isolated-vm.ts b/apps/sim/lib/execution/isolated-vm.ts index 4918380f77c..ba9832fe41f 100644 --- a/apps/sim/lib/execution/isolated-vm.ts +++ b/apps/sim/lib/execution/isolated-vm.ts @@ -1415,7 +1415,29 @@ export async function executeInIsolatedVM( distributedLeaseId, req.timeoutMs ) + let settled = false + /** + * Released even when the acquisition was undetermined. The deadline abandons + * the local wait but cannot cancel the script, so a late completion still + * registers this lease id — and unreleased it would count against the owner + * for the whole TTL, denying later executions that do have capacity. The + * lease id is unique to this execution, so removing one that was never + * registered is a no-op. + * + * Declared before the early returns below so every exit path that can leave a + * registration behind reaches it, not just the ones that run the execution. + */ + const releaseLease = () => { + if (settled) return + settled = true + releaseDistributedLease(ownerKey, distributedLeaseId).catch((error) => { + logger.error('Failed to release distributed lease', { ownerKey, error }) + }) + } + if (leaseAcquireResult !== 'acquired' && signal?.aborted) { + // Only an undetermined result can have registered; see the over-limit branch below. + if (leaseAcquireResult === 'undetermined') releaseLease() maybeCleanupOwner(ownerKey) return { result: null, @@ -1430,6 +1452,7 @@ export async function executeInIsolatedVM( ownerKey, max: DISTRIBUTED_MAX_INFLIGHT_PER_OWNER, }) + // No release: the script returns this before its ZADD, so nothing was registered. maybeCleanupOwner(ownerKey) return { result: null, @@ -1444,23 +1467,6 @@ export async function executeInIsolatedVM( // An undetermined lease cannot reject the execution: the per-process pool and // the per-owner active/queued limits above still bound this work. - let settled = false - /** - * Released even when the acquisition was undetermined. The deadline abandons - * the local wait but cannot cancel the script, so a late completion still - * registers this lease id — and unreleased it would count against the owner - * for the whole TTL, denying later executions that do have capacity. The - * lease id is unique to this execution, so removing one that was never - * registered is a no-op. - */ - const releaseLease = () => { - if (settled) return - settled = true - releaseDistributedLease(ownerKey, distributedLeaseId).catch((error) => { - logger.error('Failed to release distributed lease', { ownerKey, error }) - }) - } - const state: ExecutionState = { cancelled: false } return new Promise((resolve) => {