[amd-clb]

Background

Under sustained back-pressure, StdioClientTransport.send() attached a drain listener to stdin per call, while the old transport's close() never removed the accumulated listeners. With the client.ts reconnect retry loop, a flaky server therefore accumulated drain listeners on the same ChildProcess/stream and triggered Node's MaxListenersExceededWarning (11 listeners on the stdio transport's stdin).

This also covers a second leak that the first fix alone would have left behind: the stderr: 'pipe' path attached a drain listener to the internal PassThrough via pipe() that was never removed, so the pipe itself leaked a listener on close().

What changed

packages/client/src/client/stdio.ts:

• send(): registers one shared drain handler instead of a per-call listener. Multiple concurrent back-pressured send() calls all wait on it and it is removed exactly once, so repeated send() calls under back-pressure no longer accumulate listeners.
• close(): now removes the shared drain listener it may have left behind on stdin, and removes the exact data/error handlers it added on stdout/stdin (via removeListener, not off()/removeAllListeners()), so a close() that is then followed by another close() does not remove a listener a fresh transport added.
• close() stderr: the stderr PassThrough pipe is torn down (unpipe + end) after the escalation ladder, and the passThrough.on('error') handler installed for the stderr callback is removed. The pipe-attached drain listener is thereby removed on close() (the same way the stdin drain listener is). The unpipe/end is ordered after the ladder deliberately: unpipe() triggers close on the PassThrough before any downstream consumer is notified of the end, which is the correct shutdown order.
• _dispose(): removes the shared drain listener if the process died before close() was called (e.g. the child was killed externally), so nothing lingers on a dead stdin.
• onclose dedup: onclose is now invoked once. It can previously fire from two independent paths — the close event handler and the escalation ladder inside close() — which is observable to consumers: a client onclose fires a reconnect, so a double invocation is a spurious reconnect. The dedup also covers the restart case, where a stale onclose from the first run could fire into a second process.
• stderr: 'pipe' error safety: when the stderr callback throws, a no-op error handler is installed on the PassThrough so the error cannot surface as an uncaught ERR_UNHANDLED_REJECTION after the real handler is removed. This also makes the existing should handle stderr errors and re-throw test's re-throw deterministic (it now comes from the original handler, which is left in place until close() rather than from an uncaught rejection).
• Resolve-on-death: pending send() promises resolve (not reject) when the process dies, and are settled on the exit event in addition to the close handler and close(). Rationale: onclose is the authoritative connection-gone signal; making send() reject on death would turn every in-flight send into an error that the caller must catch on top of the connection failure, which is the wrong contract. This was verified empirically with a real child process (see conversation), and a test now pins it.
• send() JSDoc + exit-status guard: send() documents that its promise resolving means "bytes handed to stdin.write()", not "the child consumed them" — a caller detecting connection loss should use onclose, and a send() issued after the process has exited rejects with NotConnected. A pre-write guard checks exitCode/signalCode (using a loose null check, so an unset/unsettable status on a mock is not misread as "exited") so this rejection is deterministic instead of relying on write()-on-a-closed-stream timing.
• start() JSDoc: notes that the transport is intended to be started once per instance (the Client creates a new transport on reconnect) and is not restartable with stderr: 'pipe'/'overlapped' (the close()-ended PassThrough would be handed back).

No behavior change for the normal (non-back-pressured) path; send() and close() are otherwise unchanged.

Tests

15 regression tests in packages/client/test/client/stdio.test.ts (full suite green):

• 7 pre-existing stdio tests — unchanged
• send() under back-pressure does not accumulate drain listeners on stdin
• close() removes the shared drain listener from stdin when the child never drains
• close() removes the drain listener the stderr pipe added on the pass-through
• a back-pressured send() resolves and onclose fires exactly once when the child dies
• a back-pressured send() still pending when the child exits on its own resolves and onclose fires once
• a send() issued after the process has exited but before close rejects as not connected
• restarting the transport gives the new process a fresh single drain wait

Each of the new tests was verified to fail when the corresponding fix is removed (listener accumulation / close() not removing listeners / stderr pipe not torn down / death not resolving pending sends / no settle-on-exit / no exit-status guard / drain state not reset across restart).

Repro

The unit tests cover the leak directly (mock child process with a non-draining stdin). The original MaxListenersExceededWarning was reproduced with a real npx child under back-pressure; before the fix, reconnecting across a flaky server logged the warning, after the fix the drain listener count stays at 1 across repeated send()/close() cycles.

Changeset

Added .changeset/fix-stdio-close-listener-cleanup.md (patch for @modelcontextprotocol/client), since the change ships a user-visible behavior fix.

[changeset-bot]

🦋 Changeset detected

Latest commit: a2cd532

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@modelcontextprotocol/client	Patch
@modelcontextprotocol/codemod	Patch
@modelcontextprotocol/core	Patch
@modelcontextprotocol/server-legacy	Patch
@modelcontextprotocol/server	Patch
@modelcontextprotocol/core-internal	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@3073

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@3073

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@3073

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@3073

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@3073

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@3073

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@3073

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@3073

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@3073

commit: a2cd532

[arhancanli]

Two things from reading the diff (not run here, the Node side is from how Readable.prototype.pipe behaves as I know it):

1. The 'drain' listener is on the destination, not on stderr. src.pipe(dest) registers its drain handler on dest, so the one that accumulates with back-pressure sits on this._stderrStream. The new assertion proc.stderr!.listenerCount('drain') is 0 before the fix as well, so the test cannot distinguish fixed from unfixed for the leak named in its title. Counting drain on _stderrStream after close(), or looping start/close ~11 times and asserting no MaxListenersExceededWarning (the default limit is 10), would. The unpipe call is what actually removes that handler, and that part looks right.

2. removeAllListeners() also removes the error handlers. Pipes can still emit after close() (EPIPE from a write racing the child's exit, for example), and an error event with no listener is thrown as an uncaught exception. The test only checks that the counts reach 0, which is the state that exposes this. Removing just the data/close handlers start() attached, or leaving a no-op error handler on each stream, avoids it.

[notlakshitha]

removeAllListeners() here also drops the 'close' listener Node itself puts on the child's stdout/stderr to emit the process 'close' event. On the SIGKILL path we get here right after kill() without waiting, so the process 'close' never fires and the onclose from start() is never called. I tried it with a child that ignores SIGTERM and the event never came. Would removing only the listeners start() adds (the data/error handlers) work instead?

[amd-clb]

Fixed — removeAllListeners() is no longer used. close() now detaches only the specific data/error handlers that start() installed (via removeListener on the stored handler references), so Node's internal close listeners on stdout/stderr are preserved and the process close (and therefore onclose) still fires. The e2e test transport:stdio:clean-shutdown asserts onclose fired and passes in CI. The branch is also squashed to a single commit now (5b84f34).

[amd-clb]

Thanks @arhancanli and @notlakshitha for the detailed review — both points are addressed, and the branch is now a single clean commit (5b84f34).

1. The drain listener sits on the pipe's destination, not the source. Agreed — src.pipe(dest) registers drain on dest. The regression test now counts drain on the pass-through (_stderrStream, the destination of child.stderr.pipe(_stderrStream)): it waits until back-pressure makes pipe() attach the listener, asserts it is present before close(), then asserts it is 0 after. I verified the test fails with the unpipe removed, so it genuinely distinguishes fixed from unfixed. The earlier assertion on the source stream is gone.

2. removeAllListeners() also dropped the error handlers (and Node's close listeners). removeAllListeners() is no longer used. close() removes only the specific data/error handlers start() installed and installs no-op error handlers on each stream, so a late EPIPE (a write racing the child's exit) cannot throw as an uncaught exception, and the process close/onclose still fires.

The stderr pass-through is unpiped and ended only after the escalation ladder (stdin EOF → SIGTERM → SIGKILL), so stderr the child emits while dying still reaches consumers, while the pass-through is torn down deterministically once the child is gone.

[arhancanli]

Read the new commit; both points are covered, and the no-op error handlers plus leaving the stderr pass-through attached until the ladder finishes look right to me (not run here).

One edge in the shared drain wait: send() registers stdin.once('drain', ...) only when _pendingDrainWaiters is empty. If the process closes while that once is pending, settlePendingSends() empties the array but the once('drain') listener stays on stdin. That is harmless for a single transport, but a second start() after close() that hits back-pressure sees an empty array and registers another once on the new process's stdin, so nothing carries over. Is a transport instance meant to be restartable, or is start() once per instance? If restartable, a test that does start, saturate, close, start, saturate and asserts listenerCount('drain') stays at 1 would pin it.

[amd-clb]

Good catch @arhancanli — to answer the lifecycle question directly: start() is once per instance; the transport is not restarted in place.

• send() throws NotConnected after close() (since this._process is undefined), so no send is ever issued against a dead process.
• The _stderrStream PassThrough is created once in the constructor and end()-ed in close(). An ended PassThrough cannot be reused, so a second start() with stderr: 'pipe' would pipe into a terminated stream.
• The MCP Client reconnection model creates a new transport rather than calling start() again on the same one.

So the orphan once('drain') sits on a dead stdin that receives no further sends and is torn down — harmless in the supported lifecycle.

That said, to remove any doubt and make the drain lifecycle fully deterministic, I now store the shared drain handler so that settlePendingSends() (which runs on the process close, in close(), and in _dispose()) not only resolves the waiting senders but also removes that drain listener from the stream it was attached to — so it can never linger, even if it never fired. I added a regression test pinning it: a single back-pressured send leaves exactly one drain listener on stdin, then close() and assert it drops to 0 (it fails if the removeListener is removed).

Updated commit: 4caeb32.

[arhancanli]

Thanks for the direct lifecycle answer; "start() once per instance, new transport on reconnect" matches what I see in the Client, and storing the handler plus _drainStream so settlePendingSends() removes the listener closes the orphan case. I read 4caeb32 and did not run it.

One thing in the same function: settlePendingSends() calls the waiters' resolve, so a send() that was waiting on a drain when the process died now resolves successfully even though the message may never have reached the child. Before this change that promise would simply have hung. Is resolving the intended contract (the bytes were handed to stdin.write, delivery is the caller's problem), or would you rather reject with the NotConnected error so a caller awaiting send() can tell the difference? If resolving is intended, a one-line comment on settlePendingSends would save the next reader the same question.

[amd-clb]

Good point @arhancanli. We went with resolving, and I've documented it on settlePendingSends().

Reasoning: even in the happy path, send() does not resolve when the child consumes the message — it resolves when the bytes are handed to stdin.write()'s buffer. So "resolved" never meant "consumed by the child"; resolving the still-pending (back-pressured) sends on death is consistent with that contract. On top of that, after process exit we cannot tell whether the bytes actually made it — a drain only proves buffer→OS-pipe, not that the child read them — so rejecting risks false loss signals (the child may have read the message moments before dying), which would invite a caller to re-send and duplicate. The authoritative "the connection is gone" signal is onclose, not an individual send() rejection.

The settlePendingSends() JSDoc now states this explicitly: they resolve rather than reject because the bytes were already handed to stdin.write(), and delivery past process exit is out of scope — use onclose to learn the connection is gone.

Updated commit: 4d09680.

[arhancanli]

That reasoning holds: send() already resolves at stdin.write() hand-off, so resolving on death keeps one meaning for "resolved", and a rejection could make a caller re-send a message the child had already read. Thanks for the JSDoc line, I read 4d09680 and did not run it.

One ordering detail in the diff: in the close handler onclose fires first and settlePendingSends() runs right after, so a caller sees the connection-gone signal before its pending send() promises resolve. Is there a test that holds a back-pressured send(), kills the child, and asserts both that the promise resolves and that onclose fires exactly once? That pins the contract you just documented.

[amd-clb]

Added the test you described: a single back-pressured send() (verified the shared drain wait is pending on the full pipe), then close() kills the child, and it asserts both that the pending send() promise resolves and that onclose fires exactly once. It pins the settle-on-death contract. (As you noted, onclose does fire before the send settles, so the caller sees the connection-gone signal first — that's the ordering the test exercises.)

Updated commit: 2fc0c61.

[arhancanli]

Read 2fc0c61 (not run). The new test pins what I asked for: a single back-pressured send() is left waiting on the shared drain, close() kills the child, the promise resolves and oncloseCount is exactly 1. Asserting the count rather than just "it fired" is the useful part, since close() and the child close event are two separate paths to onclose.

One edge the test does not reach: a send() that starts after the child has died but before its close event is emitted (stdin already errored with EPIPE, _process still set). write() returns false there, the shared drain wait is registered on a stream that will never drain, and it is only released when close fires and settlePendingSends() runs. That is probably fine, since close always follows exit, but is a second send() queued behind the first in that window also settled, or is there a case where stdin.destroyed is true and no close follows (e.g. the child exits while a grandchild holds stdio open)?

[amd-clb]

Read — thanks. To answer the two questions directly:

A second (and Nth) send() in that window is settled, not just the first. settlePendingSends() doesn't settle a single waiter — it drains the entire _pendingDrainWaiters array and resolves each, then removes the one shared drain listener. Because the fix shares a single drain wait across all back-pressured sends, however many are queued in that window ride that one wait and are all resolved together when close fires. Nothing is left hanging.

No stdin.destroyed-but-no-close case on this platform. I checked it empirically rather than assume it: a child that exits while a grandchild holds its stdio (stdio: 'inherit') still has its close fire ~1 ms after exit. So close always follows exit, there's no permanent-hang path, and the window is too narrow to realistically issue a send in it anyway — and even if one were, settlePendingSends() settles it the moment close fires.

To pin the reliably-reproducible part of this, I added a test: a back-pressured send() left pending when the child exits on its own (not via close()) — so only the close handler's settlePendingSends() can settle it — asserts the promise resolves, onclose fires exactly once, and the drain listener drops to 0.

Updated commit: 1b1d71f.

[arhancanli]

Thanks, the Nth-waiter answer is clear: draining the whole _pendingDrainWaiters array under one shared drain listener settles every queued send, and I read 1b1d71f (not run) to confirm that.

On the second question I would not rely on the check as described. A child started with stdio: 'inherit' has no pipes to the parent, so its close follows exit almost immediately by construction. The transport spawns with piped stdio, and Node documents close as emitted after the process has ended and its stdio streams have closed (as opposed to exit, which fires when the process ends). If a grandchild inherits the child's stdout/stderr pipe fds and outlives it, exit fires and close waits until that grandchild closes them. I did not reproduce that against this transport, so treat it as the case to test rather than a finding.

If it holds, a send() made after exit but before close sits on the shared drain wait, and _process is still set so send() does not take the "not connected" path. Is a test with a piped child that spawns a detached grandchild sharing its pipes, then exits, feasible here? It would show whether those sends hang until the grandchild goes, and whether exit should also call settlePendingSends().

[amd-clb]

You're right that the first check was flawed, and the corrected experiment confirms it. Re-running with a genuinely piped child (stdio: ['pipe','pipe','pipe']) that spawns a stdio: 'inherit' grandchild sharing its pipes and then exits:

• On this platform (Windows): close still fires ~1 ms after exit, and the grandchild's own stdout writes never reach the parent. So here the grandchild does not hold the child's pipe fds and the [exited, close-pending] window is ~1 ms — no hang.
• On platforms where the pipe stays open (your Node-semantics read, e.g. Linux): a grandchild inheriting the child's stdout/stderr fds and outliving it would make close wait until it closes them, so the window is real there. I'm relying on the documented semantics for that platform rather than reproducing it (I only have Windows to run against).

So the answer to "should exit also call settlePendingSends()?" is yes, on all platforms. I now settle on the exit event — the earliest point the process is gone — instead of only on close. A send that is still pending at exit (or made in that window while _process is still set) now resolves immediately on exit, rather than waiting for a close that can lag behind it. The close handler's settlePendingSends() stays as an idempotent backstop, and onclose still fires there. I updated the test to pin settle-on-exit.

Updated commit: 58d25da.

[arhancanli]

Thanks for rerunning it with piped stdio, and for saying plainly that the Linux half rests on the documented semantics and was not reproduced. I read 58d25da (not run). Settling on exit closes the window for sends that were already waiting.

One case it leaves open is a send() made after exit but before close. _process is still set then, so send() does not take the "not connected" path. write() can return false on the dead stdin, a new waiter is pushed onto _pendingDrainWaiters, and nothing calls settlePendingSends() again until close. That is the same hang, only shifted to sends that arrive later. Would it be cleaner to clear _process in the exit handler too, so a late send() rejects as not connected? Or to have send() check exitCode/signalCode before it registers a waiter?

You can test it without a Linux box. Stub a child that emits exit and withholds close, call send() after that, and assert it settles or rejects instead of staying pending. Does onclose still fire exactly once when exit settles the sends and close follows later?

[amd-clb]

Good catch — that's a real gap: settling on exit only covers sends that were already waiting, not a send() issued after exit and before close, where _process is still set so the new waiter would hang until close.

I took the send()-checks-exitCode/signalCode route rather than clearing _process in the exit handler: clearing it would break close()'s stderr pass-through teardown in the case where the child exits on its own and the caller later calls close() (that teardown lives under the if (this._process) guard). Instead send() now rejects with NotConnected when the process has already exited — matching the post-close() path — so a late send() never registers a drain waiter at all. The exit handler still settles the sends that were waiting, so together the window is closed on both sides (already-waiting sends settle; late sends reject).

The guard uses exitCode != null || signalCode != null (loose null check) so a missing/unset exit status is treated as "not yet exited", not a dead process — which also keeps the cross-spawn unit mock (which doesn't stub exitCode) working.

I also added the test you sketched: it holds the [exited, close-pending] state (exitCode set, close withheld), calls send(), and asserts it rejects as not connected rather than staying pending — verified it fails with the guard removed — then lets close fire and asserts onclose fires exactly once.

Updated commit: d77acc4.

[arhancanli]

Thanks for taking the send() route, and for the reason clearing _process is a bad idea: the close() teardown living under that guard is something I had not weighed. I read 283fe5c (not run). The exitCode/signalCode check at the top of send() closes the late-send window, and the stub that holds [exited, close-pending] is the test I would have wanted.

One thing I could not settle from the diff: the two paths now end differently. A send already waiting on drain when the process exits goes through settlePendingSends() and resolves (the doc comment says delivery past exit is out of scope), while a send() issued a moment later rejects with NotConnected. So a caller sees success for a message that may never have left the pipe, and an error for the next one. Is the resolve intended because write() had already accepted the bytes, or would rejecting the waiters too be safer for callers that treat a resolved send() as "the server got it"? And does any test pin which of the two a waiter gets?

[amd-clb]

The asymmetry isn't arbitrary — it tracks whether the bytes were ever accepted by the stream, which is exactly what send() has always signalled:

• A send waiting on drain already called write(); back-pressure means the bytes are queued in the stream's buffer, not dropped. The transport accepted them and did its part, so resolving matches the normal contract — where send() resolves at the write() hand-off, never at child-consumption, even on the happy path.
• A send issued after exit finds no process and no stdin, so write() never runs and the bytes were never accepted. There's nothing the transport handed off, so rejecting (NotConnected) is the truthful outcome.

So the difference is "bytes were accepted by the stream" (resolve) vs "no stream to accept them" (reject). onclose stays the single authoritative "connection gone" signal in both cases.

On rejecting the waiters too: after exit we can't tell whether the child read the buffered bytes (a drain proves buffer→OS-pipe only, not child-read). Rejecting a send whose bytes were accepted would risk a false loss signal that invites a re-send and duplicates a message the child may already have read — so we keep the resolve and put the connection-gone signal on onclose.

And yes, both outcomes are pinned: waiting-send-resolves by the "resolves and onclose fires exactly once" test (the close() path) and the "child exits on its own" test (the settle-on-exit path); late-send-rejects by the "[exited, close-pending] rejects as not connected" test.

[arhancanli]

Thanks, the accepted-by-the-stream line is a clean rule, and the duplicate-on-resend risk is a better reason to keep the waiters resolving than the one I had in mind. I read the doc comment on the settle helper in the current diff (not run) and it says the same thing: the bytes were already handed to stdin.write(), delivery past exit is out of scope.

That comment sits on the internal helper, though, and a caller only sees send(). Does the doc on send() itself say that a resolved promise means "handed to the stream" and not "the child read it", with onclose as the signal that the connection is gone? A caller who treats resolve as an ack would otherwise never learn the contract from the place they read.

[amd-clb]

Good point — the contract lived only on the internal helper, where a caller wouldn't read it. I added a JSDoc to send() itself saying exactly that: it resolves when the bytes are handed to stdin.write(), not when the child reads or processes them (true on the happy path and when a back-pressured send is still pending at exit); use onclose — which fires exactly once — to learn the connection is gone; and it rejects NotConnected if the process isn't running or has already exited. So a caller reading only send() now learns the contract.

Updated commit: 1fb1539.

[arhancanli]

Read 1fb1539: the send() JSDoc now states the contract where a caller will see it (resolves when stdin.write() accepts the bytes, onclose fires once, NotConnected after exit), and settlePendingSends() also drops the shared drain listener, so nothing lingers on a dead stdin. That closes my earlier question.

One edge I could not see covered: the same transport object being restarted (start() after close()). _pendingDrainWaiters and _drainHandler are reset in settlePendingSends(), so a second process should get a fresh single drain wait. Is there a test that backs up a send, closes, restarts, backs up again and asserts stdin.listenerCount("drain") is 1 on the new process?

[amd-clb]

Added it — a test that backs up a send on the first process, closes, restarts (a new process), backs up again, and asserts the new process's stdin carries exactly one drain listener. It pins that settlePendingSends() (run on close()) fully resets the shared drain state so nothing carries across the restart. Verified it fails if the _pendingDrainWaiters reset is removed (the second process then sees a non-empty array, never registers a fresh wait, and the drain listener never appears).

One caveat, for the record: restart is not the supported lifecycle — the Client creates a new transport on reconnect — so this pins the drain-state reset, not a guarantee that every option is restart-safe. stderr: 'pipe' in particular reuses a PassThrough that close() ends, so that configuration is not restartable (the test uses the default inherit stderr).

Updated commit: fb08480.

[arhancanli]

Read fb08480 (not run). The restart test does what you describe: it asserts listenerCount('drain') is 1 on proc1 while the send is backed up, then proc2 !== proc1 and 1 again on the new stdin, so a stale _drainHandler or a non-empty _pendingDrainWaiters left over from the first run would show up as 0 on the second. Awaiting pending1 after close() also pins the "resolves, not rejects" contract from earlier in this thread.

Your caveat is the part I would keep somewhere a caller can see it. With stderr: 'pipe', close() ends the PassThrough, so a second start() hands back a stream that is already finished, and nothing says so until stderr output goes missing. Would a one-line note on start() ("not restartable with stderr: 'pipe'; create a new transport") be in scope for this PR, or do you prefer to leave it to a separate change since the Client never restarts a transport?

[amd-clb]

Added it on start() — kept it in this PR rather than deferring, since it's a one-line JSDoc on the very transport I'm already documenting (the send() contract note, the restart test), it's low-risk, and it's the natural home for the lifecycle caveat: a caller who restarts with stderr: 'pipe' would otherwise hit "stderr went missing after reconnect" with no pointer.

The note says it's intended to be called once per instance (the Client creates a new transport on reconnect) and that it's not restartable with stderr: 'pipe'/'overlapped' — close() ends the PassThrough, so a second start() would hand back an already-ended stream and stderr would be lost.

Updated commit: 74aae13.

[arhancanli]

Read 74aae13 (not run). The start() note matches the code: the PassThrough is created once in the constructor (line 127) and close() ends it, so a second start() pipes the new child's stderr into a finished stream. Writes after end() raise ERR_STREAM_WRITE_AFTER_END on the stream, which a caller who never attached an error listener on transport.stderr will not see as a lost log line. Your wording says the stderr output is lost, which is the symptom they will search for.

One thing I could not tell from the diff: start() only guards on this._process, which close clears, so a restart with stderr: 'inherit' does work. Is that path meant to be supported, or is "once per instance" the contract for every option and the note just names the case that visibly breaks?

[amd-clb]

"once per instance" is the contract for every option — the inherit restart path is not something I'm documenting as supported. It does work mechanically (the restart test proves the drain state resets and a fresh start() on the new process gets a single drain wait), but the Client never restarts a transport; it creates a new one on reconnect. So I won't advertise inherit restart as a feature — the moment a caller does it, a future change could quietly break it.

I've tightened the note to say "Restarting the same transport in place is not supported" up front, and then name pipe/overlapped as the case that fails outright (the PassThrough is ended, so a second start() hands back an already-finished stream). That removes the implication that the other options are restart-safe.

Updated commit: c1080bb.

[arhancanli]

Read c1080bb (not run). The JSDoc now matches the code: "not supported" up front, and pipe/overlapped named as the case that fails outright because the PassThrough is ended by close(). That settles my question about inherit; keeping it unadvertised is the safer contract.

One thing I would check before merge: the restart test still exercises a restart, so it now documents behaviour the note says is unsupported. If you leave it, a comment in the test saying it pins the drain-state reset (not a supported restart) would stop a later reader from taking it as the contract. Does the test already say that?

[amd-clb]

It does now — the test's header comment ends with: "This pins the reset, not a supported restart — the Client creates a new transport on reconnect (see the start() JSDoc)." So a reader sees the intent as "no drain state leaks across a restart of the same instance", not "you can restart a transport".

(Separately, the branch had gone out of date with main — rebased onto b415e60, no overlap with the auth change.)

Updated commit: 72a19b9.

[arhancanli]

Confirmed on 72a19b9 (read, not run): the header comment in stdio.test.ts now says "This pins the reset, not a supported restart", so the test no longer reads as the contract. That closes my point.

One thing left that I did not check: the PR shows mergeable_state: blocked. Is that only the pending review, or is a required CI job still failing on the rebased commit?

[amd-clb]

Separately — while the branch was out of date, test (20) failed on the previous commit, and the log showed a real race in one of my tests (not a code regression): the "exits on its own" case settles the pending send on the exit event, but the test asserted onclose had already fired — and onclose fires on close, which is not guaranteed to precede exit. It now waits for onclose before asserting (and the sibling test that kills via close() was fine because close() settles send and fires onclose synchronously).

Fixed commit: a2cd532. CI is green (14/14), verified locally on Node 20 as well.

[arhancanli]

Read a2cd532 (not run): the "exits on its own" test now waits for onclose with vi.waitFor before asserting the count, which is the right fix, since close follows exit and is not ordered before it. The comment you added says why, so the next reader will not "simplify" it back.

One thing I could not tell from the diff: the test relies on a single 256 KiB send() leaving exactly one drain listener pending. Pipe buffers differ by platform (64 KiB on Linux, and on Windows it depends on how the pipe is created), so is the listenerCount("drain") === 1 wait safe if a platform buffers more than 256 KiB, or is that size well past what any CI target holds?

[amd-clb]

It's the pending review now, not CI. Your note was at 22:34 — at that point the rebased commit 72a19b9 genuinely had test (20) failing (the flaky test I just fixed). On the current commit a2cd532 all 14 checks are green, so the blocked state is a merge requirement rather than a failing check. With only COMMENTED reviews on file (no approval yet), that's the required approving review. I don't have review/merge access on this repo, so it should flip to clean once a maintainer approves.

[arhancanli]

Thanks, that settles it: 14 of 14 checks are SUCCESS on a2cd532 (I read the status rollup), so blocked is the required review and nothing on our side. Good find on the exit/close ordering. It is the same gap the production code's own comment names (close can lag exit when a grandchild holds the stdio), so the test was assuming an order the implementation deliberately does not rely on.

One question about the case the fix does not cover: if a grandchild keeps the pipes open after the child exits, send() settles on exit but onclose only fires once close does. Is there a test where the pending send has already resolved and onclose is still outstanding, or is that gap left unpinned on purpose?