Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 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
4 changes: 2 additions & 2 deletions src/cli/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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) {
Expand Down
106 changes: 87 additions & 19 deletions src/lib/process-control.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<GracefulStopResult> {
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<string, string> = {};
const token = configuredAdminToken(env.OPENCODEX_HOME?.trim() || undefined, env as NodeJS.ProcessEnv);
Expand All @@ -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
Expand All @@ -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 {
Expand All @@ -172,14 +240,14 @@ function drainDeadlineMs(): number {
export async function stopProxy(pid: number, io: GracefulStopIo = {}): Promise<boolean> {
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") {
Expand Down
104 changes: 103 additions & 1 deletion tests/lib/process-control-graceful.test.ts
Original file line number Diff line number Diff line change
@@ -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 });
Expand Down Expand Up @@ -198,4 +198,106 @@ 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 bodyless refusal falls back to its code, never to an ownership claim", async () => {
const refusalFor = async (code: string | null): Promise<string> => {
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("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<void>(resolve => { releaseFirst = resolve; });

const refusalOf = (code: string, gate?: Promise<void>) => async (): Promise<string> => {
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");
});
});
19 changes: 12 additions & 7 deletions tests/providers/xai/grok-lifecycle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(");
});

Expand Down Expand Up @@ -507,20 +507,25 @@ 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(",
"export async function stopProxy(",
);
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"');
Expand Down
Loading