test(ci): give the Windows nested live-lock case its own lock owner - #5078
Conversation
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
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
chore
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change adds a Windows-only controller for nested live-lock validation. The runner starts it outside the repository, verifies healthy and refused child cases, checks foreign-owner protection, and confirms parent environment isolation. ChangesNested live-lock regression
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Other · Severity of issue fixed: Low Sequence Diagram(s)sequenceDiagram
participant WindowsRunner
participant NestedLiveLockController
participant BunTestChild
participant TestRunLock
WindowsRunner->>NestedLiveLockController: spawn with temp root and deadline
NestedLiveLockController->>TestRunLock: acquire or join live lock
NestedLiveLockController->>BunTestChild: spawn healthy and refusal cases
BunTestChild->>TestRunLock: validate lock capability
BunTestChild-->>NestedLiveLockController: return receipt or refusal
NestedLiveLockController->>TestRunLock: test foreign-owner timeout and release own lock
NestedLiveLockController-->>WindowsRunner: emit JSON receipt and exit status
Merge Risk: ⚪ Minimal · up to The Windows regression now runs through an isolated lock controller while preserving the outer batch no-queue setting. No actionable merge risk was identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a979029fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const controller = spawnSync( | ||
| process.execPath, | ||
| [helperPath("nested-test-run-lock-controller.ts"), root], | ||
| { cwd: root, env: controllerEnv, encoding: "utf8", timeout: SPAWN_BUDGET_MS }, |
There was a problem hiding this comment.
On a loaded Windows runner, this 45-second timeout can terminate a healthy controller before it emits its receipt. The controller performs four sequential spawnSync calls, each explicitly allowed 15 seconds, may spend up to 10 seconds acquiring its lock, and then runs the foreign-owner probe; therefore valid individual operations can exceed the enclosing budget in aggregate. Size the controller/test timeout for the complete sequence or share one deadline across the child runs so that ordinary Windows startup contention does not make this regression test fail spuriously.
Useful? React with 👍 / 👎.
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.
…ee 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.
1 participant
Summary
The nested live-lock regression in
tests/ci-workflows/test-runner.test.tsregistered only whenprocess.platform === "win32" && process.env[TEST_RUN_NO_QUEUE_ENV] !== "1". The hosted Windowsbatch leg sets
OCX_TEST_NO_QUEUE: "1"on its Test in fresh-process batches step, so the casewas skipped on the only platform it applies to. Nested Windows lock inheritance, member creation,
preload and home-guard propagation, and incomplete-capability refusal were all unverified.
The step-wide no-queue setting from #4876 is unchanged. The case now brings its own lock holder, so
it no longer depends on the outer lane having one.
hosted Windows lane.
tests/helpers/nested-test-run-lock-controller.ts, runs as a plainbun <file>child under an isolated
mkdtemproot. Only the environment copy handed to it has the no-queueopt-out removed; the outer process environment, the real home, and any pre-existing owner are
never modified. Its cwd is outside the repository so Bun loads no bunfig preload into the holder
itself, which would otherwise take the same lock and then wait on itself.
resolveWrappedTestRunLockPathand takes itwith
acquireTestRunLockfor its own run id. No owner file is written by hand. When a wrapped orbare Windows run has already published a complete capability, it joins that owner rather than
creating a second one for the same path, and then releases nothing.
it. The healthy child returns booleans only: path match, run match, token match, member receipt,
preload executed, home guard armed.
OCX_TEST_PRELOAD_PIDandOCX_TEST_HOME_GUARDare strippedfrom every child environment so those two receipts have to come from the child's own preload
rather than from inheritance.
capability is incomplete), a foreigntoken (
exact live owner no longer matches), and a foreign lock path (refusing inherited lock access). Each is checked for a non-zero exit, the matching refusal on stderr, and the absence ofa receipt on stdout.
waits it out and gives up rather than reclaiming it, leaving that owner byte-identical. The
elapsed floor is asserted, so an immediate refusal carrying the right message does not pass.
finallythe controllerrequires the full spawn count to have been waited on, then releases only a lock it acquired and
re-reads the planted foreign owner afterwards. There is no broad kill.
receiptRedactedscans the serialized body for one rather than trusting the token it happens toknow. Child stdout is parsed, never echoed.
Two deadlines, and where the child budget lives
The controller is handed an absolute deadline 10s below the caller's
spawnSynctimeout and boundseach child by what is left of it minus a cleanup reserve. Without that, four child ceilings could
outlast the 45s hard kill on a failure path, terminating the controller inside a spawn with the lock
still held and its teardown never reached. The
spawnSynctimeout remains the backstop, and nobudget was widened.
The nominal per-child timeout is declared by the test and passed to the controller rather than
re-derived inside it. That keeps the deadline bounding four cold Bun starts with the case that owns
them, and keeps
tests/ci-workflows/cold-spawn-warmup.test.tsseeing this file: an earlier revisionmoved the constant into the helper, which orphaned this file's disposition and failed
test 3/4andwindows 7/9.Closes #4991
Verification
No local suite was run. No local
bun test, focused test, typecheck, build, install, orocxinvocation was executed in this checkout. Verification is static reasoning plus hosted CI.
platform-windowsisworkflow_dispatch-only, so it was dispatched against this branch. Onhosted
windows-latestshard 6/9 of run 35380618954, at head499ca44f27, withOCX_TEST_NO_QUEUE=1actually set on the batch step, the case is recorded as executed rather thanskipped:
Both the healthy receipt and the three rejection receipts are inside that assertion: the case
fails unless all twelve controller booleans are true and diagnostics are empty. All nine Windows
shards are green at that head.
test 1/4..4/4,macos 1/2..2/2, gates, storage policy, api usage, docker smoke, keyring,structure gate, npm-global, hygiene and enforce-target are green at the same head.
No new
*.test.tsfile was added, soscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.jsonneed no entry;tests/test-layout.test.tsindexes only*.test.tsand treatstests/helpers/as support-only.File-size ratchet: neither touched file has a cap in
tests/fixtures/file-size-baseline.json, andboth are well under the 2000-line threshold.
Known limits
...\bun-test-locksdirectory, because that path is whatresolveInheritedTestRunLockvalidatesby host digest and basename. A relocated lock would be rejected by the very check under test. A
wrapped or bare Windows run already holds that lock, so the acquire branch is reached only when
no-queue is set.
reclaimStaleLockinscripts/test-run-lock.tshas a pre-existing check-then-rename race betweentwo contenders. This change adds one contender on a lane that has no other, and closing the race
would mean editing the lock implementation, which is outside this fix.
Checklist