fix: make ACP test teardown failure-safe

packages/support/acp-snapshot/src/harness.ts

@@ -163,7 +163,7 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise
   let launched: LaunchedAcpTestAgent | undefined
   let sessionId: string | undefined
   let sessionLogs: HarvestedLog[] = []
-  try {
+  const outcome = await (async (): Promise<RunResult> => {
     // Seed the workspace if the scenario ships one (a file the agent reads/edits).
     // Copied into the temp cwd so the agent's bash tools see it; the goldens
     // normalize the cwd, so the seeded paths stay stable across runs.
@@ -231,22 +231,36 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise
     // Harvest EVERY persisted log (parent + any subagent children) while the
     // temp dirs still exist, ordered primary-first.
     sessionLogs = await harvestSessionLogs(sessionsRoot)
-  } finally {
-    // Failure-safe teardown: kill a still-running child and drop the temp dirs
-    // even if seeding/spawn/a step/harvest threw, so a flaky run never leaks a
-    // process or dir. `launched` is undefined only if launch itself threw.
-    await launched?.close('SIGKILL')
-    await rm(cwd, { recursive: true, force: true })
-    await rm(sessionsRoot, { recursive: true, force: true })
-  }
+    return {
+      rawStdout: launched.rawStdout(),
+      stderr: launched.stderr(),
+      cwd,
+      ...sessionId !== undefined ? { sessionId } : {},
+      sessionLogs,
+    }
+  })().then(
+    value => ({ status: 'fulfilled', value } as const),
+    (error: unknown) => ({ status: 'rejected', error } as const),
+  )
 
-  return {
-    rawStdout: launched.rawStdout(),
-    stderr: launched.stderr(),
-    cwd,
-    ...sessionId !== undefined ? { sessionId } : {},
-    sessionLogs,
+  // Failure-safe teardown: wait for a still-running child, then attempt BOTH
+  // directory removals even when an earlier cleanup rejects. The main outcome
+  // wins over teardown noise so a step/harvest failure is never replaced; on a
+  // successful run, the first cleanup failure remains visible to the caller.
+  const cleanupResults: PromiseSettledResult<unknown>[] = []
+  const cleanup = async (action: () => Promise<unknown>): Promise<void> => {
+    cleanupResults.push(...await Promise.allSettled([action()]))
   }
+  /* v8 ignore next 1 -- launch itself can only throw on a defensive synchronous spawn API failure */
+  await cleanup(() => launched?.close('SIGKILL') ?? Promise.resolve())
+  await cleanup(() => rm(cwd, { recursive: true, force: true }))
+  await cleanup(() => rm(sessionsRoot, { recursive: true, force: true }))
+
+  if (outcome.status === 'rejected') throw outcome.error
+  const cleanupFailure = cleanupResults.find((result): result is PromiseRejectedResult => result.status === 'rejected')
+  /* v8 ignore next 1 -- defensive OS cleanup failure after an otherwise successful real subprocess run */
+  if (cleanupFailure !== undefined) throw cleanupFailure.reason
+  return outcome.value
 }
 
 /** Drive one input step over the client connection. */

packages/support/acp-snapshot/src/launcher.ts

@@ -95,7 +95,10 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe
   // A spawn-level failure is an asynchronous `error` event. Observe it in the
   // same tick as spawn so a missing cwd or OS rejection cannot crash the test
   // runner, then make startup and shutdown surface the original error.
-  const childFailure = new Promise<Error>(resolve => child.once('error', resolve))
+  // Keep observing after the first error: a fallback kill attempted during
+  // shutdown may itself report another process error, which must not become an
+  // unhandled EventEmitter error after the promise has already settled.
+  const childFailure = new Promise<Error>(resolve => child.on('error', resolve))
   const spawned = Promise.race([
     new Promise<void>(resolve => child.once('spawn', resolve)),
     childFailure.then((error): never => { throw error }),
@@ -163,14 +166,23 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe
     waitForUpdate: match => new Promise((resolve, reject) => updateWaiters.push({ match, resolve, reject })),
     async close(signal?: NodeJS.Signals): Promise<void> {
       await spawned
-      if (child.exitCode !== null || child.signalCode !== null) return
+      if (!isRunning(child)) return
+      const exited = waitForExit(child)
       if (signal === undefined) child.stdin.end()
       else child.kill(signal)
       const failure = await Promise.race([
-        waitForExit(child).then((): undefined => undefined),
+        exited.then((): undefined => undefined),
         childFailure,
       ])
-      if (failure !== undefined) throw failure
+      if (failure === undefined) return
+
+      // An `error` after spawn is not an exit edge: in particular, a failed
+      // signal can leave the subprocess live. Force termination, await the
+      // already-observed exit edge, and only then propagate the child error so
+      // callers may safely remove cwd/session resources after close rejects.
+      child.kill('SIGKILL')
+      await exited
+      throw failure
     },
   }
 }
@@ -179,3 +191,8 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe
 function waitForExit(child: ChildProcessWithoutNullStreams): Promise<void> {
   return new Promise<void>(resolve => child.once('exit', () => { resolve() }))
 }
+
+/** Whether the child still lacks either OS termination marker. */
+function isRunning(child: ChildProcessWithoutNullStreams): boolean {
+  return child.exitCode === null && child.signalCode === null
+}