Repository navigation
fix(ci,runtime): stop hiding the Bun runtime crash and pin back to 1.4.0 - #4821
Merged
Merged
Conversation
Bun re-loads the bunfig preload for every test file under --isolate (reload_entry_point_for_test_runner -> load_preloads), so a 219-file Windows shard evaluates tests/preload.ts 219 times in one process. #4796 added 'import { afterAll } from "bun:test"' to that graph to move temp-root cleanup onto the test lifecycle. On the 67th re-load Bun dereferenced a dead JSPromise in its own module loader and took SIGSEGV at 0x10, inside load_preloads and before the file's first test ran. Windows shard 5/6 failed that way on both attempts of dispatch runs 35087572377 (dc9d1fa) and 35093667426 (89bdf5f). Linux shards took the same fault as exit 139 at five unrelated batch boundaries, where the batch runner hid it by re-running each file alone: one preload load never reaches the second that faults. Run 35054231781 at 8a1b010 ran the same files in the same order, without the bun:test edge, and was green. Cleanup returns to the process exit listener, now on a 250ms removal budget because there is one listener per test file and the 15s default that a foreground teardown uses would let a contended exit outlive the job timeout. The containment and reclamation halves of #4796 are untouched, and reclamation does not depend on a handler firing. tests/ci-workflows/test-preload-module-graph.test.ts walks the preload's load-time graph, fails on any bun:test edge with the chain that introduced it, and is driven red by a fixture one hop away.
…he runtime" This reverts commit fac562d.
… is fixed Bun 1.4.2 segfaults while re-loading the bunfig preload. Under --isolate Bun re-enters tests/preload.ts once per test file, and the fault is in that path: load_preloads -> wait_for_promise -> drainMicrotasks -> moduleLoadTopSettled -> JSModuleLoader::loadModule -> JSPromise::performPromiseThenWithInternalMicrotask -> JSPromise::status, dereferencing a dead JSCell. Symbolicated from the bun.report link the Windows shard printed. It is a crash in the interpreter; no test content avoids it, and it needs at least two preload loads in one process. A/B, two Linux CI runs 40 minutes apart either side of #4064 (02bc10e), the commit that moved the pin to 1.4.2: 34274464811 6bd3274 bun 1.4.0 0 panics / 4 shards 34278407438 02bc10e bun 1.4.2 12 panics / 4 shards Also clean on 1.4.0 at ff524ae (34273011879); crashing on 1.4.2 at 365e1ca, db8996d and every sampled tree from 2026-09-10 onward. Windows shard 5/6 failed on both attempts of runs 35087572377, 35093667426 and 35098735960, always at the 67th file. The file at that position changed from codex-prompt-layers.test.ts to codex-prompt-layers-read.test.ts between candidates, and moving codex-prompt-layers.test.ts into shard 1 left shard 1 green, so the crash is bound to the preload re-load count and not to any test file. Reverses only the version-bearing hunks of 02bc10e: package.json, bun.lock, Dockerfile, and the install-scripts source oracle that pins both package.json values. The unrelated workflow catch-up in that commit is left alone. 1.4.1 is untested. This pin is a claim about 1.4.0, which has direct CI evidence on seven trees, and about 1.4.2, which crashes on every tree sampled. It is not a claim that 1.4.1 is affected: no run in this repository ever used it. Whoever moves this line next should treat 1.4.1 as an open question worth checking, not as known-bad, and should prove any move with a green lane=all dispatch rather than an ordinary PR run, because the Linux batch sweep hides this class.
… classifier Linux CI had been segfaulting twelve to fourteen times per run since 2026-09-08 and reporting green every time. run-bun-test-batches.sh classified the crash, re-ran the batch one file per process, and returned success when that sweep passed. The sweep was never a retry of a flaky test: one file per process is a configuration in which this class of defect cannot occur, so it was guaranteed to pass and guaranteed to report nothing. That is why a Bun 1.4.2 regression reached a release candidate with only the Windows lane, which has no sweep, showing it. The sweep is kept, because proving every file passes alone is the useful half: it says the defect is in multi-file process state, not in a test. What changes is the verdict. A runtime crash now records its batch, finishes the shard so one run attributes every crash site rather than the first, and exits non-zero with an error annotation. A sweep that finds a genuinely failing file still reports that file immediately. A timeout is unchanged and may still recover, because a timeout is a load condition. The classifier itself was four copies: one in the batch runner and three inline in ci.yml. ci-workflows.test.ts pinned them in sync, which only detects the drift it was written to expect - #2152 had already broken one copy by anchoring on panic(thread 2852) when Bun also emits panic(main thread). They collapse into scripts/ci/bun-crash-signatures.sh, which every lane sources. The Windows and macOS legs gain the 132-139 status arm they never had, so a silent fatal signal with no banner is now caught there too. Exit 3 is covered through the signature arm rather than the status list. It is what Windows Bun returns alongside a SIGSEGV panic, but unlike 128+N it is an ordinary small exit code any process may return, and trusting it bare would reclassify a real failure as a crash and hide it - the same mistake in the opposite direction. The banner corroborates it. tests/ci-workflows/ci-bun-crash-classifier.test.ts owns both contracts. The macOS lane harness now copies the real classifier into its sandbox rather than a stand-in, and its signature cases exit 3 instead of 139 so the signature arm is what they actually exercise - with 139 the status arm matched first and those cases would have passed against an empty list.
…two assertions Two unrelated Windows failures, both of which blocked a green lane=all dispatch just as hard as the Bun crash did. client-injection-guard: the first child-spawning scenario in the file paid a one-time cost and was billed for it. Run 35098735960 spent 30.446s in "deny" and hit the 30s deadline, then ran its siblings in 17.038s and 1.3-2.0s; run 35093667426 paid 3.814s first against 1.6-2.7s later. The deadline is a guard-behaviour budget, so first-touch transpilation, SQLite creation and Windows ACL work do not belong inside it. A beforeAll now walks the real path once and throws the result away. It swallows its own errors on purpose: the hook asserts nothing, and letting it throw would turn one broken scenario into seven failures pointing at a warm-up. server-search: the exact-search 429 case did not receive a 429 at all. It received a local 401, which the fake upstream cannot produce - it only ever returns 429 - so the request never reached the fixture. Credential resolution failed first. The case was starting a whole server between writing the fixture credential store and reading it, widening the window in which the process-wide OPENCODEX_HOME can move underneath it, and that startup is unrelated to the routing contract being asserted. It now calls handleSearch directly, which is what every other case in this file already does. Account selection, the public selector, cooldown state and the no-failover assertion are unchanged; the request-log lookup becomes a direct read of the log context the handler fills. Neither is a raised limit. The 30s guard deadline and the 45s composed watchdog both stay exactly where they were. The "fixture upstream connection reset" and the compact JSON parse error in the same job belong to server-auth.test.ts and a malformed-body case that both passed; they are log noise, not this failure.
…alog lock Windows shard 4/6 of run 35093667426 failed the WP13 A-reduced composed acceptance case after 47,774 ms with "timed out waiting for runtime-port record; child exit=null" and both child pipes still open. Healthy controls ran the same case in 13,962 ms and 13,718 ms, so the child spent 33.8 s somewhere between spawn and publishing runtime-port.json. That number has an owner. Startup cache invalidation took catalog lock K before it checked desired state, and K resolves the effective SID and LocalAppData through PowerShell, each on a 30,000 ms budget (src/codex/user-identity.ts, src/lib/windows-user-principal.ts). Results are cached per process only after they succeed, so a cold resolution that runs out its budget under six concurrent shards is paid in full. The case that failed is the one where Codex is explicitly OFF, which is precisely the case that had no reason to take K at all. An explicit OFF now short-circuits before the lock. The permit callback still re-reads intent under K, so a disable racing an enabled startup is unchanged, and enabled startup behaviour is untouched. Absence still means ON, so only an explicit false takes the new path. The composed fixture gains the assertion that an OFF startup creates no catalog-lock database, which is red against the previous code because the database was opened before the callback could look at intent. It also gains incremental stdout/stderr capture and PID/runtime-record diagnostics: the old failure printed "<stdout still open>", which proved only that EOF had not arrived and not that the child had said nothing. If another pre-record path stalls, the next log distinguishes pre-PID, post-PID, malformed-record and post-runtime health failures instead of reporting one bare timeout. The 45 s watchdog is unchanged, and src/server/index.ts stays exactly at its 893-line ratchet.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (17)
📝 WalkthroughWalkthroughChangesBun runtime handling
Codex startup synchronization
Integration test stability
Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant CIWorkflow
participant BunTestBatch
participant BunCrashClassifier
participant BunRuntime
CIWorkflow->>BunTestBatch: run test batch
BunTestBatch->>BunRuntime: execute tests
BunRuntime-->>BunTestBatch: status and log
BunTestBatch->>BunCrashClassifier: classify status and log
BunCrashClassifier-->>BunTestBatch: runtime crash or other failure
BunTestBatch-->>CIWorkflow: retry, attribute, or fail shard
Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
…uth home
Windows shard 1/6 of run 35108652486 threw out of this file's afterEach:
error: EPERM: operation not permitted, rm
...\Temp\opencodex-test-WXhR6c\tmp\ocx-management-auth-fDchUb
at removeTestTempTree (scripts/test-temp.ts:198)
at removeTreeWithRetry (tests/helpers/remove-tree.ts:27)
at tests/server/server-management-auth.test.ts:206
It is a leaked handle, not a timing race, and the budget was never the
answer. hardenConfigDir() spawns icacls.exe, which holds the directory
open until it exits, and Windows file locking is mandatory. #4789 filed
this exact failure at this exact line when the retry budget was 2.5s;
#4796 raised it to a 15s exponential schedule, and this run exhausted all
15 seconds and still got EPERM. Six times the wait changed nothing,
because the handle does not close on the remover's schedule. The process
that started the child has to wait for it.
src/config/paths.ts already documents this and already ships the wait.
The afterEach now awaits flushConfigDirHardeningForTests() first. Twenty
other test files do the same; this one starts servers and drives the ACL
path hardest and did not.
The all-directories variant is the required one. server.stop already
flushes, but through flushConfigDirHardening(), which defaults to
getConfigDir() read at stop time - and this hook restores OPENCODEX_HOME
to the real home a few lines before the removal, so a directory-scoped
flush would settle the wrong tree and leave this one held.
Three things deliberately unchanged. The 15s budget stays: a wider retry
would be treating the symptom, and this run proves it does not work
anyway. The fake icacls runners this file installs are all synchronous,
so awaiting the flush cannot hang on one. And removeTestTempTree keeps
throwing rather than reporting and continuing - that throw is what
surfaced the leak, and softening it would re-hide exactly the class of
defect the batch-runner change stopped hiding this morning.
79 other test files start a server, remove a tree in a cleanup hook, and
never flush. None has failed this way, because this is the file that
drives icacls deliberately, so they are left alone rather than edited
blind. The general guard belongs in the removal helper and is a separate
change.
lidge-jun
marked this pull request as ready for review
September 16, 2026 15:49
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. |
This was referenced Sep 16, 2026
agentHits
pushed a commit
to agentHits/opencodex
that referenced
this pull request
Sep 17, 2026
…4.0 (lidge-jun#4821) Release-blocker fix for 2.57.0. The PR aggregate ci check is green at this exact head, and the lane=all dispatch 35113947092 on the same head passed all six Windows shards individually plus both macOS shards, every test shard, gates, keyring, docker and npm-global; only the optional macos control job was cancelled by concurrency. That dispatch is the evidence the Bun pin works: windows 5/6 had failed three release candidates in a row and 1/6 failed the previous dispatch, and all six pass here with the crash masking removed. The suite had been crashing the Bun runtime on every run since 2026-09-08 while reporting green, because a crashed batch was re-run one file per process, which is a configuration in which the defect cannot occur rather than a retry. That verdict now fails. Host-owned merge decision; no local suite, typecheck, build, or install was run.
2 of 3 tasks
3 tasks done
Closed
3 tasks done
3 of 7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The test suite has been crashing the Bun runtime on every CI run since 2026-09-08 and reporting green. This fixes the crash, stops the reporting from hiding it, and repairs the two Windows failures that were underneath.
The crash is a Bun 1.4.2 regression, not a test
The Windows shard emitted a bun.report link that symbolicates into Bun's own module loader: the test runner re-loads the bunfig preload once per file under isolation, and the fault lands in that re-load, before the file's module is evaluated. That is why no test of the named file ever printed a result. Three observations rule out any file being the cause: the shard crashed at the same position on three candidates while the file at that position changed; a file that moved to another shard did not take the crash with it; and on Linux the same fault lands at five unrelated batch boundaries whose frontier files share no import, with every file passing alone.
The bisect lands on the dependency bump of 2026-09-08 that moved
dependencies.bunfrom 1.4.0 to 1.4.2, with CI runs forty minutes either side of it showing the change. This reverts only the version-bearing hunks. 1.4.1 is untested here and the pin is not a claim about it.The runner was guaranteed to report nothing
On a runtime crash the batch runner re-ran the batch one file per process and returned success. That is not a retry of a flaky test: one preload load never reaches the second that faults, so the sweep was a configuration in which the defect cannot occur. Linux CI has been segfaulting twelve to fourteen times per run and passing.
The sweep is kept for attribution and its verdict changes: a crash records its batch, lets the shard finish so one run attributes every crash site rather than only the first, and exits non-zero with an error annotation. A sweep that finds a genuinely failing file still reports that file. A timeout is untouched, because a timeout is a load condition.
Four inline signature lists collapse into one shared file used by the Linux batch runner and all three workflow legs, so Windows and macOS gain the fatal-signal arm they never had. Exit 3 is classified through the panic banner rather than the bare status, deliberately: unlike 128+N it is an ordinary small exit code, and trusting it bare would reclassify a real failure as a crash and hide it the other way.
The two failures underneath
Windows cold start and a credential race were being charged to two assertions that were not about them, and an explicitly disabled Codex startup was taking the catalog lock.
Verification
lane=alldispatch on this branch is the only thing that can prove the pin, because an ordinary run cannot while the sweep masks. That dispatch is the gate for this PR.Checklist
lane=alldispatch with all six Windows shards passing on their own.Summary by CodeRabbit
Bug Fixes
Documentation