From 6fb98ee4d3191796e8fab3832c53f70042b33c9d Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 09:54:39 +0900 Subject: [PATCH 1/3] fix(cli): report an unclean prior proxy exit instead of a silent outage A proxy killed by a native trap or SIGKILL never runs the exit cleanup that removes ocx.pid and runtime-port.json, so both records outlive it. That makes a crash distinguishable from a proxy that was never started, and status threw the distinction away: readPid() returns null for a dead pid and the report collapsed to "not running". In #1419 that was the whole user-visible outcome. An unsupervised `ocx gui` proxy died, the dashboard died with it because the same process served it, and nothing ever said a previous process had exited or that installing the service would have restarted it. Adds proxy.staleProcessState, threaded into both `ocx status` and `ocx doctor` through one shared decision helper so the two diagnostics cannot drift. The wording is cause-neutral on purpose: RuntimePortState records only pid, port, hostname and attestation, so the launch mode is unrecoverable and SIGKILL, power loss and a native trap leave identical evidence. Two false positives are excluded, because telling a user their healthy start crashed is worse than staying quiet. A start that publishes records mid-probe is caught by comparing raw records before and after the probes, the same snapshot discipline removePidIfValueIs uses for deletion. A start that has bound the port but not yet published leaves both snapshots identical, so that one is excluded on the port instead: only an unreachable health failure counts, meaning nothing accepted the connection. No watchdog. launchd KeepAlive, systemd Restart=on-failure and the Windows wrapper loop already supervise; a fourth in the CLI would duplicate them and add restart and port-ownership risk. Status and doctor stay read-only. Verification: tests/cli-status-json.test.ts 18 pass, tests/doctor.test.ts 52 pass, typecheck clean. Each guard clause was driven red by mutation, including reverting only the doctor caller while keeping the helper change, which fails the end-to-end test while every helper test stays green. Refs #1419 --- src/cli/doctor.ts | 19 ++++++- src/cli/index.ts | 9 +++ src/cli/status.ts | 101 +++++++++++++++++++++++++++++++++- tests/cli-status-json.test.ts | 79 +++++++++++++++++++++++++- tests/doctor.test.ts | 94 ++++++++++++++++++++++++++++++- 5 files changed, 298 insertions(+), 4 deletions(-) diff --git a/src/cli/doctor.ts b/src/cli/doctor.ts index 8eafd7e26c..f21709106f 100644 --- a/src/cli/doctor.ts +++ b/src/cli/doctor.ts @@ -12,6 +12,7 @@ import { homedir } from "node:os"; import { dirname, join } from "node:path"; import { getConfigDir, getConfigPath, readConfigDiagnostics } from "../config"; import { readPid } from "../config/process-state"; +import { probeUncleanExitState } from "./status"; import { findLiveProxy, type LiveProxy } from "../server/proxy-liveness"; import { BUN_RUNTIME_SOURCES } from "../lib/bun-runtime"; import type { BunRuntimeSource } from "../lib/bun-runtime"; @@ -972,6 +973,11 @@ export function proxyDownRestartHint(input: { /** Absent means "unknown"; the hint then keeps its pre-repair wording. */ serviceInstalled?: boolean; serviceConflict?: boolean; + /** + * Persisted owner records outlived their process (#1419). Cause-neutral: what is on + * disk proves an unclean exit, not which signal caused it. + */ + staleProcessState?: boolean; }): string | null { if (input.proxyRunning) return null; // `serviceViable` alone conflates "no service at all" with "registered but stale or @@ -984,7 +990,10 @@ export function proxyDownRestartHint(input: { : installedButBroken ? "Restart it with 'ocx start', or refresh the installed service: 'ocx service repair'." : "Restart it with 'ocx start', or install the persistent service: 'ocx service install'."; - return `The ocx proxy is not running. Codex/Claude clients pinned to 127.0.0.1:${input.port} fail with errors like "error sending request for url (http://127.0.0.1:${input.port}/v1/responses)". ${restart}`; + const uncleanExit = input.staleProcessState === true + ? "Previous proxy process state remains, so it did not shut down cleanly. " + : ""; + return `The ocx proxy is not running. ${uncleanExit}Codex/Claude clients pinned to 127.0.0.1:${input.port} fail with errors like "error sending request for url (http://127.0.0.1:${input.port}/v1/responses)". ${restart}`; } export async function runDoctor(args: string[] = []): Promise { @@ -1317,6 +1326,14 @@ export async function runDoctor(args: string[] = []): Promise { serviceViable: startup.serviceViable, serviceInstalled: startup.serviceInstalled, serviceConflict: startup.serviceConflict, + // Threaded through the same decision helper `ocx status` uses, so the two + // diagnostics cannot drift. A helper-only change would satisfy a unit test while + // real `ocx doctor` output never mentioned the crash (#1419). + staleProcessState: await probeUncleanExitState({ + live: Boolean(live), + port: doctorConfig.port, + hostname: doctorConfig.hostname, + }), }); if (proxyDown) hints.push(proxyDown); for (const row of providerApiKeys) { diff --git a/src/cli/index.ts b/src/cli/index.ts index 63ba2462dd..3c58ad8d2e 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -872,11 +872,20 @@ async function handleStatus() { } if (!(status.json.proxy.pid || status.json.proxy.health.ok)) { console.log(" ↳ Not running — Codex/Claude requests will fail with connection errors."); + // #1419: the records outliving the process is the only evidence the user gets that a + // proxy died rather than never started. Cause-neutral on purpose: SIGKILL, power loss + // and a native trap are indistinguishable from what is persisted. + if (status.json.proxy.staleProcessState) { + console.log(" Previous proxy process state remains, so it did not shut down cleanly."); + } // The service summary a few lines below already tells a registered-but-not-serving // user to repair. Printing "install the persistent service" unconditionally // contradicted it in the same report, and install re-registers: UAC on Windows and a // possible WinSW-to-scheduler switch for someone who already has a service. const installed = status.json.startup.serviceInstalled && !status.json.startup.serviceConflict; + if (status.json.proxy.staleProcessState && !installed) { + console.log(" No background service was available to restart it; run 'ocx service install'."); + } console.log(installed ? " Restart with 'ocx start', or refresh the installed service: 'ocx service repair'." : " Restart with 'ocx start', or install the persistent service: 'ocx service install'."); diff --git a/src/cli/status.ts b/src/cli/status.ts index d47c55d56c..677c5bac81 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -1,12 +1,13 @@ import { durableBunRuntime } from "../lib/bun-runtime"; import { codexAutoStartEnabled, getConfigPath, readConfigDiagnostics } from "../config"; -import { getPidPath, readPid, readRuntimePort, type RuntimePortState } from "../config/process-state"; +import { getPidPath, readPid, readPidFileValue, readRuntimePort, type RuntimePortState } from "../config/process-state"; import { diagnoseCodexBundledPlugins, type CodexPluginsDiagnostic } from "../codex/plugins-doctor"; import { findLiveProxy, isOpencodexHealthz, probeHostname } from "../server/proxy-liveness"; import { directLocalHttpFetch } from "../server/direct-local-http"; import type { OcxConfig } from "../types"; import { diagnoseService, serviceLogPath } from "../service"; import { collectStartupHealth, type StartupHealth } from "../codex/autostart-health"; +import { isProcessAlive } from "../lib/process-control"; import { getCodexRoutingKind } from "../codex/inject"; import { diagnoseCodexShim } from "../codex/shim"; import { displayCodexRuntimePath, effortClampAppliesToRuntime, loadLastEffortClamp, resolveCodexRuntime } from "../codex/runtime"; @@ -28,6 +29,8 @@ export type CliStatusJson = { proxy: { running: boolean; pid: number | null; + /** Persisted owner records outlived their process: the last proxy did not exit cleanly. */ + staleProcessState: boolean; health: { ok: boolean; url: string; @@ -127,6 +130,47 @@ export function proxyHealthFailureReason(error: unknown, signal: AbortSignal): " : "unreachable"; } +/** + * A proxy killed by a native trap or SIGKILL never runs the exit cleanup that removes + * `ocx.pid` and `runtime-port.json` (only SIGINT/SIGTERM/SIGHUP and normal exit are + * wired to it), so both records outlive it. That makes "crashed" and "never started" + * distinguishable — and #1419 is what it costs when we discard the distinction: the + * reporter's unsupervised `ocx gui` proxy died and every later command said only + * "not running", never that a previous process had exited or that a service would + * have restarted it. + * + * Two races have to stay closed, because a false "it crashed" is worse than a missing + * hint. `handleStart` binds the port BEFORE it publishes either record, so: + * + * - a start that publishes between two reads is caught by comparing the raw records + * observed before and after the probes (the same snapshot discipline + * `removePidIfValueIs` uses for deletion); + * - a start that has bound but not yet published leaves both snapshots identical, so + * records alone cannot see it. That one is excluded on the port instead: only an + * `unreachable` health failure counts, meaning nothing accepted the connection. A + * dead proxy leaves the port free; an in-flight start holds it and either times out + * or answers non-ok. + */ +export function isUncleanExitEvidence(input: { + live: boolean; + healthOk: boolean; + healthMessage: string; + ownerPidAlive: boolean; + pidRecordBefore: number | null; + pidRecordAfter: number | null; + runtimePidBefore: number | null; + runtimePidAfter: number | null; +}): boolean { + if (input.live || input.healthOk) return false; + // Anything other than a refused connection means something is listening: an in-flight + // start, or a foreign process on the port. Neither is evidence that we crashed. + if (input.healthMessage !== "unreachable") return false; + if (input.ownerPidAlive) return false; + if (input.pidRecordBefore !== input.pidRecordAfter) return false; + if (input.runtimePidBefore !== input.runtimePidAfter) return false; + return input.pidRecordAfter !== null || input.runtimePidAfter !== null; +} + /** * `ocx status` greens on process liveness alone, so a proxy that answers * /healthz reads healthy even when Codex is not pointed at it and every routed @@ -177,9 +221,48 @@ async function checkProxyHealth(target: ListenTarget): Promise { } } +/** + * Doctor's gatherer for the same decision. It runs its own health probe because + * `runDoctor` has already resolved liveness by the time it needs this and re-running + * `collectStatus` would repeat every unrelated diagnostic. The DECISION stays in + * `isUncleanExitEvidence` so the two surfaces cannot disagree. + */ +export async function probeUncleanExitState(input: { + live: boolean; + port?: number; + hostname?: string | null; +}): Promise { + if (input.live) return false; + const pidRecordBefore = readPidFileValue(); + const runtimePidBefore = readRuntimePort()?.pid ?? null; + const target = selectListenTarget( + { port: input.port, hostname: input.hostname ?? undefined } as OcxConfig, + pidRecordBefore, + pidRecordBefore ? readRuntimePort(pidRecordBefore) : null, + ); + const health = await checkProxyHealth(target); + const pidRecordAfter = readPidFileValue(); + const runtimePidAfter = readRuntimePort()?.pid ?? null; + const ownerPid = pidRecordAfter ?? runtimePidAfter; + return isUncleanExitEvidence({ + live: false, + healthOk: health.ok, + healthMessage: health.message, + ownerPidAlive: ownerPid !== null && isProcessAlive(ownerPid), + pidRecordBefore, + pidRecordAfter, + runtimePidBefore, + runtimePidAfter, + }); +} + export async function collectStatus(): Promise { const configDiagnostics = readConfigDiagnostics(); const config = configDiagnostics.config; + // Raw owner records BEFORE any probe. Compared against a re-read afterwards so a + // concurrent start that publishes mid-probe cannot be reported as a crash. + const pidRecordBefore = readPidFileValue(); + const runtimePidBefore = readRuntimePort()?.pid ?? null; // Prefer identity-verified liveness (runtime-port + /healthz) over ocx.pid alone (#618). // Pass the already-resolved diagnostics config so findLiveProxy does not re-load and // warn on malformed config.json (status --json must stay stderr-clean). @@ -210,6 +293,21 @@ export async function collectStatus(): Promise { label: `${listen.healthUrl} ok (live)`, } : await checkProxyHealth(listen); + // Re-read the raw records after the probes; equality with the pre-probe snapshot is + // what rules out a start that published while we were probing. + const pidRecordAfter = readPidFileValue(); + const runtimePidAfter = readRuntimePort()?.pid ?? null; + const ownerPid = pidRecordAfter ?? runtimePidAfter; + const staleProcessState = isUncleanExitEvidence({ + live: Boolean(live), + healthOk: health.ok, + healthMessage: health.message, + ownerPidAlive: ownerPid !== null && isProcessAlive(ownerPid), + pidRecordBefore, + pidRecordAfter, + runtimePidBefore, + runtimePidAfter, + }); const bunRuntime = durableBunRuntime(); const service = diagnoseService(); // A service can be registered and still not serve: the manager reports the job @@ -326,6 +424,7 @@ export async function collectStatus(): Promise { proxy: { running: Boolean(live) || Boolean(pid && health.ok), pid: live?.pid ?? pid, + staleProcessState, health: { ok: health.ok, url: health.url, diff --git a/tests/cli-status-json.test.ts b/tests/cli-status-json.test.ts index f959223342..654251908e 100644 --- a/tests/cli-status-json.test.ts +++ b/tests/cli-status-json.test.ts @@ -4,7 +4,7 @@ import { existsSync, mkdtempSync, readdirSync, rmSync, writeFileSync, mkdirSync import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; -import { proxyHealthFailureReason, resolveStatusPid, selectListenTarget } from "../src/cli/status"; +import { isUncleanExitEvidence, proxyHealthFailureReason, resolveStatusPid, selectListenTarget } from "../src/cli/status"; const repoRoot = dirname(fileURLToPath(new URL("../package.json", import.meta.url))); const cliPath = join(repoRoot, "src", "cli", "index.ts"); @@ -326,3 +326,80 @@ describe("CLI status JSON", () => { expect(target.dashboardUrl).toBe("http://localhost:10100/"); }); }); + +/** + * #1419: an unsupervised proxy died from a native trap and every later command said + * only "not running". The persisted owner records are the one piece of evidence that + * separates a crash from a proxy that was never started, and status used to discard + * it. These cases pin the predicate, including the two false-positive shapes that a + * naive implementation gets wrong. + */ +describe("unclean prior exit evidence", () => { + const base = { + live: false, + healthOk: false, + healthMessage: "unreachable", + ownerPidAlive: false, + pidRecordBefore: 4242, + pidRecordAfter: 4242, + runtimePidBefore: 4242, + runtimePidAfter: 4242, + }; + + test("both records outliving a dead owner is an unclean exit", () => { + expect(isUncleanExitEvidence(base)).toBe(true); + }); + + // Blocker 5 from the plan audit: a fixture that always writes BOTH records cannot + // tell an AND from an OR, so each record must be sufficient on its own. + test("a pid record alone is sufficient", () => { + expect(isUncleanExitEvidence({ + ...base, + runtimePidBefore: null, + runtimePidAfter: null, + })).toBe(true); + }); + + test("a runtime-port record alone is sufficient", () => { + expect(isUncleanExitEvidence({ + ...base, + pidRecordBefore: null, + pidRecordAfter: null, + })).toBe(true); + }); + + test("a clean home reports nothing", () => { + expect(isUncleanExitEvidence({ + ...base, + pidRecordBefore: null, + pidRecordAfter: null, + runtimePidBefore: null, + runtimePidAfter: null, + })).toBe(false); + }); + + test("a live proxy or a healthy probe reports nothing", () => { + expect(isUncleanExitEvidence({ ...base, live: true })).toBe(false); + expect(isUncleanExitEvidence({ ...base, healthOk: true })).toBe(false); + }); + + // Re-audit blocker 2: without this case the owner-alive clause is never exercised, + // because every other fixture names a dead pid. + test("a live owner pid is a start in progress, not a crash", () => { + expect(isUncleanExitEvidence({ ...base, ownerPidAlive: true })).toBe(false); + }); + + // Re-audit blocker 1: `handleStart` binds the port before it publishes either + // record, so a start caught in that window leaves both snapshots identical. Only a + // refused connection proves nothing holds the port. + test("a held port is not a crash even when the records look stale", () => { + expect(isUncleanExitEvidence({ ...base, healthMessage: "timed out" })).toBe(false); + expect(isUncleanExitEvidence({ ...base, healthMessage: "returned HTTP 503" })).toBe(false); + expect(isUncleanExitEvidence({ ...base, healthMessage: "responded, but not an opencodex proxy" })).toBe(false); + }); + + test("records published mid-probe suppress the report", () => { + expect(isUncleanExitEvidence({ ...base, pidRecordBefore: null })).toBe(false); + expect(isUncleanExitEvidence({ ...base, runtimePidAfter: 9999 })).toBe(false); + }); +}); diff --git a/tests/doctor.test.ts b/tests/doctor.test.ts index 75b642bba2..2fa57c5683 100644 --- a/tests/doctor.test.ts +++ b/tests/doctor.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdirSync, rmSync, utimesSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, rmSync, utimesSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { homedir, tmpdir } from "node:os"; import { @@ -656,6 +656,43 @@ describe("service memory section (#314 WP4)", () => { const conflict = proxyDownRestartHint({ proxyRunning: false, port: 10100, serviceViable: false, serviceInstalled: true, serviceConflict: true }); expect(conflict).toContain("ocx service install"); }); + + // #1419: the records outliving the process is the only signal the user gets that a + // proxy died rather than never started. Cause-neutral by design — SIGKILL, power + // loss and a native trap leave identical evidence. + test("an unclean prior exit is named before the restart path", () => { + const crashed = proxyDownRestartHint({ + proxyRunning: false, + port: 10100, + serviceViable: false, + serviceInstalled: false, + staleProcessState: true, + }); + expect(crashed).toContain("did not shut down cleanly"); + expect(crashed).toContain("ocx service install"); + + // Absent or false must not invent a crash for a proxy that was never started. + const neverStarted = proxyDownRestartHint({ + proxyRunning: false, + port: 10100, + serviceViable: false, + serviceInstalled: false, + staleProcessState: false, + }); + expect(neverStarted).not.toContain("did not shut down cleanly"); + }); + + test("the unclean-exit wording never asserts a cause", () => { + const hint = proxyDownRestartHint({ + proxyRunning: false, + port: 10100, + serviceViable: false, + staleProcessState: true, + }) ?? ""; + for (const forbidden of ["SIGTRAP", "SIGKILL", "Bun", "crash", "detached"]) { + expect(hint).not.toContain(forbidden); + } + }); }); describe("doctor abandoned response-state temps", () => { @@ -793,3 +830,58 @@ describe("doctor reclaim wiring (end to end)", () => { expect(logged.join("\n")).toContain("Unrecognized flag"); }); }); + +/** + * The wiring test, and the reason a helper-only assertion was rejected during plan + * review: `proxyDownRestartHint` can accept `staleProcessState` and stay green while + * `runDoctor` never passes it, leaving real `ocx doctor` output unchanged. This drives + * the actual command against a home holding a dead owner record. + */ +describe("doctor reports an unclean prior proxy exit", () => { + let tempHome: string; + let previousHome: string | undefined; + let logged: string[]; + const realLog = console.log; + + beforeEach(() => { + tempHome = mkdtempSync(join(tmpdir(), "ocx-doctor-unclean-")); + previousHome = process.env.OPENCODEX_HOME; + process.env.OPENCODEX_HOME = tempHome; + logged = []; + console.log = (...args: unknown[]) => { logged.push(args.map(String).join(" ")); }; + }); + + afterEach(() => { + console.log = realLog; + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + rmSync(tempHome, { recursive: true, force: true }); + }); + + const deadPid = (): number => (process.pid === 4242 ? 4243 : 4242); + + // Port 9 is the discard port: nothing listens, so the health probe is refused rather + // than timing out, which is what the predicate requires. + const seedConfig = (): void => { + writeFileSync(join(tempHome, "config.json"), JSON.stringify({ port: 9, codexAutoStart: false }), "utf8"); + }; + + test("a dead owner record surfaces the unclean-exit diagnosis", async () => { + seedConfig(); + const pid = deadPid(); + writeFileSync(join(tempHome, "ocx.pid"), String(pid), "utf8"); + writeFileSync(join(tempHome, "runtime-port.json"), JSON.stringify({ pid, port: 9, hostname: "127.0.0.1" }), "utf8"); + + await runDoctor([]); + + expect(logged.join("\n")).toContain("did not shut down cleanly"); + }); + + test("a clean home never claims a prior crash", async () => { + seedConfig(); + + await runDoctor([]); + + expect(logged.join("\n")).not.toContain("did not shut down cleanly"); + }); +}); From 33cf83cb569bdc34ec5b42a9f151136852413684 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 10:17:09 +0900 Subject: [PATCH 2/3] fix(cli): tighten the stale-record evidence after review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review found the first cut claimed more than it could prove and tested less than it appeared to. The tests were vacuous where it mattered most: replacing the returned staleProcessState with a constant false left all 70 assertions green, because every case exercised the predicate in isolation and none drove the CLI. Adds command-level tests through `ocx status --json` and human output. That mutation now fails. `unreachable` was too broad for the question being asked. It covers every non-abort failure including a socket that is accepted and then reset, which is exactly what an in-flight bind looks like — review reproduced a stale verdict against a listener that accepted and reset. Only a connect-phase ECONNREFUSED now counts, read from the errno chain rather than a message substring. Status and doctor could reach opposite verdicts about the same disk state. After a fallback-port crash or a config port change, status probed the configured port while doctor probed the recorded one. Both now go through one gatherer that probes the port named by the stale record, since that is the only port that can answer whether the process which wrote the record is gone. The wording overclaimed. Shutdown cleanup ignores unlink failures and the records carry no session provenance, so a clean exit whose unlink failed is indistinguishable from a crash. "did not shut down cleanly" became "stale process records remain, so the previous run may have exited unexpectedly", and the duplicated service-install line is gone. Verification: tests/cli-status-json.test.ts 23 pass, tests/doctor.test.ts 52 pass, typecheck clean. Three mutations driven red — constant false, probing the configured port, and accepting any non-abort failure. The fallback-port test needed an occupied configured port to discriminate at all; with both ports free it passed against the wrong implementation, which is the same vacuity again one layer down. Refs #1419 --- src/cli/doctor.ts | 2 +- src/cli/index.ts | 18 ++--- src/cli/status.ts | 108 +++++++++++++++++----------- tests/cli-status-json.test.ts | 129 ++++++++++++++++++++++++++++++++-- tests/doctor.test.ts | 20 ++++-- 5 files changed, 218 insertions(+), 59 deletions(-) diff --git a/src/cli/doctor.ts b/src/cli/doctor.ts index f21709106f..af840b950f 100644 --- a/src/cli/doctor.ts +++ b/src/cli/doctor.ts @@ -991,7 +991,7 @@ export function proxyDownRestartHint(input: { ? "Restart it with 'ocx start', or refresh the installed service: 'ocx service repair'." : "Restart it with 'ocx start', or install the persistent service: 'ocx service install'."; const uncleanExit = input.staleProcessState === true - ? "Previous proxy process state remains, so it did not shut down cleanly. " + ? "Stale process records remain, so the previous run may have exited unexpectedly. " : ""; return `The ocx proxy is not running. ${uncleanExit}Codex/Claude clients pinned to 127.0.0.1:${input.port} fail with errors like "error sending request for url (http://127.0.0.1:${input.port}/v1/responses)". ${restart}`; } diff --git a/src/cli/index.ts b/src/cli/index.ts index 3c58ad8d2e..284a8b15d8 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -872,19 +872,21 @@ async function handleStatus() { } if (!(status.json.proxy.pid || status.json.proxy.health.ok)) { console.log(" ↳ Not running — Codex/Claude requests will fail with connection errors."); - // #1419: the records outliving the process is the only evidence the user gets that a - // proxy died rather than never started. Cause-neutral on purpose: SIGKILL, power loss - // and a native trap are indistinguishable from what is persisted. - if (status.json.proxy.staleProcessState) { - console.log(" Previous proxy process state remains, so it did not shut down cleanly."); - } // The service summary a few lines below already tells a registered-but-not-serving // user to repair. Printing "install the persistent service" unconditionally // contradicted it in the same report, and install re-registers: UAC on Windows and a // possible WinSW-to-scheduler switch for someone who already has a service. const installed = status.json.startup.serviceInstalled && !status.json.startup.serviceConflict; - if (status.json.proxy.staleProcessState && !installed) { - console.log(" No background service was available to restart it; run 'ocx service install'."); + // #1419: the records outliving the process is the only evidence the user gets that a + // previous run ended without cleanup. Deliberately hedged and cause-neutral — cleanup + // ignores unlink failures and the records carry no session provenance, so this cannot + // prove a crash, only that the last run left state behind. The restart advice below is + // not repeated here; one recommendation per report. + if (status.json.proxy.staleProcessState) { + console.log(" Stale process records remain, so the previous run may have exited unexpectedly."); + if (!installed) { + console.log(" No background service was available to restart it."); + } } console.log(installed ? " Restart with 'ocx start', or refresh the installed service: 'ocx service repair'." diff --git a/src/cli/status.ts b/src/cli/status.ts index 677c5bac81..82d588eb92 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -22,6 +22,8 @@ type HealthCheck = { url: string; message: string; label: string; + /** True only for a connect-phase refusal: proof that nothing holds the port. */ + refused?: boolean; }; export type CliStatusJson = { @@ -130,6 +132,24 @@ export function proxyHealthFailureReason(error: unknown, signal: AbortSignal): " : "unreachable"; } +/** + * "Nothing is listening" is narrower than "the probe failed". `unreachable` covers every + * non-abort failure, including a socket that was ACCEPTED and then reset — which is what + * an in-flight start looks like mid-bind. Only a connect-phase refusal proves the port is + * free, so this reads the underlying errno instead of the display string. + */ +export function isConnectionRefused(error: unknown): boolean { + for (let current: unknown = error, depth = 0; current instanceof Error && depth < 4; depth++) { + const code = (current as { code?: unknown }).code; + if (code === "ECONNREFUSED" || code === "ConnectionRefused") return true; + // Bun surfaces the refusal as a plain message on some platforms; the errno name is + // still the discriminator, not a substring of arbitrary prose. + if (typeof code === "string" && code.endsWith("ECONNREFUSED")) return true; + current = (current as { cause?: unknown }).cause; + } + return false; +} + /** * A proxy killed by a native trap or SIGKILL never runs the exit cleanup that removes * `ocx.pid` and `runtime-port.json` (only SIGINT/SIGTERM/SIGHUP and normal exit are @@ -146,15 +166,21 @@ export function proxyHealthFailureReason(error: unknown, signal: AbortSignal): " * observed before and after the probes (the same snapshot discipline * `removePidIfValueIs` uses for deletion); * - a start that has bound but not yet published leaves both snapshots identical, so - * records alone cannot see it. That one is excluded on the port instead: only an - * `unreachable` health failure counts, meaning nothing accepted the connection. A - * dead proxy leaves the port free; an in-flight start holds it and either times out - * or answers non-ok. + * records alone cannot see it. That one is excluded on the port instead: the probe + * must have been REFUSED at connect, which is the only outcome proving nothing holds + * the port. A socket that is accepted and then reset — an in-flight bind — is not a + * refusal, so review caught `unreachable` being too broad for this job. + * + * What this can and cannot prove: the records outliving their process establish that the + * previous run did not complete its cleanup. It does not establish a cause, and it cannot + * fully exclude a clean exit whose `unlinkSync` failed, because cleanup ignores that + * error (`src/cli/index.ts:324-325`) and the records carry no session provenance. The + * wording therefore says the records remain and the run MAY have exited unexpectedly. */ export function isUncleanExitEvidence(input: { live: boolean; healthOk: boolean; - healthMessage: string; + healthRefused: boolean; ownerPidAlive: boolean; pidRecordBefore: number | null; pidRecordAfter: number | null; @@ -162,9 +188,7 @@ export function isUncleanExitEvidence(input: { runtimePidAfter: number | null; }): boolean { if (input.live || input.healthOk) return false; - // Anything other than a refused connection means something is listening: an in-flight - // start, or a foreign process on the port. Neither is evidence that we crashed. - if (input.healthMessage !== "unreachable") return false; + if (!input.healthRefused) return false; if (input.ownerPidAlive) return false; if (input.pidRecordBefore !== input.pidRecordAfter) return false; if (input.runtimePidBefore !== input.runtimePidAfter) return false; @@ -215,17 +239,24 @@ async function checkProxyHealth(target: ListenTarget): Promise { return { ok: true, url, message, label: `${url} ${message}` }; } catch (error) { const reason = proxyHealthFailureReason(error, controller.signal); - return { ok: false, url, message: reason, label: `${url} ${reason}` }; + return { ok: false, url, message: reason, label: `${url} ${reason}`, refused: isConnectionRefused(error) }; } finally { clearTimeout(timer); } } /** - * Doctor's gatherer for the same decision. It runs its own health probe because - * `runDoctor` has already resolved liveness by the time it needs this and re-running - * `collectStatus` would repeat every unrelated diagnostic. The DECISION stays in - * `isUncleanExitEvidence` so the two surfaces cannot disagree. + * The ONE evidence gatherer for stale-process state, shared by `ocx status` and + * `ocx doctor`. + * + * It deliberately probes the port named by the STALE RECORD, not the configured display + * port. Review found the two commands disagreeing precisely here: a proxy that hopped to + * a fallback port, or a config whose port changed after the crash, left status probing + * the configured port while doctor probed the recorded one, so one reported a crash and + * the other did not. The question being asked is "is the process that wrote this record + * gone?", and only that record's own port can answer it. + * + * `live` short-circuits before the probe so a healthy install pays nothing. */ export async function probeUncleanExitState(input: { live: boolean; @@ -234,21 +265,30 @@ export async function probeUncleanExitState(input: { }): Promise { if (input.live) return false; const pidRecordBefore = readPidFileValue(); - const runtimePidBefore = readRuntimePort()?.pid ?? null; - const target = selectListenTarget( - { port: input.port, hostname: input.hostname ?? undefined } as OcxConfig, - pidRecordBefore, - pidRecordBefore ? readRuntimePort(pidRecordBefore) : null, - ); - const health = await checkProxyHealth(target); + const runtimeBefore = readRuntimePort(); + const runtimePidBefore = runtimeBefore?.pid ?? null; + if (pidRecordBefore === null && runtimePidBefore === null) return false; + const ownerPid = pidRecordBefore ?? runtimePidBefore; + if (ownerPid !== null && isProcessAlive(ownerPid)) return false; + // The recorded port is the evidence target. Fall back to the configured port only when + // no runtime record exists, which is the pid-file-only case. + const port = runtimeBefore?.port ?? input.port ?? 10100; + const hostname = runtimeBefore?.hostname ?? input.hostname ?? undefined; + const health = await checkProxyHealth({ + port, + hostname, + source: runtimeBefore ? "runtime" : "config", + healthUrl: `http://${probeHostname(hostname)}:${port}/healthz`, + dashboardUrl: `http://localhost:${port}/`, + }); const pidRecordAfter = readPidFileValue(); const runtimePidAfter = readRuntimePort()?.pid ?? null; - const ownerPid = pidRecordAfter ?? runtimePidAfter; + const ownerPidAfter = pidRecordAfter ?? runtimePidAfter; return isUncleanExitEvidence({ live: false, healthOk: health.ok, - healthMessage: health.message, - ownerPidAlive: ownerPid !== null && isProcessAlive(ownerPid), + healthRefused: health.refused === true, + ownerPidAlive: ownerPidAfter !== null && isProcessAlive(ownerPidAfter), pidRecordBefore, pidRecordAfter, runtimePidBefore, @@ -259,10 +299,6 @@ export async function probeUncleanExitState(input: { export async function collectStatus(): Promise { const configDiagnostics = readConfigDiagnostics(); const config = configDiagnostics.config; - // Raw owner records BEFORE any probe. Compared against a re-read afterwards so a - // concurrent start that publishes mid-probe cannot be reported as a crash. - const pidRecordBefore = readPidFileValue(); - const runtimePidBefore = readRuntimePort()?.pid ?? null; // Prefer identity-verified liveness (runtime-port + /healthz) over ocx.pid alone (#618). // Pass the already-resolved diagnostics config so findLiveProxy does not re-load and // warn on malformed config.json (status --json must stay stderr-clean). @@ -293,20 +329,12 @@ export async function collectStatus(): Promise { label: `${listen.healthUrl} ok (live)`, } : await checkProxyHealth(listen); - // Re-read the raw records after the probes; equality with the pre-probe snapshot is - // what rules out a start that published while we were probing. - const pidRecordAfter = readPidFileValue(); - const runtimePidAfter = readRuntimePort()?.pid ?? null; - const ownerPid = pidRecordAfter ?? runtimePidAfter; - const staleProcessState = isUncleanExitEvidence({ + // Same gatherer `ocx doctor` uses, so the two commands cannot reach different verdicts + // about the same on-disk state (review found them diverging on fallback ports). + const staleProcessState = await probeUncleanExitState({ live: Boolean(live), - healthOk: health.ok, - healthMessage: health.message, - ownerPidAlive: ownerPid !== null && isProcessAlive(ownerPid), - pidRecordBefore, - pidRecordAfter, - runtimePidBefore, - runtimePidAfter, + port: config.port, + hostname: config.hostname, }); const bunRuntime = durableBunRuntime(); const service = diagnoseService(); diff --git a/tests/cli-status-json.test.ts b/tests/cli-status-json.test.ts index 654251908e..c88b52db7e 100644 --- a/tests/cli-status-json.test.ts +++ b/tests/cli-status-json.test.ts @@ -1,10 +1,12 @@ import { describe, expect, test } from "bun:test"; import { spawnSync } from "node:child_process"; import { existsSync, mkdtempSync, readdirSync, rmSync, writeFileSync, mkdirSync } from "node:fs"; +import { createServer } from "node:net"; +import type { AddressInfo } from "node:net"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; -import { isUncleanExitEvidence, proxyHealthFailureReason, resolveStatusPid, selectListenTarget } from "../src/cli/status"; +import { isConnectionRefused, isUncleanExitEvidence, proxyHealthFailureReason, resolveStatusPid, selectListenTarget } from "../src/cli/status"; const repoRoot = dirname(fileURLToPath(new URL("../package.json", import.meta.url))); const cliPath = join(repoRoot, "src", "cli", "index.ts"); @@ -338,7 +340,7 @@ describe("unclean prior exit evidence", () => { const base = { live: false, healthOk: false, - healthMessage: "unreachable", + healthRefused: true, ownerPidAlive: false, pidRecordBefore: 4242, pidRecordAfter: 4242, @@ -393,13 +395,130 @@ describe("unclean prior exit evidence", () => { // record, so a start caught in that window leaves both snapshots identical. Only a // refused connection proves nothing holds the port. test("a held port is not a crash even when the records look stale", () => { - expect(isUncleanExitEvidence({ ...base, healthMessage: "timed out" })).toBe(false); - expect(isUncleanExitEvidence({ ...base, healthMessage: "returned HTTP 503" })).toBe(false); - expect(isUncleanExitEvidence({ ...base, healthMessage: "responded, but not an opencodex proxy" })).toBe(false); + expect(isUncleanExitEvidence({ ...base, healthRefused: false })).toBe(false); }); test("records published mid-probe suppress the report", () => { expect(isUncleanExitEvidence({ ...base, pidRecordBefore: null })).toBe(false); expect(isUncleanExitEvidence({ ...base, runtimePidAfter: 9999 })).toBe(false); }); + + // Review blocker 2: `unreachable` covers every non-abort failure, including a socket + // that is ACCEPTED and then reset — which is what an in-flight bind looks like. Only a + // connect-phase refusal proves the port is free. + test("only a connect-phase refusal counts as nothing listening", () => { + const refused = Object.assign(new Error("connect ECONNREFUSED 127.0.0.1:9"), { code: "ECONNREFUSED" }); + expect(isConnectionRefused(refused)).toBe(true); + + const nested = new Error("fetch failed", { cause: refused }); + expect(isConnectionRefused(nested)).toBe(true); + + const reset = Object.assign(new Error("socket hang up"), { code: "ECONNRESET" }); + expect(isConnectionRefused(reset)).toBe(false); + + // A message that merely mentions refusal without the errno is not evidence. + expect(isConnectionRefused(new Error("connection refused by policy"))).toBe(false); + expect(isConnectionRefused(undefined)).toBe(false); + }); +}); + +/** + * Command-level coverage. Review found the unit tests above were satisfiable by an + * implementation that never reported anything: replacing the returned + * `staleProcessState` with a constant `false` left every predicate test green. These + * drive the real CLI, so the field has to travel from disk to output. + */ +describe("status reports stale process records end to end", () => { + const seed = (home: string, opts: { pid?: number; runtime?: boolean; port: number }): void => { + writeFileSync(join(home, "config.json"), JSON.stringify({ port: opts.port, codexAutoStart: false }), "utf8"); + const pid = opts.pid ?? (process.pid === 4242 ? 4243 : 4242); + if (opts.pid !== 0) writeFileSync(join(home, "ocx.pid"), String(pid), "utf8"); + if (opts.runtime) { + writeFileSync(join(home, "runtime-port.json"), JSON.stringify({ pid, port: opts.port, hostname: "127.0.0.1" }), "utf8"); + } + }; + + // A port nothing binds, so the probe is refused rather than accepted-then-reset. + const freePort = 9; + + test("a dead owner record surfaces in --json and in human output", () => { + const home = mkdtempSync(join(tmpdir(), "ocx-stale-json-")); + try { + seed(home, { runtime: true, port: freePort }); + + const json = runStatusJson(home); + expect(json.status).toBe(0); + const parsed = JSON.parse(json.stdout) as { proxy?: { staleProcessState?: unknown } }; + expect(parsed.proxy?.staleProcessState).toBe(true); + + const human = spawnSync(process.execPath, [cliPath, "status"], { + cwd: repoRoot, + env: { ...process.env, OPENCODEX_HOME: home }, + encoding: "utf8", + }); + expect(human.stdout).toContain("may have exited unexpectedly"); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + + test("a clean home reports false and says nothing about a previous run", () => { + const home = mkdtempSync(join(tmpdir(), "ocx-stale-clean-")); + try { + writeFileSync(join(home, "config.json"), JSON.stringify({ port: freePort, codexAutoStart: false }), "utf8"); + + const json = runStatusJson(home); + const parsed = JSON.parse(json.stdout) as { proxy?: { staleProcessState?: unknown } }; + expect(parsed.proxy?.staleProcessState).toBe(false); + + const human = spawnSync(process.execPath, [cliPath, "status"], { + cwd: repoRoot, + env: { ...process.env, OPENCODEX_HOME: home }, + encoding: "utf8", + }); + expect(human.stdout).not.toContain("may have exited unexpectedly"); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + + // Review blocker 3: a recycled pid must suppress rather than assert. This process is + // certainly alive, so recording it stands in for a reused pid. + test("a record naming a live pid is never reported as a stale exit", () => { + const home = mkdtempSync(join(tmpdir(), "ocx-stale-livepid-")); + try { + seed(home, { pid: process.pid, runtime: true, port: freePort }); + + const parsed = JSON.parse(runStatusJson(home).stdout) as { proxy?: { staleProcessState?: unknown } }; + expect(parsed.proxy?.staleProcessState).toBe(false); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + + // Review blocker 4: status and doctor disagreed when the recorded port differed from + // the configured one. Both now probe the recorded port, so both must agree. + // + // The configured port must be OCCUPIED for this to discriminate: if both ports are + // simply free, probing either one yields the same refusal and the test cannot tell the + // two implementations apart. A listener that accepts and resets is what an in-flight + // bind looks like, so a run that probed the configured port would suppress the report. + test("a fallback-port record is judged on the recorded port, not the configured one", async () => { + const home = mkdtempSync(join(tmpdir(), "ocx-stale-fallback-")); + const occupied = createServer(socket => { socket.destroy(); }); + await new Promise(resolve => { occupied.listen(0, "127.0.0.1", () => resolve()); }); + const occupiedPort = (occupied.address() as AddressInfo).port; + try { + const pid = process.pid === 4242 ? 4243 : 4242; + writeFileSync(join(home, "config.json"), JSON.stringify({ port: occupiedPort, codexAutoStart: false }), "utf8"); + writeFileSync(join(home, "ocx.pid"), String(pid), "utf8"); + writeFileSync(join(home, "runtime-port.json"), JSON.stringify({ pid, port: freePort, hostname: "127.0.0.1" }), "utf8"); + + const parsed = JSON.parse(runStatusJson(home).stdout) as { proxy?: { staleProcessState?: unknown } }; + expect(parsed.proxy?.staleProcessState).toBe(true); + } finally { + await new Promise(resolve => { occupied.close(() => resolve()); }); + rmSync(home, { recursive: true, force: true }); + } + }); }); diff --git a/tests/doctor.test.ts b/tests/doctor.test.ts index 2fa57c5683..903220096c 100644 --- a/tests/doctor.test.ts +++ b/tests/doctor.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { spawnSync } from "node:child_process"; import { existsSync, mkdirSync, mkdtempSync, rmSync, utimesSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { homedir, tmpdir } from "node:os"; @@ -668,7 +669,7 @@ describe("service memory section (#314 WP4)", () => { serviceInstalled: false, staleProcessState: true, }); - expect(crashed).toContain("did not shut down cleanly"); + expect(crashed).toContain("may have exited unexpectedly"); expect(crashed).toContain("ocx service install"); // Absent or false must not invent a crash for a proxy that was never started. @@ -679,7 +680,7 @@ describe("service memory section (#314 WP4)", () => { serviceInstalled: false, staleProcessState: false, }); - expect(neverStarted).not.toContain("did not shut down cleanly"); + expect(neverStarted).not.toContain("may have exited unexpectedly"); }); test("the unclean-exit wording never asserts a cause", () => { @@ -858,7 +859,16 @@ describe("doctor reports an unclean prior proxy exit", () => { rmSync(tempHome, { recursive: true, force: true }); }); - const deadPid = (): number => (process.pid === 4242 ? 4243 : 4242); + /** + * A pid that is certainly dead: spawn a process, wait for it to exit, then reuse its + * number. A hardcoded constant can belong to an unrelated live process on a busy + * machine, which would silently invert this fixture. + */ + const deadPid = (): number => { + const spawned = spawnSync(process.execPath, ["-e", ""], { encoding: "utf8" }); + const pid = spawned.pid; + return typeof pid === "number" && pid > 0 ? pid : (process.pid === 4242 ? 4243 : 4242); + }; // Port 9 is the discard port: nothing listens, so the health probe is refused rather // than timing out, which is what the predicate requires. @@ -874,7 +884,7 @@ describe("doctor reports an unclean prior proxy exit", () => { await runDoctor([]); - expect(logged.join("\n")).toContain("did not shut down cleanly"); + expect(logged.join("\n")).toContain("may have exited unexpectedly"); }); test("a clean home never claims a prior crash", async () => { @@ -882,6 +892,6 @@ describe("doctor reports an unclean prior proxy exit", () => { await runDoctor([]); - expect(logged.join("\n")).not.toContain("did not shut down cleanly"); + expect(logged.join("\n")).not.toContain("may have exited unexpectedly"); }); }); From 978688d22fe52833776f90414ffaae7634660c0c Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 10:32:54 +0900 Subject: [PATCH 3/3] test(cli): reserve a real free port for the stale-record fixtures Port 9 is conventionally unused but not guaranteed. If anything answers on it the probe is accepted rather than refused, which silently inverts every fixture that depends on a refusal. Bind an ephemeral port, read it, release it. Raised as a non-blocking finding during review of #2861. tests/cli-status-json.test.ts 23 pass. --- tests/cli-status-json.test.ts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/cli-status-json.test.ts b/tests/cli-status-json.test.ts index c88b52db7e..d6bc9e93ab 100644 --- a/tests/cli-status-json.test.ts +++ b/tests/cli-status-json.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, test } from "bun:test"; +import { beforeAll, describe, expect, test } from "bun:test"; import { spawnSync } from "node:child_process"; import { existsSync, mkdtempSync, readdirSync, rmSync, writeFileSync, mkdirSync } from "node:fs"; import { createServer } from "node:net"; @@ -438,8 +438,18 @@ describe("status reports stale process records end to end", () => { } }; - // A port nothing binds, so the probe is refused rather than accepted-then-reset. - const freePort = 9; + /** + * A port that is genuinely free: bind an ephemeral port, read it, release it. The + * discard port 9 is conventionally unused but not guaranteed, and if anything answers + * on it the probe is accepted rather than refused and these fixtures invert. + */ + let freePort = 9; + beforeAll(async () => { + const probe = createServer(); + await new Promise(resolve => { probe.listen(0, "127.0.0.1", () => resolve()); }); + freePort = (probe.address() as AddressInfo).port; + await new Promise(resolve => { probe.close(() => resolve()); }); + }); test("a dead owner record surfaces in --json and in human output", () => { const home = mkdtempSync(join(tmpdir(), "ocx-stale-json-"));