From 2422e53e451082e93a299b80176e77636623d74a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 16:10:57 -0700 Subject: [PATCH 1/2] fix(execution): release the lease when abort races an undetermined acquisition The cancellation return for an aborted execution ran before `releaseLease` was declared, so it exited without releasing. An undetermined acquisition can still be registered by Redis after the deadline abandons the local wait, and that member then counted against the owner for the whole TTL, denying later executions that had capacity. Declare the release before the early returns so every exit path that can leave a registration behind reaches it, and call it on the cancellation path. The over-limit return still skips it: the script answers that before its ZADD, so nothing was ever registered. --- apps/sim/lib/execution/isolated-vm.test.ts | 49 ++++++++++++++++++++++ apps/sim/lib/execution/isolated-vm.ts | 39 +++++++++-------- 2 files changed, 71 insertions(+), 17 deletions(-) diff --git a/apps/sim/lib/execution/isolated-vm.test.ts b/apps/sim/lib/execution/isolated-vm.test.ts index 2d77e438c8f..298b63303ca 100644 --- a/apps/sim/lib/execution/isolated-vm.test.ts +++ b/apps/sim/lib/execution/isolated-vm.test.ts @@ -654,6 +654,55 @@ 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 () => { let resolveLease!: (value: number) => void let markLeaseRequested!: () => void diff --git a/apps/sim/lib/execution/isolated-vm.ts b/apps/sim/lib/execution/isolated-vm.ts index 4918380f77c..286afb3bd20 100644 --- a/apps/sim/lib/execution/isolated-vm.ts +++ b/apps/sim/lib/execution/isolated-vm.ts @@ -1415,7 +1415,28 @@ 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) { + releaseLease() maybeCleanupOwner(ownerKey) return { result: null, @@ -1430,6 +1451,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 +1466,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) => { From 5a2f0e34880da7611a4e6adcaac75cf402b4cf4b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 16:17:22 -0700 Subject: [PATCH 2/2] fix(execution): keep the over-limit abort path release-free MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The abort guard fires for any non-acquired result, so an over-limit acquisition that was already aborted also released — contradicting the rule stated two lines below, where the same result returns without releasing because the script answers before its ZADD. Restrict the release to undetermined results and assert the no-release behavior in the existing over-limit abort test. --- apps/sim/lib/execution/isolated-vm.test.ts | 4 ++++ apps/sim/lib/execution/isolated-vm.ts | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/apps/sim/lib/execution/isolated-vm.test.ts b/apps/sim/lib/execution/isolated-vm.test.ts index 298b63303ca..717f9ebaa2e 100644 --- a/apps/sim/lib/execution/isolated-vm.test.ts +++ b/apps/sim/lib/execution/isolated-vm.test.ts @@ -704,6 +704,7 @@ describe('isolated-vm scheduler', () => { }) 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) => { @@ -719,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 @@ -748,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 286afb3bd20..ba9832fe41f 100644 --- a/apps/sim/lib/execution/isolated-vm.ts +++ b/apps/sim/lib/execution/isolated-vm.ts @@ -1436,7 +1436,8 @@ export async function executeInIsolatedVM( } if (leaseAcquireResult !== 'acquired' && signal?.aborted) { - releaseLease() + // Only an undetermined result can have registered; see the over-limit branch below. + if (leaseAcquireResult === 'undetermined') releaseLease() maybeCleanupOwner(ownerKey) return { result: null,