diff --git a/.nx/version-plans/version-plan-1784556610341.md b/.nx/version-plans/version-plan-1784556610341.md new file mode 100644 index 00000000..bde40772 --- /dev/null +++ b/.nx/version-plans/version-plan-1784556610341.md @@ -0,0 +1,5 @@ +--- +__default__: patch +--- + +Collapses the two competing subprocess shutdown mechanisms (the global process-level SIGINT/SIGTERM net in packages/tools/src/spawn.ts and the session's own abort-driven dispose) into one session-lifetime abort signal. Long-lived child processes (Android logcat, iOS launch process and XCTest agent, web browser, Vega app) now abort on session teardown through a single signal instead of racing a separate raw-kill handler. diff --git a/packages/cli/src/wizard/index.ts b/packages/cli/src/wizard/index.ts index 5772b26b..a404682c 100644 --- a/packages/cli/src/wizard/index.ts +++ b/packages/cli/src/wizard/index.ts @@ -5,6 +5,7 @@ import { isProject, cancelPromptAndExit, promptConfirm, + ensureSignalsDeliverable, } from '@react-native-harness/tools'; import { getProjectConfig } from './projectType.js'; import { installPlatforms } from './platforms.js'; @@ -36,6 +37,11 @@ const checkForExistingConfig = async (projectRoot: string) => { }; export const runInitWizard = async () => { + // The wizard spawns subprocesses (e.g. platform installers) while a @clack + // spinner may be active, which puts stdin in raw mode and would otherwise + // swallow SIGINT until a prompt reads it. See spawn.ts's doc comment. + ensureSignalsDeliverable(); + const projectRoot = process.cwd(); if (!isProject(projectRoot)) { diff --git a/packages/jest/src/__tests__/harness-session.test.ts b/packages/jest/src/__tests__/harness-session.test.ts index 5327b0e5..4e0c5ab3 100644 --- a/packages/jest/src/__tests__/harness-session.test.ts +++ b/packages/jest/src/__tests__/harness-session.test.ts @@ -1,13 +1,15 @@ -import { describe, expect, it, vi } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import type { AppConnection } from '@react-native-harness/bridge/server'; import { getSignalExitCodeForRunState, waitForBridgeDisconnectOrTimeout, waitForNextConnected, waitForStartupCrash, + withPlatformReadyTimeout, type HarnessRunState, } from '../harness-session.js'; import type { CrashMonitor } from '../crash-monitor.js'; +import { PlatformReadyTimeoutError } from '../errors.js'; const createConnection = (): AppConnection => ({ device: { @@ -155,6 +157,66 @@ describe('waitForStartupCrash', () => { }); }); +describe('withPlatformReadyTimeout', () => { + afterEach(() => { + vi.useRealTimers(); + }); + + it('passes the real session signal to work, unmodified by the timeout', async () => { + const controller = new AbortController(); + const work = vi.fn(async (signal: AbortSignal) => { + expect(signal).toBe(controller.signal); + return 'ready'; + }); + + await expect( + withPlatformReadyTimeout({ + timeout: 1_000, + signal: controller.signal, + work, + }), + ).resolves.toBe('ready'); + expect(work).toHaveBeenCalledTimes(1); + }); + + it('throws PlatformReadyTimeoutError on timeout without aborting the session signal', async () => { + vi.useFakeTimers(); + const controller = new AbortController(); + // Never resolves within the test; simulates a hung platform init. + const work = () => new Promise(() => undefined); + + const resultPromise = withPlatformReadyTimeout({ + timeout: 1_000, + signal: controller.signal, + work, + }); + + await vi.advanceTimersByTimeAsync(1_000); + + await expect(resultPromise).rejects.toBeInstanceOf(PlatformReadyTimeoutError); + expect(controller.signal.aborted).toBe(false); + }); + + it('propagates a genuine abort from the session signal as-is', async () => { + const controller = new AbortController(); + const abortReason = new DOMException('Aborted', 'AbortError'); + const work = (signal: AbortSignal) => + new Promise((_, reject) => { + signal.addEventListener('abort', () => reject(signal.reason)); + }); + + const resultPromise = withPlatformReadyTimeout({ + timeout: 1_000, + signal: controller.signal, + work, + }); + + controller.abort(abortReason); + + await expect(resultPromise).rejects.toBe(abortReason); + }); +}); + describe('getSignalExitCodeForRunState', () => { const createRunState = ( overrides: Partial = {} diff --git a/packages/jest/src/harness-session.ts b/packages/jest/src/harness-session.ts index 583e7d5c..325b9c57 100644 --- a/packages/jest/src/harness-session.ts +++ b/packages/jest/src/harness-session.ts @@ -40,7 +40,7 @@ import { createDiagnostics, logger, getTimeoutSignal, - raceAbortSignals, + ensureSignalsDeliverable, type Diagnostics, } from '@react-native-harness/tools'; import { @@ -220,26 +220,24 @@ const waitForAbort = (signal: AbortSignal): Promise => { }); }; -const withPlatformReadyTimeout = async (options: { +// Bounds only the init *call* with a readiness timeout; `options.signal` (the +// session-lifetime signal) is passed to `work` unmodified, so a slow init +// call keeps observing real session teardown, not a synthetic timeout abort. +// On timeout this throws PlatformReadyTimeoutError without cancelling `work` +// itself — the caller is expected to abort the session signal as part of +// tearing down, which then cancels the still-inflight call cooperatively. +export const withPlatformReadyTimeout = async (options: { timeout: number; signal: AbortSignal; work: (signal: AbortSignal) => Promise; }): Promise => { const timeoutSignal = getTimeoutSignal(options.timeout); - const combinedSignal = raceAbortSignals([options.signal, timeoutSignal]); - try { - return await options.work(combinedSignal); - } catch (error) { - if ( - error instanceof DOMException && - error.name === 'AbortError' && - timeoutSignal.aborted && - !options.signal.aborted - ) { + return await Promise.race([ + options.work(options.signal), + waitForAbort(timeoutSignal).catch(() => { throw new PlatformReadyTimeoutError(options.timeout); - } - throw error; - } + }), + ]); }; type AppReadyOptions = { @@ -490,11 +488,14 @@ export const createHarnessSession = async ( platform.platformId, ); - // Single AbortController for the entire setup phase. Registered signal - // handlers abort this so that slow inflight operations (Metro init, platform - // runner startup) are cancelled promptly on SIGTERM/SIGINT. - const setupController = new AbortController(); - const onEarlySignal = () => setupController.abort(); + // Single, session-lifetime AbortController: it aborts on early SIGTERM/SIGINT + // during setup (below) and, once the session is up, in dispose()'s finally + // (unchanged lifetime). It is the one signal the plugin manager and platform + // runner see for the entire session, so its abort reaches every long-lived + // child the runner threads it into. + ensureSignalsDeliverable(); + const sessionController = new AbortController(); + const onEarlySignal = () => sessionController.abort(); process.once('SIGTERM', onEarlySignal); process.once('SIGINT', onEarlySignal); @@ -508,7 +509,7 @@ export const createHarnessSession = async ( 'session.lock.wait', () => lockManager.acquire(resourceLockKey, { - signal: setupController.signal, + signal: sessionController.signal, onWait: () => { didWaitForResourceLock = true; logRunnerWaitingInQueue(platform); @@ -539,7 +540,7 @@ export const createHarnessSession = async ( config: harnessConfig, platform, resourceLockManager: lockManager, - signal: setupController.signal, + signal: sessionController.signal, }); } catch (error) { portResolveSpan.fail(error); @@ -560,13 +561,12 @@ export const createHarnessSession = async ( platform, }); - const pluginAbortController = new AbortController(); const pluginManager = createHarnessPluginManager({ plugins: (runtimeConfig.plugins ?? []) as Array>, projectRoot, config: runtimeConfig, runner: platform, - abortSignal: pluginAbortController.signal, + abortSignal: sessionController.signal, }); const hooks = createHookQueue(); @@ -606,7 +606,7 @@ export const createHarnessSession = async ( [HARNESS_BRIDGE_PATH]: bridge.ws as unknown as MetroWebSocketEndpoint, }, }, - setupController.signal, + sessionController.signal, ).then((instance) => { sessionLogger.debug('Metro initialized'); // Attach the Metro diagnostics deriver before the eager prewarm @@ -621,7 +621,7 @@ export const createHarnessSession = async ( instance .prewarm({ platform: platform.platformId, - signal: setupController.signal, + signal: sessionController.signal, }) .then((completed) => prewarmSpan.end({ completed })) // Swallow AbortError on teardown so it never becomes an @@ -636,7 +636,7 @@ export const createHarnessSession = async ( () => withPlatformReadyTimeout({ timeout: runtimeConfig.platformReadyTimeout, - signal: setupController.signal, + signal: sessionController.signal, work: async (signal) => { return await import(platform.runner).then((module) => module.default(platform.config, runtimeConfig, { @@ -834,7 +834,7 @@ export const createHarnessSession = async ( cleanupError = error; } finally { await resourceLease.release(); - pluginAbortController.abort(); + sessionController.abort(); } sessionLogger.debug('session resources disposed'); @@ -1105,6 +1105,9 @@ export const createHarnessSession = async ( } catch (error) { process.off('SIGTERM', onEarlySignal); process.off('SIGINT', onEarlySignal); + // Cancels any inflight init work still holding sessionController.signal + // (e.g. a platform init call abandoned after a readiness timeout race). + sessionController.abort(); await Promise.allSettled([resourceLease.release(), metroPortLease?.release()]); throw error; } diff --git a/packages/platform-android/src/__tests__/app-session.test.ts b/packages/platform-android/src/__tests__/app-session.test.ts new file mode 100644 index 00000000..b52cf2fb --- /dev/null +++ b/packages/platform-android/src/__tests__/app-session.test.ts @@ -0,0 +1,76 @@ +import { describe, expect, it } from 'vitest'; +import type { Subprocess } from '@react-native-harness/tools'; +import { createAndroidAppSession } from '../app-session.js'; + +// Mimics nano-spawn: the async iterator only ends once its abort signal +// fires, matching how startLogcat's real subprocess reacts to abort. +const createAbortableLogcatProcess = (signal: AbortSignal): Subprocess => + ({ + [Symbol.asyncIterator]: () => ({ + next: () => + new Promise<{ done: true; value: undefined }>((resolve) => { + if (signal.aborted) { + resolve({ done: true, value: undefined }); + return; + } + signal.addEventListener( + 'abort', + () => resolve({ done: true, value: undefined }), + { once: true } + ); + }), + }), + catch: () => undefined, + }) as unknown as Subprocess; + +describe('createAndroidAppSession', () => { + it('combines the session-lifetime signal with its own logcat abort controller', async () => { + let capturedSignal: AbortSignal | undefined; + const controller = new AbortController(); + + const session = await createAndroidAppSession({ + appUid: 1, + bundleId: 'com.example', + startApp: async () => undefined, + stopApp: async () => undefined, + getAppPid: async () => null, + getLogcatTimestamp: async () => '00:00:00.000', + startLogcat: (_args, options) => { + capturedSignal = options.signal; + return createAbortableLogcatProcess(options.signal); + }, + signal: controller.signal, + }); + + expect(capturedSignal?.aborted).toBe(false); + + controller.abort(); + + expect(capturedSignal?.aborted).toBe(true); + + await session.dispose(); + }); + + it('still aborts the logcat signal via dispose() with no external signal provided', async () => { + let capturedSignal: AbortSignal | undefined; + + const session = await createAndroidAppSession({ + appUid: 1, + bundleId: 'com.example', + startApp: async () => undefined, + stopApp: async () => undefined, + getAppPid: async () => null, + getLogcatTimestamp: async () => '00:00:00.000', + startLogcat: (_args, options) => { + capturedSignal = options.signal; + return createAbortableLogcatProcess(options.signal); + }, + }); + + expect(capturedSignal?.aborted).toBe(false); + + await session.dispose(); + + expect(capturedSignal?.aborted).toBe(true); + }); +}); diff --git a/packages/platform-android/src/app-session.ts b/packages/platform-android/src/app-session.ts index 264865d8..330a4561 100644 --- a/packages/platform-android/src/app-session.ts +++ b/packages/platform-android/src/app-session.ts @@ -81,6 +81,13 @@ type CreateAndroidAppSessionOptions = { getDropboxOutput?: () => Promise; getExitInfo?: () => Promise; crashArtifactWriter?: CrashArtifactWriter; + /** + * Session-lifetime abort signal (see HarnessPlatformInitOptions.signal). + * Combined with the app session's own logcatAbortController so that + * aborting the session stops the logcat stream even if dispose() hasn't + * been called yet (e.g. SIGTERM during teardown of other resources). + */ + signal?: AbortSignal; }; export const createAndroidAppSession = async ({ @@ -94,6 +101,7 @@ export const createAndroidAppSession = async ({ getDropboxOutput, getExitInfo, crashArtifactWriter, + signal, }: CreateAndroidAppSessionOptions): Promise => { const emitter = createAppSessionEmitter(); const logBuffer = createBoundedLogBuffer(); @@ -189,8 +197,11 @@ export const createAndroidAppSession = async ({ const logcatTimestamp = await getLogcatTimestamp(); const sessionStartedAt = Date.now(); const logcatAbortController = new AbortController(); + const logcatSignal = signal + ? AbortSignal.any([signal, logcatAbortController.signal]) + : logcatAbortController.signal; const logcatProcess = startLogcat(getLogcatArgs(appUid, logcatTimestamp), { - signal: logcatAbortController.signal, + signal: logcatSignal, }); // Aborting is nano-spawn's own cancellation path, so the resulting // SubprocessError is settled the same way regardless of which of the diff --git a/packages/platform-android/src/instance.ts b/packages/platform-android/src/instance.ts index 67ce7051..b9cc06f8 100644 --- a/packages/platform-android/src/instance.ts +++ b/packages/platform-android/src/instance.ts @@ -301,6 +301,7 @@ export const getAndroidEmulatorPlatformInstance = async ( getDropboxOutput: () => adb.getDropboxPrint(adbId), getExitInfo: () => adb.getActivityExitInfo(adbId, config.bundleId), crashArtifactWriter: init.crashArtifactWriter, + signal: init.signal, }); }, dispose: async () => { @@ -374,6 +375,7 @@ export const getAndroidPhysicalDevicePlatformInstance = async ( getDropboxOutput: () => adb.getDropboxPrint(adbId), getExitInfo: () => adb.getActivityExitInfo(adbId, config.bundleId), crashArtifactWriter: init?.crashArtifactWriter, + signal: init?.signal, }); }, dispose: async () => { diff --git a/packages/platform-ios/src/__tests__/app-session.test.ts b/packages/platform-ios/src/__tests__/app-session.test.ts index 98f1bf87..bd9d920e 100644 --- a/packages/platform-ios/src/__tests__/app-session.test.ts +++ b/packages/platform-ios/src/__tests__/app-session.test.ts @@ -60,4 +60,65 @@ describe('createIosAppSession', () => { vi.useRealTimers(); } }); + + it('disposes the session when the session-lifetime signal aborts', async () => { + vi.useFakeTimers(); + + try { + const launchProcess = createPendingLaunchProcess(); + const isAppRunning = vi.fn<() => Promise>().mockResolvedValue(true); + const stopApp = vi.fn(async () => undefined); + const controller = new AbortController(); + + const sessionPromise = createIosAppSession({ + launch: () => launchProcess, + stopApp, + isAppRunning, + signal: controller.signal, + }); + + await vi.advanceTimersByTimeAsync(100); + const session = await sessionPromise; + + controller.abort(); + await vi.advanceTimersByTimeAsync(1000); + + await expect(session.getState()).resolves.toMatchObject({ + status: 'disposed', + }); + expect(stopApp).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + } + }); + + it('disposes immediately when created with an already-aborted signal', async () => { + vi.useFakeTimers(); + + try { + const launchProcess = createPendingLaunchProcess(); + const isAppRunning = vi.fn<() => Promise>().mockResolvedValue(true); + const stopApp = vi.fn(async () => undefined); + const controller = new AbortController(); + controller.abort(); + + const sessionPromise = createIosAppSession({ + launch: () => launchProcess, + stopApp, + isAppRunning, + signal: controller.signal, + }); + + await vi.advanceTimersByTimeAsync(100); + const session = await sessionPromise; + await vi.advanceTimersByTimeAsync(1000); + + await expect(session.getState()).resolves.toMatchObject({ + status: 'disposed', + }); + expect(stopApp).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + } + }); }); diff --git a/packages/platform-ios/src/__tests__/instance-xctest-agent.test.ts b/packages/platform-ios/src/__tests__/instance-xctest-agent.test.ts index 8515eab7..16ebd952 100644 --- a/packages/platform-ios/src/__tests__/instance-xctest-agent.test.ts +++ b/packages/platform-ios/src/__tests__/instance-xctest-agent.test.ts @@ -53,6 +53,7 @@ describe('iOS XCTest agent runner integration', () => { undefined, ); + const initSignal = new AbortController().signal; const instance = await getAppleSimulatorPlatformInstance( { name: 'ios', @@ -65,7 +66,7 @@ describe('iOS XCTest agent runner integration', () => { }, harnessConfigWithPermissionsEnabled, { - signal: new AbortController().signal, + signal: initSignal, }, ); @@ -82,6 +83,7 @@ describe('iOS XCTest agent runner integration', () => { kind: 'simulator', id: 'sim-udid', }, + signal: initSignal, }); expect(mocks.prepare).not.toHaveBeenCalled(); expect(mocks.ensureStarted).toHaveBeenCalledTimes(1); diff --git a/packages/platform-ios/src/app-session.ts b/packages/platform-ios/src/app-session.ts index 30c69157..6d7a4ba9 100644 --- a/packages/platform-ios/src/app-session.ts +++ b/packages/platform-ios/src/app-session.ts @@ -5,18 +5,25 @@ import { type AppSessionState, type AppleAppLaunchOptions, } from '@react-native-harness/platforms'; -import { logger, type Subprocess } from '@react-native-harness/tools'; +import { logger, terminate, type Subprocess } from '@react-native-harness/tools'; import type { IosCrashReporter } from './crash-reporter.js'; const iosAppSessionLogger = logger.child('ios-app-session'); const APP_EXIT_POLL_INTERVAL_MS = 1000; const LAUNCH_FAILURE_SETTLE_MS = 100; +const LAUNCH_PROCESS_FORCE_KILL_AFTER_MS = 2000; type CreateIosAppSessionOptions = { launch: () => Subprocess; stopApp: () => Promise; isAppRunning: () => Promise; crashReporter?: IosCrashReporter; + /** + * Session-lifetime abort signal (see HarnessPlatformInitOptions.signal). + * Terminates the `--console` launch process on session teardown, in + * addition to the normal dispose() path. + */ + signal?: AbortSignal; }; const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); @@ -26,6 +33,7 @@ export const createIosAppSession = async ({ stopApp, isAppRunning, crashReporter, + signal, }: CreateIosAppSessionOptions): Promise => { const emitter = createAppSessionEmitter(); const logBuffer = createBoundedLogBuffer(); @@ -106,26 +114,31 @@ export const createIosAppSession = async ({ throw new Error('The iOS app launch finished before the app was running.'); } - return { - dispose: async () => { - if (disposed) { - return; - } + const dispose = async () => { + if (disposed) { + return; + } + + disposed = true; + stopPolling = true; + state = { status: 'disposed', occurredAt: Date.now() }; + emitter.clear(); - disposed = true; - stopPolling = true; - state = { status: 'disposed', occurredAt: Date.now() }; - emitter.clear(); + await terminate(launchProcess, { + forceAfterMs: LAUNCH_PROCESS_FORCE_KILL_AFTER_MS, + }); + await stopApp(); + await Promise.allSettled([logTask, exitTask, pollTask]); + }; - try { - (await launchProcess.nodeChildProcess).kill(); - } catch { - // Ignore termination failures for already-ended launch streams. - } + if (signal?.aborted) { + void dispose(); + } else { + signal?.addEventListener('abort', () => void dispose(), { once: true }); + } - await stopApp(); - await Promise.allSettled([logTask, exitTask, pollTask]); - }, + return { + dispose, getState: async () => state, getLogs: () => logBuffer.getLogs(), getCrashDetails: crashReporter?.getCrashDetails, diff --git a/packages/platform-ios/src/instance.ts b/packages/platform-ios/src/instance.ts index 62f8a484..227ca48e 100644 --- a/packages/platform-ios/src/instance.ts +++ b/packages/platform-ios/src/instance.ts @@ -135,6 +135,7 @@ export const getAppleSimulatorPlatformInstance = async ( id: udid, }, capabilities: [createPermissionPromptAutoAcceptCapability()], + signal: init.signal, }) : null; @@ -180,6 +181,7 @@ export const getAppleSimulatorPlatformInstance = async ( stopApp: () => simctl.stopApp(udid, config.bundleId), isAppRunning: () => simctl.isAppRunning(udid, config.bundleId), crashReporter, + signal: init.signal, }); }, dispose: async () => { @@ -249,6 +251,7 @@ export const getApplePhysicalDevicePlatformInstance = async ( codeSign: config.device.codeSign, }, capabilities: [createPermissionPromptAutoAcceptCapability()], + signal: init?.signal, }) : null; @@ -291,6 +294,7 @@ export const getApplePhysicalDevicePlatformInstance = async ( stopApp: () => devicectl.stopApp(deviceId, config.bundleId), isAppRunning: () => devicectl.isAppRunning(deviceId, config.bundleId), crashReporter, + signal: init?.signal, }); }, dispose: async () => { diff --git a/packages/platform-ios/src/xctest-agent.ts b/packages/platform-ios/src/xctest-agent.ts index cbc1b220..e967da19 100644 --- a/packages/platform-ios/src/xctest-agent.ts +++ b/packages/platform-ios/src/xctest-agent.ts @@ -4,6 +4,7 @@ import { getAvailablePort, logger, spawn, + terminate, type Subprocess, } from '@react-native-harness/tools'; import fs from 'node:fs'; @@ -833,44 +834,13 @@ const waitForChildProcessExit = async (subprocess: Subprocess) => { const stopProcess = async (options: { process: Subprocess | null; - processTask: Promise | null; shutdownTimeoutMs: number; - targetKind: XCTestAgentTarget['kind']; }) => { if (!options.process) { return; } - let childProcess: Awaited; - - try { - childProcess = await options.process.nodeChildProcess; - } catch { - return; - } - - childProcess.kill('SIGTERM'); - - if ( - await waitForShutdown({ - processTask: options.processTask, - shutdownTimeoutMs: options.shutdownTimeoutMs, - }) - ) { - return; - } - - xctestAgentLogger.warn( - 'XCTest agent session for %s target did not stop after %dms; forcing shutdown', - options.targetKind, - options.shutdownTimeoutMs - ); - childProcess.kill('SIGKILL'); - - await waitForShutdown({ - processTask: options.processTask, - shutdownTimeoutMs: options.shutdownTimeoutMs, - }); + await terminate(options.process, { forceAfterMs: options.shutdownTimeoutMs }); }; const toTestRunnerEnv = (env: Record): Record => @@ -944,6 +914,8 @@ export const createXCTestAgentController = (options: { port?: number; shutdownTimeoutMs?: number; startupTimeoutMs?: number; + /** Session-lifetime abort signal (see HarnessPlatformInitOptions.signal). */ + signal?: AbortSignal; }): XCTestAgentController => { const { target } = options; const capabilities = options.capabilities ?? []; @@ -1129,12 +1101,7 @@ export const createXCTestAgentController = (options: { ); await transport.dispose(); agentClient = null; - await stopProcess({ - process: currentProcess, - processTask, - shutdownTimeoutMs, - targetKind: target.kind, - }); + await stopProcess({ process: currentProcess, shutdownTimeoutMs }); throw error; } }; @@ -1197,20 +1164,33 @@ export const createXCTestAgentController = (options: { } await currentClient?.dispose(); - await stopProcess({ - process: currentProcess, - processTask: currentProcessTask, - shutdownTimeoutMs, - targetKind: target.kind, - }); + await stopProcess({ process: currentProcess, shutdownTimeoutMs }); + }; + + // Guards against the abort listener below redundantly re-running stop() + // when the caller has already disposed the controller explicitly (the + // normal teardown path always aborts the session signal afterwards too). + let disposed = false; + const dispose = async () => { + if (disposed) { + return; + } + disposed = true; + await stop(); }; + if (options.signal?.aborted) { + void dispose(); + } else { + options.signal?.addEventListener('abort', () => void dispose(), { + once: true, + }); + } + return { prepare, ensureStarted, stop, - dispose: async () => { - await stop(); - }, + dispose, }; }; diff --git a/packages/platform-vega/src/runner.ts b/packages/platform-vega/src/runner.ts index 4b637e30..9e005776 100644 --- a/packages/platform-vega/src/runner.ts +++ b/packages/platform-vega/src/runner.ts @@ -19,7 +19,6 @@ const getVegaRunner = async ( config: VegaPlatformConfig, init?: HarnessPlatformInitOptions ): Promise => { - void init; const parsedConfig = VegaPlatformConfigSchema.parse(config); const deviceId = parsedConfig.device.deviceId; const bundleId = parsedConfig.bundleId; @@ -35,6 +34,20 @@ const getVegaRunner = async ( throw new AppNotInstalledError(bundleId, deviceId); } + let currentAppSession: AppSession | null = null; + + // Session-lifetime signal (see HarnessPlatformInitOptions.signal): stop the + // poll loop and the app on session teardown, in addition to the normal + // dispose() path. + const disposeCurrentAppSessionOnAbort = () => void currentAppSession?.dispose(); + if (init?.signal.aborted) { + disposeCurrentAppSessionOnAbort(); + } else { + init?.signal.addEventListener('abort', disposeCurrentAppSessionOnAbort, { + once: true, + }); + } + return { createAppSession: async (): Promise => { await kepler.stopApp(deviceId, bundleId); @@ -59,7 +72,7 @@ const getVegaRunner = async ( } })(); - return { + const session: AppSession = { dispose: async () => { if (disposed) { return; @@ -77,6 +90,9 @@ const getVegaRunner = async ( addListener: emitter.addListener, removeListener: emitter.removeListener, }; + + currentAppSession = session; + return session; }, dispose: async () => { await kepler.stopApp(deviceId, bundleId); diff --git a/packages/platform-web/src/runner.ts b/packages/platform-web/src/runner.ts index affac932..314ceeba 100644 --- a/packages/platform-web/src/runner.ts +++ b/packages/platform-web/src/runner.ts @@ -12,12 +12,24 @@ const getWebRunner = async ( config: WebPlatformConfig, init?: HarnessPlatformInitOptions ): Promise => { - void init; const parsedConfig = WebPlatformConfigSchema.parse(config); let browser: Browser | null = null; let page: Page | null = null; + // Session-lifetime signal (see HarnessPlatformInitOptions.signal): close the + // browser on session teardown, in addition to the normal dispose() path. + const closeBrowserOnAbort = () => { + void browser?.close(); + browser = null; + page = null; + }; + if (init?.signal.aborted) { + closeBrowserOnAbort(); + } else { + init?.signal.addEventListener('abort', closeBrowserOnAbort, { once: true }); + } + const launchBrowser = async () => { const browserType = { chromium, diff --git a/packages/platforms/src/types.ts b/packages/platforms/src/types.ts index e3027b32..c079fe78 100644 --- a/packages/platforms/src/types.ts +++ b/packages/platforms/src/types.ts @@ -125,6 +125,14 @@ export type HarnessPlatformRunner = { }; export type HarnessPlatformInitOptions = { + /** + * Session-lifetime abort signal: it aborts when the harness session + * disposes (normal shutdown, error, or SIGINT/SIGTERM), not on a + * readiness/init timeout. Runners should hold onto it for as long as the + * session lives — e.g. to abort long-lived child processes and streams + * (device logs, launch processes, agent connections) on session teardown — + * rather than treating it as scoped to the init() call itself. + */ signal: AbortSignal; crashArtifactWriter?: CrashArtifactWriter; /** diff --git a/packages/tools/src/__tests__/terminate.test.ts b/packages/tools/src/__tests__/terminate.test.ts new file mode 100644 index 00000000..8e663956 --- /dev/null +++ b/packages/tools/src/__tests__/terminate.test.ts @@ -0,0 +1,90 @@ +import { EventEmitter } from 'node:events'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { terminate } from '../terminate.js'; +import type { Subprocess } from 'nano-spawn'; + +type MockChildProcess = EventEmitter & { + exitCode: number | null; + signalCode: string | null; + kill: ReturnType; +}; + +const createMockChildProcess = (options?: { + exitOnKill?: 'SIGTERM' | 'SIGKILL'; +}): MockChildProcess => { + const childProcess = new EventEmitter() as MockChildProcess; + childProcess.exitCode = null; + childProcess.signalCode = null; + childProcess.kill = vi.fn((signal: string) => { + if (options?.exitOnKill === signal) { + childProcess.signalCode = signal; + childProcess.emit('close'); + } + return true; + }); + return childProcess; +}; + +const createMockSubprocess = (childProcess: MockChildProcess): Subprocess => + ({ + nodeChildProcess: Promise.resolve(childProcess), + }) as unknown as Subprocess; + +afterEach(() => { + vi.useRealTimers(); +}); + +describe('terminate', () => { + it('resolves immediately when the process already exited', async () => { + const childProcess = createMockChildProcess(); + childProcess.exitCode = 0; + + await terminate(createMockSubprocess(childProcess), { forceAfterMs: 1_000 }); + + expect(childProcess.kill).toHaveBeenCalledTimes(1); + expect(childProcess.kill).toHaveBeenCalledWith('SIGTERM'); + }); + + it('sends SIGTERM and resolves once the process exits gracefully', async () => { + const childProcess = createMockChildProcess({ exitOnKill: 'SIGTERM' }); + + await terminate(createMockSubprocess(childProcess), { forceAfterMs: 1_000 }); + + expect(childProcess.kill).toHaveBeenCalledWith('SIGTERM'); + expect(childProcess.kill).toHaveBeenCalledTimes(1); + }); + + it('escalates to SIGKILL if the process does not exit before forceAfterMs', async () => { + vi.useFakeTimers(); + const childProcess = createMockChildProcess({ exitOnKill: 'SIGKILL' }); + + const done = terminate(createMockSubprocess(childProcess), { + forceAfterMs: 1_000, + }); + + await vi.advanceTimersByTimeAsync(1_000); + await done; + + expect(childProcess.kill).toHaveBeenNthCalledWith(1, 'SIGTERM'); + expect(childProcess.kill).toHaveBeenNthCalledWith(2, 'SIGKILL'); + }); + + it('clears the force-kill timer once the process exits gracefully', async () => { + vi.useFakeTimers(); + const childProcess = createMockChildProcess({ exitOnKill: 'SIGTERM' }); + + await terminate(createMockSubprocess(childProcess), { forceAfterMs: 1_000 }); + + expect(vi.getTimerCount()).toBe(0); + }); + + it('does nothing if resolving nodeChildProcess rejects', async () => { + const subprocess = { + nodeChildProcess: Promise.reject(new Error('spawn failed')), + } as unknown as Subprocess; + + await expect( + terminate(subprocess, { forceAfterMs: 1_000 }) + ).resolves.toBeUndefined(); + }); +}); diff --git a/packages/tools/src/index.ts b/packages/tools/src/index.ts index 41528a22..c73b7e56 100644 --- a/packages/tools/src/index.ts +++ b/packages/tools/src/index.ts @@ -4,6 +4,7 @@ export * from './color.js'; export * from './logger.js'; export * from './prompts.js'; export * from './spawn.js'; +export * from './terminate.js'; export * from './react-native.js'; export * from './error.js'; export * from './events.js'; diff --git a/packages/tools/src/spawn.ts b/packages/tools/src/spawn.ts index 4b183444..e6642c1b 100644 --- a/packages/tools/src/spawn.ts +++ b/packages/tools/src/spawn.ts @@ -5,6 +5,26 @@ import { logger } from './logger.js'; export type SpawnOptions = Options; const spawnLogger = logger.child('spawn'); +let signalsDeliverableEnsured = false; + +/** + * Restores default stdin raw-mode handling so SIGINT/SIGTERM keep being + * delivered to the process. @clack/prompts puts stdin into raw mode for its + * spinner/prompt UI, which otherwise suppresses those signals. Call this once + * per process, before relying on signal handlers. + */ +export const ensureSignalsDeliverable = (): void => { + if (signalsDeliverableEnsured) { + return; + } + signalsDeliverableEnsured = true; + + if (process.stdin.isTTY) { + // https://stackoverflow.com/questions/53049939/node-daemon-wont-start-with-process-stdin-setrawmodetrue/53050098#53050098 + process.stdin.setRawMode(false); + } +}; + export const spawn = ( file: string, args?: readonly string[], @@ -18,10 +38,7 @@ export const spawn = ( }; const command = [file, ...(args ?? [])].join(' '); spawnLogger.debug('running command: %s', command); - const childProcess = nanoSpawn(file, args, { ...defaultOptions, ...options }); - - setupChildProcessCleanup(childProcess); - return childProcess; + return nanoSpawn(file, args, { ...defaultOptions, ...options }); }; export const spawnAndForget = async ( @@ -37,76 +54,3 @@ export const spawnAndForget = async ( }; export { Subprocess, SubprocessError }; - -const activeChildProcesses = new Set(); -let isProcessCleanupInstalled = false; -let isTerminating = false; - -type CleanupSignal = 'SIGINT' | 'SIGTERM'; - -const SIGNAL_EXIT_CODES: Record = { - SIGINT: 130, - SIGTERM: 143, -}; - -const terminateActiveChildren = async () => { - const children = [...activeChildProcesses]; - - await Promise.allSettled( - children.map(async (childProcess) => { - try { - (await childProcess.nodeChildProcess).kill(); - } catch { - // Ignore cleanup failures while shutting down. - } - }) - ); -}; - -const installProcessCleanup = () => { - if (isProcessCleanupInstalled) { - return; - } - - isProcessCleanupInstalled = true; - - const terminate = async (signal: CleanupSignal) => { - if (isTerminating) { - return; - } - - isTerminating = true; - const shouldExitAfterCleanup = process.listenerCount(signal) <= 1; - - await terminateActiveChildren(); - - if (shouldExitAfterCleanup) { - process.exit(process.exitCode ?? SIGNAL_EXIT_CODES[signal]); - } - }; - - process.on('SIGINT', () => { - void terminate('SIGINT'); - }); - process.on('SIGTERM', () => { - void terminate('SIGTERM'); - }); -}; - -const setupChildProcessCleanup = (childProcess: Subprocess) => { - // https://stackoverflow.com/questions/53049939/node-daemon-wont-start-with-process-stdin-setrawmodetrue/53050098#53050098 - if (process.stdin.isTTY) { - // overwrite @clack/prompts setting raw mode for spinner and prompts, - // which prevents listening for SIGINT and SIGTERM - process.stdin.setRawMode(false); - } - - installProcessCleanup(); - activeChildProcesses.add(childProcess); - - const cleanup = () => { - activeChildProcesses.delete(childProcess); - }; - - childProcess.nodeChildProcess.finally(cleanup); -}; diff --git a/packages/tools/src/terminate.ts b/packages/tools/src/terminate.ts new file mode 100644 index 00000000..23f7998d --- /dev/null +++ b/packages/tools/src/terminate.ts @@ -0,0 +1,80 @@ +import type { Subprocess } from 'nano-spawn'; + +// Cancellable so the winning race branch can clear the timer instead of +// leaving a ref'd setTimeout alive for `forceAfterMs` after the process +// already exited gracefully. +const delay = (ms: number): { promise: Promise; cancel: () => void } => { + let timer: ReturnType; + const promise = new Promise((resolve) => { + timer = setTimeout(resolve, ms); + }); + return { promise, cancel: () => clearTimeout(timer) }; +}; + +const waitForExit = ( + childProcess: Awaited +): Promise => { + if (childProcess.exitCode !== null || childProcess.signalCode !== null) { + return Promise.resolve(); + } + + return new Promise((resolve) => { + const cleanup = () => { + childProcess.off('close', finish); + childProcess.off('error', finish); + }; + const finish = () => { + cleanup(); + resolve(); + }; + childProcess.once('close', finish); + childProcess.once('error', finish); + }); +}; + +export type TerminateOptions = { + /** How long to wait after SIGTERM before escalating to SIGKILL. */ + forceAfterMs: number; +}; + +/** + * Terminates a subprocess gracefully: sends SIGTERM, waits up to + * `forceAfterMs` for it to exit, and escalates to SIGKILL if it hasn't. + * Resolves once the process has exited either way. + */ +export const terminate = async ( + subprocess: Subprocess, + { forceAfterMs }: TerminateOptions +): Promise => { + let childProcess: Awaited; + try { + childProcess = await subprocess.nodeChildProcess; + } catch { + return; + } + + const exited = waitForExit(childProcess); + + try { + childProcess.kill('SIGTERM'); + } catch { + // Ignore termination failures for already-ended processes. + } + + const timedOut = Symbol('timedOut'); + const forceKill = delay(forceAfterMs); + const result = await Promise.race([ + exited.then(() => 'exited' as const), + forceKill.promise.then(() => timedOut), + ]); + forceKill.cancel(); + + if (result === timedOut) { + try { + childProcess.kill('SIGKILL'); + } catch { + // Ignore termination failures for already-ended processes. + } + await exited; + } +};