diff --git a/.changeset/tame-hens-jump.md b/.changeset/tame-hens-jump.md index 3f58617c6..0ffd02483 100644 --- a/.changeset/tame-hens-jump.md +++ b/.changeset/tame-hens-jump.md @@ -2,4 +2,4 @@ type: Fixed pr: 4552 --- -**`npm run check:env`'s npm-version check no longer misreports a timeout as a missing binary** — every `spawnSync` failure mode (ENOENT, a signal-killed timeout under load, a non-zero exit, a thrown spawn error) used to collapse into one message, "npm binary not found on PATH." Discovered live: an unrelated PR's Windows CI shard failed this check under heavy concurrent test load, and the message made a real timeout indistinguishable from npm genuinely being absent. The reason is now reported accurately; the check's own 10s timeout is unchanged. (#4460) +**`npm run check:env`'s npm-version check no longer times out under CI contention, and no longer misreports a timeout as a missing binary** — the check hand-rolled its own `spawnSync` call with a 10s timeout, duplicating (imperfectly) this repo's canonical OS-shell-projection seam. Discovered live: an unrelated PR's Windows CI shard failed this check twice under heavy concurrent test load. Now routed through `execNpm` (the same seam other npm-invoking code already uses), which gives it the canonical 15s timeout and the canonical, cross-platform-correct timeout detection — a real timeout is now reported as one, distinct from npm genuinely being absent. (#4460) diff --git a/scripts/check-env.cjs b/scripts/check-env.cjs index f3c5317ed..6a6531f53 100644 --- a/scripts/check-env.cjs +++ b/scripts/check-env.cjs @@ -31,6 +31,7 @@ const { spawnSync } = require('child_process'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); const { describeNpmVersionCheckFailure } = require('./lib/npm-version-check-diagnosis.cjs'); +const { execNpm } = require('../gsd-core/bin/lib/shell-command-projection.cjs'); // On Windows, npm ships as npm.cmd (a batch wrapper); spawnSync without // shell:true requires the exact filename including extension. @@ -181,21 +182,26 @@ function main() { // Check 2: npm version vs engines.npm (skip if field absent) // --------------------------------------------------------------------------- const enginesNpm = pkgField('engines.npm', PROJECT_ROOT); - const NPM_VERSION_TIMEOUT_MS = 10_000; - let currentNpm = ''; - let npmSpawnResult = null; - let npmSpawnThrew = null; - try { - npmSpawnResult = spawnSync(npmCmd, ['--version'], { encoding: 'utf8', timeout: NPM_VERSION_TIMEOUT_MS, shell: process.platform === 'win32' }); - if (npmSpawnResult.status === 0 && npmSpawnResult.stdout) { - currentNpm = npmSpawnResult.stdout.trim(); - } - } catch (e) { npmSpawnThrew = e; } + // #4460: was a hand-rolled spawnSync(npmCmd, ...) with its own 10s timeout + // and shell:true/npm.cmd handling, duplicating -- imperfectly -- the + // canonical execNpm seam (src/shell-command-projection.cts, "OS Shell + // Projection: All OS-facing I/O; single platform seam" per CLAUDE.md). + // Routing through it directly gives the SAME npm.cmd/shell handling other + // callers rely on, the repo's own documented npm-subprocess timeout + // (execNpm's 15s default, vs. CLAUDE.md's "60s for npm" for network-facing + // peeks -- --version never touches the network, so the general-purpose + // default is the right analogue), and the canonical, cross-platform-correct + // timeout predicate (isSpawnTimeout / result.timedOut, which checks + // error.code === 'ETIMEDOUT' -- documented there as more reliable than + // signal === 'SIGTERM', which is "platform-fragile" specifically on + // Windows, the exact platform this was discovered failing on). + const npmVersionResult = execNpm(['--version']); + const currentNpm = npmVersionResult.exitCode === 0 && npmVersionResult.stdout ? npmVersionResult.stdout : ''; if (!enginesNpm) { addCheck('npm-version', 'skip', 'engines.npm not set in package.json — skipping'); } else if (!currentNpm) { - addCheck('npm-version', 'fail', describeNpmVersionCheckFailure(npmSpawnResult, npmSpawnThrew, NPM_VERSION_TIMEOUT_MS)); + addCheck('npm-version', 'fail', describeNpmVersionCheckFailure(npmVersionResult)); } else { if (satisfiesConstraint(currentNpm, enginesNpm)) { addCheck('npm-version', 'pass', `npm ${currentNpm} satisfies ${enginesNpm}`); diff --git a/scripts/lib/npm-version-check-diagnosis.cjs b/scripts/lib/npm-version-check-diagnosis.cjs index 2f53c1c08..970e406f2 100644 --- a/scripts/lib/npm-version-check-diagnosis.cjs +++ b/scripts/lib/npm-version-check-diagnosis.cjs @@ -2,38 +2,41 @@ /** * #4460: distinguishes WHY scripts/check-env.cjs's npm-version check's - * `spawnSync(npmCmd, ['--version'], ...)` produced no usable output, - * instead of collapsing every case into "npm binary not found on PATH" -- a - * message that used to fire identically for a genuinely-missing binary AND - * for a spawnSync TIMEOUT under CI load (status stays null, stdout stays + * execNpm(['--version']) (the canonical OS-shell-projection seam, + * src/shell-command-projection.cts) produced no usable output, instead of + * collapsing every case into "npm binary not found on PATH" -- a message + * that used to fire identically for a genuinely-missing binary AND for a + * spawnSync TIMEOUT under CI load (exitCode stays non-zero, stdout stays * empty, either way). Root-caused live: an unrelated PR's Windows CI shard - * failed this check while running 51 concurrent test files; npm.cmd's own - * cold-start plausibly exceeded the 10s window under that contention, and - * the misleading message made a real timeout indistinguishable from npm - * actually being absent. + * failed this check twice in a row while running ~51 concurrent test + * files; npm.cmd's own cold-start plausibly exceeded the check's original + * 10s window under that contention, and the misleading message made a real + * timeout indistinguishable from npm actually being absent. + * + * Uses `result.timedOut` (execNpm's canonical, cross-platform-correct + * timeout predicate -- `error.code === 'ETIMEDOUT'`, per + * shell-command-projection.cts's isSpawnTimeout docstring) rather than + * checking `result.signal === 'SIGTERM'` directly: that check is documented + * there as platform-fragile, with a specifically-called-out risk of a false + * NEGATIVE on Windows -- the exact platform this failure was discovered on. * * Kept out of scripts/check-env.cjs itself (which runs its CLI unconditionally * on require, with no `require.main === module` guard) so this pure logic * can be required directly by tests without triggering a real environment - * check as a side effect. + * check. * - * @param {import('child_process').SpawnSyncReturns|null} spawnResult - * @param {Error|null} spawnThrew - * @param {number} timeoutMs + * @param {import('../../src/shell-command-projection.cts').SpawnResultOutput} result * @returns {string} */ -function describeNpmVersionCheckFailure(spawnResult, spawnThrew, timeoutMs) { - if (spawnResult && spawnResult.error && spawnResult.error.code === 'ENOENT') { +function describeNpmVersionCheckFailure(result) { + if (result.error && result.error.code === 'ENOENT') { return 'npm binary not found on PATH'; } - if (spawnResult && spawnResult.signal) { - return `npm --version was killed (signal ${spawnResult.signal}) -- likely the ${timeoutMs}ms timeout under CI load, not a missing binary`; + if (result.timedOut) { + return `npm --version timed out under CI load -- not a missing binary`; } - if (spawnResult && spawnResult.status != null && spawnResult.status !== 0) { - return `npm --version exited ${spawnResult.status} with no usable output`; - } - if (spawnThrew) { - return `npm --version could not be spawned: ${spawnThrew.message}`; + if (result.exitCode !== 0) { + return `npm --version exited ${result.exitCode} with no usable output`; } return 'npm binary not found on PATH'; } diff --git a/tests/npm-version-check-diagnosis.test.cjs b/tests/npm-version-check-diagnosis.test.cjs index ff564b1c3..1a50c76b2 100644 --- a/tests/npm-version-check-diagnosis.test.cjs +++ b/tests/npm-version-check-diagnosis.test.cjs @@ -6,11 +6,16 @@ * no-defer policy): scripts/check-env.cjs's npm-version check used to * collapse every spawnSync failure mode into one message, "npm binary not * found on PATH" -- including a TIMEOUT under CI load, which looks - * identical to a genuine ENOENT (status stays null, stdout stays empty - * either way). describeNpmVersionCheckFailure is the extracted, pure - * reason-selection logic; kept in scripts/lib/ (not scripts/check-env.cjs - * itself, which runs its CLI unconditionally on require) so it can be - * required directly here without triggering a real environment check. + * identical to a genuine ENOENT (exitCode stays non-zero, stdout stays + * empty either way). Confirmed live, twice, on real Windows CI: the check + * now correctly reports "timed out under CI load" for exactly that case. + * + * describeNpmVersionCheckFailure is the extracted, pure reason-selection + * logic operating on execNpm's SpawnResultOutput shape (the canonical + * OS-shell-projection seam, src/shell-command-projection.cts). Kept in + * scripts/lib/ (not scripts/check-env.cjs itself, which runs its CLI + * unconditionally on require) so it can be required directly here without + * triggering a real environment check. */ const { describe, test } = require('node:test'); @@ -20,40 +25,40 @@ const { describeNpmVersionCheckFailure } = require('../scripts/lib/npm-version-c describe('describeNpmVersionCheckFailure (#4460)', () => { test('a genuine ENOENT (npm truly absent) reports the original message', () => { - const spawnResult = { status: null, stdout: '', signal: null, error: Object.assign(new Error('spawnSync npm.cmd ENOENT'), { code: 'ENOENT' }) }; + const result = { exitCode: 127, stdout: '', stderr: 'npm: not found', signal: null, error: Object.assign(new Error('spawnSync npm ENOENT'), { code: 'ENOENT' }), timedOut: false }; assert.equal( - describeNpmVersionCheckFailure(spawnResult, null, 10_000), + describeNpmVersionCheckFailure(result), 'npm binary not found on PATH', ); }); - test('a signal-killed spawn (the timeout case) is reported as a timeout, not a missing binary', () => { - const spawnResult = { status: null, stdout: '', signal: 'SIGTERM', error: null }; - const reason = describeNpmVersionCheckFailure(spawnResult, null, 10_000); - assert.match(reason, /killed \(signal SIGTERM\)/); - assert.match(reason, /10000ms timeout under CI load/); + test('a timed-out spawn is reported as a timeout, not a missing binary', () => { + const result = { exitCode: 1, stdout: '', stderr: '', signal: 'SIGTERM', error: Object.assign(new Error('ETIMEDOUT'), { code: 'ETIMEDOUT' }), timedOut: true }; + const reason = describeNpmVersionCheckFailure(result); + assert.match(reason, /timed out under CI load/); assert.doesNotMatch(reason, /^npm binary not found on PATH$/); }); - test('a non-zero exit with no stdout is reported with the actual exit code', () => { - const spawnResult = { status: 1, stdout: '', signal: null, error: null }; + test('timedOut is authoritative even without an ENOENT-shaped error object', () => { + // Mirrors what execNpm/_spawnResult actually produces for a timeout: + // error is set (spawnSync always populates it when `timeout` fires), + // but its code is ETIMEDOUT, not ENOENT -- timedOut is what matters. + const result = { exitCode: 1, stdout: '', stderr: '', signal: 'SIGTERM', error: null, timedOut: true }; + assert.match(describeNpmVersionCheckFailure(result), /timed out under CI load/); + }); + + test('a non-zero exit with no stdout and no timeout is reported with the actual exit code', () => { + const result = { exitCode: 1, stdout: '', stderr: '', signal: null, error: null, timedOut: false }; assert.equal( - describeNpmVersionCheckFailure(spawnResult, null, 10_000), + describeNpmVersionCheckFailure(result), 'npm --version exited 1 with no usable output', ); }); - test('spawnSync itself throwing (not just returning a failure result) is reported with the thrown message', () => { - const thrown = new Error('EACCES: permission denied'); + test('exitCode 0 with no stdout (defensive default) falls back to the original message', () => { + const result = { exitCode: 0, stdout: '', stderr: '', signal: null, error: null, timedOut: false }; assert.equal( - describeNpmVersionCheckFailure(null, thrown, 10_000), - 'npm --version could not be spawned: EACCES: permission denied', - ); - }); - - test('no spawn result and no thrown error (defensive default) falls back to the original message', () => { - assert.equal( - describeNpmVersionCheckFailure(null, null, 10_000), + describeNpmVersionCheckFailure(result), 'npm binary not found on PATH', ); });