From 2b15751a6f2b28c1279d062ea2f7fbb1ff62f786 Mon Sep 17 00:00:00 2001 From: Christian Hoffmann Date: Tue, 18 Aug 2026 13:41:19 +0200 Subject: [PATCH 1/2] fix(fabricator): harden framing, prevent stdio deadlocks, and add regression tests --- lib/fabricator.ts | 136 ++++++++++++++++++++++++----------- test/unit/fabricator.test.ts | 110 ++++++++++++++++++++++++++++ 2 files changed, 206 insertions(+), 40 deletions(-) create mode 100644 test/unit/fabricator.test.ts diff --git a/lib/fabricator.ts b/lib/fabricator.ts index 4549ff80c..f062c7b57 100644 --- a/lib/fabricator.ts +++ b/lib/fabricator.ts @@ -3,39 +3,53 @@ import { Readable, Writable } from 'stream'; import { log } from './log'; import { Target } from './types'; -const script = ` +const FABRICATOR_MAX_FRAME_PART_SIZE = 256 * 1024 * 1024; + +export const fabricatorScript = ` var vm = require('vm'); var module = require('module'); + var MAX_FRAME_PART_SIZE = ${FABRICATOR_MAX_FRAME_PART_SIZE}; var stdin = Buffer.alloc(0); process.stdin.on('data', function (data) { stdin = Buffer.concat([ stdin, data ]); - if (stdin.length >= 4) { + while (stdin.length >= 4) { var sizeOfSnap = stdin.readInt32LE(0); - if (stdin.length >= 4 + sizeOfSnap + 4) { - var sizeOfBody = stdin.readInt32LE(4 + sizeOfSnap); - if (stdin.length >= 4 + sizeOfSnap + 4 + sizeOfBody) { - var snap = stdin.toString('utf8', 4, 4 + sizeOfSnap); - var body = Buffer.alloc(sizeOfBody); - var startOfBody = 4 + sizeOfSnap + 4; - stdin.copy(body, 0, startOfBody, startOfBody + sizeOfBody); - stdin = Buffer.alloc(0); - var code = module.wrap(body); - var s = new vm.Script(code, { - filename: snap, - produceCachedData: true, - sourceless: true - }); - if (!s.cachedDataProduced) { - console.error('Pkg: Cached data not produced.'); - process.exit(2); - } - var h = Buffer.alloc(4); - var b = s.cachedData; - h.writeInt32LE(b.length, 0); - process.stdout.write(h); - process.stdout.write(b); - } + if (sizeOfSnap < 0 || sizeOfSnap > MAX_FRAME_PART_SIZE) { + console.error('Pkg: Invalid snap size header: ' + sizeOfSnap); + process.exit(2); + } + if (stdin.length < 4 + sizeOfSnap + 4) break; + var sizeOfBody = stdin.readInt32LE(4 + sizeOfSnap); + if (sizeOfBody < 0 || sizeOfBody > MAX_FRAME_PART_SIZE) { + console.error('Pkg: Invalid body size header: ' + sizeOfBody); + process.exit(2); } + var totalSize = 4 + sizeOfSnap + 4 + sizeOfBody; + if (stdin.length < totalSize) break; + + var snap = stdin.toString('utf8', 4, 4 + sizeOfSnap); + var body = Buffer.alloc(sizeOfBody); + var startOfBody = 4 + sizeOfSnap + 4; + stdin.copy(body, 0, startOfBody, startOfBody + sizeOfBody); + + // Preserve unconsumed bytes for subsequent payloads + stdin = stdin.subarray(totalSize); + + var code = module.wrap(body); + var s = new vm.Script(code, { + filename: snap, + produceCachedData: true, + sourceless: true + }); + if (!s.cachedDataProduced) { + console.error('Pkg: Cached data not produced.'); + process.exit(2); + } + var h = Buffer.alloc(4); + var b = s.cachedData; + h.writeInt32LE(b.length, 0); + process.stdout.write(h); + process.stdout.write(b); } }); process.stdin.resume(); @@ -43,9 +57,38 @@ const script = ` const children: Record< string, - ChildProcessByStdio + ChildProcessByStdio > = {}; +export function buildFabricatorRequestChunks( + snap: string, + body: Buffer, +): [Buffer, Buffer, Buffer, Buffer] { + const snapBuf = Buffer.from(snap); + + if (snapBuf.length > FABRICATOR_MAX_FRAME_PART_SIZE) { + throw new Error( + `Fabricator snap exceeds max frame size (${snapBuf.length} bytes)`, + ); + } + + if (body.length > FABRICATOR_MAX_FRAME_PART_SIZE) { + throw new Error( + `Fabricator body exceeds max frame size (${body.length} bytes)`, + ); + } + + // Keep separate header buffers so async stream writes cannot mutate + // previously queued bytes. + const h1 = Buffer.alloc(4); + h1.writeInt32LE(snapBuf.length, 0); + + const h2 = Buffer.alloc(4); + h2.writeInt32LE(body.length, 0); + + return [h1, snapBuf, h2, body]; +} + export function fabricate( bakes: string[], fabricator: Target, @@ -67,12 +110,17 @@ export function fabricate( let child = children[key]; if (!child) { - const stderr = log.debugMode ? process.stdout : 'ignore'; - children[key] = spawn(cmd, activeBakes.concat('-e', script), { - stdio: ['pipe', 'pipe', stderr], + children[key] = spawn(cmd, activeBakes.concat('-e', fabricatorScript), { + stdio: ['pipe', 'pipe', 'pipe'], env: { PKG_EXECPATH: 'PKG_INVOKE_NODEJS' }, }); child = children[key]; + + if (child.stderr) { + child.stderr.on('data', (data: Buffer) => { + log.debug(`fabricator: ${data.toString().trim()}`); + }); + } } function kill() { @@ -103,7 +151,9 @@ export function fabricate( ); } - console.log(stdout.toString()); + if (stdout.length > 0) { + log.debug(`fabricator: unexpected close output: ${stdout.toString()}`); + } return cb(new Error(`${cmd} closed unexpectedly`)); } @@ -134,15 +184,21 @@ export function fabricate( child.stdout.on('error', onError); child.stdout.on('data', onData); - const h = Buffer.alloc(4); - let b = Buffer.from(snap); - h.writeInt32LE(b.length, 0); - child.stdin.write(h); - child.stdin.write(b); - b = body; - h.writeInt32LE(b.length, 0); - child.stdin.write(h); - child.stdin.write(b); + let requestChunks: [Buffer, Buffer, Buffer, Buffer]; + try { + requestChunks = buildFabricatorRequestChunks(snap, body); + } catch (error) { + removeListeners(); + return cb( + new Error( + `Failed to make bytecode ${fabricator.nodeRange}-${fabricator.arch} for file ${snap} error (${(error as Error).message})`, + ), + ); + } + + for (const chunk of requestChunks) { + child.stdin.write(chunk); + } } export function fabricateTwice( diff --git a/test/unit/fabricator.test.ts b/test/unit/fabricator.test.ts new file mode 100644 index 000000000..30ac45f37 --- /dev/null +++ b/test/unit/fabricator.test.ts @@ -0,0 +1,110 @@ +import assert from 'node:assert/strict'; +import { spawn } from 'node:child_process'; +import { describe, it } from 'node:test'; + +import { + buildFabricatorRequestChunks, + fabricatorScript, +} from '../../lib/fabricator'; + +function parseBlobFrames(buffer: Buffer): Buffer[] { + const out: Buffer[] = []; + let offset = 0; + + while (offset + 4 <= buffer.length) { + const sizeOfBlob = buffer.readInt32LE(offset); + if (sizeOfBlob < 0 || offset + 4 + sizeOfBlob > buffer.length) break; + + const blob = Buffer.alloc(sizeOfBlob); + buffer.copy(blob, 0, offset + 4, offset + 4 + sizeOfBlob); + out.push(blob); + offset += 4 + sizeOfBlob; + } + + return out; +} + +describe('fabricator framing', () => { + it('buildFabricatorRequestChunks uses independent headers', () => { + const body = Buffer.from('module.exports = 42;'); + const [h1, snapBuf, h2, bodyBuf] = buildFabricatorRequestChunks( + '/snapshot/app.js', + body, + ); + + assert.notEqual(h1, h2); + assert.equal(h1.readInt32LE(0), snapBuf.length); + assert.equal(h2.readInt32LE(0), bodyBuf.length); + + h2.writeInt32LE(123456, 0); + assert.equal(h1.readInt32LE(0), snapBuf.length); + }); + + it('child script preserves trailing bytes and decodes multiple payloads', async () => { + const child = spawn(process.execPath, ['-e', fabricatorScript], { + stdio: ['pipe', 'pipe', 'pipe'], + }); + + const stdoutChunks: Buffer[] = []; + const stderrChunks: Buffer[] = []; + + child.stdout.on('data', (chunk: Buffer) => { + stdoutChunks.push(chunk); + }); + + child.stderr.on('data', (chunk: Buffer) => { + stderrChunks.push(chunk); + }); + + const frame1 = Buffer.concat( + buildFabricatorRequestChunks( + '/snapshot/one.js', + Buffer.from('module.exports = 1;'), + ), + ); + const frame2 = Buffer.concat( + buildFabricatorRequestChunks( + '/snapshot/two.js', + Buffer.from('module.exports = 2;'), + ), + ); + + const splitAt = 3; + child.stdin.write(Buffer.concat([frame1, frame2.subarray(0, splitAt)])); + child.stdin.end(frame2.subarray(splitAt)); + + const [code] = (await new Promise((resolve) => { + child.on('close', (closeCode, signal) => resolve([closeCode, signal])); + })) as [number | null, NodeJS.Signals | null]; + + assert.equal(code, 0, Buffer.concat(stderrChunks).toString()); + + const frames = parseBlobFrames(Buffer.concat(stdoutChunks)); + assert.equal(frames.length, 2); + assert.ok(frames[0].length > 0); + assert.ok(frames[1].length > 0); + }); + + it('child script rejects invalid size headers', async () => { + const child = spawn(process.execPath, ['-e', fabricatorScript], { + stdio: ['pipe', 'pipe', 'pipe'], + }); + + const stderrChunks: Buffer[] = []; + child.stderr.on('data', (chunk: Buffer) => { + stderrChunks.push(chunk); + }); + + const badHeader = Buffer.alloc(4); + badHeader.writeInt32LE(-1, 0); + child.stdin.end(badHeader); + + const [code] = (await new Promise((resolve) => { + child.on('close', (closeCode, signal) => resolve([closeCode, signal])); + })) as [number | null, NodeJS.Signals | null]; + + const stderr = Buffer.concat(stderrChunks).toString(); + assert.equal(code, 2); + assert.match(stderr, /Invalid snap size header/); + }); +}); From 64a892148bdfffc7f980f58e996b9b1d19fc57a2 Mon Sep 17 00:00:00 2001 From: Christian Hoffmann Date: Sat, 5 Sep 2026 13:22:50 +0200 Subject: [PATCH 2/2] fix(fabricator): harden parent decoder, fail loudly on protocol desync Review follow-up for #294: - shared tryParseFabricatorResponse guards the parent decoder against garbage/negative headers (reachable hang via e.g. --options trace-gc leaking bake output into stdout) instead of stalling forever or throwing ERR_OUT_OF_RANGE inside a data handler - framing violations now exit 3 (FABRICATOR_PROTOCOL_EXIT_CODE), distinct from exit 2 'V8 refused to compile'; producer aborts the build loudly instead of silently degrading to --fallback-to-source - 60s response timeout per request; hung children are killed and reported with a bounded stderr tail attached to every failure - unexpected-close output embedded as a printable-safe snippet; fabricateTwice no longer retries deterministic or protocol errors - remainder copies no longer pin large backing stores - tests run against the production parser, cover body-header rejection, zero-length payloads, deterministic split offsets, and an end-to-end fabricate() run; all child waits timeout-wrapped - document frame protocol, 256MB ceiling rationale and exit-code contract in docs/ARCHITECTURE.md --- docs/ARCHITECTURE.md | 32 ++- lib/fabricator.ts | 332 ++++++++++++++++++++++++----- lib/producer.ts | 9 +- test/unit/fabricator.test.ts | 393 +++++++++++++++++++++++++++++------ 4 files changed, 656 insertions(+), 110 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index cdacdda65..6b80d7f29 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -138,6 +138,36 @@ CLI (lib/index.ts) └─ runPostBuild() — lib/hooks.ts (per-binary, sets PKG_OUTPUT) ``` +### Bytecode Fabricator IPC + +In traditional mode, `lib/fabricator.ts` compiles each JS file to V8 bytecode by +spawning the target's base binary (`PKG_EXECPATH=PKG_INVOKE_NODEJS`) and exchanging +length-prefixed frames over stdio: + +``` +Request (parent → child): [u32 snapLen][snap bytes][u32 bodyLen][body bytes] +Response (child → parent): [u32 blobLen][cachedData bytes] +``` + +All integers are little-endian. Each frame part is capped at +`FABRICATOR_MAX_FRAME_PART_SIZE` (256MB). Bodies are per-file source buffers and +`module.wrap` already caps at Node's 512MB string limit, so this ceiling cannot +reject a payload that previously worked. + +The child's exit code tells the parent how to treat a failure: + +| Exit code | Meaning | Handling | +| --------- | -------------------------------------------------------------------- | ---------------------------------------------------------------------------------------- | +| 0 | Bytecode produced | Blob is delivered to the packer | +| 2 | Well-formed request V8 refused to compile | Per-file failure: degrade to source with `--fallback-to-source`, otherwise skip the file | +| 3 | Framing violation / channel desync (`FABRICATOR_PROTOCOL_EXIT_CODE`) | Build fails loudly — a desynced channel must never degrade to source | + +On the parent side, every response header is validated against the same ceiling by +`tryParseFabricatorResponse()` (shared with the unit tests), unconsumed remainder +bytes are carried over to the next request, and each request is guarded by +`FABRICATOR_RESPONSE_TIMEOUT_MS`. A bounded tail of the child's stderr is attached +to failure messages so the cause is visible without `--debug`. + ### Binary Format The traditional executable has this layout: @@ -632,7 +662,7 @@ With `node:vfs` and `"useVfs": true` in the SEA config, assets will be auto-moun | `lib/producer.ts` | ~601 | Assembles final binary (payload injection, compression) | | `lib/sea.ts` | ~672 | SEA orchestrator (seaEnhanced + simple sea, single-bootstrap dispatch) | | `lib/sea-assets.ts` | ~188 | Generates single archive blob + manifest with offsets | -| `lib/fabricator.ts` | ~173 | V8 bytecode compilation (traditional mode only) | +| `lib/fabricator.ts` | ~460 | V8 bytecode compilation + frame IPC (traditional mode only) | | `lib/esm-transformer.ts` | ~434 | ESM to CJS transformation (traditional mode only) | | `lib/refiner.ts` | ~110 | Path compression, empty directory pruning | | `lib/common.ts` | ~375 | Path normalization, snapshot helpers, store constants | diff --git a/lib/fabricator.ts b/lib/fabricator.ts index f062c7b57..23f178fe5 100644 --- a/lib/fabricator.ts +++ b/lib/fabricator.ts @@ -3,12 +3,32 @@ import { Readable, Writable } from 'stream'; import { log } from './log'; import { Target } from './types'; -const FABRICATOR_MAX_FRAME_PART_SIZE = 256 * 1024 * 1024; +// Upper bound for one framed part (snap path or file body). Bodies are +// per-file source buffers and `module.wrap` already caps at Node's 512MB +// string limit, so this ceiling cannot reject a payload that previously +// worked. See docs/ARCHITECTURE.md for the framing description. +export const FABRICATOR_MAX_FRAME_PART_SIZE = 256 * 1024 * 1024; + +// Exit code for framing violations (corrupt size headers). Distinct from +// exit 2, which means a well-formed request V8 refused to compile, so a +// desynced pipe can never silently degrade to `--fallback-to-source`. +export const FABRICATOR_PROTOCOL_EXIT_CODE = 3; + +// A child that accepts a valid frame and then never answers must not stall +// the build forever. There is no other timeout on this path. +export const FABRICATOR_RESPONSE_TIMEOUT_MS = 60 * 1000; + +// Bounded tail of child stderr kept per child and attached to failures so +// non-debug users see the cause (e.g. `Pkg: Cached data not produced.`). +export const FABRICATOR_STDERR_TAIL_MAX_BYTES = 16 * 1024; + +const FABRICATOR_CLOSE_SNIPPET_MAX_CHARS = 512; export const fabricatorScript = ` var vm = require('vm'); var module = require('module'); var MAX_FRAME_PART_SIZE = ${FABRICATOR_MAX_FRAME_PART_SIZE}; + var PROTOCOL_EXIT_CODE = ${FABRICATOR_PROTOCOL_EXIT_CODE}; var stdin = Buffer.alloc(0); process.stdin.on('data', function (data) { stdin = Buffer.concat([ stdin, data ]); @@ -16,13 +36,13 @@ export const fabricatorScript = ` var sizeOfSnap = stdin.readInt32LE(0); if (sizeOfSnap < 0 || sizeOfSnap > MAX_FRAME_PART_SIZE) { console.error('Pkg: Invalid snap size header: ' + sizeOfSnap); - process.exit(2); + process.exit(PROTOCOL_EXIT_CODE); } if (stdin.length < 4 + sizeOfSnap + 4) break; var sizeOfBody = stdin.readInt32LE(4 + sizeOfSnap); if (sizeOfBody < 0 || sizeOfBody > MAX_FRAME_PART_SIZE) { console.error('Pkg: Invalid body size header: ' + sizeOfBody); - process.exit(2); + process.exit(PROTOCOL_EXIT_CODE); } var totalSize = 4 + sizeOfSnap + 4 + sizeOfBody; if (stdin.length < totalSize) break; @@ -32,8 +52,16 @@ export const fabricatorScript = ` var startOfBody = 4 + sizeOfSnap + 4; stdin.copy(body, 0, startOfBody, startOfBody + sizeOfBody); - // Preserve unconsumed bytes for subsequent payloads - stdin = stdin.subarray(totalSize); + // Preserve unconsumed bytes for subsequent payloads, without pinning + // a large backing store behind a small tail. + var rest = stdin.subarray(totalSize); + if (rest.length === 0) { + stdin = Buffer.alloc(0); + } else if (totalSize > 65536 && rest.length * 4 < totalSize) { + stdin = Buffer.from(rest); + } else { + stdin = rest; + } var code = module.wrap(body); var s = new vm.Script(code, { @@ -55,28 +83,125 @@ export const fabricatorScript = ` process.stdin.resume(); `; -const children: Record< - string, - ChildProcessByStdio -> = {}; +export type FabricatorFrameParseResult = + | { status: 'ok'; frame: Buffer; remainder: Buffer } + | { status: 'incomplete' } + | { status: 'protocol-error'; message: string }; -export function buildFabricatorRequestChunks( - snap: string, - body: Buffer, -): [Buffer, Buffer, Buffer, Buffer] { - const snapBuf = Buffer.from(snap); +// Single source of truth for the response side of the frame protocol: +// accumulate, validate the header against the shared max, slice one frame, +// keep the remainder. Used by the parent decoder; the child script keeps +// its own inline copy since it must stay self-contained `-e` source text. +export function tryParseFabricatorResponse( + buffer: Buffer, +): FabricatorFrameParseResult { + if (buffer.length < 4) { + return { status: 'incomplete' }; + } - if (snapBuf.length > FABRICATOR_MAX_FRAME_PART_SIZE) { - throw new Error( - `Fabricator snap exceeds max frame size (${snapBuf.length} bytes)`, - ); + const sizeOfBlob = buffer.readInt32LE(0); + + if (sizeOfBlob < 0 || sizeOfBlob > FABRICATOR_MAX_FRAME_PART_SIZE) { + return { + status: 'protocol-error', + message: `Invalid blob size header: ${sizeOfBlob}`, + }; + } + + if (buffer.length < 4 + sizeOfBlob) { + return { status: 'incomplete' }; } - if (body.length > FABRICATOR_MAX_FRAME_PART_SIZE) { - throw new Error( - `Fabricator body exceeds max frame size (${body.length} bytes)`, + const frame = Buffer.alloc(sizeOfBlob); + buffer.copy(frame, 0, 4, 4 + sizeOfBlob); + + // Copy the tail so leftover bytes never pin the whole backing store. + const remainder = Buffer.from(buffer.subarray(4 + sizeOfBlob)); + + return { status: 'ok', frame, remainder }; +} + +export function checkFabricatorFramePartSize( + label: 'snap' | 'body', + byteLength: number, +): void { + if (byteLength > FABRICATOR_MAX_FRAME_PART_SIZE) { + const error = new Error( + `Fabricator ${label} exceeds max frame size (${byteLength} bytes)`, ); + (error as NodeJS.ErrnoException).code = 'FABRICATOR_FRAME_TOO_LARGE'; + throw error; } +} + +export function isDeterministicFabricatorError(error?: Error | null): boolean { + return ( + !!error && + (error as NodeJS.ErrnoException).code === 'FABRICATOR_FRAME_TOO_LARGE' + ); +} + +export function fabricatorProtocolError(message: string): Error { + const error = new Error(`FABRICATOR_PROTOCOL: ${message}`); + (error as NodeJS.ErrnoException).code = 'FABRICATOR_PROTOCOL'; + return error; +} + +export function isFabricatorProtocolError(error?: Error | null): boolean { + return ( + !!error && (error as NodeJS.ErrnoException).code === 'FABRICATOR_PROTOCOL' + ); +} + +// Printable-safe snippet for error messages. JSON.stringify escapes rather +// than dumping raw (possibly binary) bytes to the terminal. +export function toPrintableSnippet( + buffer: Buffer, + maxChars: number = FABRICATOR_CLOSE_SNIPPET_MAX_CHARS, +): string { + return JSON.stringify(buffer.toString('utf8').trim().slice(0, maxChars)); +} + +interface FabricatorChildState { + proc: ChildProcessByStdio; + // Unconsumed response bytes. The child may pipeline answers while the + // parent still holds earlier ones; the remainder belongs to the next call. + stdoutBuf: Buffer; + stderrTail: Buffer[]; + stderrTailLength: number; +} + +const children: Record = {}; + +function appendStderrTail(state: FabricatorChildState, data: Buffer) { + state.stderrTail.push(data); + state.stderrTailLength += data.length; + + while ( + state.stderrTailLength > FABRICATOR_STDERR_TAIL_MAX_BYTES && + state.stderrTail.length > 1 + ) { + const shifted = state.stderrTail.shift() as Buffer; + state.stderrTailLength -= shifted.length; + } +} + +function stderrTailText(state: FabricatorChildState): string { + if (state.stderrTailLength === 0) { + return ''; + } + + return Buffer.concat(state.stderrTail).toString('utf8').trim().slice(0, 2000); +} + +export function buildFabricatorRequestChunks( + snap: string, + body: Buffer, +): Buffer[] { + const snapBuf = Buffer.from(snap); + + checkFabricatorFramePartSize('snap', snapBuf.length); + checkFabricatorFramePartSize('body', body.length); // Keep separate header buffers so async stream writes cannot mutate // previously queued bytes. @@ -107,20 +232,36 @@ export function fabricate( const cmd = fabricator.binaryPath; const key = JSON.stringify([cmd, activeBakes]); - let child = children[key]; - if (!child) { - children[key] = spawn(cmd, activeBakes.concat('-e', fabricatorScript), { + if (!children[key]) { + const proc = spawn(cmd, activeBakes.concat('-e', fabricatorScript), { stdio: ['pipe', 'pipe', 'pipe'], env: { PKG_EXECPATH: 'PKG_INVOKE_NODEJS' }, }); - child = children[key]; + const state: FabricatorChildState = { + proc, + stdoutBuf: Buffer.alloc(0), + stderrTail: [], + stderrTailLength: 0, + }; + children[key] = state; + + proc.stderr.on('data', (data: Buffer) => { + appendStderrTail(state, data); - if (child.stderr) { - child.stderr.on('data', (data: Buffer) => { + if (log.debugMode) { log.debug(`fabricator: ${data.toString().trim()}`); - }); - } + } + }); + } + + const state = children[key]; + const child = state.proc; + let settled = false; + + function tailSuffix(): string { + const tail = stderrTailText(state); + return tail ? ` stderr: ${tail}` : ''; } function kill() { @@ -128,46 +269,116 @@ export function fabricate( child.kill(); } - let stdout = Buffer.alloc(0); + const timer = setTimeout(() => { + if (settled) { + return; + } + + settled = true; + removeListeners(); + kill(); + cb( + fabricatorProtocolError( + `timed out after ${FABRICATOR_RESPONSE_TIMEOUT_MS}ms waiting for bytecode ${fabricator.nodeRange}-${fabricator.arch} for file ${snap}${tailSuffix()}`, + ), + ); + }, FABRICATOR_RESPONSE_TIMEOUT_MS); function onError(error: Error) { + if (settled) { + return; + } + + settled = true; + clearTimeout(timer); removeListeners(); kill(); cb( new Error( - `Failed to make bytecode ${fabricator.nodeRange}-${fabricator.arch} for file ${snap} error (${error.message})`, + `Failed to make bytecode ${fabricator.nodeRange}-${fabricator.arch} for file ${snap} error (${error.message})${tailSuffix()}`, ), ); } - function onClose(code: number) { + function onClose(code: number | null) { + if (settled) { + return; + } + + settled = true; + clearTimeout(timer); removeListeners(); kill(); + + if (code === FABRICATOR_PROTOCOL_EXIT_CODE) { + return cb( + fabricatorProtocolError( + `fabricator channel desynced for file ${snap} (exit ${code})${tailSuffix()}`, + ), + ); + } + if (code !== 0) { return cb( new Error( - `Failed to make bytecode ${fabricator.nodeRange}-${fabricator.arch} for file ${snap}`, + `Failed to make bytecode ${fabricator.nodeRange}-${fabricator.arch} for file ${snap} (exit ${code})${tailSuffix()}`, ), ); } - if (stdout.length > 0) { - log.debug(`fabricator: unexpected close output: ${stdout.toString()}`); + if (state.stdoutBuf.length > 0) { + return cb( + new Error( + `${cmd} closed unexpectedly, output: ${toPrintableSnippet(state.stdoutBuf)}${tailSuffix()}`, + ), + ); } - return cb(new Error(`${cmd} closed unexpectedly`)); + + return cb(new Error(`${cmd} closed unexpectedly${tailSuffix()}`)); + } + + // Tries to deliver one frame from the buffered bytes. Returns true when + // the call settled (frame delivered or fatal protocol error). + function tryDeliver(): boolean { + const parsed = tryParseFabricatorResponse(state.stdoutBuf); + + if (parsed.status === 'protocol-error') { + settled = true; + clearTimeout(timer); + removeListeners(); + kill(); + cb( + fabricatorProtocolError( + `invalid fabricator response for file ${snap}: ${parsed.message}${tailSuffix()}`, + ), + ); + return true; + } + + if (parsed.status === 'ok') { + settled = true; + clearTimeout(timer); + state.stdoutBuf = parsed.remainder; + removeListeners(); + cb(undefined, parsed.frame); + return true; + } + + return false; } function onData(data: Buffer) { - stdout = Buffer.concat([stdout, data]); - if (stdout.length >= 4) { - const sizeOfBlob = stdout.readInt32LE(0); - if (stdout.length >= 4 + sizeOfBlob) { - const blob = Buffer.alloc(sizeOfBlob); - stdout.copy(blob, 0, 4, 4 + sizeOfBlob); - removeListeners(); - return cb(undefined, blob); - } + if (settled) { + return; } + + // Responses are single small frames, so concatenating per event is + // bounded; the timeout above guards the only unbounded wait. + state.stdoutBuf = + state.stdoutBuf.length === 0 + ? data + : Buffer.concat([state.stdoutBuf, data]); + tryDeliver(); } function removeListeners() { @@ -184,10 +395,18 @@ export function fabricate( child.stdout.on('error', onError); child.stdout.on('data', onData); - let requestChunks: [Buffer, Buffer, Buffer, Buffer]; + // A previous call may have left a pipelined response buffered; deliver it + // without waiting for a new `data` event that will never come. + if (state.stdoutBuf.length >= 4 && tryDeliver()) { + return; + } + + let requestChunks: Buffer[]; try { requestChunks = buildFabricatorRequestChunks(snap, body); } catch (error) { + settled = true; + clearTimeout(timer); removeListeners(); return cb( new Error( @@ -213,7 +432,22 @@ export function fabricateTwice( // probably because of 'filename' cache. also, there are weird cases // when node4 can not compile as well, for example file 'lib/js-yaml/dumper.js' // of package js-yaml@3.9.0 does not get bytecode second time on node4-win-x64 - if (error) return fabricate(bakes, fabricator, snap, body, cb); + if (error) { + // Deterministic failures (oversize payload) and desynced channels + // fail loudly instead of being retried and/or degraded to source. + if ( + isDeterministicFabricatorError(error) || + isFabricatorProtocolError(error) + ) { + return cb(error); + } + + log.debug( + `fabricator: first attempt failed for ${snap}: ${error.message}; retrying`, + ); + return fabricate(bakes, fabricator, snap, body, cb); + } + cb(undefined, buffer); }); } @@ -221,9 +455,9 @@ export function fabricateTwice( export function shutdown() { for (const key in children) { if (children[key]) { - const child = children[key]; + const state = children[key]; delete children[key]; - child.kill(); + state.proc.kill(); } } } diff --git a/lib/producer.ts b/lib/producer.ts index 371a2879a..58c95b0ae 100644 --- a/lib/producer.ts +++ b/lib/producer.ts @@ -10,7 +10,7 @@ import { Readable } from 'stream'; import { STORE_BLOB, STORE_CONTENT, isDotNODE, snapshotify } from './common'; import { log, wasReported } from './log'; -import { fabricateTwice } from './fabricator'; +import { fabricateTwice, isFabricatorProtocolError } from './fabricator'; import { platform, SymLinks, Target } from './types'; import { Stripe } from './packer'; import { CompressType, getZstdCompressStream } from './compress_type'; @@ -484,6 +484,13 @@ export default function producer({ if (error) { const file = stripe.file ?? snap; + if (isFabricatorProtocolError(error)) { + // A desynced fabricator channel must never degrade to + // plain source: fail loudly instead of shipping a + // green build without bytecode. + return cb(error, null); + } + if (fallbackToSource) { log.warn( `Failed to generate V8 bytecode for ${file}. Shipping source instead. Cause: ${error.message}`, diff --git a/test/unit/fabricator.test.ts b/test/unit/fabricator.test.ts index 30ac45f37..302ac634e 100644 --- a/test/unit/fabricator.test.ts +++ b/test/unit/fabricator.test.ts @@ -1,27 +1,142 @@ import assert from 'node:assert/strict'; import { spawn } from 'node:child_process'; import { describe, it } from 'node:test'; +import { setTimeout as delay } from 'node:timers/promises'; import { buildFabricatorRequestChunks, + checkFabricatorFramePartSize, + fabricate, + FABRICATOR_MAX_FRAME_PART_SIZE, + FABRICATOR_PROTOCOL_EXIT_CODE, + fabricatorProtocolError, fabricatorScript, + isDeterministicFabricatorError, + isFabricatorProtocolError, + shutdown, + toPrintableSnippet, + tryParseFabricatorResponse, } from '../../lib/fabricator'; +import { Target } from '../../lib/types'; -function parseBlobFrames(buffer: Buffer): Buffer[] { - const out: Buffer[] = []; - let offset = 0; +const CHILD_CLOSE_TIMEOUT_MS = 30 * 1000; +const FABRICATE_TIMEOUT_MS = 60 * 1000; +// Long enough that the child processes each staged write as its own `data` +// event instead of coalescing them. +const WRITE_GAP_MS = 50; - while (offset + 4 <= buffer.length) { - const sizeOfBlob = buffer.readInt32LE(offset); - if (sizeOfBlob < 0 || offset + 4 + sizeOfBlob > buffer.length) break; +function withTimeout( + promise: Promise, + ms: number, + label: string, +): Promise { + let timer: NodeJS.Timeout | undefined; + const timeout = new Promise((_resolve, reject) => { + timer = setTimeout( + () => reject(new Error(`timed out waiting for ${label}`)), + ms, + ); + }); + + return Promise.race([promise, timeout]).finally(() => { + clearTimeout(timer); + }); +} + +interface ChildRun { + code: number | null; + stdout: Buffer; + stderr: string; +} + +// Writes each chunk with a delay in between so chunk boundaries survive to +// the child as separate `data` events, then ends stdin with the tail. +async function runFabricatorScript( + writes: Buffer[], + endChunk?: Buffer, +): Promise { + const child = spawn(process.execPath, ['-e', fabricatorScript], { + stdio: ['pipe', 'pipe', 'pipe'], + }); + + const stdoutChunks: Buffer[] = []; + const stderrChunks: Buffer[] = []; + + child.stdout.on('data', (chunk: Buffer) => { + stdoutChunks.push(chunk); + }); + + child.stderr.on('data', (chunk: Buffer) => { + stderrChunks.push(chunk); + }); + + // Attach before any await: a child that exits during the staged writes + // below may fully tear down before a later-attached listener runs, and + // Node never re-emits `exit`/`close` to late subscribers. Resolve on + // `exit` or `close`, whichever comes first — ending stdin after the + // child already exited can additionally leave `close` never firing. + const closePromise = new Promise((resolve) => { + child.once('exit', (exitCode) => resolve(exitCode)); + child.once('close', (closeCode) => resolve(closeCode)); + child.once('error', () => resolve(null)); + }); + + for (const write of writes) { + child.stdin.write(write); + await delay(WRITE_GAP_MS); + } - const blob = Buffer.alloc(sizeOfBlob); - buffer.copy(blob, 0, offset + 4, offset + 4 + sizeOfBlob); - out.push(blob); - offset += 4 + sizeOfBlob; + if (endChunk) { + child.stdin.end(endChunk); + } else { + child.stdin.end(); } - return out; + // Resolve on `exit` or `close`, whichever comes first. Ending stdin + // after the child already exited can leave `close` never firing even + // though the process is gone, so `close` alone is not reliable here. + const code = await withTimeout( + closePromise, + CHILD_CLOSE_TIMEOUT_MS, + 'fabricator child close', + ); + + return { + code, + stdout: Buffer.concat(stdoutChunks), + stderr: Buffer.concat(stderrChunks).toString('utf8'), + }; +} + +// Decodes every frame with the production parser (not a test-local copy). +function parseAllFrames(buffer: Buffer): Buffer[] { + const frames: Buffer[] = []; + let rest = buffer; + + for (;;) { + const parsed = tryParseFabricatorResponse(rest); + + if (parsed.status === 'incomplete') { + break; + } + + if (parsed.status === 'protocol-error') { + throw new Error(parsed.message); + } + + frames.push(parsed.frame); + rest = parsed.remainder; + + if (rest.length === 0) { + break; + } + } + + return frames; +} + +function frameFor(snap: string, source: string): Buffer { + return Buffer.concat(buildFabricatorRequestChunks(snap, Buffer.from(source))); } describe('fabricator framing', () => { @@ -41,70 +156,230 @@ describe('fabricator framing', () => { }); it('child script preserves trailing bytes and decodes multiple payloads', async () => { - const child = spawn(process.execPath, ['-e', fabricatorScript], { - stdio: ['pipe', 'pipe', 'pipe'], - }); + const frame1 = frameFor('/snapshot/one.js', 'module.exports = 1;'); + const frame2 = frameFor('/snapshot/two.js', 'module.exports = 2;'); - const stdoutChunks: Buffer[] = []; - const stderrChunks: Buffer[] = []; + // Split offsets inside the size header, straddling it, and late in the + // frame. Each offset runs in its own child so staged writes cannot + // coalesce and the partial-header resume path must execute. + const splitOffsets = [1, 3, 4, 5, 9, frame2.length - 1]; - child.stdout.on('data', (chunk: Buffer) => { - stdoutChunks.push(chunk); - }); + for (const splitAt of splitOffsets) { + const run = await runFabricatorScript( + [frame1, frame2.subarray(0, splitAt)], + frame2.subarray(splitAt), + ); - child.stderr.on('data', (chunk: Buffer) => { - stderrChunks.push(chunk); - }); + assert.equal(run.code, 0, `splitAt=${splitAt}: ${run.stderr}`); - const frame1 = Buffer.concat( - buildFabricatorRequestChunks( - '/snapshot/one.js', - Buffer.from('module.exports = 1;'), - ), - ); - const frame2 = Buffer.concat( - buildFabricatorRequestChunks( - '/snapshot/two.js', - Buffer.from('module.exports = 2;'), - ), - ); + const frames = parseAllFrames(run.stdout); + assert.equal(frames.length, 2, `splitAt=${splitAt}`); + assert.ok(frames[0].length > 0, `splitAt=${splitAt}`); + assert.ok(frames[1].length > 0, `splitAt=${splitAt}`); + } + }); + + it('child script rejects invalid snap size headers', async () => { + const cases: Array<[string, number]> = [ + ['negative', -1], + ['oversize', FABRICATOR_MAX_FRAME_PART_SIZE + 1], + ]; - const splitAt = 3; - child.stdin.write(Buffer.concat([frame1, frame2.subarray(0, splitAt)])); - child.stdin.end(frame2.subarray(splitAt)); + for (const [name, size] of cases) { + const badHeader = Buffer.alloc(4); + badHeader.writeInt32LE(size, 0); - const [code] = (await new Promise((resolve) => { - child.on('close', (closeCode, signal) => resolve([closeCode, signal])); - })) as [number | null, NodeJS.Signals | null]; + const run = await runFabricatorScript([badHeader]); + + // Exit code alone proves the guard fired and distinguishes the + // framing path from every compile-failure path (exit 2). The stderr + // text is intentionally not asserted: a piped write immediately + // before exit can truncate on some platforms. + assert.equal(run.code, FABRICATOR_PROTOCOL_EXIT_CODE, name); + } + }); + + it('child script rejects invalid body size headers', async () => { + // A valid snap prefix proves the *body* guard fired: the snap guard + // already passed, so only the body header check can exit non-zero here. + const snap = '/snapshot/two.js'; + const snapBuf = Buffer.from(snap); + const h1 = Buffer.alloc(4); + h1.writeInt32LE(snapBuf.length, 0); + const hBad = Buffer.alloc(4); + hBad.writeInt32LE(-1, 0); + + const run = await runFabricatorScript([Buffer.concat([h1, snapBuf, hBad])]); + + assert.equal(run.code, FABRICATOR_PROTOCOL_EXIT_CODE); + }); - assert.equal(code, 0, Buffer.concat(stderrChunks).toString()); + it('child script compiles zero-length snap and body payloads', async () => { + const run = await runFabricatorScript([ + frameFor('', 'module.exports = 1;'), + frameFor('/snapshot/empty.js', ''), + ]); - const frames = parseBlobFrames(Buffer.concat(stdoutChunks)); + assert.equal(run.code, 0, run.stderr); + + const frames = parseAllFrames(run.stdout); assert.equal(frames.length, 2); - assert.ok(frames[0].length > 0); - assert.ok(frames[1].length > 0); }); - it('child script rejects invalid size headers', async () => { - const child = spawn(process.execPath, ['-e', fabricatorScript], { - stdio: ['pipe', 'pipe', 'pipe'], + it('fabricate() covers both halves end to end', async () => { + const target = { + nodeRange: 'node22', + arch: 'x64', + platform: 'linux', + binaryPath: process.execPath, + output: '', + fabricator: undefined, + } as unknown as Target; + + try { + const buffer = await withTimeout( + new Promise((resolve, reject) => { + fabricate( + [], + target, + '/snapshot/integration.js', + Buffer.from('module.exports = 42;'), + (error, result) => { + if (error) { + reject(error); + } else { + resolve(result); + } + }, + ); + }), + FABRICATE_TIMEOUT_MS, + 'fabricate() response', + ); + + assert.ok(buffer && buffer.length > 0); + } finally { + shutdown(); + } + }); + + describe('tryParseFabricatorResponse', () => { + it('reports incomplete buffers', () => { + assert.equal( + tryParseFabricatorResponse(Buffer.alloc(0)).status, + 'incomplete', + ); + assert.equal( + tryParseFabricatorResponse(Buffer.alloc(2)).status, + 'incomplete', + ); + + const partial = Buffer.alloc(4); + partial.writeInt32LE(10, 0); + assert.equal(tryParseFabricatorResponse(partial).status, 'incomplete'); + assert.equal( + tryParseFabricatorResponse(Buffer.concat([partial, Buffer.alloc(5)])) + .status, + 'incomplete', + ); }); - const stderrChunks: Buffer[] = []; - child.stderr.on('data', (chunk: Buffer) => { - stderrChunks.push(chunk); + it('slices one frame and keeps the remainder', () => { + const blob = Buffer.from([1, 2, 3]); + const header = Buffer.alloc(4); + header.writeInt32LE(blob.length, 0); + const trailing = Buffer.from([9, 9]); + const parsed = tryParseFabricatorResponse( + Buffer.concat([header, blob, trailing]), + ); + + assert.equal(parsed.status, 'ok'); + + if (parsed.status === 'ok') { + assert.deepEqual(parsed.frame, blob); + assert.deepEqual(parsed.remainder, trailing); + } }); - const badHeader = Buffer.alloc(4); - badHeader.writeInt32LE(-1, 0); - child.stdin.end(badHeader); + it('decodes zero-length frames', () => { + const header = Buffer.alloc(4); + header.writeInt32LE(0, 0); + const parsed = tryParseFabricatorResponse(header); - const [code] = (await new Promise((resolve) => { - child.on('close', (closeCode, signal) => resolve([closeCode, signal])); - })) as [number | null, NodeJS.Signals | null]; + assert.equal(parsed.status, 'ok'); - const stderr = Buffer.concat(stderrChunks).toString(); - assert.equal(code, 2); - assert.match(stderr, /Invalid snap size header/); + if (parsed.status === 'ok') { + assert.equal(parsed.frame.length, 0); + assert.equal(parsed.remainder.length, 0); + } + }); + + it('rejects negative and oversize headers', () => { + const negative = Buffer.alloc(4); + negative.writeInt32LE(-1, 0); + const negativeParsed = tryParseFabricatorResponse(negative); + assert.equal(negativeParsed.status, 'protocol-error'); + + const oversize = Buffer.alloc(4); + oversize.writeInt32LE(FABRICATOR_MAX_FRAME_PART_SIZE + 1, 0); + const oversizeParsed = tryParseFabricatorResponse(oversize); + assert.equal(oversizeParsed.status, 'protocol-error'); + + // Garbage text on the response stream (e.g. bake output leaking to + // stdout) decodes to an implausible size and must be a protocol + // error, never a hang or a throw inside the data handler. + const garbage = Buffer.from('[123:0x5 garbage trace'); + assert.equal( + tryParseFabricatorResponse(garbage).status, + 'protocol-error', + ); + }); + }); + + describe('frame size limits', () => { + it('accepts payloads at the ceiling without allocating it', () => { + checkFabricatorFramePartSize('snap', FABRICATOR_MAX_FRAME_PART_SIZE); + checkFabricatorFramePartSize('body', FABRICATOR_MAX_FRAME_PART_SIZE); + assert.equal(isDeterministicFabricatorError(new Error('other')), false); + }); + + it('tags oversize payloads as deterministic failures', () => { + for (const label of ['snap', 'body'] as const) { + assert.throws( + () => + checkFabricatorFramePartSize( + label, + FABRICATOR_MAX_FRAME_PART_SIZE + 1, + ), + /exceeds max frame size/, + ); + } + + try { + checkFabricatorFramePartSize( + 'body', + FABRICATOR_MAX_FRAME_PART_SIZE + 1, + ); + assert.fail('expected checkFabricatorFramePartSize to throw'); + } catch (error) { + assert.equal(isDeterministicFabricatorError(error as Error), true); + assert.equal(isFabricatorProtocolError(error as Error), false); + } + }); + }); + + describe('protocol errors', () => { + it('are distinguishable from compile failures', () => { + const error = fabricatorProtocolError('desynced'); + assert.match(error.message, /FABRICATOR_PROTOCOL/); + assert.equal(isFabricatorProtocolError(error), true); + assert.equal(isFabricatorProtocolError(new Error('other')), false); + assert.equal(isFabricatorProtocolError(undefined), false); + }); + + it('printable snippets never leak raw binary', () => { + const snippet = toPrintableSnippet(Buffer.from([0, 1, 2, 104, 105])); + assert.ok(!snippet.includes(String.fromCharCode(0))); + }); }); });