fix: make ndJsonStream receive path linear in message size#210
Merged
Conversation
The receive loop accumulated undelivered bytes in a string and re-split the entire buffer on every incoming chunk, so a message of size M arriving in chunks of size c cost O(M²/c) in string scanning and copying, stalling the event loop for large messages. Scan only the newly arrived chunk for newlines, keep incomplete tails as byte segments, and decode/parse once per complete line. Receiving an M-byte message now costs O(M) regardless of chunk count (10 MB message in 64 KiB chunks: ~152 ms -> ~7 ms). Fixes #206 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…DJSON messages Address code-review findings on the receive-path fix: - Extract the byte-level incremental line splitter into a shared internal LineBuffer class and copy carried-over tails (slice instead of subarray) so a few pending bytes don't pin an entire chunk's buffer while a message is incomplete. - Rewrite parseSseStream on top of LineBuffer, removing the same O(message_size²/chunk_size) accumulate-and-resplit pattern from the HTTP/SSE receive path (10 MB event in 64 KiB chunks: ~150 ms -> ~7 ms). - Validate NDJSON lines with isJsonRpcMessage before enqueueing, matching every other transport; previously a valid-JSON primitive line like 42 threw in Connection.receiveMessage and tore down the connection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-up review fixes: - Guard NDJSON lines with isRecord instead of isJsonRpcMessage: strict validation dropped near-valid messages that the connection layer previously handled (responses with error:null alongside result, or requests missing the jsonrpc field), turning fast failures into permanently hanging requests. Only non-object lines — which throw a TypeError in receiveMessage — are skipped. - Copy carried-over tails with the Uint8Array constructor: Node's Buffer overrides slice() to return a view, so the previous slice-based copy silently kept pinning the chunk's buffer for Buffer chunks. - Make LineBuffer.push return an eager array instead of a lazy generator so partial consumption can't drop the buffered tail, and document that pushed chunks must not be mutated. - Consolidate duplicated test fixtures into src/test-support/streams.ts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Third-round review fixes:
- Guard Connection.receiveMessage with isRecord so non-object messages
from any transport (including third-party Stream implementations) are
logged and dropped instead of throwing on the in operator and tearing
down the connection.
- Validate the error member in handleResponse before destructuring: a
response like {"jsonrpc":"2.0","id":1,"error":null} previously threw
a TypeError that closed the whole connection; it now fast-rejects just
that request with RequestError.invalidRequest.
- Restore LineBuffer.push's single-condition tail handling (clearer than
the two-branch enumeration, same behavior).
- Harden test helpers: collectStream releases its reader on error,
chunkBytes rejects non-positive sizes and drops a no-op clamp and an
unused input type.
- Pass strings directly to streamFromChunks in stream.test.ts instead of
pre-encoding them.
The WS/SSE strict-validation divergence is tracked in #211.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
benbrandt
added a commit
that referenced
this pull request
Jul 6, 2026
The WebSocket and SSE receive paths validated messages with the strict isJsonRpcMessage guard and silently dropped anything that failed it, so a lenient peer's response (e.g. error:null alongside result) or request (e.g. missing the jsonrpc field) never settled the caller's promise — the same shapes worked over stdio, which only skips non-object lines. Relax the per-transport guards (ws-stream, ws-server, sse, HTTP server POST) to an object check and let the connection layer own the accept/reject policy: it dispatches lenient requests, fast-rejects structurally invalid responses with RequestError.invalidRequest, and logs anything else, as of #210. Non-object payloads are still rejected at the transport with a warning (or HTTP 400) since they carry no id to answer. Also consolidates ws-stream's private isRecord duplicate. Fixes #211 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
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.
Fixes #206
Problem
The receive side of
ndJsonStreamaccumulated undelivered bytes in a string and re-split the entire buffer on every incoming chunk:While a large message is still incomplete, each chunk copies and re-scans everything buffered so far, so a message of size M arriving in chunks of size c costs O(M²/c) — blocking the event loop for tens to hundreds of milliseconds on multi-megabyte messages (big tool results, embedded resources, base64 payloads).
parseSseStreamin the HTTP/SSE transport had the identical pattern (buffer += decode+ full-buffer regex split per chunk).Fix
New internal
LineBufferclass: scans only the newly arrived chunk for0x0A, keeps incomplete tails as byte segments (copied viasliceso a few carried-over bytes don't pin an entire chunk's buffer), and concatenates once per complete line. Receiving an M-byte message costs O(M) regardless of how many chunks it spans.ndJsonStreamandparseSseStreamare both rewritten on top of it, so the stdio and HTTP/SSE receive paths are fixed together and the framing logic is unit-tested in isolation.ndJsonStreamnow skips non-object JSON lines before enqueueing. Previously a valid-JSON primitive line like42from a peer threw aTypeErrorinConnection.receiveMessageand tore down the whole connection; now it's skipped with a warning. Object-shaped messages are passed through unvalidated exactly as before, so lenient peers (e.g. responses carryingerror: nullalongsideresult, or requests omitting thejsonrpcfield) keep working.The connection layer is hardened against the same class of malformed input from any transport:
Connection.receiveMessagedrops non-object messages instead of throwing on theinoperator, andhandleResponsefast-rejects a response whoseerrormember is not an object (e.g.error: null, which previously threw while destructuring and closed the whole connection) withRequestError.invalidRequest. The WS/SSE transports' stricter silent-drop validation is tracked separately in Unify JSON-RPC message validation policy across transports #211.Multi-byte UTF-8 characters split across chunk boundaries remain safe: lines are only decoded once complete, and
0x0Acan never appear inside a multi-byte UTF-8 sequence.The send side is unchanged — holding the writer lock across messages would be an observable behavior change (it would permanently lock
output), and the per-messagegetWriter()churn there is O(1), not quadratic.Benchmark
Same methodology as the issue (one message fed in 64 KiB chunks, median of 7 runs, Node v24, Apple Silicon):
Before grows ~quadratically; after grows linearly on both transports.
Tests
Added:
LineBufferunit tests (chunk boundaries, empty lines, flush), a >100-chunk large-message test and non-object-line and lenient-message pass-through tests forndJsonStream, and large-event / CRLF-boundary / unterminated-final-event tests forparseSseStream. Fullnpm run check(lint, format, spellcheck, build, 624 tests, typedoc) passes.🤖 Generated with Claude Code