feat(server): surface external port conflicts on all platforms; single-worker default without SO_REUSEPORT - #1605
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a regression test to verify external MQTT port-conflict detection and updates the thread server to log external bind conflicts when reusePort is enabled. The feedback recommends passing the caught error object to the logging function to ensure the root cause is preserved, in accordance with the repository's guidelines.
This comment has been minimized.
This comment has been minimized.
9ebd10c to
b1aacfa
Compare
b1aacfa to
72a5ef8
Compare
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
LGTM — the ownership analysis is convincing. Branch is conflicting with main but the hunks look mechanical (drain-hook context drift); approving on that assumption — ping me if the rebase turns out to touch anything real.
sent with Claude Fable 5
…e-worker default without SO_REUSEPORT listenOnPorts() used to swallow every EADDRINUSE (a workaround for a Node <20.11.1 reusePort bug now outside Harper's supported range), hiding real external squatters: an unrelated process holding e.g. the MQTT port silently received Harper's traffic with no error anywhere (original symptom: a second Harper instance on 8883). Every EADDRINUSE with an in-process explanation is now structurally ruled out, so the remaining ones are logged loudly (port + owning component + error): - reusePort listeners (Linux): siblings share the port and never collide, even across overlapping restarts — any EADDRINUSE is external. - Main thread (HTTP/operations ports): binds before any worker, never restarts — any EADDRINUSE is external. - Dedicated listeners (onSocket, e.g. MQTT — never bound by the main thread): when exclusive (macOS/Windows), bound only by a single owner worker (lowest eligible index) instead of every worker racing; combined with non-overlapping restarts (below), the owner's EADDRINUSE is external. The one remaining benign case — a worker's exclusive HTTP bind losing to the main thread on macOS/Windows — stays silently swallowed. All cases still resolve so a squatted port never stalls boot. restartWorkers() no longer pre-starts replacement HTTP workers on macOS (canPreStartReplacement now excludes darwin, like Windows/Bun): without working SO_REUSEPORT the replacement could never bind ports the old worker still held — its EADDRINUSE was swallowed and worker-owned listeners like MQTT were left permanently unbound after every component-reload restart. The main thread keeps serving the HTTP ports throughout, so only worker-owned listeners see the brief shutdown-first gap. threads.count now defaults to 1 on macOS/Windows (setDefaultThreads): without SO_REUSEPORT, additional HTTP workers can never share the server ports, so the CPU-based default just spawned workers that serve no direct TCP traffic. An explicit threads.count still overrides. Adds an integration test that squats the MQTT secure port before boot and asserts the conflict is logged and Harper still starts — on every platform. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
72a5ef8 to
e8f48e3
Compare
| import { createServer, type Server } from 'node:net'; | ||
| import { mkdtemp, readFile } from 'node:fs/promises'; | ||
| import { join } from 'node:path'; | ||
| import { tmpdir } from 'node:os'; |
There was a problem hiding this comment.
Blocker — no-restricted-imports lint violation
node:assert/strict is banned by the root .oxlintrc.json (rule configured as "error"), so npm run lint (oxlint --deny-warnings .) will fail on this file. Every other integration test in this directory imports from plain node:assert (rolling-restart.test.ts, ops-port-isolation.test.ts, etc.).
| import { tmpdir } from 'node:os'; | |
| import { ok } from 'node:assert'; |
Summary
Stacked on #1603 (MQTT listeners share their port via
SO_REUSEPORTon non-macOS). This makes external port conflicts detected and loudly logged on every platform, and aligns two related behaviors on platforms without workingSO_REUSEPORT(macOS/Windows).listenOnPorts()used to swallow everyEADDRINUSE— a workaround for a Node <20.11.1 reusePort bug now outside Harper's supported range (^22.18 || >=24). So an unrelated process squatting a Harper port (the original symptom: a second Harper instance holding 8883) silently received Harper's MQTT traffic with no error logged anywhere.Design
Every
EADDRINUSEwith a possible in-process explanation is structurally ruled out, so what remains is unambiguously external and gets logged (port + owning component + error):reusePortlistener (Linux)server.socket())The one remaining benign case — a worker's exclusive HTTP bind losing to the main thread on macOS/Windows — stays silently swallowed. Everything still resolves, so a squatted port never stalls boot.
Supporting changes:
restartWorkers()no longer pre-starts replacements on macOS (canPreStartReplacementnow excludes darwin, like Windows/Bun). This also fixes a latent bug: the pre-started replacement could never bind the exclusive ports the old worker still held, itsEADDRINUSEwas swallowed, and worker-owned listeners (MQTT) were left permanently unbound after every component-reload restart on macOS. The main thread keeps serving HTTP throughout, so only worker-owned listeners see the brief shutdown-first gap.threads.countdefaults to 1 on macOS/Windows (setDefaultThreads): withoutSO_REUSEPORT, extra HTTP workers can never share the server ports — the CPU-based default just spawned workers that serve no direct TCP traffic. Explicitthreads.countstill overrides.Where to look
socketRouter.tsawaitslistenOnPorts()pre-spawn); component plugins don't runhandleApplicationon the main thread, so MQTT listeners exist only in workers.canPreStartReplacementon darwin: Multi-worker HTTP rolling restart produces ~0.6–1.2s whole-pool connection-refused gap #1417 (rolling-restart conn-refused) was a LinuxSO_REUSEPORT-pool fix and Linux is unchanged.rolling-restart.test.tsfails identically on macOS on the base branch (pre-existing), so CI/Linux remains the gating signal.threadRange: the dedicated-listener owner is the lowest eligible index, not hardcoded 0.Test
integrationTests/server/external-port-conflict.test.tssquats the MQTT secure port before boot and asserts the conflict is logged on every platform (skip removed) and Harper still starts. Plus platform-stubbed unit coverage forsetDefaultThreadsand thededicatedListenermarker.🤖 Generated by Claude (Fable 5 / Opus 4.8 across revisions). Cross-model reviewed (Codex + Gemini) at each design iteration; earlier reviews drove the pivot from worker-gating → reusePort-only → this universal design.