From 0cb7410ca65224b4df5bddd41edd9ecbd92add2d Mon Sep 17 00:00:00 2001 From: Hernan Javier Ardila Sanchez Date: Mon, 10 Aug 2026 08:49:38 +0200 Subject: [PATCH] fix(services): stop embedded-service supervisor retry loop when binary cannot spawn (#9937) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(deps): bump nanoid, dompurify for 2 new Dependabot alerts (#189, #190) Bumps: nanoid ^3.3.17 (was transitive, now overridden), dompurify ^3.4.13 (with monaco-editor scoped override). Closes Dependabot #189, #190. Remaining #182-#188 (js-yaml + mermaid) already closed by #9651 merge — awaiting Dependabot re-scan. npm audit → 0 vulnerabilities. * fix(repo): harden .gitignore to also ignore a _tasks symlink (/_tasks) _tasks is a SEPARATE nested git repo (gitignored). The pattern _tasks/ (trailing slash) ignores only a directory, not a SYMLINK named _tasks. A self-referential _tasks symlink can slip in via git add -A and, once pulled, checkout materializes it over the real _tasks repo (destroying plans/specs/hands-off). Anchored /_tasks ignores the symlink too, preventing re-capture. * fix(services): stop embedded-service supervisor retry loop when binary cannot spawn A non-spawnable supervised binary (ENOENT/EACCES, or an ELF on Windows where spawn() throws EFTYPE synchronously) left the supervisor in 'starting' forever while the HealthChecker polled the dead port every healthIntervalMs. Each failed probe fired a full ProxyFetch dispatcher+native fetch pair, burning CPU and eventually collapsing the server (observed: 24 warns/min against 127.0.0.1:8317 for 2 days). - handle synchronous spawn() throws and the child 'error' event: stop the poller and transition to an explicit error state - transition to error and stop polling when FAILURE_THRESHOLD consecutive health probes fail, including during startup - waitForHealthy re-checks the state after its deadline so a mid-startup error surfaces as a rejection instead of being overwritten by 'running' --------- Co-authored-by: diegosouzapw Co-authored-by: Diego Rodrigues de Sa e Souza Co-authored-by: diegosouzapw Co-authored-by: herjarsa --- src/lib/services/ServiceSupervisor.ts | 51 +++++++++- .../serviceSupervisorSpawnError.test.ts | 95 +++++++++++++++++++ 2 files changed, 141 insertions(+), 5 deletions(-) create mode 100644 tests/unit/services/serviceSupervisorSpawnError.test.ts diff --git a/src/lib/services/ServiceSupervisor.ts b/src/lib/services/ServiceSupervisor.ts index 16555f5c75..fd7fd43dd9 100644 --- a/src/lib/services/ServiceSupervisor.ts +++ b/src/lib/services/ServiceSupervisor.ts @@ -56,6 +56,19 @@ export class ServiceSupervisor extends EventEmitter { this.checker = new HealthChecker(config.healthUrl, config.healthIntervalMs, (h) => { this.health = h; this.emit("stateChange", this.getStatus()); + // A service that fails FAILURE_THRESHOLD consecutive health probes will + // not recover by itself. Stop the poller and surface an explicit error + // state instead of probing the dead port forever — every failed probe + // fires a full ProxyFetch dispatcher+native fetch pair (e.g. against a + // CLIProxyAPI binary that cannot execute on this platform). + if (h === "unhealthy" && (this.state === "running" || this.state === "starting")) { + this.checker.stop(); + this.lastError = sanitizeErrorMessage( + `Health probe failed for ${this.config.tool} (port ${this.config.port})` + ); + this.setState("error"); + void setToolStatus(this.config.tool, "error", undefined, this.lastError); + } }); } @@ -135,7 +148,22 @@ export class ServiceSupervisor extends EventEmitter { const { command, args, env, cwd } = this.config.spawnArgs(); - const child = spawn(command, args, buildServiceSpawnOptions(env, cwd)); + // spawn() can throw SYNCHRONOUSLY on Windows when the binary is not + // executable (EFTYPE/EINVAL for an ELF or a plain text file) instead of + // emitting the child 'error' event. Handle both paths identically so a + // non-spawnable service surfaces an explicit error state and the health + // poller is stopped instead of hammering a dead port forever. + let child: ChildProcess; + try { + child = spawn(command, args, buildServiceSpawnOptions(env, cwd)); + } catch (err) { + this.checker.stop(); + const msg = sanitizeErrorMessage(err instanceof Error ? err.message : String(err)); + this.lastError = msg; + this.setState("error"); + await setToolStatus(this.config.tool, "error", undefined, msg); + return this.getStatus(); + } this.childProcess = child; this.pid = child.pid ?? null; @@ -161,6 +189,17 @@ export class ServiceSupervisor extends EventEmitter { child.once("exit", (code, signal) => { void this.handleExit(code, signal, spawnTime); }); + // Spawn failures (ENOENT, EACCES, or a non-executable binary such as an + // ELF on Windows) surface via the child 'error' event — NOT 'exit'. + // Without this handler the supervisor stays in "starting" forever and + // the health poller hammers the dead port every healthIntervalMs. + child.once("error", (err) => { + this.checker.stop(); + const msg = sanitizeErrorMessage(err instanceof Error ? err.message : String(err)); + this.lastError = msg; + this.setState("error"); + void setToolStatus(this.config.tool, "error", undefined, msg); + }); this.startedAt = new Date().toISOString(); this.checker.start(); @@ -224,10 +263,12 @@ export class ServiceSupervisor extends EventEmitter { if (this.state === "error") throw new Error(this.lastError ?? "Service failed to start"); await new Promise((r) => setTimeout(r, 1_000)); } - // Timeout reached without a healthy probe. Surface this so callers / - // dashboards do not see "running" + "unknown" health silently. We do not - // throw — the service may still be initializing — but we DO record a - // degraded marker so /status returns it and operators can act. + // Timeout reached without a healthy probe. The health poller may have + // flipped the state to "error" while we were waiting (FAILURE_THRESHOLD + // consecutive failures) — surface that instead of a degraded marker. + if (this.state === "error") { + throw new Error(this.lastError ?? "Service failed to start"); + } this.lastError = sanitizeErrorMessage( `Health probe did not succeed within ${timeoutMs}ms — service may still be initializing` ); diff --git a/tests/unit/services/serviceSupervisorSpawnError.test.ts b/tests/unit/services/serviceSupervisorSpawnError.test.ts new file mode 100644 index 0000000000..87b1f6cb9e --- /dev/null +++ b/tests/unit/services/serviceSupervisorSpawnError.test.ts @@ -0,0 +1,95 @@ +/** + * Regression tests for the embedded-services supervisor (ServiceSupervisor). + * + * Bug: when the supervised binary cannot be spawned (ENOENT / EACCES, or a + * non-executable binary such as an ELF on Windows — EFTYPE), the child emits + * the 'error' event — NOT 'exit' — and on Windows spawn() can even throw + * synchronously. The supervisor had no 'error' handler, so it stayed in + * "starting" forever while the HealthChecker kept polling the dead port every + * healthIntervalMs (each probe firing a full ProxyFetch dispatcher+native + * fetch pair, e.g. against a CLIProxyAPI port that will never answer on this + * platform). + * + * Fix under test: the supervisor now (1) handles synchronous spawn() throws + * and the child 'error' event → stops the poller and transitions to "error", + * and (2) transitions to "error" and stops the poller once the health checker + * reports FAILURE_THRESHOLD consecutive failures, including during startup, + * instead of polling the dead endpoint forever. + * + * Run: node --import tsx/esm --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test tests/unit/services/serviceSupervisorSpawnError.test.ts + */ +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtemp, writeFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { ServiceSupervisor } from "../../../src/lib/services/ServiceSupervisor.ts"; +import type { ServiceConfig } from "../../../src/lib/services/types.ts"; + +function baseConfig(overrides: Partial = {}): ServiceConfig { + return { + tool: "cliproxy", + port: 0, + spawnArgs: () => ({ + command: process.execPath, + args: ["-e", "setTimeout(() => {}, 30_000)"], + env: process.env, + cwd: tmpdir(), + }), + healthUrl: () => "http://127.0.0.1:1/v1/models", + healthIntervalMs: 50, + stopTimeoutMs: 1_000, + logsBufferBytes: 4_096, + ...overrides, + }; +} + +describe("ServiceSupervisor spawn-failure handling", () => { + it("transitions to error and stops polling when the binary cannot be spawned", async () => { + // A plain text file is not an executable: on Windows spawn() throws + // synchronously (EFTYPE/EINVAL); on POSIX the child emits 'error' + // (ENOENT/EACCES). Both paths must land in an explicit error state. + const dir = await mkdtemp(join(tmpdir(), "svc-sup-spawn-")); + const badBinary = join(dir, "not-an-executable.txt"); + await writeFile(badBinary, "this is not a runnable binary\n", "utf8"); + + const supervisor = new ServiceSupervisor( + baseConfig({ + spawnArgs: () => ({ + command: badBinary, + args: [], + env: process.env, + cwd: dir, + }), + }) + ); + + try { + const status = await supervisor.start(); + assert.equal(status.state, "error"); + assert.ok(status.lastError, "lastError should describe the spawn failure"); + assert.match( + status.lastError!, + /ENOENT|EACCES|EINVAL|EFTYPE|not recognized|spawn|%1|Win32/i + ); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + it("transitions to error after consecutive health failures instead of polling forever", async () => { + const supervisor = new ServiceSupervisor(baseConfig()); + try { + // The child runs but never opens a server on the health URL: the + // HealthChecker reaches FAILURE_THRESHOLD (3 × 50ms) and the supervisor + // must surface an explicit error instead of staying "running" with an + // endless poller. + await assert.rejects(supervisor.start(), /Health probe failed|Service failed to start/i); + assert.equal(supervisor.getStatus().state, "error"); + assert.ok(supervisor.getStatus().lastError); + } finally { + await supervisor.stop(); + } + }); +});