[1/3] fix: bound Windows ACP shutdown and retain failed cleanup

apps/cli/src/agent/AGENTS.md

@@ -81,8 +81,15 @@ 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 retained-child-handle cleanup utility, and reports termination failure; protocol
+  session-close failure must still proceed to process cleanup.
+  Windows termination must never reopen a cached PID. Observed child exit is the
+  utility's guarantee; descendant teardown requires spawn-time Job Object ownership.
+  Protocol authentication cleanup failure must return an error and retain its child
+  and provider slot until observed exit. Cancellation must permit cleanup retry.
+  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

@@ -9,6 +9,13 @@ import { afterEach, describe, expect, it, vi } from 'vitest';
 import type { Logger } from '@/utils/logger';
 import { createStdinWritableStream, createStdoutReadableStream } from '@/utils/stream';
 import { AcpAuthenticationManager, probeBuiltinAuthentication } from './acp-authentication';
+import * as acpRunner from './acp-runner';
+
+vi.mock('@/utils/windows-child-process', () => ({
+  terminateWindowsChildProcess: async (child: ChildProcess) => {
+    if (child.exitCode == null && child.signalCode == null) child.kill('SIGKILL');
+  },
+}));
 
 const createSilentLogger = (): Logger => ({
   info: () => {},
@@ -45,6 +52,52 @@ function createDeferred<T>() {
 }
 
 describe('AcpAuthenticationManager', () => {
+  it.each(['success', 'failure'] as const)(
+    'retains authentication ownership after root exit until %s cleanup settles',
+    async (outcome) => {
+      const child = createFakeChild();
+      const cleanup = createDeferred<void>();
+      const failure = new Error('client shutdown failed');
+      const shutdown = vi
+        .spyOn(acpRunner, 'shutdownLocalAcpAgent')
+        .mockImplementationOnce(async () => {
+          await cleanup.promise;
+          if (outcome === 'failure') throw failure;
+        })
+        .mockResolvedValue(undefined);
+      const spawnProcess = vi.fn(() => child);
+      const manager = new AcpAuthenticationManager(createSilentLogger(), {
+        spawnProcess: spawnProcess as never,
+        resolveLoginShellEnv: async () => ({}),
+      });
+      const input = {
+        cliType: 'builtin' as const,
+        agentType: 'codex',
+        runtimeOverrides: { codexPath: '/test/codex' },
+      };
+      const authentication = manager.authenticate({ requestId: 'owned', ...input });
+      await vi.waitFor(() => expect(spawnProcess).toHaveBeenCalledOnce());
+      manager.cancel('owned');
+      child.exitCode = 0;
+      child.emit('exit', 0, null);
+      await expect(authentication).resolves.toMatchObject({ disposition: 'cancelled' });
+      expect(manager.getAgentType('owned')).toBe('codex');
+      await expect(manager.authenticate({ requestId: 'overlap', ...input })).resolves.toMatchObject(
+        { success: false, error: 'Codex authentication is already running' }
+      );
+      manager.cancel('owned');
+      expect(shutdown).toHaveBeenCalledOnce();
+      cleanup.resolve();
+      await new Promise<void>((resolve) => setImmediate(resolve));
+      if (outcome === 'failure') {
+        expect(manager.getAgentType('owned')).toBe('codex');
+        manager.cancel('owned');
+      }
+      await vi.waitFor(() => expect(manager.getAgentType('owned')).toBeUndefined());
+      expect(shutdown).toHaveBeenCalledTimes(outcome === 'failure' ? 2 : 1);
+    }
+  );
+
   afterEach(() => {
     vi.useRealTimers();
     vi.restoreAllMocks();
@@ -316,15 +369,87 @@ 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,
       disposition: 'authenticated',
     });
   });
 
+  it.each(['direct', 'npx'] as const)(
+    'reports %s protocol cleanup failure and retains the login owner until cancellation retry exits',
+    async (launcher) => {
+      const child = createFakeChild();
+      const originalKill = child.kill;
+      child.kill = vi.fn(() => {
+        throw new Error('cleanup failed');
+      });
+      const stdin = new PassThrough();
+      const stdout = new PassThrough();
+      child.stdin = stdin;
+      child.stdout = stdout;
+      child.stderr = new PassThrough();
+      acp
+        .agent({ name: 'cleanup-test' })
+        .onRequest(acp.methods.agent.initialize, async ({ params }) => ({
+          protocolVersion: params.protocolVersion,
+          authMethods: [{ id: 'oauth', name: 'OAuth' }],
+        }))
+        .onRequest(acp.methods.agent.authenticate, async () => {
+          child.stderr?.emit('data', 'Error: Cannot find module auth-startup-dependency');
+          return {};
+        })
+        .connect(
+          acp.ndJsonStream(createStdinWritableStream(stdout), createStdoutReadableStream(stdin))
+        );
+      const spawnProcess = vi.fn(() => child);
+      const manager = new AcpAuthenticationManager(createSilentLogger(), {
+        spawnProcess: spawnProcess as never,
+        resolveLoginShellEnv: async () => ({}),
+        terminationGraceMs: 2,
+      });
+      const input = {
+        cliType: 'custom' as const,
+        agentType: 'cleanup-test',
+        customAcp:
+          launcher === 'npx'
+            ? { command: 'npx', args: ['--yes', '@lody-test/auth-cleanup-incomplete@0.0.0'] }
+            : { command: '/test/custom-acp', args: [] },
+      };
+      const progress = vi.fn();
+      await expect(
+        manager.authenticate({ requestId: 'cleanup-1', ...input, onProgress: progress })
+      ).resolves.toMatchObject({ success: false, disposition: 'error', error: 'cleanup failed' });
+      expect(progress).not.toHaveBeenCalledWith({ status: 'authenticated' });
+      expect(manager.getAgentType('cleanup-1')).toBe('cleanup-test');
+      await expect(
+        manager.authenticate({ requestId: 'cleanup-2', ...input })
+      ).resolves.toMatchObject({
+        success: false,
+        error: 'cleanup-test authentication is already running',
+      });
+      expect(spawnProcess).toHaveBeenCalledOnce();
+      manager.cancel('cleanup-1');
+      await vi.waitFor(() => expect(child.kill).toHaveBeenCalledTimes(2));
+      // Allow the rejected teardown's finally to make the same owner retryable.
+      await new Promise<void>((resolve) => setImmediate(resolve));
+      expect(manager.getAgentType('cleanup-1')).toBe('cleanup-test');
+      child.kill = originalKill;
+      manager.cancel('cleanup-1');
+      await vi.waitFor(() => expect(manager.getAgentType('cleanup-1')).toBeUndefined());
+      expect(originalKill).toHaveBeenCalled();
+      expect(manager.cancel('cleanup-1')).toEqual({ success: true, disposition: 'not-running' });
+      stdin.destroy();
+      stdout.destroy();
+      child.stderr.destroy();
+    }
+  );
   it('bridges request-scoped ACP form elicitation for a custom provider', async () => {
     const child = createFakeChild();
     const stdin = new PassThrough();
@@ -531,7 +656,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-authentication.ts

@@ -136,6 +136,9 @@ type RunningAuthentication = {
   cancelled: boolean;
   timedOut: boolean;
   terminating: boolean;
+  cleanupPromise?: Promise<void>;
+  cleanupFailed?: boolean;
+  workflowFinished?: boolean;
   acceptsAuthorizationCode: boolean;
   authorizationCodeSubmitted: boolean;
   abortController: AbortController;
@@ -622,7 +625,7 @@ export class AcpAuthenticationManager {
         detached: process.platform !== 'win32',
         windowsHide: true,
       });
-      running.child = child;
+      this.trackAuthenticationChild(options.agentType, running, child);
       child.stdin?.on('error', (error: unknown) => {
         this.logger.debug(
           `[acp-auth] ${displayName} authorization input failed: ${formatErrorMessage(error)}`
@@ -684,9 +687,8 @@ export class AcpAuthenticationManager {
       if (timeoutHandle) {
         clearTimeout(timeoutHandle);
       }
-      if (this.runningByAgentType.get(options.agentType) === running) {
-        this.runningByAgentType.delete(options.agentType);
-      }
+      running.workflowFinished = true;
+      this.releaseFinishedAuthentication(options.agentType, running);
     }
   }
 
@@ -872,8 +874,16 @@ export class AcpAuthenticationManager {
           logger: this.logger,
           logPrefix: '[acp-auth]',
           getStderrTail: () => lastStderrTail,
+          shouldRetryError: () => !running.abortController.signal.aborted && !running.cleanupFailed,
           attempt: async ({ args }) => {
             running.abortController.signal.throwIfAborted();
+            if (
+              running.cleanupFailed ||
+              running.terminating ||
+              (running.child && running.child.exitCode == null && running.child.signalCode == null)
+            ) {
+              throw new Error('Previous authentication process cleanup is incomplete');
+            }
             lastStderrTail = '';
             options.onProgress?.({ status: 'starting' });
             const child = spawnAcpProcess({
@@ -887,7 +897,7 @@ export class AcpAuthenticationManager {
               args: [...args],
               spawnImpl: this.spawnProcess,
             });
-            running.child = child;
+            this.trackAuthenticationChild(options.agentType, running, child);
             running.terminating = false;
             child.stderr?.setEncoding('utf8');
             const authorizationParser = new AcpAgentAuthorizationOutputParser();
@@ -1089,17 +1099,7 @@ export class AcpAuthenticationManager {
               running.pendingInteraction?.resolve({ action: 'cancel' });
               running.pendingInteraction = undefined;
               startupMonitor.dispose();
-              await shutdownLocalAcpAgent({
-                agentProcess: child,
-                logger: this.logger,
-                sessionLabel: `acp-auth:${options.agentType}:protocol`,
-                exitTimeoutMs: this.terminationGraceMs,
-              }).catch((error: unknown) => {
-                this.logger.debug(
-                  `[acp-auth] Failed to terminate protocol authentication process: ${formatErrorMessage(error)}`
-                );
-              });
-              if (running.child === child) running.child = undefined;
+              await this.cleanupAuthentication(options.agentType, running, 'protocol');
             }
           },
         })
@@ -1113,6 +1113,27 @@ export class AcpAuthenticationManager {
     return { success: true, disposition: 'authenticated' };
   }
 
+  private releaseFinishedAuthentication(agentType: string, running: RunningAuthentication): void {
+    const child = running.child;
+    if (
+      running.workflowFinished &&
+      !running.terminating &&
+      !running.cleanupFailed &&
+      (!child || child.exitCode != null || child.signalCode != null) &&
+      this.runningByAgentType.get(agentType) === running
+    ) {
+      this.runningByAgentType.delete(agentType);
+    }
+  }
+
+  private trackAuthenticationChild(
+    agentType: string,
+    running: RunningAuthentication,
+    child: ChildProcess
+  ): void {
+    running.child = child;
+    child.once('exit', () => this.releaseFinishedAuthentication(agentType, running));
+  }
   private terminateAuthentication(
     agentType: string,
     running: RunningAuthentication,
@@ -1121,17 +1142,42 @@ export class AcpAuthenticationManager {
     // Protocol authentication spans launch preparation, a JSON-RPC wait, and
     // possibly a second process, so the signal is raised even with no child yet.
     running.abortController.abort();
-    if (running.terminating || !running.child) return;
-    running.terminating = true;
-    void shutdownLocalAcpAgent({
-      agentProcess: running.child,
-      logger: this.logger,
-      sessionLabel: `acp-auth:${agentType}:${reason}`,
-      exitTimeoutMs: this.terminationGraceMs,
-    }).catch((error: unknown) => {
+    void this.cleanupAuthentication(agentType, running, reason).catch((error: unknown) => {
       this.logger.debug(
         `[acp-auth] Failed to terminate authentication process: ${formatErrorMessage(error)}`
       );
     });
   }
+
+  private cleanupAuthentication(
+    agentType: string,
+    running: RunningAuthentication,
+    reason: string
+  ): Promise<void> {
+    if (running.cleanupPromise) return running.cleanupPromise;
+    const child = running.child;
+    if (!child) return Promise.resolve();
+    running.terminating = true;
+    const cleanup = shutdownLocalAcpAgent({
+      agentProcess: child,
+      logger: this.logger,
+      sessionLabel: `acp-auth:${agentType}:${reason}`,
+      exitTimeoutMs: this.terminationGraceMs,
+    })
+      .then(() => {
+        running.cleanupFailed = false;
+        if (running.child === child) running.child = undefined;
+      })
+      .catch((error: unknown) => {
+        running.cleanupFailed = true;
+        throw error;
+      })
+      .finally(() => {
+        running.terminating = false;
+        running.cleanupPromise = undefined;
+        this.releaseFinishedAuthentication(agentType, running);
+      });
+    running.cleanupPromise = cleanup;
+    return cleanup;
+  }
 }

apps/cli/src/agent/acp-npx-startup-policy.ts

@@ -30,6 +30,8 @@ export type RunNpxStartupWithRecoveryOptions<T> = {
   logPrefix: string;
   attempt(input: NpxStartupAttemptInput): Promise<T>;
   getStderrTail(): string;
+  /** Stop before error classification/cache recovery when the caller cannot safely retry. */
+  shouldRetryError?: (error: unknown) => boolean;
   cleanupFailedAttempt?: () => Promise<void>;
   startupTimeouts?: AcpStartupTimeoutOptions;
   coldInitTimeoutMs?: number;
@@ -147,6 +149,7 @@ export async function runNpxStartupWithRecovery<T>(
         startupTimeouts,
       });
     } catch (error) {
+      if (options.shouldRetryError?.(error) === false) throw error;
       if (attempt >= maxAttempts) {
         throw error;
       }