Repository navigation
Conversation
…ispatch and TLS listener stamps Pins, per caller code shape, what application code using the `server` global observes from two listener changes: v5.1.26's upgrade dispatch on the per-worker UDS mirror (and the `udsMirror` reference on the port's server object), and v5.2.0's `appliedCiphers` / `verifiesClientCerts` / `mtlsRequired` / `udsMirrorUwsConfig` stamps on TLS listeners. Only server.ws / server.upgrade registrations (deployment-dependent) and code that reads or enumerates the objects server.http / server.ws / server.socket return can observe a difference; the global itself and the fastifyRoutes hand-over are unchanged. httpChainPortAll.test.js registered an http-shaped (request, next) handler as server.upgrade middleware on the 'all' pseudo-port, which throws on any real upgrade a later file dispatches in the same mocha process; it now registers an upgrade-shaped pass-through. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CjJe3sGr1tdJmvjvVZx77N Dispatch-Task: harper-upgrade-rule-global-server-5126-520
…caller-impact tests Addresses pre-push review round 1: snapshot and restore SERVERS, uwsServeConfigs and portServer so a later listenOnPorts() test does not bind this file's listeners; give the WebSocket, listener and fetch waits deadlines; drop sinon for counters and a restored receiver; pin that upgrade middleware never runs on the uWS-served mirror; attribute the uWS mirror cases to v5.2.0 (v5.1.x has no uWS backend); assert the TLS port's own 'upgrade' listener alongside the mirror's; trim comments. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CjJe3sGr1tdJmvjvVZx77N Dispatch-Task: harper-upgrade-rule-global-server-5126-520
…gMessage override, not Node server/DESIGN.md attributed the silent destroy of an un-listened WebSocket upgrade to a Node default. Plain Node (v20-v26) hands such a request to the 'request' listener; Harper's process-wide IncomingMessage.prototype .upgrade override with a no-op setter defeats that fallback, which also makes it apply to application-created http.Servers. Also detaches listenOnLoopback's error listener once listen succeeds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CjJe3sGr1tdJmvjvVZx77N Dispatch-Task: harper-upgrade-rule-global-server-5126-520
…rver/http.ts Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CjJe3sGr1tdJmvjvVZx77N Dispatch-Task: harper-upgrade-rule-global-server-5126-520
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the design documentation regarding WebSocket upgrade fallback behavior and introduces a new test suite in unitTests/server/serverGlobalCallerImpact.test.js to verify UDS mirror upgrade dispatches and TLS listener stamps. It also refactors existing upgrade chain tests to use a dedicated pass-through helper. The review feedback suggests awaiting the asynchronous server closures in the test teardown hook to prevent Mocha from hanging, and explicitly requesting connection closure in the test fetch request to avoid keeping sockets alive.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
⊙ Problem
Harper's upgrade-impact pipeline flags apps as "potentially affected" by a release step through a static pattern match on the API they use. Over a thousand apps reference the injected
serverglobal, nearly all throughserver.http/server.ws. Two release steps changed the listener objects behind that global: v5.1.25 → v5.1.26 (the per-worker UDS mirror now dispatches WebSocket upgrades, and the port's server object carries audsMirrorreference) and v5.1.26 → v5.2.0 (TLS listeners resolve ciphers per usage type and carryappliedCiphers,verifiesClientCerts,mtlsRequired,udsMirrorUwsConfig). Without a precise rule, every one of those apps gets flagged. This PR pins each caller shape with a test on main and states the rule below. It changes no runtime behavior.Caller impact rule
(a) Trigger conditions
Both changes act on the internal Node listener objects (
http.Server,tls.Server) stored inSERVERS. Neither adds, removes or changes a key of theserverglobal itself. Application code observes them through exactly two routes:Mirror upgrade dispatch (v5.1.26). A
server.ws(...)registration (and anyserver.upgrade(...)middleware on the same port) observes a difference only when all of these hold:securePort, or no port options whilehttp.securePortis configured (the default forserver.ws(listener)with no options);tls.unixDomainSocketsis enabled, so the per-worker UDS mirror exists (getHTTPServer);Before v5.1.26 such handshakes got a zero-byte close and the listener never ran. Now they reach the listener, and any
server.upgrademiddleware on the port runs first (onWebSocketdispatch). v5.1.x has no uWS backend. v5.2.0 first ships the opt-in uWS-served mirror (HARPER_UWS_UDS). On that mirrorserver.wslisteners run but upgrade middleware never does; Harper only logs a warning.New properties on returned listener objects (v5.1.26
udsMirror; v5.2.0 the rest). Code observes them only if it keeps the return value ofserver.http(...),server.ws(...)(both documented asHttpServer[]) orserver.socket(...)(documented asSocketServer) and then reads one of the new property names, enumerates the object (Object.keys,for..in, spread,Object.assign), or serializes it (JSON.stringify,util.inspect). The properties are set only on secure listeners (server/http.ts:929-931,threadServer.js:776-778);udsMirrorandudsMirrorUwsConfigadditionally needtls.unixDomainSockets(andHARPER_UWS_UDSfor the latter).The cipher resolution itself (which suite string a TLS listener is built with) depends on TLS configuration and certificate records, not on application code.
server.socket(listener, { securePort, usageType })is the one code-visible input: before v5.2.0 theusageTypeoption was ignored on raw-socket listeners; since #2003 it selects certificates and ciphers. Existing tests pin that:unitTests/security/keys.test.js"applies the operationsApi.tls layer only when usageType is 'operations-api'" and the two tests after it.(b) Caller shapes
Every row names the test in
serverGlobalCallerImpact.test.jsthat pins it.server.wson a secure portserver.ws((ws) => ws.send('hi'))tls.unixDomainSocketsand mirror-routed clients): mirror connections now reach the listenerserver.upgrademiddleware withserver.wson the same secure portserver.upgrade((req, sock, head, next) => { authorize(req); return next(req, sock, head); })server.ws+server.upgradeon a uWS-served mirror (HARPER_UWS_UDS)HARPER_UWS_UDS=1server.wslisteners run on that mirror; upgrade middleware does not (uWS accepts the handshake natively; only a warning is logged)server.upgradealoneserver.upgrade(mw, { securePort: 9443 })server.http+server.upgrade, noserver.wson that portserver.http(h, { securePort }); server.upgrade(mw, { securePort })'upgrade'listener; handshakes still get a zero-byte close)server.wson a plain port onlyserver.ws(l, { port: 9926 })server.http(listener)request handlingserver.http((req, next) => next(req))const [s] = server.http(h, { securePort }); if (s.udsMirror) …udsMirrorwasundefinedappliedCiphers,verifiesClientCerts,mtlsRequiredwereundefinedObject.keys(server.http(h, opts)[0]),JSON.stringify(s)udsMirrorUwsConfiginstead ofudsMirrorunder the newHARPER_UWS_UDSbackend)server.ws/server.httpreturn identityserver.ws(l, o)[0] === server.http(h, o)[0]server.socketreturn,securePortonlyconst s = server.socket(l, { securePort: 8883 })server.socketreturn,port+securePortserver.socket(l, { port: 1883, securePort: 8883 })server.socketwithusageTypeserver.socket(l, { securePort, usageType: 'mqtt' })keys.test.jsusageType testsserver.resources,server.contentTypes,server.httpObject.keys(server)setMcpQuotaHandlerfrom an unrelated MCP feature)routes/*.jsexportingasync (fastify, { hdbCore }) => …request._nodeRequest.socket.server.mtlsRequiredverifiesClientCerts/mtlsRequiredNo row breaks. Every difference is additive: a property that used to be
undefined, or a connection that used to be dropped and is now served.The fastify hand-over did not change. v5.1.26 had
server.http((await fastifyServer).server); v5.2.0 and main haveconst built = await fastifyServer; server.http(built.server)(fastifyRoutes.ts:62-64). Both await the build before the hand-over, and both hand over before fastify'sready(). What v5.2.0 did change for fastifyRoutes apps is outside theserverglobal: a component mounted on ahostnow refuses to load, the route prefix is computed from the external base path, and the instance is registered per port for the Bun/uWS fallback.(c) Intended contract change or accidental regression
Intended fixes; the property exposure is an incidental side effect of returning the internal objects.
Fixes #2013and adds mirror tests inunitTests/server/udsMirror.test.jsandunitTests/apiTests/mqtt-test.mjs.ciphers/SECLEVELfrom every configured source when building TLS listeners (#1841) (closes Per-certificate TLSciphers(including client-CASECLEVELoverrides) silently ignored — onlytls.ciphers/tls[0].cipherstakes effect #1840, addskeys.test.jscases andintegrationTests/security/tls-ciphers-seclevel.test.ts) addedappliedCiphers/verifiesClientCerts, read by the post-boot restart warning (security/keys.ts:1501). Decode PROXY protocol v2 with forwarded mTLS client certificates on UDS mirrors (#1858) addedmtlsRequired, read by the UDS metadata writer (server/http.ts:233). #2010 moved the raw-socket stamps onto the TLS server. All of them are internal bookkeeping read by Harper itself; none is documented in the reference docs.Nothing here is an accidental break that Harper should fix, so the task's "stop and ask before a fix" condition did not arise.
(d) Static-detection recipe
Neither change can be decided from source for the registration shapes, so a scanner has two jobs.
\bserver\.(ws|upgrade)\s*\(. Then downgrade to unaffected when the call's options literal namesport:and notsecurePort:(single-line heuristic:\bserver\.ws\s*\([^)]*\{[^}]*\bport\s*:(?![^}]*securePort)).tls.unixDomainSockets, or whose clients connect to the TLS port directly. That is most of them, and the code cannot tell.const { ws } = server; ws(l)) needs\{[^}]*\bws\b[^}]*\}\s*=\s*server\bas well.\.(udsMirror|udsMirrorUwsConfig|appliedCiphers|verifiesClientCerts|mtlsRequired)\b, or a captured return value that is enumerated or serialized:=\s*server\.(http|ws|socket)\s*\(in the same file asObject\.(keys|entries|values|assign)\(|JSON\.stringify\(|for\s*\(\s*(const|let|var)\s+\w+\s+in\b|\.\.\.\w.mtlsRequiredcollides with config-shaped objects ({ mtlsRequired: true }); the enumeration co-occurrence is file-level, not data-flow.request._nodeRequest.socket.serverorws._socket.server.server.http(listener)handlers,server.resources,server.contentTypes,server.operation, other reads of the global, and fastifyRoutes route files.(e) Main versus the target releases
Main behaves as v5.2.0 for every shape above. The mirror dispatch (
server/http.ts:1903) is present on v5.1.26 through v5.1.28, v5.2.0 through v5.2.15, v5.3.2 and main. The stamps are present on v5.2.0 through v5.2.15, v5.3.2 and main, and on no v5.1.x tag. Unrelated to these two changes, main (since v5.3.0, #3035) letsserver.http/server.ws/server.socketthrow when a port is owned by a dedicated worker pool (threadType). This rule does not cover that.💡 Solution
One new unit file,
unitTests/server/serverGlobalCallerImpact.test.js, drives the realserver.http/server.ws/server.upgrade/server.socketwithtls.unixDomainSocketstoggled. It listens the resulting mirror objects on loopback TCP, so it runs on every CI leg including Windows, and exercises real WebSocket and HTTP traffic. Eachitis named for one caller shape in the table, and old behavior is stated in a comment naming the release and PR that changed it. Teardown restoresSERVERS,uwsServeConfigsandportServer, so a laterlistenOnPorts()test does not bind these listeners. The uWS-mirror cases call the installedwsHandlerdirectly; they do not exercise uWS's native upgrade callback.server/DESIGN.mdsaid a Node HTTP server with no'upgrade'listener destroys upgrade sockets. That is only true inside Harper: the note now names the override as the cause and says it applies to everyhttp.Serveron a thread that loadsserver/http.ts, including ones an application creates (checked with a standalone server plus the override on Node v22, v24, v26: 0 bytes, closed).unitTests/server/httpChainPortAll.test.jsregistered an http-shaped(request, next)handler asserver.upgrademiddleware on the'all'pseudo-port. That throwsnext is not a functionon any real upgrade a later file dispatches in the same mocha process, and it broke the new end-to-end WebSocket tests in a full-directory run. It now registers an upgrade-shaped pass-through; what that file asserts is unchanged.⚖️ Alternatives
unitTests/apiTests/mqtt-test.mjsinstead of a new file. That block already pins the Node and uWS mirror WebSocket paths over a real socket path with a PROXY v1 header. It runs intest:unit:apitestsand is organized around MQTT, not caller shapes. A new file in the default unit suite, with oneitper rule row, keeps the rule and its tests side by side, at the cost of a less faithful transport (loopback TCP, no PROXY header) and some overlap.✅ Verification
End-to-end route: real WebSocket and HTTP traffic over the mirror listener inside the unit test, no integration test needed.
npx mocha unitTests/server/serverGlobalCallerImpact.test.js→ 17 passing; withthreadServerListenOnPorts.test.jsafter it, 25 passing (the registries this file touches are restored before that file'slistenOnPorts()).dist/: removing the mirror'upgrade'dispatch and theudsMirrorassignment (the v5.1.25 state) fails 4 of 17. Removing the three v5.2.0 stamp assignments ingetHTTPServerfails 2 of 17. Restored, all passing.npx mocha "unitTests/server/**/*.js"→ before thehttpChainPortAllfix, 3 failures in the new file from the'all'upgrade handler; at the head, 1331 passing, 0 failing.npm run test:unit:mainon the first commit → 6906 passing, 0 failing (the head's later changes are confined to the new file and re-verified by theunitTests/serverrun above).prettier --checkandoxlint --deny-warningsclean on both files.🤖 Generated with Claude Code
https://claude.ai/code/session_01CjJe3sGr1tdJmvjvVZx77N
— Claude Opus 5.5
Release target: main — tests and a design-note correction only; no runtime change to backport
Related PRs: none found (partial scan: 1 PRs with more than 100 files or 10 linked issues could not be fully matched)
Complexity: medium
Dispatch: task
harper-upgrade-rule-global-server-5126-520· queued by unknown · ran by claude/opus/high · worker kzyp-xps-1Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=4; full=1 @ 0b95566
Review-Attention: read ~3m (decisions: pin-silent-upgrade-close, design-note-vs-named-test, pin-incidental-shapes, new-unit-file-vs-extend-mqtt-test) @ 0b95566