From 0fe7c7dba5d33576a4235bbb9c9b178553aa70f8 Mon Sep 17 00:00:00 2001 From: x3M3x Date: Tue, 1 Sep 2026 19:31:07 +0400 Subject: [PATCH] fix duplicate Codex restore after graceful stop --- src/cli/index.ts | 19 ++++++++++++++----- src/lib/process-control.ts | 7 ++++--- tests/grok-lifecycle.test.ts | 12 ++++++++++-- 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/src/cli/index.ts b/src/cli/index.ts index de491b5323..e54d1ed016 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -757,6 +757,7 @@ async function handleStop() { // restart-window wait; launchd, systemd and WinSW are down when they say so. let schedulerCanRespawn = false; let stoppedService = false; + let nativeRestoreHandledByProxy = false; // An ownership mismatch means the service manager was never even contacted: the installed // service is still live and will respawn the proxy. Tearing down SHARED state in that // situation (native Codex config, the Grok fence) removes config out from under a running @@ -805,17 +806,22 @@ async function handleStop() { * guessed one fails closed into manual recovery rather than letting a later probe read * "the configured port refuses" as proof that the right proxy is down. */ - const stopWithDeferral = async (pid: number, discovered?: { hostname: string; port: number } | null): Promise => { + const stopWithDeferral = async (pid: number, discovered?: { hostname: string; port: number } | null): Promise => { // Resolve ONCE. Reading the runtime record twice let the receipt name the configured // guess while the request went to a runtime endpoint that appeared in between. const exact = discovered ?? endpointOf(readRuntimePort(pid)); claimTeardown(exact ?? configuredEndpoint(), exact ? "exact" : "guessed"); - await stopProxy(pid, { + const graceful = await stopProxy(pid, { deferSharedTeardownNonce: teardownNonce, // Only an exact endpoint may direct the request; the configured fallback is a guess // good enough to record an obligation against, not to POST a stop to. runtimeEndpoint: exact ?? undefined, }); + // A valid receipt means the proxy deferred shared teardown to this process. If the + // receipt could not be written, the proxy restores it itself and the caller must not + // attempt a second restore after a graceful stop. A hard-kill always leaves restore + // to this process. + return graceful && !teardownNonce; }; try { const serviceStop = stopServiceIfInstalledDetailed(); @@ -858,7 +864,7 @@ async function handleStop() { // verification below, so a survivor does not get its client config pulled first. // The receipt goes down first — the proxy honours the deferral only when it can // see one, so an unrecordable claim degrades to the child doing its own teardown. - await stopWithDeferral(pid); + nativeRestoreHandledByProxy = await stopWithDeferral(pid); console.log(`✅ Proxy (PID ${pid}) stopped.`); removePid(pid); removeRuntimePort(pid); @@ -888,7 +894,10 @@ async function handleStop() { try { // The probe already found where it answers, and on this path the runtime record is // typically what went missing in the first place. - await stopWithDeferral(live.pid, { hostname: live.hostname ?? "127.0.0.1", port: live.port }); + nativeRestoreHandledByProxy = await stopWithDeferral( + live.pid, + { hostname: live.hostname ?? "127.0.0.1", port: live.port }, + ); console.log(`✅ Proxy (PID ${live.pid}) stopped.`); } catch (err) { stopFailed = true; @@ -1006,7 +1015,7 @@ async function handleStop() { console.error(" The obligation is preserved; retry once the proxy is confirmed stopped."); } } - const restoreBlocked = ownershipBlocked || inheritedBlocks; + const restoreBlocked = ownershipBlocked || inheritedBlocks || nativeRestoreHandledByProxy; if (!restoreBlocked) { if (recoveredNonces.length > 0) { // A previous deferred stop died before restoring, and the probe says its endpoint is diff --git a/src/lib/process-control.ts b/src/lib/process-control.ts index 3e296d6c72..13ab3a0c95 100644 --- a/src/lib/process-control.ts +++ b/src/lib/process-control.ts @@ -132,8 +132,8 @@ function drainDeadlineMs(): number { } /** Graceful-first stop: management-API drain, then the platform kill ladder. */ -export async function stopProxy(pid: number, io: GracefulStopIo = {}): Promise { - if (!isProcessAlive(pid)) return; +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); if (graceful === "refused") { @@ -146,10 +146,11 @@ export async function stopProxy(pid: number, io: GracefulStopIo = {}): Promise { expect(stopFn).toContain("ownershipBlocked = true"); // Ownership is now one of two reasons to skip the restore; the other is an inherited // obligation whose proxy could not be confirmed down (#3008). - expect(stopFn).toContain("const restoreBlocked = ownershipBlocked ||"); + expect(stopFn).toContain("const restoreBlocked = ownershipBlocked || inheritedBlocks || nativeRestoreHandledByProxy;"); expect(stopFn).toContain("if (!restoreBlocked) {"); expect(stopFn).toContain("await restoreSharedClientStateAfterStop()"); expect(restoreFn).toContain("restoreNativeCodexAsync()"); @@ -106,6 +106,14 @@ describe("Grok fence lifecycle wiring", () => { expect(stopFn.indexOf("revertSystemEnv()")).toBeLessThan(stopFn.indexOf("if (!restoreBlocked) {")); }); + test("graceful stop skips caller restore only when the proxy performed it", () => { + const stopFn = sliceFn(CLI_SOURCE, "async function handleStop(", "async function handleUninstall("); + expect(stopFn).toContain("const graceful = await stopProxy(pid, {"); + expect(stopFn).toContain("return graceful && !teardownNonce;"); + expect(stopFn).toContain("nativeRestoreHandledByProxy = await stopWithDeferral(pid);"); + expect(stopFn).toContain("nativeRestoreHandledByProxy = await stopWithDeferral("); + }); + test("a refused Grok strip makes ocx stop fail instead of reporting success", () => { const restoreFn = sliceFn(CLI_SOURCE, "async function restoreSharedClientStateAfterStop(", "async function handleStop("); const stopFn = sliceFn(CLI_SOURCE, "async function handleStop(", "async function handleUninstall("); @@ -261,7 +269,7 @@ describe("Grok fence lifecycle wiring", () => { expect(stopFn).toContain("teardownNonce ? [teardownNonce, ...recoveredNonces] : recoveredNonces"); // The orphan path hands over the endpoint the probe already found; its runtime record // is typically what went missing in the first place. - expect(stopFn).toContain('stopWithDeferral(live.pid, { hostname: live.hostname ?? "127.0.0.1", port: live.port })'); + expect(stopFn).toContain('nativeRestoreHandledByProxy = await stopWithDeferral(\n live.pid,\n { hostname: live.hostname ?? "127.0.0.1", port: live.port },\n );'); // A live proxy with no killable pid is not "no proxy found": purging state and // restoring over it is the same failure arrived at from the other direction. expect(stopFn).toContain("} else if (live) {");