From 312ca0bf17d697389e45a67c75fe728108c7e791 Mon Sep 17 00:00:00 2001 From: David Kaya Date: Sun, 22 Mar 2026 16:44:40 +0100 Subject: [PATCH] fix: scope sidecar exit handling to the active process Wait for an intentional sidecar dispose to exit before refreshing capabilities, and ignore exit events from stale sidecar processes. This prevents a refresh from killing the old process and then having that old exit reject the new capabilities request with an unexpected sidecar exit error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/main/sidecar/sidecarProcess.ts | 37 +++++++++++++++++++++--------- src/main/sidecar/sidecarRefresh.ts | 7 ++++++ tests/main/sidecarRefresh.test.ts | 20 +++++++++++++++- 3 files changed, 52 insertions(+), 12 deletions(-) diff --git a/src/main/sidecar/sidecarProcess.ts b/src/main/sidecar/sidecarProcess.ts index bf0f745..a6f2c6e 100644 --- a/src/main/sidecar/sidecarProcess.ts +++ b/src/main/sidecar/sidecarProcess.ts @@ -1,4 +1,5 @@ import { app } from 'electron'; +import { once } from 'node:events'; import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process'; import type { @@ -12,7 +13,10 @@ import type { } from '@shared/contracts/sidecar'; import type { ChatMessageRecord } from '@shared/domain/session'; import { createSidecarEnvironment } from '@main/sidecar/sidecarEnvironment'; -import { shouldRestartSidecarOnCapabilityRefresh } from '@main/sidecar/sidecarRefresh'; +import { + shouldHandleSidecarExit, + shouldRestartSidecarOnCapabilityRefresh, +} from '@main/sidecar/sidecarRefresh'; import { markRunTurnPendingErrored, shouldHandleRunTurnEvent, @@ -72,12 +76,18 @@ export class SidecarClient { } async dispose(): Promise { - if (!this.process) { + const sidecar = this.process; + if (!sidecar) { return; } - this.process.kill(); - this.process = undefined; + if (sidecar.exitCode !== null || sidecar.signalCode !== null) { + return; + } + + const exitPromise = once(sidecar, 'exit'); + sidecar.kill(); + await exitPromise; } hasActiveRunTurn(): boolean { @@ -101,25 +111,30 @@ export class SidecarClient { resourcesPath: process.resourcesPath, platform: process.platform, }); - this.process = spawn(sidecar.command, sidecar.args, { + const childProcess = spawn(sidecar.command, sidecar.args, { cwd: sidecar.cwd, env: createSidecarEnvironment(process.env), stdio: 'pipe', windowsHide: true, }); + this.process = childProcess; - this.process.stdout.setEncoding('utf8'); - this.process.stdout.on('data', (chunk: string) => { + childProcess.stdout.setEncoding('utf8'); + childProcess.stdout.on('data', (chunk: string) => { this.stdoutBuffer += chunk; this.flushStdoutBuffer(); }); - this.process.stderr.setEncoding('utf8'); - this.process.stderr.on('data', (chunk: string) => { + childProcess.stderr.setEncoding('utf8'); + childProcess.stderr.on('data', (chunk: string) => { console.error('[kopaya sidecar]', chunk.trim()); }); - this.process.on('exit', (code) => { + childProcess.on('exit', (code) => { + if (!shouldHandleSidecarExit(this.process?.pid, childProcess.pid)) { + return; + } + const error = new Error(`The .NET sidecar exited unexpectedly with code ${code ?? 'unknown'}.`); for (const pending of this.pending.values()) { pending.reject(error); @@ -129,7 +144,7 @@ export class SidecarClient { this.stdoutBuffer = ''; }); - return this.process; + return childProcess; } private async dispatch( diff --git a/src/main/sidecar/sidecarRefresh.ts b/src/main/sidecar/sidecarRefresh.ts index 0ddd9b2..aba1b69 100644 --- a/src/main/sidecar/sidecarRefresh.ts +++ b/src/main/sidecar/sidecarRefresh.ts @@ -1,3 +1,10 @@ export function shouldRestartSidecarOnCapabilityRefresh(hasActiveRunTurn: boolean): boolean { return !hasActiveRunTurn; } + +export function shouldHandleSidecarExit( + activeProcessId: number | undefined, + exitingProcessId: number | undefined, +): boolean { + return activeProcessId !== undefined && exitingProcessId !== undefined && activeProcessId === exitingProcessId; +} diff --git a/tests/main/sidecarRefresh.test.ts b/tests/main/sidecarRefresh.test.ts index 15b0e68..f26698f 100644 --- a/tests/main/sidecarRefresh.test.ts +++ b/tests/main/sidecarRefresh.test.ts @@ -1,6 +1,9 @@ import { describe, expect, test } from 'bun:test'; -import { shouldRestartSidecarOnCapabilityRefresh } from '@main/sidecar/sidecarRefresh'; +import { + shouldHandleSidecarExit, + shouldRestartSidecarOnCapabilityRefresh, +} from '@main/sidecar/sidecarRefresh'; describe('shouldRestartSidecarOnCapabilityRefresh', () => { test('restarts the sidecar when no run-turn is active', () => { @@ -11,3 +14,18 @@ describe('shouldRestartSidecarOnCapabilityRefresh', () => { expect(shouldRestartSidecarOnCapabilityRefresh(true)).toBe(false); }); }); + +describe('shouldHandleSidecarExit', () => { + test('handles exit events from the currently active sidecar process', () => { + expect(shouldHandleSidecarExit(1234, 1234)).toBe(true); + }); + + test('ignores exit events from a stale sidecar process', () => { + expect(shouldHandleSidecarExit(1234, 5678)).toBe(false); + }); + + test('ignores exit events when either process id is missing', () => { + expect(shouldHandleSidecarExit(undefined, 5678)).toBe(false); + expect(shouldHandleSidecarExit(1234, undefined)).toBe(false); + }); +});