From 60a1eeca28f5b4567c8cd5a811cde364ddd6bf1c Mon Sep 17 00:00:00 2001 From: Sharmila Date: Tue, 2 Jun 2026 00:02:38 -0700 Subject: [PATCH] fix(cli): avoid Windows crash after network requests (#12121) * fix: avoid Windows crash after network requests * fix: destroy dispatchers before Windows error exits * test: cover destroyDispatchers and document its shutdown contract Add a JSDoc to destroyDispatchers() explaining it is for process shutdown, a unit test verifying it destroys the global and cached dispatchers, and bump @pnpm/network.fetch to minor for the new public export. * fix(network.fetch): destroy the active global dispatcher on shutdown Include getGlobalDispatcher() so destroyDispatchers() also closes the currently-active dispatcher if it was replaced via setGlobalDispatcher(). --------- Co-authored-by: Zoltan Kochan --- .changeset/destroy-windows-dispatchers.md | 6 ++++++ network/fetch/src/dispatcher.ts | 26 ++++++++++++++++++++--- network/fetch/src/index.ts | 2 +- network/fetch/test/dispatcher.test.ts | 23 +++++++++++++++++--- pnpm-lock.yaml | 3 +++ pnpm/package.json | 1 + pnpm/src/errorHandler.ts | 3 ++- pnpm/src/exit.ts | 12 +++++++++++ pnpm/src/switchCliVersion.ts | 3 ++- pnpm/test/install/global.ts | 2 +- pnpm/test/install/minimumReleaseAge.ts | 2 +- pnpm/tsconfig.json | 3 +++ 12 files changed, 75 insertions(+), 11 deletions(-) create mode 100644 .changeset/destroy-windows-dispatchers.md create mode 100644 pnpm/src/exit.ts diff --git a/.changeset/destroy-windows-dispatchers.md b/.changeset/destroy-windows-dispatchers.md new file mode 100644 index 0000000000..3e51787595 --- /dev/null +++ b/.changeset/destroy-windows-dispatchers.md @@ -0,0 +1,6 @@ +--- +"@pnpm/network.fetch": minor +"pnpm": patch +--- + +Avoided a Node.js crash when pnpm exits after network requests on Windows. diff --git a/network/fetch/src/dispatcher.ts b/network/fetch/src/dispatcher.ts index 015409b288..9299193b1e 100644 --- a/network/fetch/src/dispatcher.ts +++ b/network/fetch/src/dispatcher.ts @@ -7,7 +7,7 @@ import { PnpmError } from '@pnpm/error' import type { TlsConfig } from '@pnpm/types' import { LRUCache } from 'lru-cache' import { SocksClient } from 'socks' -import { Agent, type Dispatcher, ProxyAgent, setGlobalDispatcher } from 'undici' +import { Agent, type Dispatcher, getGlobalDispatcher, ProxyAgent, setGlobalDispatcher } from 'undici' const DEFAULT_MAX_SOCKETS = 50 const KEEP_ALIVE_TIMEOUT = 30_000 // 30 seconds @@ -20,14 +20,16 @@ const KEEP_ALIVE_MAX_TIMEOUT = 600_000 // 10 minutes // With HTTP/2, undici multiplexes many streams over 1-2 TCP connections sharing a single // congestion window. In benchmarks this was slower than opening ~50 independent HTTP/1.1 // connections that each get their own congestion window and can saturate bandwidth in parallel. -setGlobalDispatcher(new Agent({ +const GLOBAL_DISPATCHER = new Agent({ connections: DEFAULT_MAX_SOCKETS, keepAliveTimeout: KEEP_ALIVE_TIMEOUT, keepAliveMaxTimeout: KEEP_ALIVE_MAX_TIMEOUT, connect: { autoSelectFamily: true, }, -}).compose(stripSecFetchHeaders)) +}).compose(stripSecFetchHeaders) + +setGlobalDispatcher(GLOBAL_DISPATCHER) // undici's fetch() automatically adds sec-fetch-* headers (e.g. sec-fetch-mode: cors) // per the Fetch spec. Some registries like Azure DevOps Artifacts interpret these as @@ -98,6 +100,24 @@ export function clearDispatcherCache (): void { DISPATCHER_CACHE.clear() } +/** + * Destroy the global dispatcher and every cached dispatcher, closing their open + * sockets. Intended for process shutdown only: once called, the module can no + * longer perform network requests. This is used to work around a Windows crash + * that happens when the process exits while sockets are still open + * (https://github.com/nodejs/node/issues/56645). + */ +export async function destroyDispatchers (): Promise { + // getGlobalDispatcher() is included in case something replaced GLOBAL_DISPATCHER + // via setGlobalDispatcher(); the Set removes the duplicate when it is still our own instance. + const dispatchers = new Set([ + GLOBAL_DISPATCHER, + getGlobalDispatcher(), + ...DISPATCHER_CACHE.values(), + ]) + await Promise.allSettled(Array.from(dispatchers, dispatcher => dispatcher.destroy())) +} + /** * Get a dispatcher for the given URI and options. * Returns undefined if no special configuration is needed (to use global dispatcher). diff --git a/network/fetch/src/index.ts b/network/fetch/src/index.ts index 58a483bcc7..f3cd7399d3 100644 --- a/network/fetch/src/index.ts +++ b/network/fetch/src/index.ts @@ -1,4 +1,4 @@ -export { clearDispatcherCache, getDispatcher } from './dispatcher.js' +export { clearDispatcherCache, destroyDispatchers, getDispatcher } from './dispatcher.js' export { fetch, isRedirect, type RetryTimeoutOptions } from './fetch.js' export { createDispatchedFetch, type CreateDispatchedFetchOptions, createFetchFromRegistry, type CreateFetchFromRegistryOptions, type DispatcherOptions, fetchWithDispatcher } from './fetchFromRegistry.js' export type { FetchFromRegistry } from '@pnpm/fetching.types' diff --git a/network/fetch/test/dispatcher.test.ts b/network/fetch/test/dispatcher.test.ts index 94985f8521..0ad79a4140 100644 --- a/network/fetch/test/dispatcher.test.ts +++ b/network/fetch/test/dispatcher.test.ts @@ -1,9 +1,9 @@ /// import net from 'node:net' -import { afterEach, describe, expect, test } from '@jest/globals' -import { clearDispatcherCache, type DispatcherOptions, getDispatcher } from '@pnpm/network.fetch' -import { Agent, ProxyAgent } from 'undici' +import { afterEach, describe, expect, jest, test } from '@jest/globals' +import { clearDispatcherCache, destroyDispatchers, type DispatcherOptions, getDispatcher } from '@pnpm/network.fetch' +import { Agent, getGlobalDispatcher, ProxyAgent } from 'undici' afterEach(() => { clearDispatcherCache() @@ -270,3 +270,20 @@ describe('client certificates', () => { expect(d1).not.toBe(d2) // different certs → different dispatchers }) }) + +describe('destroyDispatchers', () => { + // The destroy() calls are mocked so the live global dispatcher survives the test run. + test('destroys the global dispatcher and every cached dispatcher without throwing', async () => { + const globalDestroySpy = jest.spyOn(getGlobalDispatcher(), 'destroy').mockResolvedValue() + const cached = getDispatcher('https://registry.npmjs.org/foo', { strictSsl: false })! + const cachedDestroySpy = jest.spyOn(cached, 'destroy').mockResolvedValue() + try { + await expect(destroyDispatchers()).resolves.toBeUndefined() + expect(globalDestroySpy).toHaveBeenCalled() + expect(cachedDestroySpy).toHaveBeenCalled() + } finally { + globalDestroySpy.mockRestore() + cachedDestroySpy.mockRestore() + } + }) +}) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index e460fb3328..88ba718d83 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -7811,6 +7811,9 @@ importers: '@pnpm/logger': specifier: workspace:* version: link:../core/logger + '@pnpm/network.fetch': + specifier: workspace:* + version: link:../network/fetch '@pnpm/nopt': specifier: 'catalog:' version: 0.3.1 diff --git a/pnpm/package.json b/pnpm/package.json index b63fff64ac..b22c05589f 100644 --- a/pnpm/package.json +++ b/pnpm/package.json @@ -111,6 +111,7 @@ "@pnpm/lockfile.fs": "workspace:*", "@pnpm/lockfile.types": "workspace:*", "@pnpm/logger": "workspace:*", + "@pnpm/network.fetch": "workspace:*", "@pnpm/nopt": "catalog:", "@pnpm/patching.commands": "workspace:*", "@pnpm/pkg-manifest.commands": "workspace:*", diff --git a/pnpm/src/errorHandler.ts b/pnpm/src/errorHandler.ts index 83c1272669..cfdb84896b 100644 --- a/pnpm/src/errorHandler.ts +++ b/pnpm/src/errorHandler.ts @@ -3,6 +3,7 @@ import { promisify } from 'node:util' import { logger } from '@pnpm/logger' import pidTree from 'pidtree' +import { exit } from './exit.js' import { type Global, REPORTER_INITIALIZED } from './main.js' declare const global: Global @@ -59,5 +60,5 @@ async function killProcesses (status: number): Promise { } catch { // ignore error here } - process.exit(status) + await exit(status) } diff --git a/pnpm/src/exit.ts b/pnpm/src/exit.ts new file mode 100644 index 0000000000..d5f898a4c5 --- /dev/null +++ b/pnpm/src/exit.ts @@ -0,0 +1,12 @@ +export async function exit (status: number): Promise { + if (process.platform === 'win32') { + try { + // Work around https://github.com/nodejs/node/issues/56645. + const { destroyDispatchers } = await import('@pnpm/network.fetch') + await destroyDispatchers() + } catch { + // ignore error here + } + } + process.exit(status) +} diff --git a/pnpm/src/switchCliVersion.ts b/pnpm/src/switchCliVersion.ts index c1e9bf63ce..59cdaf79b8 100644 --- a/pnpm/src/switchCliVersion.ts +++ b/pnpm/src/switchCliVersion.ts @@ -12,6 +12,7 @@ import { createStoreController } from '@pnpm/store.connection-manager' import spawn from 'cross-spawn' import semver from 'semver' +import { exit } from './exit.js' import { shouldPersistLockfile } from './shouldPersistLockfile.js' export async function switchCliVersion (config: Config, context: ConfigContext): Promise { @@ -129,7 +130,7 @@ export async function switchCliVersion (config: Config, context: ConfigContext): return } - process.exit(status ?? 0) + await exit(status ?? 0) } class VersionSwitchFail extends PnpmError { diff --git a/pnpm/test/install/global.ts b/pnpm/test/install/global.ts index deff9d6bdb..788d5d2732 100644 --- a/pnpm/test/install/global.ts +++ b/pnpm/test/install/global.ts @@ -408,9 +408,9 @@ test('global add in strict minimumReleaseAge mode reports the user-facing error' }) const output = `${result.stdout.toString()}\n${result.stderr.toString()}` - expect(result.status).toBe(1) expect(output).toContain('ERR_PNPM_NO_MATURE_MATCHING_VERSION') expect(output).not.toContain('ERR_PNPM_RESOLUTION_POLICY_VIOLATIONS_UNHANDLED') + expect(result.status).toBe(1) }) test('global update in loose minimumReleaseAge mode persists immature picks', () => { diff --git a/pnpm/test/install/minimumReleaseAge.ts b/pnpm/test/install/minimumReleaseAge.ts index f177a107cd..d82b2afe16 100644 --- a/pnpm/test/install/minimumReleaseAge.ts +++ b/pnpm/test/install/minimumReleaseAge.ts @@ -182,10 +182,10 @@ describe('lockfile minimumReleaseAge verification', () => { [PUBLIC_REGISTRY, 'install', '--frozen-lockfile'], omitMinReleaseAgeEnv ) - expect(result.status).toBe(1) const output = `${result.stdout.toString()}\n${result.stderr.toString()}` expect(output).toContain('ERR_PNPM_MINIMUM_RELEASE_AGE_VIOLATION') expect(output).toMatch(/is-odd@0\.1\.2/) + expect(result.status).toBe(1) }) test('loose mode auto-adds fresh immature picks to minimumReleaseAgeExclude', () => { diff --git a/pnpm/tsconfig.json b/pnpm/tsconfig.json index 2d6ffa4148..461e419a1e 100644 --- a/pnpm/tsconfig.json +++ b/pnpm/tsconfig.json @@ -125,6 +125,9 @@ { "path": "../lockfile/types" }, + { + "path": "../network/fetch" + }, { "path": "../patching/commands" },