install: say --network-concurrency defaults to 64 in help and docs - #38759
install: say --network-concurrency defaults to 64 in help and docs#38759robobun wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 8 minutes 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 (11)
Comment |
|
Updated 3:59 AM PT - Aug 15th, 2026
✅ @robobun, your commit 0c276fc75e9c26052131d5aa079a553bef30c9b6 passed in 🧪 To try this PR locally: bunx bun-pr 38759That installs a local version of the PR into your bun-38759 --bun |
|
Status: ready for review. Reproduced on an unmodified checkout: Current shape after review: the help string is fixed and pinned to the constant with a compile-time assert (changing the constant again fails the build until the text is updated), the docs snippets and CI for 0c276fc (build 97411): 177 of 179 jobs passed, no failing tests; the only annotations are retry-passing flakes on unrelated files, and the remaining two jobs are darwin 14 aarch64 test shards that have been waiting for an agent for several hours. The install test file itself passed on every lane that ran it, including x64 ASAN. Nothing left to do on this side; this is ready to merge once those shards report. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward help-text and docs correction (48 → 64) matching the actual constant, with a regression test that ties the printed default to observed concurrency. The inline nit about bounding the first await is worth taking but doesn't block.
Extended reasoning...
Overview
The only src/ change is a one-word help-string edit in CommandLineArguments.rs ((default 48) → (default 64)). The rest is eight docs/snippets/cli/*.mdx files and eleven copies in completions/bun-cli.json receiving the identical mechanical 48→64 replacement, plus a new test in bun-install.test.ts that parses the default out of --help and verifies against a registry stub that a plain bun install puts exactly that many requests in flight.
I confirmed DEFAULT_MAX_SIMULTANEOUS_REQUESTS_FOR_BUN_INSTALL = 64 at src/install/PackageManager.rs:289 and that no default 48 strings remain anywhere in the tree.
Security risks
None. Pure documentation/help-text; no runtime behavior changes. The test uses a local Bun.serve stub via the existing withContext/setContextHandler harness — no external network.
Level of scrutiny
Low. The help string is documentation of a constant; the constant is verifiably 64. The docs and completions edits are mechanical mirrors of the same fact. The new test is additive and follows file conventions (describe.concurrent, withContext, bunEnv spread with BUN_CONFIG_MAX_HTTP_REQUESTS: undefined, pipes drained via Promise.all, await using for cleanup).
Other factors
One nit was filed inline: if the real limit ever drops below the help-text value, the first await documentedLimitReached.promise deadlocks until the file's 5-minute default timeout instead of failing fast on expect(maxInFlight).toBe(documented). That's a diagnostics-quality concern for a hypothetical future regression, not a correctness or flakiness issue for the code as landed (help and constant are both 64). Worth addressing but not merge-blocking.
…ails with a diagnostic
There was a problem hiding this comment.
LGTM — the follow-up commit bounds the documentedLimitReached wait with proc.exited and a 30s sleep, so the actual-lower-than-documented case now fails with the maxInFlight diagnostic instead of the 5-minute file timeout. That was the only outstanding concern.
What was reviewed:
- Confirmed
DEFAULT_MAX_SIMULTANEOUS_REQUESTS_FOR_BUN_INSTALL(and the proxy variant) are both 64 insrc/install/PackageManager.rs, so the help string, eight docs snippets, and eleven completion entries are all consistent with the real default. - Grepped for remaining
default 48/"48"references — none related to this flag. - The new test uses the existing
withContextregistry stub, drains stdout/stderr/exited concurrently, unsetsBUN_CONFIG_MAX_HTTP_REQUESTS, and both bounded waits carry a comment naming why no direct signal exists.
Extended reasoning...
Overview
This PR corrects the documented default for --network-concurrency from 48 to 64 across one help-string literal in src/install/PackageManager/CommandLineArguments.rs, eight docs/snippets/cli/*.mdx flag references, and eleven entries in completions/bun-cli.json. It adds a test to test/cli/install/bun-install.test.ts that parses the number out of bun install --help and verifies against a local registry stub that a plain bun install reaches exactly that many concurrent requests and no more.
Prior review addressed
My earlier inline comment flagged that await documentedLimitReached.promise was unbounded, so an actual-limit-lower-than-documented mismatch would deadlock until the file's 5-minute default timeout with no diagnostic. Commit b7a77ff changes this to Promise.race([documentedLimitReached.promise, proc.exited, Bun.sleep(30_000)]) with an explanatory comment. In the passing case the promise resolves in ~1s so the 30s is never spent; in the failing case the assertion expect(maxInFlight).toBe(documented) runs after ~30s and reports the actual value. That fully addresses the concern.
Security risks
None. The only src/ change is a static help-string literal; everything else is docs, completions JSON, and a test. No auth, crypto, permissions, or user-input handling is touched.
Level of scrutiny
Low. This is a mechanical documentation correction where the true constant (64) is verified by reading PackageManager.rs:289-290 and by the new test which observes the runtime limit end-to-end. There is zero behavioral change to the package manager. The test is added alongside the existing --network-concurrency=<invalid> tests and follows the file's withContext(defaultOpts, ...) pattern.
Other factors
- Ruled out stragglers: grepped the repo for
default 48and"48"— the only hits are unrelated fixtures/docs, so nothing was missed. - The test's two bounded sleeps (30s for the reached-limit wait, 500ms for the not-exceeded window) each carry a comment naming why no observable signal exists, satisfying the REVIEW.md rule for literal sleeps ≥ 50ms. In the green path neither is fully waited on —
documentedLimitReachedresolves as soon as 64 requests arrive, and the 500ms window is the negative-observation bound. BUN_CONFIG_MAX_HTTP_REQUESTSis explicitly cleared from the child env so the harness's ambient env cannot skew the observed default.- No CODEOWNERS-sensitive paths, no design decisions, no API surface changes.
There was a problem hiding this comment.
LGTM — the earlier feedback on bounding the first await is addressed in b7a77ff.
What was reviewed:
- Confirmed
DEFAULT_MAX_SIMULTANEOUS_REQUESTS_FOR_BUN_INSTALLis 64 insrc/install/PackageManager.rs:289-290; the onesrc/change is the help string literal. - All eight docs snippets and eleven
completions/bun-cli.jsonentries updated; greppeddocs/for remainingdefault 48— none. - Test now races the reached-limit wait against
proc.exitedand a 30s bound, resolves on>=, drains pipes concurrently, and assertsmaxInFlight === documentedso both mismatch directions fail with a diagnostic. - The one CI failure (
test/regression/issue/09041.test.tsASAN) is unrelated — this PR touches no executed native code.
Extended reasoning...
Overview
The PR corrects a stale documented default: --network-concurrency help text and docs said 48, but DEFAULT_MAX_SIMULTANEOUS_REQUESTS_FOR_BUN_INSTALL in src/install/PackageManager.rs:289 is 64 (raised in 2de2e9f). The change is one help-string literal in CommandLineArguments.rs, eight docs/snippets/cli/*.mdx snippets, eleven descriptions in completions/bun-cli.json, and a new test in bun-install.test.ts that ties the printed default to the observed in-flight request cap.
Security risks
None. No executed code paths change — the sole src/ hunk is a display string in a clap::param! help literal. Docs and completions JSON are non-executable.
Level of scrutiny
Low for the fix itself (mechanical 48→64 across documentation surfaces, verified against the source constant). Medium for the new test, which was already reviewed once: the previous run flagged an unbounded await that would hang instead of failing diagnostically when actual < documented; the author addressed it in b7a77ff with Promise.race([documentedLimitReached.promise, proc.exited, Bun.sleep(30_000)]) and a >= resolver, and verified both failure directions produce the intended assertion message. The test follows harness conventions (withContext, setContextHandler, bunExe/env, await using, concurrent pipe drain, {...env, KEY: undefined} to clear BUN_CONFIG_MAX_HTTP_REQUESTS).
Other factors
- Grepped
docs/for any remainingdefault 48/"48"on this flag — none left. - The 500ms
Bun.sleepis a bounded observation window for a negative condition ("no request beyond the limit shows up"), which REVIEW.md permits; the passing path resolves via the promise long before then. - The single CI failure is an ASAN error in
test/regression/issue/09041.test.ts, which this PR does not touch and cannot affect (no native code change). - The prior inline review thread is marked resolved.
…share the in-flight test stub
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
… message covers it
There was a problem hiding this comment.
LGTM — mechanical 48→64 sync across help text, docs snippets, and completions, pinned to the source constant with a compile-time assert.
What was reviewed:
- Confirmed
DEFAULT_MAX_SIMULTANEOUS_REQUESTS_FOR_BUN_INSTALLand the proxy variant are both 64 insrc/install/PackageManager.rs:289-290, and that private consts are visible to the child module viasuper::. - Grepped for remaining
48references tied to network-concurrency across the repo — none. - The earlier unbounded-await issue is resolved:
limitReachednow racesproc.exitedand a 30s bound, and resolves on>=so the over-limit case unblocks it too. - The remaining inline note about the adjacent
"abcdef"loop is pre-existing and non-blocking.
Extended reasoning...
Overview
This PR corrects the documented default for --network-concurrency from 48 to 64 across three surfaces: the CLI help string in src/install/PackageManager/CommandLineArguments.rs, eight docs/snippets/cli/*.mdx flag-reference blocks, and eleven copies in completions/bun-cli.json. The actual runtime constant (DEFAULT_MAX_SIMULTANEOUS_REQUESTS_FOR_BUN_INSTALL in PackageManager.rs:289) has been 64 since it was raised in 2de2e9f; only the disconnected string literals were stale. The Rust change extracts the param to a named const (matching the existing BACKEND_PARAM pattern immediately above) and adds a const _: () = assert!(...) that pins the literal to both install-default constants at compile time. A new test in bun-install.test.ts parses (default N) from bun install --help and verifies a plain bun install against a request-holding registry stub reaches exactly N in-flight requests.
Security risks
None. This is a documentation string change plus a compile-time assertion and a test. No runtime behavior, parsing, validation, or network-facing code path changes. The test uses a local Bun.serve stub via the existing withContext/setContextHandler harness — no external network contact.
Level of scrutiny
Low. The docs and completions edits are pure string substitutions verified by grep (no remaining 48 references for this flag anywhere in the tree). The Rust change is nine lines that follow the neighboring BACKEND_PARAM idiom exactly and introduce no runtime code — only a const item and a const-eval assertion. I confirmed the private consts in the parent module are visible to the child module via super:: (Rust private items are accessible from descendant modules), and the assertion compares usize == 64 which const-evaluates cleanly. The test is the most substantive piece; it was iterated once in review to bound the wait so the actual-below-documented failure mode surfaces as an assertion instead of a file-timeout hang, and the author verified both failure directions (Expected: 48, Received: 49 on the unfixed help; Expected: 65, Received: 64 when forcing the over-documented case).
Other factors
All prior review feedback is resolved: the unbounded await documentedLimitReached was replaced with a Promise.race against proc.exited and a 30s bound in b7a77ff, and the paragraph-length justification comment above NETWORK_CONCURRENCY_PARAM was removed in 5d9af37 per the comment-cop check (the assert message carries the same information). The one inline note I left on this revision — the hardcoded "abcdef" in the adjacent invalid-value loop — is explicitly pre-existing, unchanged by this PR, and marked non-blocking. The test clears BUN_CONFIG_MAX_HTTP_REQUESTS from the environment to avoid leakage, drains stdout/stderr/exited concurrently, and the expectInstallInFlightLimit helper is factored out for reuse by #38744 (which the description says merges cleanly). CI passed the install test file on the x64 ASAN lane; the only file failures reported are pre-existing internet-dependent tests unrelated to this diff.
There was a problem hiding this comment.
LGTM — the earlier review feedback (bounded wait on limitReached, and the "abcdef" → input fix in the invalid-value loop) is in, and the comment-cop note is addressed.
What was reviewed:
- The help literal now matches
DEFAULT_MAX_SIMULTANEOUS_REQUESTS_FOR_BUN_INSTALL{,_FOR_PROXIES} = 64inPackageManager.rs, and theconst _: () = assert!(...)pins it — child modules can see parent-private consts, sosuper::…resolves. expectInstallInFlightLimit:ctx.requestedis bumped by the shared server before the custom handler runs, so thelimit + 1assertion holds afterrelease.resolve()lets the last request through;BUN_CONFIG_MAX_HTTP_REQUESTSis masked out of the env.- Docs/completions edits are the mechanical 48→64 across all eight snippets and eleven completion entries; no other flag descriptions were touched.
Extended reasoning...
Overview
This PR corrects the --network-concurrency default shown in bun install --help (and the eight sibling package-manager commands that share SHARED_TAIL_PARAMS) from 48 to 64, matching the actual constant DEFAULT_MAX_SIMULTANEOUS_REQUESTS_FOR_BUN_INSTALL = 64 in src/install/PackageManager.rs:289-290. It updates the eight docs/snippets/cli/*.mdx flag references and the eleven copies in completions/bun-cli.json, extracts the param to a named const NETWORK_CONCURRENCY_PARAM (mirroring the existing BACKEND_PARAM pattern), and adds a compile-time const _: () = assert!(...) so a future change to either constant fails the build until the help text is updated. A new test in bun-install.test.ts reads the default from --help and verifies a plain bun install against a stalling registry stub reaches exactly that many in-flight requests. It also carries a one-token drive-by fix to the adjacent invalid-value loop (previously all three iterations passed "abcdef").
Security risks
None. This is a help-text/documentation correction with no runtime behavior change; the constant itself is untouched. The test spawns bun against a local in-process Bun.serve registry via the existing dummy.registry harness — no external network.
Level of scrutiny
Low-to-moderate. The docs and completions edits are mechanical string replacements. The Rust change is nine lines: a hoisted const param plus a compile-time assert, following the file's existing BACKEND_PARAM idiom, and a one-line replacement in the SHARED_TAIL_PARAMS array. I verified the referenced constants exist in the parent module and are 64, and that Rust visibility rules allow super:: access from a child module to a parent's private consts. The test additions are the only part warranting real scrutiny, and they went through two rounds of feedback already.
Other factors
All three prior review comments have been addressed in follow-up commits: (1) the unbounded await limitReached.promise is now raced with proc.exited and a 30s bound so a real limit below the documented one fails on the maxInFlight assertion instead of the file timeout (b7a77ff); (2) the paragraph-long comment above NETWORK_CONCURRENCY_PARAM was removed since the assert message says the same thing (5d9af37); (3) the pre-existing hardcoded "abcdef" in the invalid-value loop now uses input and the assertion includes : ${input} (0c276fc). The author verified both failure directions of the new test manually and confirmed the file passes on the ASAN CI lane. The helper is structured for reuse by #38744 and the author confirmed the branches merge cleanly. Imports (setContextHandler, TestContext, spawn, env) are all already present in the file.
1 participant
Problem
bun install --help(and add, remove, update, outdated, patch, publish, link, info, which share the same flag table) prints--network-concurrency <NUM> Maximum number of concurrent network requests (default 48). The eightdocs/snippets/cli/*.mdxflag references andcompletions/bun-cli.jsonsay 48 as well.bun installactually uses 64:DEFAULT_MAX_SIMULTANEOUS_REQUESTS_FOR_BUN_INSTALLand its proxy variant are both 64 insrc/install/PackageManager.rs:289-290, and that is whatinitstores intoMAX_SIMULTANEOUS_REQUESTSwhen the flag is not passed (PackageManager.rs:2276-2290).clap::param!only takes literals, see the comment aboveBACKEND_PARAM), so nothing tied the two together and the text was never updated.Fix
src/install/PackageManager/CommandLineArguments.rs: the help string says(default 64). The param is now a named const with aconst _: () = assert!(...)under it that checks both install defaults are still 64, so the next change to the constant fails to compile until the help text is updated. Verified by setting the constant to 48: the build stops withevaluation panicked: update the default in the --network-concurrency help text (and docs/snippets/cli/*.mdx)at that assert.docs/snippets/cli/{install,add,remove,update,outdated,patch,publish,link}.mdx:default="64", and the four descriptions that repeated the number now say 64 too.completions/bun-cli.json: the eleven copies of this flag's description updated to the text the generator would now emit. Other flags in this file are stale as well; that is left for a regeneration, as with the targeted edits in install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333.bun installreally runs with and that--helpprints it.test/cli/install/bun-install.test.ts, "bun install --help states the --network-concurrency default that bun install actually uses". It reads(default N)out ofbun install --helpand hands N toexpectInstallInFlightLimit, which runs a plainbun installwith N+1 dependencies against a registry stub that holds every manifest request open and asserts exactly N requests end up in flight (the extra one has to wait for a slot). The stub lives in a helper so install: honor BUN_CONFIG_MAX_HTTP_REQUESTS #38744, which addsBUN_CONFIG_MAX_HTTP_REQUESTSrows to the neighbouring--network-concurrency=5test, can use the same mechanism; the two branches currently merge without conflicts.Expected: 48, Received: 49, because bun puts all 49 requests in flight.--network-concurrencytests under the debug build, about 1s each).maxInFlightassertion (checked by forcing N=65 against the 64 build:Expected: 65, Received: 64). The 30s race bound exists only for that failure path; the 500ms window is how long an over-limit request gets to show up.bun bd test: the only failures are the pre-existing bitbucket/gitlab/external-URL tests, which need internet access and fail identically on an unmodified checkout here. On CI the file passed on the x64 ASAN lane.(default 64)with the debug build.--network-concurrency=<invalid>loop spawnedabcdefon every iteration, so its65537and-1cases tested nothing. It now passes each value and expects it echoed in the error; all three values are rejected by the current parser, so the three tests still pass.Background
--network-concurrencycaps how many HTTP requestsbun installkeeps in flight at once. The cap is a process-wide atomic in the HTTP client (MAX_SIMULTANEOUS_REQUESTS); the HTTP thread starts queued requests until that many are active and parks the rest (src/http/HTTPThread.rsaround line 890), which is why a registry stub that never answers observes exactly the cap.SHARED_TAIL_PARAMSand concatenated into every package-manager command's flag table, so one help string covers all of the commands listed above.const _: () = assert!(...)is a compile-time assertion: the expression is evaluated while compiling and a false result is a build error. The crate already uses it to pin literals to the constants they mirror (for examplesrc/install/npm.rspins the manifest cache header length).docs/snippets/cli/*.mdxare the flag-reference blocks included by thedocs/pm/cli/*.mdxpages;completions/bun-cli.jsonis a checked-in dump of the--helpoutput thatmisctools/generate-cli-completions.tsproduces.no test proof · iteration 0 · 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