-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(cli): report an unclean prior proxy exit instead of a silent outage #2861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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"; | ||
|
|
@@ -21,13 +22,17 @@ 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 = { | ||
| schemaVersion: 1; | ||
| proxy: { | ||
| running: boolean; | ||
| pid: number | null; | ||
| /** Persisted owner records outlived their process: the last proxy did not exit cleanly. */ | ||
| staleProcessState: boolean; | ||
|
Comment on lines
+34
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This adds the required public AGENTS.md reference: src/AGENTS.md:L28-L28 Useful? React with 👍 / 👎. |
||
| health: { | ||
| ok: boolean; | ||
| url: string; | ||
|
|
@@ -127,6 +132,69 @@ 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 | ||
| * 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: 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; | ||
| healthRefused: boolean; | ||
| ownerPidAlive: boolean; | ||
| pidRecordBefore: number | null; | ||
| pidRecordAfter: number | null; | ||
| runtimePidBefore: number | null; | ||
| runtimePidAfter: number | null; | ||
| }): boolean { | ||
| if (input.live || input.healthOk) 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; | ||
| 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 | ||
|
|
@@ -171,12 +239,63 @@ async function checkProxyHealth(target: ListenTarget): Promise<HealthCheck> { | |
| 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); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 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; | ||
| port?: number; | ||
| hostname?: string | null; | ||
| }): Promise<boolean> { | ||
| if (input.live) return false; | ||
| const pidRecordBefore = readPidFileValue(); | ||
| 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 ownerPidAfter = pidRecordAfter ?? runtimePidAfter; | ||
| return isUncleanExitEvidence({ | ||
| live: false, | ||
| healthOk: health.ok, | ||
| healthRefused: health.refused === true, | ||
| ownerPidAlive: ownerPidAfter !== null && isProcessAlive(ownerPidAfter), | ||
| pidRecordBefore, | ||
| pidRecordAfter, | ||
| runtimePidBefore, | ||
| runtimePidAfter, | ||
| }); | ||
| } | ||
|
|
||
| export async function collectStatus(): Promise<CliStatusView> { | ||
| const configDiagnostics = readConfigDiagnostics(); | ||
| const config = configDiagnostics.config; | ||
|
|
@@ -210,6 +329,13 @@ export async function collectStatus(): Promise<CliStatusView> { | |
| label: `${listen.healthUrl} ok (live)`, | ||
| } | ||
| : await checkProxyHealth(listen); | ||
| // 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), | ||
| port: config.port, | ||
| hostname: config.hostname, | ||
| }); | ||
| const bunRuntime = durableBunRuntime(); | ||
| const service = diagnoseService(); | ||
| // A service can be registered and still not serve: the manager reports the job | ||
|
|
@@ -326,6 +452,7 @@ export async function collectStatus(): Promise<CliStatusView> { | |
| proxy: { | ||
| running: Boolean(live) || Boolean(pid && health.ok), | ||
| pid: live?.pid ?? pid, | ||
| staleProcessState, | ||
| health: { | ||
| ok: health.ok, | ||
| url: health.url, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Render the stale-state diagnostic for stale PID records.
staleProcessStaterequires a remaining PID or runtime record. A staleocx.pidkeepsstatus.json.proxy.pidtruthy, so the enclosing conditional at Line 873 skips this new block. The end-to-end test intests/cli-status-json.test.tsseedsocx.pidand expects this text, but humanocx statuscannot print it.Include
status.json.proxy.staleProcessStatein the proxy-down condition, or render this diagnostic outside that condition.🤖 Prompt for AI Agents