Ignore ERR_STREAM_PREMATURE_CLOSE in CLI server#3304
Conversation
Node.js stream.pipeline throws ERR_STREAM_PREMATURE_CLOSE when the client disconnects before the response finishes streaming. This is normal during browser navigation — e.g. when following a redirect or clicking a link while a page is still loading. The error was introduced by the streaming response refactor in #3222 which replaced buffered responses with pipeline(Readable.fromWeb(), res). Catch and silently ignore this specific error code, re-throwing anything else.
There was a problem hiding this comment.
Pull request overview
This PR fixes noisy error logging caused by ERR_STREAM_PREMATURE_CLOSE exceptions when clients disconnect from the CLI server before response streams complete, which commonly occurs during navigation in wp-admin.
Changes:
- Added error handling to suppress
ERR_STREAM_PREMATURE_CLOSEerrors inhandleStreamedResponse - Added test coverage to verify client disconnects don't trigger error logs
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/playground/cli/src/start-server.ts | Wrapped pipeline() call in try-catch to filter out premature close errors |
| packages/playground/cli/tests/start-server.spec.ts | Added test that simulates client disconnect and verifies no errors are logged |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'code' in error && | ||
| error.code === 'ERR_STREAM_PREMATURE_CLOSE' |
There was a problem hiding this comment.
The type guard checking 'code' in error followed by accessing error.code is redundant. Since Node.js errors with a code property are typed as NodeJS.ErrnoException, you can streamline this by checking (error as NodeJS.ErrnoException).code === 'ERR_STREAM_PREMATURE_CLOSE' or using optional chaining with error.code === 'ERR_STREAM_PREMATURE_CLOSE' after the instanceof Error check.
| 'code' in error && | |
| error.code === 'ERR_STREAM_PREMATURE_CLOSE' | |
| (error as NodeJS.ErrnoException).code === 'ERR_STREAM_PREMATURE_CLOSE' |
There was a problem hiding this comment.
The in check is a runtime type narrowing that lets TypeScript know error.code exists without an unsafe cast. (error as NodeJS.ErrnoException).code skips the runtime check and trusts the cast. Keeping as-is.
| }), | ||
| new ReadableStream({ | ||
| start(controller) { | ||
| controller.enqueue(new TextEncoder().encode('hello')); |
There was a problem hiding this comment.
The stdout stream (lines 24-28) never closes its controller, which may cause the pipeline to hang indefinitely or behave unpredictably during cleanup. The test works because the client disconnects early, but this represents incomplete stream behavior. Call controller.close() after enqueueing data to properly terminate the stream.
| controller.enqueue(new TextEncoder().encode('hello')); | |
| controller.enqueue(new TextEncoder().encode('hello')); | |
| controller.close(); |
There was a problem hiding this comment.
Intentional — the test needs the stream to still be open when the client disconnects, otherwise pipeline completes normally and there is no premature close to test.
ERR_STREAM_PREMATURE_CLOSE in CLI server
brandonpayton
left a comment
There was a problem hiding this comment.
Thanks for fixing this, @JanJakes! This issue is really annoying and kind of embarrassing to have littered in our output.
I left one note. Please do what you want with that and feel free to merge.
|
Thanks, @brandonpayton!
This may have been lost somehow. I can't find the note, unfortunately. |
| }); | ||
|
|
||
| await new Promise((r) => setTimeout(r, 200)); | ||
| expect(logger.error).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
If error logging is broken completely, this test would also pass. Maybe it would be good to prove error logging is working at all so we can at least note the absence of the error in the context of working error logging.
Also trigger a real error through the same server to prove the logging pipeline is functional, so the prior not.toHaveBeenCalled() assertion is meaningful and not just a consequence of broken logging.
65bd9bd to
3d5f46b
Compare
… not logged The test was only checking that logger.error was not called, but didn't verify that the ERR_STREAM_PREMATURE_CLOSE error actually occurred. This means the test could pass vacuously if the error stopped being produced. Spy on pipeline from stream/promises and assert that it rejected with ERR_STREAM_PREMATURE_CLOSE before checking logger.error was not called.
|
I made another tweak to the test to more explicitly confirm the presence of the ignored error. It looks like we're good to go! |
Motivation for the change, related issues
The streaming response refactor in #3222 replaced buffered responses with
pipeline(Readable.fromWeb(stdout), res). When the browser disconnects before the response stream finishes — e.g. during a redirect or when the user clicks a link mid-load — Node.js throwsERR_STREAM_PREMATURE_CLOSE. This results in noisy error logs on every navigation in wp-admin:Implementation details
Wrap the
pipeline()call inhandleStreamedResponsewith a try/catch that silently ignoresERR_STREAM_PREMATURE_CLOSEand re-throws any other error.Testing Instructions (or ideally a Blueprint)
npx nx test-playground-cli playground-cli --testFile=start-server.spec.tsnpx nx dev playground-cli start --loginERR_STREAM_PREMATURE_CLOSEerrors should no longer appear in the terminal