Repository navigation
Conversation
…ation The test spawns 5 concurrent bun install processes, each with a default network concurrency of 48, pushing up to ~240 simultaneous CONNECT tunnels through a single squid proxy to registry.npmjs.org. One of the five occasionally stalls past the 20s per-spawn timeout and gets SIGTERM'd (exit 143), showing up as a ~30% first-attempt failure rate across PR builds since 2026-07-11. The flake is not a code regression: identical merge bases produce both clean and red runs on the same day, so the trigger is environmental (squid or npm), not bun. Run the five iterations sequentially so each install has the proxy to itself, drop the per-spawn timeout, and rely on the 120s test timeout to catch a genuine proxy hang (the #19771 regression this test exists for). Pipe stdout/stderr so a real failure is diagnosable.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Updated 2:49 PM PT - Jul 14th, 2026
❌ @robobun, your commit 158e3f1 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34175That installs a local version of the PR into your bun-34175 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only de-flake, no production code touched.
What was reviewed:
- Sequential loop + 120s test-level timeout still catches the #19771 proxy-hang regression; parallelism in the original was redundancy, not part of the repro.
- Subprocess handling matches repo conventions:
await using proc, concurrentPromise.alldrain of stdout/stderr/exited, output logged beforeexpect(exitCode). - try/finally ensures temp dirs are cleaned up on failure;
expectimport added.
Extended reasoning...
Overview
Single-file change to test/cli/install/bun-install-proxy.test.ts. Converts five parallel bun install spawns into a sequential loop, drops the 20s per-spawn timeout (which was producing exit 143 / SIGTERM under transient npm slowness), raises the test-level timeout from 60s to 120s, switches to await using proc with piped stdout/stderr drained concurrently, and wraps each iteration in try/finally so the temp dir is removed even on assertion failure. Adds the missing expect import.
Security risks
None. Test-only change; no runtime, install, or proxy code paths are modified. Environment variables and spawn arguments are unchanged.
Level of scrutiny
Low. This is a CI flake fix confined to a single test file. The PR description gives a clear mechanical explanation (5 × 48 concurrent CONNECT tunnels through one squid instance → occasional starvation past 20s) and demonstrates the failure correlates with wall-clock time rather than any merged commit. The key question — does serializing the iterations weaken regression coverage for #19771? — is addressed: the original test ran 5 iterations for redundancy ("run multiple times to make sure it's not flaky"), not because concurrency was required to reproduce the hang. A genuine proxy hang will still trip the 120s test-level timeout.
Other factors
The rewrite aligns with the repo's documented test patterns in CLAUDE.md: await using for subprocess cleanup, Promise.all([stdout.text(), stderr.text(), exited]) to avoid pipe-buffer deadlock, and printing stdout/stderr before asserting the exit code so failures are diagnosable. Cleanup moved into try/finally is strictly safer than the previous .finally() chain. No existing assertions were weakened. No outstanding reviewer comments.
|
CI on build 72994: Remaining red is unrelated to this test-only diff:
Ready for review. |
|
Heads-up: #39243 changes the top of |
|
This is Claude (an AI assistant) writing on Jarred's behalf. Included in #44968. |
What
test/cli/install/bun-install-proxy.test.tshas been going red on first attempt in roughly 30% of PR builds since 2026-07-11 (and briefly on 2026-07-06), always with the same shape:Four of the five concurrent
bun installprocesses finish in ~1s; one stalls at "Resolving dependencies" until the 20sBun.spawntimeoutSIGTERMs it (143 = 128 + SIGTERM). Seen in build 72962 and many others (72954, 72935, 72927, 72916, 72912, 72910, 72906, 72903, 72900, ...).Not a code regression
I checked merge bases for every failed build on 2026-07-11: builds based on the same main commit (
55ba6e453c,ab84aa22d2,9657f37896) are both clean and red within the same hour. The flake correlates with wall-clock time, not with what was merged. Before 2026-07-06 the rate was 0/60 across several sampled days.Cause
The test spawns five
bun installprocesses in parallel, each with the default--network-concurrencyof 48. All traffic goes through a single squid container toregistry.npmjs.org, so up to ~240 CONNECT tunnels compete for the one proxy. Under current npm/network conditions one of the five occasionally gets starved past 20s.Fix
Run the five iterations sequentially instead of concurrently, drop the 20s per-spawn timeout, and rely on the 120s test-level timeout for hang detection. A genuine proxy hang (what #19771 fixed and this test guards) still fails the test; transient npm slowness no longer does. Also pipe stdout/stderr and print them on non-zero exit so a real failure is diagnosable.
The test still hits the live npm registry; making it hermetic is a larger change that #33115 is already moving toward.
[stamp-90s] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file