diff --git a/.github/workflows/build-and-make.yaml b/.github/workflows/build-and-make.yaml index dafffd24e..d901a20ee 100644 --- a/.github/workflows/build-and-make.yaml +++ b/.github/workflows/build-and-make.yaml @@ -1152,7 +1152,7 @@ jobs: - name: Upload packaged frame-copy smoke diagnostics if: always() && matrix.os == 'linux' && matrix.linux_profile == 'portable' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a with: name: packaged-frame-copy-smoke path: | diff --git a/.github/workflows/e2e-tests.yaml b/.github/workflows/e2e-tests.yaml index 866208985..74ff1e198 100644 --- a/.github/workflows/e2e-tests.yaml +++ b/.github/workflows/e2e-tests.yaml @@ -69,7 +69,7 @@ jobs: run: pnpm nx build electron-backend - name: Verify Electron process cleanup - run: pnpm exec tsx --test apps/electron-backend-e2e/src/performance/electron-process-termination.spec.ts + run: pnpm exec tsx --test apps/electron-backend-e2e/src/performance/electron-process-lifecycle.spec.ts apps/electron-backend-e2e/src/performance/electron-process-termination.spec.ts - name: Install Playwright Browsers run: pnpm exec playwright install --with-deps diff --git a/apps/electron-backend-e2e/src/electron-process-lifecycle.ts b/apps/electron-backend-e2e/src/electron-process-lifecycle.ts index 51841fcd1..c48330ca8 100644 --- a/apps/electron-backend-e2e/src/electron-process-lifecycle.ts +++ b/apps/electron-backend-e2e/src/electron-process-lifecycle.ts @@ -1,8 +1,9 @@ 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 { @@ -49,7 +50,11 @@ export async function prepareElectronApplication( export async function closeElectronApplicationAndConfirmExit( application: ClosableElectronApplication, - options: ElectronExitConfirmationOptions + options: ElectronExitConfirmationOptions, + terminate: ( + child: ElectronChildProcess, + signal: NodeJS.Signals + ) => void = terminateElectronProcess ): Promise { assertTimeout(options.closeTimeoutMs); assertTimeout(options.exitTimeoutMs); @@ -82,14 +87,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(); } diff --git a/apps/electron-backend-e2e/src/electron-test-fixtures.ts b/apps/electron-backend-e2e/src/electron-test-fixtures.ts index 6fb3abc1e..3d4a5e42c 100644 --- a/apps/electron-backend-e2e/src/electron-test-fixtures.ts +++ b/apps/electron-backend-e2e/src/electron-test-fixtures.ts @@ -31,7 +31,6 @@ import { closeElectronApplicationAndConfirmExit, prepareElectronApplication, } from './electron-process-lifecycle'; -import { terminateElectronProcess } from './electron-process-termination'; export const workspaceRoot = resolve(__dirname, '../../..'); export const electronMainPath = join( @@ -565,52 +564,7 @@ export async function launchCompetingElectronInstance( export async function closeElectronApp( app: LaunchedElectronApp ): Promise { - 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) { - terminateElectronProcess(childProcess); - } - - 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' - ); - terminateElectronProcess(childProcess, '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( diff --git a/apps/electron-backend-e2e/src/performance/electron-process-lifecycle.spec.ts b/apps/electron-backend-e2e/src/performance/electron-process-lifecycle.spec.ts index 15a6d003b..ca939452b 100644 --- a/apps/electron-backend-e2e/src/performance/electron-process-lifecycle.spec.ts +++ b/apps/electron-backend-e2e/src/performance/electron-process-lifecycle.spec.ts @@ -1,13 +1,27 @@ 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, + 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 +39,25 @@ class FakeChildProcess extends EventEmitter { } describe('Electron process lifecycle', () => { + 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; + + 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'); @@ -71,7 +104,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 () => { @@ -137,4 +170,23 @@ 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[] = []; + await closeApplication( + { + close: () => new Promise(() => undefined), + process: () => child, + }, + { 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'); + }); }); diff --git a/docs/development/electron-debugging.md b/docs/development/electron-debugging.md index 611ee15d1..6281ed6b6 100644 --- a/docs/development/electron-debugging.md +++ b/docs/development/electron-debugging.md @@ -75,6 +75,9 @@ only `electronApp.process()` can leave Electron holding the temporary profile and make the next launch exit on the single-instance lock. The E2E workflow checks this against a real Windows shell and child process. Test cleanup must target only its own launch PID, never all Electron or IPTVnator processes. +Cleanup confirms process exit even if Playwright's close promise fails or +never settles. If termination fails, it retries and then throws instead of +allowing a relaunch against a potentially locked profile. The Linux portable build uploads `packaged-frame-copy-smoke` reports and traces even when the smoke fails. Check the paused-frame screenshot and trace before