fix(fabricator): harden framing, prevent stdio deadlocks, and add regression tests - #294
fix(fabricator): harden framing, prevent stdio deadlocks, and add regression tests#294chrhoffmann wants to merge 1 commit into
Conversation
|
Hi @chrhoffmann and thanks for your PR! I will back from vacation on Monday and i will review this ASAP! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #294 +/- ##
==========================================
- Coverage 87.19% 87.10% -0.10%
==========================================
Files 23 23
Lines 7929 7985 +56
Branches 1214 1218 +4
==========================================
+ Hits 6914 6955 +41
- Misses 1008 1023 +15
Partials 7 7
🚀 New features to boost your workflow:
|
robertsLando
left a comment
robertsLando
left a comment
There was a problem hiding this comment.
Summary
Verdict: Ship with minor changes.
The child-side rewrite is genuinely correct — I fuzzed it against every one of the 119 split offsets plus byte-at-a-time delivery of a 3-frame stream and a 3MB body: zero failures. The three new tests are real regressions, not tautologies: against the base script the multi-frame case yields frames=1, expected 2, and the bad-header case exits 0 silently, which is the actual silent-corruption path. Dropping the raw console.log(stdout.toString()) binary dump is right. buildFabricatorRequestChunks is genuinely wired into fabricate(), not test-only.
Two things to settle before merge, and one correction to the PR description.
Top 3 risks
- The one reachable fabricator hang lives in the parent decoder this PR didn't touch.
- Non-debug users now see strictly less on failure than before this PR.
- A protocol corruption silently degrades to
--fallback-to-source, so a real bug ships as a green build.
Themes
- One protocol, two half-implementations. The child got
while+ bounds guards + trailing-byte retention; the parent decoder got none of it. Four independent review passes landed on this. - The correct parser exists only in the test file.
parseBlobFrameshandles multi-frame and negative sizes; production'sonDatadoes neither. - Diagnosability moved backwards, in a PR whose stated purpose is explaining failures.
Findings that fall outside the diff (can't be line-anchored)
Major — lib/fabricator.ts:160-171, the parent's onData
This reads sizeOfBlob with none of the guards just added to the child, and it is reachable, not theoretical:
bakes come from the user's --options (lib/index.ts:171), and fabricate's filter strips only --prof/--v8-options/--trace-opt/--trace-deopt. So --options trace-gc writes GC traces to stdout, straight into the framed response stream:
Buffer.from('[123:0x5').readInt32LE(0)=858927451→stdout.length >= 4 + sizeOfBlobnever becomes true,cbnever fires, and pkg hangs forever (there is no timeout anywhere in this path).- A byte with the high bit set gives a negative size →
stdout.length >= 4 + (-1)is true →Buffer.alloc(-1)throwsERR_OUT_OF_RANGEinside a'data'handler, uncaught, crashing pkg instead of routing throughonError/--fallback-to-source.
This is pre-existing, so I'm not treating it as a merge blocker — but it's the only actual deadlock in this file, and the PR title is "prevent stdio deadlocks". Worth either fixing here or being explicit that it's out of scope.
Separately: onData consumes one frame and drops everything past 4 + sizeOfBlob, and removeListener('data', ...) does not pause a flowing stream. Today that's masked only because producer.ts:478 issues one request per Multistream callback — and note the child's old stdin = Buffer.alloc(0) used to enforce that lock-step. This PR removes that enforcement, so the child can now pipeline while the parent still can't read it.
Major — no timeout on the child's response
If the child accepts a valid frame and then hangs, fabricate never calls back and pkg stalls with no log line. That's the failure mode immediately adjacent to the one this PR targets.
Major — tests don't reach the parent half
The tests exercise the extracted script and helper; fabricate() itself, the parent decode, and the stderr routing change are all untested — and that untested half is where every remaining gap is. Driving the tests through fabricate() with a Target whose binaryPath is process.execPath would cover both halves, and would remove the need for the two new exports.
Minor — lib/fabricator.ts:143
onClose(code: number) but 'close' emits number | null. Runtime behaviour is fine; the type isn't.
On the PR description
I tried to reproduce all three claimed root causes. Results:
- Claim A (shared header buffer aliasing) — real Node hazard, not reachable here. A repro confirms
Writable.write()retains Buffers by reference: with 1MB pre-filled to force backpressure, the base sendsH1=3145728instead ofH1=36. But at the mutation point onlyh(4B) +snap(~40B) are in flight — far under the 64KB pipe buffer — souv_try_writecompletes them synchronously. Without pre-filled backpressure (4 configurations tried, including a child delaying reads by 400ms) the base is uncorrupted. Worth the 2-line fix as latent-hazard cleanup; it is not a hang anyone is hitting. - Claim B (trailing-byte truncation) — theoretical.
fabricatehas exactly one caller (lib/producer.ts:478), inside aMultistreamfactory, andmultistreamcalls_next()only from the current stream'sonEnd. Targets are serialized too (lib/index.ts:313-326). One request → full response → next. Trailing bytes can never exist on the child's stdin. - Claim C (debug stderr causes backpressure deadlock) — not a bug. The base passes
process.stdoutasstdio[2], so Node inherits the fd rather than creating an unread pipe. Repro: child writes 5MB to stderr → exit 0 in 587ms. The PR's'pipe'does correctly attach a reader, so it doesn't introduce one either. Net: a routing change, not a deadlock fix.
The changes are still worth having — but the description reads as three observed deadlocks, and I could not reproduce any of them as such. Could you share the actual reproducer, or retitle to drop the deadlock claim? Also, the validation section cites npm run test:unit / npm run lint / npm run build; this repo is yarn-only at the root (npm would create a stray package-lock.json).
Two things verified clean, for the record: non-ASCII snap paths work correctly (toString('utf8', 4, 4 + sizeOfSnap) takes byte offsets, matching Buffer.from(snap) — checked end-to-end with /snapshot/ünïcodé-èà.js), and the child.stderr listener is attached once per spawn inside if (!child) with kill() deleting the cache key, so there's no leak. The 256MB ceiling also can't regress real payloads — bodies are per-file source buffers and module.wrap already caps at Node's 512MB string limit.
Coverage: correctness, DRY, performance, design/API, tests, operability, readability. Security not run — no files in its lane (build-time IPC, no auth/crypto/network surface). No prior unresolved review threads.
|
|
||
| if (child.stderr) { | ||
| child.stderr.on('data', (data: Buffer) => { | ||
| log.debug(`fabricator: ${data.toString().trim()}`); |
There was a problem hiding this comment.
[Major] · Operability
Child stderr is now captured but fed only to log.debug, which is a no-op unless --debug is passed. Combined with the change at line 154-156, a default (non-debug) user now sees strictly less than before this PR.
Why: the observable outcome for a non-debug user is identical to the old 'ignore' — Failed to make bytecode X-Y for file Z with zero indication of cause — even though Pkg: Cached data not produced. is now captured and then discarded. The pipe is paid for and returns nothing to the people who actually hit the failure. This is the one behaviour change in the PR that makes diagnosis harder rather than easier.
Fix: buffer a bounded tail of the child's stderr per child and attach it to the onClose/onError message, rather than only log.debug-ing it.
(Minor, same line: data.toString().trim() runs on every stderr chunk even when log.debugMode is false — log.debug early-returns, but only after the decode already happened. Cheap to guard.)
|
|
||
| console.log(stdout.toString()); | ||
| if (stdout.length > 0) { | ||
| log.debug(`fabricator: unexpected close output: ${stdout.toString()}`); |
There was a problem hiding this comment.
[Major] · Operability
Demoting the unexpected-close output from an unconditional console.log to log.debug removes the last diagnostic a non-debug user had on this path.
Why: dropping the raw binary dump is right — it was ugly and could spew non-text to stdout. But the replacement is invisible without --debug, so ${cmd} closed unexpectedly now arrives with no context at all. Same root cause as the stderr routing at line 119-123.
Fix: keep it out of the default stdout stream, but surface a trimmed, printable-safe snippet in the error itself so it reaches users who aren't running with --debug.
| } | ||
| if (sizeOfSnap < 0 || sizeOfSnap > MAX_FRAME_PART_SIZE) { | ||
| console.error('Pkg: Invalid snap size header: ' + sizeOfSnap); | ||
| process.exit(2); |
There was a problem hiding this comment.
[Major] · Design/API
A protocol violation exits 2, which onClose renders as the same generic Failed to make bytecode ... for file ${snap} as an ordinary "this file just won't compile".
Why: downstream (lib/producer.ts:487-505), --fallback-to-source was designed for the latter. A desynced pipe would silently degrade every remaining file to plain source behind log.warn lines — producing a green build that ships source instead of bytecode, with no way for the caller to tell a corrupt channel from an uncompilable file. A framing bug that should never happen becomes invisible in CI.
Fix: make the protocol error distinguishable from a compile failure — a dedicated exit code or a typed error — so producer.ts can abort loudly instead of degrading quietly.
The same guard on the body header (line 23-26) has no test, unlike its snap-side twin.
| 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 ]); |
There was a problem hiding this comment.
[Minor] · Performance
Buffer.concat([stdin, data]) on every chunk re-copies the whole accumulated buffer, making frame reassembly O(n²) in total bytes. The parent's stdout = Buffer.concat([stdout, data]) has the same shape.
Why: a 1MB body in 64KB chunks copies ~8.7MB (~8.7x); 5MB copies ~202MB (~40x). This runs once per JS file across a multi-thousand-file build. Pre-existing — flagging it because the PR rewrote this exact loop and kept the pattern.
Fix: accumulate chunks in an array and concat once when a complete frame is available, or track a write offset into a pre-sized buffer. Reasonable as a follow-up rather than in this PR.
| stdin.copy(body, 0, startOfBody, startOfBody + sizeOfBody); | ||
|
|
||
| // Preserve unconsumed bytes for subsequent payloads | ||
| stdin = stdin.subarray(totalSize); |
There was a problem hiding this comment.
[Minor] · Performance
subarray returns a view over the same backing ArrayBuffer, so a few leftover bytes keep the entire concatenated allocation reachable until the next data event reassigns stdin.
Why: bounded, but the retention scales with frame size — worst case a 256MB backing store held alive by a handful of slack bytes. The correctness of the fix isn't affected (the next Buffer.concat reallocates); this is purely transient memory.
Fix: copy the remainder into a fresh right-sized buffer when the consumed prefix is large relative to what's left.
| export function buildFabricatorRequestChunks( | ||
| snap: string, | ||
| body: Buffer, | ||
| ): [Buffer, Buffer, Buffer, Buffer] { |
There was a problem hiding this comment.
[Nit] · Design/API
The [Buffer, Buffer, Buffer, Buffer] return type leaks the frame layout into the signature and pins callers to arity; Buffer[] (or a single concatenated Buffer) says the same thing. The only consumer immediately does for (const chunk of requestChunks).
Entirely optional.
| fabricatorScript, | ||
| } from '../../lib/fabricator'; | ||
|
|
||
| function parseBlobFrames(buffer: Buffer): Buffer[] { |
There was a problem hiding this comment.
[Major] · DRY / Codebase Fit
parseBlobFrames is a fourth hand-rolled implementation of this protocol — and it's written correctly: it loops over multiple frames and rejects negative sizes. The shipped parser it's validating against (lib/fabricator.ts:160-171) does neither.
Why: the regression test asserts the child's output against a parser that doesn't exist in production, so a regression in the real parent decoder cannot be caught here. The correct logic lives only in the test file. That gap is the clearest signal that the hardening should be applied in both directions.
Fix: extract the "accumulate → validate header against the shared max → slice frame → keep remainder" step into one function used by both the parent's onData and this test. The child script is the one place that must keep its own inline copy, since it has to be self-contained source text.
|
|
||
| const stderr = Buffer.concat(stderrChunks).toString(); | ||
| assert.equal(code, 2); | ||
| assert.match(stderr, /Invalid snap size header/); |
There was a problem hiding this comment.
[Minor] · Tests
Asserting on the exact stderr string couples the test to a log message, and a piped-stderr write immediately before process.exit(2) can truncate.
Why: I measured 0/100 losses on Linux for a message this short (and 40/40 truncation at 200KB), so the risk is low — but Node documents pipe writes as async on macOS and this repo's matrix includes macos-latest. A flaky assertion on a log string isn't worth the coverage it adds over line 107.
Fix: assert.equal(code, 2) already proves the guard fired and distinguishes it from every other exit path. Dropping the assert.match loses nothing.
| ); | ||
|
|
||
| const splitAt = 3; | ||
| child.stdin.write(Buffer.concat([frame1, frame2.subarray(0, splitAt)])); |
There was a problem hiding this comment.
[Minor] · Tests + Correctness
write(A) immediately followed by end(B) on a pipe is routinely coalesced into a single read on the child side, so the partial-header resume path (the break at lib/fabricator.ts:21/28) may never actually execute — only the multi-frame path is deterministic here.
Why: the test's stated purpose is cross-boundary splitting, but the split isn't guaranteed to survive to the child. It would still pass if the resume logic were broken.
Fix: await the first blob (or at least a tick) before writing the tail, so the two chunks are guaranteed to arrive as separate data events.
While here: splitAt = 3 only exercises one offset. I fuzzed all 119 and the implementation is correct — but a loop over a handful of offsets (including inside the size header) would lock that in.
| assert.ok(frames[1].length > 0); | ||
| }); | ||
|
|
||
| it('child script rejects invalid size headers', async () => { |
There was a problem hiding this comment.
[Minor] · Tests
Only the snap size header rejection is tested; the body size header guard (lib/fabricator.ts:23-26) has no coverage despite being part of the same fix.
Why: the two guards are symmetric but independent — the body one sits behind an extra break at line 21, so it's on a different path, and a regression there wouldn't be caught.
Fix: send a valid snap frame followed by a -1 body size and assert exit code 2. Also worth adding: zero-length snap and zero-length body, which the current cases don't touch.
No timeout wraps either of the child-process promises in this file — if a child hangs, the test hangs rather than failing.
2 participants
Summary
This PR fixes non-deterministic hangs in bytecode fabrication by hardening parent/child framing in the fabricator path and adding focused regression tests.
Problem
pkgcould hang during builds due to corrupted frame headers and stream-boundary truncation in fabricator IPC, with additional risk from debug stderr handling under backpressure.Root Cause
stdin.write()calls.Changes
fabricator.tsstderrpiped and routed debug output via logger.buildFabricatorRequestChunks(...)to construct isolated frame chunks with distinct headers.fabricatorScriptfor direct regression testing.fabricator.test.tsAdded targeted unit regressions:
Validation
npm run test:unit✅npm run lint✅npm run build✅Risk / Compatibility
Reviewer Checklist
fabricator.tsfabricator.test.tscover the reported failure modesmaintarget (yao-pkg/pkg)