install: fail bun outdated and bun update -i when a dependency's manifest cannot be fetched - #38809
Conversation
…manifest cannot be fetched Both commands fetch the direct dependencies' manifests through populate_manifest_cache, which logs every fetch failure into the package manager's log, and then never print that log or look at it. With the registry down (5xx, connection refused, DNS) or a package missing from it, `bun outdated` printed nothing and exited 0 and `bun update -i` reported nothing to update. populate_manifest_cache also passed the dependency's optional flag where has_created_network_task takes is_required, so a required dependency's failure was logged as a warning and an optional one's as an error. The severity is now chosen per Packages variant: Ids (the two commands above) uses the dependency's is_required(), while All (the post-migration backfill) and Exact (transitive update, audit fix) are best-effort passes and log warnings, which is what their callers already assumed. After fetching, the two commands print the logged failures and exit 1 when any of them is an error; `bun outdated` still prints the table for the manifests it did get first.
|
Warning Review limit reached
Next review available in: 5 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 (6)
Comment |
|
Updated 11:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit 74fcff1 has some failures in 🧪 To try this PR locally: bunx bun-pr 38809That installs a local version of the PR into your bun-38809 --bun |
|
Status: ready for review. The diff is green on every lane; the two CI runs (builds 97191 and 97215) are red only on tests this PR does not touch, each of which passed on retry or when re-run alone: Reproduced on main without a Verdaccio: a project with a Same result with the port closed ( The new tests in |
…igration suite yarn-lock-migration.test.ts has a case that migrates a lockfile with ~900 packages against the live registry and sits at the default timeout under debug builds, so the new cases move to the pnpm v9 suite, which migrates through the same manifest pass and runs against local registries only.
…failed to get The manifest pass that runs while a yarn or pnpm lockfile is migrated leaves its entries in the network dedupe map, so when `bun outdated` or `bun update -i` then fetched the same manifest, has_created_network_task short-circuited and nothing was logged for it: the failure stayed the warning the migration had already flushed and the command still exited 0. Clear the map once that pass has completed; it runs during lockfile loading, before the command enqueues anything of its own.
There was a problem hiding this comment.
The dedupe-map gap I flagged earlier is now closed by network_dedupe_map.clear() in fetch_necessary_package_metadata_after_yarn_or_pnpm_migration, with a test in pnpm-lock-v9.test.ts covering bun outdated / bun update -i run straight off a pnpm lockfile. I re-reviewed and found no further issues; leaving final sign-off to a human since this reshapes error-vs-warning semantics across several install-path callers.
Checked: Packages::All → always-warn matches what report_migrated and warn_unchecked already expect; the .clear() sits between the migration's manifest pass and the command's own, and successfully-fetched manifests stay in manager.manifests so bun install won't re-fetch them. bunEnv already carries BUN_INTERNAL_INTERACTIVE_ASSUME_TTY=1, so the pnpm suite's run helper reaches the manifest fetch for update -i.
Extended reasoning...
Overview
Makes bun outdated and bun update -i surface manifest-fetch failures and exit 1 (instead of silently reporting nothing to update). Touches PopulateManifestCache.rs (fixes an inverted is_optional→is_required pass-through, replaces the &Dependency param with an explicit is_required: bool, adds print_fetch_failures), outdated_command.rs / update_interactive_command.rs (call the new helper and Global::crash() on error), and lockfile.rs (clears network_dedupe_map after the post-migration Packages::All pass so a later Packages::Ids pass can re-fetch what failed). Tests added in bun-update-transitive.test.ts (502/404/refused/optional matrix for outdated, 502 for update -i) and pnpm-lock-v9.test.ts (post-migration manifest severity for install; outdated/update -i off a pnpm lockfile with a 404 registry).
Security risks
None identified. Log messages come from the existing run_tasks failure formatting; no new user-controlled data is echoed. Tests use local Bun.serve({port:0}) registries.
Level of scrutiny
Medium-high. This is a user-facing behavior change to package-manager error handling that fans out to four call sites (outdated, update -i, transitive update/audit fix via Exact, and post-migration install via All). The network_dedupe_map.clear() in the migration helper affects every caller of load_from_cwd that migrates — I traced bun install and confirmed successful manifests are served from manager.manifests (so no double-fetch) and tarball tasks use distinct task IDs, and the new install-after-migration test asserts toStrictEqual on exactly two fetch lines which would catch a duplicate. Still, the severity-per-variant policy and the dedupe clear are the kind of cross-cutting decision a maintainer should confirm.
Other factors
My previous review's one finding (the migration→Ids dedupe short-circuit) was addressed in 2cee2c1 exactly as suggested (option 1: clear the map between passes) and covered by a new test.concurrent.each over outdated and update -i. The comment-cop bot's four flags were resolved in 0ebd716. I verified dep.behavior.is_required() is the same predicate the install enqueue path uses (PackageManagerEnqueue.rs:1018 etc.), and that bunEnv in harness.ts:84 sets BUN_INTERNAL_INTERACTIVE_ASSUME_TTY=1 so the pnpm suite's plain run helper bypasses the early TTY refusal in update_interactive_command.rs:261. CI for 2cee2c1 was still building at the time of review.
There was a problem hiding this comment.
LGTM. The dedupe-map fix from the earlier review round is in and covered by the two run straight off the pnpm lockfile cases; nothing new to flag.
What was reviewed:
- Confirmed
has_created_network_task's third param isis_required(runTasks.rs:1730), so the oldis_optionalpass-through was inverted;dep.behavior.is_required()matches the install-path idiom at PackageManagerEnqueue.rs:1018/1283/etc. - Checked
network_dedupe_map.clear()runs afterpopulate_manifest_cache(All)has drained (sleep_untilwaits forpending_task_count == 0), so no in-flight task loses its bookkeeping; successfully-fetched manifests stay inmanager.manifestsso a following install won't redundantly refetch. - Verified
report_migratedonly flushes a warnings-only log, soPackages::Allmoving to always-warning keeps the migration's log-print path unchanged for required deps and stops optional-dep manifest 404s from failing a migrating install (covered by the new pnpm test).
Extended reasoning...
Overview
The PR makes bun outdated and bun update -i print manifest-fetch failures and exit 1 when a required direct dependency's manifest cannot be fetched, instead of silently reporting nothing outdated. It fixes an inverted is_optional/is_required argument in start_manifest_task, adds print_fetch_failures to surface the accumulated log, and clears network_dedupe_map after the post-migration manifest backfill so a following command's own fetch is not deduplicated away. Six files touched: three Rust source files in the install/CLI layer, two test files with ~10 new tests.
Security risks
None. This is error-reporting/exit-code plumbing in the package manager CLI. No parsing of untrusted input, no auth/crypto, no new network-facing surface.
Level of scrutiny
Medium. The package manager is user-facing and correctness-sensitive, but the change is narrow: it reads an already-populated log, threads a boolean through a helper whose signature I confirmed against the callee, and clears a map at a point where all its tasks have completed. The behavior change for Packages::All (optional-dep manifest failure during migration downgraded from error to warning) is intentional, well-argued in the description, and consistent with a non-migrating install.
Other factors
I reviewed this PR earlier and flagged the migration dedupe hole; the author confirmed the trace, fixed it in 2cee2c1 with network_dedupe_map.clear(), and added the two run straight off the pnpm lockfile tests that fail without that line. The comment-cop feedback (paragraph-length doc comments) was addressed in 0ebd716. Tests cover the variant matrix (502/404/connection-refused, optional vs required, -r, both commands, both migration paths), each asserts the healthy state first, and the description records that the existing suites still pass. The refactor of outdated_dispatch to unify the two branches into (ids, was_filtered) is behavior-preserving — same Packages::Ids call and same was_filtered value fed to the table printer.
#39445) Stacked on #36463 (the base branch is that PR's branch, so the diff here is only the additions). Merging this into #36463 adds the behavior changes listed below; #36463 itself now covers the #38333 install batch, the optional-peer correction, and the TOML / `bun init` fixes, so this PR no longer touches those. ### Problem - These 1.3 to 1.4 behavior changes are not in the guide at `701b3e2a0`: - MySQL: the first `caching_sha2_password` connection over plain TCP is refused unless `allowPublicKeyRetrieval: true` (#31129; 1.3.14 requested the key automatically, `MySQLConnection.zig` in the 1.3.14 tag). SQL `tls` / `ssl` options now require TLS instead of falling back to plaintext, and `?ssl=` / `?ssl-mode=` are read (`shared.ts` 1.3.14 only read `?sslmode=`; #37669). - Install: `~/.npmrc` fallback when `XDG_CONFIG_HOME` is set (#36289), credentials in `--registry` / env / bunfig object URLs are sent and outrank same-host `.npmrc` tokens (#38796, #38824), `bun outdated` exits 1 on fetch failures (#38809), new `dedupe` / `up` commands shadow scripts of those names and `bun feedback` is removed (#38333, #38444), `workspace:` ranges inside registry packages (#37669), isolated store entry names (#39014). - Runtime: `module.enableCompileCache()` / `NODE_COMPILE_CACHE` implemented (#34660), `require()` / `import` not-found messages (#34660), `AbortError` message without the period (#39277; 1.3.14's `BunCommonStrings.h` has the period), GCM IV length (#34092), `mkdtemp("")` (#34908), vm options (#38381), `server.reload` (#38697), ICU 75/73 to 78 (#38013), Compression stream chunking (#38695), `Bun.SQL` sqlite bindings (#35950). - Bundler: `splitting` with `cjs` / `iife` is an error (#32685), block-scoped `enum` lowers to `let` (#34249), exports emitted ascending instead of descending (#35957; `doStep5.zig` in 1.3.14 used `sortDesc`), minified `$` (#35668). - The TOML integer bullet did not say what the limit or the fix is. ### Fix - Adds a MySQL public key section (plus a summary table row), a TLS note under the `PGSSLMODE` section, an `.npmrc` / credentials addendum to the `bunfig.toml` section, a `module.enableCompileCache()` section, and the rest as bullets in the existing lists. - `docs/pm/overrides.mdx`: one-line change adding a pointer to this guide in the existing `lockfileVersion` 3 limitation. (The base branch briefly had a duplicate "Nested overrides" section; it removed that itself in `8257d01acb`, and this PR was rebased over it.) - Verification: each runtime claim was run against `1.4.0-canary.1+8326d1bd3` (22 commits behind main; contains every change referenced), and each install or bundler claim was checked against the source on main, with the 1.3 side taken from the `bun-v1.3.14` tag where the PR body did not state it. The `/runtime/sql#mysql` and `/upgrade-to-1.4` links resolve. `prettier --check` passes. ### Not included on purpose - Lifecycle scripts no longer receiving `npm_package_name` / `npm_package_version` / `npm_package_json` / `npm_config_local_prefix` during `bun install`, and transitive `"*"` ranges no longer deduplicating onto the root's version: regressions with open fixes (#36690, #38110, #38770). They need either the fixes or a guide line before release. - Postgres `sslmode=prefer` / `allow` (including `PGSSLMODE=prefer`, which 1.4 newly reads) hangs until the connection timeout against a server without SSL because nothing sends the startup message after the `N` reply. Same code in 1.3.14; filed as a bug instead of documented. <details> <summary>Commands used to verify the runtime claims</summary> ``` timers/promises setTimeout with an aborted signal # "The operation was aborted" bun req.cjs # Cannot find module ... Require stack: bun b.mjs (import() of a missing package / relative file) # Cannot find package 'x' imported from /path, ERR_MODULE_NOT_FOUND bun a_static.mjs (unhandled static import) # printed line still: Cannot find package 'x' from '/path' process.versions.icu # 78.3 createCipheriv("aes-128-gcm", key, Buffer.alloc(129)) # ERR_CRYPTO_INVALID_IV DecompressionStream of a 1 MiB gzip member # 16 chunks of 65536 bytes new SQL("sqlite://:memory:") with ${[1,2]} / ${new Date()} # Binding expected ... fs.mkdtempSync("") # EINVAL vm.runInThisContext("1", []) # ERR_INVALID_ARG_TYPE NODE_COMPILE_CACHE=/tmp/cc bun cc.cjs # creates /tmp/cc/v1.4.0-x86_64-<sha>-<uid> NODE_DISABLE_COMPILE_CACHE=1 + enableCompileCache() # status 3 (DISABLED) bun dedupe / bun up with package.json scripts of those names # built-in command runs bun feedback # Script not found "feedback" Bun.build({ splitting: true, format: "cjs" }) # Code splitting is currently only supported ... bun build of a function-scoped enum and import * as ns # let Color; exports a, m, z new SQL({ url: "postgres://...", tls: true }) on a non-TLS server # ERR_POSTGRES_TLS_NOT_AVAILABLE Bun.TOML.parse("a = 9007199254740993") # Integer cannot be losslessly represented ... ``` </details> <details> <summary>Previous revision</summary> The first revision of this PR (`3c5611454a`) also rewrote the package manager section for #38333 / #38853 (nested overrides and `lockfileVersion: 3`, the optional-peer correction, `bun update`, `bunfig.toml` over `.npmrc`, `--filter`) and fixed the TOML date and `bun init` lines. #36463 picked those up in its own commits the same day, so this PR was rebased onto its new head and reduced to the items above. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · docs-only change; test-proof not applicable <!-- robobun:evidence:end -->
#39445) Stacked on #36463 (the base branch is that PR's branch, so the diff here is only the additions). Merging this into #36463 adds the behavior changes listed below; #36463 itself now covers the #38333 install batch, the optional-peer correction, and the TOML / `bun init` fixes, so this PR no longer touches those. ### Problem - These 1.3 to 1.4 behavior changes are not in the guide at `701b3e2a0`: - MySQL: the first `caching_sha2_password` connection over plain TCP is refused unless `allowPublicKeyRetrieval: true` (#31129; 1.3.14 requested the key automatically, `MySQLConnection.zig` in the 1.3.14 tag). SQL `tls` / `ssl` options now require TLS instead of falling back to plaintext, and `?ssl=` / `?ssl-mode=` are read (`shared.ts` 1.3.14 only read `?sslmode=`; #37669). - Install: `~/.npmrc` fallback when `XDG_CONFIG_HOME` is set (#36289), credentials in `--registry` / env / bunfig object URLs are sent and outrank same-host `.npmrc` tokens (#38796, #38824), `bun outdated` exits 1 on fetch failures (#38809), new `dedupe` / `up` commands shadow scripts of those names and `bun feedback` is removed (#38333, #38444), `workspace:` ranges inside registry packages (#37669), isolated store entry names (#39014). - Runtime: `module.enableCompileCache()` / `NODE_COMPILE_CACHE` implemented (#34660), `require()` / `import` not-found messages (#34660), `AbortError` message without the period (#39277; 1.3.14's `BunCommonStrings.h` has the period), GCM IV length (#34092), `mkdtemp("")` (#34908), vm options (#38381), `server.reload` (#38697), ICU 75/73 to 78 (#38013), Compression stream chunking (#38695), `Bun.SQL` sqlite bindings (#35950). - Bundler: `splitting` with `cjs` / `iife` is an error (#32685), block-scoped `enum` lowers to `let` (#34249), exports emitted ascending instead of descending (#35957; `doStep5.zig` in 1.3.14 used `sortDesc`), minified `$` (#35668). - The TOML integer bullet did not say what the limit or the fix is. ### Fix - Adds a MySQL public key section (plus a summary table row), a TLS note under the `PGSSLMODE` section, an `.npmrc` / credentials addendum to the `bunfig.toml` section, a `module.enableCompileCache()` section, and the rest as bullets in the existing lists. - `docs/pm/overrides.mdx`: one-line change adding a pointer to this guide in the existing `lockfileVersion` 3 limitation. (The base branch briefly had a duplicate "Nested overrides" section; it removed that itself in `8257d01acb`, and this PR was rebased over it.) - Verification: each runtime claim was run against `1.4.0-canary.1+8326d1bd3` (22 commits behind main; contains every change referenced), and each install or bundler claim was checked against the source on main, with the 1.3 side taken from the `bun-v1.3.14` tag where the PR body did not state it. The `/runtime/sql#mysql` and `/upgrade-to-1.4` links resolve. `prettier --check` passes. ### Not included on purpose - Lifecycle scripts no longer receiving `npm_package_name` / `npm_package_version` / `npm_package_json` / `npm_config_local_prefix` during `bun install`, and transitive `"*"` ranges no longer deduplicating onto the root's version: regressions with open fixes (#36690, #38110, #38770). They need either the fixes or a guide line before release. - Postgres `sslmode=prefer` / `allow` (including `PGSSLMODE=prefer`, which 1.4 newly reads) hangs until the connection timeout against a server without SSL because nothing sends the startup message after the `N` reply. Same code in 1.3.14; filed as a bug instead of documented. <details> <summary>Commands used to verify the runtime claims</summary> ``` timers/promises setTimeout with an aborted signal # "The operation was aborted" bun req.cjs # Cannot find module ... Require stack: bun b.mjs (import() of a missing package / relative file) # Cannot find package 'x' imported from /path, ERR_MODULE_NOT_FOUND bun a_static.mjs (unhandled static import) # printed line still: Cannot find package 'x' from '/path' process.versions.icu # 78.3 createCipheriv("aes-128-gcm", key, Buffer.alloc(129)) # ERR_CRYPTO_INVALID_IV DecompressionStream of a 1 MiB gzip member # 16 chunks of 65536 bytes new SQL("sqlite://:memory:") with ${[1,2]} / ${new Date()} # Binding expected ... fs.mkdtempSync("") # EINVAL vm.runInThisContext("1", []) # ERR_INVALID_ARG_TYPE NODE_COMPILE_CACHE=/tmp/cc bun cc.cjs # creates /tmp/cc/v1.4.0-x86_64-<sha>-<uid> NODE_DISABLE_COMPILE_CACHE=1 + enableCompileCache() # status 3 (DISABLED) bun dedupe / bun up with package.json scripts of those names # built-in command runs bun feedback # Script not found "feedback" Bun.build({ splitting: true, format: "cjs" }) # Code splitting is currently only supported ... bun build of a function-scoped enum and import * as ns # let Color; exports a, m, z new SQL({ url: "postgres://...", tls: true }) on a non-TLS server # ERR_POSTGRES_TLS_NOT_AVAILABLE Bun.TOML.parse("a = 9007199254740993") # Integer cannot be losslessly represented ... ``` </details> <details> <summary>Previous revision</summary> The first revision of this PR (`3c5611454a`) also rewrote the package manager section for #38333 / #38853 (nested overrides and `lockfileVersion: 3`, the optional-peer correction, `bun update`, `bunfig.toml` over `.npmrc`, `--filter`) and fixed the TOML date and `bun init` lines. #36463 picked those up in its own commits the same day, so this PR was rebased onto its new head and reduced to the items above. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · docs-only change; test-proof not applicable <!-- robobun:evidence:end -->
Problem
bun outdatedprints only its header and exits 0, indistinguishable from "everything is current".bun update -iin the same state printsChecked 1 dependency, nothing to updateand exits 0.bun updateandnpm outdatedboth exit 1 here.populate_manifest_cache(src/install/PackageManager/PopulateManifestCache.rs), andrun_tasksrecords each failed fetch in the package manager's log (GET <url> - 502,ConnectionRefused downloading package manifest <name>,runTasks.rs:416-512). Neitheroutdated_command.rsnorupdate_interactive_command.rsprinted that log or looked at it after the fetch; a dependency without a manifest was then skipped by the table / picker as if it were up to date.start_manifest_taskpassed the dependency's optional flag tohas_created_network_task, whose parameter isis_required(PopulateManifestCache.rs:61, inherited from the Zig version). So a required dependency's failure was logged as a warning and an optional dependency's as an error. Exiting on logged errors would not have fired for normal dependencies without fixing this.Fix
start_manifest_tasktakesis_requiredand passes it straight through (and!is_requiredtofor_manifest), the same predicate the install path uses (PackageManagerEnqueue.rs:1016,:1163).Packagesvariant, documented on the variants:Ids(onlybun outdatedandbun update -iuse it):dep.behavior.is_required(), so a required dependency's failure is an error and an optional one's a warning.Exact(transitivebun update,bun audit fix) andAll(the bin/os/cpu backfill after a yarn/pnpm migration): always warnings. Their callers are best-effort and already depended on getting warnings:update_transitive::warn_uncheckedfilters onKind::Warn,migration::report_migratedonly flushes a warnings-only log.Exactalready got warnings before (its placeholderDependencyhad no optional bit); the only change forAllis that an optional dependency's missing manifest no longer fails a migratingbun install, which matches a non-migrating install.populate_manifest_cache::print_fetch_failuresprints the log and returnshas_errors().bun outdatedcalls it after printing the table (so what could be checked is still listed),bun update -iright after the fetch (before offering an incomplete list); bothGlobal::crash()on an error, as the lockfile-load failures in the same files do. Under--silentthe log level already drops the warnings, so only the error lines print.bun updatefails on it too. Package-name arguments do not narrow the fetch, as before, so a failing dependency outside the pattern still fails the command.Allpass leaves its task ids innetwork_dedupe_map, so the command's own pass would skip the same manifest and log nothing (review finding).fetch_necessary_package_metadata_after_yarn_or_pnpm_migrationnow clears the map once its pass has completed; it runs during lockfile loading, before the command enqueues anything, and a manifest it did get is served from memory anyway.test/cli/install/bun-update-transitive.test.ts: 502 / 404 /-r/ registry stopped forbun outdated(error line, header only, exit 1), optional dependency (warning, exit 0), 502 forbun update -i(error line, exit 1, lockfile untouched); each runs the healthy command first. All 6 fail on main (exit 0, no line printed;update -iprintsChecked 1 dependency, nothing to update), and the existing transitive-manifest-failure tests next to them still pass.test/cli/install/migration/pnpm-lock-v9.test.ts:bun installmigrating a pnpm-lock.yaml against a local registry that 404s everything now warns about the manifest for an optional dependency too (main logserror:for it and exits 1); the required-dependency case is unchanged (manifest warning, tarball error, exit 1) and is included as the contrast.bun outdatedandbun update -irun straight off the pnpm lockfile print the migration's warning and then their own error line and exit 1; without the dedupe-map clear they print the warning alone and exit 0. (Both migrations share the manifest pass; the pnpm suite is the one that runs against local registries only.)bun-install-registry.test.ts(outdatedblock),bun-update.test.ts,bun-audit.test.ts,yarn-lock-migration.test.ts,update_interactive_install.test.tspass with the debug build;cargo clippyonbun_installandbun_runtimeis clean.Background
populate_manifest_cacheis the manifest-only fetch used by commands that read manifests without installing. It enqueues one network task per package and runs the task loop until they complete;run_tasksin that mode has no callback, so a failed fetch ends up as a line inmanager.log(abun_ast::Log, the same error/warning accumulatorbun installprints at the end) and nothing else.is_required);run_tasksreads that flag only to decide whether the failure line is an error or a warning. That flag is what was inverted.Global::crash()isexit(1); it flushes output, andprint_fetch_failuresflushes stdout itself before writing to stderr so the table lands before the error lines.