From 3f5d1412ce4386d6c5aae71207a01aaec7729e45 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 9 Sep 2026 17:22:28 +0900 Subject: [PATCH 1/2] test(config): give the config-lock holder child a platform-policy readiness budget and stop masking readiness failures --- .../000_plan.md | 173 ++++++++++++++++++ tests/config/config-mutation-lock.test.ts | 60 ++++-- 2 files changed, 221 insertions(+), 12 deletions(-) create mode 100644 devlog/_plan/260909_config_mutation_lock_windows/000_plan.md diff --git a/devlog/_plan/260909_config_mutation_lock_windows/000_plan.md b/devlog/_plan/260909_config_mutation_lock_windows/000_plan.md new file mode 100644 index 0000000000..3db3662fcb --- /dev/null +++ b/devlog/_plan/260909_config_mutation_lock_windows/000_plan.md @@ -0,0 +1,173 @@ +# config-mutation-lock Windows fixture: readiness budget + failure unmasking + +## Reader summary + +Windows shard 2/6 of run +[34321628628](https://github.com/lidge-jun/opencodex/actions/runs/34321628628) +(attempt 1, job 102369384143, head `ddcf8b5f9b13`, branch +`codex/pr3997-caller-main-cooldown`, a `workflow_dispatch` lane run) failed +`tests/config/config-mutation-lock.test.ts` at line 111 with `Expected: 0 / +Received: 143` after 5915.82 ms. The 143 is not a lock defect and not the +30 s teardown kill: it is the readiness-timeout path's own `child.kill()`, +and the `finally` block's exit-0 expectation then masks the real error. The +fix gives spawned-child readiness a measured 30 s budget, stops the masking, +and corrects a stale comment. No product code, no workflow changes. + +## Loop spec + +- **Loop archetype:** satisfy-spec repair of a CI test fixture. +- **Trigger:** delegated follow-up from the managing task after the xAI OAuth + unit completed; user-authorized as a small isolated maintainer PR. +- **Goal:** the Windows flake either passes (child ready within a measured + budget) or fails with the real readiness error instead of a bare 143. +- **Non-goals:** product code, CI workflow files, other tests, skipped tests, + accepting 143 as a valid outcome, bare timeout bumps without observability. + No local suite/typecheck/build (user restriction). +- **Verifier:** remote `ci.yml` — PR lane (Linux `test`, macOS + `platform-macos`, `gates`) on the PR, then a `workflow_dispatch` + `lane=all` run on the exact PR head whose Windows shards execute this file; + the previously failing test must pass there. +- **Stop condition:** PR published, PR lane green, Windows dispatch run green + for this file at the exact head, managing task handed the report. +- **Memory artifact:** this unit directory; goalplan + `.codexclaw/goalplans/` entry for this session's second goal. +- **Expected terminal outcomes:** DONE = both CI evidences green at the exact + head. BLOCKED = the Windows run shows the failure is NOT the readiness budget + (e.g. child never acquires the lock even in 30 s → real lock defect, which + would be out of this task's scope and handed back with evidence). +- **Escalation condition:** any need to touch `src/` or `.github/`, or a + Windows re-failure after the fix. + +## Verified cause (log + source, no patch before this was established) + +Timeline of the failing attempt (test duration 5915.82 ms): + +1. Parent spawns the Bun child and enters `waitForPath(readyPath)` — + **500 attempts × 10 ms = 5 s** budget (tests/config/config-mutation-lock.test.ts:27-34). +2. The child must boot Bun, transpile the `src/config.ts` import chain, and + acquire the mutation lock before writing `holder-ready`. On this loaded + runner that exceeds 5 s: the sibling child in `an abruptly exited holder + releases the OS-backed transaction…` needed **8290.11 ms** end-to-end in the + same shard (and passed, because `waitForOwnedChild` allows 30 s). The lock + itself is healthy — every other test in the file passed, and attempt 2 of the + run was green. +3. `waitForPath` throws at ~5 s; the catch kills the child — SIGTERM, exit + **143** — and rethrows an enriched error with the child's stderr + (tests/config/config-mutation-lock.test.ts:85-92). +4. The `finally` block (line 109-112) runs `writeFileSync(releasePath)` and + `expect(await waitForOwnedChild(child)).toBe(0)`. The child is already dead + with 143, so this expectation throws and **replaces** the enriched readiness + error — the log shows only the 143 mismatch at line 111, and the "child + stderr" text never appears. +5. The stale comment in `waitForOwnedChild` (lines 36-41) attributes a 5858 ms + / 143 failure to "this helper's own `kill()`" from the 5 s era — that helper + now waits 30 s, so the explanation is wrong; the 143 comes from the + readiness-timeout catch. + +## File change map + +| Path | Action | What | +|------|--------|------| +| `tests/config/config-mutation-lock.test.ts` | MODIFY | readiness wait reuses the predeclared platform policy `watchdogMs(5_000)` (5 s local / 30 s CI / 45 s Windows CI) with an elapsed deadline, a final recheck, and fail-fast on an already-exited child; unmask the primary readiness failure in both holder tests' `finally`; correct the stale `waitForOwnedChild` comment | + +OUT: `src/**`, `.github/**`, `tests/helpers/ci-watchdog.ts` (imported, not +modified), every other test file. + +Forensics correction (independent verifier Descartes, forwarded by the managing +task after the first plan draft): the readiness budget must reuse the EXISTING +`watchdogMs(5_000)` platform policy from `tests/helpers/ci-watchdog.ts` +(Windows CI floor 45 s) rather than a new hardcoded 30 s constant — that helper +is the repository's declared answer to "spawned children are slow on loaded +Windows CI", so this fix expresses policy, not a local bump. It also directed +the fail-fast on `child.exited` (no 45 s poll on an already-dead child) and +extending the unmasking to the management-API holder test. + +## Diff-level design + +### 1. `waitForPath` → `waitForOwnedChildReady` — platform-policy budget, fail-fast + +Before (lines 27-34): a fixed 500 × 10 ms (5 s) poll with no knowledge of the +child. + +After: the wait takes the spawned child, budgets `watchdogMs(5_000)` (5 s +locally, 30 s on CI, 45 s on Windows CI — the predeclared policy in +`tests/helpers/ci-watchdog.ts`), polls on an elapsed-time deadline with a final +`existsSync` recheck, and races each 10 ms tick against `child.exited` so a +child that died before writing the marker fails immediately with its exit code +and stderr instead of burning the whole budget. + +### 2. Unmask the primary failure in both holder tests + +Before (lines 84-112): + +```ts + try { + try { + await waitForPath(readyPath); + } catch (error) { + child.kill(); + await child.exited; + const stderr = await new Response(child.stderr).text().catch(() => ""); + throw new Error(`${(error as Error).message}\nchild stderr: ${stderr}`); + } + ... + } finally { + writeFileSync(releasePath, "release"); + expect(await waitForOwnedChild(child)).toBe(0); + } +``` + +After: + +```ts + let childKilled = false; + try { + try { + await waitForPath(readyPath); + } catch (error) { + childKilled = true; + child.kill(); + await child.exited; + const stderr = await new Response(child.stderr).text().catch(() => ""); + throw new Error(`${(error as Error).message}\nchild stderr: ${stderr}`); + } + ... + } finally { + writeFileSync(releasePath, "release"); + // The readiness-timeout path already killed the child; expecting exit 0 here + // would mask that primary error with a bare 143. + if (!childKilled) { + expect(await waitForOwnedChild(child)).toBe(0); + } + } +``` + +The happy path is unchanged: release marker is always written (bounded cleanup), +the exit-0 core assertion still runs whenever the child was not sacrificed, and +every lock assertion (not stolen, immediate writer failure, no stale writes) is +untouched. + +### 3. `waitForOwnedChild` comment correction + +Replace the stale 5 s-era explanation with the verified provenance: + +```ts + // The child polls for the release marker on a 10 ms sleep, so its exit is bounded by + // the filesystem noticing that write plus one Bun teardown; a loaded Windows runner + // needs real room for both. A surfaced exit 143 is never this helper's own kill() + // (which fires only after the full budget) — it is the readiness-timeout path's + // child.kill(), so read the readiness error, not this wait. +``` + +## Regression evidence plan + +- The failure mode is exercised by construction: if readiness ever exceeds the + budget again, the thrown error is the enriched `waitForPath` message (with + child stderr), asserted by reading the code path — a dedicated test for the + masking fix would require a child that never becomes ready, i.e. a deliberate + 30 s negative test; that cost is not justified for a CI fixture, and the + unmasking is straight-line control flow reviewed in the diff. +- Positive path: `ci.yml` PR lane plus a `workflow_dispatch` `lane=all` run + on the exact PR head; the Windows shard executing + `tests\config\config-mutation-lock.test.ts` must pass, and the run must + show this file's tests green. diff --git a/tests/config/config-mutation-lock.test.ts b/tests/config/config-mutation-lock.test.ts index 1d508639b6..f874ce66de 100644 --- a/tests/config/config-mutation-lock.test.ts +++ b/tests/config/config-mutation-lock.test.ts @@ -8,6 +8,7 @@ import { nextAtomicTempSequence } from "../../src/config/atomic-write"; import { CodexCredentialRefreshLockTimeoutError, getCodexAccountCredential, saveCodexAccountCredential } from "../../src/codex/account-store"; import type { OcxConfig } from "../../src/types"; import { ManagementRequest, managementHeaders } from "../helpers/management-auth"; +import { watchdogMs } from "../helpers/ci-watchdog"; import { removeTreeWithRetry } from "../helpers/remove-tree"; import { repoPath, repoRoot } from "../helpers/repo-root"; @@ -24,20 +25,36 @@ function config(port = 10100): OcxConfig { }; } -async function waitForPath(path: string): Promise { - for (let attempt = 0; attempt < 500; attempt += 1) { +async function waitForOwnedChildReady(child: ReturnType, path: string): Promise { + // Readiness budget follows the predeclared platform policy: 5 s locally, 30 s on CI, + // 45 s on Windows CI (watchdogMs), because spawning a Bun child that imports + // src/config.ts takes real time on a loaded runner — a sibling child in the failing + // shard needed 8.3 s end to end. The deadline uses elapsed time with a final recheck, + // and an already-exited child fails fast with its exit code instead of polling the + // full budget. + const budgetMs = watchdogMs(5_000); + const deadline = performance.now() + budgetMs; + while (performance.now() < deadline) { if (existsSync(path)) return; - await Bun.sleep(10); + const exit = await Promise.race([ + Bun.sleep(10).then(() => null), + child.exited.then(code => code as number | null), + ]); + if (exit !== null) { + const stderr = await new Response(child.stderr).text().catch(() => ""); + throw new Error(`config-lock child exited ${exit} before writing marker ${path}\nchild stderr: ${stderr}`); + } } - throw new Error(`Timed out waiting for child marker ${path}`); + if (existsSync(path)) return; + throw new Error(`Timed out waiting ${budgetMs}ms for child marker ${path}`); } async function waitForOwnedChild(child: ReturnType): Promise { // The child polls for the release marker on a 10 ms sleep, so its exit is bounded by the - // filesystem noticing that write plus one Bun teardown. On a loaded Windows runner both - // are slower than the 5 s this used to allow: shard 2/6 measured 5858 ms end to end and - // reported exit 143, which is this helper's own `kill()`, not a lock defect. Give the - // teardown room; a genuine hang still fails, it just takes longer to say so. + // filesystem noticing that write plus one Bun teardown; a loaded Windows runner needs + // real room for both. This helper's own kill() fires only after the full budget and + // throws, so a surfaced exit 143 is never from here — it is the readiness-timeout + // catch's child.kill(), and the error to read is the readiness failure, not this wait. const result = await Promise.race([ child.exited.then(exitCode => ({ exitCode })), Bun.sleep(30_000).then(() => null), @@ -81,10 +98,12 @@ test("a live cross-process holder is not stolen and runtime writers fail immedia stderr: "pipe", }); + let childKilled = false; try { try { - await waitForPath(readyPath); + await waitForOwnedChildReady(child, readyPath); } catch (error) { + childKilled = true; child.kill(); await child.exited; const stderr = await new Response(child.stderr).text().catch(() => ""); @@ -108,7 +127,11 @@ test("a live cross-process holder is not stolen and runtime writers fail immedia expect(getCodexAccountCredential("busy-account")).toBeNull(); } finally { writeFileSync(releasePath, "release"); - expect(await waitForOwnedChild(child)).toBe(0); + // The readiness-failure path already killed the child; expecting exit 0 here + // would mask that primary error with a bare 143. + if (!childKilled) { + expect(await waitForOwnedChild(child)).toBe(0); + } } saveConfig(config(20200)); @@ -386,8 +409,17 @@ test("management API maps config mutation lock contention to retryable 503", asy stderr: "pipe", }); + let childKilled = false; try { - await waitForPath(readyPath); + try { + await waitForOwnedChildReady(child, readyPath); + } catch (error) { + childKilled = true; + child.kill(); + await child.exited; + const stderr = await new Response(child.stderr).text().catch(() => ""); + throw new Error(`${(error as Error).message}\nchild stderr: ${stderr}`); + } const { handleManagementAPI } = await import("../../src/server/management-api"); const url = new URL("http://localhost/api/codex-auth/auto-switch"); const response = await handleManagementAPI( @@ -406,6 +438,10 @@ test("management API maps config mutation lock contention to retryable 503", asy }); } finally { writeFileSync(releasePath, "release"); - expect(await waitForOwnedChild(child)).toBe(0); + // The readiness-failure path already killed the child; expecting exit 0 here + // would mask that primary error with a bare 143. + if (!childKilled) { + expect(await waitForOwnedChild(child)).toBe(0); + } } }); From b499227d7c0091cbc13ca0fdd295776a9bd4a66d Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 9 Sep 2026 18:55:12 +0900 Subject: [PATCH 2/2] devlog: match the plan example to the implemented waitForOwnedChildReady helper --- .../000_plan.md | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/devlog/_plan/260909_config_mutation_lock_windows/000_plan.md b/devlog/_plan/260909_config_mutation_lock_windows/000_plan.md index 3db3662fcb..e5226fa05a 100644 --- a/devlog/_plan/260909_config_mutation_lock_windows/000_plan.md +++ b/devlog/_plan/260909_config_mutation_lock_windows/000_plan.md @@ -123,7 +123,7 @@ After: let childKilled = false; try { try { - await waitForPath(readyPath); + await waitForOwnedChildReady(child, readyPath); } catch (error) { childKilled = true; child.kill(); @@ -162,11 +162,14 @@ Replace the stale 5 s-era explanation with the verified provenance: ## Regression evidence plan - The failure mode is exercised by construction: if readiness ever exceeds the - budget again, the thrown error is the enriched `waitForPath` message (with - child stderr), asserted by reading the code path — a dedicated test for the - masking fix would require a child that never becomes ready, i.e. a deliberate - 30 s negative test; that cost is not justified for a CI fixture, and the - unmasking is straight-line control flow reviewed in the diff. + budget again, the thrown error is the enriched `waitForOwnedChildReady` + message (with child stderr), asserted by reading the code path. A child that + *dies* before writing the marker is caught immediately by the `child.exited` + race rather than at the deadline; only a child that stays alive and never + becomes ready costs the full platform budget, and a dedicated test for that + would be a deliberate 45 s negative test on Windows CI — a cost not justified + for a CI fixture, where the unmasking is straight-line control flow reviewed + in the diff. - Positive path: `ci.yml` PR lane plus a `workflow_dispatch` `lane=all` run on the exact PR head; the Windows shard executing `tests\config\config-mutation-lock.test.ts` must pass, and the run must