Count a worker as replaced when its replacement is serving, not when it exits

[kriszyp]

Split out of #2341 at Kris's request. #2341 has since merged (squashed, as c4dd962) and this branch is now rebased directly onto main; it builds on that PR's whenWorkerStarted helper, which is why this diff is the restart-pacing change alone.

restartWorkers() advanced its maxWorkersDown throttle when a worker exited. On the platforms that cannot pre-start a replacement into the predecessor's listening port — Windows, macOS, Bun — the replacement only starts once that worker is gone, and old workers exit in milliseconds while a replacement takes seconds to boot, so the loop could walk the whole pool down before any replacement was serving. A worker now counts as replaced only once its replacement reports CHILD_STARTED, which is what the throttle was there to bound; and if replacements keep failing to start, the restart stops rather than continuing to take down workers that are still serving. On SO_REUSEPORT platforms the replacement is already awaited before its predecessor is shut down, so nothing changes there.

For the human reviewer

1. The effect is structural; I could not measure it. Two probes on macOS came back flat: fresh MQTT-over-WebSocket connections across a restart_service http were refused on 2.5% of attempts with this change and 2.6% without (690/695 attempts, 8 threads), and get_status's per-worker roster never dipped below 8 on either build. The first is explained by a dedicated worker-owned listener having a single owner worker — the client-visible gap is that owner's replacement time, which no throttle shrinks; the second by the roster counting registered workers rather than ready ones. So the review's original "total downtime during a deploy" framing is overstated for macOS. What the change buys is a bound on how many worker threads are simultaneously not-yet-serving, which matters most for load on Windows/Bun, where CI's shards are the only signal I have.
2. It changes restart pacing on three platforms. A restart there now advances at replacement-boot speed rather than predecessor-exit speed: slower in wall-clock terms, never below the intended capacity. That is also the ordering the harper#1813 EADDRINUSE hazard lives in — a drain delaying the predecessor's port release while the replacement tries to bind. This change makes the loop wait on that replacement rather than move past it, which is the safer side of the hazard, but it deserves a second opinion from whoever knows that failure best.
3. The bail-out is a policy call. Readiness throttling bounds how many workers are down at once but not how many fail: with replacements that never come up, walking the rest of the pool leaves nothing serving. So the restart now stops once the number of replacements that never started reaches maxWorkersDown, logs it, and reports the untouched workers as left on the previous code (workersKeptOnOldCode, which Wait for the restart a component deploy triggers, and tell MQTT clients why a publish was refused #2341 already surfaces in the log). The alternative is to keep rolling and end with an empty pool. Failed slots do free their throttle entry as they settle, so up to maxWorkersDown workers can be unavailable before the stop trips — that is the bound the parameter already promises, not a new one.
4. Not fixed, disclosed. CHILD_STARTED can still certify a worker whose dedicated listener failed to bind (harper#1813) — readiness is "the worker reported started", not "every listener is bound". And this is a no-op for job workers: they are the !overlapping path, their replacement is still started without being awaited, and they serve no connections. The gemini leg read that path as Windows/Bun and concluded "total downtime"; the platform axis is canPreStartReplacement, not overlapping, so that conclusion does not hold.
5. No test. Neither observable I tried distinguishes the two builds, so a regression test would assert something I cannot demonstrate. Pinning it needs a per-worker readiness probe that does not exist yet.
6. Rebase note. Wait for the restart a component deploy triggers, and tell MQTT clients why a publish was refused #2341 merging while this branch still carried its unsquashed history meant a literal per-commit git rebase origin/main reconflicted on content main already had (verbatim, in every file but this one). Resolved by squashing this branch's unique commits into one tree state and rebasing that single commit onto main — one 3-way merge instead of 24, verified by diffing the result against main (clean everywhere but manageThreads.js) and reading through the two real conflicts to confirm the branch's version was a strict superset of what main already had. The rebase's own pre-push review caught a real bug in the process: the abort accounting's untouched count (point 3 above) didn't exclude a worker that exited on its own mid-restart — it's spliced out of workers and auto-restarted onto the new code by the ordinary exit handler, but was still being counted as "left on old code". Fixed at manageThreads.js:724.

Verification

npm run build and npm run lint:required clean. integrationTests/{deploy,mqtt} 80 passed / 0 failed / 1 skipped and the deploy-restart-topic-availability suite 4/4 on macOS, which is a non-pre-start platform, so every restart in those suites exercises this path.

Re-verified after the rebase, on Linux: npm run build clean; unitTests/components/{awaitRestart,requestRestart}.test.js and unitTests/server/threads/*.test.js (58 passing); integrationTests/mqtt/deploy-restart-topic-availability.test.ts (4/4) and integrationTests/components/acl-connect.test.ts (13/13). Linux pre-starts replacements (canPreStartReplacement), so these don't exercise the readiness-throttle path either — see point 5 above, unchanged by the rebase.

Pre-existing macOS breakage found while verifying, not caused by this PR: integrationTests/components/shutdown-drain-e2e.test.ts fails 2/2 on a clean main checkout with a fresh build, with the EADDRINUSE-during-drain signature that file's own comment documents for Bun (Failed to bind TLS listener for component 'mqtt' … address already in use, 4 occurrences). It is skipped only under Bun, and CI runs Linux and Windows, so macOS red goes unseen. Worth its own issue — say the word and I'll file it.

Complexity: medium

_{Review-Coverage: authored=claude; ran=codex,gemini; declined=cursor-grok,cursor-composer,domain; rounds=3 @ cb6c70d}

_{Human-Review-Need: 3 @ cb6c70d}

[gemini-code-assist]

Code Review

This pull request modifies the worker restart logic in server/threads/manageThreads.js to ensure a worker is only considered fully replaced once its replacement is accepting connections, rather than immediately upon the old worker's exit. This prevents the worker pool from being depleted during sequential restarts. The feedback suggests using .finally() instead of .then() when resolving the replacement promise to guarantee that the worker is removed from the waitingToFinish tracking array even if the replacement or shutdown fails.

[claude]

Reviewed; no blockers found.

[DavidCockerill]

Approving — the readiness/exit synchronization holds, including the failure and auto-restart paths.

🧊 In plain terms: on platforms that can't start a replacement worker before the old one lets go of its port, the restart loop used to count a worker as "handled" the moment it exited. Old workers die in milliseconds and replacements take seconds, so the loop could walk the whole pool down before anything was serving again. Now a worker only counts once its replacement is actually accepting connections.

I traced the parts that looked fragile and they aren't:

• The circuit breaker can actually fire. It increments on if (!started), which only works if whenWorkerStarted resolves falsy rather than rejecting — otherwise the .catch on replaced would swallow it and the counter would sit at 0 forever. It resolves: new Promise((resolve) => …) with no reject path, returning false both on boot timeout and on exit-before-CHILD_STARTED.
• replaced referencing itself inside its own .then to splice out of waitingToFinish is fine — the callback runs long after the binding completes.
• The splice is in the chain that resolves replaced, so by the time Promise.race(waitingToFinish) wakes, the array is already trimmed. No stale entries accumulating in the throttle.

Two comments overclaim, and I'd fix the comments rather than the code.

replacementsFailedToStart is only incremented inside the !canPreStartReplacement branch, and OVERLAPPING_RESTART_TYPES is HTTP-only — so the breaker covers exactly one combination: HTTP workers on Windows/macOS/Bun. On Linux the counter never leaves 0 and manageThreads.js:718 is dead code.

That's defensible rather than broken: on Linux the HTTP path pre-starts and awaits the replacement, so a failure leaves the old worker serving and increments workersKeptOnOldCode instead — no capacity is lost, so there's nothing for a breaker to stop. The gap is the other half. Non-HTTP workers are shut down with their replacement fired unawaited from the exit handler (:697), on every platform, with no readiness tracking and no contribution to the counter. If those replacements never come up the loop still walks the entire non-HTTP pool and drains it — which is exactly the scenario the comment at :715-717 says is now prevented. Same for :701-703: "a worker counts as replaced only once its replacement is accepting connections" holds for both overlapping paths, not for the non-overlapping one, where replaced is whenDone alone.

One observation rather than a finding: the failure timeout is Math.max(threadTerminationTimeout * 2, 60000), and the breaker needs maxWorkersDown failures to trip, so a fleet where every replacement wedges in boot can sit in a throttled restart for minutes before giving up. Bounded by the Promise.race throttle, so nothing runs away — just slower than "stops rather than continuing" suggests.

Context for whoever reads this next: this does not touch the harper#1585 self-restart deadlock, whose root cause is the !started branch clearing worker.wasShutdown = false on a worker an outer shutdown is about to terminate. That path is unchanged here. If anything this is mildly protective, since the breaker terminates fewer workers when replacements are failing.

Coverage: read the restart loop and both platform paths, whenWorkerStarted's resolve contract, the splice/throttle interaction, and the untouched accounting on the break. Not executed.

_{— DAIvid (Claude Opus 5) · cross-model: Codex (graded) + Gemini + Harper domain adjudication}

[kriszyp]

CI triage: all three red checks are pre-existing, none are caused by this PR.

This PR's diff is a single file (server/threads/manageThreads.js, restartWorkers throttling). None of the failing tests reach that code.

check	failing test	verdict
Integration Tests 6/6 (Windows, Node.js v24)	integrationTests/components/branched-database.test.ts:120 — keeps the application's own data across a restart	Deterministic main-branch breakage, already tracked as Branched database is discarded on restart on Windows (main integration gate red). Red on every recent main run (1e4c1636, d0176f26, 65578740, 85f7b183, cd78f58f…) and on unrelated PR Publish TLS state transactionally…. The test drives a whole-process killHarper/startHarper, not a restartWorkers() rolling restart.
Unit Test (Node.js v22)	unitTests/resources/txn-tracking.test.js:77 — Slow txn will expire	Timing flake. The resource sleeps 40 ms and then does DB work; the assertion runs after Promise.race([delay(50), result]). This is precisely the delay(N)-then-assert pattern AGENTS.md names as the root of flakiness class #1138.
Unit Test (Node.js v24)	unitTests/resources/caching.test.js:215 — Can load cached data	Timing flake. setTTLExpiration(0.01) (10 ms TTL) raced by await delay(5). The identical assertion fails on main at 65578740 (run).

The decisive evidence that the unit legs are ambient rather than PR-caused: re-running the failed unit jobs against the identical sha (cb6c70d9, run 33407204195) flipped Node v24 to green, while Node v22 failed on a different test — unitTests/resources/vectorIndex.test.js:1009 (HNSW greedy routing above layer 0) — which is itself a confirmed main flake (main run 33395485069). A rotating cast of failures on a fixed commit is flakiness by definition; Unit Test on main has failed roughly 20 of its last 25 runs.

Local verification of this branch (Linux, Node 26):

• npm run build — clean
• unitTests/server/threads/*.test.js + unitTests/components/awaitRestart.test.js — 57 passing
• unitTests/resources/txn-tracking.test.js + unitTests/resources/caching.test.js — 58 passing
• this PR's own integrationTests/server/rolling-restart.test.ts — passing

No changes were pushed. The Windows integration leg will stay red on this branch until #2410 is fixed; the two unit legs should clear on a re-run.

— Claude Opus 5 (dev-agent)