From 034c7243e773bf0d9cce9d7f273564ba74531530 Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 17 Sep 2026 12:46:31 +0900 Subject: [PATCH] test(codex): isolate the lock child's database and wait on a real signal `a contender with a deadline waits for the holder instead of failing immediately` went red on dev (run 35177450461, job 105062310488) with the HOLDER reporting busy, which should have been impossible: the parent had already seen its hold marker. Both facts were true. The marker was written from inside the lock callback, so it proved the child had ENTERED the section, not that it finished holding it. The child's coordination database lived under the ambient OPENCODEX_HOME, which every file in the same CI batch shares, so another test reading it could turn the holder's COMMIT into SQLITE_BUSY. With timeoutMs 0 the child had no retry, the acquisition rolled back, and it returned busy after having already published the marker the parent was waiting on. The 150ms sleep was the second half of the same problem. It was standing in for "the waiter is now actually waiting", and nothing made that true - on a loaded runner the parent could release before the contention it exists to measure had begun, which would also have made the waitedMs > 0 assertion a coin flip. So: the child's database moves to the per-test temp root, which removes the cross-file contention entirely; the waiter runs as its own child and publishes a wait marker only after confirming its lock promise did not settle synchronously; and the parent releases the holder only once it has seen that marker. waitedMs now comes back from the child, so the property the case exists to prove is guaranteed by construction rather than by timing. No other blind sequencing remains in this file - every other wait is the existing waitFor, which polls an observable condition. No local suite, focused test, typecheck, build, or install was run. --- .../codex-write-lock.test.ts | 29 +++++++++++++++---- tests/helpers/codex-write-lock-child.ts | 26 +++++++++++++++-- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/tests/codex-integration/codex-write-lock.test.ts b/tests/codex-integration/codex-write-lock.test.ts index b3becfad47..71d0631f53 100644 --- a/tests/codex-integration/codex-write-lock.test.ts +++ b/tests/codex-integration/codex-write-lock.test.ts @@ -286,7 +286,15 @@ describe("two real processes contend for one lock", () => { function spawnChild(payload: Record) { return Bun.spawn(["bun", childPath], { - env: { ...process.env, CODEX_HOME: codexHome, OCX_LOCK_CHILD_PAYLOAD: JSON.stringify(payload) }, + env: { + ...process.env, + CODEX_HOME: codexHome, + // N is the lock under test. Give the child processes in this case their + // own C database so unrelated files in the same Bun batch cannot make a + // holder retry after it has published its held marker. + OPENCODEX_HOME: join(root, ".opencodex"), + OCX_LOCK_CHILD_PAYLOAD: JSON.stringify(payload), + }, stdout: "pipe", stderr: "pipe", }); @@ -298,6 +306,7 @@ describe("two real processes contend for one lock", () => { env: { ...process.env, CODEX_HOME: codexHome, + OPENCODEX_HOME: join(root, ".opencodex"), ...env, OCX_LOCK_CHILD_PAYLOAD: JSON.stringify(payload), }, @@ -309,7 +318,13 @@ describe("two real processes contend for one lock", () => { async function childResult(child: ReturnType) { const [stdout] = await Promise.all([new Response(child.stdout).text(), child.exited]); const line = stdout.trim().split("\n").filter(Boolean).at(-1) ?? "{}"; - return JSON.parse(line) as { status: string; reason?: string; value?: string; lockId?: string }; + return JSON.parse(line) as { + status: string; + reason?: string; + value?: string; + waitedMs?: number; + lockId?: string; + }; } // A spawned holder child boots in 8-19 s on a loaded windows-latest shard; the 10 s @@ -362,14 +377,18 @@ describe("two real processes contend for one lock", () => { test("a contender with a deadline waits for the holder instead of failing immediately", async () => { const holdMarker = join(root, "held-2"); const releaseMarker = join(root, "release-2"); + const waitMarker = join(root, "waiting-2"); const holder = spawnChild({ holdMarker, releaseMarker, timeoutMs: 0, holdMs: 20_000 }); await waitFor(holdMarker); - const waiter = withCodexWriteLock(options({ timeoutMs: 5_000 }), publishing("waited")); - await Bun.sleep(150); + const waiter = spawnChild({ timeoutMs: 5_000, waitMarker }); + // The waiter writes this only after withCodexWriteLock has returned its + // pending promise. Because the holder is still held, that means the waiter + // has attempted N and reached the retry wait rather than failing fast. + await waitFor(waitMarker); writeFileSync(releaseMarker, "go"); - const [waited, holderResult] = await Promise.all([waiter, childResult(holder)]); + const [waited, holderResult] = await Promise.all([childResult(waiter), childResult(holder)]); expect(holderResult.status).toBe("acquired"); expect(waited.status).toBe("acquired"); expect(waited.status === "acquired" && waited.waitedMs).toBeGreaterThan(0); diff --git a/tests/helpers/codex-write-lock-child.ts b/tests/helpers/codex-write-lock-child.ts index e2ff77648a..d9603c352e 100644 --- a/tests/helpers/codex-write-lock-child.ts +++ b/tests/helpers/codex-write-lock-child.ts @@ -17,12 +17,13 @@ const payload = JSON.parse(process.env.OCX_LOCK_CHILD_PAYLOAD ?? "{}") as { timeoutMs?: number; holdMarker?: string; releaseMarker?: string; + waitMarker?: string; holdMs?: number; }; const admitted = { authoritySnapshotId: "authority-child" } as AdmissionSnapshot; -const result = await withCodexWriteLock( +const pending = withCodexWriteLock( { timeoutMs: payload.timeoutMs ?? 0, admitted, @@ -75,9 +76,28 @@ const result = await withCodexWriteLock( }, ); +if (payload.waitMarker) { + let settled = false; + void pending.then( + () => { settled = true; }, + () => { settled = true; }, + ); + // Flush reactions for a promise that completed synchronously. When a holder + // already owns N, an unsettled promise here means withCodexWriteLock tried to + // acquire it and suspended in its retry wait. + await Promise.resolve(); + if (!settled) writeFileSync(payload.waitMarker, "waiting"); +} + +const result = await pending; + console.log(JSON.stringify({ status: result.status, - ...(result.status === "acquired" ? { value: result.value, lockId: result.lockId } : {}), - ...(result.status === "busy" ? { reason: result.reason, lockId: result.lockId } : {}), + ...(result.status === "acquired" + ? { value: result.value, waitedMs: result.waitedMs, lockId: result.lockId } + : {}), + ...(result.status === "busy" + ? { reason: result.reason, waitedMs: result.waitedMs, lockId: result.lockId } + : {}), ...(result.status === "refused" ? { reason: result.reason, message: result.message } : {}), }));