fix(fabricator): harden frame protocol on both sides, fail loudly on desync - #294
fix(fabricator): harden frame protocol on both sides, fail loudly on desync#294chrhoffmann wants to merge 2 commits 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.23% 85.99% -1.25%
==========================================
Files 23 23
Lines 7929 8226 +297
Branches 1214 1242 +28
==========================================
+ Hits 6917 7074 +157
- Misses 1005 1144 +139
- Partials 7 8 +1
🚀 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.
| 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.
chrhoffmann
changed the title
Review follow-up for yao-pkg#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
Review follow-up for yao-pkg#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
bc2d1ab to
64a8921
Compare
robertsLando
left a comment
robertsLando
left a comment
There was a problem hiding this comment.
Deep review — 4 files, +777 / −65 LOC
Verdict: Needs work. Two blockers, both in the wiring around a parser that is itself solid.
Top 3 risks
- The pipelined fast path answers a request that was never sent, delivering another file's bytecode — the exact silent corruption this PR exists to stop (
lib/fabricator.ts:398). - The 60s timeout is tagged as a protocol error, so a slow-but-healthy compile now hard-fails with
--fallback-to-sourceignored (lib/fabricator.ts:277). - The stated root cause —
--options trace-gcwriting to the child's stdout — is documented but not removed. The frame channel still shares stdout, so that flag turns from a hang into an unbuildable configuration.
Strengths. The corrected, honestly-labelled problem statement in the PR description is genuinely rare and made this review much faster. Tests assert against the production parser rather than a copy. 11 of the 13 threads from the previous round are verifiably addressed. The frame layout and exit-code contract are now written down in docs/ARCHITECTURE.md.
Themes
- Guards on a channel that's still polluted. The PR identifies stdout-sharing as the root cause, then hardens the reader instead of moving the frames off stdout. Everything in the
trace-gcfamily below follows from that one decision. - New fatal class, old plumbing.
FABRICATOR_PROTOCOLis now the only build-aborting error infabricator.ts, but it doesn't go throughwasReported(), doesn't appear in the user-facing docs, and doesn't clean up the partially-written output file. - The parser is tested; the state machine around it isn't. Both blockers live in code the suite never drives, which is why they survived a green run.
Two findings that fall outside the diff
[Major · Design] lib/fabricator.ts — the bakes filter is the actual root cause, and it's untouched.
fabricate() strips only --prof / --v8-options / --trace-opt / --trace-deopt. But --options trace-gc writes to the child's stdout (measured: ~15KB stdout, 0 stderr), as do trace-gc-verbose, print-bytecode, trace-ic and trace-turbo. This PR converts that from an infinite hang into a hard, unescapable abort — a supported user flag becomes an unbuildable configuration with no override. The durable fix is to stop sharing stdout between V8 traces and frames: move the frame channel to a dedicated fd (stdio[3]). Widening the filter is the cheaper stopgap.
[Minor · Docs] docs-site/ still promises the old contract.
guide/bytecode.md:59, guide/targets.md:99 and guide/getting-started.md:127 all state that --fallback-to-source ships source "when bytecode generation fails". The new always-fatal exception is documented only in the contributor-facing docs/ARCHITECTURE.md. Users hitting the new abort will read the guide and conclude pkg is broken.
Coverage
Specialists run: Correctness, DRY (repo-wide), Performance, Design/API/BackCompat, Tests, Operability, Readability. Every lane returned findings; none came back clean.
Not run: Security — no changed file matched its lane. The child's spawn arguments are build-time flags rather than untrusted input, and the -e script is a module constant with only numeric constants interpolated.
Reviewed against a dedicated worktree at 64a8921, so repo-wide reuse checks and sibling-file context were complete.
| h.writeInt32LE(b.length, 0); | ||
| child.stdin.write(h); | ||
| child.stdin.write(b); | ||
| // A previous call may have left a pipelined response buffered; deliver it |
There was a problem hiding this comment.
[Blocker] · Correctness
The "pipelined response" fast path returns a leftover buffered frame as this request's answer and never writes the request.
Why: A non-empty state.stdoutBuf can only exist when the child sent more than one frame for one request — i.e. the channel is already desynced. Delivering it hands the caller another file's bytecode, and because no request was sent, the channel stays permanently one frame behind: every subsequent file gets the previous file's blob. That is exactly the silent corruption this PR exists to stop. The >= 4 guard also doesn't cover a 1–3 byte leftover, or a stale header whose payload never arrived — those get the real response concatenated onto them and misframe into a plausible-looking blob.
Fix: Treat any non-empty stdoutBuf at request time as a protocol error, not as a deliverable.
| return; | ||
| } | ||
|
|
||
| settled = true; |
There was a problem hiding this comment.
[Blocker] · Design / Correctness
The 60s response timeout is constructed via fabricatorProtocolError(), so fabricateTwice won't retry and producer.ts aborts the build with --fallback-to-source bypassed.
Why: A stalled child is not proof of desync. QEMU-emulated cross-compile is a documented supported path (docs-site/guide/bytecode.md:29), and large bundles on loaded CI can legitimately exceed 60s for a single file. Those builds completed before this PR and now hard-fail with no override.
Fix: Reserve FABRICATOR_PROTOCOL for header corruption and exit-3. Give the timeout its own code that keeps the fallback/skip path, and make the duration configurable.
| tryDeliver(); | ||
| } | ||
|
|
||
| function removeListeners() { |
There was a problem hiding this comment.
[Major] · Correctness
removeListener('data', onData) does not pause a flowing stream — chunks emitted between calls are silently dropped.
Why: The remainder carry-over premise this PR builds on is unreachable, and garbage the child writes between requests is discarded instead of detected. The desync then surfaces as a 60s stall rather than as a protocol error.
Fix: Use one persistent per-child data handler that owns stdoutBuf and dispatches to the pending request, or pause() the stream on settle.
| // 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; |
There was a problem hiding this comment.
[Major] · Design / API
FABRICATOR_PROTOCOL_EXIT_CODE = 3 collides with Node's reserved exit code 3 (Internal JavaScript Parse Error).
Why: The child is a Node binary, often of a different version. A bootstrap or -e parse failure would be misread as a framing violation and unconditionally abort the build, with --fallback-to-source bypassed.
Fix: Pick a code outside Node's reserved 1–14 range.
| ); | ||
| } | ||
|
|
||
| export function fabricatorProtocolError(message: string): Error { |
There was a problem hiding this comment.
[Major] · Design / DRY
fabricatorProtocolError returns a bare Error; every other fatal in lib/ goes through wasReported() (index.ts:271, walker.ts:223, packer.ts:113, config.ts:324, hooks.ts:29).
Why: This is now the only build-aborting error class in fabricator.ts, and it reaches bin.ts:17 unreported — so the user gets a raw stack trace containing the FABRICATOR_PROTOCOL: token plus up to 2000 chars of child stderr, with no hints. That is the opposite of the readable failure this PR is chasing. Flagged independently by both the design and DRY passes.
Fix: Route the fatal through wasReported with hint lines; keep .code purely as the internal discriminator for isFabricatorProtocolError.
| return; | ||
| } | ||
|
|
||
| let requestChunks: Buffer[]; |
There was a problem hiding this comment.
[Minor] · Operability
This catch path omits tailSuffix() and skips kill(), unlike every other failure path in fabricate().
Why: Harmless today (no request was written), but it is the one error path with a different shape, which is how inconsistencies become bugs later.
Fix: Match the surrounding error-handling shape.
| length-prefixed frames over stdio: | ||
|
|
||
| ``` | ||
| Request (parent → child): [u32 snapLen][snap bytes][u32 bodyLen][body bytes] |
There was a problem hiding this comment.
[Minor] · Design / Docs
Three inaccuracies in the new protocol section: "cannot reject a payload that previously worked" is false, the frame fields are documented as u32, and the exit-code table is incomplete.
Why: MAX_STRING_LENGTH is 536,870,888, so bodies between 256MB and 512MB did compile before this ceiling. Both sides read the header with readInt32LE (signed), not unsigned — which is precisely why the negative-size guard exists. And the timeout also produces FABRICATOR_PROTOCOL, which the table doesn't mention.
Fix: Correct the ceiling rationale, document the fields as i32, and add the timeout row.
|
|
||
| // 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; |
There was a problem hiding this comment.
[Nit] · DRY
FABRICATOR_STDERR_TAIL_MAX_BYTES is exported but has no consumer in lib/, test/, or docs.
Why: The repo exports constants only where they cross module boundaries (common.ts STORE_*, types.ts NODE_OSES).
Fix: Make it module-private.
| return { status: 'ok', frame, remainder }; | ||
| } | ||
|
|
||
| export function checkFabricatorFramePartSize( |
There was a problem hiding this comment.
[Nit] · DRY
checkFabricatorFramePartSize/isDeterministicFabricatorError and fabricatorProtocolError/isFabricatorProtocolError are two hand-rolled copies of one tag-then-predicate shape.
Why: Every new failure class will add a third pair.
Fix: One tagged-error factory plus one predicate parameterised by code.
| }); | ||
| if (!s.cachedDataProduced) { | ||
| console.error('Pkg: Cached data not produced.'); | ||
| process.exit(2); |
There was a problem hiding this comment.
[Nit] · Readability
process.exit(2) in the child carries no comment, and 65536 at line 60 is an unexplained threshold.
Why: The parent explains the 2-vs-3 exit-code split at lines 12–15; the child, which is where both codes are actually produced, does not. A reader in the -e string has to trace back out to understand the contract.
Fix: One short line each on the why.
2 participants
Summary
This PR reworks the fabricator frame protocol so that both sides of the
parent/child pipe are hardened, protocol failures are loud and
distinguishable from ordinary compile failures, and a hung fabricator can
no longer stall a build forever. Includes targeted regression tests and an
architecture-doc section for the protocol.
Rewritten after the review on this PR. The original description claimed
three reproducible stdio deadlocks; none of them reproduced as such (see
"Corrected problem statement"). The one reachable hang was in the parent
decoder, which the first iteration didn't touch — this iteration fixes it.
Corrected problem statement
Reachable (fixed here):
bakescome from--options, andfabricate()'s filter only strips--prof/--v8-options/--trace-opt/
--trace-deopt. Sopkg --options trace-gcpassestrace-gcto thefabricator child, which writes GC traces to stdout — straight into the
framed response stream. The old parent decoder read the first 4 bytes as a
size (
Buffer.from('[123:0x5').readInt32LE(0)→858927451), sostdout.length >= 4 + sizeOfBlobnever became true, the callback neverfired, and pkg hung forever (no timeout existed on this path). A header
byte with the high bit set was worse: a negative size made
Buffer.alloc(-1)throwERR_OUT_OF_RANGEinside a'data'handler,crashing pkg instead of routing through the error path.
Not reproducible as deadlocks (kept as hardening, honestly labeled):
Writable.write()retains buffers by reference under backpressure), butunreachable at this call site. Fixed as latent-hazard hygiene.
fabricate()has exactly onecaller and requests are serialized, so trailing bytes can't occur on the
child's stdin today. Multi-frame parsing retained as defensive
correctness.
than creating an unread pipe. The stderr change below is a diagnosability
improvement only.
Changes
lib/fabricator.tstryParseFabricatorResponse()(accumulate → validate header against the shared max → slice one frame →
keep remainder) used by the parent decoder and the unit tests. The
child script keeps the only other copy, since it must remain
self-contained
-esource text.child and surface a tagged
FABRICATOR_PROTOCOLerror instead of hangingor throwing inside a
datahandler. Buffered remainders are carriedacross calls so nothing is dropped.
3(
FABRICATOR_PROTOCOL_EXIT_CODE); exit2still means "well-formedrequest V8 refused to compile". The parent maps exit 3 to a typed
FABRICATOR_PROTOCOLerror.FABRICATOR_RESPONSE_TIMEOUT_MS(60s) per request — a childthat accepts a frame and then hangs now produces a loud error with
context, not an eternal stall.
to every failure message, so
Pkg: Cached data not produced.reachesnon-debug users; unexpected-close output is embedded as a printable-safe
snippet instead of a raw binary
console.log(or an invisibledebug-only line); stderr decoding only runs when
log.debugModeis on.fabricateTwice: no retry for deterministic failures(
FABRICATOR_FRAME_TOO_LARGE) or protocol errors; the first attempt'serror is logged before any legitimate retry.
few slack bytes; request-chunk builder returns
Buffer[].per-file source buffers and
module.wrapalready caps at Node's 512MBstring limit, so it cannot reject a payload that previously worked.
lib/producer.tsA
FABRICATOR_PROTOCOLerror now aborts the build.--fallback-to-sourcewas designed for "this file won't compile"; a desynced pipe would
previously silently degrade every remaining file to plain source behind
log.warnlines — a green build shipping no bytecode. That can't happenanymore.
docs/ARCHITECTURE.mdDocuments the frame layout
(
[u32 snapLen][snap][u32 bodyLen][body]→[u32 blobLen][cachedData]),the 256MB ceiling rationale, and the exit-code contract.
Tests (
test/unit/fabricator.test.ts)the size header), staged writes so chunk boundaries survive to the child.
fabricate()withbinaryPath = process.execPath, coveringboth halves.
test instead of hanging CI.
before
exitcan truncate on macOS); the exit code alone proves theguard fired.
Validation
yarn build✅yarn lint✅yarn test:unit✅ (271 pass, repeated runs for flake)and a 3MB body — zero failures.
Review response
degrading to
--fallback-to-source