[Bug] Late ECONNRESET after an upgraded socket closes can terminate dsh web

[johnymoo]

Summary

@deepseek-ai/dsh-host-webserver@0.1.1-rc.2 removes its upgraded socket error listener in the socket's close callback. A transport error delivered after that callback, such as a late ECONNRESET during a browser reload or tunnel disconnect, then has no listener and is thrown as an unhandled EventEmitter error. This can terminate the entire dsh web process.

I found this while testing an unofficial community plugin, but the reproduction below uses only the official published dsh-host-webserver package and @deepseek-ai/cordis.

This appears to conflict with the package README contract: an upgraded-socket transport error should be logged and its socket destroyed, and should never exit the process.

Environment

• @deepseek-ai/dsh-host-webserver: 0.1.1-rc.2 (current npm release)
• Also source-compared with 0.1.0-rc.8; the relevant lifecycle is unchanged
• @deepseek-ai/cordis: 4.0.1
• Node.js: v25.4.0
• macOS arm64

Relevant lifecycle

The upgrade handler installs onError, then removes it on close:

socket.on('error', onError)
socket.once('close', () => {
  socket.off('error', onError)
  this.upgradedSockets.delete(socket)
})

Node may still deliver a queued transport error after the close callback has run.

Minimal reproduction

Install the two published packages in an empty directory, then run:

import { createConnection } from 'node:net'
import { Context, Service } from '@deepseek-ai/cordis'
import WebServer from '@deepseek-ai/dsh-host-webserver'

const ctx = new Context()
const webServer = new WebServer(ctx, { host: '127.0.0.1', port: 0 })
let upgraded

webServer.registerUpgrade({
  path: '/probe',
  handler: (_request, socket) => { upgraded = socket },
})

await webServer[Service.init]()
const client = createConnection({ host: '127.0.0.1', port: webServer.port })
await new Promise((resolve, reject) => {
  client.once('connect', resolve)
  client.once('error', reject)
})

client.write(
  'GET /probe HTTP/1.1\r\n' +
  'Host: localhost\r\n' +
  'Connection: Upgrade\r\n' +
  'Upgrade: websocket\r\n\r\n',
)

while (upgraded === undefined) {
  await new Promise(resolve => setTimeout(resolve, 5))
}

const closed = new Promise(resolve => upgraded.once('close', resolve))
upgraded.destroy()
await closed

console.log('late error listeners:', upgraded.listenerCount('error'))
upgraded.emit(
  'error',
  Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }),
)

Observed:

late error listeners: 0
Error: read ECONNRESET
Emitted 'error' event on Socket instance

The same probe leaves one listener and does not throw when a connection-lifetime compatibility listener is added before the upgrade.

Expected behavior

A late transport error on a closed upgraded socket must not become an uncaught exception or terminate the Harness process. The server should remain available for subsequent HTTP and WebSocket connections.

Suggested acceptance criteria

• Closing an upgraded socket and then delivering a late ECONNRESET does not throw.
• The Harness process remains alive and the web server continues serving requests.
• Existing logging and socket-destruction behavior for errors delivered before close remains intact.
• Add a focused regression test under packages/host/webserver/tests/webserver.spec.ts.

A possible implementation is to retain an error listener for the socket's full object lifetime, or replace the logging handler with a no-op listener after close. The exact ownership choice is best made by the package maintainers.

Related failure class: #3251 reports another process-fatal transport gap, but it concerns SDK stdout EPIPE; this report is specifically the WebServer upgraded-socket close race.

[argszero]

Confirmed against v0.1.1-rc.2 source — your diagnosis is exact, and I traced the asymmetry that makes this a one-line bug rather than a design decision.

Source facts (packages/host/webserver/src/index.ts)

• The upgrade handler attaches onError (lines 197-201), removes it in the close callback (lines 202-204), and deletes the socket from upgradedSockets. The removal was introduced by commit 7f3a2dae91 "fix(web): quiesce websocket teardown" (Aug 4) — the quiescence intent is about teardown ordering (stop signalling after close), and the socket.off('error', onError) is collateral cleanup that creates the unhandled-error window.
• Node's EventEmitter special-cases the error event: a delivered error event with zero listeners throws and, absent a handler on the process, terminates it. Your repro (listener count zero after close, then emit) is exactly that path.
• The asymmetry: the server-level error listener at line 235 is attached with server.on('error', ...) and is never removed — only the per-socket upgraded handler is detached. So the codebase already has the "permanent error listener" pattern; the upgrade path deviates from it.

README contract (packages/host/webserver/README.md line 9) states verbatim: "An upgrade-handler exception or upgraded-socket transport error is logged as a warning and destroys its socket" — the observed behavior (process exit) violates it.

Fix (2 lines, minimal)

• Drop the socket.off('error', onError) from the close callback, and make onError idempotent for an already-closed socket: guard the warn with socket.destroyed, and keep the unconditional socket.destroy() call — destroy() on a closed socket is a documented no-op. The guard only suppresses a duplicate warn for late errors. Keeping the listener satisfies the README contract (logged and destroyed) and matches the server-level pattern at line 235. Keep this.upgradedSockets.delete(socket) — that is the teardown bookkeeping that matters.
• Alternative if you want to keep the detach: replace the removed listener with a permanent no-op error sink (attach an empty error handler) — equivalent outcome, slightly less diagnostic fidelity.

Regression test: your repro is already a test — after close, assert the upgraded socket still has at least one error listener, then emit an error on it: it must not throw and must not reach an uncaught-exception handler. Add a second case for the double-error path (error after destroy) to assert no warn spam.

Family: teardown-timing cleanup overreach — the same shape as #4055 (client-hmr SSE upgrade channel) and the earlier quiesce work: a "stop acting on the thing" cleanup that accidentally removes the safety net. Worth a sweep for other off('error')-on-close patterns across packages/host (grep for socket.off with the error event name inside a close callback).

Note for maintainers: commit 7f3a2dae91 is on the rc.2 line, so this ships in the current release — a small follow-up fix commit would cover it.