Uh oh!
There was an error while loading. Please reload this page.
🤖 ci: fix merge-queue flake families (unit segfault double-run, integration oversubscription) - #3939
Conversation
…tion oversubscription)
ibetitsmike
commented
Aug 24, 2026
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:0ff84cc340
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ibetitsmike
commented
Aug 24, 2026
@codex review Addressed both findings: inner stream-end waits now use the suite budget, and the jest worker count comes from the shared workerBudgetFor computation instead of a CLI override. |
This comment has been minimized.
This comment has been minimized.
ibetitsmike
commented
Aug 24, 2026
@codex review |
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This comment has been minimized.
This comment has been minimized.
ibetitsmike
commented
Aug 24, 2026
@codex review Follow-up commit: a fresh merge-queue kick today (run 32719185534, PR #3932) failed the same way on tests/ui/layout/analyticsEscape.test.ts, whose 60s budget died at 66s alongside focus.test.ts, on a PR touching neither area. Gave it the same budget treatment as focus (60s -> 120s outer, 10s -> 30s inner waits) and noted the run ID. Prior clean verdict was for f802ed3; re-requesting for this head. |
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This comment has been minimized.
This comment has been minimized.
ibetitsmike
commented
Aug 24, 2026
@codex review Follow-up: the previous CI run (32720490232) exposed a second memory failure mode under the bounded worker budget: with fewer workers, each one runs more test files and integration suites leak enough per file that workers died at V8's ~4GB heap cap ("Jest worker ran out of memory and crashed"). Added |
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Fixes the three mechanisms that have been kicking fully validated PRs out of the merge queue: a unit-job Bun segfault caused by a desynced test-file exclusion list, integration-job cancellation caused by the job timeout colliding with jest's own test timeout, and integration test flakes caused by jest worker memory oversubscription on CI runners.
Background
Recent merge-queue evidence (all on green PRs whose diffs never touched the failing areas):
Segmentation fault(exit 132). The logs show the crash is not teardown noise: it happens mid-SandboxHostServicesuite inside the monolithic coverage run.src/node/services/sandbox/sandboxHostService.test.tswas added to theisolated_unit_testslist in 🤖 feat: add shared agent-foundation layer (event spine, journal kit, sandbox host, capability grants) #3865 (QuickJS-heavy suites are known to crash Bun under coverage in a shared process) but was never added to the hand-maintained duplicatefindexclusion list, so the suite ran a second time inside the shared process, where it segfaults intermittently.testTimeoutis 10 minutes (jest.config.js), so any single hung test makes jest wait right through the 10-minute job cap; the runner then kills the job and destroys all diagnostics.focus.test.tsin run 32715740782 kicking PR 🤖 feat: sync a per-project VS Code .code-workspace file with worktrees #3932,sendModeDropdown.test.tsin run 32666269009,anthropicCacheStrategy.test.tsin run 32665306160, plusanalyticsEscape.test.tsat 66s of its 60s budget in run 32719185534 kicking PR 🤖 feat: sync a per-project VS Code .code-workspace file with worktrees #3932's attempt 5, plus earlierundo.test.ts/analyticsHeader.test.tskicks): all share one mechanism. CI runs jest with--maxWorkers=100%: 16 forked workers on the 16-core/64GB runner at ~4.7GiB peak each (perscripts/lib/worker_budget.js) oversubscribes memory (~75GiB peak) and thrashes. In the failing logs, suites that normally finish in seconds pass at 100-135s wall time while the only tests that fail are the ones with tight explicit timeouts (30s/45s), timing out in setup. The--maxWorkers=100%flag predates the repo's memory-aware worker budget (🤖 ci: run tests/ui on browser OR backend changes #1326 vs 🤖 fix: align composer pickers and size local workers by memory #3760), so CI never got the sizing fix that local runs already use.Implementation
findexclusions from theisolated_unit_testsarray instead of maintaining a duplicate list, so the two can never drift again. This removes the double-run of the crashing suite (and fixes the whole desync class rather than one instance). No exit-code masking anywhere: a genuinely failing test still fails the job exactly as before.--maxWorkersoverride sojest.config.js's memory- and cgroup-awareworkerBudgetFor("jest")sizes the pool (per Codex review; CLI flags take precedence over config), andtimeout-minutes: 10→20so a hung test hits jest's 10-minutetestTimeoutand produces a real failure with logs instead of a log-destroying job cancellation.workerIdleMemoryLimit: "2GB"recycles leaky workers between files.focus.test.ts30s → 120s (aligned with its siblingundo.test.ts)anthropicCacheStrategy.test.ts45s → 120s (peer provider tests already budget 45-150s per live call), andanalyticsEscape.test.ts60s → 120s after it died at 66s in run 32719185534. Other flagged tests already use the harness's 30s windows and should recover via the worker fix; they were left untouched to avoid churn.Live-API tests (e.g.
anthropicCacheStrategy) are deliberately not excluded frommerge_group: the mq run is the last gate that executes tests for the merged combination (main-push runs skip test jobs for the merge-queue bot), so quarantining them there would remove real coverage. The single observed kick was timeout-tuning, not provider outage.Validation
findinvocation and the new derived one side by side; the old list containssandboxHostService.test.ts(the double-run), the new list differs by exactly that one file (759 → 758, no other adds/removes).TEST_INTEGRATION=1 bun x jest tests/ui/review/focus.test.ts tests/ipc/providers/anthropicCacheStrategy.test.tspasses locally (2/2, including the live Anthropic call).make lint-actions(actionlint + zizmor) andmake static-checkpass.Risks
CI-config only plus two test-timeout constants; no production code. The main tradeoff is failure-detection latency: a genuinely hung integration run is now killed at 20 min instead of 10 (but now with jest diagnostics at the 10-minute testTimeout), and the two touched tests take up to 120s before reporting a real hang. Suite wall time at 8 workers may shift slightly in either direction (less thrash, fewer lanes); healthy runs have ample headroom against the 20-minute cap either way.
Generated with
xum• Model:anthropic:claude-fable-5• Thinking:xhigh