merge(dev): sync lidge-jun#810 onto tip before full-review verdict

Author: Wibias

devlog/_plan/260731_pr_issue_triage_round/050_windows_ci_flake_rca.md

@@ -192,3 +192,68 @@ macOS의 claude 핑 테스트도 같은 계열이다. 90ms 침묵에 25ms 핑 
 
 두 문서를 합치지 않고 나란히 둔다. 이 문서는 **이 라운드에서 무엇을 고쳤는지**의
 기록이고, 그 문서는 **모드 분류와 개선 후보**의 조사다. 목적이 다르다.
+
+## 마지막 라운드 — 남은 세 건과 정책화
+
+`000_research.md`의 개선 후보 5번(예산 정책화)을 실행했다. 그 전에 남은 실패 세 건을
+각각 확인했는데, **셋 다 원인이 달랐다.**
+
+### 1. server-auth 루프 테스트 — 내가 한 번 놓쳤다
+
+이전 라운드에서 하니스 4개짜리 하나를 고쳤는데, 같은 파일에 **5개짜리와 6개짜리가
+더 있었다.** `startPoolRetryHarness()`는 호출마다 `TEST_DIR`를 지우고 다시 만들고,
+서버를 띄우고, 전역 `fetch`를 리다이렉트한다. 그걸 다섯 번 하면 5초 안에 안 끝난다.
+
+둘 다 body 커서를 쓰는 단일 하니스로 바꿨다. 각 케이스는 여전히 자기 원본 400과
+`acct-pool-a` 단일 dispatch를 증명한다.
+
+### 2. WS-REBIND-01 — 예산 초과가 아니었다
+
+748ms에 죽었다. 5초 예산에 748ms면 타임아웃이 아니다. 테스트 안에 **자체 1초 데드라인**이
+있었다:
+
+```
+setTimeout(() => reject(new Error("websocket retry timed out")), 1_000);
+```
+
+실제 서버를 멈췄다 다시 띄우고, WebSocket을 열고, 계정 간 2홉 재시도를 기다리는 데
+1초다. 부하 걸린 러너에서는 낙관적이다.
+
+**내부 데드라인이 테스트 예산보다 짧으면 타임아웃보다 빨리 죽어서 로직 에러처럼 보인다.**
+이게 이 건이 유독 무작위처럼 보였던 이유다. 헬퍼에 `INTERNAL_DEADLINE_MS`로 이름을 붙였다.
+
+덤으로 하니스가 **WebSocket 레지스트리를 안 지운다**는 것도 찾았다. 프로세스 전역이고
+teardown에서 살아남는데, WS-REBIND-01은 계정별 정확한 소켓 개수를 단언한다. 다섯 가지
+상태를 지우면서 이것만 빠져 있었다.
+
+다만 **로컬에서 재현은 못 했다.** 리셋을 빼고 돌려도, 소켓을 하나 흘려놓고 돌려도
+통과했다. 그래서 커밋에 "방어적 조치이고 재현된 수정이 아니다"라고 적었다. 재현 못 한 걸
+고쳤다고 쓰면 다음 사람이 잘못된 근거 위에서 판단한다.
+
+### 3. executeArchivedCleanup — 우리 문제가 아니다
+
+**이미 20초를 받고 25052ms에 죽었다.** 20초를 넘기는 테스트는 예산 문제가 아니라는
+신호라 먼저 재봤다.
+
+```
+(pass) executeArchivedCleanup > permanent cleanup removes logs, goals, and memory rows … [7.16ms]
+```
+
+로컬 **7.16ms**, CI **25052ms**. 3500배 차이다. sleep도 spawn도 없는 동기 SQLite
+작업이다. 테스트가 느린 게 아니라 러너가 그 순간 그만큼 느렸다는 뜻이다.
+
+숫자를 다시 올리는 대신 `STORE_BUDGET_MS`로 묶었다. 이 파일에만 흩어진 리터럴이
+22개였다.
+
+## 정책 — `tests/helpers/test-budget.ts`
+
+예산을 올린 커밋이 네 개가 되도록 매번 숫자를 따로 정하고 있었다. 스위트 전체에
+timeout 리터럴이 52개다. 정당한 예산과 침묵당한 플레이크를 구분할 방법이 없다.
+
+헬퍼는 **숫자가 아니라 규칙**을 적는다. 둘 다 참일 때만 예산을 올릴 수 있다:
+
+1. 그 기다림이 테스트가 단언하는 것에 본질적이다.
+2. **ablation이 여전히 실패한다.**
+
+1번만 있고 2번이 없으면 무력한 테스트를 고정시키는 것이다. `020`에서 지적한 바로 그
+실패 모드다.

tests/codex-app-server-processes.test.ts

@@ -359,16 +359,23 @@ describe("Windows Win32_Process owner enumeration (#476)", () => {
         [
           "-NoProfile", "-NoLogo", "-NonInteractive", "-WindowStyle", "Hidden",
           "-Command",
-          "Start-Sleep -Seconds 25 # codex app-server integration-probe",
+          "Start-Sleep -Seconds 45 # codex app-server integration-probe",
         ],
         { stdio: "ignore", windowsHide: true },
       );
       try {
         expect(child.pid).toBeGreaterThan(1);
-        // Brief settle so Win32_Process can observe the child.
-        Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 250);
-        const snapshots = listWindowsSnapshots();
-        const match = snapshots.find(snapshot => snapshot.pid === child.pid);
+        // Brief settle so Win32_Process can observe the child. A loaded Windows
+        // runner can also exhaust one CIM enumeration deadline, so tolerate one
+        // transient empty result while keeping the production timeout unchanged.
+        Bun.sleepSync(250);
+        let snapshots = listWindowsSnapshots();
+        let match = snapshots.find(snapshot => snapshot.pid === child.pid);
+        if (!match) {
+          Bun.sleepSync(250);
+          snapshots = listWindowsSnapshots();
+          match = snapshots.find(snapshot => snapshot.pid === child.pid);
+        }
         expect(match).toBeDefined();
         expect(match!.owner).toMatch(/\\/);
         expect(match!.commandLine.toLowerCase()).toContain("codex app-server");
@@ -381,6 +388,6 @@ describe("Windows Win32_Process owner enumeration (#476)", () => {
         }
       }
     },
-    { timeout: 30_000 },
+    { timeout: 35_000 },
   );
 });

tests/helpers/test-budget.ts

@@ -0,0 +1,50 @@
+/**
+ * Test budgets for work that is genuinely slow, not for making red go away.
+ *
+ * Bun's default is 5s. That is generous for a pure function and thin for anything
+ * that spawns a process, binds a server, or writes SQLite on a loaded CI runner —
+ * which is why a run of windows-latest failures kept blaming a different test each
+ * time. The blame moved; the cause did not.
+ *
+ * ## When you may raise a budget
+ *
+ * Both must hold:
+ *
+ * 1. **The wait is intrinsic to the assertion.** The tray socket-inheritance test
+ *    launches PowerShell, which launches Bun, then rebinds the port to prove the
+ *    child never inherited the listen socket. Those processes ARE the proof, so it
+ *    gets a budget. The sidebar route tests spawned `gh` only incidentally while
+ *    claiming route reachability, so they got the spawn DELETED instead. Ask which
+ *    kind you have before reaching for a number.
+ * 2. **The ablation still fails.** Revert the production behaviour the test covers
+ *    and confirm the test goes red. A budget that hides a vacuous test is worse than
+ *    the flake, because it converts a reliability signal into silence.
+ *
+ * If only (1) holds, you have not finished. If neither holds, delete the wait.
+ *
+ * ## Why these numbers
+ *
+ * They are headroom against runner contention, not measured durations. Local timings
+ * are typically two to four orders of magnitude smaller: the storage cleanup test
+ * that blew a 20s CI budget runs in ~7ms here. Sizing to the local number is what
+ * produced the original 5s failures.
+ */
+
+/** Real child process: PowerShell, a CLI smoke test, an external binary. */
+export const SPAWN_BUDGET_MS = 45_000;
+
+/** Binds a real server or opens a real socket, including restart-and-reconnect flows. */
+export const SERVER_BUDGET_MS = 30_000;
+
+/** Touches SQLite or the filesystem repeatedly. Slow on Windows for reasons outside our code. */
+export const STORE_BUDGET_MS = 30_000;
+
+/**
+ * A deadline *inside* a test, for an await that would otherwise hang forever.
+ *
+ * Keep these at least a few times under the surrounding budget. An internal deadline
+ * shorter than the test budget fails faster than a timeout and reads as a logic error:
+ * WS-REBIND-01 died at 748ms against its own hardcoded 1s while the budget was 5s, and
+ * that mismatch is exactly why it looked random rather than slow.
+ */
+export const INTERNAL_DEADLINE_MS = 15_000;

tests/server-auth.test.ts

@@ -5,7 +5,8 @@ import { request as httpRequest } from "node:http";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { saveCodexAccountCredential } from "../src/codex/account-store";
-import { getTrackedCodexWebSocketCountForAccount } from "../src/codex/websocket-registry";
+import { clearCodexWebSocketRegistry, getTrackedCodexWebSocketCountForAccount } from "../src/codex/websocket-registry";
+import { INTERNAL_DEADLINE_MS, SERVER_BUDGET_MS } from "./helpers/test-budget";
 import { clearAccountNeedsReauth, clearAccountQuota, getAccountQuota, isAccountNeedsReauth, markAccountNeedsReauth, updateAccountQuota } from "../src/codex/auth-api";
 import {
   CODEX_THREAD_AFFINITY_IDLE_TTL_MS,
@@ -162,6 +163,12 @@ async function startPoolRetryHarness(
   clearAccountQuota();
   clearAccountNeedsReauth("pool-a");
   clearAccountNeedsReauth("pool-b");
+  // The registry is process-global and survives a harness teardown. WS-REBIND-01
+  // asserts exact per-account socket counts, so a socket leaked by any earlier test
+  // in this file shifts its snapshots and fails it in milliseconds — which reads as
+  // a flake next to the timeouts, but is ordinary shared state. Reset it with the
+  // rest rather than leaving one of six kinds of state uncleaned.
+  clearCodexWebSocketRegistry();
 
   const dispatches: string[] = [];
   const upstream = Bun.serve({
@@ -1288,7 +1295,7 @@ describe("server local API auth", () => {
       if (globalThis.fetch === matrixFetch) globalThis.fetch = originalGlobalFetch;
       await upstream.stop(true);
     }
-  }, { timeout: 30_000 });
+  }, { timeout: SERVER_BUDGET_MS });
 
   test("internal web-search and vision never forward a non-ChatGPT bearer as Direct sidecar auth", async () => {
     if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true });
@@ -1984,15 +1991,18 @@ describe("server local API auth", () => {
       `${exact} suffix`,
       exact.replace("ChatGPT account.", "ChatGPT account"),
     ];
-    for (const detail of cases) {
-      const body = JSON.stringify({ detail });
-      const harness = await startPoolRetryHarness(() => rejectionResponse(body));
-      try {
-        await expectOriginal400(await harness.request(), body);
-        expect(harness.dispatches).toEqual(["acct-pool-a"]);
-      } finally {
-        await stopPoolRetryHarness(harness);
+    const negativeBodies = cases.map(detail => JSON.stringify({ detail }));
+    let nextNegative = 0;
+    const negative = await startPoolRetryHarness(() => rejectionResponse(negativeBodies[nextNegative++]!));
+    try {
+      for (const body of negativeBodies) {
+        const priorDispatches = negative.dispatches.length;
+        await expectOriginal400(await negative.request(), body);
+        expect(negative.dispatches.slice(priorDispatches)).toEqual(["acct-pool-a"]);
       }
+      expect(nextNegative).toBe(negativeBodies.length);
+    } finally {
+      await stopPoolRetryHarness(negative);
     }
     const positive = await startPoolRetryHarness(accountId => accountId === "acct-pool-a"
       ? rejectionResponse(JSON.stringify({ detail: `  THE   '${POOL_RETRY_MODEL}' MODEL IS NOT SUPPORTED\nWHEN USING CODEX WITH A CHATGPT ACCOUNT.  ` }))
@@ -2006,14 +2016,23 @@ describe("server local API auth", () => {
   }, 12_000);
 
   test("valid JSON wrong top-level shape never authorizes a pool retry", async () => {
-    for (const body of ['"string"', "42", "true", "null", '["detail"]']) {
-      const harness = await startPoolRetryHarness(() => rejectionResponse(body));
-      try {
+    // One harness, five bodies — same reason as the sibling above. Each
+    // startPoolRetryHarness() wipes and recreates TEST_DIR, binds a server, and
+    // redirects global fetch; five of those did not fit Bun's 5s default on a
+    // Windows runner, and the request still in flight when the budget expired
+    // raced the next test through that same global fetch.
+    const bodies = ['"string"', "42", "true", "null", '["detail"]'];
+    let nextBody = 0;
+    const harness = await startPoolRetryHarness(() => rejectionResponse(bodies[nextBody++]!));
+    try {
+      for (const body of bodies) {
+        const priorDispatches = harness.dispatches.length;
         await expectOriginal400(await harness.request(), body);
-        expect(harness.dispatches).toEqual(["acct-pool-a"]);
-      } finally {
-        await stopPoolRetryHarness(harness);
+        expect(harness.dispatches.slice(priorDispatches)).toEqual(["acct-pool-a"]);
       }
+      expect(nextBody).toBe(bodies.length);
+    } finally {
+      await stopPoolRetryHarness(harness);
     }
   });
 
@@ -2085,7 +2104,13 @@ describe("server local API auth", () => {
           if (String(event.data).includes("response.completed")) resolve();
         });
         ws.addEventListener("error", () => reject(new Error("websocket retry failed")), { once: true });
-        setTimeout(() => reject(new Error("websocket retry timed out")), 1_000);
+        // This 1s was the real cause of WS-REBIND-01 failing at 748ms on windows-latest:
+        // an internal deadline, not Bun's test budget, which is why it died far too fast
+        // to look like a timeout. The test stops and restarts a real server, opens a real
+        // WebSocket, and waits for a two-hop retry across accounts — a second of that on a
+        // contended runner is optimistic. The assertions below are unchanged; only the
+        // room to reach them grew.
+        setTimeout(() => reject(new Error("websocket retry timed out")), INTERNAL_DEADLINE_MS);
       });
       expect(harness.dispatches).toEqual(["acct-pool-a", "acct-pool-b"]);
       expect(registrySnapshots).toEqual([[1, 0], [0, 1]]);
@@ -2095,7 +2120,7 @@ describe("server local API auth", () => {
       ws.close();
       await stopPoolRetryHarness(harness);
     }
-  });
+  }, { timeout: 30_000 });
 
   test("passthrough connect failure records selected pool account health", async () => {
     if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true });