Skip to content

Commit 5a98a62

Browse files
authored
fix(isolated-vm): drain worker host deploys without failing in-flight executions (#9389)
* fix(isolated-vm): drain worker host deploys without failing in-flight executions - Retire each remote worker connection at a max age (IVM_REMOTE_WORKER_MAX_AGE_MS, default 2 min) so a host the load balancer deregistered stops getting new executions and drains before its deregistration delay cuts the connections - Re-dispatch an execution whose connection closed before it was sent; one that may have reached the host is never re-run and fails with a clear error - Cap raw bytes a host connection may send before authenticating (16 KiB) - Return a cancelled call before reading the worker-target flag * fix(isolated-vm): address review on host drain - Keep counting a refused connection's bytes until its socket closes - Skip a redispatch the caller cancelled in between - Look up the owner's state after the admission waits - Parse IVM_REMOTE_WORKER_MAX_AGE_MS strictly, falling back below 1s - Hold the inspected execution open until the worker check finishes * test(isolated-vm): report an execution that fails before it is held
1 parent 06a5b91 commit 5a98a62

6 files changed

Lines changed: 509 additions & 83 deletions

File tree

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -459,6 +459,7 @@ export const env = createEnv({
459459
IVM_MAX_BROKERS_PER_EXECUTION: z.string().optional().default('1000'), // Max broker calls per sandbox task execution
460460
IVM_REMOTE_WORKER_URL: z.string().optional(), // ws(s)://<host>/v1/worker of a dedicated worker host; workers run there unless the isolated-vm-local-workers kill switch is on
461461
IVM_REMOTE_WORKER_TOKEN: z.string().optional(), // Token the worker host expects (its IVM_HOST_TOKEN, >= 32 chars); required with IVM_REMOTE_WORKER_URL
462+
IVM_REMOTE_WORKER_MAX_AGE_MS: z.coerce.number().int().positive().optional(), // (default 120000) Age at which a worker host connection stops taking executions, so host deploys drain; keep below the host's deregistration delay
462463

463464
// Knowledge Base Processing Configuration - Shared across all processing methods
464465
KB_CONFIG_MAX_DURATION: z.number().optional().default(600), // Max processing duration in seconds (10 minutes)

‎apps/sim/lib/execution/isolated-vm-host.cjs‎

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,10 @@
1414
* are the objects the worker
1515
* exchanges over IPC. Host-originated frames are `{ type: 'host:heartbeat' }`
1616
* every 30s and `{ type: 'host:draining' }` on SIGTERM, after which the app
17-
* retires the connection once its executions finish. A breaking change to the
17+
* retires the connection once its executions finish. A load balancer that
18+
* deregisters this host sends nothing until SIGTERM, so the app also retires
19+
* each connection at a maximum age, and a deregistered host drains as its
20+
* connections age out. A breaking change to the
1821
* contract gets a new path served alongside this one until every client has
1922
* rolled.
2023
*
@@ -36,6 +39,12 @@ const { WebSocketServer } = require('ws')
3639
const WORKER_PATH = '/v1/worker'
3740
const HEARTBEAT_INTERVAL_MS = 30000
3841
const AUTH_TIMEOUT_MS = 5000
42+
/**
43+
* Raw bytes a connection may send before it authenticates: the auth frame is
44+
* well under 1 KiB. Without this, `maxPayload` lets every unauthenticated
45+
* connection make the host buffer a full-size frame.
46+
*/
47+
const MAX_PRE_AUTH_BYTES = 16 * 1024
3948
const WORKER_START_TIMEOUT_MS = 10000
4049
const MAX_STARTUP_STDERR_CHARS = 64 * 1024
4150
const MIN_TOKEN_LENGTH = 32
@@ -179,13 +188,28 @@ server.on('upgrade', (req, socket, head) => {
179188
rejectUpgrade(socket, 503, 'Service Unavailable')
180189
return
181190
}
182-
wss.handleUpgrade(req, socket, head, (ws) => authenticate(ws))
191+
wss.handleUpgrade(req, socket, head, (ws) => authenticate(ws, socket))
183192
})
184193

185194
/** Admits the connection to a worker slot only once its first frame proves it holds the token. */
186-
function authenticate(ws) {
195+
function authenticate(ws, socket) {
187196
pendingConnections++
188197
let settled = false
198+
let authenticated = false
199+
let preAuthBytes = 0
200+
// Counts what arrives on the TCP socket, so an oversized frame is cut off as it streams in
201+
// rather than after ws has buffered all of it.
202+
const onPreAuthData = (chunk) => {
203+
// ws parses each chunk before this listener runs, so the chunk that authenticated still arrives.
204+
// A refused connection stays counted: its peer may ignore the close and keep sending.
205+
if (authenticated) return
206+
preAuthBytes += chunk.length
207+
if (preAuthBytes <= MAX_PRE_AUTH_BYTES) return
208+
if (settle()) {
209+
log('warn', 'Rejected worker connection', { reason: 'too much data before authentication' })
210+
}
211+
socket.destroy()
212+
}
189213
const settle = () => {
190214
if (settled) return false
191215
settled = true
@@ -199,6 +223,7 @@ function authenticate(ws) {
199223
ws.close(UNAUTHORIZED_CLOSE_CODE, 'unauthorized')
200224
}
201225
const authTimeout = setTimeout(() => refuse('authentication timed out'), AUTH_TIMEOUT_MS)
226+
socket.on('data', onPreAuthData)
202227
const onPreAuthError = (error) => {
203228
log('warn', 'Worker connection error before authentication', { error: error.message })
204229
}
@@ -214,6 +239,8 @@ function authenticate(ws) {
214239
return
215240
}
216241
if (!settle()) return
242+
authenticated = true
243+
socket.off('data', onPreAuthData)
217244
if (draining) {
218245
ws.close(1012, 'draining')
219246
return

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

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,17 @@ const HOST_SILENCE_LIMIT_MS = 75_000
99
const REMOTE_WORKER_URL_PATTERN = /^wss?:\/\/[^/]+\/v1\/worker$/
1010
const MIN_REMOTE_WORKER_TOKEN_LENGTH = 32
1111

12+
/**
13+
* Thrown by a remote worker's `send` once its connection is closing: nothing was
14+
* delivered, so the message never reached the host.
15+
*/
16+
export class WorkerConnectionClosedError extends Error {
17+
constructor() {
18+
super('Remote isolated-vm worker connection is closed')
19+
this.name = 'WorkerConnectionClosedError'
20+
}
21+
}
22+
1223
/**
1324
* The slice of `ChildProcess` the isolated-vm pool uses, so a local child and a
1425
* connection to a remote worker host are interchangeable.
@@ -113,9 +124,12 @@ export function connectRemoteWorker(options: RemoteWorkerOptions): IsolatedVMWor
113124
})
114125

115126
return {
116-
/** Returns false once the connection is closing; the pending `exit` settles this worker's executions. */
127+
/**
128+
* Throws {@link WorkerConnectionClosedError} once the connection is closing, so the pool can
129+
* tell a message that never left this process from one the host may have acted on.
130+
*/
117131
send(message) {
118-
if (socket.readyState !== WebSocket.OPEN) return false
132+
if (socket.readyState !== WebSocket.OPEN) throw new WorkerConnectionClosedError()
119133
socket.send(JSON.stringify(message))
120134
return true
121135
},

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

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,47 @@ class UnreachableWebSocket extends EventTarget {
275275
close() {}
276276
}
277277

278+
/**
279+
* A worker host that accepts every connection and answers each execution. A test closes one by
280+
* setting `readyState` to CLOSING, the state a connection is in before its close event arrives.
281+
*/
282+
class ReachableWebSocket extends EventTarget {
283+
static readonly OPEN = 1
284+
static readonly instances: ReachableWebSocket[] = []
285+
readyState = 0
286+
readonly executionIds: number[] = []
287+
288+
constructor() {
289+
super()
290+
ReachableWebSocket.instances.push(this)
291+
setImmediate(() => {
292+
this.readyState = 1
293+
this.dispatchEvent(new Event('open'))
294+
})
295+
}
296+
297+
send(data: string) {
298+
const message = JSON.parse(data) as { type: string; executionId?: number }
299+
if (message.type === 'auth') this.reply({ type: 'ready' })
300+
if (message.type === 'execute' && message.executionId !== undefined) {
301+
this.executionIds.push(message.executionId)
302+
this.reply({
303+
type: 'result',
304+
executionId: message.executionId,
305+
result: { result: 'ran remotely', stdout: '' },
306+
})
307+
}
308+
}
309+
310+
close() {}
311+
312+
private reply(message: unknown) {
313+
setImmediate(() =>
314+
this.dispatchEvent(new MessageEvent('message', { data: JSON.stringify(message) }))
315+
)
316+
}
317+
}
318+
278319
describe('isolated-vm scheduler', () => {
279320
it('recovers from an initial spawn failure and drains queued work', async () => {
280321
const { executeInIsolatedVM, spawnMock } = await loadExecutionModule({
@@ -416,6 +457,69 @@ describe('isolated-vm scheduler', () => {
416457
}
417458
})
418459

460+
it('returns a cancelled call without waiting on the worker target flag', async () => {
461+
featureFlagsMockFns.mockIsFeatureEnabled.mockImplementation(() => new Promise(() => {}))
462+
try {
463+
const { executeInIsolatedVM } = await loadExecutionModule({
464+
envOverrides: {
465+
IVM_REMOTE_WORKER_URL: 'ws://worker-host.internal:3100/v1/worker',
466+
IVM_REMOTE_WORKER_TOKEN: 'remote-worker-token-of-at-least-32-chars',
467+
},
468+
spawns: [],
469+
})
470+
const controller = new AbortController()
471+
controller.abort()
472+
473+
const result = await executeInIsolatedVM(
474+
{
475+
code: 'return 1',
476+
params: {},
477+
envVars: {},
478+
contextVariables: {},
479+
timeoutMs: 100,
480+
requestId: 'req-pre-aborted',
481+
},
482+
{ signal: controller.signal }
483+
)
484+
485+
expect(result.termination).toBe('cancelled')
486+
} finally {
487+
featureFlagsMockFns.mockIsFeatureEnabled.mockReset().mockResolvedValue(false)
488+
}
489+
})
490+
491+
it('moves an execution to a new worker when its connection closed before it was sent', async () => {
492+
ReachableWebSocket.instances.length = 0
493+
vi.stubGlobal('WebSocket', ReachableWebSocket)
494+
const { executeInIsolatedVM } = await loadExecutionModule({
495+
envOverrides: {
496+
IVM_REMOTE_WORKER_URL: 'ws://worker-host.internal:3100/v1/worker',
497+
IVM_REMOTE_WORKER_TOKEN: 'remote-worker-token-of-at-least-32-chars',
498+
},
499+
spawns: [],
500+
})
501+
const request = {
502+
code: 'return 1',
503+
params: {},
504+
envVars: {},
505+
contextVariables: {},
506+
timeoutMs: 100,
507+
}
508+
509+
const warm = await executeInIsolatedVM({ ...request, requestId: 'req-warm' })
510+
expect(warm.result).toBe('ran remotely')
511+
const [closing] = ReachableWebSocket.instances
512+
if (!closing) throw new Error('Expected the first worker connection')
513+
closing.readyState = 2
514+
515+
const result = await executeInIsolatedVM({ ...request, requestId: 'req-after-close' })
516+
517+
expect(result.error).toBeUndefined()
518+
expect(result.result).toBe('ran remotely')
519+
expect(closing.executionIds).toHaveLength(1)
520+
expect(ReachableWebSocket.instances).toHaveLength(2)
521+
})
522+
419523
it('rejects new requests when the queue is full', async () => {
420524
const holder = createControllableReadyProc()
421525
const { executeInIsolatedVM } = await loadExecutionModule({

0 commit comments

Comments
 (0)