diff --git a/desktop/electron/main.ts b/desktop/electron/main.ts index 34b41d3e..69b1f365 100644 --- a/desktop/electron/main.ts +++ b/desktop/electron/main.ts @@ -389,5 +389,7 @@ app.on('before-quit', () => { trayController = null terminalService?.killAll() previewService?.close() - getServerRuntime().stopAll() + // Synchronous on quit so the Windows taskkill completes before the process + // exits, otherwise the fire-and-forget kill can leave orphaned sidecars. + getServerRuntime().stopAll(true) }) diff --git a/desktop/electron/services/serverRuntime.ts b/desktop/electron/services/serverRuntime.ts index 5840d36f..a3c1e815 100644 --- a/desktop/electron/services/serverRuntime.ts +++ b/desktop/electron/services/serverRuntime.ts @@ -64,10 +64,10 @@ export class ElectronServerRuntime { await this.startAdaptersSidecars(serverUrl) } - stopAll() { - this.stopAdaptersSidecars() + stopAll(sync = false) { + this.stopAdaptersSidecars(sync) if (this.server) { - killSidecar(this.server.child) + killSidecar(this.server.child, sync) this.server = null } } @@ -125,9 +125,9 @@ export class ElectronServerRuntime { } } - private stopAdaptersSidecars() { + private stopAdaptersSidecars(sync = false) { for (const child of this.adapters.splice(0)) { - killSidecar(child) + killSidecar(child, sync) } } diff --git a/desktop/electron/services/sidecarManager.test.ts b/desktop/electron/services/sidecarManager.test.ts index c2a69c07..d8d7c3c3 100644 --- a/desktop/electron/services/sidecarManager.test.ts +++ b/desktop/electron/services/sidecarManager.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' import path from 'node:path' import { mkdtempSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' @@ -7,12 +7,18 @@ import { createAdapterPlan, createServerPlan, httpToWebSocketUrl, + killSidecar, mergeProxyEnv, proxyUrlFromElectronProxyRules, pushStartupLog, resolveHostTriple, + type SidecarChild, } from './sidecarManager' +function fakeChild(pid = 4321) { + return { pid, kill: vi.fn() } as unknown as SidecarChild & { kill: ReturnType } +} + describe('Electron sidecar manager', () => { it('maps host platform to existing sidecar target triples', () => { expect(resolveHostTriple('darwin', 'arm64')).toBe('aarch64-apple-darwin') @@ -113,4 +119,33 @@ describe('Electron sidecar manager', () => { expect(httpToWebSocketUrl('http://127.0.0.1:3456')).toBe('ws://127.0.0.1:3456') expect(httpToWebSocketUrl('https://example.com')).toBe('wss://example.com') }) + + it('kills non-Windows sidecars with a signal', () => { + const child = fakeChild() + const spawnAsync = vi.fn() + const spawnSyncFn = vi.fn() + killSidecar(child, false, { platform: 'darwin', spawnAsync: spawnAsync as never, spawnSyncFn: spawnSyncFn as never }) + expect(child.kill).toHaveBeenCalledTimes(1) + expect(spawnAsync).not.toHaveBeenCalled() + expect(spawnSyncFn).not.toHaveBeenCalled() + }) + + it('uses async taskkill on Windows by default', () => { + const child = fakeChild(777) + const spawnAsync = vi.fn() + const spawnSyncFn = vi.fn() + killSidecar(child, false, { platform: 'win32', spawnAsync: spawnAsync as never, spawnSyncFn: spawnSyncFn as never }) + expect(spawnAsync).toHaveBeenCalledWith('taskkill', ['/F', '/T', '/PID', '777'], { stdio: 'ignore' }) + expect(spawnSyncFn).not.toHaveBeenCalled() + expect(child.kill).not.toHaveBeenCalled() + }) + + it('uses synchronous taskkill on Windows during shutdown to avoid orphaned sidecars', () => { + const child = fakeChild(777) + const spawnAsync = vi.fn() + const spawnSyncFn = vi.fn() + killSidecar(child, true, { platform: 'win32', spawnAsync: spawnAsync as never, spawnSyncFn: spawnSyncFn as never }) + expect(spawnSyncFn).toHaveBeenCalledWith('taskkill', ['/F', '/T', '/PID', '777'], { stdio: 'ignore' }) + expect(spawnAsync).not.toHaveBeenCalled() + }) }) diff --git a/desktop/electron/services/sidecarManager.ts b/desktop/electron/services/sidecarManager.ts index 55ad51a5..e78e63ab 100644 --- a/desktop/electron/services/sidecarManager.ts +++ b/desktop/electron/services/sidecarManager.ts @@ -1,4 +1,4 @@ -import { spawn, type ChildProcessByStdio } from 'node:child_process' +import { spawn, spawnSync, type ChildProcessByStdio } from 'node:child_process' import { mkdirSync, existsSync } from 'node:fs' import type { Readable } from 'node:stream' import net from 'node:net' @@ -213,9 +213,24 @@ export function spawnSidecar(plan: SidecarPlan): SidecarChild { }) } -export function killSidecar(child: SidecarChild) { - if (process.platform === 'win32' && child.pid) { - spawn('taskkill', ['/F', '/T', '/PID', String(child.pid)], { stdio: 'ignore' }) +export type KillSidecarDeps = { + platform?: NodeJS.Platform + spawnAsync?: typeof spawn + spawnSyncFn?: typeof spawnSync +} + +/** + * Terminate a sidecar process. On Windows we shell out to `taskkill /T` to also + * reap the child process tree (the Bun sidecar spawns workers). Pass `sync=true` + * during app shutdown so the kill completes before the process exits — otherwise + * the async `taskkill` is fire-and-forget and can leave orphaned processes. + */ +export function killSidecar(child: SidecarChild, sync = false, deps: KillSidecarDeps = {}) { + const platform = deps.platform ?? process.platform + if (platform === 'win32' && child.pid) { + const args = ['/F', '/T', '/PID', String(child.pid)] + if (sync) (deps.spawnSyncFn ?? spawnSync)('taskkill', args, { stdio: 'ignore' }) + else (deps.spawnAsync ?? spawn)('taskkill', args, { stdio: 'ignore' }) return } child.kill()