diff --git a/src/cli/index.ts b/src/cli/index.ts index d59f9e3257..a92787c8c2 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -952,7 +952,7 @@ async function handleStop() { if (detail) console.error(` ${detail}`); if (err instanceof ProxyOwnershipRefusedError) { ownershipBlocked = true; - console.error(" Skipping shared teardown (native Codex restore, Grok config): the foreign proxy is still running."); + console.error(" Skipping shared teardown (native Codex restore, Grok config): the refusing proxy is still running."); } } } else { @@ -979,7 +979,7 @@ async function handleStop() { if (detail) console.error(` ${detail}`); if (err instanceof ProxyOwnershipRefusedError) { ownershipBlocked = true; - console.error(" Skipping shared teardown (native Codex restore, Grok config): the foreign proxy is still running."); + console.error(" Skipping shared teardown (native Codex restore, Grok config): the refusing proxy is still running."); } } } else if (live) { diff --git a/src/lib/process-control.ts b/src/lib/process-control.ts index 49da43987c..6d699be8fd 100644 --- a/src/lib/process-control.ts +++ b/src/lib/process-control.ts @@ -80,11 +80,49 @@ export type GracefulStopResult = boolean | "refused" | "teardown-unconfirmed"; */ let lastRefusalMessage: string | null = null; +/** + * The server's machine-readable reason for the most recent 409, captured alongside the + * message so a refusal that arrives without a body still names the right cause. Without it + * the fallback has to guess, and guessing "ownership" sent operators to re-check + * CODEX_HOME for a refusal the scheduler wrapper had issued (#4169). + */ +let lastRefusalCode: string | null = null; + /** The server's explanation for the most recent 409, or `null` when it sent none. */ export function lastStopRefusalMessage(): string | null { return lastRefusalMessage; } +/** The server's `code` for the most recent 409, or `null` when it sent none. */ +export function lastStopRefusalCode(): string | null { + return lastRefusalCode; +} + +/** + * Wording for a refusal whose body carried no message. Each branch mirrors a refusal the + * management API can return from `POST /api/stop`; the default stays cause-neutral because + * naming the wrong cause is worse than naming none — it costs the operator the time they + * spend acting on it. + */ +function refusalFallbackMessage(code: string | null): string { + switch (code) { + case "respawnable_service": + return "The running proxy refused to stop: a service manager that can respawn it owns " + + "the process. Run `ocx stop`, which verifies the respawn window."; + case "self_unload_service": + return "The running proxy refused to stop: it is the installed service itself, so " + + "stopping the manager from inside it would end the process before native Codex is " + + "restored. Run `ocx stop`."; + case "service_state_unknown": + return "The running proxy refused to stop: the service manager state could not be read, " + + "so it cannot tell whether a wrapper would respawn it. Run `ocx service status` to " + + "see the query error."; + default: + return "The running proxy refused to stop and sent no reason. Run `ocx service status` " + + "to inspect the service state."; + } +} + /** * A proxy declined shutdown (HTTP 409). There is more than one reason it can say no — a * scheduler wrapper under another home, or the proxy being the installed service itself @@ -104,9 +142,28 @@ export class ProxyOwnershipRefusedError extends Error {} * attest the process exit code or completion of every drain/shutdown hook. */ export async function stopProxyGracefully(pid: number, io: GracefulStopIo = {}): Promise { + return (await stopProxyGracefullyDetailed(pid, io)).result; +} + +/** + * The refusal a single stop attempt received, carried back to that attempt's caller. + * + * Module-scoped state cannot do this job: two overlapping stops race, and the first would + * report the second's cause. The exported accessors stay as observational state for callers + * that only want the last refusal, but the error text is built from this per-call value. + */ +type StopRefusal = { message: string | null; code: string | null }; + +async function stopProxyGracefullyDetailed( + pid: number, + io: GracefulStopIo = {}, +): Promise<{ result: GracefulStopResult; refusal: StopRefusal }> { + const refusal: StopRefusal = { message: null, code: null }; + const done = (result: GracefulStopResult): { result: GracefulStopResult; refusal: StopRefusal } => + ({ result, refusal }); const readRuntime = io.readRuntime ?? readRuntimePort; const runtime = io.runtimeEndpoint ?? readRuntime(pid); - if (!runtime?.port) return false; + if (!runtime?.port) return done(false); const env = io.env ?? process.env; const headers: Record = {}; const token = configuredAdminToken(env.OPENCODEX_HOME?.trim() || undefined, env as NodeJS.ProcessEnv); @@ -128,20 +185,31 @@ export async function stopProxyGracefully(pid: number, io: GracefulStopIo = {}): // longer than a health poll so we prefer drain over taskkill /F. signal: AbortSignal.timeout(io.exitTimeoutMs ? Math.min(io.exitTimeoutMs, 10_000) : 10_000), }); - // 409 is the proxy REFUSING to stop (a service installed under another home owns it and - // would respawn it anyway). That is a policy answer, not a dead endpoint — escalating to - // SIGTERM here would run the daemon's cleanup and strip shared config out from under the + // 409 is the proxy REFUSING to stop. There is more than one reason it can say no — a + // respawning service manager, the proxy being the installed service itself, or an + // unreadable scheduler state — so both the message and the code are captured rather + // than assumed. That is a policy answer, not a dead endpoint — escalating to SIGTERM + // here would run the daemon's cleanup and strip shared config out from under the // still-running service. Report the refusal instead of forcing. if (res.status === 409) { - lastRefusalMessage = await res.json() + const parsed = await res.json() .then(body => { - const message = (body as { message?: unknown } | null)?.message; - return typeof message === "string" && message.trim() ? message.trim() : null; + const record = body as { message?: unknown; code?: unknown } | null; + const message = record?.message; + const code = record?.code; + return { + message: typeof message === "string" && message.trim() ? message.trim() : null, + code: typeof code === "string" && code.trim() ? code.trim() : null, + }; }) - .catch(() => null); - return "refused"; + .catch(() => ({ message: null, code: null })); + refusal.message = parsed.message; + refusal.code = parsed.code; + lastRefusalMessage = parsed.message; + lastRefusalCode = parsed.code; + return done("refused"); } - if (!res.ok) return false; + if (!res.ok) return done(false); const body: unknown = await res.json().catch(() => null); const expectedTeardown = io.deferSharedTeardownNonce ? "deferred" : "performed"; sharedTeardownConfirmed = body !== null @@ -150,14 +218,14 @@ export async function stopProxyGracefully(pid: number, io: GracefulStopIo = {}): && "success" in body && body.success === true && "sharedTeardown" in body && body.sharedTeardown === expectedTeardown; } catch { - return false; + return done(false); } const waitExit = io.waitExit ?? waitForExit; // Honor the server's own drain window: /api/stop answers 200 first, then drains for // config.shutdownTimeoutMs. Waiting less than that hard-kills mid-drain. const exitTimeoutMs = io.exitTimeoutMs ?? drainDeadlineMs(); - if (!waitExit(pid, exitTimeoutMs)) return false; - return sharedTeardownConfirmed ? true : "teardown-unconfirmed"; + if (!waitExit(pid, exitTimeoutMs)) return done(false); + return done(sharedTeardownConfirmed ? true : "teardown-unconfirmed"); } function drainDeadlineMs(): number { @@ -172,14 +240,14 @@ function drainDeadlineMs(): number { export async function stopProxy(pid: number, io: GracefulStopIo = {}): Promise { if (!isProcessAlive(pid)) return false; const runtime = io.runtimeEndpoint ?? readRuntimePort(pid); - const graceful = await stopProxyGracefully(pid, io); + const { result: graceful, refusal } = await stopProxyGracefullyDetailed(pid, io); if (graceful === "refused") { - // The proxy refused on purpose (foreign service owns it). Forcing would strip shared - // config while that service keeps the proxy alive. + // The proxy refused on purpose. Forcing would strip shared config while whatever owns + // the process keeps it alive. The server's own message is preferred; the fallback is + // selected from its code so an empty body still names the right cause. Both come from + // THIS attempt, so an overlapping stop cannot lend it the wrong reason. throw new ProxyOwnershipRefusedError( - lastRefusalMessage - ?? "The running proxy refused to stop: a service installed under a different " - + "CODEX_HOME/OPENCODEX_HOME owns it. Run the stop from that home.", + refusal.message ?? refusalFallbackMessage(refusal.code), ); } if (graceful === "teardown-unconfirmed") { diff --git a/tests/lib/process-control-graceful.test.ts b/tests/lib/process-control-graceful.test.ts index 9fad73d38b..f4e370df48 100644 --- a/tests/lib/process-control-graceful.test.ts +++ b/tests/lib/process-control-graceful.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { gracefulStopHost, lastStopRefusalMessage, stopProxyGracefully } from "../../src/lib/process-control"; +import { gracefulStopHost, lastStopRefusalCode, lastStopRefusalMessage, ProxyOwnershipRefusedError, stopProxy, stopProxyGracefully } from "../../src/lib/process-control"; function okResponse(): Response { return new Response(JSON.stringify({ success: true, sharedTeardown: "performed" }), { status: 200 }); @@ -198,4 +198,107 @@ describe("409 refusal reporting", () => { expect(result).toBe("refused"); expect(lastStopRefusalMessage()).toBeNull(); }); + + test("the refusal code is captured alongside the message", async () => { + // The message alone cannot drive the fallback: a refusal that arrives with an empty or + // unparseable body still has to name a cause, and #4169 showed what happens when the + // fallback guesses one — the operator re-checks CODEX_HOME for a refusal the scheduler + // wrapper issued. + await stopProxyGracefully(7, { + readRuntime: () => ({ port: 10100 }), + fetchFn: (async () => new Response( + JSON.stringify({ success: false, code: "respawnable_service", message: "wrapper owns it" }), + { status: 409, headers: { "content-type": "application/json" } }, + )) as typeof fetch, + waitExit: () => true, + env: {}, + }); + expect(lastStopRefusalCode()).toBe("respawnable_service"); + + await stopProxyGracefully(7, { + readRuntime: () => ({ port: 10100 }), + fetchFn: (async () => new Response("not json", { status: 409 })) as typeof fetch, + waitExit: () => true, + env: {}, + }); + expect(lastStopRefusalCode()).toBeNull(); + }); + + test("a refusal without a message falls back by code, never to an ownership claim", async () => { + const refusalFor = async (code: string | null): Promise => { + const body = code === null ? "not json" : JSON.stringify({ success: false, code }); + try { + await stopProxy(process.pid, { + readRuntime: () => ({ port: 10100 }), + fetchFn: (async () => new Response(body, { + status: 409, + headers: { "content-type": "application/json" }, + })) as typeof fetch, + waitExit: () => { throw new Error("must not wait for a refused stop"); }, + env: {}, + }); + } catch (err) { + if (err instanceof ProxyOwnershipRefusedError) return err.message; + throw err; + } + throw new Error("stopProxy must throw on a refusal"); + }; + + const respawnable = await refusalFor("respawnable_service"); + expect(respawnable).toContain("respawn"); + expect(respawnable).toContain("ocx stop"); + + const selfUnload = await refusalFor("self_unload_service"); + expect(selfUnload).toContain("installed service itself"); + + const unknownState = await refusalFor("service_state_unknown"); + expect(unknownState).toContain("ocx service status"); + + const noBody = await refusalFor(null); + expect(noBody).toContain("sent no reason"); + + // None of them may assert the cause that #4169 was filed for. + for (const message of [respawnable, selfUnload, unknownState, noBody]) { + expect(message).not.toContain("CODEX_HOME"); + expect(message).not.toContain("OPENCODEX_HOME"); + } + }); + + test("concurrent refusals each keep their own cause", async () => { + // Reading the reason from module state lets one stop publish its refusal and a second + // overwrite it before the first continuation consumes it. Starting both together is + // what actually reproduces that: verified against the pre-fix global handoff, where + // this schedule fails with the first call throwing the second's cause + // ("...it is the installed service itself..." for the respawnable_service stop). + // A schedule that lets one call finish entirely before resuming the other does NOT + // discriminate — the parked call republishes its own globals last and passes either way. + const refusalOf = (code: string) => async (): Promise => { + try { + await stopProxy(process.pid, { + readRuntime: () => ({ port: 10100 }), + fetchFn: (async () => new Response(JSON.stringify({ success: false, code }), { + status: 409, + headers: { "content-type": "application/json" }, + })) as typeof fetch, + waitExit: () => { throw new Error("must not wait for a refused stop"); }, + env: {}, + }); + } catch (err) { + if (err instanceof ProxyOwnershipRefusedError) return err.message; + throw err; + } + throw new Error("stopProxy must throw on a refusal"); + }; + + // Repeated because the interleaving is scheduler-dependent; the pre-fix code fails on + // the first iteration, but a single run would be a weak guard against reintroduction. + for (let i = 0; i < 20; i++) { + const [respawnable, selfUnload] = await Promise.all([ + refusalOf("respawnable_service")(), + refusalOf("self_unload_service")(), + ]); + expect(respawnable).toContain("respawn"); + expect(selfUnload).toContain("installed service itself"); + } + }); }); diff --git a/tests/providers/xai/grok-lifecycle.test.ts b/tests/providers/xai/grok-lifecycle.test.ts index 88212768af..ccb1f8d80f 100644 --- a/tests/providers/xai/grok-lifecycle.test.ts +++ b/tests/providers/xai/grok-lifecycle.test.ts @@ -150,7 +150,7 @@ describe("Grok fence lifecycle wiring", () => { // shared teardown must be skipped at both call sites, exactly like the service-manager path. const ownershipRefusals = stopFn.match(/err instanceof ProxyOwnershipRefusedError[\s\S]{0,200}?ownershipBlocked = true;/g); expect(ownershipRefusals).toHaveLength(2); - expect(stopFn.match(/Skipping shared teardown \(native Codex restore, Grok config\): the foreign proxy is still running\./g)).toHaveLength(2); + expect(stopFn.match(/Skipping shared teardown \(native Codex restore, Grok config\): the refusing proxy is still running\./g)).toHaveLength(2); expect(PROCESS_CONTROL_SOURCE).toContain("throw new ProxyOwnershipRefusedError("); }); @@ -507,10 +507,12 @@ describe("POST /api/stop teardown", () => { }); test("a 409 does not escalate to a forced kill", () => { - // Escalating would run the daemon's cleanup and strip shared config while the foreign - // service keeps the proxy alive — the exact hole the ownership gate exists to close. + // Escalating would run the daemon's cleanup and strip shared config while the refusing + // service keeps the proxy alive — the exact hole the refusal gate exists to close. // The 409 branch may capture the server's reason first (#4023 added a second refusal - // cause), but it must still return "refused" without falling through to !res.ok. + // cause, #4169 the code that names it), but it must still yield "refused" without + // falling through to !res.ok. Matched loosely so a wrapped return (`done("refused")`) + // still satisfies the invariant this guards, which is ordering, not spelling. const stopGracefully = sliceFn( PROCESS_CONTROL_SOURCE, "export async function stopProxyGracefully(", @@ -518,9 +520,12 @@ describe("POST /api/stop teardown", () => { ); const four09At = stopGracefully.indexOf("res.status === 409"); expect(four09At).toBeGreaterThan(-1); - expect(stopGracefully.slice(four09At)).toContain('return "refused"'); - expect(stopGracefully.indexOf('return "refused"', four09At)) - .toBeLessThan(stopGracefully.indexOf("if (!res.ok) return false;", four09At)); + const refusedReturn = /return (?:done\()?"refused"/; + const okFallthrough = /if \(!res\.ok\) return (?:done\()?false/; + const afterFour09 = stopGracefully.slice(four09At); + expect(afterFour09).toMatch(refusedReturn); + expect(afterFour09.search(refusedReturn)) + .toBeLessThan(afterFour09.search(okFallthrough)); const stopProxyFn = sliceFn(PROCESS_CONTROL_SOURCE, "export async function stopProxy(", "export function killProxy("); const refusedAt = stopProxyFn.indexOf('graceful === "refused"');