From 506473d081a954747066905e8e5912d8ade34676 Mon Sep 17 00:00:00 2001 From: yeongjunyoo <47925973+yeongjunyoo@users.noreply.github.com> Date: Thu, 10 Sep 2026 10:34:27 +0900 Subject: [PATCH 1/3] fix(stop): name the real refusal cause instead of asserting ownership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `POST /api/stop` refuses for three distinct reasons — `respawnable_service`, `self_unload_service` and `service_state_unknown` — but `stopProxy` fell back to a hardcoded "a service installed under a different CODEX_HOME/OPENCODEX_HOME owns it" for all of them whenever the server sent no readable message. #4023 already carried the server's message through for this reason; the fallback was the half of it left guessing. When it guesses wrong the cost is not cosmetic: the operator re-checks CODEX_HOME, and the two commands point at each other — `/api/stop` says to run `ocx stop`, and `ocx stop` reports an ownership mismatch that does not exist. Capture the refusal `code` alongside the message and select the fallback from it, defaulting to cause-neutral wording rather than a specific wrong cause. The CLI's teardown notice no longer calls the process "foreign" either, since a respawning wrapper or the service itself is not another home's proxy. Refs #4169 --- src/cli/index.ts | 4 +- src/lib/process-control.ts | 70 ++++++++++++++++++---- tests/lib/process-control-graceful.test.ts | 67 ++++++++++++++++++++- tests/providers/xai/grok-lifecycle.test.ts | 2 +- 4 files changed, 127 insertions(+), 16 deletions(-) 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..ab714fd8d7 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 @@ -128,17 +166,26 @@ 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 refusal = 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); + .catch(() => ({ message: null, code: null })); + lastRefusalMessage = refusal.message; + lastRefusalCode = refusal.code; return "refused"; } if (!res.ok) return false; @@ -174,12 +221,11 @@ export async function stopProxy(pid: number, io: GracefulStopIo = {}): Promise { 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 bodyless refusal falls back to its 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"); + } + }); }); diff --git a/tests/providers/xai/grok-lifecycle.test.ts b/tests/providers/xai/grok-lifecycle.test.ts index 88212768af..eb3ea9431d 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("); }); From b7d744f75cdae55b88b57969390c9090d0ed8acd Mon Sep 17 00:00:00 2001 From: yeongjunyoo <47925973+yeongjunyoo@users.noreply.github.com> Date: Thu, 10 Sep 2026 10:50:21 +0900 Subject: [PATCH 2/3] fix(stop): carry the refusal cause per attempt, not in module state Review follow-up: `stopProxyGracefully` wrote the 409 message and code to module-scoped state that `stopProxy` read after awaiting it, so two overlapping stops could interleave and the first would throw with the second's cause. Thread the refusal back through a private per-call result (`stopProxyGracefullyDetailed`) and build the error from that. The exported `stopProxyGracefully` signature and `GracefulStopResult` are unchanged, and the `lastStopRefusalMessage`/`lastStopRefusalCode` accessors stay as observational state for callers that only want the most recent refusal. The 409 source-oracle in grok-lifecycle asserted the literal `return "refused"`. It now matches a wrapped return too: the invariant it guards is that the 409 branch yields "refused" before the !res.ok fallthrough, which is ordering rather than spelling. Refs #4169 --- src/lib/process-control.ts | 46 ++++++++++++++++------ tests/lib/process-control-graceful.test.ts | 37 +++++++++++++++++ tests/providers/xai/grok-lifecycle.test.ts | 17 +++++--- 3 files changed, 82 insertions(+), 18 deletions(-) diff --git a/src/lib/process-control.ts b/src/lib/process-control.ts index ab714fd8d7..6d699be8fd 100644 --- a/src/lib/process-control.ts +++ b/src/lib/process-control.ts @@ -142,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); @@ -173,7 +192,7 @@ export async function stopProxyGracefully(pid: number, io: GracefulStopIo = {}): // 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) { - const refusal = await res.json() + const parsed = await res.json() .then(body => { const record = body as { message?: unknown; code?: unknown } | null; const message = record?.message; @@ -184,11 +203,13 @@ export async function stopProxyGracefully(pid: number, io: GracefulStopIo = {}): }; }) .catch(() => ({ message: null, code: null })); - lastRefusalMessage = refusal.message; - lastRefusalCode = refusal.code; - return "refused"; + 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 @@ -197,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 { @@ -219,13 +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. 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. + // 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 ?? refusalFallbackMessage(lastRefusalCode), + 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 6b010cf79c..30a0cbf5bd 100644 --- a/tests/lib/process-control-graceful.test.ts +++ b/tests/lib/process-control-graceful.test.ts @@ -263,4 +263,41 @@ describe("409 refusal reporting", () => { expect(message).not.toContain("OPENCODEX_HOME"); } }); + + test("overlapping refusals each keep their own cause", async () => { + // Reading the reason from module state would let the second stop overwrite the first + // one's cause before it is read, so the earlier caller reports a refusal it never got. + let releaseFirst: (() => void) | null = null; + const firstReached = new Promise(resolve => { releaseFirst = resolve; }); + + const refusalOf = (code: string, gate?: Promise) => async (): Promise => { + try { + await stopProxy(process.pid, { + readRuntime: () => ({ port: 10100 }), + fetchFn: (async () => { + if (gate) await gate; + return 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"); + }; + + // The first call parks inside fetch until the second has already resolved and written + // its own code, which is exactly the interleaving module-scoped state cannot survive. + const first = refusalOf("respawnable_service", firstReached)(); + const second = await refusalOf("self_unload_service")(); + releaseFirst?.(); + + expect(await first).toContain("respawn"); + expect(second).toContain("installed service itself"); + }); }); diff --git a/tests/providers/xai/grok-lifecycle.test.ts b/tests/providers/xai/grok-lifecycle.test.ts index eb3ea9431d..ccb1f8d80f 100644 --- a/tests/providers/xai/grok-lifecycle.test.ts +++ b/tests/providers/xai/grok-lifecycle.test.ts @@ -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"'); From 4d72ef010363b80cd78f65148a5228d6797a3117 Mon Sep 17 00:00:00 2001 From: yeongjunyoo <47925973+yeongjunyoo@users.noreply.github.com> Date: Thu, 10 Sep 2026 13:39:17 +0900 Subject: [PATCH 3/3] test(stop): make the overlap case actually discriminate the global handoff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous overlap test parked one call inside fetch and let the other finish completely, then resumed it. The pre-fix global handoff passes that schedule too, because the parked call republishes its own globals last — so the test guarded nothing. Start both stops together instead. Verified as a counterfactual against the pre-fix implementation (506473d): the concurrent schedule fails there on the first iteration, with the respawnable_service call throwing the self_unload cause. It passes on the per-call result. Also renames the fallback test: a response with no readable body has no code either, so "falls back by code" describes it more accurately than "bodyless refusal falls back to its code". Refs #4169 --- tests/lib/process-control-graceful.test.ts | 47 +++++++++++----------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/tests/lib/process-control-graceful.test.ts b/tests/lib/process-control-graceful.test.ts index 30a0cbf5bd..f4e370df48 100644 --- a/tests/lib/process-control-graceful.test.ts +++ b/tests/lib/process-control-graceful.test.ts @@ -224,7 +224,7 @@ describe("409 refusal reporting", () => { expect(lastStopRefusalCode()).toBeNull(); }); - test("a bodyless refusal falls back to its code, never to an ownership claim", async () => { + 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 { @@ -264,23 +264,22 @@ describe("409 refusal reporting", () => { } }); - test("overlapping refusals each keep their own cause", async () => { - // Reading the reason from module state would let the second stop overwrite the first - // one's cause before it is read, so the earlier caller reports a refusal it never got. - let releaseFirst: (() => void) | null = null; - const firstReached = new Promise(resolve => { releaseFirst = resolve; }); - - const refusalOf = (code: string, gate?: Promise) => async (): Promise => { + 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 () => { - if (gate) await gate; - return new Response(JSON.stringify({ success: false, code }), { - status: 409, - headers: { "content-type": "application/json" }, - }); - }) as typeof fetch, + 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: {}, }); @@ -291,13 +290,15 @@ describe("409 refusal reporting", () => { throw new Error("stopProxy must throw on a refusal"); }; - // The first call parks inside fetch until the second has already resolved and written - // its own code, which is exactly the interleaving module-scoped state cannot survive. - const first = refusalOf("respawnable_service", firstReached)(); - const second = await refusalOf("self_unload_service")(); - releaseFirst?.(); - - expect(await first).toContain("respawn"); - expect(second).toContain("installed service itself"); + // 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"); + } }); });