Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds a shared Bun test policy for timeouts and concurrency. All package runners and the script CLI adopt the policy. Auth tests add explicit per-test timeouts and worker-pool scheduling. Behavioral tests verify policy and runner invariants. ChangesBun test execution policy
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
WalkthroughBefore this PR, each Bun test runner carried its own timeout, concurrency, and process-management choices, so workspaces such as CLI, auth, agents, core, and scripts could drift apart in how aggressively they parallelized tests and how they handled slow or timed-out suites. After the PR, those runners all draw from one shared Bun test scheduling policy, giving the repository a single source of truth for scheduling behavior while still preserving workspace-specific notes where needed. Release NotesNew Features
Bug Fixes
Tests
Refactor
Changes
Sequence DiagramsequenceDiagram
participant SharedPolicy as Shared Bun test scheduling policy
participant Runner as Workspace/shards runner
participant Child as bun test child process
Runner->>SharedPolicy: request concurrency and timeout policy
SharedPolicy-->>Runner: unified scheduling settings
Runner->>Child: spawn isolated test process with policy-derived timeout/concurrency
Child-->>Runner: per-file pass/fail result
Runner->>Runner: aggregate results under shared policy contract
Magnitude🎯 2 (M) Related
Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
|
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
CI was green on only 9 of the last 20 first attempts. Nearly every PR needed a rerun, and the failures never named the change under review: a different file timed out on each run and every one of them passed in isolation. Four workspace runners each carried their own concurrency and timeout policy. `agents` was tuned against measurement in #3084; the other three were not, and the shards still running the untuned policy are the shards that fail — `cli` 5/15 first attempts, `agents` 4/16, `core` 2/14. The tests were not broken, they were starved. `cli` had the worst of both ends: a pool sized to the full core count and a 30s per-test bound. `auth` passed no `--timeout` at all, so it ran on Bun's 5s default; a `[test] timeout` key in bunfig.toml does not help, because Bun 1.3.14 ignores it. `scripts/run_bun_tests.ts` defaulted to 30s while the workspace runner beside it used 180s, so the same test had two different budgets depending on which path CI took. Throughput does not degrade gracefully when the pool matches the core count; it collapses. Measured on the 680-file `cli` suite, 16 cores: processes per core wall clock 0.25 184s 0.50 124s 1.00 >9min, not one file completed GitHub's ubuntu-latest runner has 4 vCPUs, and `cli` was asking for 4 processes — exactly the setting where nothing finishes. Half the cores is both the fastest point measured and the one that leaves a test able to finish inside its budget. scripts/lib/bun-test-policy.ts now owns that policy and the runners consume it. Differences that were deliberate are preserved: `core` keeps its cap of 2, `cli` keeps a larger budget for the integration files that spawn the real CLI, and every runner keeps its concurrency override for pinning a run while chasing a flake. What is gone is the divergence nobody chose. Two defects surfaced while wiring it up. `auth` scheduled in fixed batches, so a batch advanced only when its slowest file finished and left workers idle with queued work; it now uses the same worker pool as the others. And `cli`'s integration budget was a fixed 120s, which silently became *smaller* than the unit budget once that rose to 180s, handing the slowest tests in the workspace the tightest bound — it is now expressed as a multiple of the shared budget, so it cannot invert again. The invariant that allowed this is now a test: no runner may spawn `bun test` without an explicit `--timeout`, none may recompute concurrency for itself, and all must derive from the shared module. `scripts/tests/` files are listed individually in tsconfig.scripts.json, so the new suite is registered there — adding it immediately caught a type error the file would otherwise have carried. Cost, measured rather than assumed: `core` 95.59s before, 95.91s after.
scripts/run_bun_tests.ts spawns bun test children and was changed by this PR to share the timeout policy, but it was missing from RUNNERS — so the invariants that pin the fix did not actually cover it. Removing its --timeout or its import of the shared module would have gone unnoticed, which is the exact failure this suite exists to prevent. It already satisfies all three invariants; adding it only closes the hole in the coverage.
The module comment still described a per-file timeout of 60s on POSIX and 180s on Windows. Those values moved to the shared policy in this PR, so the comment now contradicted the code it sits above.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/auth/run-bun-tests.ts`:
- Around line 216-231: Update the timeout handling in runTestFile so a timed-out
child sets a timeout flag and sends SIGKILL but does not resolve immediately;
resolve the test promise from the child’s close event, following the pattern in
run-bun-tests.ts for the child lifecycle. This keeps the worker occupied until
the process has fully closed while preserving the timeout result.
In `@scripts/lib/bun-test-policy.ts`:
- Around line 116-126: Update the environment override handling in the policy
function around options.envVar to validate the parsed integer with
Number.isSafeInteger() before returning it. Reject values that are not safe
integers using the existing invalid-override error path, while preserving valid
positive integer overrides and the current unclamped behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 76292270-008f-4727-bac6-3ae56f59f59e
📒 Files selected for processing (10)
packages/agents/run-bun-tests.tspackages/auth/run-bun-tests.tspackages/auth/src/__tests__/run-bun-tests.behavior.test.tspackages/cli/run-bun-tests.tspackages/core/run-bun-tests.tspackages/core/tsconfig.runner.jsonscripts/lib/bun-test-policy.tsscripts/run_bun_tests.tsscripts/tests/bun-test-policy.bun.test.tstsconfig.scripts.json
CodeRabbit caught that auth reported a timed-out file from inside the timeout callback. kill() only sends a signal, so the result was produced while the child was still winding down — and because this PR converted auth from fixed batches to a worker pool, the freed slot is filled immediately. The pool would exceed its concurrency cap exactly when the machine is already struggling, which is the failure this PR exists to remove. cli had the same defect and already used a worker pool, so it was doing this in production. Both now settle from the exit handler and carry the timeout reason on a flag; core already awaited the reap and agents already settled from exit, so those were correct. killProcessTree sends SIGKILL to the process group, which cannot be ignored, so exit is guaranteed to arrive. Also rejects a concurrency override too large to be an exact integer: the digit-shape check accepted an arbitrarily long run of digits, which parseInt rounds to an imprecise Number that would then size the worker pool. I dropped the source-scanning test I first wrote for the reaping rule. core resolves inside its timeout callback too, but only after awaiting the kill, which is correct — and no text heuristic distinguishes that from resolving immediately. A guard that cannot tell right from wrong is worse than none, so the requirement is documented on the budget it belongs to instead.
A full local suite on a machine running several checkouts at once still timed out two agents files that pass in about a second standalone. That is not something the policy can size around: availableParallelism() reports the machine's cores, so half of them is still more than the runner actually has when other work already owns the box. CI runners are dedicated, so the two coincide there and the issue this fixes is unaffected. The env override is the escape hatch on a shared machine, and the comment now says so rather than leaving the next person to rediscover it.
Collaborator
Author
Note on the
|
Open
7 tasks
This was referenced Aug 9, 2026
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
1 participant
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.
TLDR
CI was green on only 9 of the last 20 first attempts. The failures never named the change under review — a different file timed out on each run, and every one of them passed in isolation.
Four workspace test runners each carried their own concurrency and timeout policy.
agentswas tuned against measurement in #3084; the other three were not, and the shards still running the untuned policy are exactly the shards that fail. The tests were not broken, they were starved.This gives all of them one policy, and pins the invariant that let them drift.
Reviewers should look at: the measurement table below (it is the whole argument), and
scripts/lib/bun-test-policy.ts.Dive Deeper
The measurement that decides it
Throughput does not degrade gracefully when the pool matches the core count — it collapses. The 680-file
clisuite on a 16-core machine:GitHub's
ubuntu-latesthas 4 vCPUs, andcliwas asking for 4 processes — precisely the setting where nothing finishes. Half the cores is both the fastest point measured and the one that leaves a test able to finish inside its budget.What each runner was doing
agents(tuned in #3084)clicoreauthscripts/run_bun_tests.tsclihad the worst of both ends.authpassed no--timeoutat all — and a[test] timeoutkey inbunfig.tomldoes not help, because Bun 1.3.14 ignores it.scripts/run_bun_tests.tsdefaulted to 30s while the workspace runner beside it used 180s, so the same test had two different budgets depending on which path CI took.What is centralised, and what is not
scripts/lib/bun-test-policy.tsowns concurrency and the timeout budgets. Differences that were deliberate are preserved:corekeeps its cap of 2 (its files are unusually heavy).clikeeps a larger budget for integration files that spawn the real CLI.What is gone is the divergence nobody chose.
Two defects found while wiring it up
authscheduled in fixed batches. A batch advanced only when its slowest file finished, leaving workers idle with queued work. It now uses the same worker pool as the other runners.cli's integration budget was a fixed 120s — which silently became smaller than the unit budget once that rose to 180s, handing the slowest tests in the workspace the tightest bound. It is now a multiple of the shared budget, so it cannot invert again. This was caught by an existing test in the repo, not by me.Ruled out
SIGKILLs in CI logs are the runners' own per-file timeout kills.isolateStorageRoots()usesmkdtempSync, so every process gets a unique root.PermissionsModifyTrustDialog.test.tsxfailed twice on a branch whose diff contained zeropackages/cli/files.Reviewer Test Plan
1. The policy and its invariants.
The last three cases are the ones that matter: no runner may spawn
bun testwithout an explicit--timeout, none may recompute concurrency for itself, and all must derive from the shared module. To see them bite, delete the--timeoutargument from any runner and re-run — the suite names the offending file.2. Confirm the collapse for yourself. On an N-core machine:
3. Each runner still works.
4. The real verdict is this PR's own CI — see the result below.
Result on this PR's own CI
The first attempt was green: 39 pass, 0 fail, 0 reruns.
Shard cost, measured rather than assumed (
Test (ubuntu-latest) [cli], attempt 1):The
clishard costs about two minutes more. That is a real cost and worth stating plainly — but a shard that failed on 3 of those 4 first attempts had to be run roughly twice to land, so the expected time to a greenclishard goes down, and the human round-trip of noticing a spurious failure and clicking rerun disappears.Every other shard on this run:
agents6min,scripts8min,providers4min,rest3min,core2min — all green.One run is not a flake rate. The honest measure is the first-attempt green rate of the PRs that follow this one, against the 9/20 baseline.
Testing Matrix
Verified on macOS 26.4 arm64 / Bun 1.3.14:
npm run typecheck— exit 0npm run lint— exit 0npm run format— cleannpm run build— exit 0bun scripts/start.ts --profile-load stepfun-37 "write me a haiku and nothing else"— exit 0bun test scripts/tests/bun-test-policy.bun.test.ts— 19 passpackages/auth— 43/43 ·packages/core— 359/359 ·packages/cli— 680/680 files, 8761 tests, 0 failedcore95.59s before → 95.91s afternpm run testacross every workspace exited 1, with two agents files timing out:createAgent.harness.behavior.test.tsandlspControl.behavior.test.ts. Both passtogether in 1.05s standalone. That run was on a machine at load average 30
with four other checkouts running their own suites — and
agentsconcurrency isunchanged by this PR, since it was already the tuned runner. Everything else,
including the
clisuite this PR changes most, passed.Windows is worth a look from someone who has one:
corekeeps itswin32 ? 1 : 2cap, and that path is unchanged, but I cannot exercise it.Known limitation
availableParallelism()reports the machine's cores, not this process's share ofthem. On a CI runner — which is dedicated — those are the same number, so the issue
this PR fixes is unaffected. On a development machine running several checkouts at
once they are not, and half the cores is still more than the runner actually has;
the two agents timeouts above are exactly that. Making the pool load-aware would be
a different change that buys CI nothing, so the
LLXPRT_*_TEST_CONCURRENCYoverridestays the escape hatch, and the module now says so.
On local repetition
I tried to demonstrate the reduced flake rate by running the agents suite repeatedly. The development machine is shared with other worktrees and sat at load average 20–60 on 16 cores throughout, which is the very condition under test. Numbers taken there would be noise dressed as evidence, so I am not presenting any. The measurements above were taken when the box was quiet, and CI is the honest authority for the flake rate.
Linked issues / bugs
Fixes #3139
Extends the tuning done for
agentsin #3084 to the runners it never reached.