From 5a979029fe3f7647c2cc6df5f47e2b0a37c52071 Mon Sep 17 00:00:00 2001 From: JUN Date: Sat, 19 Sep 2026 03:06:49 +0900 Subject: [PATCH 1/3] test(ci): give the Windows nested live-lock case its own lock owner The nested live-lock regression registered only when OCX_TEST_NO_QUEUE was not 1, and the hosted Windows batch leg sets exactly that, so the case was skipped on the only platform it applies to. A controller child now owns the lock instead of borrowing the lane's: it runs with the opt-out removed for itself alone, resolves the user-scoped path through the ordinary safe path, acquires it for its own run id, and spawns the nested Bun children that must inherit it. The outer environment, the real home, and any pre-existing owner are left untouched. Closes #4991 --- tests/ci-workflows/test-runner.test.ts | 99 +++--- .../nested-test-run-lock-controller.ts | 287 ++++++++++++++++++ 2 files changed, 341 insertions(+), 45 deletions(-) create mode 100644 tests/helpers/nested-test-run-lock-controller.ts diff --git a/tests/ci-workflows/test-runner.test.ts b/tests/ci-workflows/test-runner.test.ts index a40a989ffe6..f2c8e5b83d3 100644 --- a/tests/ci-workflows/test-runner.test.ts +++ b/tests/ci-workflows/test-runner.test.ts @@ -26,7 +26,10 @@ import { selectChangedComparisonRef, SERIAL_FULL_SUITE_FILES, } from "../../scripts/test"; -import { repoPath, repoRoot } from "../helpers/repo-root"; +import { + NESTED_LIVE_LOCK_RECEIPT_KEY, +} from "../helpers/nested-test-run-lock-controller"; +import { helperPath, repoPath, repoRoot } from "../helpers/repo-root"; import { acquireTestRunLock, resolveBareTestRunIdentity, @@ -1059,53 +1062,59 @@ describe("bun test user lock", () => { expect(resolveCalls).toBe(0); }); - test.if(process.platform === "win32" && process.env[TEST_RUN_NO_QUEUE_ENV] !== "1")( - "nested Windows Bun tests inherit the acquired live lock and refuse an incomplete capability", + // Windows-only, and deliberately no longer gated on the no-queue opt-out. The hosted + // batch leg sets OCX_TEST_NO_QUEUE=1 for its own six-file processes, which skipped this + // case on the only platform it covers (#4991). The controller below owns a lock of its + // own rather than borrowing the lane's, so the regression now runs under either setting. + test.if(process.platform === "win32")( + "a nested Windows Bun test inherits an independently owned live lock and refuses an incomplete capability", () => { - const root = mkdtempSync(join(tmpdir(), "opencodex-nested-test-")); + const root = mkdtempSync(join(tmpdir(), "opencodex-nested-lock-")); + const environmentBefore = JSON.stringify({ + noQueue: process.env[TEST_RUN_NO_QUEUE_ENV], + runId: process.env[TEST_RUN_ID_ENV], + lockPath: process.env[TEST_RUN_LOCK_PATH_ENV], + // Presence only. The token is never rendered, here or by the controller. + hasToken: process.env[TEST_RUN_LOCK_TOKEN_ENV] !== undefined, + }); try { - const lockPath = process.env[TEST_RUN_LOCK_PATH_ENV]; - expect(Boolean(lockPath && process.env[TEST_RUN_LOCK_TOKEN_ENV] && process.env[TEST_RUN_ID_ENV])).toBe(true); - const ownerBefore = readFileSync(join(lockPath!, "owner.json"), "utf8"); - const fixture = join(root, "nested.test.ts"); - writeFileSync(fixture, ` - import { test } from "bun:test"; - import { readFileSync, existsSync } from "node:fs"; - import { join } from "node:path"; - test("nested lock receipt", () => { - const path = process.env.OCX_TEST_RUN_LOCK_PATH; - const owner = JSON.parse(readFileSync(join(path, "owner.json"), "utf8")); - console.log(JSON.stringify({ nestedLockReceipt: { - samePath: path === ${JSON.stringify(lockPath)}, - sameRun: owner.runId === ${JSON.stringify(process.env[TEST_RUN_ID_ENV])}, - sameToken: owner.token === process.env.OCX_TEST_RUN_LOCK_TOKEN, - member: existsSync(join(path, "members", process.pid + "-" + owner.token)), - preloadRan: process.env.OCX_TEST_PRELOAD_PID === String(process.pid), - guardArmed: process.env.OCX_TEST_HOME_GUARD === "1", - } })); - }); - `); - const args = ["test", "--preload", repoPath("tests/preload.ts"), fixture]; - const child = spawnSync(process.execPath, args, { - cwd: root, env: { ...process.env }, encoding: "utf8", timeout: INTERNAL_DEADLINE_MS, - }); - // Keep process diagnostics bounded and never render the owner token or child output. - expect(child.status).toBe(0); - const marker = child.stdout.split("\n").find(line => line.startsWith('{"nestedLockReceipt":')); - expect(marker ? JSON.parse(marker).nestedLockReceipt : null).toEqual({ - samePath: true, sameRun: true, sameToken: true, member: true, preloadRan: true, guardArmed: true, - }); - expect(readFileSync(join(lockPath!, "owner.json"), "utf8") === ownerBefore).toBe(true); - - const incomplete = { ...process.env }; - delete incomplete[TEST_RUN_LOCK_TOKEN_ENV]; - const refused = spawnSync(process.execPath, args, { - cwd: root, env: incomplete, encoding: "utf8", timeout: INTERNAL_DEADLINE_MS, + // Only the controller's copy loses the opt-out, and its cwd stays outside the + // repository so Bun loads no bunfig preload into the lock owner itself. + const controllerEnv = { ...process.env }; + delete controllerEnv[TEST_RUN_NO_QUEUE_ENV]; + const controller = spawnSync( + process.execPath, + [helperPath("nested-test-run-lock-controller.ts"), root], + { cwd: root, env: controllerEnv, encoding: "utf8", timeout: SPAWN_BUDGET_MS }, + ); + const prefix = '{"' + NESTED_LIVE_LOCK_RECEIPT_KEY + '":'; + const line = (controller.stdout ?? "").split("\n").find(entry => entry.startsWith(prefix)); + const payload = line + ? JSON.parse(line) as { nestedLiveLockReceipt: Record; diagnostics: string[] } + : null; + // Booleans and redacted controller notes only; raw child output never surfaces here. + expect(payload?.diagnostics ?? ["the controller printed no receipt"]).toEqual([]); + expect(payload?.nestedLiveLockReceipt).toEqual({ + lockOwned: true, + healthyChildExited: true, + healthyReceiptComplete: true, + missingTokenRefused: true, + wrongTokenRefused: true, + wrongPathRefused: true, + foreignOwnerTimedOut: true, + foreignOwnerUntouched: true, + ownerContentUnchanged: true, + childrenReaped: true, + releasedOnlyOwnLock: true, + receiptRedacted: true, }); - expect(refused.status).toBe(1); - expect(refused.stderr.includes("capability is incomplete")).toBe(true); - expect(refused.stdout.includes('{"nestedLockReceipt":')).toBe(false); - expect(readFileSync(join(lockPath!, "owner.json"), "utf8") === ownerBefore).toBe(true); + expect(controller.status).toBe(0); + expect(JSON.stringify({ + noQueue: process.env[TEST_RUN_NO_QUEUE_ENV], + runId: process.env[TEST_RUN_ID_ENV], + lockPath: process.env[TEST_RUN_LOCK_PATH_ENV], + hasToken: process.env[TEST_RUN_LOCK_TOKEN_ENV] !== undefined, + })).toBe(environmentBefore); } finally { removeTreeWithRetry(root); } diff --git a/tests/helpers/nested-test-run-lock-controller.ts b/tests/helpers/nested-test-run-lock-controller.ts new file mode 100644 index 00000000000..75adb2c18c9 --- /dev/null +++ b/tests/helpers/nested-test-run-lock-controller.ts @@ -0,0 +1,287 @@ +/** + * Lock owner for the Windows nested live-lock regression (issue #4991). + * + * The regression in tests/ci-workflows/test-runner.test.ts proves that a nested Bun test + * inherits the live test-run lock exactly and refuses an incomplete capability. It used + * to read that capability out of its own environment, so it could only run while the + * outer process already held a lock — and the hosted Windows batch leg sets + * OCX_TEST_NO_QUEUE=1 precisely so that it does not. The case was therefore skipped on + * the only platform it applies to, and the coverage existed on paper only. + * + * This controller supplies the missing owner instead of borrowing the lane's. It runs as + * a plain "bun " child with exactly one environment change — the no-queue opt-out + * removed for this process and its descendants — resolves the user-scoped lock through + * the ordinary safe path, and acquires it for its own run id. Nothing here writes an + * owner file by hand: a fabricated capability would only prove that a child trusts what + * it is told, which is the inverse of the contract under test. + * + * Two things about how it is launched are load-bearing. It must be spawned with a cwd + * OUTSIDE the repository so Bun loads no bunfig preload into the owner itself; a + * preloaded controller would take the same lock in tests/preload.ts and then wait on + * itself. And it must be handed a temporary root it may write into, because every + * fixture it generates and the foreign-owner probe it plants live there. + * + * Everything below runs only as an entry point. The test file imports the receipt key + * from here, and an import must not acquire a lock or spawn anything. + * + * Output is one JSON line of booleans plus redacted diagnostics. The owner token never + * reaches stdout, and child output is parsed rather than echoed. + */ +import { randomUUID } from "node:crypto"; +import { spawnSync, type SpawnSyncReturns } from "node:child_process"; +import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { join, win32 } from "node:path"; +import { + acquireTestRunLock, + resolveWrappedTestRunLockPath, + TEST_RUN_ID_ENV, + TEST_RUN_LOCK_PATH_ENV, + TEST_RUN_LOCK_TOKEN_ENV, + TEST_RUN_NO_QUEUE_ENV, + type TestRunLock, +} from "../../scripts/test-run-lock"; +import { repoPath } from "./repo-root"; + +/** Shape the caller asserts on; every field must be true for the case to pass. */ +export interface NestedLiveLockReceipt { + lockOwned: boolean; + healthyChildExited: boolean; + healthyReceiptComplete: boolean; + missingTokenRefused: boolean; + wrongTokenRefused: boolean; + wrongPathRefused: boolean; + foreignOwnerTimedOut: boolean; + foreignOwnerUntouched: boolean; + ownerContentUnchanged: boolean; + childrenReaped: boolean; + releasedOnlyOwnLock: boolean; + receiptRedacted: boolean; +} + +export const NESTED_LIVE_LOCK_RECEIPT_KEY = "nestedLiveLockReceipt"; +const CHILD_MARKER = '{"nestedLockReceipt":'; +const CHILD_RECEIPT_KEYS = ["samePath", "sameRun", "sameToken", "member", "preloadRan", "guardArmed"] as const; +const CHILD_DEADLINE_MS = 15_000; +const ACQUIRE_POLL_MS = 250; +const ACQUIRE_MAX_WAIT_MS = 10_000; +const FOREIGN_POLL_MS = 100; +const FOREIGN_MAX_WAIT_MS = 300; + +async function runNestedLiveLockController(tempRoot: string | undefined): Promise { + const receipt: NestedLiveLockReceipt = { + lockOwned: false, + healthyChildExited: false, + healthyReceiptComplete: false, + missingTokenRefused: false, + wrongTokenRefused: false, + wrongPathRefused: false, + foreignOwnerTimedOut: false, + foreignOwnerUntouched: false, + ownerContentUnchanged: false, + childrenReaped: false, + releasedOnlyOwnLock: false, + receiptRedacted: false, + }; + const diagnostics: string[] = []; + const note = (message: string): void => { diagnostics.push(message); }; + const describeError = (error: unknown): string => (error instanceof Error ? error.message : String(error)); + let lock: TestRunLock | undefined; + let ownerFile: string | undefined; + let ownerBefore: string | undefined; + let ownerToken: string | undefined; + // Whether each spawned child was waited on, which is what reaping means for spawnSync. + const settledChildren: boolean[] = []; + + try { + if (process.platform !== "win32") throw new Error("the nested live-lock controller is Windows-only"); + if (!tempRoot) throw new Error("the nested live-lock controller needs a temporary root argument"); + if (process.env[TEST_RUN_NO_QUEUE_ENV] !== undefined) { + throw new Error("the controller environment must have the no-queue opt-out removed"); + } + + // A wrapped or bare Windows run already owns this lock and handed us its complete + // capability; joining it is the honest move, because a second owner for one path is + // exactly the clobber this suite exists to prevent. The hosted no-queue lane has no + // such owner, so there we resolve and acquire one of our own. + const inheritedPath = process.env[TEST_RUN_LOCK_PATH_ENV]?.trim(); + const inheritedToken = process.env[TEST_RUN_LOCK_TOKEN_ENV]?.trim(); + const inheritedRunId = process.env[TEST_RUN_ID_ENV]?.trim(); + const joining = Boolean(inheritedPath && inheritedToken && inheritedRunId); + const resolved = joining ? inheritedPath : resolveWrappedTestRunLockPath({ env: process.env }); + if (!resolved) throw new Error("the user-scoped Bun test lock path did not resolve"); + const lockPath = resolved; + const runId = joining && inheritedRunId ? inheritedRunId : "nested-live-lock-" + randomUUID(); + + lock = await acquireTestRunLock({ + runId, + lockPath, + validatedRuntimePath: true, + env: process.env, + joinExistingOwnerToken: joining ? inheritedToken : undefined, + pollMs: ACQUIRE_POLL_MS, + maxWaitMs: ACQUIRE_MAX_WAIT_MS, + }); + const owner = lock.owner; + if (!owner) throw new Error("the run lock produced no owner record"); + ownerToken = owner.token; + receipt.lockOwned = true; + + // Publish the capability into our own environment so the children below inherit it the + // way any descendant of a real run does, rather than being handed a constructed one. + process.env[TEST_RUN_ID_ENV] = runId; + process.env[TEST_RUN_LOCK_PATH_ENV] = lockPath; + process.env[TEST_RUN_LOCK_TOKEN_ENV] = owner.token; + + const activeOwnerFile = join(lockPath, "owner.json"); + const activeOwnerBefore = readFileSync(activeOwnerFile, "utf8"); + ownerFile = activeOwnerFile; + ownerBefore = activeOwnerBefore; + receipt.ownerContentUnchanged = true; + const confirmOwnerUnchanged = (): void => { + const current = existsSync(activeOwnerFile) ? readFileSync(activeOwnerFile, "utf8") : null; + if (current === activeOwnerBefore) return; + receipt.ownerContentUnchanged = false; + note("the owner receipt changed while a nested child ran"); + }; + + const fixture = join(tempRoot, "nested-live-lock.test.ts"); + writeFileSync(fixture, [ + 'import { test } from "bun:test";', + 'import { existsSync, readFileSync } from "node:fs";', + 'import { join } from "node:path";', + 'test("nested lock receipt", () => {', + ' const path = process.env.OCX_TEST_RUN_LOCK_PATH ?? "";', + ' const owner = JSON.parse(readFileSync(join(path, "owner.json"), "utf8"));', + " console.log(JSON.stringify({ nestedLockReceipt: {", + " samePath: path === " + JSON.stringify(lockPath) + ",", + " sameRun: owner.runId === " + JSON.stringify(runId) + + " && process.env.OCX_TEST_RUN_ID === " + JSON.stringify(runId) + ",", + " sameToken: owner.token === process.env.OCX_TEST_RUN_LOCK_TOKEN,", + ' member: existsSync(join(path, "members", process.pid + "-" + owner.token)),', + " preloadRan: process.env.OCX_TEST_PRELOAD_PID === String(process.pid),", + ' guardArmed: process.env.OCX_TEST_HOME_GUARD === "1",', + " } }));", + "});", + "", + ].join("\n")); + + const args = ["test", "--preload", repoPath("tests", "preload.ts"), fixture]; + const runChild = (mutate?: (env: NodeJS.ProcessEnv) => void): SpawnSyncReturns => { + const env = { ...process.env }; + // Drop the two receipts the child is supposed to produce for itself. Inherited, they + // would report a preload that never ran and a guard nobody armed. + delete env.OCX_TEST_PRELOAD_PID; + delete env.OCX_TEST_HOME_GUARD; + mutate?.(env); + const result = spawnSync(process.execPath, args, { + cwd: tempRoot, env, encoding: "utf8", timeout: CHILD_DEADLINE_MS, + }); + // spawnSync returns only after the child has been waited on, so a settled status or + // signal IS the reap. A liveness probe on the pid would be a race against pid reuse. + settledChildren.push(result.status !== null || result.signal !== null); + return result; + }; + const refusal = (result: SpawnSyncReturns, needle: string, label: string): boolean => { + const refused = result.status !== 0 + && (result.stderr ?? "").includes(needle) + && !(result.stdout ?? "").includes(CHILD_MARKER); + if (!refused) note(label + " was not refused (status " + String(result.status) + ")"); + return refused; + }; + + const healthy = runChild(); + receipt.healthyChildExited = healthy.status === 0; + if (!receipt.healthyChildExited) { + note("the healthy child exited with status " + String(healthy.status) + " signal " + String(healthy.signal)); + } + const marker = (healthy.stdout ?? "").split("\n").find(line => line.startsWith(CHILD_MARKER)); + const nested = marker + ? (JSON.parse(marker) as { nestedLockReceipt?: Record }).nestedLockReceipt + : undefined; + receipt.healthyReceiptComplete = nested !== undefined + && CHILD_RECEIPT_KEYS.every(key => nested[key] === true); + if (!receipt.healthyReceiptComplete) note("nested receipt: " + JSON.stringify(nested ?? null)); + confirmOwnerUnchanged(); + + receipt.missingTokenRefused = refusal( + runChild(env => { delete env[TEST_RUN_LOCK_TOKEN_ENV]; }), + "capability is incomplete", + "a child holding no token", + ); + confirmOwnerUnchanged(); + + receipt.wrongTokenRefused = refusal( + runChild(env => { env[TEST_RUN_LOCK_TOKEN_ENV] = randomUUID(); }), + "exact live owner no longer matches", + "a child holding a foreign token", + ); + confirmOwnerUnchanged(); + + receipt.wrongPathRefused = refusal( + runChild(env => { + env[TEST_RUN_LOCK_PATH_ENV] = win32.join(win32.dirname(lockPath), "opencodex-bun-test-not-this-host.lock"); + }), + "refusing inherited lock access", + "a child holding a foreign lock path", + ); + confirmOwnerUnchanged(); + + // The acquire path must wait out a live owner it does not own and then give up rather + // than reclaim it. Planted under the temporary root so the probe can never reach the + // real lock, and owned by this very pid so its liveness is a fact, not a fixture. + const foreignLock = join(tempRoot, "foreign-owner.lock"); + mkdirSync(foreignLock, { recursive: true, mode: 0o700 }); + const foreignOwnerFile = join(foreignLock, "owner.json"); + const foreignOwner = JSON.stringify({ + version: 1, + runId: "foreign-" + randomUUID(), + token: randomUUID(), + pid: process.pid, + acquiredAt: new Date().toISOString(), + }) + "\n"; + writeFileSync(foreignOwnerFile, foreignOwner, { encoding: "utf8", mode: 0o600 }); + try { + await acquireTestRunLock({ + runId: "timeout-probe-" + randomUUID(), + lockPath: foreignLock, + env: process.env, + pollMs: FOREIGN_POLL_MS, + maxWaitMs: FOREIGN_MAX_WAIT_MS, + }); + note("the controller took a lock a live foreign owner still held"); + } catch (error) { + receipt.foreignOwnerTimedOut = describeError(error).includes("timed out after"); + if (!receipt.foreignOwnerTimedOut) note("unexpected foreign-owner failure: " + describeError(error)); + } + receipt.foreignOwnerUntouched = existsSync(foreignOwnerFile) + && readFileSync(foreignOwnerFile, "utf8") === foreignOwner; + confirmOwnerUnchanged(); + } catch (error) { + note("controller failure: " + describeError(error)); + } finally { + receipt.childrenReaped = settledChildren.length > 0 && settledChildren.every(Boolean); + try { + if (lock?.acquired) { + lock.release(); + receipt.releasedOnlyOwnLock = ownerFile !== undefined && !existsSync(ownerFile); + } else if (lock && ownerFile !== undefined && ownerBefore !== undefined) { + // Joined rather than acquired: leaving the other owner exactly as found IS the claim. + receipt.releasedOnlyOwnLock = existsSync(ownerFile) + && readFileSync(ownerFile, "utf8") === ownerBefore; + } + } catch (error) { + note("release failure: " + describeError(error)); + } + + const token = ownerToken; + const body = { + [NESTED_LIVE_LOCK_RECEIPT_KEY]: receipt, + diagnostics: token ? diagnostics.map(entry => entry.split(token).join("")) : diagnostics, + }; + receipt.receiptRedacted = token === undefined || !JSON.stringify(body).includes(token); + process.stdout.write(JSON.stringify(body) + "\n"); + process.exitCode = diagnostics.length === 0 && Object.values(receipt).every(Boolean) ? 0 : 1; + } +} + +if (import.meta.main) await runNestedLiveLockController(process.argv[2]); From 508d1a3f1479c62da85da6f9aa1c75aef592d5d8 Mon Sep 17 00:00:00 2001 From: JUN Date: Sat, 19 Sep 2026 03:20:47 +0900 Subject: [PATCH 2/3] test(ci): bound the nested live-lock controller and harden its receipt Adversarial review of the first commit found three real weaknesses. The controller could spend more than the caller's 45s hard kill across four child spawns, so a failure path could terminate it inside a spawn with the lock still held and its teardown never reached. It is now handed an absolute deadline 10s short of that kill and bounds every child by what is left of it, minus a cleanup reserve. A join failure inside registerMember can carry a member filename, and that path runs before the controller learns its own token, so a redactor keyed on that token was blind exactly where a leak was possible. Diagnostics now strip every UUID-shaped substring, and receiptRedacted scans for one instead of being vacuously true on the green path. Two other receipts were weak: the acquire-timeout probe accepted the message without waiting, and release checked only that our own owner file was gone. They now require the elapsed floor and the planted foreign owner intact after release. childrenReaped requires the full spawn count so a skipped scenario cannot pass. lockOwned is renamed lockHeld because the controller joins an existing owner when a wrapped run already published one. --- tests/ci-workflows/test-runner.test.ts | 22 +++- .../nested-test-run-lock-controller.ts | 119 ++++++++++++------ 2 files changed, 96 insertions(+), 45 deletions(-) diff --git a/tests/ci-workflows/test-runner.test.ts b/tests/ci-workflows/test-runner.test.ts index f2c8e5b83d3..1988e09c70c 100644 --- a/tests/ci-workflows/test-runner.test.ts +++ b/tests/ci-workflows/test-runner.test.ts @@ -1064,12 +1064,18 @@ describe("bun test user lock", () => { // Windows-only, and deliberately no longer gated on the no-queue opt-out. The hosted // batch leg sets OCX_TEST_NO_QUEUE=1 for its own six-file processes, which skipped this - // case on the only platform it covers (#4991). The controller below owns a lock of its - // own rather than borrowing the lane's, so the regression now runs under either setting. + // case on the only platform it covers (#4991). The controller below holds a lock in its + // own right rather than borrowing the lane's, so the regression now runs either way. + // + // Two deadlines, not one. The controller is told to finish 10s before the hard kill so + // it always reaches its own teardown — releasing the lock and confirming its children + // were reaped — instead of being terminated inside a spawn with the lock still held. + // The spawnSync timeout stays the backstop for a controller that ignores its deadline. test.if(process.platform === "win32")( - "a nested Windows Bun test inherits an independently owned live lock and refuses an incomplete capability", + "a nested Windows Bun test inherits the live lock its controller holds and refuses an incomplete capability", () => { const root = mkdtempSync(join(tmpdir(), "opencodex-nested-lock-")); + const controllerBudgetMs = SPAWN_BUDGET_MS - 10_000; const environmentBefore = JSON.stringify({ noQueue: process.env[TEST_RUN_NO_QUEUE_ENV], runId: process.env[TEST_RUN_ID_ENV], @@ -1079,12 +1085,16 @@ describe("bun test user lock", () => { }); try { // Only the controller's copy loses the opt-out, and its cwd stays outside the - // repository so Bun loads no bunfig preload into the lock owner itself. + // repository so Bun loads no bunfig preload into the lock holder itself. const controllerEnv = { ...process.env }; delete controllerEnv[TEST_RUN_NO_QUEUE_ENV]; const controller = spawnSync( process.execPath, - [helperPath("nested-test-run-lock-controller.ts"), root], + [ + helperPath("nested-test-run-lock-controller.ts"), + root, + String(Date.now() + controllerBudgetMs), + ], { cwd: root, env: controllerEnv, encoding: "utf8", timeout: SPAWN_BUDGET_MS }, ); const prefix = '{"' + NESTED_LIVE_LOCK_RECEIPT_KEY + '":'; @@ -1095,7 +1105,7 @@ describe("bun test user lock", () => { // Booleans and redacted controller notes only; raw child output never surfaces here. expect(payload?.diagnostics ?? ["the controller printed no receipt"]).toEqual([]); expect(payload?.nestedLiveLockReceipt).toEqual({ - lockOwned: true, + lockHeld: true, healthyChildExited: true, healthyReceiptComplete: true, missingTokenRefused: true, diff --git a/tests/helpers/nested-test-run-lock-controller.ts b/tests/helpers/nested-test-run-lock-controller.ts index 75adb2c18c9..e4f2ce99e57 100644 --- a/tests/helpers/nested-test-run-lock-controller.ts +++ b/tests/helpers/nested-test-run-lock-controller.ts @@ -8,24 +8,29 @@ * OCX_TEST_NO_QUEUE=1 precisely so that it does not. The case was therefore skipped on * the only platform it applies to, and the coverage existed on paper only. * - * This controller supplies the missing owner instead of borrowing the lane's. It runs as + * This controller supplies the missing holder instead of borrowing the lane's. It runs as * a plain "bun " child with exactly one environment change — the no-queue opt-out * removed for this process and its descendants — resolves the user-scoped lock through - * the ordinary safe path, and acquires it for its own run id. Nothing here writes an - * owner file by hand: a fabricated capability would only prove that a child trusts what - * it is told, which is the inverse of the contract under test. + * the ordinary safe path, and acquires it for its own run id. When a wrapped or bare + * Windows run has already published a complete capability it joins that owner instead, + * because a second owner for one path is the clobber this suite exists to prevent. + * Nothing here writes an owner file by hand: a fabricated capability would only prove + * that a child trusts what it is told, which is the inverse of the contract under test. * - * Two things about how it is launched are load-bearing. It must be spawned with a cwd - * OUTSIDE the repository so Bun loads no bunfig preload into the owner itself; a + * Three things about how it is launched are load-bearing. It must be spawned with a cwd + * OUTSIDE the repository so Bun loads no bunfig preload into the holder itself; a * preloaded controller would take the same lock in tests/preload.ts and then wait on - * itself. And it must be handed a temporary root it may write into, because every - * fixture it generates and the foreign-owner probe it plants live there. + * itself. It must be handed a temporary root it may write into, because every fixture it + * generates and the foreign-owner probe it plants live there. And it must be handed a + * deadline: every child it spawns is bounded by what is left of that deadline minus a + * cleanup reserve, so the controller always reaches its own teardown rather than being + * killed inside a spawn with the lock still held. * * Everything below runs only as an entry point. The test file imports the receipt key * from here, and an import must not acquire a lock or spawn anything. * - * Output is one JSON line of booleans plus redacted diagnostics. The owner token never - * reaches stdout, and child output is parsed rather than echoed. + * Output is one JSON line of booleans plus diagnostics with every UUID-shaped substring + * removed. Child output is parsed, never echoed. */ import { randomUUID } from "node:crypto"; import { spawnSync, type SpawnSyncReturns } from "node:child_process"; @@ -44,7 +49,7 @@ import { repoPath } from "./repo-root"; /** Shape the caller asserts on; every field must be true for the case to pass. */ export interface NestedLiveLockReceipt { - lockOwned: boolean; + lockHeld: boolean; healthyChildExited: boolean; healthyReceiptComplete: boolean; missingTokenRefused: boolean; @@ -61,15 +66,28 @@ export interface NestedLiveLockReceipt { export const NESTED_LIVE_LOCK_RECEIPT_KEY = "nestedLiveLockReceipt"; const CHILD_MARKER = '{"nestedLockReceipt":'; const CHILD_RECEIPT_KEYS = ["samePath", "sameRun", "sameToken", "member", "preloadRan", "guardArmed"] as const; +/** Healthy, missing token, foreign token, foreign path. A short count means one was skipped. */ +const EXPECTED_CHILD_SPAWNS = 4; const CHILD_DEADLINE_MS = 15_000; const ACQUIRE_POLL_MS = 250; const ACQUIRE_MAX_WAIT_MS = 10_000; const FOREIGN_POLL_MS = 100; const FOREIGN_MAX_WAIT_MS = 300; +/** Time kept back from every child so teardown runs before the caller's hard kill. */ +const CLEANUP_RESERVE_MS = 4_000; +const MINIMUM_CHILD_ALLOWANCE_MS = 1_000; +/** + * Any UUID, not merely the token this process knows about. Several errors in + * scripts/test-run-lock.ts can carry a member filename, and one of them is reachable + * before acquire returns, so a redactor keyed on our own token would be blind exactly + * where a leak is possible. Built fresh per call because a global regex carries + * lastIndex between a replace and a test. + */ +const uuidPattern = (): RegExp => /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/gi; -async function runNestedLiveLockController(tempRoot: string | undefined): Promise { +async function runNestedLiveLockController(tempRoot: string | undefined, deadlineAt: number): Promise { const receipt: NestedLiveLockReceipt = { - lockOwned: false, + lockHeld: false, healthyChildExited: false, healthyReceiptComplete: false, missingTokenRefused: false, @@ -85,28 +103,34 @@ async function runNestedLiveLockController(tempRoot: string | undefined): Promis const diagnostics: string[] = []; const note = (message: string): void => { diagnostics.push(message); }; const describeError = (error: unknown): string => (error instanceof Error ? error.message : String(error)); + const budgetLeftMs = (): number => deadlineAt - Date.now() - CLEANUP_RESERVE_MS; let lock: TestRunLock | undefined; let ownerFile: string | undefined; let ownerBefore: string | undefined; let ownerToken: string | undefined; + let foreignOwnerFile: string | undefined; + let foreignOwner: string | undefined; // Whether each spawned child was waited on, which is what reaping means for spawnSync. const settledChildren: boolean[] = []; try { if (process.platform !== "win32") throw new Error("the nested live-lock controller is Windows-only"); if (!tempRoot) throw new Error("the nested live-lock controller needs a temporary root argument"); + if (!Number.isFinite(deadlineAt)) throw new Error("the nested live-lock controller needs a deadline argument"); if (process.env[TEST_RUN_NO_QUEUE_ENV] !== undefined) { throw new Error("the controller environment must have the no-queue opt-out removed"); } - // A wrapped or bare Windows run already owns this lock and handed us its complete - // capability; joining it is the honest move, because a second owner for one path is - // exactly the clobber this suite exists to prevent. The hosted no-queue lane has no - // such owner, so there we resolve and acquire one of our own. + // A wrapped or bare Windows run already holds this lock and handed us its complete + // capability, so join it. The hosted no-queue lane has no such holder, and there we + // resolve and acquire one of our own. const inheritedPath = process.env[TEST_RUN_LOCK_PATH_ENV]?.trim(); const inheritedToken = process.env[TEST_RUN_LOCK_TOKEN_ENV]?.trim(); const inheritedRunId = process.env[TEST_RUN_ID_ENV]?.trim(); const joining = Boolean(inheritedPath && inheritedToken && inheritedRunId); + // Known before the join can fail, so a failure inside registerMember cannot reach the + // diagnostics with a live token the redactor has not been told about. + if (joining) ownerToken = inheritedToken; const resolved = joining ? inheritedPath : resolveWrappedTestRunLockPath({ env: process.env }); if (!resolved) throw new Error("the user-scoped Bun test lock path did not resolve"); const lockPath = resolved; @@ -119,12 +143,12 @@ async function runNestedLiveLockController(tempRoot: string | undefined): Promis env: process.env, joinExistingOwnerToken: joining ? inheritedToken : undefined, pollMs: ACQUIRE_POLL_MS, - maxWaitMs: ACQUIRE_MAX_WAIT_MS, + maxWaitMs: Math.max(ACQUIRE_POLL_MS, Math.min(ACQUIRE_MAX_WAIT_MS, budgetLeftMs())), }); const owner = lock.owner; if (!owner) throw new Error("the run lock produced no owner record"); ownerToken = owner.token; - receipt.lockOwned = true; + receipt.lockHeld = true; // Publish the capability into our own environment so the children below inherit it the // way any descendant of a real run does, rather than being handed a constructed one. @@ -166,7 +190,11 @@ async function runNestedLiveLockController(tempRoot: string | undefined): Promis ].join("\n")); const args = ["test", "--preload", repoPath("tests", "preload.ts"), fixture]; - const runChild = (mutate?: (env: NodeJS.ProcessEnv) => void): SpawnSyncReturns => { + const runChild = (label: string, mutate?: (env: NodeJS.ProcessEnv) => void): SpawnSyncReturns => { + const allowance = Math.min(CHILD_DEADLINE_MS, budgetLeftMs()); + if (allowance < MINIMUM_CHILD_ALLOWANCE_MS) { + throw new Error("the controller ran out of budget before spawning " + label); + } const env = { ...process.env }; // Drop the two receipts the child is supposed to produce for itself. Inherited, they // would report a preload that never ran and a guard nobody armed. @@ -174,7 +202,7 @@ async function runNestedLiveLockController(tempRoot: string | undefined): Promis delete env.OCX_TEST_HOME_GUARD; mutate?.(env); const result = spawnSync(process.execPath, args, { - cwd: tempRoot, env, encoding: "utf8", timeout: CHILD_DEADLINE_MS, + cwd: tempRoot, env, encoding: "utf8", timeout: Math.floor(allowance), }); // spawnSync returns only after the child has been waited on, so a settled status or // signal IS the reap. A liveness probe on the pid would be a race against pid reuse. @@ -189,7 +217,7 @@ async function runNestedLiveLockController(tempRoot: string | undefined): Promis return refused; }; - const healthy = runChild(); + const healthy = runChild("the healthy child"); receipt.healthyChildExited = healthy.status === 0; if (!receipt.healthyChildExited) { note("the healthy child exited with status " + String(healthy.status) + " signal " + String(healthy.signal)); @@ -204,21 +232,21 @@ async function runNestedLiveLockController(tempRoot: string | undefined): Promis confirmOwnerUnchanged(); receipt.missingTokenRefused = refusal( - runChild(env => { delete env[TEST_RUN_LOCK_TOKEN_ENV]; }), + runChild("the tokenless child", env => { delete env[TEST_RUN_LOCK_TOKEN_ENV]; }), "capability is incomplete", "a child holding no token", ); confirmOwnerUnchanged(); receipt.wrongTokenRefused = refusal( - runChild(env => { env[TEST_RUN_LOCK_TOKEN_ENV] = randomUUID(); }), + runChild("the foreign-token child", env => { env[TEST_RUN_LOCK_TOKEN_ENV] = randomUUID(); }), "exact live owner no longer matches", "a child holding a foreign token", ); confirmOwnerUnchanged(); receipt.wrongPathRefused = refusal( - runChild(env => { + runChild("the foreign-path child", env => { env[TEST_RUN_LOCK_PATH_ENV] = win32.join(win32.dirname(lockPath), "opencodex-bun-test-not-this-host.lock"); }), "refusing inherited lock access", @@ -231,15 +259,18 @@ async function runNestedLiveLockController(tempRoot: string | undefined): Promis // real lock, and owned by this very pid so its liveness is a fact, not a fixture. const foreignLock = join(tempRoot, "foreign-owner.lock"); mkdirSync(foreignLock, { recursive: true, mode: 0o700 }); - const foreignOwnerFile = join(foreignLock, "owner.json"); - const foreignOwner = JSON.stringify({ + const plantedFile = join(foreignLock, "owner.json"); + const planted = JSON.stringify({ version: 1, runId: "foreign-" + randomUUID(), token: randomUUID(), pid: process.pid, acquiredAt: new Date().toISOString(), }) + "\n"; - writeFileSync(foreignOwnerFile, foreignOwner, { encoding: "utf8", mode: 0o600 }); + writeFileSync(plantedFile, planted, { encoding: "utf8", mode: 0o600 }); + foreignOwnerFile = plantedFile; + foreignOwner = planted; + const probeStartedAt = Date.now(); try { await acquireTestRunLock({ runId: "timeout-probe-" + randomUUID(), @@ -250,38 +281,48 @@ async function runNestedLiveLockController(tempRoot: string | undefined): Promis }); note("the controller took a lock a live foreign owner still held"); } catch (error) { - receipt.foreignOwnerTimedOut = describeError(error).includes("timed out after"); + // The elapsed floor is the point: an immediate refusal would satisfy the message + // alone while proving nothing about waiting for the holder. + receipt.foreignOwnerTimedOut = describeError(error).includes("timed out after") + && Date.now() - probeStartedAt >= FOREIGN_MAX_WAIT_MS; if (!receipt.foreignOwnerTimedOut) note("unexpected foreign-owner failure: " + describeError(error)); } - receipt.foreignOwnerUntouched = existsSync(foreignOwnerFile) - && readFileSync(foreignOwnerFile, "utf8") === foreignOwner; + receipt.foreignOwnerUntouched = existsSync(plantedFile) + && readFileSync(plantedFile, "utf8") === planted; confirmOwnerUnchanged(); } catch (error) { note("controller failure: " + describeError(error)); } finally { - receipt.childrenReaped = settledChildren.length > 0 && settledChildren.every(Boolean); + receipt.childrenReaped = settledChildren.length === EXPECTED_CHILD_SPAWNS + && settledChildren.every(Boolean); try { + // Releasing must remove our own lock and nothing else, so the planted foreign owner + // is re-read afterwards rather than only before. + const foreignIntact = foreignOwnerFile === undefined + || (existsSync(foreignOwnerFile) && readFileSync(foreignOwnerFile, "utf8") === foreignOwner); if (lock?.acquired) { lock.release(); - receipt.releasedOnlyOwnLock = ownerFile !== undefined && !existsSync(ownerFile); + receipt.releasedOnlyOwnLock = ownerFile !== undefined && !existsSync(ownerFile) && foreignIntact; } else if (lock && ownerFile !== undefined && ownerBefore !== undefined) { - // Joined rather than acquired: leaving the other owner exactly as found IS the claim. + // Joined rather than acquired: leaving the other holder exactly as found IS the claim. receipt.releasedOnlyOwnLock = existsSync(ownerFile) - && readFileSync(ownerFile, "utf8") === ownerBefore; + && readFileSync(ownerFile, "utf8") === ownerBefore + && foreignIntact; } } catch (error) { note("release failure: " + describeError(error)); } - const token = ownerToken; const body = { [NESTED_LIVE_LOCK_RECEIPT_KEY]: receipt, - diagnostics: token ? diagnostics.map(entry => entry.split(token).join("")) : diagnostics, + diagnostics: diagnostics.map(entry => entry.replace(uuidPattern(), "")), }; - receipt.receiptRedacted = token === undefined || !JSON.stringify(body).includes(token); + receipt.receiptRedacted = !uuidPattern().test(JSON.stringify(body)); process.stdout.write(JSON.stringify(body) + "\n"); process.exitCode = diagnostics.length === 0 && Object.values(receipt).every(Boolean) ? 0 : 1; } } -if (import.meta.main) await runNestedLiveLockController(process.argv[2]); +if (import.meta.main) { + await runNestedLiveLockController(process.argv[2], Number(process.argv[3])); +} From 499ca44f27b7b1b33f793f1e51de5a6baa3162a3 Mon Sep 17 00:00:00 2001 From: JUN Date: Sat, 19 Sep 2026 03:30:42 +0900 Subject: [PATCH 3/3] test(ci): keep the nested case's child deadline where the guard can see it Moving the per-child timeout into the controller removed this file's only spawn-options INTERNAL_DEADLINE_MS, so the cold-spawn warm-up guard stopped matching it and its disposition became an orphan. That failed test 3/4 on Linux and windows 7/9. The deadline that bounds four cold Bun starts belongs to the case that owns them, not to the helper, so the test now declares the child spawn options and hands them over; the controller only narrows them to what its own deadline still allows. The guard's inventory and its scan agree again, and the helper no longer re-derives a budget constant. --- tests/ci-workflows/test-runner.test.ts | 7 +++++ .../nested-test-run-lock-controller.ts | 30 ++++++++++++++----- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/tests/ci-workflows/test-runner.test.ts b/tests/ci-workflows/test-runner.test.ts index 1988e09c70c..9ac74058420 100644 --- a/tests/ci-workflows/test-runner.test.ts +++ b/tests/ci-workflows/test-runner.test.ts @@ -1071,11 +1071,17 @@ describe("bun test user lock", () => { // it always reaches its own teardown — releasing the lock and confirming its children // were reaped — instead of being terminated inside a spawn with the lock still held. // The spawnSync timeout stays the backstop for a controller that ignores its deadline. + // + // The nominal per-child timeout is declared here rather than inside the helper, so the + // deadline that bounds four cold Bun starts stays with the case that owns them and + // tests/ci-workflows/cold-spawn-warmup.test.ts keeps seeing this file. The controller + // narrows it to whatever its own deadline still allows. test.if(process.platform === "win32")( "a nested Windows Bun test inherits the live lock its controller holds and refuses an incomplete capability", () => { const root = mkdtempSync(join(tmpdir(), "opencodex-nested-lock-")); const controllerBudgetMs = SPAWN_BUDGET_MS - 10_000; + const childSpawn = { timeout: INTERNAL_DEADLINE_MS }; const environmentBefore = JSON.stringify({ noQueue: process.env[TEST_RUN_NO_QUEUE_ENV], runId: process.env[TEST_RUN_ID_ENV], @@ -1094,6 +1100,7 @@ describe("bun test user lock", () => { helperPath("nested-test-run-lock-controller.ts"), root, String(Date.now() + controllerBudgetMs), + JSON.stringify(childSpawn), ], { cwd: root, env: controllerEnv, encoding: "utf8", timeout: SPAWN_BUDGET_MS }, ); diff --git a/tests/helpers/nested-test-run-lock-controller.ts b/tests/helpers/nested-test-run-lock-controller.ts index e4f2ce99e57..964d8e951d5 100644 --- a/tests/helpers/nested-test-run-lock-controller.ts +++ b/tests/helpers/nested-test-run-lock-controller.ts @@ -21,10 +21,11 @@ * OUTSIDE the repository so Bun loads no bunfig preload into the holder itself; a * preloaded controller would take the same lock in tests/preload.ts and then wait on * itself. It must be handed a temporary root it may write into, because every fixture it - * generates and the foreign-owner probe it plants live there. And it must be handed a - * deadline: every child it spawns is bounded by what is left of that deadline minus a - * cleanup reserve, so the controller always reaches its own teardown rather than being - * killed inside a spawn with the lock still held. + * generates and the foreign-owner probe it plants live there. And it must be handed both + * a deadline and the caller's spawn options: the nominal per-child timeout belongs to the + * case that owns these children, and the controller only narrows it to what is left of + * the deadline minus a cleanup reserve, so the controller always reaches its own teardown + * rather than being killed inside a spawn with the lock still held. * * Everything below runs only as an entry point. The test file imports the receipt key * from here, and an import must not acquire a lock or spawn anything. @@ -68,7 +69,6 @@ const CHILD_MARKER = '{"nestedLockReceipt":'; const CHILD_RECEIPT_KEYS = ["samePath", "sameRun", "sameToken", "member", "preloadRan", "guardArmed"] as const; /** Healthy, missing token, foreign token, foreign path. A short count means one was skipped. */ const EXPECTED_CHILD_SPAWNS = 4; -const CHILD_DEADLINE_MS = 15_000; const ACQUIRE_POLL_MS = 250; const ACQUIRE_MAX_WAIT_MS = 10_000; const FOREIGN_POLL_MS = 100; @@ -85,7 +85,15 @@ const MINIMUM_CHILD_ALLOWANCE_MS = 1_000; */ const uuidPattern = (): RegExp => /[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/gi; -async function runNestedLiveLockController(tempRoot: string | undefined, deadlineAt: number): Promise { +interface ChildSpawnOptions { + timeout: number; +} + +async function runNestedLiveLockController( + tempRoot: string | undefined, + deadlineAt: number, + childSpawn: ChildSpawnOptions | undefined, +): Promise { const receipt: NestedLiveLockReceipt = { lockHeld: false, healthyChildExited: false, @@ -117,6 +125,9 @@ async function runNestedLiveLockController(tempRoot: string | undefined, deadlin if (process.platform !== "win32") throw new Error("the nested live-lock controller is Windows-only"); if (!tempRoot) throw new Error("the nested live-lock controller needs a temporary root argument"); if (!Number.isFinite(deadlineAt)) throw new Error("the nested live-lock controller needs a deadline argument"); + if (!childSpawn || !Number.isFinite(childSpawn.timeout) || childSpawn.timeout <= 0) { + throw new Error("the nested live-lock controller needs the caller's child spawn options"); + } if (process.env[TEST_RUN_NO_QUEUE_ENV] !== undefined) { throw new Error("the controller environment must have the no-queue opt-out removed"); } @@ -191,7 +202,7 @@ async function runNestedLiveLockController(tempRoot: string | undefined, deadlin const args = ["test", "--preload", repoPath("tests", "preload.ts"), fixture]; const runChild = (label: string, mutate?: (env: NodeJS.ProcessEnv) => void): SpawnSyncReturns => { - const allowance = Math.min(CHILD_DEADLINE_MS, budgetLeftMs()); + const allowance = Math.min(childSpawn.timeout, budgetLeftMs()); if (allowance < MINIMUM_CHILD_ALLOWANCE_MS) { throw new Error("the controller ran out of budget before spawning " + label); } @@ -324,5 +335,8 @@ async function runNestedLiveLockController(tempRoot: string | undefined, deadlin } if (import.meta.main) { - await runNestedLiveLockController(process.argv[2], Number(process.argv[3])); + const spawnOptions = process.argv[4] + ? JSON.parse(process.argv[4]) as ChildSpawnOptions + : undefined; + await runNestedLiveLockController(process.argv[2], Number(process.argv[3]), spawnOptions); }