Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions src/cli/doctor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { accessSync, constants, existsSync, readFileSync } from "node:fs";
import { homedir } from "node:os";
import { dirname, join } from "node:path";
import { getConfigDir, getConfigPath, readConfigDiagnostics, readPid, readRuntimePort, resolveEnvValue } from "../config";
import { findLiveProxy } from "../server/proxy-liveness";
import { gracefulStopHost } from "../lib/process-control";
import { maskAccountId } from "../lib/privacy";
import { loadServiceTokenFromFile } from "../lib/service-secrets";
Expand Down Expand Up @@ -718,9 +719,21 @@ export async function runDoctor(args: string[] = []): Promise<void> {
}
}

// #618: identity-verified liveness first so pid-file absence does not hide a live service.
// Reuse the diagnostics config already loaded above so doctor stays read-only on malformed JSON.
const live = await findLiveProxy({
configFn: () => ({ port: doctorConfig.port, hostname: doctorConfig.hostname }),
});
const livePid = live ? live.pid : readPid();
const liveRuntime = live
? { pid: live.pid ?? 0, port: live.port, hostname: live.hostname }
: (livePid ? readRuntimePort(livePid) : null);

const currentProxyEnv = collectProxyEnv();
const configuredProxy = collectConfiguredProxy();
const runningProxyEnv = collectRunningProxyEnv();
const runningProxyEnv = collectRunningProxyEnv({
readPidFn: () => (live ? live.pid : readPid()),
});

console.log("\nCurrent doctor process proxy env (presence only)");
for (const row of currentProxyEnv) {
Expand All @@ -742,17 +755,10 @@ export async function runDoctor(args: string[] = []): Promise<void> {
}
}

// #314: service-process memory/runtime identity via the authed management
// endpoint. readPid() FIRST (liveness), then the pid-scoped runtime record —
// readRuntimePort alone can serve a stale file pointing at a foreign port.
// Hoisted out of the block below: the Hints section reuses the same liveness pair
// for the proxy-down restart hint.
const livePid = readPid();
const liveRuntime = livePid ? readRuntimePort(livePid) : null;
console.log("\nMemory / runtime");
{
const runtime = liveRuntime;
if (!runtime) {
if (!runtime || !live) {
console.log(` -- doctor process Bun ${Bun.version} (this is NOT the service process)`);
console.log(" -- no running ocx proxy found (no live pid/runtime record)");
} else {
Expand Down Expand Up @@ -815,8 +821,8 @@ export async function runDoctor(args: string[] = []): Promise<void> {
// Hints, not fixes.
const hints: string[] = [];
const proxyDown = proxyDownRestartHint({
proxyRunning: Boolean(livePid && liveRuntime),
port: doctorConfig.port ?? 10100,
proxyRunning: Boolean(live),
port: live?.port ?? doctorConfig.port ?? 10100,
serviceViable: startup.serviceViable,
});
if (proxyDown) hints.push(proxyDown);
Expand Down
60 changes: 47 additions & 13 deletions src/cli/status.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { durableBunRuntime } from "../lib/bun-runtime";
import { codexAutoStartEnabled, getConfigPath, getPidPath, readConfigDiagnostics, readPid, readRuntimePort, type RuntimePortState } from "../config";
import { diagnoseCodexBundledPlugins, type CodexPluginsDiagnostic } from "../codex/plugins-doctor";
import { isOpencodexHealthz, probeHostname } from "../server/proxy-liveness";
import { findLiveProxy, isOpencodexHealthz, probeHostname } from "../server/proxy-liveness";
import type { OcxConfig } from "../types";
import { diagnoseService } from "../service";
import { collectStartupHealth, type StartupHealth } from "../codex/autostart-health";
Expand Down Expand Up @@ -102,6 +102,14 @@ export function selectListenTarget(
};
}

/** Prefer live result (including authoritative null pid) over the on-disk pid file. */
export function resolveStatusPid(
live: { pid: number | null } | null,
pidFile: number | null,
): number | null {
return live ? live.pid : pidFile;
}

async function checkProxyHealth(target: ListenTarget): Promise<HealthCheck> {
const url = target.healthUrl;
const controller = new AbortController();
Expand Down Expand Up @@ -132,9 +140,33 @@ async function checkProxyHealth(target: ListenTarget): Promise<HealthCheck> {
export async function collectStatus(): Promise<CliStatusView> {
const configDiagnostics = readConfigDiagnostics();
const config = configDiagnostics.config;
const pid = readPid();
const listen = selectListenTarget(config, pid, pid ? readRuntimePort(pid) : null);
const health = await checkProxyHealth(listen);
// 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).
const live = await findLiveProxy({
configFn: () => ({ port: config.port, hostname: config.hostname }),
});
const pidFile = readPid();
// Preserve an authoritative null from orphan/legacy liveness — do not restore pidFile.
const pid = resolveStatusPid(live, pidFile);
const listen = live
? {
port: live.port,
hostname: live.hostname,
source: live.source,
healthUrl: `http://${probeHostname(live.hostname)}:${live.port}/healthz`,
dashboardUrl: `http://localhost:${live.port}/`,
}
: selectListenTarget(config, pidFile, pidFile ? readRuntimePort(pidFile) : null);
// findLiveProxy already identity-probed /healthz; avoid a second fetch that can race.
const health = live
? {
ok: true,
url: listen.healthUrl,
message: `ok (pid ${live.pid ?? "unknown"})`,
label: `${listen.healthUrl} ok (live)`,
}
: await checkProxyHealth(listen);
const bunRuntime = durableBunRuntime();
const service = diagnoseService();
const serviceSummary = service.summary;
Expand Down Expand Up @@ -228,22 +260,24 @@ export async function collectStatus(): Promise<CliStatusView> {
runtimeVersion: clampActive ? (lastClamp?.runtimeVersion ?? null) : null,
},
};
const proxyLabel = pid && health.ok
? `running (PID ${pid})`
: pid
? `PID file points to PID ${pid}, but health check failed`
: health.ok
? "reachable, but PID file is missing or stale"
: "not running";
const proxyLabel = live
? `running (PID ${live.pid ?? pid ?? "unknown"})`
: pid && health.ok
? `running (PID ${pid})`
: pid
? `PID file points to PID ${pid}, but health check failed`
: health.ok
? "reachable, but PID file is missing or stale"
: "not running";

return {
proxyLabel,
healthLabel: health.label,
json: {
schemaVersion: 1,
proxy: {
running: Boolean(pid && health.ok),
pid,
running: Boolean(live) || Boolean(pid && health.ok),
pid: live?.pid ?? pid,
health: {
ok: health.ok,
url: health.url,
Expand Down
17 changes: 14 additions & 3 deletions src/server/proxy-liveness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ export interface LiveProxy {
port: number;
/** Raw bind hostname the probe succeeded against; compose URLs via `probeHostname`. */
hostname?: string;
/** Whether the successful probe used runtime-port metadata or the configured listen port. */
source: "runtime" | "config";
}

/**
Expand Down Expand Up @@ -118,7 +120,7 @@ export async function findLiveProxy(io: LivenessIo = {}): Promise<LiveProxy | nu
// healthz confirmed the pid itself → trusted; a pidless legacy body did not,
// so the cheap pid must pass full identity verification before it is returned.
const trusted = identity.pid === pid ? pid : killablePid(pid);
return { pid: trusted, port: runtime.port, hostname: runtime.hostname };
return { pid: trusted, port: runtime.port, hostname: runtime.hostname, source: "runtime" };
}
}
}
Expand All @@ -133,12 +135,21 @@ export async function findLiveProxy(io: LivenessIo = {}): Promise<LiveProxy | nu
// Only the healthz-reported pid is authoritative here. The record's pid may be stale
// (its process dead, the port reused by a pidless legacy proxy) — synthesizing it
// would hand destructive callers (stopProxy → kill fallback) a reusable pid.
if (identity) return { pid: identity.pid ?? null, port: record.port, hostname: record.hostname };
if (identity) {
return { pid: identity.pid ?? null, port: record.port, hostname: record.hostname, source: "runtime" };
}
}

const config = configFn();
const port = config.port ?? 10100;
const identity = await proxyIdentityAt(port, { hostname: config.hostname }, io);
if (identity) return { pid: identity.pid ?? killablePid(pid), port, hostname: config.hostname };
if (identity) {
return {
pid: identity.pid ?? killablePid(pid),
port,
hostname: config.hostname,
source: "config",
};
}
return null;
}
9 changes: 8 additions & 1 deletion tests/cli-status-json.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 { selectListenTarget } from "../src/cli/status";
import { 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");
Expand Down Expand Up @@ -285,6 +285,13 @@ describe("CLI status JSON", () => {
expect(target.dashboardUrl).toBe("http://localhost:58195/");
});

test("resolveStatusPid preserves an authoritative null from live orphan checks", () => {
expect(resolveStatusPid({ pid: null }, 4242)).toBeNull();
expect(resolveStatusPid({ pid: 1111 }, 4242)).toBe(1111);
expect(resolveStatusPid(null, 4242)).toBe(4242);
expect(resolveStatusPid(null, null)).toBeNull();
});
Comment on lines +288 to +293

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Add consumer-level regressions for verified live proxies without a usable PID file.

The new unit test only validates resolveStatusPid; it cannot catch incorrect status JSON/URLs or doctor restart and memory behavior when findLiveProxy() discovers an orphaned runtime record.

  • tests/cli-status-json.test.ts#L288-L293: add a collectStatus() regression with a missing/stale PID file and identity-verified runtime-record proxy; assert running, health/dashboard URLs, resolved PID, fallback port, and listen.source.
  • src/cli/status.ts#L143-L169: keep the status regression focused on the liveness-first branch.
  • src/cli/status.ts#L263-L280: assert the returned JSON reports the verified proxy as running even without a valid PID file.
  • src/cli/doctor.ts#L722-L736: add a doctor regression for the same scenario.
  • src/cli/doctor.ts#L758-L761: assert doctor fetches runtime diagnostics from the verified live port.
  • src/cli/doctor.ts#L824-L825: assert doctor does not emit an unnecessary proxy-restart hint.

As per path instructions, “A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem.”

📍 Affects 3 files
  • tests/cli-status-json.test.ts#L288-L293 (this comment)
  • src/cli/status.ts#L143-L169
  • src/cli/status.ts#L263-L280
  • src/cli/doctor.ts#L722-L736
  • src/cli/doctor.ts#L758-L761
  • src/cli/doctor.ts#L824-L825
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/cli-status-json.test.ts` around lines 288 - 293, Strengthen the
consumer-level regressions for an identity-verified live proxy with a missing or
stale PID file: in tests/cli-status-json.test.ts:288-293, add a collectStatus()
case asserting running state, health/dashboard URLs, resolved PID, fallback
port, and listen.source; keep src/cli/status.ts:143-169 focused on the
liveness-first branch and ensure src/cli/status.ts:263-280 reports the verified
proxy as running. Add the matching doctor regression in
src/cli/doctor.ts:722-736, asserting at src/cli/doctor.ts:758-761 that
diagnostics use the verified live port and at src/cli/doctor.ts:824-825 that no
unnecessary proxy-restart hint is emitted.

Source: Path instructions


test("listen target brackets raw IPv6 hostnames in the health URL", () => {
const target = selectListenTarget(
{ port: 10100, hostname: "::1" },
Expand Down
16 changes: 8 additions & 8 deletions tests/proxy-liveness.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ describe("findLiveProxy", () => {
}) as typeof fetch,
});

expect(live).toEqual({ pid: 4242, port: 58195 });
expect(live).toEqual({ pid: 4242, port: 58195, source: "runtime" });
expect(urls).toEqual(["http://127.0.0.1:58195/healthz"]);
});

Expand All @@ -78,7 +78,7 @@ describe("findLiveProxy", () => {
fetchFn: (async () => healthz(OURS)) as typeof fetch,
});

expect(live).toEqual({ pid: 4242, port: 10100 });
expect(live).toEqual({ pid: 4242, port: 10100, source: "config" });
});

test("a foreign listener on the configured port is not treated as our proxy", async () => {
Expand All @@ -104,7 +104,7 @@ describe("findLiveProxy", () => {
}) as typeof fetch,
});

expect(live).toEqual({ pid: 4242, port: 58195, hostname: "::1" });
expect(live).toEqual({ pid: 4242, port: 58195, hostname: "::1", source: "runtime" });
expect(urls).toEqual(["http://[::1]:58195/healthz"]);
});

Expand All @@ -120,7 +120,7 @@ describe("findLiveProxy", () => {

// The record's pid 1111 may be dead/reused — synthesizing it would let `ocx stop`
// kill an unrelated process via the taskkill/kill fallback.
expect(live).toEqual({ pid: null, port: 58195, hostname: undefined });
expect(live).toEqual({ pid: null, port: 58195, hostname: undefined, source: "runtime" });
});

test("an orphaned record whose healthz pid mismatches is rejected (config fallback still runs)", async () => {
Expand All @@ -145,7 +145,7 @@ describe("findLiveProxy", () => {

// The runtime probe fails the pid check; the config fallback probes the same port
// without a pid expectation and adopts the reported live pid instead.
expect(live).toEqual({ pid: 9999, port: 58195 });
expect(live).toEqual({ pid: 9999, port: 58195, source: "config" });
});

test("a pidless legacy healthz never promotes an unverified cheap pid to a kill target", async () => {
Expand All @@ -158,7 +158,7 @@ describe("findLiveProxy", () => {
fetchFn: (async () => healthz(legacyBody)) as typeof fetch,
});

expect(live).toEqual({ pid: null, port: 58195, hostname: undefined });
expect(live).toEqual({ pid: null, port: 58195, hostname: undefined, source: "runtime" });
});

test("a pidless legacy healthz returns the cheap pid once full identity verification echoes it", async () => {
Expand All @@ -176,7 +176,7 @@ describe("findLiveProxy", () => {
});

expect(verified).toEqual([1111]);
expect(live).toEqual({ pid: 1111, port: 58195, hostname: undefined });
expect(live).toEqual({ pid: 1111, port: 58195, hostname: undefined, source: "runtime" });
});

test("a verifier answering with a DIFFERENT pid than the candidate is rejected (TOCTOU guard)", async () => {
Expand All @@ -189,6 +189,6 @@ describe("findLiveProxy", () => {
fetchFn: (async () => healthz(legacyBody)) as typeof fetch,
});

expect(live).toEqual({ pid: null, port: 58195, hostname: undefined });
expect(live).toEqual({ pid: null, port: 58195, hostname: undefined, source: "runtime" });
});
});
Loading