Skip to content

fix(http): WebSocket upgrades were silently dropped on the per-worker UDS mirror listeners - #2015

Merged
kriszyp merged 3 commits into
mainfrom
kris/uds-websocket-upgrade
Jul 31, 2026
Merged

fix(http): WebSocket upgrades were silently dropped on the per-worker UDS mirror listeners#2015
kriszyp merged 3 commits into
mainfrom
kris/uds-websocket-upgrade

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 31, 2026

Copy link
Copy Markdown
Member

Fixes #2013.

What was broken

With tls.unixDomainSockets: true (a fronting proxy terminating TLS and routing to per-worker UDS mirrors), every WebSocket handshake against a mirror was destroyed with a zero-byte close — no HTTP response, no error, nothing in the log. Plain HTTP and SSE worked; only Upgrade died. This took down all WS traffic behind the proxy, including MQTT-over-WSS from browsers. Reproduced on 5.1.22, 5.1.25, and main.

Two independent defects in server/http.ts:

  1. The UDS mirror has no 'upgrade' listener. onWebSocket() attaches the upgrade middleware dispatch via server.on('upgrade', …) on the port-keyed server only. The mirror is a separate http.Server (SERVERS[udsPath]), and a Node HTTP server with zero 'upgrade' listeners destroys upgrade sockets (socket.destroy() in _http_server.js) — exactly the observed silent close. The experimental HARPER_UWS_UDS path had the same gap: onWebSocket() set wsHandler on uwsServeConfigs[port] but never on the mirror's uwsServeConfigs[udsPath].

  2. enableProxyProtocol()'s interception outlives its purpose and corrupts post-upgrade traffic. The wrapper captured the HTTP parser's 'data' listeners at connection time and forwarded to that captured array forever. On upgrade, Node removes its parser listener by reference (a no-op — the wrapper holds it) and ws attaches new listeners, so every inbound WS frame was delivered both to ws and to the freed parser. Verified empirically: once the parser pool re-issues that parser to another connection, frames from the upgraded socket are injected into the other connection's parser (clientError: Parse Error: Data after 'Connection: close') — cross-connection corruption, so fixing (1) alone was not enough.

The fix

  • getHTTPServer() exposes the mirror on the port server (server.udsMirror for Node, server.udsMirrorUwsConfig for HARPER_UWS_UDS); onWebSocket() attaches the same upgrade dispatch to the mirror (chains stay keyed by the mirrored port) and installs the uWS wsHandler on the mirror config. The uWS handler creation is extracted into one helper used by both the port config and the mirror config.
  • enableProxyProtocol() now hands the socket back to its original listeners as soon as the PROXY header decision is made: it removes its wrapper and re-attaches the captured listeners, so all post-header traffic (including protocol handoffs like HTTP upgrade → ws) runs on real listeners with native semantics.
  • Adjacent fix (found by the Codex review leg): registerWsBehavior() passed { maxPayload } to uWS's app.ws(), but uWS's option key is maxPayloadLength — the configured WS payload cap (including the wsMaxPayload this PR propagates) was silently ignored and uWS's 16 KiB default applied. Now passed under the correct key, with a regression test (oversized frame must close the connection; verified fails pre-fix).

Known limitation, now surfaced instead of silent (raised in review): uWS-served transports (HARPER_UWS_HTTP ports, HARPER_UWS_UDS mirrors) accept WS handshakes natively in app.ws(), so custom server.upgrade() middleware cannot run pre-handshake there — this predates this PR on the uWS port path, no core component registers such middleware, and auth is unaffected (it runs in the WS connection chain on both paths, matching Node's upgrade-then-authorize order). onUpgrade()/installUwsWsHandler() now warn when custom upgrade middleware is registered for a uWS-served port. A real middleware bridge for uWS (if ever needed) is follow-up work.

Not affected / out of scope, per analysis requested in the thread: the raw MQTT (non-WS) UDS mirrors (server.socket() path — no HTTP upgrade involved) and the HARPER_H2C_UDS h2c mirror (HTTP/1.1 Upgrade doesn't exist in h2; the fronting proxy routes WS to the h1 mirror by ALPN). websocketChains itself was never stranded — it's invoked through the upgrade dispatch, which was the missing link.

Verification

  • Full stack: booted this branch with tls.unixDomainSockets: true; the original repro (curl --unix-socket … -H Upgrade:websocket) now returns 101 on the mirror (was 000), and an MQTT-over-WebSocket CONNECT through the mirror reaches the broker and gets a CONNACK back.
  • New regression tests (both verified to fail against the pre-fix code with the exact defect signatures):
    • unitTests/server/udsMirror.test.js: real UDS socket + PROXY v1 header + WS handshake + masked-frame echo, with an interleaved HTTP request to force parser-pool reuse (fails pre-fix with Parse Error: Data after 'Connection: close'); plus a listener-restoration unit test on the wrapper.
    • unitTests/apiTests/mqtt-test.mjs: server.ws({securePort}) with tls_unixDomainSockets on — asserts the mirror gets the upgrade dispatch and completes a real handshake + echo through the mirror socket (fails pre-fix with a zero-byte close); plus a HARPER_UWS_UDS test asserting the uWS mirror config receives the wsHandler/wsMaxPayload.
    • unitTests/server/serverHelpers/uwsServer.test.js: oversized-frame close test for the maxPayloadLength fix (fails pre-fix).
  • Suites run locally: test:unit:resources clean (1334 passing); test:unit:main 2904 passing with 2 failures and test:unit:apitests 182 passing with 10 failures — all 12 failures reproduce identically on a clean origin/main baseline in this environment (verified with a stash + rebuild), so they are pre-existing and unrelated; test:integration:all passes.

Notes

  • A v5.1 backport is needed (the affected deployments run 5.1.x, and the team wants a permanent core patch to load-test against rather than infra workarounds). I'll open the cherry-pick PR once this lands; it should ride the same 5.1 patch as Fix MQTT secure-port UDS metadata publishing an empty certificate list (SNI proxy served the node cert on 8883) #2010/[v5.1] Fix MQTT secure-port UDS metadata publishing an empty certificate list #2011.
  • Cross-model review (thorough): Gemini (agy) + Codex legs + Harper-domain adjudication. No blockers, no significant concerns. Gemini's one raised blocker (uWS-backed port leaving the Node mirror unwired) was refuted with code evidence — the HARPER_UWS_HTTP branch requires !secure and returns early, while mirrors are created only for secure ports, so the combination is unreachable. Codex's two real findings (the maxPayloadLength key bug; node: prefixes) are fixed in this PR; its uWS-mirror test-gap suggestion is covered by the added HARPER_UWS_UDS config test. Gemini's socket.unshift alternative was considered and declined: the manual forward preserves the exact pre-existing delivery mechanism the split-header tests pin, and the module's own Node-v24 comment documents that the parser's intake doesn't reliably see stream-level re-emission.
  • Generated by Claude (Fable 5) pairing with Kris.

With tls.unixDomainSockets enabled, the per-worker UDS mirror is a separate
http.Server that never received the 'upgrade' listener onWebSocket() attaches
to the port-keyed server, so Node destroyed every WS handshake on it with a
zero-byte close (no response, no log). The uWS mirror (HARPER_UWS_UDS) had the
same gap for its wsHandler.

enableProxyProtocol()'s data interception also outlived its purpose: it kept
forwarding post-upgrade frames to the captured (freed) HTTP parser, which the
parser pool can re-issue to another connection — verified cross-connection
corruption. The wrapper now removes itself and restores the original listeners
once the PROXY header decision is made.

Also fixes registerWsBehavior passing maxPayload to uWS app.ws(), whose real
option key is maxPayloadLength — the configured WS payload cap was silently
ignored.

Fixes #2013

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Patch cherry-pick: conflict

Cherry-pick onto v5.1 produced conflicts on commit(s): a057bca24cef2420d0cbb2e06c8a5422d7a06208 16a422840f5f7c1ef734fcd731ba79a53e7748fd

The conflict markers are committed on branch cherry-pick/v5.1/pr-2015.
A pull request has been opened to land this patch: #2019

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses critical issues with WebSocket upgrades and proxy protocol handling on Unix Domain Socket (UDS) mirrors. It ensures that WebSocket upgrade listeners and uWS configurations are correctly propagated to per-worker UDS mirror servers, fixes a bug where maxPayload was used instead of maxPayloadLength in uWS configuration, and updates enableProxyProtocol to clean up its wrapper listener after the PROXY header is resolved to prevent data leakage into pooled HTTP parsers. Comprehensive unit and integration tests have been added to verify these fixes. I have no feedback to provide as there are no review comments.

@kriszyp
kriszyp marked this pull request as ready for review July 31, 2026 02:27
@kriszyp
kriszyp requested a review from harper-joseph July 31, 2026 02:28
…istener

uWS accepts WebSocket handshakes natively in app.ws(), so pre-handshake
upgrade middleware cannot run on HARPER_UWS_HTTP ports or HARPER_UWS_UDS
mirrors (auth is unaffected: it runs in the WS connection chain on both
paths). Surface the gap with a warning instead of silently skipping the
middleware; the default handler onWebSocket registers is exempt since
uWS performs its job natively.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kriszyp added a commit that referenced this pull request Jul 31, 2026
…5.1 backport of #2015)

Hand-adapted cherry-pick of 02b4b8a43/16a422840 onto v5.1: the uWS pieces
(HARPER_UWS_UDS mirror wsHandler, maxPayloadLength key, upgrade-middleware
warning) don't exist on this branch and are omitted; the PROXY handoff fix
is grafted onto v5.1's PROXY-v1-only enableProxyProtocol.

The per-worker UDS mirror is a separate http.Server that never received the
'upgrade' listener onWebSocket() attaches to the port-keyed server, so Node
destroyed every WS handshake on it with a zero-byte close. enableProxyProtocol's
data interception also outlived the header decision, forwarding post-upgrade
frames into the freed HTTP parser (re-poolable across connections); the wrapper
now removes itself and restores the original listeners once the PROXY header
resolves.

Fixes #2013 on v5.1

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… finally

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kriszyp
kriszyp merged commit c28e5f8 into main Jul 31, 2026
45 of 46 checks passed
@kriszyp
kriszyp deleted the kris/uds-websocket-upgrade branch July 31, 2026 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WebSocket upgrades are silently dropped on per-worker Unix-domain-socket mirror listeners

2 participants