diff --git a/CHANGELOG.md b/CHANGELOG.md index 8251ee574..8ab84c0af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ Versioning](https://semver.org/spec/v2.0.0.html). - GitHub Actions caching now uses `http` or `https` based on the scheme of `ACTIONS_RESULTS_URL`. Always calling `https.request` broke `http://` cache proxies used by some third-party runners. +- `WIREIT_PARALLEL`, `WIREIT_WATCH_POLL_MS`, and `WIREIT_MAX_OPEN_FILES` now + reject any value that is not a positive integer, such as `2abc` or `1.5`. ### Changed diff --git a/README.md b/README.md index 5f2fa8d01..8cd6919a3 100644 --- a/README.md +++ b/README.md @@ -890,11 +890,11 @@ The following environment variables affect the behavior of Wireit: | `WIREIT_LOGGER` | How to present progress and results on the command line.

Options:
| | `WIREIT_CACHE_MAX_ENTRIES` | Maximum number of [local cache](#local-caching) entries to retain per script.

Defaults to `10`.

Must be a positive integer or `infinity`. | | `WIREIT_DEBUG_LOG_FILE` | Path to a file which will receive detailed event logging. | -| `WIREIT_MAX_OPEN_FILES` | Limits the number of file descriptors Wireit will have open concurrently. Prevents resource exhaustion when checking large numbers of cached files. Set to a lower number if you hit file descriptor limits. | +| `WIREIT_MAX_OPEN_FILES` | Limits the number of file descriptors Wireit will have open concurrently. Prevents resource exhaustion when checking large numbers of cached files. Set to a lower number if you hit file descriptor limits.

Defaults to `200`.

Must be a positive integer. | | `WIREIT_PARALLEL` | [Maximum number of scripts to run at one time](#parallelism).

Defaults to 2×logical CPU cores.

Must be a positive integer or `infinity`. | | `WIREIT_WATCH` | Set to `true` to enable [watch mode](#watch-mode). | | `WIREIT_WATCH_STRATEGY` | How Wireit determines when a file has changed which warrants a new watch iteration.

Options:
| -| `WIREIT_WATCH_POLL_MS` | When `WIREIT_WATCH_STRATEGY` is `poll`, how many milliseconds to wait between each filesystem poll. Defaults to `500`. | +| `WIREIT_WATCH_POLL_MS` | When `WIREIT_WATCH_STRATEGY` is `poll`, how many milliseconds to wait between each filesystem poll. Defaults to `500`.

Must be a positive integer. | | `CI` | Affects the default value of `WIREIT_CACHE`.

Automatically set to `true` by [GitHub Actions](https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables) and most other CI (continuous integration) services.

Must be exactly `true`. If unset or any other value, interpreted as `false`. | ### Glob patterns diff --git a/src/cli-options.ts b/src/cli-options.ts index 1af746197..282ee3efb 100644 --- a/src/cli-options.ts +++ b/src/cli-options.ts @@ -14,6 +14,7 @@ import {Console, Logger} from './logging/logger.js'; import {MetricsLogger} from './logging/metrics-logger.js'; import {QuietCiLogger, QuietLogger} from './logging/quiet-logger.js'; import * as fs from './util/fs.js'; +import {parsePositiveInteger} from './util/parse-positive-integer.js'; import {unreachable} from './util/unreachable.js'; export const packageDir = await (async (): Promise => { @@ -112,8 +113,8 @@ export const getOptions = async (): Promise> => { if (workerString == null || workerString === '') { return {ok: true, value: defaultValue}; } - const parsedInt = parseInt(workerString, 10); - if (Number.isNaN(parsedInt) || parsedInt <= 0) { + const parsedInt = parsePositiveInteger(workerString); + if (parsedInt === undefined) { return { ok: false, error: { @@ -178,8 +179,8 @@ export const getOptions = async (): Promise> => { if (str.match(/^infinity$/i)) { return {ok: true, value: Infinity}; } - const parsedInt = parseInt(str, 10); - if (Number.isNaN(parsedInt) || parsedInt <= 0) { + const parsedInt = parsePositiveInteger(str); + if (parsedInt === undefined) { return { ok: false, error: { @@ -198,6 +199,27 @@ export const getOptions = async (): Promise> => { return cacheMaxEntriesResult; } + // These are read elsewhere, which falls back to the default for an invalid + // value: WIREIT_MAX_OPEN_FILES when util/fs.ts loads, before this runs, and + // WIREIT_WATCH_POLL_MS only in watch mode. Checking them here makes an + // invalid value fail like the other integer options. + for (const name of ['WIREIT_MAX_OPEN_FILES', 'WIREIT_WATCH_POLL_MS']) { + const str = process.env[name] ?? ''; + if (str !== '' && parsePositiveInteger(str) === undefined) { + return { + ok: false, + error: { + reason: 'invalid-usage', + message: + `Expected the ${name} env variable to be ` + + `a positive integer, got ${JSON.stringify(str)}`, + script, + type: 'failure', + }, + }; + } + } + const failureModeResult = ((): Result => { const str = process.env['WIREIT_FAILURES']; if (!str) { @@ -520,23 +542,12 @@ function readWatchConfigFromEnv(): Options['watch'] { return DEFAULT_WATCH_STRATEGY; } case 'poll': { - let interval = DEFAULT_WATCH_INTERVAL; - const intervalStr = process.env['WIREIT_WATCH_POLL_MS']; - if (intervalStr) { - const parsed = Number(intervalStr); - if (Number.isNaN(parsed) || parsed <= 0) { - console.error( - `⚠️ Expected WIREIT_WATCH_POLL_MS to be a positive integer, ` + - `got ${JSON.stringify(intervalStr)}. Using default interval of ` + - `${DEFAULT_WATCH_INTERVAL}ms.`, - ); - } else { - interval = parsed; - } - } return { strategy: 'poll', - interval, + // getOptions has already failed for an invalid value. + interval: + parsePositiveInteger(process.env['WIREIT_WATCH_POLL_MS'] ?? '') ?? + DEFAULT_WATCH_INTERVAL, }; } default: { diff --git a/src/test/cli-options.test.ts b/src/test/cli-options.test.ts index 72b5a3027..ea81c9b94 100644 --- a/src/test/cli-options.test.ts +++ b/src/test/cli-options.test.ts @@ -111,6 +111,28 @@ const commands: AgentCommands[] = [ }, ]; +const positiveIntegerEnvTests: Array<{ + name: string; + env: Record; + expected: Pick | Pick; +}> = [ + { + name: 'WIREIT_PARALLEL=" +02 "', + env: {WIREIT_PARALLEL: ' +02 '}, + expected: {numWorkers: 2}, + }, + { + name: 'WIREIT_CACHE_MAX_ENTRIES=" +02 "', + env: {WIREIT_CACHE_MAX_ENTRIES: ' +02 '}, + expected: {cacheMaxEntries: 2}, + }, + { + name: 'WIREIT_CACHE_MAX_ENTRIES=5', + env: {WIREIT_CACHE_MAX_ENTRIES: '5'}, + expected: {cacheMaxEntries: 5}, + }, +]; + for (const {agent, runCmd, testCmd, startCmd, needsExtraDashes} of commands) { if (agent === 'nodeRun' && NODE_MAJOR_VERSION < 22) { // node --run was added in Node 22. @@ -565,24 +587,24 @@ for (const {agent, runCmd, testCmd, startCmd, needsExtraDashes} of commands) { }), ); - void test( - `${agent} WIREIT_CACHE_MAX_ENTRIES=5`, - rigTest(async ({rig}) => { - await assertOptions( - rig, - `${runCmd} main ${extraDashes}`, - { - agent, - script: { - packageDir: rig.temp, - name: 'main', + for (const {name, env, expected} of positiveIntegerEnvTests) { + void test( + `${agent} ${name}`, + rigTest(async ({rig}) => { + await assertOptions( + rig, + `${runCmd} main ${extraDashes}`, + { + agent, + script: { + packageDir: rig.temp, + name: 'main', + }, + ...expected, }, - cacheMaxEntries: 5, - }, - { - WIREIT_CACHE_MAX_ENTRIES: '5', - }, - ); - }), - ); + env, + ); + }), + ); + } } diff --git a/src/test/errors-usage.test.ts b/src/test/errors-usage.test.ts index 826d1331c..4e1b811f3 100644 --- a/src/test/errors-usage.test.ts +++ b/src/test/errors-usage.test.ts @@ -121,6 +121,110 @@ void test( }), ); +void test( + 'partially numeric parallelism', + rigTest(async ({rig}) => { + await rig.write({ + 'package.json': { + scripts: { + main: 'wireit', + }, + wireit: { + main: {command: 'node --version'}, + }, + }, + }); + const result = rig.exec('npm run main', { + env: {WIREIT_PARALLEL: '2abc'}, + }); + const done = await result.exit; + assert.equal(done.code, 1); + assert.ok( + done.stderr.includes( + `❌ [main] Invalid usage: Expected the WIREIT_PARALLEL env variable to be a positive integer, got "2abc"`, + ), + ); + }), +); + +void test( + 'partially numeric WIREIT_CACHE_MAX_ENTRIES', + rigTest(async ({rig}) => { + await rig.write({ + 'package.json': { + scripts: { + main: 'wireit', + }, + wireit: { + main: {command: 'node --version'}, + }, + }, + }); + const result = rig.exec('npm run main', { + env: {WIREIT_CACHE_MAX_ENTRIES: '2abc'}, + }); + const done = await result.exit; + assert.equal(done.code, 1); + assert.ok( + done.stderr.includes( + `❌ [main] Invalid usage: Expected the WIREIT_CACHE_MAX_ENTRIES env variable to be a positive integer or "infinity", got "2abc"`, + ), + ); + }), +); + +void test( + 'fractional WIREIT_MAX_OPEN_FILES', + rigTest(async ({rig}) => { + await rig.write({ + 'package.json': { + scripts: { + main: 'wireit', + }, + wireit: { + main: {command: 'node --version'}, + }, + }, + }); + const result = rig.exec('npm run main', { + env: {WIREIT_MAX_OPEN_FILES: '1.5'}, + }); + const done = await result.exit; + assert.equal(done.code, 1); + assert.ok( + done.stderr.includes( + `❌ [main] Invalid usage: Expected the WIREIT_MAX_OPEN_FILES env variable to be a positive integer, got "1.5"`, + ), + ); + }), +); + +void test( + 'fractional WIREIT_WATCH_POLL_MS', + rigTest(async ({rig}) => { + await rig.write({ + 'package.json': { + scripts: { + main: 'wireit', + }, + wireit: { + main: {command: 'node --version'}, + }, + }, + }); + const result = rig.exec('npm run main', { + env: {WIREIT_WATCH_POLL_MS: '1.5'}, + }); + const done = await result.exit; + assert.equal(done.code, 1); + assert.ok( + done.stderr.includes( + `❌ [main] Invalid usage: Expected the WIREIT_WATCH_POLL_MS env variable to be a positive integer, got "1.5"`, + ), + ); + }), +); + void test( 'nonsense WIREIT_CACHE', rigTest(async ({rig}) => { diff --git a/src/util/fs.ts b/src/util/fs.ts index a2a79ed94..f1f399ab3 100644 --- a/src/util/fs.ts +++ b/src/util/fs.ts @@ -15,6 +15,7 @@ import { } from 'fs'; import {Deferred} from './deferred.js'; import './dispose.js'; +import {parsePositiveInteger} from './parse-positive-integer.js'; export {constants} from 'fs'; declare global { @@ -57,18 +58,20 @@ export class Semaphore { } } -export const fileBudget = (() => { - let maxOpenFiles = Number(process.env['WIREIT_MAX_OPEN_FILES']); - if (isNaN(maxOpenFiles)) { - // This is tricky to get right. There's no simple cross-platform way to - // determine what our current limits are. Windows it's 512, on macOS it - // defaults to 256, and on Linux it varies a lot. - // 200 gives us a bit of headroom for other things that might be using - // file descriptors in our process, like node internals. - maxOpenFiles = 200; - } - return new Semaphore(maxOpenFiles); -})(); +/** + * This is tricky to get right. There's no simple cross-platform way to + * determine what our current limits are. Windows it's 512, on macOS it + * defaults to 256, and on Linux it varies a lot. + * 200 gives us a bit of headroom for other things that might be using + * file descriptors in our process, like node internals. + */ +const DEFAULT_MAX_OPEN_FILES = 200; + +export const fileBudget = new Semaphore( + // getOptions reports an invalid value as an error before any script runs. + parsePositiveInteger(process.env['WIREIT_MAX_OPEN_FILES'] ?? '') ?? + DEFAULT_MAX_OPEN_FILES, +); export async function mkdir( path: string, diff --git a/src/util/parse-positive-integer.ts b/src/util/parse-positive-integer.ts new file mode 100644 index 000000000..d6b15558a --- /dev/null +++ b/src/util/parse-positive-integer.ts @@ -0,0 +1,23 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Parse a positive integer from an environment variable, or return undefined + * for anything else. + * + * Surrounding whitespace, a leading "+", and leading zeroes are accepted, + * because parseInt accepted them. Trailing characters, fractions, exponents, + * and integers too large to represent exactly are rejected, where parseInt or + * Number would have silently read a different value. + */ +export function parsePositiveInteger(value: string): number | undefined { + const normalized = value.trim(); + if (!/^\+?\d+$/.test(normalized)) { + return undefined; + } + const parsed = Number(normalized); + return Number.isSafeInteger(parsed) && parsed > 0 ? parsed : undefined; +}