From 8764d019ecb4a503d6be34d8b8234f031b1f77e5 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 13 Sep 2026 04:05:29 -0400 Subject: [PATCH] fix: Exit Cleanly When Native Executor Shuts Down Concurrently (#191) * fix: Exit Cleanly When Native Executor Shuts Down Concurrently A native BYOM worker under systemd KillMode=control-group receives SIGTERM at the same time as its forked SRT executor. The child ignores IPC once it is shutting down, so the parent's close handshake is left pending until the child exits, which rejects it with 'Native executor is unavailable'. That rejection escaped the CLI finally block and turned an idle administrative stop into exit status 1. Treat the close handshake as best-effort: the executor is terminated in finally regardless, and the active command has already drained, so a lost or stalled reply carries no mutation risk. Also make the child report exit status 0 when its own SRT teardown succeeded. Closes #190 * fix: Surface Explicit Executor Cleanup Failures During Close Only a lost, refused, or stalled close handshake is benign at shutdown. A negative close reply from the executor is a real cleanup failure and still rejects so pool shutdown can aggregate it. --- .../code/src/native-process-child.test.ts | 2 +- packages/code/src/native-process-child.ts | 5 +- packages/code/src/native-process.test.ts | 70 +++++++++++++++++++ packages/code/src/native-process.ts | 22 ++++-- 4 files changed, 91 insertions(+), 8 deletions(-) diff --git a/packages/code/src/native-process-child.test.ts b/packages/code/src/native-process-child.test.ts index 1d4866ae..8aca3652 100644 --- a/packages/code/src/native-process-child.test.ts +++ b/packages/code/src/native-process-child.test.ts @@ -33,7 +33,7 @@ for (const signal of ['SIGINT', 'SIGHUP', 'SIGTERM'] as const) { // installation finished without requiring platform SRT dependencies. child.once('message', () => child.kill(signal)); child.send({ id: 'startup-probe', type: 'probe' }); - assert.deepEqual(await exited, { code: 1, signal: null }); + assert.deepEqual(await exited, { code: 0, signal: null }); }, ); } diff --git a/packages/code/src/native-process-child.ts b/packages/code/src/native-process-child.ts index b5221731..05ffedfe 100644 --- a/packages/code/src/native-process-child.ts +++ b/packages/code/src/native-process-child.ts @@ -25,7 +25,10 @@ const shutdown = () => { if (shuttingDown) return; shuttingDown = true; active?.controller.abort(); - void (sandbox?.close() ?? Promise.resolve()).finally(() => process.exit(1)); + void (sandbox?.close() ?? Promise.resolve()).then( + () => process.exit(0), + () => process.exit(1), + ); setTimeout(() => process.exit(1), 5000); }; process.on('disconnect', shutdown); diff --git a/packages/code/src/native-process.test.ts b/packages/code/src/native-process.test.ts index 8dcfd4c1..800dcf11 100644 --- a/packages/code/src/native-process.test.ts +++ b/packages/code/src/native-process.test.ts @@ -301,6 +301,76 @@ test('executor close drains an active command before closing IPC', async () => { await assert.rejects(sandbox.execute(request), /unavailable/); }); +test('executor close resolves when the child exits during the close handshake', async () => { + const fake = fixture(); + const sandbox = new NativeProcessWorkspaceCommandSandbox( + { workspaceRoot: '/workspace' }, + fake.fork, + ); + await sandbox.prepare(); + Object.assign(fake.child, { + send(message: Record, callback: (error: null) => void) { + fake.messages.push(message); + callback(null); + queueMicrotask(() => { + Object.assign(fake.child, { connected: false }); + fake.child.emit('exit', 1, null); + fake.child.emit('disconnect'); + }); + return true; + }, + }); + await sandbox.close(); + assert.equal(fake.messages.filter((m) => m.type === 'close').length, 1); + await assert.rejects(sandbox.execute(request), /unavailable/); +}); + +test('executor close still reports a cleanup failure the child replies with', async () => { + const fake = fixture(); + const sandbox = new NativeProcessWorkspaceCommandSandbox( + { workspaceRoot: '/workspace' }, + fake.fork, + ); + await sandbox.prepare(); + Object.assign(fake.child, { + send(message: Record, callback: (error: null) => void) { + fake.messages.push(message); + callback(null); + queueMicrotask(() => + fake.child.emit('message', { + id: message.id, + ok: false, + code: 'COMMAND_UNAVAILABLE', + errorMessage: 'scratch cleanup failed', + mutation: false, + requiresQuarantine: false, + }), + ); + return true; + }, + }); + await assert.rejects(sandbox.close(), /scratch cleanup failed/); + assert.equal(fake.killCalls, 1); + await assert.rejects(sandbox.execute(request), /unavailable/); +}); + +test('executor close skips the handshake once the child is already lost', async () => { + const fake = fixture(); + const sandbox = new NativeProcessWorkspaceCommandSandbox( + { workspaceRoot: '/workspace' }, + fake.fork, + ); + await sandbox.prepare(); + Object.assign(fake.child, { connected: false }); + fake.child.emit('exit', 1, null); + await sandbox.close(); + assert.equal( + fake.messages.some((m) => m.type === 'close'), + false, + ); + await assert.rejects(sandbox.execute(request), /unavailable/); +}); + test('executor startup loss is not reported as an applied mutation', async () => { const fake = fixture(); const sandbox = new NativeProcessWorkspaceCommandSandbox( diff --git a/packages/code/src/native-process.ts b/packages/code/src/native-process.ts index 96d89355..e1a02498 100644 --- a/packages/code/src/native-process.ts +++ b/packages/code/src/native-process.ts @@ -73,6 +73,15 @@ export function nativeExecutorEnvironment( ); } +/** The executor process was lost, refused a send, or stalled past its + * deadline, as opposed to a failure the executor reported explicitly. */ +class NativeExecutorUnavailableError extends WorkspaceToolError { + constructor(mutation: boolean) { + super('Native executor is unavailable', 'COMMAND_UNAVAILABLE', mutation); + this.name = 'NativeExecutorUnavailableError'; + } +} + /** One persistent, process-isolated SRT manager per workspace. No automatic * restart/replay: losing IPC after execution starts is an ambiguous mutation. */ export class NativeProcessWorkspaceCommandSandbox @@ -109,11 +118,7 @@ export class NativeProcessWorkspaceCommandSandbox } private unavailable(mutation: boolean): WorkspaceToolError { - return new WorkspaceToolError( - 'Native executor is unavailable', - 'COMMAND_UNAVAILABLE', - mutation, - ); + return new NativeExecutorUnavailableError(mutation); } private async start(): Promise { @@ -340,12 +345,17 @@ export class NativeProcessWorkspaceCommandSandbox return this.closing; } + /** An executor that exits, disconnects, or stalls while closing is + * terminated in `finally` regardless, and the active command has already + * drained, so only a failure the executor reports explicitly is surfaced. */ private async stop(): Promise { await this.active?.catch(() => undefined); await this.ready?.catch(() => undefined); try { if (this.child?.connected && !this.failed) - await this.rpc('close', {}, 10_000, false); + await this.rpc('close', {}, 10_000, false).catch((error: unknown) => { + if (!(error instanceof NativeExecutorUnavailableError)) throw error; + }); } finally { this.failed = true; this.terminate();