From 4b3eb16347bf7aee4599f655355cee9d2140addc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A8=8B=E5=BA=8F=E5=91=98=E9=98=BF=E6=B1=9F=28Relakkes?= =?UTF-8?q?=29?= Date: Mon, 1 Jun 2026 02:21:46 +0800 Subject: [PATCH] fix: prevent orphaned desktop CLI sidecars Packaged desktop shutdown was killing the server sidecar with SIGKILL on Unix, which skipped server-side cleanup for CLI subprocesses spawned by active chat sessions. The native layer now gives sidecars a SIGTERM grace window, and the server waits for active CLI sessions to exit before completing signal-driven shutdown. Constraint: Tauri shell CommandChild::kill maps to SIGKILL on Unix. Rejected: Only clean stale processes on next launch | leaves user-owned sessions and sockets in an unknown state after every unclean app exit. Confidence: high Scope-risk: moderate Directive: Do not replace the SIGTERM grace path with direct CommandChild::kill without rechecking packaged-app process-tree shutdown. Tested: bun test src/server/__tests__/conversation-service.test.ts Tested: bun run check:server Tested: bun run check:native Tested: git diff --check Not-tested: Fresh DMG install plus manual quit/reopen process-tree smoke. --- desktop/src-tauri/src/lib.rs | 45 +++++++++++++ .../__tests__/conversation-service.test.ts | 48 ++++++++++++++ src/server/index.ts | 38 ++++++++--- src/server/services/conversationService.ts | 65 ++++++++++++++++--- 4 files changed, 179 insertions(+), 17 deletions(-) diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index df28e10a..88137194 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -231,6 +231,7 @@ const APP_MODE_FILE: &str = "app-mode.json"; const MIN_WINDOW_WIDTH: u32 = 960; const MIN_WINDOW_HEIGHT: u32 = 640; const MIN_VISIBLE_PIXELS: i64 = 64; +const SIDECAR_GRACEFUL_TERMINATION_TIMEOUT: Duration = Duration::from_millis(3_000); #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] #[serde(rename_all = "lowercase")] @@ -1819,9 +1820,53 @@ fn kill_sidecar_child(child: CommandChild) { } } + #[cfg(unix)] + { + terminate_unix_sidecar_child(child); + } + + #[cfg(not(unix))] + { + let _ = child.kill(); + } +} + +#[cfg(unix)] +fn terminate_unix_sidecar_child(child: CommandChild) { + let pid = child.pid(); + let pid_text = pid.to_string(); + + // tauri-plugin-shell's CommandChild::kill() maps to SIGKILL on Unix. + // Give bundled sidecars a SIGTERM window first so the server can stop + // CLI sessions it spawned before the native app exits. + let _ = StdCommand::new("kill") + .args(["-TERM", &pid_text]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status(); + + let deadline = Instant::now() + SIDECAR_GRACEFUL_TERMINATION_TIMEOUT; + while Instant::now() < deadline { + if !is_unix_process_running(pid) { + return; + } + std::thread::sleep(Duration::from_millis(50)); + } + let _ = child.kill(); } +#[cfg(unix)] +fn is_unix_process_running(pid: u32) -> bool { + StdCommand::new("kill") + .args(["-0", &pid.to_string()]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + .map(|status| status.success()) + .unwrap_or(false) +} + #[cfg(unix)] fn kill_stale_unix_adapter_sidecars() { let current_pid = std::process::id(); diff --git a/src/server/__tests__/conversation-service.test.ts b/src/server/__tests__/conversation-service.test.ts index aa438e22..4e25349d 100644 --- a/src/server/__tests__/conversation-service.test.ts +++ b/src/server/__tests__/conversation-service.test.ts @@ -599,6 +599,54 @@ describe('ConversationService', () => { expect(args).toContain('--worktree-base-ref') expect(args).toContain('feature/rail') }) + + test('stopAllSessionsAndWait kills every active CLI subprocess and waits for exits', async () => { + const service = new ConversationService() as any + const killed: string[] = [] + const drained: string[] = [] + + const makeSession = (sessionId: string) => { + let resolveExit: (code: number) => void = () => {} + const exited = new Promise((resolve) => { + resolveExit = resolve + }) + + return { + proc: { + kill: () => { + killed.push(sessionId) + resolveExit(0) + }, + exited, + }, + outputCallbacks: [], + workDir: tmpDir, + permissionMode: 'default', + sdkToken: `${sessionId}-token`, + sdkSocket: null, + pendingOutbound: [], + startupPending: false, + startupExitCode: null, + stdoutLines: [], + stderrLines: [], + outputDrain: Promise.resolve().then(() => { + drained.push(sessionId) + }), + sdkMessages: [], + initMessage: null, + pendingPermissionRequests: new Map(), + } + } + + service.sessions.set('session-a', makeSession('session-a')) + service.sessions.set('session-b', makeSession('session-b')) + + await service.stopAllSessionsAndWait(500) + + expect(killed.sort()).toEqual(['session-a', 'session-b']) + expect(drained.sort()).toEqual(['session-a', 'session-b']) + expect(service.getActiveSessions()).toEqual([]) + }) }) function sanitizeMemoryPath(value: string): string { diff --git a/src/server/index.ts b/src/server/index.ts index 87efd24f..1815a28a 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -454,26 +454,46 @@ export function startServer(port = PORT, host = HOST) { // ─── Graceful shutdown: kill all CLI subprocesses on exit ──────────────────── +let shutdownInProgress: Promise | null = null + function cleanupAllSessions() { const active = conversationService.getActiveSessions() if (active.length > 0) { console.log(`[Server] Shutting down — killing ${active.length} CLI subprocess(es)`) - for (const sessionId of active) { - conversationService.stopSession(sessionId) - } + conversationService.stopAllSessions() } } +async function cleanupAllSessionsAndWait() { + const active = conversationService.getActiveSessions() + if (active.length > 0) { + console.log(`[Server] Shutting down — killing ${active.length} CLI subprocess(es)`) + await conversationService.stopAllSessionsAndWait() + } +} + +function shutdownAndExit(signal: 'SIGTERM' | 'SIGINT', exitCode: number) { + if (shutdownInProgress) return + + shutdownInProgress = (async () => { + console.log(`[Server] Received ${signal}`) + await cleanupAllSessionsAndWait() + process.exit(exitCode) + })().catch((error) => { + console.error( + `[Server] ${signal} shutdown cleanup failed:`, + error instanceof Error ? error.message : error, + ) + process.exit(1) + }) +} + process.on('SIGTERM', () => { - console.log('[Server] Received SIGTERM') - cleanupAllSessions() - process.exit(0) + shutdownAndExit('SIGTERM', 0) }) process.on('SIGINT', () => { - console.log('[Server] Received SIGINT') - cleanupAllSessions() - process.exit(0) + shutdownAndExit('SIGINT', 0) }) process.on('exit', () => { diff --git a/src/server/services/conversationService.ts b/src/server/services/conversationService.ts index 5bccfb50..704a5e90 100644 --- a/src/server/services/conversationService.ts +++ b/src/server/services/conversationService.ts @@ -733,10 +733,10 @@ export class ConversationService { stopSession(sessionId: string): void { const session = this.sessions.get(sessionId) - if (session) { - session.proc.kill() - this.sessions.delete(sessionId) - } + if (!session) return + + this.sessions.delete(sessionId) + this.killProcess(sessionId, session) } async stopSessionAndWait(sessionId: string, timeoutMs = 2_000): Promise { @@ -744,15 +744,64 @@ export class ConversationService { if (!session) return this.sessions.delete(sessionId) - session.proc.kill() + await this.stopProcessAndWait(sessionId, session, timeoutMs) + } - await Promise.race([ - session.proc.exited.catch(() => undefined), - new Promise((resolve) => setTimeout(resolve, timeoutMs)), + stopAllSessions(): void { + for (const sessionId of this.getActiveSessions()) { + this.stopSession(sessionId) + } + } + + async stopAllSessionsAndWait(timeoutMs = 2_000): Promise { + const activeSessions = Array.from(this.sessions.entries()) + if (activeSessions.length === 0) return + + this.sessions.clear() + await Promise.all( + activeSessions.map(([sessionId, session]) => + this.stopProcessAndWait(sessionId, session, timeoutMs), + ), + ) + } + + private async stopProcessAndWait( + sessionId: string, + session: SessionProcess, + timeoutMs: number, + ): Promise { + this.killProcess(sessionId, session, 'SIGTERM') + + const exited = await Promise.race([ + session.proc.exited.then(() => true, () => true), + new Promise((resolve) => setTimeout(() => resolve(false), timeoutMs)), ]) + if (!exited) { + this.killProcess(sessionId, session, 'SIGKILL') + await Promise.race([ + session.proc.exited.catch(() => undefined), + new Promise((resolve) => setTimeout(resolve, 500)), + ]) + } await this.waitForProcessOutputDrain(session, timeoutMs) } + private killProcess( + sessionId: string, + session: SessionProcess, + signal?: NodeJS.Signals, + ): void { + try { + session.proc.kill(signal) + } catch (error) { + console.warn( + `[ConversationService] Failed to kill CLI subprocess for ${sessionId}: ${ + error instanceof Error ? error.message : String(error) + }`, + ) + } + } + markSessionDeleted(sessionId: string): void { this.deletedSessions.add(sessionId) this.stopSession(sessionId)