Fix e2e Electron teardown cleanup

Co-authored-by: Orca <help@stably.ai>

tests/e2e/helpers/electron-process-shutdown.ts

@@ -0,0 +1,192 @@
+import type { ChildProcess } from 'node:child_process'
+import { execFileSync } from 'node:child_process'
+import { existsSync, readFileSync, readdirSync } from 'node:fs'
+import path from 'node:path'
+import type { ElectronApplication } from '@stablyai/playwright-test'
+
+const GRACEFUL_CLOSE_TIMEOUT_MS = 10_000
+const PROCESS_EXIT_TIMEOUT_MS = 5_000
+const FORCE_KILL_WAIT_MS = 2_000
+
+function delay(ms: number): Promise<void> {
+  return new Promise((resolve) => {
+    const timeout = setTimeout(resolve, ms)
+    timeout.unref?.()
+  })
+}
+
+function hasExited(proc: ChildProcess): boolean {
+  return proc.exitCode !== null || proc.signalCode !== null
+}
+
+function waitForExit(proc: ChildProcess, timeoutMs: number): Promise<boolean> {
+  if (hasExited(proc)) {
+    return Promise.resolve(true)
+  }
+
+  return new Promise((resolve) => {
+    let settled = false
+    const finish = (exited: boolean): void => {
+      if (settled) {
+        return
+      }
+      settled = true
+      clearTimeout(timeout)
+      proc.off('exit', onExit)
+      proc.off('close', onExit)
+      resolve(exited)
+    }
+    const onExit = (): void => finish(true)
+    const timeout = setTimeout(() => finish(false), timeoutMs)
+    timeout.unref?.()
+    proc.once('exit', onExit)
+    proc.once('close', onExit)
+  })
+}
+
+async function withTimeout<T>(promise: Promise<T>, timeoutMs: number, message: string): Promise<T> {
+  let timeout: NodeJS.Timeout | null = null
+  try {
+    return await Promise.race([
+      promise,
+      new Promise<never>((_, reject) => {
+        timeout = setTimeout(() => reject(new Error(message)), timeoutMs)
+        timeout.unref?.()
+      })
+    ])
+  } finally {
+    if (timeout) {
+      clearTimeout(timeout)
+    }
+  }
+}
+
+function readPosixDescendantPids(rootPid: number): number[] {
+  try {
+    const output = execFileSync('ps', ['-eo', 'pid=,ppid='], { encoding: 'utf8' })
+    const childrenByParent = new Map<number, number[]>()
+    for (const line of output.split('\n')) {
+      const [pidText, ppidText] = line.trim().split(/\s+/)
+      const pid = Number(pidText)
+      const ppid = Number(ppidText)
+      if (!Number.isInteger(pid) || !Number.isInteger(ppid)) {
+        continue
+      }
+      const children = childrenByParent.get(ppid) ?? []
+      children.push(pid)
+      childrenByParent.set(ppid, children)
+    }
+
+    const descendants: number[] = []
+    const stack = [...(childrenByParent.get(rootPid) ?? [])]
+    while (stack.length > 0) {
+      const pid = stack.pop()
+      if (!pid) {
+        continue
+      }
+      descendants.push(pid)
+      stack.push(...(childrenByParent.get(pid) ?? []))
+    }
+    return descendants
+  } catch {
+    return []
+  }
+}
+
+function killPid(pid: number, signal: NodeJS.Signals): void {
+  try {
+    process.kill(pid, signal)
+  } catch {
+    /* already dead or inaccessible */
+  }
+}
+
+async function forceKillPidTree(pid: number): Promise<void> {
+  if (!pid) {
+    return
+  }
+
+  if (process.platform === 'win32') {
+    try {
+      execFileSync('taskkill', ['/pid', String(pid), '/T', '/F'], { stdio: 'ignore' })
+    } catch {
+      /* already dead or taskkill unavailable */
+    }
+    return
+  }
+
+  // Why: CI showed orphaned Electron renderer and shell children after the
+  // parent close path returned. Capture the tree before killing the root so
+  // descendants do not get reparented out from under the cleanup.
+  const pids = [...readPosixDescendantPids(pid), pid]
+  for (const targetPid of [...pids].reverse()) {
+    killPid(targetPid, 'SIGTERM')
+  }
+  await delay(FORCE_KILL_WAIT_MS)
+  for (const targetPid of [...pids].reverse()) {
+    killPid(targetPid, 'SIGKILL')
+  }
+}
+
+async function forceKillProcessTree(proc: ChildProcess): Promise<void> {
+  const pid = proc.pid
+  if (!pid || hasExited(proc)) {
+    return
+  }
+
+  await forceKillPidTree(pid)
+  await waitForExit(proc, PROCESS_EXIT_TIMEOUT_MS)
+}
+
+export async function closeElectronAppForE2E(app: ElectronApplication): Promise<void> {
+  const proc = app.process()
+  try {
+    await withTimeout(app.close(), GRACEFUL_CLOSE_TIMEOUT_MS, 'Timed out closing Electron app')
+    if (proc) {
+      const exited = await waitForExit(proc, PROCESS_EXIT_TIMEOUT_MS)
+      if (!exited) {
+        await forceKillProcessTree(proc)
+      }
+    }
+  } catch {
+    if (proc) {
+      await forceKillProcessTree(proc)
+    }
+  }
+}
+
+function readDaemonPidFiles(userDataDir: string): number[] {
+  const daemonDir = path.join(userDataDir, 'daemon')
+  if (!existsSync(daemonDir)) {
+    return []
+  }
+
+  const pids: number[] = []
+  for (const entry of readdirSync(daemonDir)) {
+    if (!entry.endsWith('.pid')) {
+      continue
+    }
+    try {
+      const raw = readFileSync(path.join(daemonDir, entry), 'utf8').trim()
+      const parsed = JSON.parse(raw) as { pid?: unknown }
+      if (typeof parsed.pid === 'number' && Number.isInteger(parsed.pid)) {
+        pids.push(parsed.pid)
+      }
+    } catch {
+      const pid = Number(readFileSync(path.join(daemonDir, entry), 'utf8').trim())
+      if (Number.isInteger(pid)) {
+        pids.push(pid)
+      }
+    }
+  }
+  return pids
+}
+
+export async function cleanupE2EDaemons(userDataDir: string): Promise<void> {
+  // Why: app quit intentionally leaves daemon PTYs alive for warm reattach.
+  // E2E temp profiles are deleted after each test, so their detached daemons
+  // must be stopped explicitly or CI accumulates orphan Electron/shell trees.
+  for (const pid of readDaemonPidFiles(userDataDir)) {
+    await forceKillPidTree(pid)
+  }
+}

tests/e2e/helpers/orca-app.ts

@@ -25,6 +25,7 @@ import { execSync } from 'child_process'
 import os from 'os'
 import path from 'path'
 import { TEST_REPO_PATH_FILE } from '../global-setup'
+import { cleanupE2EDaemons, closeElectronAppForE2E } from './electron-process-shutdown'
 
 type OrcaTestFixtures = {
   electronApp: ElectronApplication
@@ -219,27 +220,10 @@ export const test = base.extend<OrcaTestFixtures, OrcaWorkerFixtures>({
       }
     })
     await provideFixture(app)
-    // Why: Electron's graceful shutdown runs before-quit/will-quit handlers,
-    // cleans up PTY child processes, and flushes session state to disk. Give
-    // it 10s for a clean exit, then SIGKILL the process tree immediately.
-    // SIGTERM doesn't reliably stop the Electron process tree on macOS.
-    const appProcess = app.process()
-    try {
-      await Promise.race([
-        app.close(),
-        new Promise<never>((_, reject) => {
-          setTimeout(() => reject(new Error('Timed out closing Electron app')), 10_000)
-        })
-      ])
-    } catch {
-      if (appProcess) {
-        try {
-          appProcess.kill('SIGKILL')
-        } catch {
-          /* already dead */
-        }
-      }
-    }
+    // Why: the Playwright close promise can settle before all Electron and PTY
+    // descendants are gone in CI; worker teardown then hangs on open handles.
+    await closeElectronAppForE2E(app)
+    await cleanupE2EDaemons(userDataDir)
     rmSync(userDataDir, { recursive: true, force: true })
   },
 

tests/e2e/helpers/orca-restart.ts

@@ -19,6 +19,7 @@ import { execSync } from 'child_process'
 import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'fs'
 import os from 'os'
 import path from 'path'
+import { cleanupE2EDaemons, closeElectronAppForE2E } from './electron-process-shutdown'
 
 type LaunchedOrca = {
   app: ElectronApplication
@@ -31,7 +32,7 @@ type RestartSession = {
   /** Gracefully close a launch, letting beforeunload flush session state. */
   close: (app: ElectronApplication) => Promise<void>
   /** Remove the shared userDataDir after the test is done. */
-  dispose: () => void
+  dispose: () => Promise<void>
 }
 
 function shouldLaunchHeadful(testInfo: TestInfo): boolean {
@@ -98,29 +99,11 @@ export function createRestartSession(testInfo: TestInfo): RestartSession {
   }
 
   const close = async (app: ElectronApplication): Promise<void> => {
-    // Why: mirror the shared fixture's shutdown race — give Electron 10s to run
-    // before-quit/will-quit (which drives the beforeunload → session.setSync
-    // flush this suite relies on) and only then fall back to SIGKILL.
-    const proc = app.process()
-    try {
-      await Promise.race([
-        app.close(),
-        new Promise<never>((_, reject) => {
-          setTimeout(() => reject(new Error('Timed out closing Electron app')), 10_000)
-        })
-      ])
-    } catch {
-      if (proc) {
-        try {
-          proc.kill('SIGKILL')
-        } catch {
-          /* already dead */
-        }
-      }
-    }
+    await closeElectronAppForE2E(app)
   }
 
-  const dispose = (): void => {
+  const dispose = async (): Promise<void> => {
+    await cleanupE2EDaemons(userDataDir)
     if (existsSync(userDataDir)) {
       rmSync(userDataDir, { recursive: true, force: true })
     }

tests/e2e/resource-usage-warm-reattach.spec.ts

@@ -178,7 +178,7 @@ test.describe('Resource Usage warm-reattach', () => {
       if (firstApp) {
         await session.close(firstApp)
       }
-      session.dispose()
+      await session.dispose()
     }
   })
 })

tests/e2e/terminal-restart-persistence.spec.ts

@@ -158,7 +158,7 @@ test.describe('Terminal restart persistence', () => {
       if (firstApp) {
         await session.close(firstApp)
       }
-      session.dispose()
+      await session.dispose()
     }
   })
 
@@ -223,7 +223,7 @@ test.describe('Terminal restart persistence', () => {
       if (firstApp) {
         await session.close(firstApp)
       }
-      session.dispose()
+      await session.dispose()
     }
   })
 
@@ -271,7 +271,7 @@ test.describe('Terminal restart persistence', () => {
       if (app) {
         await session.close(app)
       }
-      session.dispose()
+      await session.dispose()
     }
   })
 })