diff --git a/bin/cli/commands/doctor.mjs b/bin/cli/commands/doctor.mjs index 817013dd19..f373d8cc5e 100644 --- a/bin/cli/commands/doctor.mjs +++ b/bin/cli/commands/doctor.mjs @@ -10,6 +10,7 @@ import { getCliToken, CLI_TOKEN_HEADER } from "../utils/cliToken.mjs"; import { printHeading } from "../io.mjs"; import { t } from "../i18n.mjs"; import { readDatabaseHealth, readEncryptedCredentialSamples } from "../sqlite.mjs"; +import { getCrashLogPath } from "../runtime/processSupervisor.mjs"; const STATIC_SALT = "omniroute-field-encryption-v1"; const KEY_LENGTH = 32; @@ -380,6 +381,33 @@ function checkMemory() { }); } +// #13538: surfaces the supervisor's give-up crash record (persisted by +// ServerSupervisor.persistCrashLog(), bin/cli/runtime/processSupervisor.mjs) +// so a user whose `--tray` worker died silently (detached, stdio:"ignore") +// has something concrete `doctor` can point at without needing `--log`. +function checkCrashLog() { + const crashLogPath = getCrashLogPath(); + if (!fs.existsSync(crashLogPath)) { + return ok("Crash log", "No supervisor crash record found", { crashLogPath }); + } + + try { + const stat = fs.statSync(crashLogPath); + const contents = fs.readFileSync(crashLogPath, "utf8"); + const lastEntry = contents.split("\n").filter(Boolean).slice(-6).join("\n"); + return warn( + "Crash log", + `Supervisor recorded a give-up crash at ${crashLogPath} (last modified ${stat.mtime.toISOString()})`, + { crashLogPath, modifiedAt: stat.mtime.toISOString(), tail: lastEntry } + ); + } catch (error) { + return warn("Crash log", `Crash record exists at ${crashLogPath} but could not be read`, { + crashLogPath, + error: error instanceof Error ? error.message : String(error), + }); + } +} + async function fetchWithTimeout(url, options = {}) { const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), CHECK_TIMEOUT_MS); @@ -579,6 +607,7 @@ export async function collectDoctorChecks(context = {}, options = {}) { checks.push(await checkNodeRuntime(rootDir)); checks.push(await checkNativeBinary(rootDir)); checks.push(checkMemory()); + checks.push(checkCrashLog()); if (!options.skipLiveness) { checks.push(await checkServerLiveness(options)); diff --git a/bin/cli/runtime/processSupervisor.mjs b/bin/cli/runtime/processSupervisor.mjs index 3bef5fa08d..b93dfb57bf 100644 --- a/bin/cli/runtime/processSupervisor.mjs +++ b/bin/cli/runtime/processSupervisor.mjs @@ -1,7 +1,9 @@ import { spawn } from "node:child_process"; +import { mkdirSync, appendFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { writePidFile, cleanupPidFile, killAllSubprocesses, isPidRunning } from "../utils/pid.mjs"; +import { resolveDataDir } from "../data-dir.mjs"; import { RESTART_RESET_MS, DEFAULT_MAX_RESTARTS, @@ -19,6 +21,13 @@ import { const CRASH_LOG_LINES = 50; +// #13538: shared path resolver so `omniroute doctor` (bin/cli/commands/doctor.mjs) +// can surface the same file persistCrashLog() writes, without duplicating the +// `/server/...` convention from bin/cli/utils/pid.mjs. +export function getCrashLogPath() { + return join(resolveDataDir(), "server", "crash.log"); +} + const PACKAGE_ROOT = join(dirname(fileURLToPath(import.meta.url)), "..", "..", ".."); // Bun needs the Node-compat polyfill preloaded (#9761). The file ships at the // package root via package.json "files" (see scripts/build/pack-artifact-policy.ts) @@ -164,7 +173,8 @@ export class ServerSupervisor { if (aliveMs >= RESTART_RESET_MS) this.restartCount = 0; if (this.restartCount >= this.maxRestarts) { - console.error(`\n⚠ Server crashed ${this.maxRestarts} times in <30s.`); + const summary = `Server crashed ${this.maxRestarts} times in <30s.`; + console.error(`\n⚠ ${summary}`); if (this.onCrashCallback) { const action = this.onCrashCallback(this.crashLog); if (action === "disable-mitm-and-retry") { @@ -175,6 +185,14 @@ export class ServerSupervisor { } } this.dumpCrashLog(); + // #13538: the give-up path used to only console.error() this diagnostic. + // In `--tray`/`--tray-worker` mode this process is launched detached with + // stdio:"ignore" (bin/cli/tray/detachedTray.mjs buildTrayLaunch()), so + // that console output is discarded by the OS and nothing ever explains + // why the tray + gateway disappeared together. Best-effort persist a + // durable record next to the existing per-service PID file convention + // (bin/cli/utils/pid.mjs) so it survives the process exit below. + this.persistCrashLog(summary); process.exit(exitCode ?? 1); return; } @@ -206,6 +224,23 @@ export class ServerSupervisor { console.error("--- End crash log ---\n"); } + // #13538: best-effort append a durable crash record to + // `/server/crash.log`, mirroring the `//.pid` + // layout from bin/cli/utils/pid.mjs. Wrapped in try/catch — this diagnostic + // write must NEVER block or fail shutdown (the give-up branch always calls + // process.exit() right after this). + persistCrashLog(summary) { + try { + const crashLogPath = getCrashLogPath(); + mkdirSync(dirname(crashLogPath), { recursive: true }); + const timestamp = new Date().toISOString(); + const body = [`[${timestamp}] ${summary}`, ...this.crashLog, ""].join("\n"); + appendFileSync(crashLogPath, body, "utf8"); + } catch { + // Best-effort only — a diagnostic write failure must not prevent shutdown. + } + } + stop() { this.isShuttingDown = true; if (this.child?.pid) { diff --git a/changelog.d/fixes/13538-tray-crash-diagnostics.md b/changelog.d/fixes/13538-tray-crash-diagnostics.md new file mode 100644 index 0000000000..d1744b46f0 --- /dev/null +++ b/changelog.d/fixes/13538-tray-crash-diagnostics.md @@ -0,0 +1 @@ +- **fix(cli):** persist the supervisor's give-up crash record to `/server/crash.log` (surfaced by `omniroute doctor`) instead of only printing it — the console output was discarded when `--tray` mode's detached worker exited, leaving no trace of why the gateway/tray disappeared (#13538) — thanks @ProphetOfDoom-PoD diff --git a/tests/unit/fixtures/issue-13538-crashing-server.mjs b/tests/unit/fixtures/issue-13538-crashing-server.mjs new file mode 100644 index 0000000000..28cd3628bd --- /dev/null +++ b/tests/unit/fixtures/issue-13538-crashing-server.mjs @@ -0,0 +1,4 @@ +// Fixture used by tests/unit/issue-13538-tray-crash-diagnostics-lost.test.ts: +// a minimal "server" that always exits non-zero immediately, simulating a +// gateway process that repeatedly crashes under load. +process.exit(1); diff --git a/tests/unit/issue-13538-tray-crash-diagnostics-lost.test.ts b/tests/unit/issue-13538-tray-crash-diagnostics-lost.test.ts new file mode 100644 index 0000000000..77e7d73be2 --- /dev/null +++ b/tests/unit/issue-13538-tray-crash-diagnostics-lost.test.ts @@ -0,0 +1,102 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { existsSync, mkdtempSync, rmSync, readFileSync } from "node:fs"; +import { join, dirname } from "node:path"; +import { tmpdir } from "node:os"; +import { fileURLToPath } from "node:url"; + +// #13538: "Windows native tray gateway exits silently under concurrent OpenCode +// requests" — reporter says the tray worker + gateway both disappear with "no +// graceful shutdown entry" and no diagnostic anywhere. +// +// Root cause (bin/cli/runtime/processSupervisor.mjs handleExit(), give-up +// branch): once the supervised server child crashes >= maxRestarts times +// within RESTART_RESET_MS, the supervisor dumps its crash log with +// console.error() only and calls process.exit(). In `--tray`/`--tray-worker` +// mode this IS the process that owns the tray icon (bin/cli/commands/serve.mjs +// runWithSupervisor()), and on Windows/Linux that whole process is launched +// detached with `stdio: "ignore"` (bin/cli/tray/detachedTray.mjs +// buildTrayLaunch(), already asserted by +// tests/unit/cli/tray-detached.test.ts:44-61). So every console.error the +// give-up path writes is discarded by the OS, and nothing ever persists a +// record of *why* the tray + gateway died — matching the reporter's "no +// graceful shutdown entry and no corresponding Windows Application crash +// event". +// +// This test proves the underlying, platform-independent defect: the give-up +// path never persists its crash log anywhere outside the dying process's own +// (possibly-ignored) stdio. It asserts the desired behavior — a durable +// per-service crash record next to the existing PID file convention +// (bin/cli/utils/pid.mjs getServicePidPath: `//...`) — and +// fails today because no such write exists. + +process.env.PORT = "0"; // waitUntilPortFree no-ops on port 0 (matches cli-process-supervisor.test.ts) + +const HERE = dirname(fileURLToPath(import.meta.url)); +const CRASHING_SERVER = join(HERE, "fixtures", "issue-13538-crashing-server.mjs"); + +test("#13538: supervisor give-up path persists a discoverable crash record (currently lost)", async () => { + const dataDir = mkdtempSync(join(tmpdir(), "omniroute-13538-")); + const origDataDir = process.env.DATA_DIR; + process.env.DATA_DIR = dataDir; + + const { ServerSupervisor } = await import("../../bin/cli/runtime/processSupervisor.mjs"); + + const exits: Array = []; + const origExit = process.exit.bind(process); + // @ts-ignore — stub so the real give-up exit doesn't kill the test runner, + // exactly like tests/unit/cli-process-supervisor.test.ts does. + process.exit = (code?: number) => { + exits.push(code); + }; + + const supervisor = new ServerSupervisor({ + serverPath: CRASHING_SERVER, + env: { ...process.env }, + maxRestarts: 1, + memoryLimit: 64, + }); + + try { + supervisor.start(); + + // Real child process crashes twice: 1st crash restarts (backoff ~1s), 2nd + // crash exhausts maxRestarts=1 and hits the give-up branch. + const deadline = Date.now() + 15_000; + while (exits.length === 0 && Date.now() < deadline) { + await new Promise((r) => setTimeout(r, 100)); + } + + assert.equal(exits.length, 1, "supervisor must give up exactly once"); + assert.equal(exits[0], 1, "give-up exit code must be non-zero (matches production behavior)"); + + // Desired behavior: the give-up path leaves a durable, discoverable record + // of the crash (so a tray worker killed via this path — spawned detached + // with stdio:"ignore" per bin/cli/tray/detachedTray.mjs buildTrayLaunch() + // — is still diagnosable afterward, e.g. by `omniroute doctor`). + const expectedCrashRecord = join(dataDir, "server", "crash.log"); + assert.equal( + existsSync(expectedCrashRecord), + true, + `expected a persisted crash record at ${expectedCrashRecord} after the supervisor gave up, ` + + `but none was written — the only trace (console.error) is discarded when the process is ` + + `spawned with stdio:"ignore" (the exact config used for the detached tray worker on ` + + "Windows/Linux), leaving zero diagnostic trace of why the gateway/tray disappeared (#13538)" + ); + + // The record itself must be useful, not just present: it should carry the + // buffered crash log lines the give-up branch already prints to console. + const contents = readFileSync(expectedCrashRecord, "utf8"); + assert.match( + contents, + /crashed 1 times/, + "crash record must include the give-up summary line, not just an empty file" + ); + } finally { + // @ts-ignore + process.exit = origExit; + if (origDataDir === undefined) delete process.env.DATA_DIR; + else process.env.DATA_DIR = origDataDir; + rmSync(dataDir, { recursive: true, force: true }); + } +});