From 21ba118e0d55580660da382548e0660c5b760536 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 31 Aug 2026 08:38:45 -0600 Subject: [PATCH 1/4] squash: PR#2363 net work for conflict analysis --- server/threads/manageThreads.js | 54 +++++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 13d302c8da..84bb805fcc 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -529,7 +529,7 @@ async function restartWorkers( maxWorkersDown = maxWorkersDown * workers.length; } // make a copy of the workers before iterating them, as the workers array mutates a lot during this - let waitingToFinish = []; // promises for workers we have shut down and are waiting to exit + let waitingToFinish = []; // promises for workers we are replacing, spliced as each is replaced // Every replacement that was started without being awaited first, so this function can still // resolve only once each one is accepting connections. let replacementsStarting = []; @@ -539,6 +539,7 @@ async function restartWorkers( // costs capacity until startWorker's auto-restart brings a fresh one up. let workersKeptOnOldCode = 0; let replacementsNotStarted = 0; + let replacementsFailedToStart = 0; // We can only start the replacement *before* the old worker releases its port when the OS lets // both listen on the same port at once (SO_REUSEPORT). Without that — Windows (no SO_REUSEPORT), // macOS (unreliable SO_REUSEPORT, so workers bind exclusively), and Bun — the replacement can't @@ -550,7 +551,9 @@ async function restartWorkers( // thread keeps serving the HTTP ports throughout. This ordering is also what lets // listenOnPorts() treat a dedicated listener's EADDRINUSE as an external conflict. const canPreStartReplacement = process.platform !== 'win32' && process.platform !== 'darwin' && !isBun; - for (let worker of workers.slice(0)) { + const restarting = workers.slice(0); + for (let index = 0; index < restarting.length; index++) { + const worker = restarting[index]; // Terminal shutdown: stop replacing workers mid-loop — the guard for every replacement start below. if (processShuttingDown && startReplacementThreads) break; if ((name && worker.name !== name) || worker.wasShutdown) continue; // filter by type, if specified @@ -639,13 +642,16 @@ async function restartWorkers( // Overlapping types we couldn't pre-start (Windows/Bun): start the replacement now that the old // worker is releasing its port. server.close() stops accepting immediately, so the port frees up // well before the replacement finishes booting and binds. - if (overlapping && startReplacementThreads && !canPreStartReplacement && !processShuttingDown) - replacementsStarting.push( - whenWorkerStarted(worker.startCopy()).then((started) => { - onProgress?.(); - return started; - }) - ); + let replacementStarting; + if (overlapping && startReplacementThreads && !canPreStartReplacement && !processShuttingDown) { + replacementStarting = whenWorkerStarted(worker.startCopy()).then((started) => { + if (!started) replacementsFailedToStart++; + onProgress?.(); + return started; + }); + replacementsStarting.push(replacementStarting); + } + let whenDone = new Promise((resolve) => { // in case the exit inside the thread doesn't timeout, force it from the outside @@ -688,18 +694,40 @@ async function restartWorkers( clearTimeout(timeout); onProgress?.(); worker.extendTerminateDeadline = undefined; - const index = waitingToFinish.indexOf(whenDone); - if (index > -1) waitingToFinish.splice(index, 1); // non-overlapping types have no advance replacement, so start it once the old one is gone if (!overlapping && startReplacementThreads && !processShuttingDown) worker.startCopy(); resolve(); }); }); - waitingToFinish.push(whenDone); + // A worker counts as replaced once its replacement is accepting connections, not merely once it + // has exited. Where the replacement cannot be pre-started it boots only after the old worker is + // gone, so throttling on the exit alone let the loop take the whole pool down while the first + // replacements were still booting. + // This promise is held unawaited between throttle points, so it must not be able to reject. + const replaced = (replacementStarting ? Promise.all([whenDone, replacementStarting]) : whenDone) + .catch((error) => harperLogger.warn('Error waiting for a worker to be replaced', error)) + .then(() => { + const index = waitingToFinish.indexOf(replaced); + if (index > -1) waitingToFinish.splice(index, 1); + }); + waitingToFinish.push(replaced); if (waitingToFinish.length >= maxWorkersDown) { - // throttle how many workers are draining/down at once to limit load + // throttle how many workers are down at once to limit load await Promise.race(waitingToFinish); } + // 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 would leave nothing serving. + // The workers not yet touched are still running the old code, which beats none running at all. + if (replacementsFailedToStart >= maxWorkersDown) { + const untouched = restarting + .slice(index + 1) + .filter((other) => (!name || other.name === name) && !other.wasShutdown).length; + harperLogger.error( + `${replacementsFailedToStart} replacement worker thread(s) did not start; stopping this restart with ${untouched} worker(s) still on the previous code` + ); + workersKeptOnOldCode += untouched; + break; + } } await Promise.all(waitingToFinish); // A caller awaiting this needs it to mean "the pool is serving the new code", so wait out the From ba26fc85bf56e70ccd2040839fa136573cf18640 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 31 Aug 2026 08:42:35 -0600 Subject: [PATCH 2/4] fix(restart): count a worker as replaced when its replacement is serving, not when it exits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit restartWorkers() throttled on maxWorkersDown by counting a worker as "down" until it exited, not until its replacement was actually serving. Where the replacement can't be pre-started (Windows/macOS/Bun) it only boots after the old worker is gone, so throttling on exit alone let the loop race ahead of booting replacements and take the whole pool down at once. - waitingToFinish now tracks a worker until its replacement is confirmed serving (or given up on), not merely until the old worker exits. - A new replacementsFailedToStart counter stops the restart, leaving the remaining pool on the old code, once too many replacements in a row fail to come up — readiness throttling bounds how many workers are down at once, but not how many outright fail to return. This is the pool-availability throttle rework called out as shipping separately when #2341 (harper#2335) landed. --- server/threads/manageThreads.js | 1 - 1 file changed, 1 deletion(-) diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 84bb805fcc..4816472fa5 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -652,7 +652,6 @@ async function restartWorkers( replacementsStarting.push(replacementStarting); } - let whenDone = new Promise((resolve) => { // in case the exit inside the thread doesn't timeout, force it from the outside const armTerminate = (delay) => From 2696b41b42b7b4e034868b5738f9eae5e3ca3ff6 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 31 Aug 2026 09:05:33 -0600 Subject: [PATCH 3/4] fix(restart): don't count an auto-restarted worker as still on old code, avoid index shadow --- server/threads/manageThreads.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index 4816472fa5..a868093041 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -698,16 +698,14 @@ async function restartWorkers( resolve(); }); }); - // A worker counts as replaced once its replacement is accepting connections, not merely once it - // has exited. Where the replacement cannot be pre-started it boots only after the old worker is - // gone, so throttling on the exit alone let the loop take the whole pool down while the first - // replacements were still booting. - // This promise is held unawaited between throttle points, so it must not be able to reject. + // A worker counts as replaced only once its replacement is accepting connections, not merely + // once it has exited. This promise is held unawaited between throttle points, so it must not + // be able to reject. const replaced = (replacementStarting ? Promise.all([whenDone, replacementStarting]) : whenDone) .catch((error) => harperLogger.warn('Error waiting for a worker to be replaced', error)) .then(() => { - const index = waitingToFinish.indexOf(replaced); - if (index > -1) waitingToFinish.splice(index, 1); + const at = waitingToFinish.indexOf(replaced); + if (at > -1) waitingToFinish.splice(at, 1); }); waitingToFinish.push(replaced); if (waitingToFinish.length >= maxWorkersDown) { @@ -720,7 +718,11 @@ async function restartWorkers( if (replacementsFailedToStart >= maxWorkersDown) { const untouched = restarting .slice(index + 1) - .filter((other) => (!name || other.name === name) && !other.wasShutdown).length; + // a worker that exited on its own mid-restart is spliced out of `workers` and + // auto-restarted onto the new code (see the exit handler above); it is not still on + // the previous code even though this loop never got to it. + .filter((other) => (!name || other.name === name) && !other.wasShutdown && workers.includes(other)) + .length; harperLogger.error( `${replacementsFailedToStart} replacement worker thread(s) did not start; stopping this restart with ${untouched} worker(s) still on the previous code` ); From cb6c70d9efe8d7165281ce9b4bd017b2743b83c2 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Mon, 31 Aug 2026 09:10:02 -0600 Subject: [PATCH 4/4] style: prettier formatting --- server/threads/manageThreads.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/threads/manageThreads.js b/server/threads/manageThreads.js index a868093041..9ad683b322 100644 --- a/server/threads/manageThreads.js +++ b/server/threads/manageThreads.js @@ -721,8 +721,7 @@ async function restartWorkers( // a worker that exited on its own mid-restart is spliced out of `workers` and // auto-restarted onto the new code (see the exit handler above); it is not still on // the previous code even though this loop never got to it. - .filter((other) => (!name || other.name === name) && !other.wasShutdown && workers.includes(other)) - .length; + .filter((other) => (!name || other.name === name) && !other.wasShutdown && workers.includes(other)).length; harperLogger.error( `${replacementsFailedToStart} replacement worker thread(s) did not start; stopping this restart with ${untouched} worker(s) still on the previous code` );