`CopilotClient` stdin error handler re-throws inside an EventEmitter callback, causing `uncaughtException`

[sergiou87]

In nodejs/src/client.ts, connectToChildProcessViaStdio() attaches an 'error' listener to the spawned CLI's stdin that re-throws the error whenever the client is not in the middle of forceStop():

copilot-sdk/nodejs/src/client.ts

Lines 1801 to 1806 in c69ea43

	// Add error handler to stdin to prevent unhandled rejections during forceStop
	this.cliProcess.stdin?.on("error", (err) => {
	if (!this.forceStopping) {
	throw err;
	}
	});

Throwing synchronously inside a Node.js EventEmitter callback does not reject any caller's promise and does not route through any existing error handling — it surfaces as an uncaughtException on the Node process, which by default terminates the host application. This defeats the listener's stated purpose ("prevent unhandled rejections during forceStop"): without the listener, an emitted 'error' with no handler crashes the process; with this listener, an emitted 'error' outside forceStop also crashes the process, just one tick later.

This affects any SDK consumer using RuntimeConnection.forStdio(...) whenever the CLI child process dies for any reason other than a user-initiated forceStop() — e.g. the CLI crashing, being SIGKILL'd by the OS, or closing its stdin — because the next write attempt by StreamMessageWriter will trigger EPIPE on the pipe and emit 'error'.

Reproduction

1. Create a CopilotClient configured with RuntimeConnection.forStdio(...).
2. After connect() succeeds, send SIGKILL to the CLI child process (or otherwise close its stdin) without calling forceStop() on the client.
3. Trigger any RPC that causes a write to the now-broken stdin pipe.

Expected: the SDK surfaces the failure through its normal error path — pending RPCs reject, state transitions to "disconnected", and the host application stays alive so the consumer can decide what to do.

Actual: Node emits 'error' on stdin, the listener re-throws, and the host application terminates with an uncaughtException.

Why the current implementation doesn't achieve its goal

The comment says the listener exists "to prevent unhandled rejections during forceStop." Two issues:

1. It re-introduces the very crash it's trying to suppress. Node's default for an 'error' event with no listener is to throw uncaughtException. Re-throwing for the non-forceStop branch produces the same outcome with extra steps.
2. The throw doesn't reach any caller. It happens synchronously inside an EventEmitter callback, so it doesn't reject any in-flight sendRequest promise, doesn't reach attachConnectionHandlers's connection.onError, and doesn't transition state to "error".

The information is also redundant: a broken stdin pipe almost always implies the child process exited, and the existing cliProcess.on("exit", ...) handler in startCLIServer already populates processExitPromise, which verifyProtocolVersion (and any other call site using raceAgainstExit) uses to fail in-flight requests with a meaningful message including the CLI's stderr.

Suggested fixes

Two viable options, in increasing order of behavior change:

Option 1 — Swallow the error (minimal change)

A stdin 'error' is virtually always a side effect of the CLI process dying, and that path is already covered: cliProcess.on("exit", ...) populates processExitPromise (which rejects in-flight RPCs that use raceAgainstExit), and MessageConnection's own writer-error detection fires onError / onClose (which sets state = "disconnected").

The only reason an 'error' listener needs to be present at all is so Node doesn't promote the event to uncaughtException. So the smallest correct change is to keep the listener but make it a no-op:

this.cliProcess.stdin?.on("error", () => {
    // EPIPE on stdin means the CLI exited; the exit handler populates
    // processExitPromise and the MessageConnection's own writer-error
    // detection sets state = "disconnected".
});

Option 2 — Route through the existing connection-teardown path

If we want the stdin error to actively drive teardown (so that RPCs not using raceAgainstExit also fail fast instead of hanging on a dead pipe), dispose the connection. This mirrors how a TCP socket disconnect is already handled in connectViaTcp() and is consistent with the try { dispose() } catch {} pattern already used in forceStop():

this.cliProcess.stdin?.on("error", () => {
    if (this.forceStopping) {
        return; // forceStop is already tearing everything down
    }
    this.state = "error";
    try {
        this.connection?.dispose();
    } catch {
        // dispose is idempotent and may race with forceStop
    }
});

This:

• Keeps the 'error' listener registered so Node never promotes the event to uncaughtException.
• Disposes the MessageConnection, which fires the existing onClose handler (sets state = "disconnected") and rejects pending requests through vscode-jsonrpc's normal channels.
• Does not duplicate processExitPromise's job — that promise continues to independently reject in-flight RPCs racing against it with the captured stderr.

Either option fixes the crash; Option 2 additionally makes failure propagation more deterministic.

Environment

• Package: @github/copilot-sdk (Node.js SDK)
• File: nodejs/src/client.ts, connectToChildProcessViaStdio()
• Affects: RuntimeConnection.forStdio(...) consumers when the CLI child dies outside of forceStop().

Bug Confirmed ✅

I investigated this issue and confirmed it is a genuine bug.

What I looked at

File: nodejs/src/client.ts, lines 1801–1806, in connectToChildProcessViaStdio():

// Add error handler to stdin to prevent unhandled rejections during forceStop
this.cliProcess.stdin?.on("error", (err) => {
    if (!this.forceStopping) {
        throw err;
    }
});

What I found

The analysis in the issue is correct. The throw err on line 1804 fires synchronously inside a Node.js EventEmitter callback. Node.js does not propagate synchronous throws from EventEmitter listeners through promise chains — it surfaces them as an uncaughtException, which by default terminates the process.

This means:

• When forceStopping is true: the error is swallowed ✅ (intended behavior)
• When forceStopping is false: the error is re-thrown inside an EventEmitter callback → uncaughtException → process crash ❌ (same outcome as having no listener at all, just delayed by one tick)

The comment says the listener exists "to prevent unhandled rejections during forceStop," but the throw in the non-forceStop branch defeats this for the common case where the CLI dies unexpectedly (SIGKILL, crash, OS kill, etc.).

Additionally, as the issue notes, the information is redundant — the cliProcess.on("exit", ...) handler and MessageConnection's own writer-error detection already handle cleanup when the CLI exits.

Root cause

The else branch (re-throwing) was likely intended to preserve error visibility for unexpected failures, but this approach doesn't work in EventEmitter callbacks in Node.js.

Recommended fix

At minimum, Option 1 from the issue (make the handler a no-op) is the correct fix, since the existing exit handler and MessageConnection teardown already cover this scenario. Option 2 (actively disposing the connection) would additionally make failure propagation more deterministic for RPCs not using raceAgainstExit.

Generated by Bug Handler for issue #1427 · ● 1.5M · ◷