Skip to content

Commit 2018225

Browse files
committed
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.
1 parent 2bda859 commit 2018225

3 files changed

Lines changed: 96 additions & 31 deletions

File tree

apps/sim/lib/core/config/env.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,7 @@ export const env = createEnv({
432432
IVM_MAX_OWNER_WEIGHT: z.string().optional().default('5'), // Max accepted weight for weighted owner scheduling
433433
IVM_DISTRIBUTED_MAX_INFLIGHT_PER_OWNER:z.string().optional().default('2200'), // Max owner in-flight leases across replicas
434434
IVM_DISTRIBUTED_LEASE_MIN_TTL_MS: z.string().optional().default('120000'), // Min TTL for distributed in-flight leases (ms)
435+
IVM_LEASE_REDIS_DEADLINE_MS: z.string().optional().default('1000'), // Deadline for one distributed lease round trip (ms)
435436
IVM_QUEUE_TIMEOUT_MS: z.string().optional().default('300000'), // Max queue wait before rejection (ms)
436437
IVM_MAX_EXECUTIONS_PER_WORKER: z.string().optional().default('200'), // Max lifetime executions before worker is recycled
437438
IVM_MAX_BROKER_ARGS_JSON_CHARS: z.string().optional().default('262144'), // Max JSON payload size for sandbox task broker args (isolate→host)

apps/sim/lib/execution/isolated-vm.test.ts

Lines changed: 65 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -498,7 +498,7 @@ describe('isolated-vm scheduler', () => {
498498
expect(result.error?.message).toContain('Too many concurrent')
499499
})
500500

501-
it('fails closed when Redis is configured but unavailable', async () => {
501+
it('falls back to local limits when no Redis client is available', async () => {
502502
const { executeInIsolatedVM } = await loadExecutionModule({
503503
envOverrides: {
504504
REDIS_URL: 'redis://localhost:6379',
@@ -516,14 +516,11 @@ describe('isolated-vm scheduler', () => {
516516
ownerKey: 'user:redis-down',
517517
})
518518

519-
expect(result.error).toMatchObject({
520-
isSystemError: true,
521-
message: 'Code execution coordination is temporarily unavailable. Please try again later.',
522-
})
523-
expect(result.result).toBeNull()
519+
expect(result.error).toBeUndefined()
520+
expect(result.result).toBe('ok')
524521
})
525522

526-
it('fails closed when Redis lease evaluation errors', async () => {
523+
it('falls back to local limits when the lease evaluation errors', async () => {
527524
const { executeInIsolatedVM } = await loadExecutionModule({
528525
envOverrides: {
529526
REDIS_URL: 'redis://localhost:6379',
@@ -548,10 +545,68 @@ describe('isolated-vm scheduler', () => {
548545
ownerKey: 'user:redis-error',
549546
})
550547

551-
expect(result.error).toMatchObject({
552-
isSystemError: true,
553-
message: 'Code execution coordination is temporarily unavailable. Please try again later.',
548+
expect(result.error).toBeUndefined()
549+
expect(result.result).toBe('ok')
550+
})
551+
552+
it('falls back to local limits when the lease round trip exceeds its deadline', async () => {
553+
const { executeInIsolatedVM } = await loadExecutionModule({
554+
envOverrides: {
555+
REDIS_URL: 'redis://localhost:6379',
556+
IVM_LEASE_REDIS_DEADLINE_MS: '5',
557+
},
558+
spawns: [() => createReadyProc('ok')],
559+
redisEvalImpl: (...args: unknown[]) => {
560+
const script = String(args[0] ?? '')
561+
// Never settles, so only the deadline can decide the outcome.
562+
if (script.includes('ZREMRANGEBYSCORE')) {
563+
return new Promise<number>(() => {})
564+
}
565+
return 1
566+
},
567+
})
568+
569+
const result = await executeInIsolatedVM({
570+
code: 'return "ok"',
571+
params: {},
572+
envVars: {},
573+
contextVariables: {},
574+
timeoutMs: 100,
575+
requestId: 'req-9',
576+
ownerKey: 'user:redis-slow',
577+
})
578+
579+
expect(result.error).toBeUndefined()
580+
expect(result.result).toBe('ok')
581+
})
582+
583+
it('still rejects when Redis answers that the owner is over its lease limit', async () => {
584+
const { executeInIsolatedVM } = await loadExecutionModule({
585+
envOverrides: {
586+
IVM_DISTRIBUTED_MAX_INFLIGHT_PER_OWNER: '1',
587+
REDIS_URL: 'redis://localhost:6379',
588+
},
589+
spawns: [() => createReadyProc('ok')],
590+
redisEvalImpl: (...args: unknown[]) => {
591+
const script = String(args[0] ?? '')
592+
if (script.includes('ZREMRANGEBYSCORE')) {
593+
return 0
594+
}
595+
return 1
596+
},
597+
})
598+
599+
const result = await executeInIsolatedVM({
600+
code: 'return "ok"',
601+
params: {},
602+
envVars: {},
603+
contextVariables: {},
604+
timeoutMs: 100,
605+
requestId: 'req-10',
606+
ownerKey: 'user:over-limit',
554607
})
608+
609+
expect(result.error?.message).toContain('Too many concurrent')
555610
expect(result.result).toBeNull()
556611
})
557612

apps/sim/lib/execution/isolated-vm.ts

Lines changed: 30 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,16 @@ const MAX_EXECUTIONS_PER_WORKER = Number.parseInt(env.IVM_MAX_EXECUTIONS_PER_WOR
140140
const MAX_BROKER_ARGS_JSON_CHARS = Number.parseInt(env.IVM_MAX_BROKER_ARGS_JSON_CHARS) || 262_144
141141
const MAX_BROKERS_PER_EXECUTION = Number.parseInt(env.IVM_MAX_BROKERS_PER_EXECUTION) || 1000
142142
const DISTRIBUTED_KEY_PREFIX = 'ivm:fair:v1:owner'
143-
const LEASE_REDIS_DEADLINE_MS = 200
143+
/**
144+
* Deadline for a single lease round trip, kept below the shared Redis client's
145+
* `commandTimeout` so this race still resolves first.
146+
*
147+
* Both this deadline and `commandTimeout` are plain `setTimeout`s, so what they
148+
* actually measure is event-loop scheduling, not Redis. A value near normal loop
149+
* latency therefore reports a healthy Redis as unreachable whenever a garbage
150+
* collection pause lands on the call. Keep it well clear of that floor.
151+
*/
152+
const LEASE_REDIS_DEADLINE_MS = Number.parseInt(env.IVM_LEASE_REDIS_DEADLINE_MS) || 1000
144153
const QUEUE_RETRY_DELAY_MS = 1000
145154
const DISTRIBUTED_LEASE_GRACE_MS = 30000
146155

@@ -347,7 +356,16 @@ function ownerRedisKey(ownerKey: string): string {
347356
return `${DISTRIBUTED_KEY_PREFIX}:${ownerKey}`
348357
}
349358

350-
type LeaseAcquireResult = 'acquired' | 'limit_exceeded' | 'unavailable'
359+
/**
360+
* Outcome of one distributed lease acquisition.
361+
*
362+
* `limit_exceeded` is an answer from Redis — the owner is genuinely over its
363+
* share — and is the only outcome that denies an execution. `undetermined`
364+
* means no answer arrived before the deadline, which is not a denial and must
365+
* never be projected as one: the local admission limits below still bound the
366+
* work, so the caller falls back to them.
367+
*/
368+
type LeaseAcquireResult = 'acquired' | 'limit_exceeded' | 'undetermined'
351369

352370
async function tryAcquireDistributedLease(
353371
ownerKey: string,
@@ -358,10 +376,10 @@ async function tryAcquireDistributedLease(
358376

359377
const redis = getRedisClient()
360378
if (!redis) {
361-
logger.error('Redis is configured but unavailable for distributed lease acquisition', {
379+
logger.warn('No Redis client for distributed lease acquisition; using local limits', {
362380
ownerKey,
363381
})
364-
return 'unavailable'
382+
return 'undetermined'
365383
}
366384

367385
const now = Date.now()
@@ -407,11 +425,12 @@ async function tryAcquireDistributedLease(
407425
])
408426
return Number(result) === 1 ? 'acquired' : 'limit_exceeded'
409427
} catch (error) {
410-
logger.error('Failed to acquire distributed owner lease; execution will be rejected', {
428+
logger.warn('Distributed owner lease undetermined; using local limits', {
411429
ownerKey,
430+
deadlineMs: LEASE_REDIS_DEADLINE_MS,
412431
error,
413432
})
414-
return 'unavailable'
433+
return 'undetermined'
415434
} finally {
416435
clearTimeout(deadlineTimer)
417436
}
@@ -1416,26 +1435,16 @@ export async function executeInIsolatedVM(
14161435
},
14171436
}
14181437
}
1419-
if (leaseAcquireResult === 'unavailable') {
1420-
logger.error('Isolated-vm execution rejected because its distributed lease is unavailable', {
1421-
ownerKey,
1422-
})
1423-
maybeCleanupOwner(ownerKey)
1424-
return {
1425-
result: null,
1426-
stdout: '',
1427-
error: {
1428-
message: 'Code execution coordination is temporarily unavailable. Please try again later.',
1429-
name: 'Error',
1430-
isSystemError: true,
1431-
},
1432-
}
1433-
}
1438+
// An undetermined lease cannot reject the execution: the per-process pool and
1439+
// the per-owner active/queued limits above still bound this work.
14341440

14351441
let settled = false
1442+
const holdsDistributedLease = leaseAcquireResult === 'acquired'
14361443
const releaseLease = () => {
14371444
if (settled) return
14381445
settled = true
1446+
// Nothing was registered when the lease was undetermined; skip the round trip.
1447+
if (!holdsDistributedLease) return
14391448
releaseDistributedLease(ownerKey, distributedLeaseId).catch((error) => {
14401449
logger.error('Failed to release distributed lease', { ownerKey, error })
14411450
})

0 commit comments

Comments
 (0)