test(electron): fix Windows cleanup and retain packaged smoke diagnostics (#1665)

* test(electron): reap Windows process trees and retain smoke diagnostics

* test(electron): require confirmed process exit before relaunch

* test(electron): retain process handle after application exit

* test(electron): bind captured processes to application instances

apps/electron-backend-e2e/src/electron-process-lifecycle.ts

@@ -1,13 +1,13 @@
 import type { ChildProcess } from 'node:child_process';
+import { terminateElectronProcess } from './electron-process-termination';
 
 type ElectronChildProcess = Pick<
     ChildProcess,
-    'exitCode' | 'kill' | 'once' | 'removeListener' | 'signalCode'
+    'exitCode' | 'kill' | 'once' | 'pid' | 'removeListener' | 'signalCode'
 >;
 
 export interface ClosableElectronApplication {
     close(): Promise<void>;
-    process(): ElectronChildProcess;
 }
 
 export interface ElectronExitConfirmationOptions {
@@ -15,6 +15,19 @@ export interface ElectronExitConfirmationOptions {
     readonly exitTimeoutMs: number;
 }
 
+const electronProcesses = new WeakMap<
+    ClosableElectronApplication,
+    ElectronChildProcess
+>();
+
+export function captureElectronProcess<Process extends ElectronChildProcess>(
+    application: ClosableElectronApplication & { process(): Process }
+): Process {
+    const child = application.process();
+    electronProcesses.set(application, child);
+    return child;
+}
+
 export interface PrepareElectronApplicationOptions<Application, Prepared> {
     readonly application: Application;
     readonly dispose: (application: Application) => Promise<void>;
@@ -49,11 +62,19 @@ export async function prepareElectronApplication<Application, Prepared>(
 
 export async function closeElectronApplicationAndConfirmExit(
     application: ClosableElectronApplication,
-    options: ElectronExitConfirmationOptions
+    options: ElectronExitConfirmationOptions,
+    terminate: (
+        child: ElectronChildProcess,
+        signal: NodeJS.Signals
+    ) => void = terminateElectronProcess
 ): Promise<void> {
     assertTimeout(options.closeTimeoutMs);
     assertTimeout(options.exitTimeoutMs);
-    const child = application.process();
+    // Playwright discards its process dispatcher when Electron exits. Keep
+    // using the Node handle captured immediately after launch, including when
+    // the application's last window already caused a normal process exit.
+    const child = electronProcesses.get(application);
+    if (!child) throw new Error('electron-process-handle-not-captured');
     if (hasExited(child)) return;
 
     const exit = observeProcessExit(child);
@@ -82,14 +103,19 @@ export async function closeElectronApplicationAndConfirmExit(
         }
         if (first.kind === 'close-rejected') failure = first.failure;
 
-        try {
-            child.kill();
-        } catch (killFailure) {
-            failure = failure
-                ? new AggregateError([failure, killFailure])
-                : killFailure;
+        // A failed termination must not skip exit observation or let a
+        // caller restart against a profile that is still owned by Electron.
+        for (const signal of ['SIGTERM', 'SIGKILL'] as const) {
+            try {
+                terminate(child, signal);
+            } catch (killFailure) {
+                failure = failure
+                    ? new AggregateError([failure, killFailure])
+                    : killFailure;
+            }
+            if (await resolvesWithin(exit.promise, options.exitTimeoutMs))
+                return;
         }
-        if (await resolvesWithin(exit.promise, options.exitTimeoutMs)) return;
     } finally {
         exit.cancel();
     }

apps/electron-backend-e2e/src/electron-process-termination.ts

@@ -0,0 +1,25 @@
+import { execFileSync, type ChildProcess } from 'node:child_process';
+
+type ElectronProcess = Pick<ChildProcess, 'pid' | 'kill'>;
+
+export function terminateElectronProcess(
+    child: ElectronProcess,
+    signal: NodeJS.Signals = 'SIGTERM',
+    platform: NodeJS.Platform = process.platform,
+    run: typeof execFileSync = execFileSync
+): void {
+    // Playwright launches Electron through cmd.exe on Windows. Killing only
+    // that ChildProcess leaves Electron (and its profile lock) alive.
+    if (platform === 'win32') {
+        if (!Number.isSafeInteger(child.pid) || (child.pid ?? 0) <= 0) {
+            throw new Error('electron-process-pid-unavailable');
+        }
+        run('taskkill.exe', ['/pid', String(child.pid), '/T', '/F'], {
+            stdio: 'pipe',
+            timeout: 5000,
+            windowsHide: true,
+        });
+        return;
+    }
+    child.kill(signal);
+}

apps/electron-backend-e2e/src/electron-test-fixtures.ts

@@ -7,7 +7,7 @@ import {
     Page,
     test as base,
 } from '@playwright/test';
-import { spawn } from 'child_process';
+import { spawn, type ChildProcess } from 'child_process';
 import { createServer, Server } from 'http';
 import {
     accessSync,
@@ -28,6 +28,7 @@ import {
     writeDataDirOwnerMarker,
 } from './data-dir-reaper';
 import {
+    captureElectronProcess,
     closeElectronApplicationAndConfirmExit,
     prepareElectronApplication,
 } from './electron-process-lifecycle';
@@ -234,6 +235,7 @@ export async function launchElectronApp(
         args,
         env: buildElectronLaunchEnvironment(dataDir, options),
     });
+    const electronProcess = captureElectronProcess(electronApp);
     return prepareElectronApplication({
         application: electronApp,
         dispose: (application) =>
@@ -242,7 +244,7 @@ export async function launchElectronApp(
                 exitTimeoutMs: electronAppKillWaitMs,
             }),
         prepare: async (application) => {
-            attachElectronProcessDiagnostics(application);
+            attachElectronProcessDiagnostics(electronProcess);
             const mainWindow = await findMainWindow(application);
             await waitForAppReady(mainWindow);
             await startPortalDebugCapture(mainWindow);
@@ -458,26 +460,31 @@ export async function launchPackagedElectronApp(
             NODE_ENV: 'test',
         },
     });
-    attachElectronProcessDiagnostics(electronApp);
-
-    const mainWindow = await findMainWindow(electronApp);
-    await waitForAppReady(mainWindow);
-
-    return {
-        electronApp,
-        mainWindow,
-    };
+    const electronProcess = captureElectronProcess(electronApp);
+    return prepareElectronApplication({
+        application: electronApp,
+        dispose: (application) =>
+            closeElectronApplicationAndConfirmExit(application, {
+                closeTimeoutMs: electronAppCloseTimeoutMs,
+                exitTimeoutMs: electronAppKillWaitMs,
+            }),
+        prepare: async (application) => {
+            attachElectronProcessDiagnostics(electronProcess);
+            const mainWindow = await findMainWindow(application);
+            await waitForAppReady(mainWindow);
+            return {
+                electronApp: application,
+                mainWindow,
+            };
+        },
+    });
 }
 
-function attachElectronProcessDiagnostics(
-    electronApp: ElectronApplication
-): void {
+function attachElectronProcessDiagnostics(childProcess: ChildProcess): void {
     if (!process.env['CI']) {
         return;
     }
 
-    const childProcess = electronApp.process();
-
     childProcess.stdout?.on('data', (chunk: Buffer) => {
         console.log(`[electron stdout] ${chunk.toString().trimEnd()}`);
     });
@@ -564,52 +571,7 @@ export async function launchCompetingElectronInstance(
 export async function closeElectronApp(
     app: LaunchedElectronApp
 ): Promise<void> {
-    try {
-        const closePromise = app.electronApp.close();
-        const closed = await waitForPromiseWithTimeout(
-            closePromise,
-            electronAppCloseTimeoutMs
-        );
-
-        if (closed) {
-            return;
-        }
-
-        console.warn(
-            `Electron app did not close within ${electronAppCloseTimeoutMs}ms; killing process`
-        );
-        const childProcess = app.electronApp.process();
-
-        if (!childProcess.killed) {
-            childProcess.kill();
-        }
-
-        await waitForPromiseWithTimeout(
-            closePromise.catch(() => undefined),
-            electronAppKillWaitMs
-        );
-
-        // SIGTERM asks Electron for a graceful quit, which the app can
-        // legitimately refuse — the unsaved-settings close guard cancels the
-        // quit while it waits for an answer. A process that survives here
-        // would outlive the test, hold its data dir, and time out the worker
-        // teardown, so escalate to SIGKILL.
-        if (
-            childProcess.exitCode === null &&
-            childProcess.signalCode === null
-        ) {
-            console.warn(
-                'Electron app survived SIGTERM; escalating to SIGKILL'
-            );
-            childProcess.kill('SIGKILL');
-            await waitForPromiseWithTimeout(
-                closePromise.catch(() => undefined),
-                electronAppKillWaitMs
-            );
-        }
-    } catch (error) {
-        console.warn('Failed to close Electron app cleanly:', error);
-    }
+    await closeElectronAppAndConfirmExit(app);
 }
 
 export async function closeElectronAppAndConfirmExit(
@@ -621,26 +583,6 @@ export async function closeElectronAppAndConfirmExit(
     });
 }
 
-async function waitForPromiseWithTimeout(
-    promise: Promise<unknown>,
-    timeoutMs: number
-): Promise<boolean> {
-    let timeoutId: NodeJS.Timeout | undefined;
-
-    try {
-        return await Promise.race([
-            promise.then(() => true),
-            new Promise<boolean>((resolvePromise) => {
-                timeoutId = setTimeout(() => resolvePromise(false), timeoutMs);
-            }),
-        ]);
-    } finally {
-        if (timeoutId) {
-            clearTimeout(timeoutId);
-        }
-    }
-}
-
 function assertPackagedRendererBuildIsElectronSafe(): void {
     if (!existsSync(packagedRendererIndexPath)) {
         throw new Error(

apps/electron-backend-e2e/src/legacy-playlist-migration.e2e.ts

@@ -18,6 +18,7 @@ import {
     openSettingsSection,
 } from './electron-test-fixtures';
 import { seedLegacyProfile, legacyPlaylists } from './legacy-profile-fixture';
+import { captureElectronProcess } from './electron-process-lifecycle';
 import { applyTheme } from './theme-contrast';
 
 interface StartupTestGlobals {
@@ -94,6 +95,7 @@ require(${JSON.stringify(electronMainPath)});`
         ],
         env: buildElectronLaunchEnvironment(dataDir),
     });
+    captureElectronProcess(app);
     const page = await app.firstWindow();
     try {
         // The route is intentionally still empty while recovery is pending.

apps/electron-backend-e2e/src/performance/electron-process-lifecycle.spec.ts

@@ -1,13 +1,28 @@
 import assert from 'node:assert/strict';
 import { EventEmitter } from 'node:events';
 import { describe, it } from 'node:test';
+import {
+    closeElectronApp,
+    type LaunchedElectronApp,
+} from '../electron-test-fixtures';
 
 import {
-    closeElectronApplicationAndConfirmExit,
+    captureElectronProcess,
+    closeElectronApplicationAndConfirmExit as closeApplication,
     ElectronApplicationDisposalError,
     prepareElectronApplication,
 } from '../electron-process-lifecycle';
 
+// Fake children have no OS PID; exercise lifecycle decisions independently
+// of the platform process-tree integration test.
+const closeElectronApplicationAndConfirmExit: typeof closeApplication = (
+    app,
+    options
+) =>
+    closeApplication(app, options, (child, signal) => {
+        child.kill(signal);
+    });
+
 class FakeChildProcess extends EventEmitter {
     exitCode: number | null = null;
     signalCode: NodeJS.Signals | null = null;
@@ -25,6 +40,94 @@ class FakeChildProcess extends EventEmitter {
 }
 
 describe('Electron process lifecycle', () => {
+    it('rejects cleanup when no process was captured for that application', async () => {
+        await assert.rejects(
+            closeApplication(
+                { close: async () => assert.fail('exit cannot be confirmed') },
+                { closeTimeoutMs: 1, exitTimeoutMs: 1 }
+            ),
+            /electron-process-handle-not-captured/
+        );
+    });
+
+    it('cleans the replacement application after a partial restart assignment', async () => {
+        const oldChild = new FakeChildProcess();
+        const newChild = new FakeChildProcess();
+        const oldApplication = {
+            close: async () => {
+                oldChild.exitCode = 0;
+                oldChild.emit('exit', 0, null);
+            },
+            process: () => oldChild,
+        };
+        const newApplication = {
+            close: async () => {
+                newChild.exitCode = 0;
+                newChild.emit('exit', 0, null);
+            },
+            process: () => newChild,
+        };
+        const app = {
+            electronApp: oldApplication,
+        } as unknown as LaunchedElectronApp;
+        captureElectronProcess(oldApplication);
+        captureElectronProcess(newApplication);
+        await closeElectronApp(app);
+        // Existing E2E callers replace the application/window fields only.
+        app.electronApp =
+            newApplication as unknown as LaunchedElectronApp['electronApp'];
+
+        await closeElectronApp(app);
+        assert.equal(newChild.exitCode, 0);
+    });
+
+    it('confirms an already exited process after Playwright disposes its dispatcher', async () => {
+        const child = new FakeChildProcess();
+        const app = {
+            electronApp: {
+                close: async () => {
+                    assert.fail(
+                        'an exited application must not be closed again'
+                    );
+                },
+                process: () => child,
+            },
+        } as unknown as LaunchedElectronApp;
+
+        captureElectronProcess(app.electronApp);
+        child.exitCode = 0;
+        app.electronApp.process = () => {
+            throw new TypeError(
+                "Cannot read properties of undefined (reading '_object')"
+            );
+        };
+        await closeElectronApp(app);
+        assert.equal(child.killCalls, 0);
+    });
+
+    it('does not return from public cleanup when close and termination both fail', async () => {
+        const child = new FakeChildProcess();
+        child.kill = () => {
+            throw new Error('termination failed');
+        };
+        const app = {
+            electronApp: {
+                close: async () => {
+                    throw new Error('CDP disconnected');
+                },
+                process: () => child,
+            },
+        } as unknown as LaunchedElectronApp;
+
+        captureElectronProcess(app.electronApp);
+        app.electronApp.process = () => {
+            assert.fail('cleanup must use the retained child process');
+        };
+        await assert.rejects(
+            closeElectronApp(app),
+            /electron-process-exit-unconfirmed/
+        );
+    });
     it('closes and confirms exit when post-spawn launch preparation fails', async () => {
         const child = new FakeChildProcess();
         const launchFailure = new Error('renderer readiness failed');
@@ -37,6 +140,7 @@ describe('Electron process lifecycle', () => {
             },
             process: () => child,
         };
+        captureElectronProcess(application);
 
         await assert.rejects(
             prepareElectronApplication({
@@ -56,6 +160,40 @@ describe('Electron process lifecycle', () => {
         assert.equal(child.exitCode, 0);
     });
 
+    it('preserves preparation failure when Electron has already exited', async () => {
+        const child = new FakeChildProcess();
+        const launchFailure = new Error('renderer closed before readiness');
+        const application = {
+            close: async () => {
+                assert.fail('an exited application must not be closed again');
+            },
+            process: () => child,
+        };
+        captureElectronProcess(application);
+
+        await assert.rejects(
+            prepareElectronApplication({
+                application,
+                dispose: (app) =>
+                    closeElectronApplicationAndConfirmExit(app, {
+                        closeTimeoutMs: 10,
+                        exitTimeoutMs: 10,
+                    }),
+                prepare: async () => {
+                    child.exitCode = 0;
+                    application.process = () => {
+                        assert.fail(
+                            'the Playwright dispatcher is already disposed'
+                        );
+                    };
+                    throw launchFailure;
+                },
+            }),
+            (error: unknown) => error === launchFailure
+        );
+        assert.equal(child.killCalls, 0);
+    });
+
     it('fails when process exit remains unconfirmed after forced teardown', async () => {
         const child = new FakeChildProcess();
         child.shouldExitOnKill = false;
@@ -63,6 +201,7 @@ describe('Electron process lifecycle', () => {
             close: () => new Promise<void>(() => undefined),
             process: () => child,
         };
+        captureElectronProcess(application);
 
         await assert.rejects(
             closeElectronApplicationAndConfirmExit(application, {
@@ -71,7 +210,7 @@ describe('Electron process lifecycle', () => {
             }),
             /electron-process-exit-unconfirmed/
         );
-        assert.equal(child.killCalls, 1);
+        assert.equal(child.killCalls, 2);
     });
 
     it('preserves both preparation and unconfirmed-disposal failures', async () => {
@@ -107,6 +246,7 @@ describe('Electron process lifecycle', () => {
             },
             process: () => child,
         };
+        captureElectronProcess(application);
 
         await Promise.race([
             closeElectronApplicationAndConfirmExit(application, {
@@ -129,6 +269,7 @@ describe('Electron process lifecycle', () => {
             close: () => new Promise<void>(() => undefined),
             process: () => child,
         };
+        captureElectronProcess(application);
 
         await closeElectronApplicationAndConfirmExit(application, {
             closeTimeoutMs: 1,
@@ -137,4 +278,25 @@ describe('Electron process lifecycle', () => {
         assert.equal(child.killCalls, 1);
         assert.equal(child.signalCode, 'SIGTERM');
     });
+
+    it('still observes exit after the first termination attempt throws', async () => {
+        const child = new FakeChildProcess();
+        const signals: NodeJS.Signals[] = [];
+        const application = {
+            close: () => new Promise<void>(() => undefined),
+            process: () => child,
+        };
+        captureElectronProcess(application);
+        await closeApplication(
+            application,
+            { closeTimeoutMs: 1, exitTimeoutMs: 1 },
+            (_child, signal) => {
+                signals.push(signal);
+                if (signal === 'SIGTERM') throw new Error('termination failed');
+                child.kill();
+            }
+        );
+        assert.deepEqual(signals, ['SIGTERM', 'SIGKILL']);
+        assert.equal(child.signalCode, 'SIGTERM');
+    });
 });

apps/electron-backend-e2e/src/performance/electron-process-termination.spec.ts

@@ -0,0 +1,112 @@
+import assert from 'node:assert/strict';
+import { spawn, type execFileSync } from 'node:child_process';
+import { once } from 'node:events';
+import { describe, it } from 'node:test';
+import { terminateElectronProcess } from '../electron-process-termination';
+
+describe('Electron forced termination', () => {
+    it('terminates the Windows shell and its Electron descendants together', () => {
+        let shellAlive = true;
+        let electronAlive = true;
+        const child = {
+            pid: 1234,
+            kill: () => {
+                shellAlive = false;
+                return true;
+            },
+        };
+        const run = ((file, args, options) => {
+            assert.equal(file, 'taskkill.exe');
+            assert.deepEqual(args, ['/pid', '1234', '/T', '/F']);
+            assert.equal(options?.timeout, 5000);
+            shellAlive = false;
+            electronAlive = false;
+            return Buffer.alloc(0);
+        }) as typeof execFileSync;
+
+        terminateElectronProcess(child, 'SIGTERM', 'win32', run);
+
+        assert.equal(shellAlive, false);
+        assert.equal(
+            electronAlive,
+            false,
+            'Electron must release the profile before relaunch'
+        );
+    });
+
+    it('preserves signal escalation on Unix', () => {
+        const signals: NodeJS.Signals[] = [];
+        const child = {
+            pid: 1234,
+            kill: (signal: NodeJS.Signals) => {
+                signals.push(signal);
+                return true;
+            },
+        };
+        terminateElectronProcess(child, 'SIGTERM', 'linux');
+        terminateElectronProcess(child, 'SIGKILL', 'darwin');
+        assert.deepEqual(signals, ['SIGTERM', 'SIGKILL']);
+    });
+
+    it('does not fall back to killing only the shell when tree termination fails', () => {
+        const failure = new Error('taskkill failed');
+        const child = {
+            pid: 1234,
+            kill: () => assert.fail('would orphan the Electron descendant'),
+        };
+        const run = (() => {
+            throw failure;
+        }) as typeof execFileSync;
+        assert.throws(
+            () => terminateElectronProcess(child, 'SIGTERM', 'win32', run),
+            (error) => error === failure
+        );
+    });
+
+    it(
+        'reaps a real Windows shell child instead of orphaning its descendant',
+        {
+            skip: process.platform !== 'win32',
+            timeout: 15000,
+        },
+        async () => {
+            // Match Playwright's Windows launch topology: cmd.exe -> application.
+            const shell = spawn(
+                `"${process.execPath}"`,
+                [
+                    '-e',
+                    '"setInterval(() => {}, 1000); console.log(process.pid)"',
+                ],
+                { shell: true, stdio: ['ignore', 'pipe', 'pipe'] }
+            );
+            try {
+                const [output] = await once(shell.stdout, 'data', {
+                    signal: AbortSignal.timeout(5000),
+                });
+                const descendantPid = Number(String(output).trim());
+                assert.ok(
+                    Number.isSafeInteger(descendantPid) && descendantPid > 0
+                );
+                assert.notEqual(descendantPid, shell.pid);
+                process.kill(descendantPid, 0);
+
+                terminateElectronProcess(shell);
+                await assert.rejects(
+                    async () => process.kill(descendantPid, 0),
+                    {
+                        code: 'ESRCH',
+                    }
+                );
+            } finally {
+                if (shell.exitCode === null && shell.signalCode === null) {
+                    // The successful taskkill can precede Node's exit event.
+                    try {
+                        terminateElectronProcess(shell);
+                    } catch {
+                        /* already exited */
+                    }
+                }
+            }
+        }
+    );
+});