Skip to content

Commit b72c03f

Browse files
committed
feat(windows): count atomic-replace retries so the envelope can be argued from evidence
Both plan audits wanted the 75ms retry envelope widened. Neither could show it failing in the field, and one explicitly declined to raise its severity for exactly that reason. Counting is the honest next step: if these stay at zero across a release the envelope is fine, and if they do not, the change cites numbers. Counters live beside the retry loop, keyed by which publisher retried, with separate retried and exhausted totals. Exposed as GET /api/system/windows-replace-retries -- a sibling of /api/system/memory rather than a field on it, because that payload is memory-shaped and appending filesystem counters would make both harder to consume. The publisher label is a closed union (ReplacePublisher), and that union is the privacy enforcement, not privacy:scan. The scanner reads file text (scripts/privacy-scan.ts:187) and cannot tell that a runtime string came from a path -- a path could carry a username. A closed union makes the same mistake a typecheck failure instead, which the new test pins with @ts-expect-error. Scope note, stated because the plan originally overreached here: these counters are process-local. Asserting they stay zero across the Windows suite would need a finalizer aggregating many short-lived sharded processes, which does not exist. Evidence comes from local runs and voluntary bug reports. tests/system-routes.test.ts is new; handleSystemRoutes coverage previously lived scattered in memory-watchdog.test.ts and codex-restart-route.test.ts. Verification: bun test tests/system-routes.test.ts (9 pass), bun run typecheck clean, bun run privacy:scan passed.
1 parent 16a5d0a commit b72c03f

2 files changed

Lines changed: 146 additions & 0 deletions

File tree

src/server/management/system-routes.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import { getActiveTurnCount, isDraining } from "../lifecycle";
2727
import { getActiveMemoryWatchdog, observedMemoryCounter } from "../memory-watchdog";
2828
import { responseStateMetrics } from "../../responses/state";
2929
import { appOwnedBytesSnapshot } from "../../lib/app-owned-memory";
30+
import { readWindowsReplaceRetryCounters } from "../../lib/windows-atomic-replace";
3031
import {
3132
SYSTEM_RESTART_EXPECTED_PID_HEADER,
3233
parseExpectedSystemRestartPid,
@@ -115,6 +116,20 @@ export async function handleSystemRoutes(ctx: ManagementContext): Promise<Respon
115116
});
116117
}
117118

119+
/**
120+
* Windows atomic-replace retry counters.
121+
*
122+
* A sibling route rather than a field on /api/system/memory: that payload is
123+
* memory-shaped, and appending unrelated filesystem counters to it makes both
124+
* harder to consume.
125+
*
126+
* Keys are a closed union of publisher labels (ReplacePublisher), never a
127+
* path — a path could carry a username. The type is what enforces that; a
128+
* text scan cannot see a runtime value.
129+
*/
130+
if (url.pathname === "/api/system/windows-replace-retries" && req.method === "GET") {
131+
return jsonResponse({ counters: readWindowsReplaceRetryCounters() });
132+
}
118133
if (url.pathname === "/api/system/restart" && req.method === "POST") {
119134
const expectedPid = parseExpectedSystemRestartPid(
120135
req.headers.get(SYSTEM_RESTART_EXPECTED_PID_HEADER),

tests/system-routes.test.ts

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
/**
2+
* GET /api/system/windows-replace-retries.
3+
*
4+
* The Windows atomic replace retries EBUSY/EPERM/EACCES twice, about 75ms of
5+
* tolerance in total. Both plan audits wanted that envelope widened; neither
6+
* could show it failing in the field. These counters exist to answer that with
7+
* evidence instead of intuition, so the endpoint's whole job is to report
8+
* whether the retry path ever fires.
9+
*
10+
* Scope note: the counters are process-local. A CI assertion that they stay
11+
* zero across the suite would need a finalizer that aggregates many short-lived
12+
* sharded processes, which does not exist. This file covers the route.
13+
*/
14+
import { afterEach, describe, expect, test } from "bun:test";
15+
16+
import { handleManagementAPI } from "../src/server/management-api";
17+
import {
18+
readWindowsReplaceRetryCounters,
19+
renameAtomicFile,
20+
resetWindowsReplaceRetryCountersForTests,
21+
type ReplacePublisher,
22+
} from "../src/lib/windows-atomic-replace";
23+
import type { OcxConfig } from "../src/types";
24+
25+
function config(): OcxConfig {
26+
return {
27+
port: 10100,
28+
defaultProvider: "openai",
29+
providers: {
30+
openai: {
31+
adapter: "openai-chat",
32+
baseUrl: "https://api.example.test/v1",
33+
apiKey: "sk-secret-value",
34+
defaultModel: "gpt-test",
35+
},
36+
},
37+
};
38+
}
39+
40+
/** A rename that fails with a Windows sharing violation `failures` times, then succeeds. */
41+
function flakyIo(failures: number, code = "EBUSY") {
42+
let seen = 0;
43+
return {
44+
platform: "win32" as NodeJS.Platform,
45+
rename: () => {
46+
if (seen++ < failures) {
47+
const error = new Error(code) as NodeJS.ErrnoException;
48+
error.code = code;
49+
throw error;
50+
}
51+
},
52+
sleep: () => {},
53+
};
54+
}
55+
56+
afterEach(() => {
57+
resetWindowsReplaceRetryCountersForTests();
58+
});
59+
60+
describe("windows replace retry counters", () => {
61+
test("a clean replace records nothing", () => {
62+
renameAtomicFile("a", "b", flakyIo(0), "config");
63+
expect(readWindowsReplaceRetryCounters()).toEqual({});
64+
});
65+
66+
test("a transient sharing violation is counted and then succeeds", () => {
67+
renameAtomicFile("a", "b", flakyIo(1), "prompt-journal");
68+
expect(readWindowsReplaceRetryCounters()).toEqual({
69+
"prompt-journal": { retried: 1, exhausted: 0 },
70+
});
71+
});
72+
73+
test("exhausting the envelope rethrows and is counted separately", () => {
74+
expect(() => renameAtomicFile("a", "b", flakyIo(99), "config-ownership")).toThrow();
75+
// Two retries then the throw: the envelope is 2, not unbounded.
76+
expect(readWindowsReplaceRetryCounters()).toEqual({
77+
"config-ownership": { retried: 2, exhausted: 1 },
78+
});
79+
});
80+
81+
test("a non-Windows error is not retried and not counted", () => {
82+
const io = { ...flakyIo(99, "ENOENT") };
83+
expect(() => renameAtomicFile("a", "b", io, "config")).toThrow();
84+
expect(readWindowsReplaceRetryCounters()).toEqual({});
85+
});
86+
87+
test("POSIX never retries even on a matching code", () => {
88+
const io = { ...flakyIo(99), platform: "linux" as NodeJS.Platform };
89+
expect(() => renameAtomicFile("a", "b", io, "config")).toThrow();
90+
expect(readWindowsReplaceRetryCounters()).toEqual({});
91+
});
92+
93+
test("publisher labels are a closed set, so a path can never become a key", () => {
94+
// The union is the privacy enforcement: privacy:scan reads file text and
95+
// cannot tell that a runtime string came from a path. If this list ever
96+
// grows, it grows deliberately and in review.
97+
const publishers: ReplacePublisher[] = ["config", "prompt-journal", "config-ownership"];
98+
for (const publisher of publishers) renameAtomicFile("a", "b", flakyIo(1), publisher);
99+
expect(Object.keys(readWindowsReplaceRetryCounters()).sort()).toEqual(
100+
["config", "config-ownership", "prompt-journal"],
101+
);
102+
// @ts-expect-error a path is not a ReplacePublisher
103+
renameAtomicFile("a", "b", flakyIo(0), "C:\\Users\\someone\\.opencodex");
104+
});
105+
});
106+
107+
describe("GET /api/system/windows-replace-retries", () => {
108+
test("reports the snapshot", async () => {
109+
renameAtomicFile("a", "b", flakyIo(1), "config");
110+
const req = new Request("http://127.0.0.1:10100/api/system/windows-replace-retries", { headers: { Host: "127.0.0.1:10100" } });
111+
const res = await handleManagementAPI(req, new URL(req.url), config());
112+
expect(res).not.toBeNull();
113+
expect(res!.status).toBe(200);
114+
const body = await res!.json() as { counters: Record<string, { retried: number; exhausted: number }> };
115+
expect(body.counters).toEqual({ config: { retried: 1, exhausted: 0 } });
116+
});
117+
118+
test("an empty snapshot is an empty object, not an error", async () => {
119+
const req = new Request("http://127.0.0.1:10100/api/system/windows-replace-retries", { headers: { Host: "127.0.0.1:10100" } });
120+
const res = await handleManagementAPI(req, new URL(req.url), config());
121+
expect(res!.status).toBe(200);
122+
expect(await res!.json()).toEqual({ counters: {} });
123+
});
124+
125+
test("the route is GET-only", async () => {
126+
const req = new Request("http://127.0.0.1:10100/api/system/windows-replace-retries", { method: "POST" });
127+
const res = await handleManagementAPI(req, new URL(req.url), config());
128+
// Either unmatched (null) or a non-200 — never a successful mutation.
129+
expect(res === null || res.status !== 200).toBe(true);
130+
});
131+
});

0 commit comments

Comments
 (0)