Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/code/src/native-process-child.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
},
);
}
5 changes: 4 additions & 1 deletion packages/code/src/native-process-child.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
70 changes: 70 additions & 0 deletions packages/code/src/native-process.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, any>, 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<string, any>, 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(
Expand Down
22 changes: 16 additions & 6 deletions packages/code/src/native-process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<void> {
Expand Down Expand Up @@ -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<void> {
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();
Expand Down