fix(install): return non-zero exit code when tarballs fail to download - #11828
Merged
Merged
Conversation
Contributor
|
❌ @dylan-conway, your commit has failing tests :( 💪 1 failing tests Darwin AARCH64
💻 1 failing tests Darwin x64 baseline
💻 1 failing tests Darwin x64
🐧💪 1 failing tests Linux AARCH64
🪟💻 3 failing tests Windows x64 baseline
🪟💻 2 failing tests Windows x64
|
3 tasks done
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 18, 2026
#43168) ### Problem - `bun update my-alias` erases an optional `npm:` alias when its download fails. Example: `"my-alias": "npm:nope@^1.0.0"` and a registry that is down. bun prints `warn: ConnectionRefused downloading package manifest nope`, saves the lockfile, exits 0 and writes `"my-alias": ""`. - The plain form (`"nope": "^1.0.0"`, `bun update nope`) prints the same warning, exits 1 and writes nothing. - The four download-failure arms of `run_tasks_erased` (`runTasks.rs:545`, `:602`, `:856`, `:939`) fail an update request with `strings::eql(request.name, name)`. `name` is the registry name (`nope`). `request.name` is the package.json key (`my-alias`). ### Fix - `fail_update_requests` replaces the four copies and keeps the name comparison. It also fails a request when the download was for a dependency that `bind_update_requests` would bind it to. - That is a waiter of the task. For an npm tarball (no waiters) it is a dependency that resolved to the package. `Lockfile::workspaces_of_update_request` says which dependency lists a request names, for both functions. - Verified: `test/cli/install/bun-update-transitive.test.ts` (32 new cases, 20 fail without the fix). Also the `bun add`, `bun update`, `bunx`, catalog and workspace suites. - Self-reviewed: split on its advice. The package.json write-back changes follow in a second PR. ### Background - An update request is one positional of `bun add` or `bun update`. It names a package.json key. - A request with `failed` set makes `install_with_manager` return `InstallFailed` before it writes package.json or bun.lock. - A failed download of an optional dependency is a warning, so the install continues. - `task_queue[task_id]` lists the dependencies that wait for a download task. <details><summary>Notes</summary> Found by a read of the code. There is no user report. The name comparison dates from #4046 and #11828, which introduced the exit 1 contract for a named request. Confirmed on canary 1.4.3-canary.1+c6b7fcb5b. Reproduction (no network needed): ```sh echo '{"name":"x","optionalDependencies":{"my-alias":"npm:nope@^1.0.0"}}' > package.json BUN_CONFIG_REGISTRY=http://127.0.0.1:9/ bun update my-alias; echo "exit $?"; cat package.json ``` Every spelling that makes the package.json key differ from the registry name has the bug. The tests cover each one with four kinds of failure (manifest 404, registry that closes the connection, tarball 404, tarball host that closes the connection), one per arm: | Entry | Request | Before | After | | --- | --- | --- | --- | | `"aliased": "npm:leaf@^1.0.0"` | `bun update aliased` (also with `--latest`) | exit 0, `Saved lockfile`, entry `""` | exit 1, nothing written | | `"renamed": "*"` with `"overrides": {"renamed": "npm:leaf@^1.0.0"}` | `bun update renamed` | exit 0, `Saved lockfile` | exit 1, nothing written | | `"cataloged": "catalog:"` with a catalog entry `npm:leaf@^1.0.0` | `bun update cataloged` | exit 0, `Saved lockfile` | exit 1, nothing written | | two aliases of one package, no lockfile, tarball 404 | `bun update second` | exit 0, `second` rewritten | exit 1, nothing written | | alias whose locked version is not in the cache, tarball 404 in the install phase | `bun update aliased` | exit 0, both files written | exit 1, nothing written | | none | `bun add --optional aliased@npm:leaf@^1.0.0`, manifest 404 | exit 0, unresolved entry and bun.lock written | exit 1, nothing written | | none | `bun add aliased@npm:leaf@^1.0.0`, manifest 404 | exit 1, two errors (the 404, then `aliased@npm:leaf@^1.0.0 failed to resolve`) | exit 1, the 404 only | | `"leaf": "^1.0.0"` | `bun update leaf` | exit 1 | unchanged | | `"aliased": "npm:leaf@^1.0.0"` | `bun update leaf` | exit 1 | unchanged | - The last two rows are the reason the name comparison stays. A request by registry name does not match the waiting dependency `aliased`, and it failed correctly before. - The "two aliases" row is the reason for the match on the resolution. The tarball task belongs to `first`. `second` resolved to the same package without a task of its own. - `test/regression/issue/15276.test.ts` (`bunx bunbunbunbunbun@npm:another-bun@1.0.0`) expected the second error line of the `bun add` row above. A failed request stops the install before that line (`// prevent redundant errors` in `install_with_manager`), which is what `bunx another-bun@1.0.0` already prints. The test now expects the 404 line alone, and it gets the 404 from a local server and no longer from registry.npmjs.org. - The match is limited to the dependency lists that `bind_update_requests` uses: the cwd workspace, or the workspaces that `--filter` / `-r` select. Without the limit, an optional `"leaf": "npm:missing@^9.0.0"` in another workspace fails `bun update leaf` in pkg1, whose own `leaf` resolved. One test pins this. The alias range in that test does not admit pkg1's range on purpose: when it does, bun resolves pkg1's plain `leaf` through the alias as well (`known_npm_aliases`), and the request fails on main too. - A failed request in the install phase writes nothing with the hoisted linker. The helper clears `Do::INSTALL_PACKAGES`, and `install_hoisted_packages` returns `InstallFailed` when that flag is clear (`hoisted_install.rs:463`, `:481`, `:584`), before `package_json_write_back::flush`. The "install phase" test covers it. - I removed each clause of the helper in a local build (waiters match, resolution match, name comparison, the workspace limit). Each removal fails at least one of the new tests. - The connection failures in the tests come from a TCP listener that closes each connection. A stopped server would free its port for another concurrent test. - The git clone and checkout arms of the same function are not touched here. #43075 and #43076 change them. #43076 replaces the same four blocks with a helper that compares names only, so the PR that lands second needs a rebase. Its `forget_failed_git_task` can call this helper with `(task_id, name, None)` before it removes the `task_queue` entry. - Not changed: the name comparison also fails a request when another workspace fails to download a different version of the same registry name. main does the same. A precise rule for a request by registry name needs the update scope of the request. - Not fixed here, different code paths: - With the isolated linker, a tarball that fails in the install phase goes to `on_package_download_error_store` and never reaches the request loop. bun exits 1 but saves bun.lock and package.json. Plain names and aliases behave the same, on main too. - `bun update <name>` rewrites the entry of an optional or peer dependency that no version satisfies (`""`, or the range of another workspace under `-r`). A second PR follows. - A bare `bun update --latest` with an optional dependency whose manifest returns 404 leaves the temporary `latest` literal in package.json. </details>
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
2 participants
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.
What does this PR do?
Optional dependencies will log warnings instead of errors.
fixes #11812
closes #11745
How did you verify your code works?
added tests for optional and non-optional dependencies with missing tarballs in the registry