From fbcc43ce89f9e43b208fd6ea6a75d2dcc0372554 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Sun, 21 Jun 2026 21:17:47 -0300 Subject: [PATCH] fix(cli): supervisor restarts on spontaneous exit-0 (OOM cgroup) + waits for port before respawn (#4425) (#4578) Co-authored-by: Diego Rodrigues de Sa e Souza --- bin/cli/runtime/processSupervisor.mjs | 25 ++++++++-- bin/cli/runtime/supervisorPolicy.mjs | 56 +++++++++++++++++++++++ tests/unit/cli-process-supervisor.test.ts | 34 ++++++++++++-- tests/unit/supervisor-policy-4425.test.ts | 54 ++++++++++++++++++++++ 4 files changed, 160 insertions(+), 9 deletions(-) create mode 100644 bin/cli/runtime/supervisorPolicy.mjs create mode 100644 tests/unit/supervisor-policy-4425.test.ts diff --git a/bin/cli/runtime/processSupervisor.mjs b/bin/cli/runtime/processSupervisor.mjs index b16de9670e..ff201cb091 100644 --- a/bin/cli/runtime/processSupervisor.mjs +++ b/bin/cli/runtime/processSupervisor.mjs @@ -1,12 +1,18 @@ import { spawn } from "node:child_process"; import { dirname } from "node:path"; import { writePidFile, cleanupPidFile, killAllSubprocesses } from "../utils/pid.mjs"; +import { + RESTART_RESET_MS, + DEFAULT_MAX_RESTARTS, + shouldExitInsteadOfRestart, + computeRestartDelayMs, + waitUntilPortFree, +} from "./supervisorPolicy.mjs"; const CRASH_LOG_LINES = 50; -const RESTART_RESET_MS = 30_000; export class ServerSupervisor { - constructor({ serverPath, env, maxRestarts = 2, memoryLimit = 512, onCrashCallback }) { + constructor({ serverPath, env, maxRestarts = DEFAULT_MAX_RESTARTS, memoryLimit = 512, onCrashCallback }) { this.serverPath = serverPath; this.env = env; this.maxRestarts = maxRestarts; @@ -54,7 +60,10 @@ export class ServerSupervisor { const exitCode = typeof code === "number" ? code : null; cleanupPidFile("server"); - if (this.isShuttingDown || exitCode === 0) { + // #4425: only exit on an intentional shutdown. A spontaneous code-0 exit (e.g. a + // systemd MemoryMax cgroup kill, which reports the process exited cleanly) is anomalous + // and must be restarted, not treated as a graceful stop that leaves the gateway dead. + if (shouldExitInsteadOfRestart(this.isShuttingDown)) { process.exit(exitCode ?? 0); return; } @@ -79,12 +88,18 @@ export class ServerSupervisor { } this.restartCount++; - const delay = Math.min(1000 * 2 ** (this.restartCount - 1), 10_000); + const delay = computeRestartDelayMs(this.restartCount); console.error( `\n⚠ Server exited (code=${code ?? "?"}). Restarting in ${delay / 1000}s... (${this.restartCount}/${this.maxRestarts})` ); if (this.crashLog.length) this.dumpCrashLog(); - setTimeout(() => this.start(), delay); + // #4425: after a crash the OS may not have released the listen socket yet — restarting + // immediately produced the EADDRINUSE cascade that exhausted the restart budget. Wait + // (bounded) for the port to free up before respawning. + setTimeout(async () => { + await waitUntilPortFree(process.env.PORT || 20128); + this.start(); + }, delay); } dumpCrashLog() { diff --git a/bin/cli/runtime/supervisorPolicy.mjs b/bin/cli/runtime/supervisorPolicy.mjs new file mode 100644 index 0000000000..4505f60b6a --- /dev/null +++ b/bin/cli/runtime/supervisorPolicy.mjs @@ -0,0 +1,56 @@ +import net from "node:net"; + +// #4425: bumped from 30s — the old window reset the crash counter too quickly, so during +// an EADDRINUSE cascade the supervisor kept "recovering" then crashing within the window +// and exhausted its restart budget. A longer window keeps the counter meaningful. +export const RESTART_RESET_MS = 60_000; + +// #4425: bumped from 2 — more recovery headroom before the supervisor gives up. +export const DEFAULT_MAX_RESTARTS = 3; + +/** + * #4425: a clean child exit (code 0) is only intentional when the supervisor itself is + * shutting down. A spontaneous code-0 exit is anomalous — e.g. a systemd `MemoryMax` + * cgroup kill reports the process exited with code 0 — and MUST be restarted, not treated + * as a graceful stop (which left the gateway dead with `Restart=on-failure`). + */ +export function shouldExitInsteadOfRestart(isShuttingDown) { + return isShuttingDown === true; +} + +/** Exponential backoff (1s, 2s, 4s, …) capped at 10s, matching the prior inline formula. */ +export function computeRestartDelayMs(restartCount) { + return Math.min(1000 * 2 ** (Math.max(1, restartCount) - 1), 10_000); +} + +/** Resolve true when nothing is listening on `port` (so a restart won't hit EADDRINUSE). */ +export function isPortFree(port, host = "127.0.0.1") { + return new Promise((resolve) => { + const tester = net.createServer(); + tester.once("error", (err) => { + // EADDRINUSE = something is bound → not free. Any other error → treat as free. + resolve(!(err && err.code === "EADDRINUSE")); + }); + tester.once("listening", () => { + tester.close(() => resolve(true)); + }); + tester.listen(port, host); + }); +} + +/** + * #4425: wait until `port` is free before respawning. After a crash the OS may not have + * released the listen socket yet; restarting immediately produced the EADDRINUSE cascade + * that exhausted the restart budget. Polls up to `timeoutMs`, then proceeds anyway so a + * stuck port never blocks recovery forever. + */ +export async function waitUntilPortFree(port, timeoutMs = 10_000, intervalMs = 250) { + const p = Number(port); + if (!Number.isFinite(p) || p <= 0) return true; + const deadline = Date.now() + timeoutMs; + for (;;) { + if (await isPortFree(p)) return true; + if (Date.now() >= deadline) return false; + await new Promise((r) => setTimeout(r, intervalMs)); + } +} diff --git a/tests/unit/cli-process-supervisor.test.ts b/tests/unit/cli-process-supervisor.test.ts index 99012ad82e..642a1c7182 100644 --- a/tests/unit/cli-process-supervisor.test.ts +++ b/tests/unit/cli-process-supervisor.test.ts @@ -2,6 +2,10 @@ import test from "node:test"; import assert from "node:assert/strict"; import { EventEmitter } from "node:events"; +// #4425: the supervisor now waits for the listen port to free up before respawning. +// Point that probe at the no-op port 0 so the restart tests don't open real sockets. +process.env.PORT = "0"; + // Stub para o processSupervisor: testa a lógica de restart/backoff/MITM sem processos reais. class StubChild extends EventEmitter { @@ -41,9 +45,13 @@ test("detectMitmCrash retorna false com menos de 2 sinais", async () => { // --- ServerSupervisor: lógica de restart --- -test("ServerSupervisor.handleExit com code=0 chama process.exit(0)", async () => { +test("ServerSupervisor.handleExit com code=0 espontâneo reinicia em vez de sair (#4425)", async () => { const { ServerSupervisor } = await import("../../bin/cli/runtime/processSupervisor.mjs"); + // waitUntilPortFree no-ops on port 0, so the scheduled restart fires fast and leaks no timer. + const origPort = process.env.PORT; + process.env.PORT = "0"; + const exits: number[] = []; const origExit = process.exit.bind(process); // @ts-ignore @@ -56,11 +64,27 @@ test("ServerSupervisor.handleExit com code=0 chama process.exit(0)", async () => env: {}, maxRestarts: 2, }); + let started = 0; + // Stub start() so the scheduled restart never spawns the fake server. + supervisor.start = () => { + started++; + return null as any; + }; supervisor.handleExit(0); + // #4425: a spontaneous code-0 exit (e.g. a systemd MemoryMax cgroup kill, which reports + // a clean exit) must NOT terminate the supervisor — it schedules a restart instead. + assert.equal(exits.length, 0, "must not process.exit on a spontaneous code-0 exit"); + assert.equal(supervisor.restartCount, 1); + + // Let the scheduled restart fire so no timer leaks past the test. + await new Promise((r) => setTimeout(r, 1100)); + assert.equal(started, 1, "restart should fire after the backoff delay"); + // @ts-ignore process.exit = origExit; - assert.equal(exits[0], 0); + if (origPort === undefined) delete process.env.PORT; + else process.env.PORT = origPort; }); test("ServerSupervisor.handleExit com isShuttingDown=true chama process.exit imediato", async () => { @@ -125,6 +149,7 @@ test("ServerSupervisor.handleExit exibe crash log ao reiniciar", async () => { console.error = origErr; assert.ok(logs.some((l) => l.includes("line1") || l.includes("crash log"))); + await new Promise((r) => setTimeout(r, 1100)); // drain the scheduled restart timer }); test("ServerSupervisor chama onCrashCallback após maxRestarts atingido", async () => { @@ -179,7 +204,7 @@ test("ServerSupervisor retorna 'disable-mitm-and-retry' chama start() novamente" assert.equal(supervisor.restartCount, 0); // foi resetado }); -test("ServerSupervisor reseta restartCount após processo viver >=30s", async () => { +test("ServerSupervisor reseta restartCount após viver >= RESTART_RESET_MS (#4425: 60s)", async () => { const { ServerSupervisor } = await import("../../bin/cli/runtime/processSupervisor.mjs"); const supervisor = new ServerSupervisor({ @@ -189,10 +214,11 @@ test("ServerSupervisor reseta restartCount após processo viver >=30s", async () }); supervisor.start = () => null as any; supervisor.restartCount = 2; - supervisor.startedAt = Date.now() - 31_000; // viveu 31s + supervisor.startedAt = Date.now() - 61_000; // #4425: reset window bumped 30s→60s supervisor.handleExit(1); assert.equal(supervisor.restartCount, 1); // reset p/ 0, depois incrementado p/ 1 + await new Promise((r) => setTimeout(r, 1100)); // drain the scheduled restart timer }); // --- Node.js v24 compat: process.exit() must receive a number (#3748) --- diff --git a/tests/unit/supervisor-policy-4425.test.ts b/tests/unit/supervisor-policy-4425.test.ts new file mode 100644 index 0000000000..6c83c4be41 --- /dev/null +++ b/tests/unit/supervisor-policy-4425.test.ts @@ -0,0 +1,54 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import net from "node:net"; + +// #4425: the supervisor treated a clean exit (code 0) as intentional and exited instead +// of restarting — but a systemd MemoryMax cgroup kill reports code 0, so the OOM'd gateway +// stayed dead. And it restarted immediately after a crash, hitting EADDRINUSE before the +// OS released the port. supervisorPolicy centralizes the restart decision + a port-wait. + +const { + RESTART_RESET_MS, + DEFAULT_MAX_RESTARTS, + shouldExitInsteadOfRestart, + computeRestartDelayMs, + isPortFree, + waitUntilPortFree, +} = await import("../../bin/cli/runtime/supervisorPolicy.mjs"); + +test("#4425 spontaneous code-0 exit restarts (only shutdown exits)", () => { + assert.equal(shouldExitInsteadOfRestart(false), false); // OOM cgroup code-0 → restart + assert.equal(shouldExitInsteadOfRestart(true), true); // operator stop() → exit +}); + +test("#4425 tuned recovery constants", () => { + assert.equal(RESTART_RESET_MS, 60_000); + assert.equal(DEFAULT_MAX_RESTARTS, 3); +}); + +test("#4425 restart backoff is 1s,2s,4s… capped at 10s", () => { + assert.equal(computeRestartDelayMs(1), 1000); + assert.equal(computeRestartDelayMs(2), 2000); + assert.equal(computeRestartDelayMs(3), 4000); + assert.equal(computeRestartDelayMs(10), 10_000); +}); + +test("#4425 isPortFree detects a bound port and waitUntilPortFree resolves once released", async () => { + const server = net.createServer(); + await new Promise((resolve) => server.listen(0, "127.0.0.1", () => resolve())); + const port = (server.address() as net.AddressInfo).port; + + assert.equal(await isPortFree(port), false, "bound port is not free"); + + // waitUntilPortFree should time out (return false) while the port stays bound. + assert.equal(await waitUntilPortFree(port, 300, 50), false); + + await new Promise((resolve) => server.close(() => resolve())); + assert.equal(await isPortFree(port), true, "released port is free"); + assert.equal(await waitUntilPortFree(port, 1000, 50), true); +}); + +test("#4425 waitUntilPortFree no-ops on an invalid port", async () => { + assert.equal(await waitUntilPortFree(undefined), true); + assert.equal(await waitUntilPortFree(0), true); +});