From 2018225a90da48d460edd5eb95162861057f0e9f Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 13:25:48 -0700 Subject: [PATCH 1/3] fix(execution): treat an undetermined lease as a fallback, not a denial The distributed owner lease is a cross-process fairness check, not a correctness lock. A round trip that did not answer before its deadline was reported as a hard failure and rejected the execution, even though the per-process pool and the per-owner active/queued limits still bound the work. - Fall back to the local limits when the lease is undetermined. Only `limit_exceeded` denies an execution, since it is an actual answer. - Rename that outcome from `unavailable` to `undetermined` so the absence of an answer is not read as a negative one, and log it at warn. - Make the round-trip deadline configurable and raise its default. This deadline and the client's `commandTimeout` are both plain timers, so a value near normal event-loop latency misreads a scheduling pause as an unreachable dependency. - Skip the release round trip when no lease was ever registered. --- apps/sim/lib/core/config/env.ts | 1 + apps/sim/lib/execution/isolated-vm.test.ts | 75 +++++++++++++++++++--- apps/sim/lib/execution/isolated-vm.ts | 51 +++++++++------ 3 files changed, 96 insertions(+), 31 deletions(-) diff --git a/apps/sim/lib/core/config/env.ts b/apps/sim/lib/core/config/env.ts index 5bc20313e08..74462eb6c72 100644 --- a/apps/sim/lib/core/config/env.ts +++ b/apps/sim/lib/core/config/env.ts @@ -432,6 +432,7 @@ export const env = createEnv({ IVM_MAX_OWNER_WEIGHT: z.string().optional().default('5'), // Max accepted weight for weighted owner scheduling IVM_DISTRIBUTED_MAX_INFLIGHT_PER_OWNER:z.string().optional().default('2200'), // Max owner in-flight leases across replicas IVM_DISTRIBUTED_LEASE_MIN_TTL_MS: z.string().optional().default('120000'), // Min TTL for distributed in-flight leases (ms) + IVM_LEASE_REDIS_DEADLINE_MS: z.string().optional().default('1000'), // Deadline for one distributed lease round trip (ms) IVM_QUEUE_TIMEOUT_MS: z.string().optional().default('300000'), // Max queue wait before rejection (ms) IVM_MAX_EXECUTIONS_PER_WORKER: z.string().optional().default('200'), // Max lifetime executions before worker is recycled IVM_MAX_BROKER_ARGS_JSON_CHARS: z.string().optional().default('262144'), // Max JSON payload size for sandbox task broker args (isolate→host) diff --git a/apps/sim/lib/execution/isolated-vm.test.ts b/apps/sim/lib/execution/isolated-vm.test.ts index 9959a369f93..30a17c7d19d 100644 --- a/apps/sim/lib/execution/isolated-vm.test.ts +++ b/apps/sim/lib/execution/isolated-vm.test.ts @@ -498,7 +498,7 @@ describe('isolated-vm scheduler', () => { expect(result.error?.message).toContain('Too many concurrent') }) - it('fails closed when Redis is configured but unavailable', async () => { + it('falls back to local limits when no Redis client is available', async () => { const { executeInIsolatedVM } = await loadExecutionModule({ envOverrides: { REDIS_URL: 'redis://localhost:6379', @@ -516,14 +516,11 @@ describe('isolated-vm scheduler', () => { ownerKey: 'user:redis-down', }) - expect(result.error).toMatchObject({ - isSystemError: true, - message: 'Code execution coordination is temporarily unavailable. Please try again later.', - }) - expect(result.result).toBeNull() + expect(result.error).toBeUndefined() + expect(result.result).toBe('ok') }) - it('fails closed when Redis lease evaluation errors', async () => { + it('falls back to local limits when the lease evaluation errors', async () => { const { executeInIsolatedVM } = await loadExecutionModule({ envOverrides: { REDIS_URL: 'redis://localhost:6379', @@ -548,10 +545,68 @@ describe('isolated-vm scheduler', () => { ownerKey: 'user:redis-error', }) - expect(result.error).toMatchObject({ - isSystemError: true, - message: 'Code execution coordination is temporarily unavailable. Please try again later.', + expect(result.error).toBeUndefined() + expect(result.result).toBe('ok') + }) + + it('falls back to local limits when the lease round trip exceeds its deadline', async () => { + const { executeInIsolatedVM } = await loadExecutionModule({ + envOverrides: { + REDIS_URL: 'redis://localhost:6379', + IVM_LEASE_REDIS_DEADLINE_MS: '5', + }, + spawns: [() => createReadyProc('ok')], + redisEvalImpl: (...args: unknown[]) => { + const script = String(args[0] ?? '') + // Never settles, so only the deadline can decide the outcome. + if (script.includes('ZREMRANGEBYSCORE')) { + return new Promise(() => {}) + } + return 1 + }, + }) + + const result = await executeInIsolatedVM({ + code: 'return "ok"', + params: {}, + envVars: {}, + contextVariables: {}, + timeoutMs: 100, + requestId: 'req-9', + ownerKey: 'user:redis-slow', + }) + + expect(result.error).toBeUndefined() + expect(result.result).toBe('ok') + }) + + it('still rejects when Redis answers that the owner is over its lease limit', async () => { + const { executeInIsolatedVM } = await loadExecutionModule({ + envOverrides: { + IVM_DISTRIBUTED_MAX_INFLIGHT_PER_OWNER: '1', + REDIS_URL: 'redis://localhost:6379', + }, + spawns: [() => createReadyProc('ok')], + redisEvalImpl: (...args: unknown[]) => { + const script = String(args[0] ?? '') + if (script.includes('ZREMRANGEBYSCORE')) { + return 0 + } + return 1 + }, + }) + + const result = await executeInIsolatedVM({ + code: 'return "ok"', + params: {}, + envVars: {}, + contextVariables: {}, + timeoutMs: 100, + requestId: 'req-10', + ownerKey: 'user:over-limit', }) + + expect(result.error?.message).toContain('Too many concurrent') expect(result.result).toBeNull() }) diff --git a/apps/sim/lib/execution/isolated-vm.ts b/apps/sim/lib/execution/isolated-vm.ts index 3081506f75b..5946ac232b0 100644 --- a/apps/sim/lib/execution/isolated-vm.ts +++ b/apps/sim/lib/execution/isolated-vm.ts @@ -140,7 +140,16 @@ const MAX_EXECUTIONS_PER_WORKER = Number.parseInt(env.IVM_MAX_EXECUTIONS_PER_WOR const MAX_BROKER_ARGS_JSON_CHARS = Number.parseInt(env.IVM_MAX_BROKER_ARGS_JSON_CHARS) || 262_144 const MAX_BROKERS_PER_EXECUTION = Number.parseInt(env.IVM_MAX_BROKERS_PER_EXECUTION) || 1000 const DISTRIBUTED_KEY_PREFIX = 'ivm:fair:v1:owner' -const LEASE_REDIS_DEADLINE_MS = 200 +/** + * Deadline for a single lease round trip, kept below the shared Redis client's + * `commandTimeout` so this race still resolves first. + * + * Both this deadline and `commandTimeout` are plain `setTimeout`s, so what they + * actually measure is event-loop scheduling, not Redis. A value near normal loop + * latency therefore reports a healthy Redis as unreachable whenever a garbage + * collection pause lands on the call. Keep it well clear of that floor. + */ +const LEASE_REDIS_DEADLINE_MS = Number.parseInt(env.IVM_LEASE_REDIS_DEADLINE_MS) || 1000 const QUEUE_RETRY_DELAY_MS = 1000 const DISTRIBUTED_LEASE_GRACE_MS = 30000 @@ -347,7 +356,16 @@ function ownerRedisKey(ownerKey: string): string { return `${DISTRIBUTED_KEY_PREFIX}:${ownerKey}` } -type LeaseAcquireResult = 'acquired' | 'limit_exceeded' | 'unavailable' +/** + * Outcome of one distributed lease acquisition. + * + * `limit_exceeded` is an answer from Redis — the owner is genuinely over its + * share — and is the only outcome that denies an execution. `undetermined` + * means no answer arrived before the deadline, which is not a denial and must + * never be projected as one: the local admission limits below still bound the + * work, so the caller falls back to them. + */ +type LeaseAcquireResult = 'acquired' | 'limit_exceeded' | 'undetermined' async function tryAcquireDistributedLease( ownerKey: string, @@ -358,10 +376,10 @@ async function tryAcquireDistributedLease( const redis = getRedisClient() if (!redis) { - logger.error('Redis is configured but unavailable for distributed lease acquisition', { + logger.warn('No Redis client for distributed lease acquisition; using local limits', { ownerKey, }) - return 'unavailable' + return 'undetermined' } const now = Date.now() @@ -407,11 +425,12 @@ async function tryAcquireDistributedLease( ]) return Number(result) === 1 ? 'acquired' : 'limit_exceeded' } catch (error) { - logger.error('Failed to acquire distributed owner lease; execution will be rejected', { + logger.warn('Distributed owner lease undetermined; using local limits', { ownerKey, + deadlineMs: LEASE_REDIS_DEADLINE_MS, error, }) - return 'unavailable' + return 'undetermined' } finally { clearTimeout(deadlineTimer) } @@ -1416,26 +1435,16 @@ export async function executeInIsolatedVM( }, } } - if (leaseAcquireResult === 'unavailable') { - logger.error('Isolated-vm execution rejected because its distributed lease is unavailable', { - ownerKey, - }) - maybeCleanupOwner(ownerKey) - return { - result: null, - stdout: '', - error: { - message: 'Code execution coordination is temporarily unavailable. Please try again later.', - name: 'Error', - isSystemError: true, - }, - } - } + // 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 + const holdsDistributedLease = leaseAcquireResult === 'acquired' const releaseLease = () => { if (settled) return settled = true + // Nothing was registered when the lease was undetermined; skip the round trip. + if (!holdsDistributedLease) return releaseDistributedLease(ownerKey, distributedLeaseId).catch((error) => { logger.error('Failed to release distributed lease', { ownerKey, error }) }) From b9e640c4896cb8d2540246aaa2ef98df485377d4 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 13:34:03 -0700 Subject: [PATCH 2/3] fix(execution): reclaim a lease Redis registers after the local deadline Addresses review findings on the fallback path. - Always release the lease. The deadline abandons the local wait but cannot cancel the script, so a late completion still registers the lease id; leaving it unreleased kept it counted against the owner for the whole TTL and denied later executions that did have capacity. The id is unique per execution, so removing one that was never registered is a no-op. - Treat a non-positive configured deadline as unconfigured. A timer of zero or less fires immediately, which would leave every acquisition undetermined and silently drop cross-replica enforcement. - Cover both with tests: a lease that completes after the deadline is still released, and a non-positive deadline still lets a real answer land. --- apps/sim/lib/execution/isolated-vm.test.ts | 71 ++++++++++++++++++++++ apps/sim/lib/execution/isolated-vm.ts | 19 ++++-- 2 files changed, 86 insertions(+), 4 deletions(-) diff --git a/apps/sim/lib/execution/isolated-vm.test.ts b/apps/sim/lib/execution/isolated-vm.test.ts index 30a17c7d19d..4e6dedf6869 100644 --- a/apps/sim/lib/execution/isolated-vm.test.ts +++ b/apps/sim/lib/execution/isolated-vm.test.ts @@ -8,6 +8,7 @@ import { loggerMock, redisConfigMockFns, } from '@sim/testing' +import { sleep } from '@sim/utils/helpers' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' type MockProc = EventEmitter & { @@ -580,6 +581,76 @@ describe('isolated-vm scheduler', () => { expect(result.result).toBe('ok') }) + it('releases a lease that Redis registers after the local deadline', async () => { + const scripts: string[] = [] + let completeAcquire!: (value: number) => void + const lateAcquire = new Promise((resolve) => { + completeAcquire = resolve + }) + const { executeInIsolatedVM } = await loadExecutionModule({ + envOverrides: { + REDIS_URL: 'redis://localhost:6379', + IVM_LEASE_REDIS_DEADLINE_MS: '5', + }, + spawns: [() => createReadyProc('ok')], + redisEvalImpl: (...args: unknown[]) => { + const script = String(args[0] ?? '') + scripts.push(script) + // Settles only once the test says so, standing in for a script the + // deadline abandoned locally but that Redis still runs to completion. + if (script.includes('ZREMRANGEBYSCORE')) return lateAcquire + return 1 + }, + }) + + const result = await executeInIsolatedVM({ + code: 'return "ok"', + params: {}, + envVars: {}, + contextVariables: {}, + timeoutMs: 100, + requestId: 'req-11', + ownerKey: 'user:redis-late', + }) + completeAcquire(1) + + expect(result.error).toBeUndefined() + expect(scripts.some((script) => script.includes("'ZREM'"))).toBe(true) + }) + + it('ignores a non-positive configured deadline instead of abandoning every lease', async () => { + const { executeInIsolatedVM } = await loadExecutionModule({ + envOverrides: { + IVM_DISTRIBUTED_MAX_INFLIGHT_PER_OWNER: '1', + IVM_LEASE_REDIS_DEADLINE_MS: '-1', + REDIS_URL: 'redis://localhost:6379', + }, + spawns: [() => createReadyProc('ok')], + redisEvalImpl: async (...args: unknown[]) => { + const script = String(args[0] ?? '') + if (script.includes('ZREMRANGEBYSCORE')) { + // Arrives after a non-positive timer would already have fired, so the + // answer only lands in time when the default deadline is restored. + await sleep(25) + return 0 + } + return 1 + }, + }) + + const result = await executeInIsolatedVM({ + code: 'return "ok"', + params: {}, + envVars: {}, + contextVariables: {}, + timeoutMs: 100, + requestId: 'req-12', + ownerKey: 'user:negative-deadline', + }) + + expect(result.error?.message).toContain('Too many concurrent') + }) + it('still rejects when Redis answers that the owner is over its lease limit', async () => { const { executeInIsolatedVM } = await loadExecutionModule({ envOverrides: { diff --git a/apps/sim/lib/execution/isolated-vm.ts b/apps/sim/lib/execution/isolated-vm.ts index 5946ac232b0..4918380f77c 100644 --- a/apps/sim/lib/execution/isolated-vm.ts +++ b/apps/sim/lib/execution/isolated-vm.ts @@ -148,8 +148,14 @@ const DISTRIBUTED_KEY_PREFIX = 'ivm:fair:v1:owner' * actually measure is event-loop scheduling, not Redis. A value near normal loop * latency therefore reports a healthy Redis as unreachable whenever a garbage * collection pause lands on the call. Keep it well clear of that floor. + * + * A non-positive configured value is treated as unconfigured rather than + * honored: a timer of zero or less fires immediately, which would leave every + * acquisition undetermined and silently drop cross-replica enforcement. */ -const LEASE_REDIS_DEADLINE_MS = Number.parseInt(env.IVM_LEASE_REDIS_DEADLINE_MS) || 1000 +const CONFIGURED_LEASE_REDIS_DEADLINE_MS = Number.parseInt(env.IVM_LEASE_REDIS_DEADLINE_MS) +const LEASE_REDIS_DEADLINE_MS = + CONFIGURED_LEASE_REDIS_DEADLINE_MS > 0 ? CONFIGURED_LEASE_REDIS_DEADLINE_MS : 1000 const QUEUE_RETRY_DELAY_MS = 1000 const DISTRIBUTED_LEASE_GRACE_MS = 30000 @@ -1439,12 +1445,17 @@ export async function executeInIsolatedVM( // the per-owner active/queued limits above still bound this work. let settled = false - const holdsDistributedLease = leaseAcquireResult === 'acquired' + /** + * 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 - // Nothing was registered when the lease was undetermined; skip the round trip. - if (!holdsDistributedLease) return releaseDistributedLease(ownerKey, distributedLeaseId).catch((error) => { logger.error('Failed to release distributed lease', { ownerKey, error }) }) From 0c08be4ac93ddadba528be22330905399f775cdb Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 14:30:45 -0700 Subject: [PATCH 3/3] test(execution): stop the lease deadline override leaking between tests - Add `IVM_LEASE_REDIS_DEADLINE_MS` to the harness env reset. It was absent, so a test that overrode it left the value in the module-scoped mock env for every later test in the file, quietly changing their fallback timing. - Drop the duplicate over-limit test and fold its extra assertion into the existing one; the two had identical setup and covered the same path. --- apps/sim/lib/execution/isolated-vm.test.ts | 33 ++-------------------- 1 file changed, 3 insertions(+), 30 deletions(-) diff --git a/apps/sim/lib/execution/isolated-vm.test.ts b/apps/sim/lib/execution/isolated-vm.test.ts index 4e6dedf6869..2d77e438c8f 100644 --- a/apps/sim/lib/execution/isolated-vm.test.ts +++ b/apps/sim/lib/execution/isolated-vm.test.ts @@ -185,6 +185,7 @@ const { mockSpawn, mockExecSync, mockEnv } = vi.hoisted(() => ({ IVM_MAX_OWNER_WEIGHT: '5', IVM_DISTRIBUTED_MAX_INFLIGHT_PER_OWNER: '100', IVM_DISTRIBUTED_LEASE_MIN_TTL_MS: '1000', + IVM_LEASE_REDIS_DEADLINE_MS: '1000', IVM_QUEUE_TIMEOUT_MS: '1000', IVM_MAX_FETCH_RESPONSE_BYTES: '', IVM_MAX_FETCH_RESPONSE_CHARS: '', @@ -246,6 +247,7 @@ async function loadExecutionModule(options: { IVM_MAX_OWNER_WEIGHT: '5', IVM_DISTRIBUTED_MAX_INFLIGHT_PER_OWNER: '100', IVM_DISTRIBUTED_LEASE_MIN_TTL_MS: '1000', + IVM_LEASE_REDIS_DEADLINE_MS: '1000', IVM_QUEUE_TIMEOUT_MS: '1000', IVM_MAX_FETCH_RESPONSE_BYTES: '', IVM_MAX_FETCH_RESPONSE_CHARS: '', @@ -497,6 +499,7 @@ describe('isolated-vm scheduler', () => { }) expect(result.error?.message).toContain('Too many concurrent') + expect(result.result).toBeNull() }) it('falls back to local limits when no Redis client is available', async () => { @@ -651,36 +654,6 @@ describe('isolated-vm scheduler', () => { expect(result.error?.message).toContain('Too many concurrent') }) - it('still rejects when Redis answers that the owner is over its lease limit', async () => { - const { executeInIsolatedVM } = await loadExecutionModule({ - envOverrides: { - IVM_DISTRIBUTED_MAX_INFLIGHT_PER_OWNER: '1', - REDIS_URL: 'redis://localhost:6379', - }, - spawns: [() => createReadyProc('ok')], - redisEvalImpl: (...args: unknown[]) => { - const script = String(args[0] ?? '') - if (script.includes('ZREMRANGEBYSCORE')) { - return 0 - } - return 1 - }, - }) - - const result = await executeInIsolatedVM({ - code: 'return "ok"', - params: {}, - envVars: {}, - contextVariables: {}, - timeoutMs: 100, - requestId: 'req-10', - ownerKey: 'user:over-limit', - }) - - expect(result.error?.message).toContain('Too many concurrent') - expect(result.result).toBeNull() - }) - it('reports cancellation when abort races a rejected distributed lease', async () => { let resolveLease!: (value: number) => void let markLeaseRequested!: () => void