fix(execution): release the lease when abort races an undetermined acquisition (#7237)

* 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.

* fix(execution): keep the over-limit abort path release-free

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.

Author: waleedlatif1

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<void>((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<number>(() => {})
+        }
+        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<number>((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()
   })
 

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<IsolatedVMExecutionResult>((resolve) => {