Skip to content

Commit 2d0c6a9

Browse files
committed
fix(service): verify the proxy is really gone before reporting a stop (#764)
`ocx service stop` reported success and restored native Codex while a proxy was still listening, leaving both pointing at each other. The cause is that `ops.stop()` reports the outcome of the STOP COMMAND, not of the process. A Windows scheduler task whose wrapper survives `schtasks /end` respawns its child a few seconds later, so a stop that returned success can still leave a live proxy. The tracked-pid cleanup does not catch it either: the respawned child writes a pid this process never recorded, and `stopTrackedProxyIfRunning` returns "none" the moment the pid file is absent. Stop is now verified instead of inferred. After the manager and the tracked proxy are stopped, the command polls for a live proxy across the observed restart window; if one answers, it reports the port, leaves native Codex alone, and exits non-zero. Restoring the config on top of a running proxy is the part that turns a failed stop into a broken installation, so that is the step that must not happen on an unverified stop. Deliberately different from PR #780, which waits only when `schtasks /end` returns an ERROR. The reported case is an /end that SUCCEEDS while the wrapper lives — on that path there is nothing for the command to report, so waiting on its exit status cannot help. That PR's other work (orphan discovery, PID verification, Microsoft-account preflight) is untouched here and still worth reviewing separately. Four tests on an injected clock, so no wall-clock sleeping. Ablation removing the restart-window polling in favour of a single probe: 1 pass / 3 fail, including the respawn case itself; restored 4/4. One test asserts a genuinely stopped proxy returns null — without it the check could report a survivor every time and strand native Codex unrestored, which is worse than the bug. Another asserts a throwing probe is not read as absence, since an unreachable health endpoint means unknown, not stopped. 87 pass across service, proxy-liveness and both tray suites. Windows behavior needs the matrix CI; the logic here is clock-injected and platform-neutral.
1 parent 13a76a3 commit 2d0c6a9

2 files changed

Lines changed: 124 additions & 0 deletions

File tree

src/service.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1626,6 +1626,39 @@ function platformOps(backend: ServiceBackend = "scheduler"): ServiceOps | null {
16261626

16271627
type TrackedProxyCleanupResult = "none" | "stale" | "stopped";
16281628

1629+
/**
1630+
* Whether a proxy is still answering after the service manager claimed to stop it.
1631+
*
1632+
* `ops.stop()` reports the outcome of the STOP COMMAND, not of the process. A Windows scheduler
1633+
* task whose wrapper survives `schtasks /end` respawns its child a few seconds later, so a stop
1634+
* that returned success can still leave a live proxy — and `ocx service stop` then restored
1635+
* native Codex on top of a running one (#764). The tracked-pid cleanup does not catch it either:
1636+
* the respawned child writes a different pid, or none this process knows about.
1637+
*
1638+
* Probed rather than assumed, and bounded: the observed respawn window is ~5s, so a few seconds
1639+
* of polling either sees it come back or it is genuinely gone.
1640+
*/
1641+
export async function proxyStillLiveAfterStop(deps: {
1642+
findProxy?: () => Promise<{ port: number } | null>;
1643+
sleep?: (ms: number) => Promise<void>;
1644+
now?: () => number;
1645+
} = {}): Promise<{ port: number } | null> {
1646+
const findProxy = deps.findProxy ?? findLiveProxy;
1647+
const sleep = deps.sleep ?? ((ms: number) => new Promise<void>(r => setTimeout(r, ms)));
1648+
const now = deps.now ?? Date.now;
1649+
const deadline = now() + 7000;
1650+
for (;;) {
1651+
try {
1652+
const live = await findProxy();
1653+
if (live) return live;
1654+
} catch {
1655+
// A probe failure is not proof the proxy is gone; keep polling until the deadline.
1656+
}
1657+
if (now() >= deadline) return null;
1658+
await sleep(1000);
1659+
}
1660+
}
1661+
16291662
async function stopTrackedProxyIfRunning(): Promise<TrackedProxyCleanupResult> {
16301663
const pid = readPid();
16311664
if (!pid) return "none";
@@ -1952,6 +1985,20 @@ export async function serviceCommand(...args: (string | undefined)[]): Promise<v
19521985
if (ops.status() !== null || isServiceInstalled()) ops.stop();
19531986
await stopTrackedProxyForServiceCommand();
19541987
{
1988+
// Verify rather than trust the stop command: a surviving wrapper respawns its child
1989+
// seconds later, and restoring native Codex on top of a live proxy is the failure #764
1990+
// reports as "stop reports success without stopping the proxy".
1991+
const survivor = await proxyStillLiveAfterStop();
1992+
if (survivor) {
1993+
console.error(
1994+
`❌ service stop did not take effect: a proxy is still listening on port ${survivor.port}.`
1995+
+ "\nNative Codex was NOT restored, because doing so while the proxy is running leaves"
1996+
+ " both pointing at each other. Check for a second service backend (`ocx service status`)"
1997+
+ " or a manually started proxy, then re-run `ocx service stop`.",
1998+
);
1999+
process.exitCode = 1;
2000+
break;
2001+
}
19552002
const restore = restoreNativeCodex();
19562003
if (restore.success) console.log("✅ service stopped + native Codex restored.");
19572004
else console.error(`⚠️ service stopped, but native Codex restore FAILED: ${restore.message}\nRun \`ocx restore\` (or check $CODEX_HOME/config.toml) before using native Codex.`);
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
import { describe, expect, test } from "bun:test";
2+
import { proxyStillLiveAfterStop } from "../src/service";
3+
4+
/**
5+
* #764: `ocx service stop` reported success while the proxy kept running, and native Codex was
6+
* restored on top of it.
7+
*
8+
* The subtlety is which failure mode matters. PR #780 waited only when `schtasks /end` returned
9+
* an ERROR -- but the reported case is an /end that SUCCEEDS while the wrapper survives and
10+
* respawns its child a few seconds later. On that path the stop command has nothing to report,
11+
* so the outcome has to be probed rather than inferred from the command's exit status.
12+
*/
13+
14+
/** Deterministic clock: no wall-clock sleeping, and the deadline is reached by construction. */
15+
function fakeClock(startMs = 0) {
16+
let current = startMs;
17+
return {
18+
now: () => current,
19+
sleep: async (ms: number) => { current += ms; },
20+
};
21+
}
22+
23+
describe("service stop verification (#764)", () => {
24+
test("a proxy that respawns inside the restart window is detected", async () => {
25+
// The reported failure: /end succeeded, the wrapper lived, and the child came back at ~5s.
26+
// A single probe immediately after the stop command would have seen nothing and passed.
27+
const clock = fakeClock();
28+
let probes = 0;
29+
const live = await proxyStillLiveAfterStop({
30+
findProxy: async () => {
31+
probes += 1;
32+
return probes >= 5 ? { port: 10100 } : null;
33+
},
34+
...clock,
35+
});
36+
expect(live).toEqual({ port: 10100 });
37+
expect(probes).toBeGreaterThan(1);
38+
});
39+
40+
test("a genuinely stopped proxy returns null within the bound", async () => {
41+
// The control. Without it the check could report a survivor every time and block every
42+
// legitimate stop -- worse than the bug, since it would strand native Codex unrestored.
43+
const clock = fakeClock();
44+
let probes = 0;
45+
const live = await proxyStillLiveAfterStop({
46+
findProxy: async () => { probes += 1; return null; },
47+
...clock,
48+
});
49+
expect(live).toBeNull();
50+
expect(probes).toBeGreaterThan(1);
51+
});
52+
53+
test("a probe that throws does not count as proof the proxy is gone", async () => {
54+
// A failing health probe means "unknown", not "stopped". Treating an exception as absence
55+
// would restore native Codex on top of a proxy that is merely unreachable for a moment.
56+
const clock = fakeClock();
57+
let probes = 0;
58+
const live = await proxyStillLiveAfterStop({
59+
findProxy: async () => {
60+
probes += 1;
61+
if (probes < 4) throw new Error("connection refused");
62+
return { port: 10100 };
63+
},
64+
...clock,
65+
});
66+
expect(live).toEqual({ port: 10100 });
67+
});
68+
69+
test("an immediately live proxy is caught on the first probe", async () => {
70+
const clock = fakeClock();
71+
const live = await proxyStillLiveAfterStop({
72+
findProxy: async () => ({ port: 10100 }),
73+
...clock,
74+
});
75+
expect(live).toEqual({ port: 10100 });
76+
});
77+
});

0 commit comments

Comments
 (0)