Fix maxBuffer being ignored or wrongly applied

Author: sindresorhus

docs/api.md

@@ -1027,6 +1027,8 @@ Largest amount of data allowed on [`stdout`](#resultstdout), [`stderr`](#results
 
 By default, this applies to both `stdout` and `stderr`, but [different values can also be passed](output.md#stdoutstderr-specific-options).
 
+With [synchronous methods](execution.md#synchronous-execution), this cannot be enforced per file descriptor while the subprocess runs, since `spawnSync()` only accepts a single value, which limits the total output of all file descriptors. The `maxBuffer.stdout` value is used for it. The other file descriptors' own values are enforced from the output, once the subprocess ended, and cannot be higher than `maxBuffer.stdout`.
+
 When reached, [`error.isMaxBuffer`](#errorismaxbuffer) becomes `true`.
 
 [More info.](output.md#big-output)

docs/execution.md

@@ -161,7 +161,7 @@ Synchronous execution is generally discouraged as it holds the CPU and prevents
 - [IPC](ipc.md): [`sendMessage()`](api.md#sendmessagemessage-sendmessageoptions), [`getOneMessage()`](api.md#getonemessagegetonemessageoptions), [`getEachMessage()`](api.md#geteachmessagegeteachmessageoptions), [`result.ipcOutput`](output.md#any-output-type), [`ipc`](api.md#optionsipc) option, [`serialization`](api.md#optionsserialization) option, [`ipcInput`](input.md#any-input-type) option.
 - [`result.all`](api.md#resultall) is not interleaved.
 - [`detached`](api.md#optionsdetached) option.
-- The [`maxBuffer`](api.md#optionsmaxbuffer) option is always measured in bytes, not in characters, [lines](api.md#optionslines) nor [objects](transform.md#object-mode). Also, it ignores transforms and the [`encoding`](api.md#optionsencoding) option.
+- The [`maxBuffer`](api.md#optionsmaxbuffer) option is always measured in bytes, not in characters, [lines](api.md#optionslines) nor [objects](transform.md#object-mode). Also, it ignores transforms and the [`encoding`](api.md#optionsencoding) option, and the `maxBuffer.stdout` value limits the total output of all file descriptors.
 
 <hr>
 

lib/io/max-buffer.js

@@ -35,9 +35,10 @@ const getMaxBufferUnit = (readableObjectMode, lines, encoding) => {
 	return 'characters';
 };
 
-// Check the `maxBuffer` option with `result.ipcOutput`
-export const checkIpcMaxBuffer = (subprocess, ipcOutput, maxBuffer) => {
-	if (ipcOutput.length !== maxBuffer) {
+// Check the `maxBuffer` option with `result.ipcOutput`, before buffering one more message.
+// Like the stream-based `maxBuffer`, the threshold is hit when the buffered amount would exceed it, which also applies to non-integer values.
+export const checkIpcMaxBuffer = (ipcOutput, maxBuffer) => {
+	if (ipcOutput.length + 1 <= maxBuffer) {
 		return;
 	}
 
@@ -57,33 +58,53 @@ const getMaxBufferInfo = (error, maxBuffer) => {
 		return {streamName: 'output', threshold: maxBuffer[1], unit: 'bytes'};
 	}
 
-	const {maxBufferInfo: {fdNumber, unit}} = error;
+	const {maxBufferInfo: {fdNumber, unit, threshold = getFdSpecificValue(maxBuffer, fdNumber)}} = error;
 	delete error.maxBufferInfo;
 
-	const threshold = getFdSpecificValue(maxBuffer, fdNumber);
 	if (fdNumber === 'ipc') {
 		return {streamName: 'IPC output', threshold, unit: 'messages'};
 	}
 
 	return {streamName: getStreamName(fdNumber), threshold, unit};
 };
 
-// The only way to apply `maxBuffer` with `spawnSync()` is to use the native `maxBuffer` option Node.js provides.
-// However, this has multiple limitations, and cannot behave the exact same way as the async behavior.
-// When the `maxBuffer` is hit, a `ENOBUFS` error is thrown.
-export const isMaxBufferSync = (resultError, output, maxBuffer) => resultError?.code === 'ENOBUFS'
-	&& output !== null
-	&& output.some(result => result !== null && result.length > getMaxBufferSync(maxBuffer));
-
-// When `maxBuffer` is hit, ensure the result is truncated
-export const truncateMaxBufferSync = (result, isMaxBuffer, maxBuffer) => {
-	if (!isMaxBuffer) {
-		return result;
+// The native `maxBuffer` option passed to `spawnSync()` does not allow differentiating values per file descriptor, so we use `stdout`'s.
+// Since each other file descriptor's own limit is capped by `stdout`'s, this is also the highest one.
+// Node.js applies it to the total output of all piped file descriptors, not to each of them. When it is hit, the subprocess is killed and a `ENOBUFS` error is thrown.
+export const getMaxBufferSync = ([, stdoutMaxBuffer]) => stdoutMaxBuffer;
+
+// File descriptors with a limit lower than `spawnSync()`'s native `maxBuffer` are enforced from the output, after the subprocess exits.
+// Since no native error was thrown, we create the `ENOBUFS` error ourselves.
+// Like with asynchronous methods, file descriptors which are not buffered are not limited by their own value. However, when they are piped, e.g. to a file, they still count towards the native limit, which is `stdout`'s value.
+export const handleMaxBufferSync = ({resultError, output, maxBuffer, buffer}) => {
+	const fdNumber = findMaxBufferSync(output, maxBuffer, buffer);
+	if (fdNumber === undefined) {
+		// The native limit can be hit by the total output without any single file descriptor being over its own limit
+		return {resultError, isMaxBuffer: resultError?.code === 'ENOBUFS'};
 	}
 
-	const maxBufferValue = getMaxBufferSync(maxBuffer);
-	return result.length > maxBufferValue ? result.slice(0, maxBufferValue) : result;
+	const threshold = getMaxBufferValueSync(maxBuffer, fdNumber);
+	const error = resultError ?? Object.assign(new MaxBufferError(), {code: 'ENOBUFS'});
+	error.maxBufferInfo = {fdNumber, unit: 'bytes', threshold};
+	return {resultError: error, isMaxBuffer: true};
 };
 
-// `spawnSync()` does not allow differentiating `maxBuffer` per file descriptor, so we always use `stdout`
-export const getMaxBufferSync = ([, stdoutMaxBuffer]) => stdoutMaxBuffer;
+const findMaxBufferSync = (output, maxBuffer, buffer) => {
+	if (output === null) {
+		return;
+	}
+
+	const fdNumber = output.findIndex((result, index) => result !== null
+		&& buffer[index]
+		&& result.length > getMaxBufferValueSync(maxBuffer, index));
+	return fdNumber === -1 ? undefined : fdNumber;
+};
+
+// `spawnSync()` uses `maxBuffer.stdout` as its native `maxBuffer`, so other file descriptors' limits can only be lower than that
+const getMaxBufferValueSync = (maxBuffer, fdNumber) => Math.min(maxBuffer[1], maxBuffer[fdNumber]);
+
+// When `maxBuffer` is hit, ensure each buffered file descriptor's result is truncated at its own limit
+export const truncateMaxBufferSync = (result, fdNumber, {maxBuffer, buffer}) => {
+	const maxBufferValue = getMaxBufferValueSync(maxBuffer, fdNumber);
+	return buffer[fdNumber] && result.length > maxBufferValue ? result.slice(0, maxBufferValue) : result;
+};

lib/io/output-sync.js

@@ -7,7 +7,7 @@ import {FILE_TYPES} from '../stdio/type.js';
 import {truncateMaxBufferSync} from './max-buffer.js';
 
 // Apply `stdout`/`stderr` options, after spawning, in sync mode
-export const transformOutputSync = ({fileDescriptors, syncResult: {output}, options, isMaxBuffer, verboseInfo}) => {
+export const transformOutputSync = ({fileDescriptors, syncResult: {output}, options, verboseInfo}) => {
 	if (output === null) {
 		return {output: Array.from({length: 3})};
 	}
@@ -21,21 +21,20 @@ export const transformOutputSync = ({fileDescriptors, syncResult: {output}, opti
 			fdNumber,
 			state,
 			outputFiles,
-			isMaxBuffer,
 			verboseInfo,
 		}, options));
 	return {output: transformedOutput, ...state};
 };
 
 const transformOutputResultSync = (
-	{result, fileDescriptors, fdNumber, state, outputFiles, isMaxBuffer, verboseInfo},
+	{result, fileDescriptors, fdNumber, state, outputFiles, verboseInfo},
 	{buffer, encoding, lines, stripFinalNewline, maxBuffer},
 ) => {
 	if (result === null) {
 		return;
 	}
 
-	const truncatedResult = truncateMaxBufferSync(result, isMaxBuffer, maxBuffer);
+	const truncatedResult = truncateMaxBufferSync(result, fdNumber, {maxBuffer, buffer});
 	const uint8ArrayResult = bufferToUint8Array(truncatedResult);
 	const {stdioItems, objectMode} = fileDescriptors[fdNumber];
 	const chunks = runOutputGeneratorsSync([uint8ArrayResult], stdioItems, encoding, state);

lib/ipc/buffer-messages.js

@@ -29,7 +29,7 @@ export const waitForIpcOutput = async ({
 		reference: true,
 	})) {
 		if (buffer) {
-			checkIpcMaxBuffer(subprocess, ipcOutput, maxBuffer);
+			checkIpcMaxBuffer(ipcOutput, maxBuffer);
 			ipcOutput.push(message);
 		}
 

lib/methods/main-sync.js

@@ -96,7 +96,6 @@ const spawnSubprocessSync = ({file, commandArguments, options, command, escapedC
 		fileDescriptors,
 		syncResult,
 		options,
-		isMaxBuffer,
 		verboseInfo,
 	});
 	const stdio = output.map((stdioOutput, fdNumber) => stripNewline(stdioOutput, options, fdNumber));

lib/resolve/exit-sync.js

@@ -1,12 +1,20 @@
 import {DiscardedError} from '../return/final-error.js';
-import {isMaxBufferSync} from '../io/max-buffer.js';
+import {handleMaxBufferSync} from '../io/max-buffer.js';
 import {isFailedExit} from './exit-async.js';
 
 // Retrieve exit code, signal name and error information, with synchronous methods
-export const getExitResultSync = ({error, status: exitCode, signal, output}, {maxBuffer}) => {
-	const resultError = getResultError(error, exitCode, signal);
-	const isTimedOut = resultError?.code === 'ETIMEDOUT';
-	const isMaxBuffer = isMaxBufferSync(resultError, output, maxBuffer);
+export const getExitResultSync = ({error, status: exitCode, signal, output}, {maxBuffer, buffer}) => {
+	const initialError = getResultError(error, exitCode, signal);
+	const isTimedOut = initialError?.code === 'ETIMEDOUT';
+	// A file descriptor over its `maxBuffer` does not stop the subprocess, so the timeout that did is reported instead. That file descriptor is still truncated.
+	const {resultError, isMaxBuffer} = isTimedOut
+		? {resultError: initialError, isMaxBuffer: false}
+		: handleMaxBufferSync({
+			resultError: initialError,
+			output,
+			maxBuffer,
+			buffer,
+		});
 	return {
 		resultError,
 		exitCode,

test/fixtures/max-buffer-forever.js

@@ -0,0 +1,8 @@
+#!/usr/bin/env node
+import process from 'node:process';
+import {getWriteStream} from '../helpers/fs.js';
+
+const fdNumber = Number(process.argv[2]);
+const bytes = '.'.repeat(Number(process.argv[3]));
+getWriteStream(fdNumber).write(bytes);
+setTimeout(() => {}, 1e8);

test/helpers/max-buffer.js

@@ -3,10 +3,8 @@ import {execa, execaSync} from '../../index.js';
 export const maxBuffer = 10;
 
 export const assertErrorMessage = (t, shortMessage, {execaMethod = execa, length = maxBuffer, fdNumber = 1, unit = 'characters'} = {}) => {
-	const [expectedStreamName, expectedUnit] = execaMethod === execaSync
-		? ['output', 'bytes']
-		: [STREAM_NAMES[fdNumber], unit];
-	t.true(shortMessage.includes(`${expectedStreamName} was larger than ${length} ${expectedUnit}`));
+	const expectedUnit = execaMethod === execaSync ? 'bytes' : unit;
+	t.true(shortMessage.includes(`${STREAM_NAMES[fdNumber]} was larger than ${length} ${expectedUnit}`));
 };
 
 const STREAM_NAMES = ['stdin', 'stdout', 'stderr', 'stdio[3]'];