ci(windows): the batch leg is one test run, so it must not queue against itself - #4876
Conversation
…nst itself dev went red on windows 4/9 of dispatch 35191675127, and the cause is mine. #4851 replaced the Windows leg's single `bun test` invocation with 25 sequential batch invocations. tests/preload.ts takes a user-scoped machine-local lock, and line 101 makes it win32-only, so Linux and macOS have run this same batch runner unqueued for a long time while Windows had never batched at all. The lock joins workers that share a run ID and blocks anything with a different one, so each batch now queued behind the previous batch's stragglers: [test] bare Bun worker 2548 is waiting for test run pid 7272 to release the user lock. ##[warning]Bun test process timed out after 480s in shard 4/9 batch 6/25. ##[error]Shard 4/9 batch 6: every file passed alone The first file of that batch then ran in 11.25s during attribution. The eight minutes were queue, not work, which is why a larger bound would only have hidden it for longer. scripts/test-run-lock.ts already names this case in its own timeout message: set OCX_TEST_NO_QUEUE=1 only when overlapping test runners are intentional. A dedicated CI job running its own batches back to back is one logical test run, so the batch step now sets it. What the queue protects against - an unrelated second suite stacking load on a developer's machine - cannot happen in that job, and each batch still creates its isolated home and arms the live-home and service-manager guards before it would have reached the lock. The lock's own unit tests now pin an explicitly queued environment, so the workflow bypass can never silently turn their acquisitions into no-ops. That was the real risk in disabling a guard by environment variable. The 480-second batch bound and the 30-minute job ceiling are unchanged. No local suite, focused test, typecheck, build, or install was run.
|
✅ 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 (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe Windows batch workflow now disables the user-scoped test queue. Documentation records the behavior. CI tests verify queue-bypass propagation and explicitly preserve queued lock tests. ChangesCI queue bypass
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to This PR changes only Windows CI queueing and its validation; no production behavior or material current-head risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (3 skipped: 3 unsupported.)
✨ 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 |
리뷰 · 우선순위 76 / 80이 PR은 Windows CI 배치 다리가 자기 자신과 큐잉하지 않도록 고칩니다. 현재 고치는 방법은 한도를 늘리는 게 아니라, Windows 배치 step에만 위험했던 부분은 env로 가드를 끄는 순간 단위 테스트까지 같이 no-op이 되는 것이었습니다. 이 PR은 types.ts/config.ts 분할과 무관하고, 중복 PR도 아닙니다. base는 .github/workflows/ci.yml (platform-windows Test env) - OCX_TEST_NO_QUEUE=1을 Windows 배치 step에만 두어, win32 전용 user lock이 연속 배치를 서로 막는 경로를 끈다. 한도 확대가 아니라 큐 제거다. tests/ci-workflows/test-runner.test.ts (queuedTestEnv) - 잠금 단위 테스트가 프로세스에 상속된 OCX_TEST_NO_QUEUE에 물리지 않도록 빈 env로 큐 경로를 고정한다. 우회가 테스트를 공허하게 만드는 위험을 닫는다. tests/ci-workflows/ci-workflows.test.ts (windowsTest.env.OCX_TEST_NO_QUEUE) - 워크플로 shape 테스트가 Windows step에 우회 env가 남는 것을 고정한다. 나중에 env가 빠지면 CI shape가 바로 빨개진다. tests/ci-workflows/ci-crash-disposition.test.ts (noQueueFlags) - primary뿐 아니라 attribution 스윕에도 플래그가 전달되는지 확인한다. 실패 프로세스 생존자가 진단 스윕까지 다시 큐에 태우는 회귀를 막는다. structure/ops/docs-and-release.md · structure/overview.md - Windows 배치가 한 logical runner라는 계약을 문서에 반영한다. 워크플로만 바꾸고 문서가 남는 드리프트를 피한다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: 3776e44144
ℹ️ 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".
| BUN_TEST_FILE_SCOPE: all | ||
| BUN_TEST_BATCH_SIZE: "6" | ||
| BUN_TEST_BATCH_TIMEOUT_SECONDS: "480" | ||
| OCX_TEST_NO_QUEUE: "1" |
There was a problem hiding this comment.
Setting this variable on the whole workflow step propagates it into every Bun test process, so tests/ci-workflows/test-runner.test.ts:1038 evaluates its Windows-only test.if(... && process.env[TEST_RUN_NO_QUEUE_ENV] !== "1") to false. That test is the only executed coverage proving nested Windows Bun processes inherit and validate the live lock capability; because it is already skipped on non-Windows platforms, this change silently disables it everywhere. Scope the bypass so the outer batch runner consumes it without exposing it to test code, or explicitly restore the queued environment for this integration case.
AGENTS.md reference: .github/AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
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. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 3776e44144c832361ce93ae5feb19b99a2ab8220.
The current Codex review finding is valid and blocks this workflow change. Setting OCX_TEST_NO_QUEUE=1 on the entire Windows batch step reaches every Bun test process. tests/ci-workflows/test-runner.test.ts gates its only Windows nested-lock integration case with process.env[TEST_RUN_NO_QUEUE_ENV] !== "1", so that case is now skipped on Windows; it was already skipped on Linux/macOS. The new queuedTestEnv = {} arguments preserve direct unit cases but do not restore this process-level integration test.
Please scope the bypass so only the outer batch-runner lock is disabled while the tests receive an explicitly queued environment, or change the Windows integration fixture so it deliberately clears the bypass and proves the complete inherited lock capability without competing with the workflow runner. Add a workflow-shape assertion for that boundary. The in-progress lane=all result cannot close this blocker because the current head can report green with the relevant integration test silently skipped.
… diagnostics The evidence this PR's budget argument rested on turned out to be an artifact of how it was produced. Both evidence branches were dispatched from refs whose merge-base predates #4876, and workflow_dispatch reads the workflow from the dispatched ref, so those runs used a ci.yml without OCX_TEST_NO_QUEUE and reproduced exactly the batch-serialization bug #4876 had already fixed. The batch-4 log shows it directly: one line saying a bare Bun worker is waiting for another test run to release the user lock, then eight minutes with no (pass) at all. Confirmed on the branches: OCX_TEST_NO_QUEUE appears 0 times in codex/ci-evidence-4875's ci.yml and once in dev's. So whether these deadlines are actually too small is undetermined again, and the COLD_SPAWN_BUDGET_MS use is withdrawn. Both files keep their original bounds. What does not depend on that evidence stays. Neither wait observed its child, so a child that died and one that was merely slow produced the same message; both now race the exit and report the code and stderr immediately, re-checking the awaited signal first so a child that signalled and then exited is not misreported. And the owner harness's timeout branch awaited a promise that only resolves at EOF while describing a child that is still running, which could never settle; it is bounded now. Those are diagnostic defects on their own terms, and they are what would have made the original evidence readable in the first place.
…eout (#4902) * test(windows): size the first-child readiness wait for cold start, and report dead children Two Windows shards failed on two different files during heavy queue contention, and both are the same defect: an in-test deadline that has to cover a Windows cold first child start, sized for an idle runner. run 35211904734 windows 3/9 failed at waitFor(holdMarker), not at the contention assertion -- that line was never reached, and holdMs was never approached. run 35210400258 windows 7/9 failed at the first wait of its case, for "listening", with no events at all. dev at 6d19a07 passed all nine Windows shards on an idle queue (run 35215552842), so neither is a defect in the code under test. This is not a number raised to make red go away. The deadlines were smaller than the range this repository has already measured for the wait they bound. COLD_SPAWN_BUDGET_MS records a first child publishing at 50.7s while the next spawn in the same file was ready in 1.76s, with surviving readiness waits from 2.0s to 19.7s; codex-write-lock.test.ts says in its own comment that a holder child boots in 8-19s on a loaded shard, and bounded that wait at 15s. A case the file calls normal is slower than the deadline it has to meet. Each file now spends that named ceiling exactly once, on its first child, as the constant's own contract requires; every later wait keeps the ordinary bound. The second defect is that neither wait observed the child. A child that died and one that was merely slow produced the same message, so CI could not tell them apart, and the two want opposite fixes. Both waits now race the exit and report the code and stderr immediately. That is also what makes the native-main evidence readable: its stderr promise resolves at EOF, so an empty stderr in that message means the child had already exited silently rather than that it was still booting. The timeout branch in the owner harness also awaited that same EOF promise while claiming the child was still running, which could never settle. It is bounded now. Closes #4901 * test(windows): drop the readiness-budget change, keep the child-death diagnostics The evidence this PR's budget argument rested on turned out to be an artifact of how it was produced. Both evidence branches were dispatched from refs whose merge-base predates #4876, and workflow_dispatch reads the workflow from the dispatched ref, so those runs used a ci.yml without OCX_TEST_NO_QUEUE and reproduced exactly the batch-serialization bug #4876 had already fixed. The batch-4 log shows it directly: one line saying a bare Bun worker is waiting for another test run to release the user lock, then eight minutes with no (pass) at all. Confirmed on the branches: OCX_TEST_NO_QUEUE appears 0 times in codex/ci-evidence-4875's ci.yml and once in dev's. So whether these deadlines are actually too small is undetermined again, and the COLD_SPAWN_BUDGET_MS use is withdrawn. Both files keep their original bounds. What does not depend on that evidence stays. Neither wait observed its child, so a child that died and one that was merely slow produced the same message; both now race the exit and report the code and stderr immediately, re-checking the awaited signal first so a child that signalled and then exited is not misreported. And the owner harness's timeout branch awaited a promise that only resolves at EOF while describing a child that is still running, which could never settle; it is bounded now. Those are diagnostic defects on their own terms, and they are what would have made the original evidence readable in the first place.
|
Post-merge follow-up: the step-wide |
2 participants
Summary
devwent red onwindows 4/9of dispatch 35191675127, and the cause is mine.#4851 replaced the Windows leg's single
bun testinvocation with 25 sequential batch invocations.tests/preload.tstakes a user-scoped machine-local lock, and line 101 makes it win32-only — so Linux and macOS have run this same batch runner unqueued for a long time, while Windows had never batched at all. The lock joins workers sharing a run ID and blocks anything with a different one, so each batch began queueing behind the previous batch's stragglers:The first file of that batch then ran in 11.25s during attribution. The eight minutes were queue, not work — which is exactly why a larger bound would only have hidden it for longer.
scripts/test-run-lock.tsalready names this case in its own timeout message: setOCX_TEST_NO_QUEUE=1only when overlapping test runners are intentional. A dedicated CI job running its own batches back to back is one logical test run, so the batch step now sets it. What the queue protects against — an unrelated second suite stacking load on a developer's machine — cannot happen inside that job, and each batch still creates its isolated home and arms the live-home and service-manager guards before it would have reached the lock.The lock's own unit tests now pin an explicitly queued environment, so the workflow bypass can never silently turn their acquisitions into no-ops. That was the real hazard in disabling a guard by environment variable, and it is closed rather than assumed away.
The 480-second batch bound and the 30-minute job ceiling are unchanged. Nothing was widened.
Verification
No local suite, focused test, typecheck, build, or install was run; this lane is hosted-CI-only by task contract. Verification is static plus exact-head hosted CI, and the Windows behaviour can only be proven by a
lane=alldispatch on this head.Evidence the wait was a queue rather than work:
The alternative considered and rejected was reverting Windows to a single un-batched invocation. That would have restored green equally well but given up the per-batch bound that names a wedged file, which is why #4851 added batching in the first place.
Checklist
structure/docs updateddevSummary by CodeRabbit
Bug Fixes
Documentation
Tests