diff --git a/.nx/version-plans/fix-cancel-race-timers-and-listeners.md b/.nx/version-plans/fix-cancel-race-timers-and-listeners.md new file mode 100644 index 0000000..d2a9e1d --- /dev/null +++ b/.nx/version-plans/fix-cancel-race-timers-and-listeners.md @@ -0,0 +1,10 @@ +--- +__default__: patch +--- + +Harness runs on iOS no longer linger for up to ~25 seconds after tests finish +printing results. Uncancelled timers and abort listeners left over from +internal `Promise.race` calls (XCTest agent shutdown, Android/iOS/Vega app +session polling, and startup crash detection) could keep the process alive +past test completion; they are now cleaned up as soon as the other side of +the race settles. diff --git a/packages/jest/src/__tests__/harness-session.test.ts b/packages/jest/src/__tests__/harness-session.test.ts index 4e0c5ab..b800e82 100644 --- a/packages/jest/src/__tests__/harness-session.test.ts +++ b/packages/jest/src/__tests__/harness-session.test.ts @@ -155,6 +155,40 @@ describe('waitForStartupCrash', () => { await expect(waitPromise).rejects.toThrow('Aborted'); expect(watch).not.toHaveBeenCalled(); }); + + it('removes its abort listener on the session signal once the crash watch wins', async () => { + let rejectWatch!: (error: unknown) => void; + const cancel = vi.fn(); + const watch = vi.fn(() => ({ + promise: new Promise((_, reject) => { + rejectWatch = reject; + }), + cancel, + })); + const crashMonitor = { watch } as unknown as CrashMonitor; + const controller = new AbortController(); + const addSpy = vi.spyOn(controller.signal, 'addEventListener'); + const removeSpy = vi.spyOn(controller.signal, 'removeEventListener'); + + const waitPromise = waitForStartupCrash({ + crashMonitor, + detectNativeCrashes: true, + testFilePath: '/test.harness.ts', + signal: controller.signal, + }); + + // The crash watch wins the race, not the session signal. + rejectWatch(new Error('native crash detected')); + + await expect(waitPromise).rejects.toThrow('native crash detected'); + expect(cancel).toHaveBeenCalledTimes(1); + + // Regression: forgetting to cancel the losing waitForAbort() left its + // listener attached to this session-lifetime signal for the rest of + // the session — one leaked listener per test file. + expect(addSpy).toHaveBeenCalledTimes(1); + expect(removeSpy).toHaveBeenCalledTimes(1); + }); }); describe('withPlatformReadyTimeout', () => { diff --git a/packages/jest/src/harness-session.ts b/packages/jest/src/harness-session.ts index 53e8828..8a740e9 100644 --- a/packages/jest/src/harness-session.ts +++ b/packages/jest/src/harness-session.ts @@ -41,6 +41,7 @@ import { logger, getTimeoutSignal, ensureSignalsDeliverable, + waitForAbort, type Diagnostics, } from '@react-native-harness/tools'; import { @@ -204,22 +205,6 @@ export const getSignalExitCodeForRunState = ( // Internal helpers // --------------------------------------------------------------------------- -const createAbortError = () => - new DOMException('The operation was aborted', 'AbortError'); - -const waitForAbort = (signal: AbortSignal): Promise => { - if (signal.aborted) { - return Promise.reject(signal.reason ?? createAbortError()); - } - return new Promise((_, reject) => { - signal.addEventListener( - 'abort', - () => reject(signal.reason ?? createAbortError()), - { once: true }, - ); - }); -}; - // 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. @@ -232,12 +217,17 @@ export const withPlatformReadyTimeout = async (options: { work: (signal: AbortSignal) => Promise; }): Promise => { const timeoutSignal = getTimeoutSignal(options.timeout); - return await Promise.race([ - options.work(options.signal), - waitForAbort(timeoutSignal).catch(() => { - throw new PlatformReadyTimeoutError(options.timeout); - }), - ]); + const abortWait = waitForAbort(timeoutSignal); + try { + return await Promise.race([ + options.work(options.signal), + abortWait.promise.catch(() => { + throw new PlatformReadyTimeoutError(options.timeout); + }), + ]); + } finally { + abortWait.cancel(); + } }; type AppReadyOptions = { @@ -264,15 +254,17 @@ export const waitForStartupCrash = async ({ signal: AbortSignal; }) => { if (!detectNativeCrashes) { - return await waitForAbort(signal); + return await waitForAbort(signal).promise; } const watch = crashMonitor.watch(testFilePath, 'startup'); watch.promise.catch(ignorePromiseRejection); // suppress unhandled-rejection when abort wins race + const abortWait = waitForAbort(signal); try { - return await Promise.race([watch.promise, waitForAbort(signal)]); + return await Promise.race([watch.promise, abortWait.promise]); } finally { watch.cancel(); + abortWait.cancel(); } }; diff --git a/packages/platform-android/src/__tests__/adb-wait.test.ts b/packages/platform-android/src/__tests__/adb-wait.test.ts new file mode 100644 index 0000000..f05abb1 --- /dev/null +++ b/packages/platform-android/src/__tests__/adb-wait.test.ts @@ -0,0 +1,95 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + spawn: vi.fn(), +})); + +// The global test/setup.ts mocks '../adb.js' wholesale (including +// waitForEmulator itself), which is right for consumers of this module but +// defeats the purpose here: this suite exercises the *real* polling loop +// (and its private waitWithSignal helper) to catch timer/listener leaks. +// Re-declaring the mock for this file restores the actual implementation, +// while `spawn` and `getAdbBinaryPath` (genuine cross-module imports) are +// swapped out to keep `getDeviceIds()` from touching a real adb binary. +vi.mock('../adb.js', async () => { + const actual = await vi.importActual( + '../adb.js' + ); + + return actual; +}); + +vi.mock('../environment.js', async () => { + const actual = await vi.importActual( + '../environment.js' + ); + + return { ...actual, getAdbBinaryPath: () => '/fake/sdk/platform-tools/adb' }; +}); + +vi.mock('@react-native-harness/tools', async () => { + const actual = await vi.importActual< + typeof import('@react-native-harness/tools') + >('@react-native-harness/tools'); + + return { ...actual, spawn: mocks.spawn }; +}); + +const { waitForEmulator } = await import('../adb.js'); + +afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); +}); + +describe('waitForEmulator polling', () => { + it('does not leak an abort listener per poll iteration when the delay wins', async () => { + vi.useFakeTimers(); + // No matching device on any poll: `adb devices` reports an empty list. + mocks.spawn.mockResolvedValue({ stdout: 'List of devices attached\n' }); + + const controller = new AbortController(); + const addSpy = vi.spyOn(controller.signal, 'addEventListener'); + const removeSpy = vi.spyOn(controller.signal, 'removeEventListener'); + + const resultPromise = waitForEmulator('Test_AVD', controller.signal); + resultPromise.catch(() => undefined); + + for (let i = 0; i < 3; i += 1) { + await vi.advanceTimersByTimeAsync(1_000); + } + + // Regression: each 1s poll iteration used to add an 'abort' listener + // to the local waitForAbort helper that was never removed when the + // delay (not the abort) won the race, leaking one listener per second + // of polling against this session-lifetime signal. + expect(addSpy.mock.calls.length).toBeGreaterThanOrEqual(3); + const addCallsBeforeAbort = addSpy.mock.calls.length; + + controller.abort(new Error('teardown')); + await expect(resultPromise).rejects.toThrow('teardown'); + + // Every listener added while losing the race — including the one still + // pending when the abort itself won — must be removed again. + expect(removeSpy).toHaveBeenCalledTimes(addCallsBeforeAbort); + }); + + it('clears the pending 1s poll timer when the signal aborts mid-wait', async () => { + vi.useFakeTimers(); + mocks.spawn.mockResolvedValue({ stdout: 'List of devices attached\n' }); + + const controller = new AbortController(); + const resultPromise = waitForEmulator('Test_AVD', controller.signal); + resultPromise.catch(() => undefined); + + // Let the loop enter its 1s wait, then abort before the timer fires. + await vi.advanceTimersByTimeAsync(0); + controller.abort(new Error('teardown')); + + await expect(resultPromise).rejects.toThrow('teardown'); + // Regression: the bare setTimeout-backed `wait()` used to have no + // handle to clear, so it stayed alive in the event loop for the rest + // of the 1s interval even after the abort branch won. + expect(vi.getTimerCount()).toBe(0); + }); +}); diff --git a/packages/platform-android/src/adb.ts b/packages/platform-android/src/adb.ts index 50e9250..10bb4f5 100644 --- a/packages/platform-android/src/adb.ts +++ b/packages/platform-android/src/adb.ts @@ -1,8 +1,10 @@ import { type AndroidAppLaunchOptions } from '@react-native-harness/platforms'; import { + delay, logger, spawn, SubprocessError, + waitForAbort, type Subprocess, } from '@react-native-harness/tools'; import { spawn as nodeSpawn } from 'node:child_process'; @@ -34,28 +36,6 @@ import { AdbPermissionGrantError, } from './adb-errors.js'; -const wait = async (ms: number): Promise => { - await new Promise((resolve) => { - setTimeout(resolve, ms); - }); -}; - -const waitForAbort = (signal: AbortSignal): Promise => { - if (signal.aborted) { - return Promise.reject(signal.reason); - } - - return new Promise((_, reject) => { - signal.addEventListener( - 'abort', - () => { - reject(signal.reason); - }, - { once: true } - ); - }); -}; - const waitWithSignal = async ( ms: number, signal: AbortSignal @@ -64,7 +44,18 @@ const waitWithSignal = async ( throw signal.reason; } - await Promise.race([wait(ms), waitForAbort(signal)]); + // Both sides of this race must be torn down explicitly when they lose: + // an uncancelled timer keeps a ref'd Timeout alive, and an uncancelled + // abort wait leaks a listener on `signal`, which is a session-lifetime + // signal here and gets polled against once per second. + const timer = delay(ms); + const abort = waitForAbort(signal); + try { + await Promise.race([timer.promise, abort.promise]); + } finally { + timer.cancel(); + abort.cancel(); + } }; const EMULATOR_STARTUP_OBSERVATION_TIMEOUT_MS = 5000; @@ -617,13 +608,19 @@ export const startEmulator = async ( throw error; }); - const observationTimeout = wait(EMULATOR_STARTUP_OBSERVATION_TIMEOUT_MS).then( + const observationTimer = delay(EMULATOR_STARTUP_OBSERVATION_TIMEOUT_MS); + const observationTimeout = observationTimer.promise.then( () => 'timeout' as const ); try { await Promise.race([earlyExit, observedBoot, observationTimeout]); } finally { + // If `earlyExit` or `observedBoot` won the race, this timer would + // otherwise sit ref'd in the event loop for the rest of its 5s; + // cancelling here is a no-op on the happy "proceed detached" path + // where the timer itself won. + observationTimer.cancel(); cleanup(); } diff --git a/packages/platform-ios/src/__tests__/app-session.test.ts b/packages/platform-ios/src/__tests__/app-session.test.ts index bd9d920..b91c26f 100644 --- a/packages/platform-ios/src/__tests__/app-session.test.ts +++ b/packages/platform-ios/src/__tests__/app-session.test.ts @@ -61,6 +61,39 @@ describe('createIosAppSession', () => { } }); + it('resolves dispose() without waiting out the pending poll delay', async () => { + vi.useFakeTimers(); + + try { + const launchProcess = createPendingLaunchProcess(); + const isAppRunning = vi.fn<() => Promise>().mockResolvedValue(true); + const stopApp = vi.fn(async () => undefined); + + const sessionPromise = createIosAppSession({ + launch: () => launchProcess, + stopApp, + isAppRunning, + }); + + await vi.advanceTimersByTimeAsync(100); + const session = await sessionPromise; + + // Let the poll loop enter its 1s wait before tearing down. + await vi.advanceTimersByTimeAsync(0); + + // Regression: dispose() used to await the poll task directly, which + // was blocked on an uncancelled 1s sleep(). With fake timers and no + // advanceTimersByTimeAsync call left below, this await only resolves + // if dispose() itself resolves the pending poll delay instead of + // waiting for the underlying timer to fire. + await session.dispose(); + + expect(vi.getTimerCount()).toBe(0); + } finally { + vi.useRealTimers(); + } + }); + it('disposes the session when the session-lifetime signal aborts', async () => { vi.useFakeTimers(); diff --git a/packages/platform-ios/src/__tests__/xctest-agent.test.ts b/packages/platform-ios/src/__tests__/xctest-agent.test.ts index f38057a..e26ca82 100644 --- a/packages/platform-ios/src/__tests__/xctest-agent.test.ts +++ b/packages/platform-ios/src/__tests__/xctest-agent.test.ts @@ -514,6 +514,34 @@ describe('xctest-agent orchestration', () => { expect(mocks.kill).not.toHaveBeenCalled(); }); + it('clears the graceful-shutdown timers once shutdown succeeds, leaving no pending timer', async () => { + vi.useFakeTimers(); + + try { + const controller = createXCTestAgentController({ + port: 49154, + shutdownTimeoutMs: 30_000, + target: { + kind: 'simulator', + id: 'sim-timeout', + }, + }); + + await controller.ensureStarted(); + await controller.dispose(); + + expect(mocks.shutdown).toHaveBeenCalledTimes(1); + expect(mocks.kill).not.toHaveBeenCalled(); + // Regression coverage for the process lingering ~25s after a harness + // run completed: waitForShutdown/waitForGracefulShutdown each raced a + // 30s delay() against the real shutdown and never cleared it when the + // shutdown won, leaving two ref'd Timeouts alive in the event loop. + expect(vi.getTimerCount()).toBe(0); + } finally { + vi.useRealTimers(); + } + }); + it('kills the agent process when graceful shutdown times out', async () => { mocks.shutdown.mockResolvedValue(undefined); diff --git a/packages/platform-ios/src/app-session.ts b/packages/platform-ios/src/app-session.ts index 6d7a4ba..94576b0 100644 --- a/packages/platform-ios/src/app-session.ts +++ b/packages/platform-ios/src/app-session.ts @@ -5,7 +5,12 @@ import { type AppSessionState, type AppleAppLaunchOptions, } from '@react-native-harness/platforms'; -import { logger, terminate, type Subprocess } from '@react-native-harness/tools'; +import { + delay, + logger, + terminate, + type Subprocess, +} from '@react-native-harness/tools'; import type { IosCrashReporter } from './crash-reporter.js'; const iosAppSessionLogger = logger.child('ios-app-session'); @@ -26,8 +31,6 @@ type CreateIosAppSessionOptions = { signal?: AbortSignal; }; -const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); - export const createIosAppSession = async ({ launch, stopApp, @@ -42,6 +45,35 @@ export const createIosAppSession = async ({ let disposed = false; let stopPolling = false; let hasObservedRunning = false; + let pollDelayTimeout: ReturnType | null = null; + let resolvePollDelay: (() => void) | null = null; + + // Unlike a raced delay() loser (which is fine to just discard), this wait + // is directly `await`ed by pollTask with nothing else racing it, so + // cancelling it must also resolve the promise immediately — otherwise + // dispose() blocks on Promise.allSettled([...pollTask]) for up to + // APP_EXIT_POLL_INTERVAL_MS instead of returning right away. + const waitForNextPoll = () => + new Promise((resolve) => { + resolvePollDelay = () => { + resolvePollDelay = null; + pollDelayTimeout = null; + resolve(); + }; + + pollDelayTimeout = setTimeout(() => { + resolvePollDelay?.(); + }, APP_EXIT_POLL_INTERVAL_MS); + }); + + const cancelPendingPollDelay = () => { + if (pollDelayTimeout) { + clearTimeout(pollDelayTimeout); + pollDelayTimeout = null; + } + + resolvePollDelay?.(); + }; const setExited = (reason: 'observed-exit' | 'process-gone') => { if (disposed || state.status !== 'running') { @@ -93,21 +125,32 @@ export const createIosAppSession = async ({ iosAppSessionLogger.debug('iOS app session poll failed', error); } - await sleep(APP_EXIT_POLL_INTERVAL_MS); + if (stopPolling) { + return; + } + + await waitForNextPoll(); } })(); - const launchSettled = await Promise.race([ - launchProcess.then( - () => 'settled' as const, - () => 'settled' as const - ), - sleep(LAUNCH_FAILURE_SETTLE_MS).then(() => 'running' as const), - ]); + const launchFailureSettleTimer = delay(LAUNCH_FAILURE_SETTLE_MS); + let launchSettled: 'settled' | 'running'; + try { + launchSettled = await Promise.race([ + launchProcess.then( + () => 'settled' as const, + () => 'settled' as const + ), + launchFailureSettleTimer.promise.then(() => 'running' as const), + ]); + } finally { + launchFailureSettleTimer.cancel(); + } if (launchSettled === 'settled' && !(await isAppRunning())) { disposed = true; stopPolling = true; + cancelPendingPollDelay(); emitter.clear(); await Promise.allSettled([logTask, exitTask, pollTask]); await launchProcess; @@ -121,6 +164,7 @@ export const createIosAppSession = async ({ disposed = true; stopPolling = true; + cancelPendingPollDelay(); state = { status: 'disposed', occurredAt: Date.now() }; emitter.clear(); diff --git a/packages/platform-ios/src/xctest-agent.ts b/packages/platform-ios/src/xctest-agent.ts index e967da1..3da1344 100644 --- a/packages/platform-ios/src/xctest-agent.ts +++ b/packages/platform-ios/src/xctest-agent.ts @@ -2,6 +2,7 @@ import { createHarnessArtifactDirectory, getHarnessCacheRootPath, getAvailablePort, + delay as cancellableDelay, logger, spawn, terminate, @@ -734,8 +735,10 @@ const getRuntimeConfiguration = ( }, getDefaultRuntimeConfiguration()); }; +// Sequential wait between readiness polls; not raced against anything, so +// the plain (uncancelled) promise is fine here. const delay = async (ms: number) => { - await new Promise((resolve) => setTimeout(resolve, ms)); + await cancellableDelay(ms).promise; }; const waitForAgentReady = async (options: { @@ -771,12 +774,17 @@ const waitForShutdown = async (options: { } const timedOut = Symbol('timedOut'); - const result = await Promise.race([ - options.processTask.then(() => undefined), - delay(options.shutdownTimeoutMs).then(() => timedOut), - ]); - - return result !== timedOut; + const shutdownTimer = cancellableDelay(options.shutdownTimeoutMs); + try { + const result = await Promise.race([ + options.processTask.then(() => undefined), + shutdownTimer.promise.then(() => timedOut), + ]); + + return result !== timedOut; + } finally { + shutdownTimer.cancel(); + } }; const waitForGracefulShutdown = async (options: { @@ -786,27 +794,32 @@ const waitForGracefulShutdown = async (options: { }): Promise<{ didStop: boolean; requestError: unknown | null }> => { let requestError: unknown | null = null; const timedOut = Symbol('timedOut'); + const shutdownTimer = cancellableDelay(options.shutdownTimeoutMs); + + try { + const result = await Promise.race([ + (async () => { + try { + await options.client.shutdown(); + } catch (error) { + requestError = error; + } - const result = await Promise.race([ - (async () => { - try { - await options.client.shutdown(); - } catch (error) { - requestError = error; - } - - return await waitForShutdown({ - processTask: options.processTask, - shutdownTimeoutMs: options.shutdownTimeoutMs, - }); - })(), - delay(options.shutdownTimeoutMs).then(() => timedOut), - ]); + return await waitForShutdown({ + processTask: options.processTask, + shutdownTimeoutMs: options.shutdownTimeoutMs, + }); + })(), + shutdownTimer.promise.then(() => timedOut), + ]); - return { - didStop: result !== timedOut && result === true, - requestError, - }; + return { + didStop: result !== timedOut && result === true, + requestError, + }; + } finally { + shutdownTimer.cancel(); + } }; const waitForChildProcessExit = async (subprocess: Subprocess) => { diff --git a/packages/platform-vega/src/runner.ts b/packages/platform-vega/src/runner.ts index 74ae680..a627269 100644 --- a/packages/platform-vega/src/runner.ts +++ b/packages/platform-vega/src/runner.ts @@ -12,9 +12,6 @@ import * as kepler from './kepler.js'; const APP_EXIT_POLL_INTERVAL_MS = 1000; -const sleep = (ms: number) => - new Promise((resolve) => setTimeout(resolve, ms)); - const getVegaRunner: HarnessPlatformRunnerFactory< VegaPlatformConfig, HarnessConfig @@ -57,6 +54,35 @@ const getVegaRunner: HarnessPlatformRunnerFactory< let state: AppSessionState = { status: 'running' }; let disposed = false; let stopPolling = false; + let pollDelayTimeout: ReturnType | null = null; + let resolvePollDelay: (() => void) | null = null; + + // Unlike a raced delay() loser (which is fine to just discard), this + // wait is directly `await`ed by pollTask with nothing else racing it, + // so cancelling it must also resolve the promise immediately — + // otherwise dispose() blocks on `await pollTask` for up to + // APP_EXIT_POLL_INTERVAL_MS instead of returning right away. + const waitForNextPoll = () => + new Promise((resolve) => { + resolvePollDelay = () => { + resolvePollDelay = null; + pollDelayTimeout = null; + resolve(); + }; + + pollDelayTimeout = setTimeout(() => { + resolvePollDelay?.(); + }, APP_EXIT_POLL_INTERVAL_MS); + }); + + const cancelPendingPollDelay = () => { + if (pollDelayTimeout) { + clearTimeout(pollDelayTimeout); + pollDelayTimeout = null; + } + + resolvePollDelay?.(); + }; const pollTask = (async () => { while (!stopPolling) { @@ -68,7 +94,11 @@ const getVegaRunner: HarnessPlatformRunnerFactory< return; } - await sleep(APP_EXIT_POLL_INTERVAL_MS); + if (stopPolling) { + return; + } + + await waitForNextPoll(); } })(); @@ -80,6 +110,7 @@ const getVegaRunner: HarnessPlatformRunnerFactory< disposed = true; stopPolling = true; + cancelPendingPollDelay(); state = { status: 'disposed', occurredAt: Date.now() }; emitter.clear(); await kepler.stopApp(deviceId, bundleId); diff --git a/packages/tools/src/__tests__/abort.test.ts b/packages/tools/src/__tests__/abort.test.ts index 498bc1d..50b5a51 100644 --- a/packages/tools/src/__tests__/abort.test.ts +++ b/packages/tools/src/__tests__/abort.test.ts @@ -1,10 +1,15 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; -import { getTimeoutSignal, raceAbortSignals, withAbortTimeout } from '../abort.js'; +import { + getTimeoutSignal, + raceAbortSignals, + waitForAbort, + withAbortTimeout, +} from '../abort.js'; const createAbortError = () => new DOMException('The operation was aborted', 'AbortError'); -const waitForAbort = (signal: AbortSignal): Promise => { +const waitForAbortReason = (signal: AbortSignal): Promise => { if (signal.aborted) { return Promise.resolve(signal.reason); } @@ -29,7 +34,7 @@ describe('abort helpers', () => { vi.useFakeTimers(); const signal = getTimeoutSignal(1_000); - const abortPromise = waitForAbort(signal); + const abortPromise = waitForAbortReason(signal); await vi.advanceTimersByTimeAsync(1_000); @@ -42,7 +47,7 @@ describe('abort helpers', () => { const first = new AbortController(); const second = new AbortController(); const signal = raceAbortSignals([first.signal, second.signal]); - const abortPromise = waitForAbort(signal); + const abortPromise = waitForAbortReason(signal); const secondReason = new Error('second'); second.abort(secondReason); @@ -56,16 +61,61 @@ describe('abort helpers', () => { const controller = new AbortController(); const signal = withAbortTimeout(controller.signal, 1_000); - const abortPromise = waitForAbort(signal); + const abortPromise = waitForAbortReason(signal); controller.abort(createAbortError()); await expect(abortPromise).resolves.toBeInstanceOf(DOMException); const timedSignal = withAbortTimeout(new AbortController().signal, 1_000); - const timedAbortPromise = waitForAbort(timedSignal); + const timedAbortPromise = waitForAbortReason(timedSignal); await vi.advanceTimersByTimeAsync(1_000); await expect(timedAbortPromise).resolves.toBeInstanceOf(DOMException); }); }); + +describe('waitForAbort', () => { + it('rejects with the abort reason once the signal aborts', async () => { + const controller = new AbortController(); + const abortWait = waitForAbort(controller.signal); + const reason = new Error('teardown'); + + controller.abort(reason); + + await expect(abortWait.promise).rejects.toBe(reason); + }); + + it('rejects immediately when the signal is already aborted', async () => { + const controller = new AbortController(); + const reason = new Error('already aborted'); + controller.abort(reason); + + const abortWait = waitForAbort(controller.signal); + + await expect(abortWait.promise).rejects.toBe(reason); + // cancel() must be a safe no-op in this path. + expect(() => abortWait.cancel()).not.toThrow(); + }); + + it('removes its abort listener when cancelled after losing a race', async () => { + const controller = new AbortController(); + const addSpy = vi.spyOn(controller.signal, 'addEventListener'); + const removeSpy = vi.spyOn(controller.signal, 'removeEventListener'); + + // This is the bug class under test: racing waitForAbort against real + // work and forgetting to cancel the loser leaks one 'abort' listener + // (and everything its closure retains) per call on a long-lived signal. + for (let i = 0; i < 5; i += 1) { + const abortWait = waitForAbort(controller.signal); + abortWait.promise.catch(() => undefined); + await Promise.race([Promise.resolve('work'), abortWait.promise]); + abortWait.cancel(); + } + + expect(addSpy).toHaveBeenCalledTimes(5); + // Every listener added while losing the race must be removed again — + // without cancel(), removeSpy would stay at 0 while addSpy grows. + expect(removeSpy).toHaveBeenCalledTimes(5); + }); +}); diff --git a/packages/tools/src/__tests__/delay.test.ts b/packages/tools/src/__tests__/delay.test.ts new file mode 100644 index 0000000..409c248 --- /dev/null +++ b/packages/tools/src/__tests__/delay.test.ts @@ -0,0 +1,55 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { delay } from '../delay.js'; + +afterEach(() => { + vi.useRealTimers(); +}); + +describe('delay', () => { + it('resolves after the requested duration', async () => { + vi.useFakeTimers(); + + const pending = delay(1_000); + let resolved = false; + void pending.promise.then(() => { + resolved = true; + }); + + await vi.advanceTimersByTimeAsync(999); + expect(resolved).toBe(false); + + await vi.advanceTimersByTimeAsync(1); + expect(resolved).toBe(true); + }); + + it('clears the underlying timer when cancelled, leaving no pending timer', async () => { + vi.useFakeTimers(); + + // This is the bug class under test: racing delay() against real work + // and forgetting to cancel the loser leaves a ref'd Timeout alive in + // the event loop until it eventually fires, which keeps a process from + // exiting even after the work it was racing against has already won. + const pending = delay(30_000); + const work = Promise.resolve('winner'); + + const result = await Promise.race([ + work, + pending.promise.then(() => 'timedOut' as const), + ]); + pending.cancel(); + + expect(result).toBe('winner'); + expect(vi.getTimerCount()).toBe(0); + }); + + it('is a no-op to cancel a delay that has already fired', async () => { + vi.useFakeTimers(); + + const pending = delay(100); + await vi.advanceTimersByTimeAsync(100); + await pending.promise; + + expect(() => pending.cancel()).not.toThrow(); + expect(vi.getTimerCount()).toBe(0); + }); +}); diff --git a/packages/tools/src/abort.ts b/packages/tools/src/abort.ts index e0932e4..c70e132 100644 --- a/packages/tools/src/abort.ts +++ b/packages/tools/src/abort.ts @@ -15,3 +15,53 @@ export const withAbortTimeout = ( ): AbortSignal => { return raceAbortSignals([signal, getTimeoutSignal(timeout)]); }; + +const createAbortError = () => + new DOMException('The operation was aborted', 'AbortError'); + +const noop = () => undefined; + +export type AbortWait = { + /** Rejects with the signal's abort reason once `signal` aborts. */ + promise: Promise; + /** + * Removes the underlying 'abort' listener without settling `promise`. + * Call this (typically in a `finally`) when `signal` is a long-lived, + * session-lifetime signal and this wait lost a `Promise.race` — otherwise + * the listener (and everything it closes over) stays attached for the + * remaining lifetime of `signal`, leaking one listener per call. + */ + cancel: () => void; +}; + +/** + * Self-cleaning counterpart to racing a bare `signal.addEventListener` + * against other work: returns both the rejecting promise and a `cancel()` + * to detach the listener when this side loses the race. + */ +export const waitForAbort = (signal: AbortSignal): AbortWait => { + if (signal.aborted) { + const promise = Promise.reject(signal.reason ?? createAbortError()); + // A caller may legitimately construct this, decide it no longer needs + // to wait, and call cancel() without ever awaiting or racing `promise` + // (cancel() can't retroactively un-reject it, unlike the pending-signal + // path below). Attach a no-op handler so that case doesn't surface as + // an unhandled rejection; callers that do consume `promise` still see + // the real rejection normally. + promise.catch(noop); + + return { promise, cancel: noop }; + } + + let reject!: (reason?: unknown) => void; + const onAbort = () => reject(signal.reason ?? createAbortError()); + const promise = new Promise((_, rejectFn) => { + reject = rejectFn; + signal.addEventListener('abort', onAbort, { once: true }); + }); + + return { + promise, + cancel: () => signal.removeEventListener('abort', onAbort), + }; +}; diff --git a/packages/tools/src/delay.ts b/packages/tools/src/delay.ts new file mode 100644 index 0000000..0f21598 --- /dev/null +++ b/packages/tools/src/delay.ts @@ -0,0 +1,22 @@ +export type CancellableDelay = { + promise: Promise; + cancel: () => void; +}; + +/** + * Returns a promise that resolves after `ms`, plus a `cancel()` that clears + * the underlying timer. + * + * Use this instead of a bare `setTimeout`-backed promise whenever the delay + * competes in a `Promise.race`: if the delay loses the race, calling + * `cancel()` (typically in a `finally`) clears the timer immediately instead + * of leaving a ref'd `Timeout` alive in the event loop until it eventually + * fires, which otherwise keeps the process from exiting. + */ +export const delay = (ms: number): CancellableDelay => { + let timer: ReturnType; + const promise = new Promise((resolve) => { + timer = setTimeout(resolve, ms); + }); + return { promise, cancel: () => clearTimeout(timer) }; +}; diff --git a/packages/tools/src/index.ts b/packages/tools/src/index.ts index b043be0..337a0f9 100644 --- a/packages/tools/src/index.ts +++ b/packages/tools/src/index.ts @@ -1,4 +1,5 @@ export * from './abort.js'; +export * from './delay.js'; export * from './net.js'; export * from './color.js'; export * from './logger.js'; diff --git a/packages/tools/src/terminate.ts b/packages/tools/src/terminate.ts index 23f7998..7f12549 100644 --- a/packages/tools/src/terminate.ts +++ b/packages/tools/src/terminate.ts @@ -1,15 +1,5 @@ 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) }; -}; +import { delay } from './delay.js'; const waitForExit = ( childProcess: Awaited