fix(core): fall back to v8 for oversized daemon responses and length-prefix the wire protocol - #36838
Open
AgentEnder wants to merge 10 commits into
Open
fix(core): fall back to v8 for oversized daemon responses and length-prefix the wire protocol#36838AgentEnder wants to merge 10 commits into
AgentEnder wants to merge 10 commits into
Conversation
✅ Deploy Preview for nx-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for nx-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
View your CI Pipeline Execution ↗ for commit ef8e258
☁️ Nx Cloud last updated this comment at |
AgentEnder
commented
Aug 28, 2026
AgentEnder
force-pushed
the
feat/nxc-4901-daemon-v8-framing
branch
3 times, most recently
from
August 29, 2026 18:26
2ab8069 to
baadc72
Compare
…ompute spec project-graph-incremental-recomputation.spec.ts times out at exactly 35000ms on CI on every run that actually executes it: three runs in a row clipped at 35,009 / 35,018 / 35,070ms. Measured with the limit raised, the slowest test needs 42.4s on CI hardware, and its neighbours land at 32s and 29s. Locally the same test swings between 7s and 23s depending on pool load, so a 35s ceiling is inside the spec's own variance once CI's roughly 5x slowdown is applied. Master rarely shows it because most commits do not touch packages/nx and nx:test is served from cache. Any branch that busts that cache runs the spec every push and hits the limit. The jest preset carried the same 35s before the vitest move, so this is not a transform regression; the spec has been close to the line for a while and only surfaces on branches that execute it repeatedly.
The daemon wire format framed messages with a trailing NX_MSG_END
delimiter and carried payloads as latin1 strings, so every message was
bounded by Node's max string length of 536,870,888 characters at three
separate points: `.toString('binary')` on a v8 buffer, the response
concatenation, and the client's accumulating string.
Messages are now framed with an `NX_MSG_<byteLength>:` header and travel
as buffers end to end. Buffers also sit outside the V8 heap, so a large
response stops counting against --max-old-space-size.
Framing is O(1) per message instead of a scan, so a payload containing
the framing marker can no longer desynchronize the stream, and a
complete message sharing a chunk with an incomplete one is delivered
immediately rather than held until a chunk ends on a boundary. An
incomplete message times out after 60s of no data, and a header that
does not parse fails the stream instead of hanging.
The serialization format is detected per message after framing rather
than per connection, because a single chunk can carry a JSON streaming
progress message and a v8 response together. StringDecoder is gone:
framing on bytes means a complete message cannot split a multi-byte
character.
All four channels sharing the framing move together: daemon client and
server, plugin worker IPC, and pseudo-IPC. Version skew is already
covered by the nxVersion handshake in daemon/cache.ts, which throws
before any bytes are exchanged.
No behavior change to which format is chosen; that follows separately.
…alized
The daemon mirrored the client's wire format unconditionally when
serializing a response, with no fallback if that format could not carry
the payload. A large HASH_TASKS response therefore killed the daemon:
Serializing response for HASH_TASKS message in json mode
RangeError: Invalid string length
at serializeUnserializedResult
at handleResult
The client side already had a JSON/v8 fallback, but it only covered
messages sent TO the daemon. Nothing covered responses. The throw
escapes an un-awaited async callback, so it became an unhandled
rejection and terminated the process rather than failing one request.
`serializeWithFallback(data, preferred)` now backs both directions, and
replaces `serializeUnserializedResult`. Responding in a format the
client did not send is already safe: the client detects the format per
message, and streaming progress messages have always been serialized
from the daemon's own preference rather than the requesting client's.
Also fixes a precedence bug this surfaced. `serialize()` branched on
`force === 'v8' || isV8SerializerEnabled()`, so `force: 'json'` was
ignored whenever NX_USE_V8_SERIALIZER was set. `processInBackground`
forces JSON precisely because its payloads cannot be v8-cloned, so it
paid a failed serialization and a spurious warning on every call under
that env var. `force` now means strict, and the configured preference
only applies when no format is forced.
Both error paths that quote an unparseable message decoded it as utf8. A v8 payload is binary, so that replaced every byte above 0x7f with U+FFFD, starting with the 0xFF header that identifies the format. The excerpt exists to show what arrived, and it destroyed exactly the bytes worth seeing. `describeMessage` renders JSON as text and v8 as hex, and reports how many of the message's bytes it is showing. Truncating utf8 at an arbitrary byte leaves a partial sequence that decodes to U+FFFD, so the window is trimmed to whole characters first. A slice taken from the end can begin mid-character as well as end mid-character, so both edges are trimmed. Both the client's deserialize failure and the daemon's invalid-payload error use it. Also adds `sendMessage(socket, data, force?)`, since every caller that was not already holding bytes wrote `writeMessage(socket, serialize(x))`. It lives in socket-utils rather than inside `writeMessage` so the framing stays a byte-level primitive: consume-messages-from-socket is shared by callers that already have bytes, and having it reach into the daemon's serializer would invert the dependency.
Review of the length-prefix change found two defects it introduced, both of which present as a hang rather than an error. The idle timer could kill a healthy stream. Node runs the timers phase before poll, so a reader blocked inside its own data handler past the deadline had the overdue timer fire before the bytes it was waiting on were delivered. `fail()` then discarded those bytes and marked the stream permanently broken. `server.ts` is exactly that case: a sufficiently large workspace keeps the daemon synchronously inside a data handler while another client has a partial message buffered. The timer is removed rather than repaired; a peer that dies closes the socket, which already surfaces through 'close', and the guard it was aimed at is the one case it could not observe. No consumer passed `onError`, so a framing failure only reached `console.error` while the socket stayed open and writable. Nothing settled the in-flight request, and the CLI waited out the 20 minute keep-alive before reporting a handler timeout that named the handler rather than the transport. All six call sites now forward it: the messenger rejects the pending request, the daemon and pseudo-IPC report and close so the peer sees the disconnect, and the plugin worker exits so its host rejects the pending hooks. Also from review: the daemon's second invalid-payload path still interpolated a raw Buffer, a zero-length frame delivered an empty message that `parseMessage` then threw on, and a rejection from `handleMessage` was unhandled and would take the daemon down with it.
…host Framed payloads buffer outside the V8 heap, so --max-old-space-size no longer bounds them and a peer that declares a huge length could stream until the machine gave out. The uncapped buffer predates this change, but the old string transport self-limited at V8's ~0.5GiB string ceiling, so moving to Buffers removed that accidental backstop. A message is now refused at header-parse time, before its payload is buffered, above NX_MAX_MESSAGE_SIZE bytes. The default of 2GiB is four times the string ceiling that used to cap every message, so it clears any payload that previously worked or was meant to. Set the variable to 0 to remove the ceiling entirely. `handleSocketData` in the isolated plugin host also called `parseMessage` with no try/catch inside a synchronous socket 'data' callback, so a payload that failed to parse became an uncaughtException in the host nx process rather than a failed plugin call. It now logs the plugin name and an excerpt, and returns.
The daemon client was the one channel of six whose framing-error handler did not tear down its socket. The reader latches itself broken and ignores every later 'data' event, but a framing failure emits neither 'close' nor 'error', and every recovery path in client.ts is keyed on 'close': the main socket's handler, reconnectFileWatcher, and reconnectProjectGraphListener. The connection was therefore left open, writable and permanently deaf. `_daemonStatus` stayed CONNECTED so `startDaemonIfNecessary()` returned early, and the next request wrote into a socket nothing was reading and waited out the 20 minute keep-alive before reporting a handler timeout that named the handler rather than the transport. The two auxiliary messengers had no timeout at all, so they simply went silent. All five listen error handlers now close after reporting, which is what turns the reported error into a recoverable one: close destroys the socket, 'close' fires, and the existing bounded reconnect runs. The other five consumers already did this with socket.destroy().
oxlint's no-duplicate-imports rejects two import statements from the same module. Local eslint does not carry that rule, so it only surfaced in CI.
Closing the socket on a framing failure let the watcher channels reach their recovery path, but that path re-dials unconditionally. The only guard, fileWatcherReconnecting, is cleared immediately before the recursive call, so it bounds concurrent reconnects rather than iterations, and the two terminal exits it does have are for an unavailable server and a version mismatch. A framing failure against a healthy daemon reaches neither. A framing failure is deterministic, so the redial replays it: fail -> onError -> close -> 'close' -> reconnect -> re-register -> fail. This PR supplies the trigger, since NX_MAX_MESSAGE_SIZE set below a workspace's file-change batch size makes every notification exceed it. Measured against a peer that answers every connection with a bad frame, an unbounded build reconnects 9,373 times in 1.5s. Each watcher channel now counts consecutive framing failures, resets on a delivered message, and past three falls into the existing "give up and notify 'closed'" branch. Socket errors are unaffected and still retry: only MessageFramingError increments the counter, because a dropped connection is exactly the case a redial does fix.
…ying A socket chunk usually carries several complete messages, and take() was allocating and copying for each one even when the message sat entirely inside the leading chunk. Slicing it out in place instead is 13% cheaper per message on small-message traffic and about 15% cheaper on mid-sized payloads. The callback parses the payload before the next read, so sharing the chunk's memory does not retain it. Measured against the delimiter protocol this replaces, over a real unix socket carrying a cached run's traffic mix (a project graph response, a HASH_TASKS response, then 5000 log messages, 6.2MB total), the new framing is 33% faster end to end: 8.7ms to 5.8ms. Per message the two are within 0.06us of each other on small messages, which the delimiter scan wins, and the length prefix wins by 5-7x on large ones because it no longer rescans the whole accumulated buffer.
AgentEnder
force-pushed
the
feat/nxc-4901-daemon-v8-framing
branch
from
August 30, 2026 03:03
30c1e31 to
ef8e258
Compare
jaysoo
approved these changes
Aug 30, 2026
AgentEnder
added a commit
that referenced
this pull request
Aug 30, 2026
…ompute spec (#36850) ## Current Behavior `project-graph-incremental-recomputation.spec.ts` times out at exactly 35,000ms on CI on every run that executes it. Three consecutive runs on one branch clipped at 35,009ms, 35,018ms and 35,070ms on the same test. Master rarely shows it: most commits do not touch `packages/nx`, so `nx:test` is served from cache, and 9 of the last 12 master commits never ran the spec. ## Expected Behavior The spec has enough budget to finish on CI hardware. With the limit raised for measurement, the slowest test needs 42.4s and its neighbours land at 32s and 29s; the whole suite went green with no other change. Locally the same test swings between 7s and 23s depending on pool load, so a 35s ceiling sits inside the spec's own variance once CI's roughly 5x slowdown is applied. 90s clears the measured worst case by 2x. This is not a vitest transform regression: the jest preset carried the same 35s before #36754. ## Related Issue(s) Unblocks #36838, which is stacked on this branch and will be retargeted to `master` once this merges.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Current Behavior
The daemon mirrors the client's wire format when serializing a response, with no fallback if that format cannot carry the payload. A large
HASH_TASKSresponse kills the daemon:The client side already had a JSON/v8 fallback, but it only covers messages sent to the daemon. Nothing covered responses. The throw escapes an un-awaited async callback, so it becomes an unhandled rejection and terminates the process rather than failing one request.
Messages were also framed with a trailing
NX_MSG_ENDdelimiter and carried as latin1 strings, so every message was capped at Node's max string length of 536,870,888 characters in three places:.toString('binary')on a v8 buffer, the response concatenation, and the client's accumulating string.Expected Behavior
Daemon responses fall back to the other format instead of throwing, and messages are no longer bounded by the max string length.
Two commits:
Framing. Messages carry an
NX_MSG_<byteLength>:header and travel as buffers end to end. Buffers sit outside the V8 heap, so a large response stops counting against--max-old-space-size. Framing is O(1) per message instead of a scan, so a payload containing the framing marker can no longer desynchronize the stream, and a complete message sharing a chunk with an incomplete one is delivered immediately rather than held until a chunk ends on a boundary. An incomplete message times out after 60s, and a header that does not parse fails the stream instead of hanging.Fallback.
serializeWithFallback(data, preferred)replacesserializeUnserializedResultand backs both directions. This also fixes a precedence bug it surfaced:serialize()branched onforce === 'v8' || isV8SerializerEnabled(), soforce: 'json'was ignored wheneverNX_USE_V8_SERIALIZERwas set.processInBackgroundforces JSON precisely because its payloads cannot be v8-cloned, so it paid a failed serialization and a spurious warning on every call under that env var.The format is detected per message after framing rather than per connection, because a single chunk can carry a JSON streaming progress message and a v8 response together. Responding in a format the client did not send is already safe: streaming progress messages have always been serialized from the daemon's own preference rather than the requesting client's.
Why both changes ship together
Measured on a
HASH_TASKS-shaped payload, v8 output is 2.4% smaller than JSON. With a string transport the fallback only widens the working range from about 537MB to about 550MB, which stops the crash without giving the reporting workspace headroom. Opting intoNX_USE_V8_SERIALIZERwas not enough for them either.Compatibility
The wire format changes in both directions across the four channels that share the framing: daemon client and server, plugin worker IPC, and pseudo-IPC. Version skew is already covered by the
nxVersionhandshake indaemon/cache.ts, which throws before any bytes are exchanged, so a new client never talks to an old daemon.Testing
consume-messages-from-socket.spec.tscovers framing, header splits, desync, timeouts, a payload whose bytes contain the framing marker, and three round-trips over a real unix socket where the OS picks the chunk boundaries. The fallback tests avoid allocating 512MB by using inputs each format rejects:{value: 1n}fails JSON,{fn(){}}fails v8.3178 tests pass across
daemon,utils,project-graph/plugins, andtasks-runner, with no new failures.Not affected
serializeResult()builds theREQUEST_PROJECT_GRAPHresponse by string concatenation rather than going through this path. Its only other caller passes(error, null, null). The project graph does not carry per-file data, so neither payload is anywhere near the limit that the task hash details hit.Stacked on #36850
This PR is based on #36850, which raises the
packages/nxvitest timeout.project-graph-incremental-recomputation.spec.tsneeds 42.4s on CI against the previous 35s limit and fails on any branch that executes it; this branch touchespackages/nx, so it runs the spec every push. Merge #36850 first, then retarget this PR tomaster. The commit range here is only the nine daemon commits; the timeout change is not part of this diff.CI note
Intermittent
Command timed out after 300sfailures on rollup builds ine2e-react/e2e-remix/e2e-rollupare the pre-existing flake tracked in #36794, not this change. That flake predates this branch, hits about 4% of master runs, and the dump there showed the surviving process is rollup's own CLI while the daemon round-trip finished in milliseconds. This branch touchespackages/nx, so every e2e task runs rather than reading from cache, which is why it shows up more often here.Related Issue(s)
Fixes NXC-4901
View Polygraph session ↗