Conversation
|
Status: reproduced and fixed; ready for review.
|
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 29 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 16 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped precedence fix.
What was reviewed:
- Traced the three
MAX_SIMULTANEOUS_REQUESTSstores againstasync_http::load_env(AsyncHTTP.rs:205-234) — invalid/zero env values still fall through to the pre-stored install default, and the.max(1)clamp on--network-concurrency 0is preserved. - Checked that moving
env.has_http_proxy()beforeoptions.load()is safe:options.loadonly reads from the already-populatedDotEnvLoaderand does not load additional env sources that could add proxy vars. - Test refactor:
ctx.requestedis the per-context counter incremented by the shared dummy registry (dummy.registry.ts:350),envisbunEnvaliased at import, and theit.eachrows cover flag-only, env-only, and flag-overrides-env.
Extended reasoning...
Overview
Three files: src/install/PackageManager.rs reorders when MAX_SIMULTANEOUS_REQUESTS is written during PackageManager::init so that BUN_CONFIG_MAX_HTTP_REQUESTS (parsed inside options.load → async_http::load_env) is no longer clobbered by the install default; test/cli/install/bun-install.test.ts converts the existing --network-concurrency=5 test into an it.each with two additional rows for the env var alone and for flag-overrides-env; and docs/runtime/environment-variables.mdx clarifies the install default (64) and precedence.
Security risks
None. This adjusts a process-wide concurrency cap for outbound HTTP during bun install. No auth, crypto, path handling, or untrusted-input parsing is touched. The env var parser itself (load_env) is unchanged and already bounds-checks to 1..=65535.
Level of scrutiny
Low-to-medium. The Rust change is a mechanical reordering of three .store(.., Ordering::Relaxed) calls on a static atomic during single-threaded init, before the HTTP thread starts. I verified: (1) load_env only stores when the env value parses to a nonzero u16, so an invalid/zero value now falls back to the install default that was pre-stored — matching the PR description's probe matrix; (2) cli.network_concurrency is Option<u16>, so usize::from(n.max(1)) is the same clamp as the removed n.max(1) as usize; (3) env.has_http_proxy() reads http_proxy/HTTP_PROXY/https_proxy/HTTPS_PROXY from the already-loaded DotEnvLoader, and options.load (PackageManagerOptions.rs:403-672) does not mutate those keys, so calling it before options.load returns the same result as before.
Other factors
The test refactor follows REVIEW.md harness conventions: await using proc, concurrent pipe drain via Promise.all, {...env, ...extraEnv} spread over bunEnv, and a combined-object toEqual assertion. The dropped > 20 throw inside the stub is safe to remove because a thrown handler would have returned a 500 and triggered install-side retries, inflating ctx.requested past 51 — the maxConcurrentRequests <= cap assertion already guards the invariant. The 10 ms Bun.sleep is simulated registry latency (needed for requests to overlap so the peak counter is meaningful), not a wait-for-condition; it's commented as such. The PR description shows the env-var row failing with Received: 51 on the unfixed build and passing with the fix, satisfying the fails-for-the-right-reason requirement.
00df52c to
3ebff88
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — reorders the three MAX_SIMULTANEOUS_REQUESTS stores so the env var isn't clobbered by the install default.
What was reviewed:
- Traced
options.load→async_http::load_env(AsyncHTTP.rs:205–234): only stores when the env var parses to a nonzerou16, so invalid/zero falls through to the install default set just before it. env.has_http_proxy()moved beforeoptions.load—envis passed by shared ref, so the proxy check can't differ between the old and new position;http_thread::initstill runs after all three stores.- Test refactor:
ctx.requestedis the harness's per-context counter (dummy.registry.ts:350), the droppedurlsassertion was dead, and the> 20throw would have caused 500 retries — themaxConcurrentRequests <= capassertion is the real guard.
Extended reasoning...
Overview
The PR fixes BUN_CONFIG_MAX_HTTP_REQUESTS being ignored by bun install. Three files: PackageManager.rs reorders atomic stores in init (default before options.load, CLI flag after, guarded by if let Some); bun-install.test.ts converts the existing --network-concurrency=5 test into an it.each with two new rows (env-only, flag-overrides-env); the docs table row for the env var now states the install default (64) and flag precedence.
Security risks
None. This is init-time ordering of atomic stores to a process-wide concurrency cap. The values themselves are unchanged (same clamp, same default constants), and load_env already validates the env var as a u16 in [1, 65535].
Level of scrutiny
Low-to-medium. The change is a mechanical reordering of three stores with a clear invariant (flag > env > default). I verified: load_env is reached via PackageManagerOptions::load at line 661 and only writes when the env value is valid; env is a &DotEnvLoader throughout so has_http_proxy() returns the same value at the earlier position; the HTTP thread starts at http_thread::init further down, so all three stores land before any request is scheduled. The usize::from(network_concurrency.max(1)) matches the old .max(1) as usize semantics (the type is u16 so usize::from is lossless).
Other factors
The test changes are a strict improvement over the original: the dead urls assertion and unused stdin: "pipe" are gone, pipes are drained concurrently with Promise.all, await using handles cleanup, and ctx.requested reuses the harness counter that other tests in this file already rely on. The Bun.sleep(10) in the stub is simulated registry latency (needed to make requests overlap so peak concurrency is observable) and is commented as such — not a wait-for-condition sleep. The comment-cop bot's feedback about the paragraph-long comment was addressed in 3a2a9c8. The one CI failure (test-http-chunk-problem.js) is unrelated and was picked up via rebase onto #38726.
|
Heads-up from #38759, which fixes the The two branches merge cleanly right now. #38759 puts its registry stub in a module-level helper, |
PackageManager::init stored the --network-concurrency value (or the 64 default) into MAX_SIMULTANEOUS_REQUESTS after options.load had already applied BUN_CONFIG_MAX_HTTP_REQUESTS, so the env var never had an effect on bun install. Store the install default before options.load and only the explicit --network-concurrency value after it, so the precedence is flag, then env var, then default.
3a2a9c8 to
3e547e8
Compare
There was a problem hiding this comment.
LGTM — focused fix that reorders the MAX_SIMULTANEOUS_REQUESTS stores so BUN_CONFIG_MAX_HTTP_REQUESTS (applied inside options.load → load_env) is no longer clobbered by the install default, with --network-concurrency still winning when passed.
What was reviewed:
- Verified
load_envinsrc/http/AsyncHTTP.rswritesMAX_SIMULTANEOUS_REQUESTSand is called fromPackageManagerOptions::load, so storing the default before it and the flag after it gives flag > env > default. - Checked that moving
env.has_http_proxy()to beforeoptions.loadis safe — it reads only the already-populatedDotEnvLoader, not anythingoptions.loadsets up. - Confirmed
cli.network_concurrencyisOption<u16>, so the.max(1)clamp andusize::fromconversion are unchanged from the oldas usizecast. - Test refactor: the three
it.eachrows spreadextraEnvafterbunEnv(which does not setBUN_CONFIG_MAX_HTTP_REQUESTS), and each row runs in its ownwithContextunderdescribe.concurrent, so no shared-counter races.
Extended reasoning...
Overview
The PR fixes bun install to honor BUN_CONFIG_MAX_HTTP_REQUESTS. Three files touched: src/install/PackageManager.rs (reorder two stores around options.load), test/cli/install/bun-install.test.ts (convert the existing --network-concurrency=5 test into an it.each with two new rows for the env var and for flag-overrides-env precedence), and a one-line clarification in docs/runtime/environment-variables.mdx.
Correctness
I traced the flow: PackageManagerOptions::load → bun_http::async_http::load_env (line 701 of PackageManagerOptions.rs), which parses BUN_CONFIG_MAX_HTTP_REQUESTS and stores into the MAX_SIMULTANEOUS_REQUESTS atomic (AsyncHTTP.rs:233). The old code then unconditionally overwrote that with the flag-or-64 default. The new code stores the default first, lets load_env overwrite it if the env var is set, then overwrites again only if --network-concurrency was explicitly passed (Option<u16>). Invalid/zero env values still fall back to the default because load_env returns early without storing in those cases. env.has_http_proxy() only reads the DotEnv map, which is populated before init runs, so calling it before options.load is equivalent. Nothing between the old and new store locations reads the atomic — the HTTP thread starts later in init.
Security risks
None. This adjusts an integer concurrency cap read from an env var the process already trusts; load_env already validates it as a u16 in [1, 65535].
Level of scrutiny
Low-to-medium. It's a small reordering of existing stores in single-threaded init code, with no new allocation, FFI, or JS interaction. The test change is a mechanical table-ification of an existing test plus two new rows; the removed > 20 throw in the stub was redundant with the toBeLessThanOrEqual assertion (and would have triggered retries on 500, muddying the count).
Other factors
The comment-cop feedback was addressed (the precedence comment is now a single line). The robobun note about #38759's expectInstallInFlightLimit helper is a forward-looking consolidation suggestion, not a blocker — the branches merge cleanly and this test's toBeLessThanOrEqual(cap) assertion still fails hard on the unfixed build (51 vs 5). CI passed on Linux and Windows; the pending darwin-arm64 shards are an agent-queue backlog unrelated to the change.
Hold every manifest request until the cap is reached, wait a bounded window for a client that ignores the cap to open more connections, then release. The peak must equal the cap, so the rows also catch a client that uses fewer slots than configured.
|
Took the exact-cap idea in 957ee08: the stub now holds every manifest request until Holding until exactly The stub stays inline in the |
There was a problem hiding this comment.
LGTM — small, well-scoped fix that reorders the MAX_SIMULTANEOUS_REQUESTS stores around options.load() so BUN_CONFIG_MAX_HTTP_REQUESTS is honored, with --network-concurrency still winning.
What was reviewed:
- Verified
options.load()→async_http::load_env()writes the env var intoMAX_SIMULTANEOUS_REQUESTSand falls through on invalid/zero input, so the pre-store install default (64 / proxy) survives — matches the probe matrix in the description. - Confirmed
env.has_http_proxy()is safe to call beforeoptions.load()(env is already loaded;BUN_MANIFEST_CACHEis read from it two lines earlier), and both stores land before the HTTP thread is started later ininit. - Test: the hold-until-released stub makes
peakInFlightan exact count (single-threaded handler, requests parked onawait released); theBun.sleep(50)aftercapReachedcarries the required justification comment; pipes drained concurrently;bunEnvdoesn't setBUN_CONFIG_MAX_HTTP_REQUESTSso the flag-only row is clean.
Extended reasoning...
Overview
Three files: src/install/PackageManager.rs splits the single unconditional MAX_SIMULTANEOUS_REQUESTS.store(...) (which ran after options.load() and always stomped the env-var write inside load_env) into (a) a default store before options.load() and (b) a conditional CLI-flag store after it. test/cli/install/bun-install.test.ts converts the existing --network-concurrency=5 test into an it.each with two new rows (env-var only; flag overriding env-var) and tightens the assertion from toBeLessThanOrEqual to toBe(cap). docs/runtime/environment-variables.mdx documents the install default of 64 and the flag override.
Security risks
None. This is a config-precedence fix for an HTTP concurrency knob. No auth, crypto, path handling, or untrusted-input parsing is touched. Lowering concurrency (the intended use) is strictly safer.
Level of scrutiny
Low-to-moderate. The Rust change is a pure reordering of atomic stores in single-threaded init code, preserving all existing values (.max(1) clamp on the flag, proxy default, 64 default). I traced the call chain: PackageManagerOptions::load at line ~701 calls bun_http::async_http::load_env, which parses BUN_CONFIG_MAX_HTTP_REQUESTS and stores it only when valid and non-zero — so the install default stored just before survives on invalid input, matching the PR's probe output. The HTTP thread is started later in init (after the CA setup that follows the removed block), so ordering relative to consumers is unchanged.
Other factors
- The comment-cop bot's "paragraph-long comment" flag was resolved in 3e547e8 — the comment is now a single line.
- The robobun note about #38759 (a shared
expectInstallInFlightLimithelper) is future-consolidation guidance, not a blocker; the two branches merge cleanly per that comment. - The rewritten test is strictly stronger than the original: exact
toBe(cap)instead of<= 5, generated 51-dep package.json instead of a hardcoded list, and the request-holding stub removes the timing dependency the originalBun.sleep(10)had. The remainingBun.sleep(50)sits after the cap is provably reached and only detects over-cap sends; the accompanying comment satisfies the review rule for justified sleeps at that threshold. - Verified
bunEnv(whichenvaliases) does not setBUN_CONFIG_MAX_HTTP_REQUESTS, so theextraEnv: {}row is not contaminated. - CI: Linux and Windows lanes green per the status comment; darwin arm64 shards are queued on infra, unrelated to this diff.
This branch has not been deployed
2 participants
Problem
BUN_CONFIG_MAX_HTTP_REQUESTShas no effect onbun install: with it set to 8, a loopback registry still sees 64 requests in flight. Only--network-concurrencyworks. Same on 1.3.14; the docs list the variable as applying tobun install.PackageManagerOptions::load(src/install/PackageManager/PackageManagerOptions.rs:661) callsasync_http::load_env, which stores the variable intoMAX_SIMULTANEOUS_REQUESTS.PackageManager::init(src/install/PackageManager.rs, formerly line 2276) then unconditionally stored--network-concurrencyor the 64 default over it.Retry-Afterhandling for 429s inbun install, this variable is the documented way to get past a rate limiting registry, so the documented mitigation was a no-op.Fix
PackageManager::initnow stores the install default (64, or the proxy default) beforeoptions.load, and afteroptions.loadstores only an explicitly passed--network-concurrency. Precedence is therefore flag, then env var, then default.if let Some(network_concurrency)guard on the post-options.loadstore; the values themselves are unchanged (--network-concurrency 0still clamps to 1, an invalid or zero env value still logs and falls back to the default, the same as underfetch).bun installdocs row forBUN_CONFIG_MAX_HTTP_REQUESTSnow states the install default and that--network-concurrencyoverrides the variable.test/cli/install/bun-install.test.ts, "bun install with ... keeps exactly N requests in flight". The existing--network-concurrency=5case became anit.eachrow (same input; its 51-dependency package.json is now generated). Two new rows:BUN_CONFIG_MAX_HTTP_REQUESTS=5, and--network-concurrency=2together withBUN_CONFIG_MAX_HTTP_REQUESTS=50, which pins the precedence. The registry stub holds every manifest request until the cap is reached, waits a bounded window for a client that ignores the cap to open more connections, then releases them all, so the peak must equal the cap.bun bd test test/cli/install/bun-install.test.ts -t "requests in flight": 3 pass with the fix; the env var row fails withReceived: 51on the unfixed debug build and on release 1.4.0.bun-install.test.tson the fixed build: the only failures are the 14 tests that need public internet (bitbucket/gitlab/vercel URLs) plusshould support --registry CLI flag, and all of them fail identically on the unfixed build in this environment.Background
MAX_SIMULTANEOUS_REQUESTS(src/http/AsyncHTTP.rs) is the process-wide cap on in-flight HTTP requests. The HTTP thread reads it on every drain, so whatever value is in it when the first request is scheduled is what applies;bun installstarts the HTTP thread later ininit, after both stores.async_http::load_envis the shared parser forBUN_CONFIG_MAX_HTTP_REQUESTS; the runtime (bun run, fetch) calls it directly on top of the static's 256 default, andbun installreaches it throughoptions.load. Layering the install default underneath it, instead of on top of it, is what makes the same function serve both.Probe of the report's matrix against the fixed debug build (80 leaf deps, loopback stub holding each request 30 ms)
Before the fix,
env=8andenv=4 flag=8read 64 and 8 respectively; the other rows are unchanged.no test proof · iteration 7 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-install.test.ts