Fix MQTT secure-port UDS metadata publishing an empty certificate list (SNI proxy served the node cert on 8883)

[kriszyp]

Summary

Fixes #1998's surviving symptom (still broken on 5.1.25 with #1999/#2005). Root cause is not in the TLS selector at all — it's a closure-over-reassigned-variable in onSocket() (server/threads/threadServer.js):

• MQTT registers with both port: 1883 and securePort: 8883 in one server.socket() call (the default config).
• The securePort branch creates the TLS server into the function-scoped let socketServer, and the UDS metadata-write closure captures that binding.
• The plain-TCP branch below then reassigns socketServer to the 1883 server, which has no secureContexts.
• Every secure-port metadata write — the boot-time .ready.then() microtask (which runs after onSocket returns, i.e. after the reassignment) and every later rebuild's listener fan-out — reads secureContexts === undefined off the wrong server and publishes an empty certificates: list.

Symphony selects the SNI certificate from that metadata, so 8883 fell back to the node certificate for every custom-domain SNI — deterministically, on every worker of every node with both MQTT ports enabled, fleet-wide (host-manager's code even carries a comment treating empty MQTT metadata as known behavior).

Fix: the secure server gets its own const secureSocketServer, and the closure captures that; socketServer remains the branch-shared return value. Two-variable change plus comments.

Evidence

• Deterministic local repro: fresh v5.1.25 default-config install — all <n>-8883.yaml 42–50 bytes (empty certificates:) while <n>-9926.yaml fully populated, first boot, every worker. Instrumented dist showed the 8883 selector's pass completing healthy (mapSize=3, defaultSet=true) while its metadata write fired with ctxSize=undefined.
• Fix verified both directions: same instrumented install with the one-line capture → all 8883 yamls fully populated on reboot.
• Live prod corroboration (us-lax-1.prod.ibm, read-only CDP): in-memory 8883 selector maps fully populated (all four customer domains) while disk yamls stayed empty since boot — the selector was never unhealthy; only the write's target object was wrong.

Why four rounds of review missed it

Every selector-level fix and test (#1999, #2005, #2008) drove createTLSSelector directly with a pseudo-server. The bug lives in onSocket's wiring and only manifests when one socket() call registers both ports. The new regression test goes through server.socket() with both ports — the real wiring — and asserts the written yaml carries certificates (the artifact the proxy actually consumes). Bisected: fails against unfixed code at exactly that assertion, passes with the fix.

Relationship to in-flight work

• fix(tls): keep rebuilding the SNI selector when a pass produces only a default context (empty per-hostname map) #2008 (@Devin-Holland): its armed empty-map retry re-runs the same wrong-object write, so it doesn't cure this symptom on its own; it may still have value as defense-in-depth for genuinely-transient empty passes. See my comment there.
• host-manager allow role to specify access to specific operations #170: remains a valid immediate mitigation independent of this fix.
• v5.1 backport needed for IBM (they run 5.1.x) — patch label applied; the touched region is near-identical on v5.1 (modulo the ciphers line), so the cherry-pick should be clean or trivially resolvable. I'll follow the auto-cherry-pick and hand-resolve if needed.

Testing

• New regression test in unitTests/apiTests/mqtt-test.mjs (both-ports registration → written yaml contains certificates), bisect-verified.
• Full mqtt-test.mjs: 27 passing locally; 2 failures (retained record with upsert, mTLS subscribe) reproduce identically on pristine main — pre-existing local-env flake (known: local unit suites are unreliable on this box), CI is the arbiter.
• npm run build, prettier, oxlint clean.

Generated by Claude (Fable 5); investigation and live-prod evidence with Kris. Local repro artifacts referenced in the FDE Slack thread.

[gemini-code-assist]

Code Review

This pull request resolves a bug (#1998) where registering both a plain TCP port and a secure port in a single socket call caused the shared socketServer binding to be reassigned to the plain TCP server, leading to empty certificates in the secure-port UDS metadata. The fix introduces a local secureSocketServer constant to correctly preserve the secure socket server instance within the metadata-write closure. A regression test has also been added to verify this behavior. There are no review comments, so I have no feedback to provide.

[claude]

Reviewed; no blockers found.

[Devin-Holland]

Verified — no blockers

Independently verified

I re-derived the diagnosis from the code rather than taking the write-up on trust, and it holds — this is the mechanism, and it explains every observation that didn't add up before.

The reassignment really does outlive the closure. On the shipped v5.1.25 tag, onSocket: let socketServer (:492) → secure branch assigns it (:497) → const writeMetadata = () => writeUdsMetadata(yamlPath, options.securePort, socketServer) (:542) → secureContextsListeners.push(writeMetadata) (:544) → if (options.port) { … socketServer = createSocketServer(…) } (:547, :549). The closure captures the binding, the binding is rebound to the plain-TCP server, and both write paths (the .ready.then() microtask and every later listener fan-out) run after onSocket returns. secureContexts is undefined on that object, writeUdsMetadata gates on contexts?.size > 0, so it emits a bare certificates:.

The precondition is the shipped default, not a config edge case. static/defaultConfig.yaml ships mqtt.network.port: 1883 and securePort: 8883, so every default install passes both, hits the reassignment, and publishes an empty 8883 list on every worker. That's the "deterministically, fleet-wide" claim, confirmed.

And it explains why the HTTP mirror was always fine — which is the part I most wanted an independent structural reason for, rather than an absence of evidence. In http.ts the same closure shape is safe on both counts: const server = (httpServers[port] = …) (:529) is a const, and the port it closes over (:600) is a parameter of getHTTPServer(port, secure, options), never rebound. Two files, same pattern, one has a mutable binding and one doesn't, and the symptom tracked exactly that.

Class sweep — this was the only instance. Since a finding like this is usually a sample of a class, I scripted a hunt across threadServer.js and http.ts for the same shape (a function-scope let, reassigned, captured by a closure that outlives the function). Four other candidates surfaced and all resolve to false positives with reasons: threadServer.js listen_on is declared inside the loop body (per-iteration) and passed to .listen() synchronously; port at :43 is a different scope from the for (let port in …) loop variables captured at :430/:451/:465 (also per-iteration); listening is a local const shadowing the module-level let; http.ts's socket/response/body are request-scoped with synchronous consumers. socketServer was the real one.

Fix completeness. writeMetadata was the only deferred closure in onSocket, so converting it plus every synchronous use to secureSocketServer is the whole surface. SERVERS[securePort] gets the TLS server and SERVERS[port] the plain one, unchanged; socketServer = secureSocketServer preserves the return value for the securePort-only case, so return semantics are identical either way — and the MQTT caller doesn't consume the return anyway.

The test is the right test. Driving global.server.socket() with both ports and asserting the written yaml carries BEGIN CERTIFICATE targets the artifact the proxy actually consumes, which is exactly the seam every prior round missed by driving createTLSSelector with a pseudo-server. It runs in CI and passes (✔ … 103ms), so it's genuinely gating and not just present.

The one CI failure isn't yours

Unit Test (Node.js v22) is red, and it's worth being explicit that it has nothing to do with this change: 1333 passing, 1 failing, and the failure is Audit log > check log after writes and prune — AssertionError: Should have at least a couple of update events. That's the pre-existing flake, and main still carries the old assertion wording, so it will keep reddening every PR until #2002 lands. Landing #2002 is the unblock; nothing to do here. Your new test passed in that same job.

Marked COMMENT rather than approving only because this is still a draft — flip it to ready and I'll approve on the re-read.

On my own miss

Worth recording, since I reviewed #1999 twice and pushed hard on the root cause. I got as far as proving the yaml had been written and therefore updateTLS() must have completed with an empty map — that part was right, and it's why the original subscribe-throws diagnosis was wrong. But I then enumerated "exactly two ways to complete a pass with an empty map" and both were selector-side: every cert failing the per-cert build, or the table having no rows. The third way — the map is perfectly healthy and writeUdsMetadata is simply handed the wrong object — never occurred to me, even though I had read writeUdsMetadata and grep-traced all three of its call sites. I verified that socketServer was passed and never asked what it was bound to at call time. Reading a call site isn't reading the value.

Still open after this (not blockers)

• The empty-map-with-a-default-context case that #2008 targeted is now unaddressed with it closed. Narrow (needs a cert whose hostnames resolve to [] — no usable SANs and no CN) and a different trigger from the customer's, but with this fix the write reaches the right object and still finds an empty map, so it would still publish an empty list. Worth an issue rather than scope here.
• #2004 (swap while secureContexts is non-empty leaves a dead subscription with no armed retry) is unrelated to this and still stands.

No code changes requested — the absence of inline threads here is deliberate, not an omission. Really nice piece of debugging; the mapSize=3, defaultSet=true / ctxSize=undefined instrumentation pair is what turns this from a plausible story into a proven one.

Reviewed by Claude (Opus 5). Verification commands reproducible against the v5.1.25 tag and both branches.

[Devin-Holland]

CI status, since two consecutive red runs will read as "this PR is broken" at a glance — it isn't.

Both failures on Unit Test (Node.js v22) are the same single assertion, and the new MQTT test passes in both runs:

1333 passing / 15 pending / 1 failing
1) Audit log > check log after writes and prune:
   AssertionError: Should have at least a couple of update events

Three things that put it outside this PR:

• It fails on main from unrelated pushes. Run 30572285474 — the chore: migrate to @harperfast/code-guidelines push, nothing to do with TLS — failed on the identical assertion. Roughly 2 of the last 5 main Unit Test runs are red (the other one on a different assertion, entry-count source should not be consulted…, so there's more than one flaky test on main right now).
• [v5.1] Fix MQTT secure-port UDS metadata publishing an empty certificate list #2011 carries this same new test and went green — 46/46 checks, 927 passing in its unit job — and v5.1 has the same pre-deflake assertion text, so that's a real counterfactual rather than a different suite getting lucky.
• I did specifically consider whether the new test could be perturbing it, since it opens real listeners on 21887/28887 that are never closed and leaves a selector registered in liveTLSRebuilders — plausible timing pressure on a test that polls for subscription events later in the same mocha process. The [v5.1] Fix MQTT secure-port UDS metadata publishing an empty certificate list #2011 result argues against it, but if you want it ruled out completely, closing those two servers in the test's finally would settle it cheaply.

#2002 is the unblock — it's still open, and main still carries the old assertion, so this will keep reddening every PR until it lands.

Nothing needed on this PR from my side; flip it out of draft and I'll approve.

Claude (Opus 5)

[kriszyp]

Re the patch-ci conflict report: expected — v5.1 drifted in this region (ciphers line, no dedicatedListener). The hand-resolved backport is #2011 (green, approved).

[kriszyp]

Applied your suggestion in 14a0235 (both branches) — the finally block now closes every server the call created before restoring the registries, so the perturbation theory is ruled out structurally rather than by counterfactual. Flipping out of draft per your note; expecting the v22 job may still redden on the audit-log flake until #2002 lands.