fix: bound Windows ACP shutdown and retain failed cleanup

apps/cli/src/agent/AGENTS.md

@@ -81,8 +81,11 @@ arrive: context/message-flow.md "Upstream".
   answer before giving up on the upstream turn's response: the Codex adapter drains
   session notifications before refusing, so the turn's response routinely wins that
   race and would otherwise mask the refusal.
-- `acp-runner.ts` — process spawn/restart around the client. Spawn + initialize +
-  `newSession`/`loadSession` share `acp-session-start-gate.ts` (default 2,
+- `acp-runner.ts` — process spawn/restart around the client.
+  Auxiliary ACP shutdown shares one termination attempt per owned child, uses the
+  Windows process-tree cleanup helper, and reports termination failure; protocol
+  session-close failure must still proceed to process cleanup.
+  Spawn + initialize + `newSession`/`loadSession` share `acp-session-start-gate.ts` (default 2,
   `LODY_MAX_CONCURRENT_ACP_SESSION_STARTS`). Unbounded concurrent Codex starts
   each spawn a lody.exe adapter, a Codex app-server, and a lody.exe MCP child;
   they contend on `~/.codex` and freeze every in-flight session until Lody

apps/cli/src/agent/acp-authentication.test.ts

@@ -10,6 +10,12 @@ import type { Logger } from '@/utils/logger';
 import { createStdinWritableStream, createStdoutReadableStream } from '@/utils/stream';
 import { AcpAuthenticationManager, probeBuiltinAuthentication } from './acp-authentication';
 
+vi.mock('@/utils/windows-process-tree', () => ({
+  terminateWindowsProcessTree: async (child: ChildProcess) => {
+    if (child.exitCode == null && child.signalCode == null) child.kill('SIGKILL');
+  },
+}));
+
 const createSilentLogger = (): Logger => ({
   info: () => {},
   warn: () => {},
@@ -316,8 +322,12 @@ describe('AcpAuthenticationManager', () => {
       disposition: 'error',
       error: 'Kimi Code authentication timed out. Please try again.',
     });
-    expect(stuckChild.kill).toHaveBeenNthCalledWith(1, 'SIGTERM');
-    expect(stuckChild.kill).toHaveBeenNthCalledWith(2, 'SIGKILL');
+    if (process.platform === 'win32') {
+      expect(stuckChild.kill).toHaveBeenCalledWith('SIGKILL');
+    } else {
+      expect(stuckChild.kill).toHaveBeenNthCalledWith(1, 'SIGTERM');
+      expect(stuckChild.kill).toHaveBeenNthCalledWith(2, 'SIGKILL');
+    }
 
     await expect(manager.authenticate({ requestId: 'auth-2', ...input })).resolves.toEqual({
       success: true,
@@ -531,7 +541,7 @@ describe('AcpAuthenticationManager', () => {
       success: true,
       disposition: 'cancelled',
     });
-    expect(child.kill).toHaveBeenCalledWith('SIGTERM');
+    expect(child.kill).toHaveBeenCalledWith(process.platform === 'win32' ? 'SIGKILL' : 'SIGTERM');
   });
 
   it('bridges ACP URL consent without retaining authentication process output', async () => {

apps/cli/src/agent/acp-runner.test.ts

@@ -3,7 +3,11 @@ import type { ChildProcess } from 'child_process';
 import fs from 'node:fs';
 import os from 'node:os';
 import path from 'node:path';
-import { afterEach, describe, expect, it, vi } from 'vitest';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+
+const treeCleanup = vi.hoisted(() => vi.fn<() => Promise<void>>());
+vi.mock('@/utils/windows-process-tree', () => ({ terminateWindowsProcessTree: treeCleanup }));
+const nativePlatform = process.platform;
 
 import { __test__, shutdownLocalAcpAgent, spawnAcpProcess } from './acp-runner';
 import type { Logger } from '@/utils/logger';
@@ -136,7 +140,12 @@ base_url = "https://gateway.example/v1"
 });
 
 describe('shutdownLocalAcpAgent', () => {
+  beforeEach(() => {
+    Object.defineProperty(process, 'platform', { value: 'linux' });
+    treeCleanup.mockReset();
+  });
   afterEach(() => {
+    Object.defineProperty(process, 'platform', { value: nativePlatform });
     vi.useRealTimers();
     vi.restoreAllMocks();
   });
@@ -187,7 +196,7 @@ describe('shutdownLocalAcpAgent', () => {
     expect(child.kill).toHaveBeenNthCalledWith(2, 'SIGKILL');
   });
 
-  if (process.platform !== 'win32') {
+  {
     it('terminates the ACP process group on POSIX when the child has a PID', async () => {
       const child = createFakeChildProcess({ pid: 1234 });
       const processKill = vi.spyOn(process, 'kill').mockImplementation((pid, signal) => {
@@ -235,4 +244,109 @@ describe('shutdownLocalAcpAgent', () => {
       expect(child.kill).not.toHaveBeenCalled();
     });
   }
+
+  it('recognizes a child that already exited from a signal', async () => {
+    const child = createFakeChildProcess();
+    child.signalCode = 'SIGTERM';
+    await shutdownLocalAcpAgent({
+      agentProcess: child,
+      logger: createSilentLogger(),
+      sessionLabel: 'signal',
+    });
+    expect(child.kill).not.toHaveBeenCalled();
+  });
+
+  it('rejects when force termination never produces an exit', async () => {
+    vi.useFakeTimers();
+    const child = createFakeChildProcess({ exitOnSigterm: false, exitOnSigkill: false });
+    const result = shutdownLocalAcpAgent({
+      agentProcess: child,
+      logger: createSilentLogger(),
+      sessionLabel: 'stuck',
+      exitTimeoutMs: 10,
+    });
+    const assertion = expect(result).rejects.toThrow('did not exit after SIGKILL');
+    await vi.advanceTimersByTimeAsync(20);
+    await assertion;
+    expect(child.listenerCount('exit')).toBe(0);
+  });
+
+  it('reports a signaling failure and allows a later cleanup attempt', async () => {
+    const child = createFakeChildProcess();
+    vi.mocked(child.kill).mockImplementationOnce(() => {
+      throw new Error('permission denied');
+    });
+    const options = { agentProcess: child, logger: createSilentLogger(), sessionLabel: 'retry' };
+    await expect(shutdownLocalAcpAgent(options)).rejects.toThrow('permission denied');
+    await shutdownLocalAcpAgent(options);
+    expect(child.exitCode).toBe(0);
+  });
+
+  it('waits for shared Windows tree cleanup despite protocol close failure', async () => {
+    Object.defineProperty(process, 'platform', { value: 'win32' });
+    const child = createFakeChildProcess({ pid: 1234 });
+    let finish: (() => void) | undefined;
+    treeCleanup.mockImplementation(
+      () =>
+        new Promise<void>((resolve) => {
+          finish = resolve;
+        })
+    );
+    const options = {
+      agentProcess: child,
+      logger: createSilentLogger(),
+      sessionLabel: 'tree',
+      client: {
+        closeSession: async () => {
+          throw new Error('closed');
+        },
+      } as never,
+      acpSessionId: 'acp' as never,
+    };
+    let settled = false;
+    const first = shutdownLocalAcpAgent(options).then(() => {
+      settled = true;
+    });
+    const second = shutdownLocalAcpAgent(options);
+    await Promise.resolve();
+    await Promise.resolve();
+    expect(settled).toBe(false);
+    expect(treeCleanup).toHaveBeenCalledTimes(1);
+    child.signalCode = 'SIGKILL';
+    finish?.();
+    await Promise.all([first, second]);
+    expect(settled).toBe(true);
+    expect(child.kill).not.toHaveBeenCalled();
+  });
+
+  it('reports Windows tree failure and permits retry', async () => {
+    Object.defineProperty(process, 'platform', { value: 'win32' });
+    const child = createFakeChildProcess({ pid: 1234 });
+    treeCleanup.mockRejectedValueOnce(new Error('tree failed')).mockImplementationOnce(async () => {
+      child.signalCode = 'SIGKILL';
+    });
+    const options = {
+      agentProcess: child,
+      logger: createSilentLogger(),
+      sessionLabel: 'tree-retry',
+    };
+    await expect(shutdownLocalAcpAgent(options)).rejects.toThrow('tree failed');
+    await shutdownLocalAcpAgent(options);
+    expect(child.signalCode).toBe('SIGKILL');
+  });
+
+  it('does not equate Windows helper success with wrapper exit', async () => {
+    Object.defineProperty(process, 'platform', { value: 'win32' });
+    vi.useFakeTimers();
+    treeCleanup.mockResolvedValue(undefined);
+    const result = shutdownLocalAcpAgent({
+      agentProcess: createFakeChildProcess({ pid: 1234 }),
+      logger: createSilentLogger(),
+      sessionLabel: 'tree-timeout',
+      exitTimeoutMs: 10,
+    });
+    const assertion = expect(result).rejects.toThrow('did not exit after Windows tree termination');
+    await vi.advanceTimersByTimeAsync(10);
+    await assertion;
+  });
 });

apps/cli/src/agent/acp-runner.ts

@@ -13,6 +13,7 @@ import { v4 as uuidV4 } from 'uuid';
 import { z } from 'zod';
 
 import type { Logger } from '@/utils/logger';
+import { terminateWindowsProcessTree } from '@/utils/windows-process-tree';
 import type { TerminalManager } from '@/session/terminal-manager';
 import {
   AgentClient,
@@ -162,8 +163,12 @@ export const createAcpClient = async (options: CreateAcpClientOptions) => {
   return { client, acpSessionId: sessionResponse.sessionId as ACPSessionId, sessionResponse };
 };
 
+function hasChildExited(child: ChildProcess): boolean {
+  return child.exitCode !== null || child.signalCode != null;
+}
+
 function waitForChildProcessExit(child: ChildProcess, timeoutMs: number): Promise<boolean> {
-  if (child.exitCode !== null) {
+  if (hasChildExited(child)) {
     return Promise.resolve(true);
   }
 
@@ -174,7 +179,7 @@ function waitForChildProcessExit(child: ChildProcess, timeoutMs: number): Promis
     };
     const onTimeout = () => {
       cleanup();
-      resolve(child.exitCode !== null);
+      resolve(hasChildExited(child));
     };
     const cleanup = () => {
       clearTimeout(timeoutHandle);
@@ -195,20 +200,48 @@ function signalChildProcess(child: ChildProcess, signal: NodeJS.Signals): void {
   child.kill(signal);
 }
 
-async function terminateChildProcess(
+const childTerminations = new WeakMap<ChildProcess, Promise<void>>();
+
+function terminateChildProcess(
+  child: ChildProcess,
+  logger: Logger,
+  sessionLabel: string,
+  exitTimeoutMs: number
+): Promise<void> {
+  const pending = childTerminations.get(child);
+  if (pending) return pending;
+  const termination = terminateChildProcessOnce(child, logger, sessionLabel, exitTimeoutMs);
+  childTerminations.set(child, termination);
+  void termination.catch(() => {
+    if (childTerminations.get(child) === termination) childTerminations.delete(child);
+  });
+  return termination;
+}
+
+async function terminateChildProcessOnce(
   child: ChildProcess,
   logger: Logger,
   sessionLabel: string,
   exitTimeoutMs: number
 ): Promise<void> {
-  if (child.exitCode !== null) {
+  if (process.platform === 'win32') {
+    await terminateWindowsProcessTree(child, true, { timeoutMs: exitTimeoutMs });
+    if (!(await waitForChildProcessExit(child, exitTimeoutMs))) {
+      throw new Error(
+        `[${sessionLabel}] ACP agent process did not exit after Windows tree termination`
+      );
+    }
+    return;
+  }
+  if (hasChildExited(child)) {
     return;
   }
 
   try {
     signalChildProcess(child, 'SIGTERM');
-  } catch {
-    return;
+  } catch (error) {
+    if (hasChildExited(child)) return;
+    throw error;
   }
 
   if (await waitForChildProcessExit(child, exitTimeoutMs)) {
@@ -220,10 +253,13 @@ async function terminateChildProcess(
   );
   try {
     signalChildProcess(child, 'SIGKILL');
-  } catch {
-    return;
+  } catch (error) {
+    if (hasChildExited(child)) return;
+    throw error;
+  }
+  if (!(await waitForChildProcessExit(child, exitTimeoutMs))) {
+    throw new Error(`[${sessionLabel}] ACP agent process did not exit after SIGKILL`);
   }
-  await waitForChildProcessExit(child, exitTimeoutMs);
 }
 
 export type SpawnAcpProcessOptions = {

apps/cli/src/commands/start-session-shutdown.test.ts

@@ -0,0 +1,81 @@
+import { afterEach, expect, it, vi } from 'vitest';
+import type { ChildProcess } from 'child_process';
+import { EventEmitter } from 'events';
+import type { SessionId, WorkspaceId } from '@lody/shared';
+import { Session } from '../session/session';
+import { createStartShutdownController } from './start-shutdown';
+import type { Logger } from '../utils/logger';
+import type { SessionProcessHandle } from '../session/session-sandbox';
+
+afterEach(() => vi.useRealTimers());
+
+it('runs the Session process phase before outer exit when terminal disposal hangs', async () => {
+  vi.useFakeTimers();
+  const logger: Logger = {
+    info: vi.fn(),
+    warn: vi.fn(),
+    error: vi.fn(),
+    debug: vi.fn(),
+    success: vi.fn(),
+    setLevel: vi.fn(),
+    child: () => logger,
+    close: async () => {},
+  };
+  const session = new Session(
+    {
+      workspaceId: 'workspace' as WorkspaceId,
+      sessionId: 'session' as SessionId,
+      userId: 'user',
+      machineId: 'machine',
+      agentCliType: 'builtin',
+      agentType: 'codex',
+      userName: 'test',
+      userEmail: 'test@example.com',
+    },
+    logger,
+    process.cwd()
+  );
+  session.acpSessionId = 'acp' as never;
+  const dispose = vi
+    .spyOn(session.terminalManager, 'disposeAll')
+    .mockImplementation(() => new Promise(() => {}));
+  const child = new EventEmitter() as ChildProcess;
+  child.exitCode = null;
+  child.signalCode = null;
+  const terminate = vi.fn(async () => {
+    child.signalCode = 'SIGKILL';
+  });
+  const handle: SessionProcessHandle = {
+    child,
+    terminate,
+    inspectExit: async () => null,
+    onExit: () => () => {},
+    onClose: () => () => {},
+    onError: () => () => {},
+  };
+  // @ts-expect-error - synthetic owned process fixture
+  session.agentProcess = handle;
+  const exit = vi.fn();
+  const controller = createStartShutdownController({
+    signals: [],
+    logger,
+    shutdown: () => session.terminate(false),
+    forceShutdown: () => session.terminate(true),
+    flushTelemetry: async () => {},
+    exit,
+  });
+  const result = controller.shutdown('SIGTERM');
+  await vi.advanceTimersByTimeAsync(14_999);
+  expect(terminate).not.toHaveBeenCalled();
+  expect(exit).not.toHaveBeenCalled();
+  await vi.advanceTimersByTimeAsync(1);
+  expect(terminate).toHaveBeenCalledWith(true);
+  expect(exit).not.toHaveBeenCalled();
+  await vi.advanceTimersByTimeAsync(5_000);
+  await result;
+  expect(dispose).toHaveBeenCalledTimes(1);
+  expect(exit).toHaveBeenCalledWith(143);
+  // Failed terminal cleanup remains truthfully retryable despite root termination.
+  // @ts-expect-error - retained cleanup state
+  expect(session.status).toBe('stopping');
+});