From cf1ce284284aa257847a1cf8e16fbc8ccaa72166 Mon Sep 17 00:00:00 2001 From: Tabitha-2696 Date: Fri, 16 Oct 2026 01:16:05 -0400 Subject: [PATCH 1/5] fix(mcpl): a transport error on a connection with no 'error' listener is logged, not thrown (#391) setupLifecycle passed every transport error on as the connection's own 'error', with no listener check. EventEmitter throws an 'error' nothing listens for, and the transport reports its errors from I/O callbacks, where nothing catches that throw, so it ended the process. The framework always listens, so hosts weren't affected, but McplServerConnection is exported, and a probe, a playtest or a harness holding one often attaches no 'error' listener. Probed on main 91e70eda: a separate process holding a bare WebSocket connection died with "Unhandled 'error' event" when its server sent a frame with a reserved opcode. #208's reportInboundFailure already guarded the same case, emitting only with a listener and catching one that throws. That guard is now one private helper, emitToErrorListeners, used by both sites: it gives the error to the listeners if there are any, catching and logging one that throws, and says whether there were. reportInboundFailure logs its line as before and hands the error to the helper. A transport error with no listener is logged as `[mcpl] : transport error: `, its message through logDetail; with a listener nothing is logged, as before. A listener that throws on a transport error is caught and logged now too, where it used to throw into the transport's callback. The stdio transport passes on the child's own 'error' (a failed kill, say) and a read error on its stderr, since the connection always listens on it once wired; since #384, a failure on stdin or stdout ends the child instead. A WebSocket error, the child's own 'error' and a stderr read error reach this path, which is why the probe uses a WebSocket. Tests (test/mcpl-transport-error-listeners.test.ts): - with no 'error' listener, a transport error doesn't throw and is logged; - with a listener, the listener gets the error and nothing is logged (a control: it passes on the base too); - a listener that throws on a transport error is caught and logged; - a separate process holding a bare WebSocket connection (fixture mcpl-bare-ws-error-probe.mjs) survives a reserved-opcode frame, sees the connection close, and logs the error. Co-Authored-By: Claude Opus 5.5 --- .../391-transport-error-no-listener.fixed.md | 9 +++ src/mcpl/server-connection.ts | 26 ++++++-- test/fixtures/mcpl-bare-ws-error-probe.mjs | 37 +++++++++++ test/mcpl-transport-error-listeners.test.ts | 66 +++++++++++++++++++ 4 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 changelog.d/391-transport-error-no-listener.fixed.md create mode 100644 test/fixtures/mcpl-bare-ws-error-probe.mjs create mode 100644 test/mcpl-transport-error-listeners.test.ts diff --git a/changelog.d/391-transport-error-no-listener.fixed.md b/changelog.d/391-transport-error-no-listener.fixed.md new file mode 100644 index 000000000..e249116dd --- /dev/null +++ b/changelog.d/391-transport-error-no-listener.fixed.md @@ -0,0 +1,9 @@ +- **A transport error no longer ends a process whose `McplServerConnection` + has no `'error'` listener** (#391). The connection passed each transport + error (a WebSocket protocol error, say) on as its own `'error'`, which + EventEmitter throws when nothing listens, from inside the transport's + callback, where nothing caught it. The framework always listens, so hosts + weren't affected; a probe, a playtest or a harness holding a bare connection + was. With no listener the error is now logged + (`[mcpl] : transport error: …`), and a listener that throws is + caught and logged. diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 7f7eb4337..d0047a0ec 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -1426,12 +1426,23 @@ export class McplServerConnection extends EventEmitter { // the log line prints the same through log-value. const error = new Error(`MCPL server "${this.id}" inbound dispatch failed${where(boundedForLog)}: ${boundedForLog(describeCause(cause))}`, { cause }); console.error(`[mcpl] ${this.id}: inbound dispatch failed${where(logValue)}: ${logDetail(describeCause(cause))}`); - // EventEmitter's unhandled 'error' behavior would defeat containment before - // framework wiring, so stderr remains the fallback when no listener exists. - if (this.listenerCount('error') > 0) { - try { this.emit('error', error); } - catch (reportError) { console.error(`[mcpl] ${this.id}: an 'error' listener failed: ${logDetail(describeCause(reportError))}`); } - } + this.emitToErrorListeners(error); + } + + /** + * Give an error to the connection's 'error' listeners, and say whether it + * has any. EventEmitter throws an 'error' that nothing listens for, and + * from a transport's callback, where these errors arise, nothing catches + * that throw, so it ends the process. A bare connection has no listener: + * the class is exported, and a probe, a playtest or a harness often attaches + * none. So with no listener the caller logs the error instead. A listener + * that throws is caught and logged too. + */ + private emitToErrorListeners(error: Error): boolean { + if (this.listenerCount('error') === 0) return false; + try { this.emit('error', error); } + catch (listenerError) { console.error(`[mcpl] ${this.id}: an 'error' listener failed: ${logDetail(describeCause(listenerError))}`); } + return true; } /** @@ -1527,7 +1538,8 @@ export class McplServerConnection extends EventEmitter { private setupLifecycle(transport: McplTransport): void { this.hostCloseRequested = false; transport.on('error', (err: Error) => { - this.emit('error', new Error(`MCPL server "${this.id}" transport error: ${err.message}`)); + const error = new Error(`MCPL server "${this.id}" transport error: ${err.message}`); + if (!this.emitToErrorListeners(error)) console.error(`[mcpl] ${this.id}: transport error: ${logDetail(err.message)}`); }); transport.on('close', (info: TransportCloseInfo) => { diff --git a/test/fixtures/mcpl-bare-ws-error-probe.mjs b/test/fixtures/mcpl-bare-ws-error-probe.mjs new file mode 100644 index 000000000..1161ec27e --- /dev/null +++ b/test/fixtures/mcpl-bare-ws-error-probe.mjs @@ -0,0 +1,37 @@ +// Runs as a separate process holding a bare McplServerConnection over +// WebSocket, with no 'error' listener, as a probe or a playtest would. The +// server completes the handshake, then sends a frame with a reserved opcode, +// which the ws client reports as an 'error' and answers by closing. An error +// thrown with nothing listening would end this process before it prints +// "survived". +import { WebSocketServer } from 'ws'; +import { McplServerConnection } from '../../src/mcpl/server-connection.js'; + +const wss = new WebSocketServer({ host: '127.0.0.1', port: 0 }); +await new Promise((resolve) => wss.once('listening', resolve)); +wss.on('connection', (ws) => { + ws.on('message', (data) => { + const message = JSON.parse(String(data)); + if (message.method === 'initialize') { + ws.send(JSON.stringify({ jsonrpc: '2.0', id: message.id, result: { protocolVersion: '2024-11-05', capabilities: {} } })); + } else if (message.method === 'notifications/initialized') { + // A frame header with opcode 3, which RFC 6455 reserves. + ws._socket.write(Buffer.from([0x83, 0x00])); + } + }); +}); + +const connection = await McplServerConnection.connect( + { id: 'probe', url: `ws://127.0.0.1:${wss.address().port}` }, + { version: '0.5' }, +); +const closed = new Promise((resolve) => connection.once('close', resolve)); +connection.ready(); +console.log('connected'); +const timer = setTimeout(() => { console.log('no close within 30 s'); process.exit(1); }, 30_000); +await closed; +clearTimeout(timer); +console.log('closed'); +console.log('survived'); +await connection.close(); +wss.close(); diff --git a/test/mcpl-transport-error-listeners.test.ts b/test/mcpl-transport-error-listeners.test.ts new file mode 100644 index 000000000..ea228c6b6 --- /dev/null +++ b/test/mcpl-transport-error-listeners.test.ts @@ -0,0 +1,66 @@ +/** + * A transport error on a connection nothing listens to for 'error' (#391). + * EventEmitter throws an 'error' with no listener, and a transport reports its + * errors from I/O callbacks, where nothing catches the throw: it ended the + * process. McplServerConnection is exported, and a probe, a playtest or a + * harness holding one often attaches no 'error' listener (the framework + * always does). The error is logged instead, and given to listeners when + * there are any, a throwing one caught as #208's inbound failures are. + */ +import { test, type TestContext } from 'node:test'; +import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; + +import { McplServerConnection } from '../src/mcpl/server-connection.js'; +import { McplTransport } from '../src/mcpl/transport.js'; + +class LineTransport extends McplTransport { + constructor() { super(); } + readonly kind = 'websocket' as const; + writeLine(): void {} + async close(): Promise {} +} + +function bareConnection(t: TestContext) { + const transport = new LineTransport(); + const connection = new (McplServerConnection as any)('srv', null, transport) as McplServerConnection; + connection.ready(); + const logged: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); + const reset = Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }); + return { connection, transport, logged, reset }; +} + +test("a transport error on a connection with no 'error' listener is logged, never thrown", (t) => { + const { connection, transport, logged, reset } = bareConnection(t); + assert.equal(connection.listenerCount('error'), 0); + assert.doesNotThrow(() => transport.emit('error', reset)); + assert.deepEqual(logged, ['[mcpl] srv: transport error: "read ECONNRESET"']); +}); + +test("with an 'error' listener, a transport error is the listener's, and nothing is logged", (t) => { + const { connection, transport, logged, reset } = bareConnection(t); + const reported: Error[] = []; + connection.on('error', (error: Error) => reported.push(error)); + transport.emit('error', reset); + assert.deepEqual(reported.map((error) => error.message), ['MCPL server "srv" transport error: read ECONNRESET']); + assert.deepEqual(logged, []); +}); + +test("an 'error' listener that throws on a transport error is caught and logged", (t) => { + const { connection, transport, logged, reset } = bareConnection(t); + connection.on('error', () => { throw new Error('listener exploded'); }); + assert.doesNotThrow(() => transport.emit('error', reset)); + assert.deepEqual(logged, [`[mcpl] srv: an 'error' listener failed: "listener exploded"`]); +}); + +test('a separate process holding a bare WebSocket connection survives a malformed frame, and sees the close', () => { + const probe = spawnSync(process.execPath, [ + fileURLToPath(new URL('./fixtures/mcpl-bare-ws-error-probe.mjs', import.meta.url)), + ], { encoding: 'utf8', timeout: 60_000 }); + assert.equal(probe.error, undefined); + assert.equal(probe.status, 0, probe.stderr); + assert.deepEqual(probe.stdout.trim().split('\n'), ['connected', 'closed', 'survived']); + assert.match(probe.stderr, /\[mcpl\] probe: transport error: "Invalid WebSocket frame: invalid opcode 3"/); +}); From 2c650268a9403e382f994ff603fb4dc5da3a0aa4 Mon Sep 17 00:00:00 2001 From: Tabitha-2696 Date: Fri, 16 Oct 2026 16:46:34 -0400 Subject: [PATCH 2/5] fix(mcpl): a transport's own 'error' with nothing listening is logged, not thrown, so a malformed frame at upgrade no longer ends the host (#391) Our internal review of the first commit found the same rule broken one layer down, and a probe confirmed it. A WebSocket MCPL server that sends a malformed frame in the same write as its 101 upgrade response ended the process that was connecting, framework hosts included: on main 14e25dc0, 3 of 3 runs of the new fixture mcpl-ws-upgrade-error-probe.mjs exited 1 with "Unhandled 'error' event ... Emitted 'error' event on WebSocketTransport instance" and "RangeError: Invalid WebSocket frame: invalid opcode 3". ws's setSocket unshifts the bytes that came with the upgrade before it emits 'open' (ws/lib/websocket.js 263-271), the transport is constructed in that 'open' handler, and the frame's error is raised on a nextTick, which Node runs before the promise continuation where handshake() attaches its listener. The transport forwarded ws's 'error' with no listener check, so EventEmitter threw. #391's case spares framework hosts; this one ends them, so it's fixed here, as #391's rule at the transport's own forwarders. McplTransport gains reportError: it emits 'error' when something listens and logs `[mcpl] a transport error, with nothing listening for it: ` otherwise. WebSocketTransport's ws 'error' forwarder and StdioTransport's child 'error' forwarder use it; no trigger was found for the stdio one's window, but the rule is the same. #384 has since rewritten the stdio pipe listeners: an error on stdin or stdout ends the transport through the exit's close and emits nothing. stderr's read-error listener emitted only when something listened and was silent otherwise; it goes through reportError too, so with nothing listening it's logged. After the fix the probe's connect is refused and the process lives: promptly when the server closes the connection after answering ws's close frame ("transport closed before handshake completed (websocket closed, code 1006)"), and after about 30 s when it never does, when ws's close timeout or the handshake's own 30 s timeout ends the wait. The review also found these, fixed here too: - a listener that threw on a transport error was logged without the error it was handling, so the transport error's text reached no log line. emitToErrorListeners' line now ends ", handling "; - the helper's doc said the caller always logs when nothing listens, but reportInboundFailure logs first either way. It now says so; - the process test's assertion message carries the probe's stdout too, where the fixture prints its outcome. Tests (test/mcpl-transport-error-listeners.test.ts): a stand-in child, its stderr and a stand-in socket each emit 'error' with nothing listening on the transport: nothing is thrown, all three are logged, and once listeners attach they get the errors and nothing more is logged. A separate process connects to the upgrade probe's server: it is refused, the error is logged, and it exits 0. The throwing-listener test asserts the line with what it was handling. Co-Authored-By: Claude Opus 5.5 --- src/mcpl/server-connection.ts | 12 +++- src/mcpl/transport.ts | 31 +++++++--- test/fixtures/mcpl-ws-upgrade-error-probe.mjs | 48 ++++++++++++++++ test/mcpl-transport-error-listeners.test.ts | 56 +++++++++++++++++-- 4 files changed, 131 insertions(+), 16 deletions(-) create mode 100644 test/fixtures/mcpl-ws-upgrade-error-probe.mjs diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index d0047a0ec..c349d3f0d 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -1435,13 +1435,19 @@ export class McplServerConnection extends EventEmitter { * from a transport's callback, where these errors arise, nothing catches * that throw, so it ends the process. A bare connection has no listener: * the class is exported, and a probe, a playtest or a harness often attaches - * none. So with no listener the caller logs the error instead. A listener - * that throws is caught and logged too. + * none. So with no listener, the error is the caller's to log, unless it + * already has. A listener that throws is caught and logged, with the error + * it was handling. */ private emitToErrorListeners(error: Error): boolean { if (this.listenerCount('error') === 0) return false; try { this.emit('error', error); } - catch (listenerError) { console.error(`[mcpl] ${this.id}: an 'error' listener failed: ${logDetail(describeCause(listenerError))}`); } + catch (listenerError) { + console.error( + `[mcpl] ${this.id}: an 'error' listener failed: ${logDetail(describeCause(listenerError))}` + + `, handling ${logDetail(error.message)}`, + ); + } return true; } diff --git a/src/mcpl/transport.ts b/src/mcpl/transport.ts index 8be8747ed..d13dd7ada 100644 --- a/src/mcpl/transport.ts +++ b/src/mcpl/transport.ts @@ -23,6 +23,7 @@ import { StringDecoder } from 'node:string_decoder'; import WebSocket from 'ws'; import type { McplServerConfig } from './types.js'; +import { logDetail } from './log-value.js'; /** Why/how a transport closed. `code`/`signal` are populated for stdio child * exits; `reason` carries a WebSocket close reason or an error message. */ @@ -97,6 +98,20 @@ export abstract class McplTransport extends EventEmitter { else this.stderrBuffer.push(line); } + /** + * Report a transport error: to the 'error' listeners if there are any, else + * as a log line. EventEmitter throws an 'error' that nothing listens for, + * and a transport reports its errors from I/O callbacks, where nothing + * catches that throw, so it would end the process. A transport has no + * listener from its creation until the handshake attaches one, and a + * WebSocket server that sends a malformed frame with its upgrade response + * makes ws raise an 'error' in that window. + */ + protected reportError(err: Error): void { + if (this.listenerCount('error') > 0) this.emit('error', err); + else console.error(`[mcpl] a ${this.kind} transport error, with nothing listening for it: ${logDetail(err.message)}`); + } + /** Emit 'close' exactly once. */ protected markClosed(info: TransportCloseInfo): void { if (this._closed) return; @@ -228,7 +243,7 @@ export class StdioTransport extends McplTransport { if (carry.length > 0) this.pushStderr(carry); }); - this.child.on('error', (err) => this.emit('error', err)); + this.child.on('error', (err) => this.reportError(err)); // The child's stdin and stdout carry the protocol, so after an error on // either, a write's EPIPE or a read error, this transport can't reach the // child even if it's still running. The handler sends a running child @@ -249,13 +264,11 @@ export class StdioTransport extends McplTransport { // listener absorbs that copy. this.child.stdout?.on('error', endOnPipeError); this.rl.on('error', () => { /* readline's copy of a stdout error, handled above */ }); - // stderr carries diagnostics only: a read error on it is reported if - // anyone listens and never thrown. It ends nothing once the connection - // is wired; during a handshake the listener is the handshake's, which - // fails it. - this.child.stderr?.on('error', (err) => { - if (this.listenerCount('error') > 0) this.emit('error', err); - }); + // stderr carries diagnostics only: a read error on it is reported, to the + // listeners or else in the log, and never thrown. It ends nothing once + // the connection is wired; during a handshake the listener is the + // handshake's, which fails it. + this.child.stderr?.on('error', (err) => this.reportError(err)); this.child.on('exit', (code, signal) => this.markClosed({ code, signal, @@ -347,7 +360,7 @@ export class WebSocketTransport extends McplTransport { } }); - this.ws.on('error', (err: Error) => this.emit('error', err)); + this.ws.on('error', (err: Error) => this.reportError(err)); this.ws.on('close', (code: number, reason: Buffer) => { const text = reason?.toString('utf8'); this.markClosed(text ? { code, reason: text, reasonFromPeer: true } : { code, reason: 'websocket closed' }); diff --git a/test/fixtures/mcpl-ws-upgrade-error-probe.mjs b/test/fixtures/mcpl-ws-upgrade-error-probe.mjs new file mode 100644 index 000000000..e3e722d4e --- /dev/null +++ b/test/fixtures/mcpl-ws-upgrade-error-probe.mjs @@ -0,0 +1,48 @@ +// Runs as a separate process: connects to a WebSocket server that sends a +// frame with a reserved opcode in the same write as its 101 upgrade response, +// before the host has sent initialize. ws raises that frame's 'error' before +// the handshake has attached any listener. Prints "refused after N ms: …" +// when connect() rejects, or "connected"; an 'error' thrown with nothing +// listening would end the process before it prints either. +// Mode "answers-close" (the default): the server answers the client's close +// frame and ends the socket, as most servers do. Mode "silent": it never +// answers, so the connect waits out a timeout. +import { createServer } from 'node:http'; +import { createHash } from 'node:crypto'; +import { McplServerConnection } from '../../src/mcpl/server-connection.js'; + +const mode = process.argv[2] ?? 'answers-close'; +const server = createServer(); +server.on('upgrade', (request, socket) => { + const accept = createHash('sha1') + .update(`${request.headers['sec-websocket-key']}258EAFA5-E914-47DA-95CA-C5AB0DC85B11`) + .digest('base64'); + socket.write(Buffer.concat([ + Buffer.from( + 'HTTP/1.1 101 Switching Protocols\r\nUpgrade: websocket\r\nConnection: Upgrade\r\n' + + `Sec-WebSocket-Accept: ${accept}\r\n\r\n`, + ), + // A frame header with opcode 3, which RFC 6455 reserves. + Buffer.from([0x83, 0x00]), + ])); + socket.on('error', () => {}); + if (mode === 'answers-close') { + // The client's next frame is its close frame: answer it with one, and end. + socket.once('data', () => socket.end(Buffer.from([0x88, 0x00]))); + } +}); +await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + +const started = Date.now(); +try { + const connection = await McplServerConnection.connect( + { id: 'probe', url: `ws://127.0.0.1:${server.address().port}` }, + { version: '0.5' }, + ); + console.log('connected'); + await connection.close(); +} catch (error) { + console.log(`refused after ${Date.now() - started} ms: ${error.message}`); +} +server.closeAllConnections?.(); +server.close(); diff --git a/test/mcpl-transport-error-listeners.test.ts b/test/mcpl-transport-error-listeners.test.ts index ea228c6b6..8707e8723 100644 --- a/test/mcpl-transport-error-listeners.test.ts +++ b/test/mcpl-transport-error-listeners.test.ts @@ -9,11 +9,12 @@ */ import { test, type TestContext } from 'node:test'; import assert from 'node:assert/strict'; +import { EventEmitter } from 'node:events'; import { spawnSync } from 'node:child_process'; import { fileURLToPath } from 'node:url'; import { McplServerConnection } from '../src/mcpl/server-connection.js'; -import { McplTransport } from '../src/mcpl/transport.js'; +import { McplTransport, StdioTransport, WebSocketTransport } from '../src/mcpl/transport.js'; class LineTransport extends McplTransport { constructor() { super(); } @@ -48,11 +49,13 @@ test("with an 'error' listener, a transport error is the listener's, and nothing assert.deepEqual(logged, []); }); -test("an 'error' listener that throws on a transport error is caught and logged", (t) => { +test("an 'error' listener that throws on a transport error is caught and logged, with the error it was handling", (t) => { const { connection, transport, logged, reset } = bareConnection(t); connection.on('error', () => { throw new Error('listener exploded'); }); assert.doesNotThrow(() => transport.emit('error', reset)); - assert.deepEqual(logged, [`[mcpl] srv: an 'error' listener failed: "listener exploded"`]); + assert.deepEqual(logged, [ + `[mcpl] srv: an 'error' listener failed: "listener exploded", handling ${JSON.stringify('MCPL server "srv" transport error: read ECONNRESET')}`, + ]); }); test('a separate process holding a bare WebSocket connection survives a malformed frame, and sees the close', () => { @@ -60,7 +63,52 @@ test('a separate process holding a bare WebSocket connection survives a malforme fileURLToPath(new URL('./fixtures/mcpl-bare-ws-error-probe.mjs', import.meta.url)), ], { encoding: 'utf8', timeout: 60_000 }); assert.equal(probe.error, undefined); - assert.equal(probe.status, 0, probe.stderr); + assert.equal(probe.status, 0, `stdout: ${probe.stdout}\nstderr: ${probe.stderr}`); assert.deepEqual(probe.stdout.trim().split('\n'), ['connected', 'closed', 'survived']); assert.match(probe.stderr, /\[mcpl\] probe: transport error: "Invalid WebSocket frame: invalid opcode 3"/); }); + +// A transport reports its own errors through reportError, which gives them to +// its 'error' listeners, or logs them when it has none: from its creation +// until the handshake attaches a listener, it has none (#391's rule, at the +// transport's own forwarders). +test("a transport with nothing listening for 'error' logs its errors, and gives them to a listener once there is one", (t) => { + const logged: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); + // Stand-ins for a spawned child and a socket, as the stdin test in + // mcpl-request-handler-errors.test.ts uses. + const child = Object.assign(new EventEmitter(), { + stdin: new EventEmitter(), stdout: new EventEmitter(), stderr: new EventEmitter(), killed: false, exitCode: null, signalCode: null, + }); + const stdio = new (StdioTransport as any)(child, new EventEmitter()) as StdioTransport; + const ws = new EventEmitter(); + const socket = new (WebSocketTransport as any)(ws) as WebSocketTransport; + assert.doesNotThrow(() => child.emit('error', new Error('kill EPERM'))); + assert.doesNotThrow(() => child.stderr.emit('error', new Error('read EIO'))); + assert.doesNotThrow(() => ws.emit('error', new Error('Invalid WebSocket frame: invalid opcode 3'))); + assert.deepEqual(logged, [ + '[mcpl] a stdio transport error, with nothing listening for it: "kill EPERM"', + '[mcpl] a stdio transport error, with nothing listening for it: "read EIO"', + '[mcpl] a websocket transport error, with nothing listening for it: "Invalid WebSocket frame: invalid opcode 3"', + ]); + const reported: string[] = []; + stdio.on('error', (error: Error) => reported.push(error.message)); + socket.on('error', (error: Error) => reported.push(error.message)); + child.emit('error', new Error('kill EPERM')); + child.stderr.emit('error', new Error('read EIO')); + ws.emit('error', new Error('read ECONNRESET')); + assert.deepEqual(reported, ['kill EPERM', 'read EIO', 'read ECONNRESET']); + assert.equal(logged.length, 3, 'nothing more is logged once a listener has them'); +}); + +test('a WebSocket server that sends a malformed frame with its upgrade response is refused, and the connecting process survives', () => { + // The server's frame arrives with the 101, and ws raises its error before + // the handshake has attached a listener to the new transport. + const probe = spawnSync(process.execPath, [ + fileURLToPath(new URL('./fixtures/mcpl-ws-upgrade-error-probe.mjs', import.meta.url)), + ], { encoding: 'utf8', timeout: 60_000 }); + assert.equal(probe.error, undefined); + assert.equal(probe.status, 0, `stdout: ${probe.stdout}\nstderr: ${probe.stderr}`); + assert.match(probe.stdout, /^refused after \d+ ms: MCPL server "probe" transport closed before handshake completed/m); + assert.match(probe.stderr, /\[mcpl\] a websocket transport error, with nothing listening for it: "Invalid WebSocket frame: invalid opcode 3"/); +}); From 46084eb775c74eaff9d16e39692de35a6306446e Mon Sep 17 00:00:00 2001 From: Tabitha-2696 Date: Fri, 16 Oct 2026 16:47:48 -0400 Subject: [PATCH 3/5] docs(changelog): #391's entry names the malformed frame at upgrade, which ended framework hosts The transport-level fix reaches further than #391's bare connection: a WebSocket server that sent a malformed frame with its 101 ended any host connecting to it. The fragment now opens with that, and says a throwing 'error' listener is logged with the error it was handling. Co-Authored-By: Claude Opus 5.5 --- .../391-transport-error-no-listener.fixed.md | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/changelog.d/391-transport-error-no-listener.fixed.md b/changelog.d/391-transport-error-no-listener.fixed.md index e249116dd..63542c00a 100644 --- a/changelog.d/391-transport-error-no-listener.fixed.md +++ b/changelog.d/391-transport-error-no-listener.fixed.md @@ -1,9 +1,13 @@ +- **A WebSocket MCPL server that sends a malformed frame with its upgrade + response no longer ends the host** (#391). ws raised the frame's error + before the handshake had attached a listener to the new transport, and + the transport passed it on with nothing listening, which EventEmitter + throws from an I/O callback. Such an error is now logged + (`[mcpl] a websocket transport error, with nothing listening for it: …`), + and the connect is refused. - **A transport error no longer ends a process whose `McplServerConnection` - has no `'error'` listener** (#391). The connection passed each transport - error (a WebSocket protocol error, say) on as its own `'error'`, which - EventEmitter throws when nothing listens, from inside the transport's - callback, where nothing caught it. The framework always listens, so hosts - weren't affected; a probe, a playtest or a harness holding a bare connection - was. With no listener the error is now logged + has no `'error'` listener** (#391). The framework always listens, so hosts + weren't affected, but a probe, a playtest or a harness holding a bare + connection was. With no listener the error is now logged (`[mcpl] : transport error: …`), and a listener that throws is - caught and logged. + caught and logged with the error it was handling. From b2cff98a798377f99e2d7bdebe9a0f6ce299bad8 Mon Sep 17 00:00:00 2001 From: Tabitha-2696 Date: Sat, 17 Oct 2026 00:52:39 -0400 Subject: [PATCH 4/5] fix(mcpl): a transport error logged with nothing listening names its server (#391) Our internal review of the transport-level fix found these, each fixed here. reportError's line named only the transport's kind, so with several MCPL servers an operator couldn't tell which one had sent the malformed frame, and in the window before the handshake listens that line is the only record of it. Each transport now keeps its server's id from the config, and the line reads `[mcpl] : a transport error, with nothing listening for it: `. The stdio and WebSocket transports always give the id; a test's stand-in transport may leave it out, and its line then names no server. The changelog's second entry said hosts weren't affected, beside a first entry about how they were. It now says a connection the framework wires always has a listener. The first entry says when the connect is refused: at once when the server closes the connection, as a WebSocket server does after answering ws's close frame, and after about 30 s when it never does. The upgrade probe's comment says what ends the wait: ws reads nothing more once it has failed the connection, so it's the socket's end, not the server's answering frame. Tests: the stand-in transports and the upgrade process assert the line with the server's id. Co-Authored-By: Claude Opus 5.5 --- .../391-transport-error-no-listener.fixed.md | 12 ++++++----- src/mcpl/transport.ts | 21 ++++++++++++------- test/fixtures/mcpl-ws-upgrade-error-probe.mjs | 6 ++++-- test/mcpl-transport-error-listeners.test.ts | 12 +++++------ 4 files changed, 31 insertions(+), 20 deletions(-) diff --git a/changelog.d/391-transport-error-no-listener.fixed.md b/changelog.d/391-transport-error-no-listener.fixed.md index 63542c00a..92bb1d358 100644 --- a/changelog.d/391-transport-error-no-listener.fixed.md +++ b/changelog.d/391-transport-error-no-listener.fixed.md @@ -3,11 +3,13 @@ before the handshake had attached a listener to the new transport, and the transport passed it on with nothing listening, which EventEmitter throws from an I/O callback. Such an error is now logged - (`[mcpl] a websocket transport error, with nothing listening for it: …`), - and the connect is refused. + (`[mcpl] : a websocket transport error, with nothing listening for + it: …`), and the connect is refused: at once when the server then closes + the connection, as a WebSocket server does after answering the close frame + ws sends, and after about 30 s when it never does. - **A transport error no longer ends a process whose `McplServerConnection` - has no `'error'` listener** (#391). The framework always listens, so hosts - weren't affected, but a probe, a playtest or a harness holding a bare - connection was. With no listener the error is now logged + has no `'error'` listener** (#391). A connection the framework wires always + has one, but a probe, a playtest or a harness holding a bare connection + often doesn't. With no listener the error is now logged (`[mcpl] : transport error: …`), and a listener that throws is caught and logged with the error it was handling. diff --git a/src/mcpl/transport.ts b/src/mcpl/transport.ts index d13dd7ada..22d699ad5 100644 --- a/src/mcpl/transport.ts +++ b/src/mcpl/transport.ts @@ -56,7 +56,10 @@ export abstract class McplTransport extends EventEmitter { private lineBuffer: string[] = []; private stderrBuffer: string[] = []; - protected constructor() { + /** The server's id from its config, so a line the transport logs names it. + * The stdio and WebSocket transports always give it; a test's stand-in + * transport may leave it out. */ + protected constructor(protected readonly serverId?: string) { super(); // Flush buffered inbound lines the moment a consumer subscribes. this.on('newListener', (event) => { @@ -109,7 +112,10 @@ export abstract class McplTransport extends EventEmitter { */ protected reportError(err: Error): void { if (this.listenerCount('error') > 0) this.emit('error', err); - else console.error(`[mcpl] a ${this.kind} transport error, with nothing listening for it: ${logDetail(err.message)}`); + else { + const server = this.serverId === undefined ? '' : `${this.serverId}: `; + console.error(`[mcpl] ${server}a ${this.kind} transport error, with nothing listening for it: ${logDetail(err.message)}`); + } } /** Emit 'close' exactly once. */ @@ -215,8 +221,9 @@ export class StdioTransport extends McplTransport { private constructor( private readonly child: ChildProcess, private readonly rl: ReadlineInterface, + serverId: string, ) { - super(); + super(serverId); this.rl.on('line', (line: string) => this.pushLine(line)); @@ -308,7 +315,7 @@ export class StdioTransport extends McplTransport { }); }); const rl = createInterface({ input: child.stdout! }); - return new StdioTransport(child, rl); + return new StdioTransport(child, rl, config.id); } writeLine(json: string): void { @@ -347,8 +354,8 @@ const WS_OPEN_TIMEOUT_MS = 15_000; export class WebSocketTransport extends McplTransport { readonly kind = 'websocket' as const; - private constructor(private readonly ws: WebSocket) { - super(); + private constructor(private readonly ws: WebSocket, serverId: string) { + super(serverId); this.ws.on('message', (data: WebSocket.RawData, isBinary: boolean) => { // MCPL frames are UTF-8 JSON text. A frame MAY contain multiple @@ -419,7 +426,7 @@ export class WebSocketTransport extends McplTransport { settled = true; clearTimeout(timer); ws.removeAllListeners('error'); // the instance re-attaches its own handlers - resolve(new WebSocketTransport(ws)); + resolve(new WebSocketTransport(ws, config.id)); }); ws.once('error', (err: Error) => { diff --git a/test/fixtures/mcpl-ws-upgrade-error-probe.mjs b/test/fixtures/mcpl-ws-upgrade-error-probe.mjs index e3e722d4e..d27679953 100644 --- a/test/fixtures/mcpl-ws-upgrade-error-probe.mjs +++ b/test/fixtures/mcpl-ws-upgrade-error-probe.mjs @@ -5,8 +5,10 @@ // when connect() rejects, or "connected"; an 'error' thrown with nothing // listening would end the process before it prints either. // Mode "answers-close" (the default): the server answers the client's close -// frame and ends the socket, as most servers do. Mode "silent": it never -// answers, so the connect waits out a timeout. +// frame and ends the socket, as most servers do. ws reads nothing more once it +// has failed the connection, so it's the socket's end, not the answering +// frame, that ends the wait. Mode "silent": it never answers or ends, so the +// connect waits out a 30 s timeout, ws's close timeout or the handshake's own. import { createServer } from 'node:http'; import { createHash } from 'node:crypto'; import { McplServerConnection } from '../../src/mcpl/server-connection.js'; diff --git a/test/mcpl-transport-error-listeners.test.ts b/test/mcpl-transport-error-listeners.test.ts index 8707e8723..8fc8cde0f 100644 --- a/test/mcpl-transport-error-listeners.test.ts +++ b/test/mcpl-transport-error-listeners.test.ts @@ -80,16 +80,16 @@ test("a transport with nothing listening for 'error' logs its errors, and gives const child = Object.assign(new EventEmitter(), { stdin: new EventEmitter(), stdout: new EventEmitter(), stderr: new EventEmitter(), killed: false, exitCode: null, signalCode: null, }); - const stdio = new (StdioTransport as any)(child, new EventEmitter()) as StdioTransport; + const stdio = new (StdioTransport as any)(child, new EventEmitter(), 'child-srv') as StdioTransport; const ws = new EventEmitter(); - const socket = new (WebSocketTransport as any)(ws) as WebSocketTransport; + const socket = new (WebSocketTransport as any)(ws, 'ws-srv') as WebSocketTransport; assert.doesNotThrow(() => child.emit('error', new Error('kill EPERM'))); assert.doesNotThrow(() => child.stderr.emit('error', new Error('read EIO'))); assert.doesNotThrow(() => ws.emit('error', new Error('Invalid WebSocket frame: invalid opcode 3'))); assert.deepEqual(logged, [ - '[mcpl] a stdio transport error, with nothing listening for it: "kill EPERM"', - '[mcpl] a stdio transport error, with nothing listening for it: "read EIO"', - '[mcpl] a websocket transport error, with nothing listening for it: "Invalid WebSocket frame: invalid opcode 3"', + '[mcpl] child-srv: a stdio transport error, with nothing listening for it: "kill EPERM"', + '[mcpl] child-srv: a stdio transport error, with nothing listening for it: "read EIO"', + '[mcpl] ws-srv: a websocket transport error, with nothing listening for it: "Invalid WebSocket frame: invalid opcode 3"', ]); const reported: string[] = []; stdio.on('error', (error: Error) => reported.push(error.message)); @@ -110,5 +110,5 @@ test('a WebSocket server that sends a malformed frame with its upgrade response assert.equal(probe.error, undefined); assert.equal(probe.status, 0, `stdout: ${probe.stdout}\nstderr: ${probe.stderr}`); assert.match(probe.stdout, /^refused after \d+ ms: MCPL server "probe" transport closed before handshake completed/m); - assert.match(probe.stderr, /\[mcpl\] a websocket transport error, with nothing listening for it: "Invalid WebSocket frame: invalid opcode 3"/); + assert.match(probe.stderr, /\[mcpl\] probe: a websocket transport error, with nothing listening for it: "Invalid WebSocket frame: invalid opcode 3"/); }); From ce33bfb09e802e1c4d5631c7fcbcee4b5550f075 Mon Sep 17 00:00:00 2001 From: Tabitha-2696 Date: Sun, 18 Oct 2026 05:09:06 -0400 Subject: [PATCH 5/5] test(mcpl): the bare-connection process test goes, since every removal it caught another test catches (#391) The test spawned a process holding a bare WebSocket connection, from the fixture mcpl-bare-ws-error-probe.mjs, and checked that it survived a malformed frame and saw the close. Each removal of this change's pieces that failed it also failed one of the in-process tests of a connection or transport with no 'error' listener, so it caught nothing they don't, for a process and a fixture of its own. The test and the fixture go. The upgrade probe's process test stays, since it runs ws's own order of events at upgrade, which the fix is for. Co-Authored-By: Claude Opus 5.5 --- test/fixtures/mcpl-bare-ws-error-probe.mjs | 37 --------------------- test/mcpl-transport-error-listeners.test.ts | 10 ------ 2 files changed, 47 deletions(-) delete mode 100644 test/fixtures/mcpl-bare-ws-error-probe.mjs diff --git a/test/fixtures/mcpl-bare-ws-error-probe.mjs b/test/fixtures/mcpl-bare-ws-error-probe.mjs deleted file mode 100644 index 1161ec27e..000000000 --- a/test/fixtures/mcpl-bare-ws-error-probe.mjs +++ /dev/null @@ -1,37 +0,0 @@ -// Runs as a separate process holding a bare McplServerConnection over -// WebSocket, with no 'error' listener, as a probe or a playtest would. The -// server completes the handshake, then sends a frame with a reserved opcode, -// which the ws client reports as an 'error' and answers by closing. An error -// thrown with nothing listening would end this process before it prints -// "survived". -import { WebSocketServer } from 'ws'; -import { McplServerConnection } from '../../src/mcpl/server-connection.js'; - -const wss = new WebSocketServer({ host: '127.0.0.1', port: 0 }); -await new Promise((resolve) => wss.once('listening', resolve)); -wss.on('connection', (ws) => { - ws.on('message', (data) => { - const message = JSON.parse(String(data)); - if (message.method === 'initialize') { - ws.send(JSON.stringify({ jsonrpc: '2.0', id: message.id, result: { protocolVersion: '2024-11-05', capabilities: {} } })); - } else if (message.method === 'notifications/initialized') { - // A frame header with opcode 3, which RFC 6455 reserves. - ws._socket.write(Buffer.from([0x83, 0x00])); - } - }); -}); - -const connection = await McplServerConnection.connect( - { id: 'probe', url: `ws://127.0.0.1:${wss.address().port}` }, - { version: '0.5' }, -); -const closed = new Promise((resolve) => connection.once('close', resolve)); -connection.ready(); -console.log('connected'); -const timer = setTimeout(() => { console.log('no close within 30 s'); process.exit(1); }, 30_000); -await closed; -clearTimeout(timer); -console.log('closed'); -console.log('survived'); -await connection.close(); -wss.close(); diff --git a/test/mcpl-transport-error-listeners.test.ts b/test/mcpl-transport-error-listeners.test.ts index 8fc8cde0f..7c54e068a 100644 --- a/test/mcpl-transport-error-listeners.test.ts +++ b/test/mcpl-transport-error-listeners.test.ts @@ -58,16 +58,6 @@ test("an 'error' listener that throws on a transport error is caught and logged, ]); }); -test('a separate process holding a bare WebSocket connection survives a malformed frame, and sees the close', () => { - const probe = spawnSync(process.execPath, [ - fileURLToPath(new URL('./fixtures/mcpl-bare-ws-error-probe.mjs', import.meta.url)), - ], { encoding: 'utf8', timeout: 60_000 }); - assert.equal(probe.error, undefined); - assert.equal(probe.status, 0, `stdout: ${probe.stdout}\nstderr: ${probe.stderr}`); - assert.deepEqual(probe.stdout.trim().split('\n'), ['connected', 'closed', 'survived']); - assert.match(probe.stderr, /\[mcpl\] probe: transport error: "Invalid WebSocket frame: invalid opcode 3"/); -}); - // A transport reports its own errors through reportError, which gives them to // its 'error' listeners, or logs them when it has none: from its creation // until the handshake attaches a listener, it has none (#391's rule, at the