diff --git a/tests/e2e/helpers/electron-process-shutdown.ts b/tests/e2e/helpers/electron-process-shutdown.ts new file mode 100644 index 00000000000..6f0db8455e7 --- /dev/null +++ b/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 { + 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 { + 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(promise: Promise, timeoutMs: number, message: string): Promise { + let timeout: NodeJS.Timeout | null = null + try { + return await Promise.race([ + promise, + new Promise((_, 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() + 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 { + 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 { + 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 { + 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 { + // 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) + } +} diff --git a/tests/e2e/helpers/orca-app.ts b/tests/e2e/helpers/orca-app.ts index 4a626d4596e..e8eee35bc8d 100644 --- a/tests/e2e/helpers/orca-app.ts +++ b/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({ } }) 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((_, 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 }) }, diff --git a/tests/e2e/helpers/orca-restart.ts b/tests/e2e/helpers/orca-restart.ts index 7e8d40306a8..ee4466d049a 100644 --- a/tests/e2e/helpers/orca-restart.ts +++ b/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 /** Remove the shared userDataDir after the test is done. */ - dispose: () => void + dispose: () => Promise } function shouldLaunchHeadful(testInfo: TestInfo): boolean { @@ -98,29 +99,11 @@ export function createRestartSession(testInfo: TestInfo): RestartSession { } const close = async (app: ElectronApplication): Promise => { - // 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((_, 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 => { + await cleanupE2EDaemons(userDataDir) if (existsSync(userDataDir)) { rmSync(userDataDir, { recursive: true, force: true }) } diff --git a/tests/e2e/resource-usage-warm-reattach.spec.ts b/tests/e2e/resource-usage-warm-reattach.spec.ts index 8749f1c52ec..b346c12af8a 100644 --- a/tests/e2e/resource-usage-warm-reattach.spec.ts +++ b/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() } }) }) diff --git a/tests/e2e/terminal-restart-persistence.spec.ts b/tests/e2e/terminal-restart-persistence.spec.ts index 0e812d97dea..d4fd3c2d227 100644 --- a/tests/e2e/terminal-restart-persistence.spec.ts +++ b/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() } }) })