diff --git a/src/install/PackageManager/runTasks.rs b/src/install/PackageManager/runTasks.rs index 318e0b6ef2e4..3feca04e87af 100644 --- a/src/install/PackageManager/runTasks.rs +++ b/src/install/PackageManager/runTasks.rs @@ -542,16 +542,7 @@ fn run_tasks_erased( ); } - if manager.subcommand != Subcommand::Remove { - for request in manager.update_requests.iter_mut() { - if strings::eql(request.name, name) { - request.failed = true; - manager.options.do_.remove(Do::SAVE_LOCKFILE); - manager.options.do_.remove(Do::SAVE_YARN_LOCK); - manager.options.do_.remove(Do::INSTALL_PACKAGES); - } - } - } + fail_update_requests(manager, task.task_id, name, None); } continue; @@ -599,16 +590,7 @@ fn run_tasks_erased( response.status_code, ); } - if manager.subcommand != Subcommand::Remove { - for request in manager.update_requests.iter_mut() { - if strings::eql(request.name, name) { - request.failed = true; - manager.options.do_.remove(Do::SAVE_LOCKFILE); - manager.options.do_.remove(Do::SAVE_YARN_LOCK); - manager.options.do_.remove(Do::INSTALL_PACKAGES); - } - } - } + fail_update_requests(manager, task.task_id, name, None); continue; } @@ -853,16 +835,12 @@ fn run_tasks_erased( .fmt(&manager.lockfile.buffers.string_bytes, PathSep::Auto,), ); } - if manager.subcommand != Subcommand::Remove { - for request in manager.update_requests.iter_mut() { - if strings::eql(request.name, extract.name.slice()) { - request.failed = true; - manager.options.do_.remove(Do::SAVE_LOCKFILE); - manager.options.do_.remove(Do::SAVE_YARN_LOCK); - manager.options.do_.remove(Do::INSTALL_PACKAGES); - } - } - } + fail_update_requests( + manager, + task.task_id, + extract.name.slice(), + Some(extract.dependency_id), + ); if let Some(removed) = manager.task_queue.remove(&task.task_id) { drop(removed); @@ -936,16 +914,12 @@ fn run_tasks_erased( response.status_code, ); } - if manager.subcommand != Subcommand::Remove { - for request in manager.update_requests.iter_mut() { - if strings::eql(request.name, extract.name.slice()) { - request.failed = true; - manager.options.do_.remove(Do::SAVE_LOCKFILE); - manager.options.do_.remove(Do::SAVE_YARN_LOCK); - manager.options.do_.remove(Do::INSTALL_PACKAGES); - } - } - } + fail_update_requests( + manager, + task.task_id, + extract.name.slice(), + Some(extract.dependency_id), + ); if let Some(removed) = manager.task_queue.remove(&task.task_id) { drop(removed); @@ -1946,6 +1920,66 @@ pub(crate) fn network_task_has_failed(this: &PackageManager, task_id: Task::Id) .is_some_and(|e| e.failed) } +/// `bun add` / `bun update ` exits 1 and saves nothing when a download for the request fails. +fn fail_update_requests( + this: &mut PackageManager, + task_id: Task::Id, + package_name: &[u8], + tarball_dependency_id: Option, +) { + if this.subcommand == Subcommand::Remove { + return; + } + let lockfile = &*this.lockfile; + let string_buf = lockfile.buffers.string_bytes.as_slice(); + let dependencies = lockfile.buffers.dependencies.as_slice(); + let resolutions = lockfile.buffers.resolutions.as_slice(); + let waiters = this.task_queue.get(&task_id).map_or(&[][..], Vec::as_slice); + // An npm tarball task has no waiters: the dependencies it was for already resolved to its package. + let package_id = tarball_dependency_id + .and_then(|id| resolutions.get(id as usize).copied()) + .filter(|&package_id| package_id != INVALID_PACKAGE_ID); + let was_for = |id: DependencyID| { + package_id.is_some_and(|package_id| resolutions.get(id as usize) == Some(&package_id)) + || waiters.iter().any(|waiter| { + matches!( + waiter, + bun_install::TaskCallbackContext::Dependency(waiting) + | bun_install::TaskCallbackContext::RootDependency(waiting) if *waiting == id + ) + }) + }; + let pending = this.pending_filtered_write.as_deref(); + + let mut any_failed = false; + for request in this.update_requests.iter_mut() { + // A request names a package.json key, which an `npm:` alias or an override spells differently from `package_name`. + let names_its_dependency = lockfile + .workspaces_of_update_request(pending, this.workspace_name_hash, request) + .into_iter() + .any(|workspace_id| { + let lists = lockfile.packages.items_dependencies(); + lists.get(workspace_id as usize).is_some_and(|list| { + (list.off..list.off + list.len).any(|id| { + dependencies + .get(id as usize) + .is_some_and(|dependency| request.matches(dependency, string_buf)) + && was_for(id) + }) + }) + }); + if names_its_dependency || strings::eql(request.name, package_name) { + request.failed = true; + any_failed = true; + } + } + if any_failed { + this.options + .do_ + .remove(Do::SAVE_LOCKFILE | Do::SAVE_YARN_LOCK | Do::INSTALL_PACKAGES); + } +} + /// The first failed download in a `run_tasks` pass halves the number of /// concurrent requests (down to the configured minimum). fn throttle_after_network_error(manager: &PackageManager, has_network_error: &mut bool) { diff --git a/src/install/lockfile.rs b/src/install/lockfile.rs index e753842367a5..4c46bc82500e 100644 --- a/src/install/lockfile.rs +++ b/src/install/lockfile.rs @@ -950,6 +950,19 @@ impl Lockfile { } } + /// The workspaces whose dependency lists `request` names: the ones that received it under `--filter` / `-r`, else the cwd's. + pub(crate) fn workspaces_of_update_request( + &self, + pending: Option<&crate::package_manager_real::add_remove_with_filter::PendingWrite>, + workspace_name_hash: Option, + request: &UpdateRequest, + ) -> Vec { + match pending { + Some(pending) => pending.workspace_ids_receiving(self, request.name_hash), + None => vec![self.get_workspace_package_id(workspace_name_hash)], + } + } + /// Re-runnable: package_json_write_back binds again after re-deriving the declared columns. #[cold] #[inline(never)] @@ -963,19 +976,12 @@ impl Lockfile { let string_buf = self.buffers.string_bytes.as_slice(); let string_buf_ptr = bun_ptr::RawSlice::new(string_buf); let slice = self.packages.slice(); - let cwd_workspace = [self.get_workspace_package_id(workspace_name_hash)]; 'request_updated: for update in updates.iter_mut() { update.e_string = None; - let filtered: Vec; - let workspace_ids: &[PackageID] = match pending { - Some(pending) => { - filtered = pending.workspace_ids_receiving(self, update.name_hash); - &filtered - } - None => &cwd_workspace, - }; - for &workspace_package_id in workspace_ids { + let workspace_ids = + self.workspaces_of_update_request(pending, workspace_name_hash, update); + for &workspace_package_id in &workspace_ids { let dep_list = slice.items_dependencies()[workspace_package_id as usize]; let res_list = slice.items_resolutions()[workspace_package_id as usize]; let workspace_deps: &[Dependency] = diff --git a/test/cli/install/bun-update-transitive.test.ts b/test/cli/install/bun-update-transitive.test.ts index 43d7ae599a80..7607ca55ef05 100644 --- a/test/cli/install/bun-update-transitive.test.ts +++ b/test/cli/install/bun-update-transitive.test.ts @@ -1,6 +1,6 @@ import { file, write } from "bun"; import { afterAll, beforeAll, expect, test } from "bun:test"; -import { exists } from "fs/promises"; +import { exists, rm } from "fs/promises"; import { VerdaccioRegistry, bunEnv, bunExe, tempDir } from "harness"; import { join } from "path"; @@ -1235,7 +1235,12 @@ type Manifests = Record>; // Serves one manifest per name from memory; verdaccio has no parent whose newer version keeps a range on the same child, and its dist-tags cannot move mid-test. `tags` is read per request, so a test can move a tag after installing. -type RegistryKnobs = { times?: Record>; status?: Record }; +// `status` is keyed by package name or by tarball file name ("leaf-1.1.0.tgz"); `tarballOrigin` replaces this server's origin in every `dist.tarball`. +type RegistryKnobs = { + times?: Record>; + status?: Record; + tarballOrigin?: string; +}; async function serveRegistry(manifests: Manifests, tags: Tags = {}, knobs: RegistryKnobs = {}) { const tarballs = new Map(); @@ -1252,16 +1257,17 @@ async function serveRegistry(manifests: Manifests, tags: Tags = {}, knobs: Regis port: 0, fetch(request) { const { origin, pathname } = new URL(request.url); - const tarball = tarballs.get(pathname); - if (tarball) return new Response(tarball); const name = pathname.slice(1); - const entry = manifests[name]; const status = knobs.status?.[name]; if (status) return new Response("registry says no", { status }); + const tarball = tarballs.get(pathname); + if (tarball) return new Response(tarball); + const entry = manifests[name]; if (!entry) return new Response("not found", { status: 404 }); + const tarballOrigin = knobs.tarballOrigin ?? origin; const versions: Json = {}; for (const [version, extra] of Object.entries(entry)) { - versions[version] = { name, version, dist: { tarball: `${origin}/${name}-${version}.tgz` }, ...extra }; + versions[version] = { name, version, dist: { tarball: `${tarballOrigin}/${name}-${version}.tgz` }, ...extra }; } const latest = Object.keys(entry).sort(Bun.semver.order).at(-1); const time = knobs.times?.[name]; @@ -1860,6 +1866,165 @@ test.concurrent( }, ); +// A named request whose download fails exits 1 and writes nothing, also when the failure is only a warning (an optional dependency). The request names the package.json key; the failed download reports the registry name, which an `npm:` alias spells differently. +const LEAF_ONLY: Manifests = { leaf: { "1.0.0": {}, "1.1.0": {} } }; +const warningLines = (stderr: string) => stderr.split("\n").filter(line => line.startsWith("warn:")); + +// A host that closes every connection before it answers. It keeps its port for the whole test; the port of a stopped server could go to another test's registry. +const hangUp = () => + Bun.listen({ hostname: "127.0.0.1", port: 0, socket: { open: socket => void socket.end(), data() {} } }); + +// Breaks the download of leaf's manifest or of leaf@1.1.0's tarball; `down` is the origin of a `hangUp()` host. Returns the one warning to expect and the flags the update needs. +type Outage = (server: Bun.Server, knobs: RegistryKnobs, down: string) => { warning: unknown; flags?: string[] }; +const OUTAGES: Record = { + "a 404 for the manifest": (server, knobs) => { + knobs.status!.leaf = 404; + return { warning: `warn: ${manifestFailure(server, 404)}` }; + }, + "a registry that hangs up": (_server, _knobs, down) => ({ + warning: expect.stringMatching(/^warn: \w+ downloading package manifest leaf$/), + flags: ["--registry", `${down}/`], + }), + "a 404 for the tarball": (server, knobs) => { + knobs.status!["leaf-1.1.0.tgz"] = 404; + return { warning: `warn: GET ${server.url.origin}/leaf-1.1.0.tgz - 404` }; + }, + "a tarball host that hangs up": (_server, knobs, down) => { + knobs.tarballOrigin = down; + return { warning: expect.stringMatching(/^warn: \w+ downloading tarball leaf@1\.1\.0$/) }; + }, +}; + +// [label, the package.json key that reaches leaf, the fields that declare it with `range` on leaf, the update's arguments] +const aliasedLeaf = (range: string) => ({ optionalDependencies: { aliased: `npm:leaf@${range}` } }); +const OPTIONAL_LEAF_ENTRIES: [string, string, (range: string) => Json, string[]][] = [ + ["a plain entry", "leaf", range => ({ optionalDependencies: { leaf: range } }), ["leaf"]], + ["an npm: alias named by its key", "aliased", aliasedLeaf, ["aliased"]], + ["an npm: alias named by its key with --latest", "aliased", aliasedLeaf, ["aliased", "--latest"]], + ["an npm: alias named by its target", "aliased", aliasedLeaf, ["leaf"]], + [ + "an entry that an override renames", + "renamed", + range => ({ optionalDependencies: { renamed: "*" }, overrides: { renamed: `npm:leaf@${range}` } }), + ["renamed"], + ], + [ + "a catalog: entry that is an npm: alias", + "cataloged", + range => ({ + workspaces: { catalog: { cataloged: `npm:leaf@${range}` } }, + optionalDependencies: { cataloged: "catalog:" }, + }), + ["cataloged"], + ], +]; + +test.concurrent.each( + Object.keys(OUTAGES).flatMap(outage => OPTIONAL_LEAF_ENTRIES.map(entry => [outage, ...entry] as const)), +)("%s fails `bun update ` for %s and writes nothing", async (outage, _, key, fields, args) => { + const knobs: RegistryKnobs = { status: {} }; + using server = await serveRegistry(LEAF_ONLY, {}, knobs); + using down = hangUp(); + const optional = (range: string) => ({ name: "foo", ...fields(range) }); + const dir = await setupServed(server, "update-failed-download-", optional("1.0.0"), optional("^1.0.0")); + expect(await installedVersion(dir, key)).toBe("1.0.0"); + const before = { packageJson: await packageJsonText(dir), lock: await lockText(dir) }; + + const { warning, flags = [] } = OUTAGES[outage](server, knobs, `http://127.0.0.1:${down.port}`); + const { stderr, exitCode } = await run(dir, "update", ...args, ...flags); + expect(warningLines(stderr)).toStrictEqual([warning]); + expect(errorLines(stderr)).toStrictEqual([]); + expect(stderr).not.toContain("Saved lockfile"); + expect(await packageJsonText(dir)).toBe(before.packageJson); + expect(await lockText(dir)).toBe(before.lock); + expect(await installedVersion(dir, key)).toBe("1.0.0"); + expect(exitCode).toBe(1); +}); + +// An npm tarball is downloaded once for every entry that resolves to it, on behalf of the first one. +test.concurrent("a 404 for the tarball fails the request for a second alias of the same package", async () => { + const knobs: RegistryKnobs = { status: { "leaf-1.1.0.tgz": 404 } }; + using server = await serveRegistry(LEAF_ONLY, {}, knobs); + const packageJson = stringify({ + name: "foo", + optionalDependencies: { first: "npm:leaf@^1.0.0", second: "npm:leaf@^1.0.0" }, + }); + const dir = String(tempDir("update-failed-shared-tarball-", { "package.json": packageJson })); + await servedBunfig(server, dir); + + const { stderr, exitCode } = await run(dir, "update", "second"); + expect(warningLines(stderr)).toStrictEqual([`warn: GET ${server.url.origin}/leaf-1.1.0.tgz - 404`]); + expect(errorLines(stderr)).toStrictEqual([]); + expect(await packageJsonText(dir)).toBe(packageJson); + expect(await exists(join(dir, "bun.lock"))).toBe(false); + expect(exitCode).toBe(1); +}); + +// The failed download is the only message: a warning for an optional dependency, an error otherwise. A request that is not failed goes on to a second error, " failed to resolve". +test.concurrent.each([ + ["warn", ["--optional", "leaf"]], + ["warn", ["--optional", "aliased@npm:leaf@^1.0.0"]], + ["error", ["leaf"]], + ["error", ["aliased@npm:leaf@^1.0.0"]], +])("a 404 for the manifest is the one %s of `bun add %p`, which writes nothing", async (level, args) => { + const knobs: RegistryKnobs = { status: { leaf: 404 } }; + using server = await serveRegistry(LEAF_ONLY, {}, knobs); + const packageJson = stringify({ name: "foo" }); + const dir = String(tempDir("add-failed-download-", { "package.json": packageJson })); + await servedBunfig(server, dir); + + const { stderr, exitCode } = await run(dir, "add", ...args); + expect([...warningLines(stderr), ...errorLines(stderr)]).toStrictEqual([`${level}: ${manifestFailure(server, 404)}`]); + expect(await packageJsonText(dir)).toBe(packageJson); + expect(await exists(join(dir, "bun.lock"))).toBe(false); + expect(exitCode).toBe(1); +}); + +// A version that bun.lock already holds gets no download while it resolves. The install phase downloads it, after the package.json entry was rewritten in memory. +test.concurrent.each([ + ["a plain entry", "leaf", ""], + ["an npm: alias", "aliased", "npm:leaf@"], +])("a 404 for a tarball that the install phase downloads fails `bun update ` for %s", async (_, key, target) => { + const knobs: RegistryKnobs = { status: {} }; + using server = await serveRegistry(LEAF_ONLY, {}, knobs); + const packageJson = { name: "foo", optionalDependencies: { [key]: `${target}^1.0.0` } }; + const dir = await installServed(server, "update-failed-install-download-", packageJson); + expect(await installedVersion(dir, key)).toBe("1.1.0"); + await Promise.all(["node_modules", ".bun-cache"].map(name => rm(join(dir, name), { recursive: true, force: true }))); + const before = { packageJson: await packageJsonText(dir), lock: await lockText(dir) }; + + knobs.status!["leaf-1.1.0.tgz"] = 404; + const { stderr, exitCode } = await run(dir, "update", key); + expect(warningLines(stderr)).toStrictEqual([`warn: GET ${server.url.origin}/leaf-1.1.0.tgz - 404`]); + expect(errorLines(stderr)).toStrictEqual([]); + expect(await packageJsonText(dir)).toBe(before.packageJson); + expect(await lockText(dir)).toBe(before.lock); + expect(exitCode).toBe(1); +}); + +// pkg2's optional `leaf` is an alias of a package the registry does not have. It has the key of the request, but the request is for pkg1's entry. The alias range must not admit pkg1's range, or bun resolves pkg1's `leaf` through the alias too. +test.concurrent("a failed download for the same key in another workspace does not fail the request", async () => { + using server = await serveRegistry(LEAF_ONLY); + const pkg2 = { name: "pkg2", version: "1.0.0", optionalDependencies: { leaf: "npm:missing@^9.0.0" } }; + const dir = String( + tempDir("update-failed-download-elsewhere-", { + "package.json": stringify(ROOT), + "packages/pkg1/package.json": stringify(member("pkg1", { leaf: "^1.0.0" })), + "packages/pkg2/package.json": stringify(pkg2), + }), + ); + await servedBunfig(server, dir); + + const { stderr, exitCode } = await runIn(dir, "packages/pkg1", "update", "leaf"); + expect(warningLines(stderr)).toStrictEqual([`warn: GET ${server.url.origin}/missing - 404`]); + expect(errorLines(stderr)).toStrictEqual([]); + expect(stderr).toContain("Saved lockfile"); + expect(await packageJsonOf(dir, "packages/pkg1")).toStrictEqual(member("pkg1", { leaf: "^1.1.0" })); + expect(await packageJsonOf(dir, "packages/pkg2")).toStrictEqual(pkg2); + expect(await installedVersion(dir, "leaf")).toBe("1.1.0"); + expect(exitCode).toBe(0); +}); + test.concurrent("`bun update ` from a member leaves a sibling's own entry alone but lets it follow", async () => { const { dir, pkg2Text } = await staleMembers("~1.0.0", "^1.0.0"); const { stderr, exitCode } = await runIn(dir, "packages/pkg1", "update", "no-deps"); diff --git a/test/regression/issue/15276.test.ts b/test/regression/issue/15276.test.ts index fcae8b61d250..0fff469fc878 100644 --- a/test/regression/issue/15276.test.ts +++ b/test/regression/issue/15276.test.ts @@ -1,17 +1,21 @@ import { expect, test } from "bun:test"; import { bunEnv, bunExe } from "harness"; -test("parsing npm aliases without package manager does not crash", () => { +test("parsing npm aliases without package manager does not crash", async () => { // Easiest way to repro this regression with `bunx bunbunbunbunbun@npm:another-bun@1.0.0`. The package // doesn't need to exist, we just need `bunx` to parse the package version. - const { stdout, stderr, exitCode } = Bun.spawnSync({ + using registry = Bun.serve({ port: 0, fetch: () => new Response("{}", { status: 404 }) }); + await using proc = Bun.spawn({ cmd: [bunExe(), "x", "bunbunbunbunbun@npm:another-bun@1.0.0"], stdout: "pipe", stderr: "pipe", - env: bunEnv, + env: { ...bunEnv, BUN_CONFIG_REGISTRY: registry.url.href }, }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // The 404 fails the request, so it is the only error, as for `bunx another-bun@1.0.0`. + expect(stderr).toContain(`error: GET ${registry.url.href}another-bun - 404`); + expect(stderr).not.toContain("failed to resolve"); + expect(stdout).toBe(""); expect(exitCode).toBe(1); - expect(stderr.toString()).toContain("error: bunbunbunbunbun@npm:another-bun@1.0.0 failed to resolve"); - expect(stdout.toString()).toBe(""); });