diff --git a/src/install/PackageManager/PackageManagerEnqueue.rs b/src/install/PackageManager/PackageManagerEnqueue.rs index f0c91da56b53..bbc936f18868 100644 --- a/src/install/PackageManager/PackageManagerEnqueue.rs +++ b/src/install/PackageManager/PackageManagerEnqueue.rs @@ -619,6 +619,32 @@ pub unsafe fn enqueue_patch_task_pre(this: &mut PackageManager, task: *mut Patch let _ = this.pending_pre_calc_hashes.fetch_add(1, Ordering::Relaxed); } +/// Mirrors the npm arm's `should_update` gate in +/// `get_or_put_resolved_package_with_find_result`: a `bun update` target must +/// not bind to the in-memory lockfile package on its first enqueue, or the +/// update never re-fetches. +fn is_update_target(this: &mut PackageManager, dependency: &Dependency, id: DependencyID) -> bool { + if !this.to_update { + return false; + } + let this_ptr: *mut PackageManager = this; + // Update direct deps of the current workspace; catalogs are root-scoped. + // SAFETY: `is_root_dependency` reads `manager.root_package_id` / + // `manager.workspace_name_hash` only — disjoint from `manager.lockfile`. + if dependency.version.tag != dependency::version::Tag::Catalog + && !unsafe { &*(*this_ptr).lockfile }.is_root_dependency(unsafe { &mut *this_ptr }, id) + { + return false; + } + // By name_hash, not `updating_packages`: that map only records + // npm/dist-tag deps. Matches `Diff::generate`'s update-target test. + this.update_requests.is_empty() + || this + .update_requests + .iter() + .any(|r| r.name_hash == dependency.name_hash) +} + /// Q: "What do we do with a dependency in a package.json?" /// A: "We enqueue it!" pub fn enqueue_dependency_with_main_and_success_fn( @@ -641,7 +667,14 @@ pub fn enqueue_dependency_with_main_and_success_fn( return Ok(()); } + // Git/github/tarball `realname()` is only learned from the first extract, + // so on a fresh parse fall back to the alias; `package_index` is keyed by + // real names, and hashing "" would re-download lockfile-resolved packages. let mut name = dependency.realname(); + let is_provisional_name = name.is_empty(); + if is_provisional_name { + name = dependency.name; + } let mut name_hash = match dependency.version.tag { dependency::version::Tag::DistTag | dependency::version::Tag::Git @@ -1176,10 +1209,15 @@ pub fn enqueue_dependency_with_main_and_success_fn( let dep: Repository = *version.git(); let res = Resolution::init(ResolutionTagged::Git(dep)); - // First: see if we already loaded the git package in-memory - if let Some(pkg_id) = this.lockfile.get_package_id(name_hash, None, &res) { - success_fn(this, id, pkg_id); - return Ok(()); + // First: see if we already loaded the git package in-memory. + // On the fresh (alias-named) enqueue, `bun update` targets skip + // the bind so the update re-fetches; the post-checkout re-enqueue + // binds as before. + if !(is_provisional_name && is_update_target(this, dependency, id)) { + if let Some(pkg_id) = this.lockfile.get_package_id(name_hash, None, &res) { + success_fn(this, id, pkg_id); + return Ok(()); + } } // reshaped for borrowck — `alias`/`url` borrow @@ -1286,10 +1324,13 @@ pub fn enqueue_dependency_with_main_and_success_fn( let dep: &Repository = version.github(); let res = Resolution::init(ResolutionTagged::Github(*dep)); - // First: see if we already loaded the github package in-memory - if let Some(pkg_id) = this.lockfile.get_package_id(name_hash, None, &res) { - success_fn(this, id, pkg_id); - return Ok(()); + // First: see if we already loaded the github package in-memory. + // See the git arm for the update-target gate. + if !(is_provisional_name && is_update_target(this, dependency, id)) { + if let Some(pkg_id) = this.lockfile.get_package_id(name_hash, None, &res) { + success_fn(this, id, pkg_id); + return Ok(()); + } } let url = this.alloc_github_url(dep); @@ -1475,10 +1516,13 @@ pub fn enqueue_dependency_with_main_and_success_fn( } }; - // First: see if we already loaded the tarball package in-memory - if let Some(pkg_id) = this.lockfile.get_package_id(name_hash, None, &res) { - success_fn(this, id, pkg_id); - return Ok(()); + // First: see if we already loaded the tarball package in-memory. + // See the git arm for the update-target gate. + if !(is_provisional_name && is_update_target(this, dependency, id)) { + if let Some(pkg_id) = this.lockfile.get_package_id(name_hash, None, &res) { + success_fn(this, id, pkg_id); + return Ok(()); + } } // reshaped for borrowck — `url` borrows `string_bytes`; @@ -1968,8 +2012,8 @@ fn get_or_put_resolved_package_with_find_result( // borrows `this.lockfile` and `this` at once. Split via raw root. let should_update = { let this_ptr: *mut PackageManager = this; - // SAFETY: `is_root_dependency` reads `manager.root_dependency_list` / - // `manager.workspace_package_json_cache` only — disjoint from + // SAFETY: `is_root_dependency` reads `manager.root_package_id` / + // `manager.workspace_name_hash` only — disjoint from // `manager.lockfile`. this.to_update // Update direct deps of the current workspace; catalogs are root-scoped. diff --git a/src/install/PackageManager/runTasks.rs b/src/install/PackageManager/runTasks.rs index b4fee5fbb9d9..60d96b42cd7c 100644 --- a/src/install/PackageManager/runTasks.rs +++ b/src/install/PackageManager/runTasks.rs @@ -1387,6 +1387,64 @@ pub fn run_tasks( None, ); manager.task_batch.push(ThreadPoolBatch::from(queued)); + } else if C::IS_STORE_INSTALLER { + // Installing! The isolated store parked its entry contexts + // under `checkout_id` (`enqueue_git_for_checkout`) with a + // `Dependency` waiter under the clone id. Enqueue each + // waiter's checkout directly from its lockfile resolution: + // re-running the dependency resolver here would bind the + // already-resolved dependency and return without ever + // scheduling the checkout, starving the parked entry. + if let Some(waiters) = manager.task_queue.remove(&task.id) { + for waiter in waiters.iter() { + let dep_id = match waiter { + bun_install::TaskCallbackContext::Dependency(id) => *id, + _ => continue, + }; + let pkg_id = manager.lockfile.buffers.resolutions[dep_id as usize]; + if pkg_id == INVALID_PACKAGE_ID { + continue; + } + let res = manager.lockfile.packages.items_resolution()[pkg_id as usize]; + if res.tag != bun_install::ResolutionTag::Git { + continue; + } + let (dep_name_handle, is_required) = { + let dep = &manager.lockfile.buffers.dependencies[dep_id as usize]; + (dep.name, dep.behavior.is_required()) + }; + // SAFETY: `string_bytes` lives as long as + // `manager.lockfile` and is not reallocated while + // install-phase tasks are draining. + let string_buf = unsafe { + bun_ptr::detach_lifetime( + manager.lockfile.buffers.string_bytes.as_slice(), + ) + }; + // SAFETY: `res.tag == Git` checked above — `value.git` + // is the active union arm. + let res_git = res.git(); + let resolved = res_git.resolved.slice(string_buf); + let checkout_id = Task::Id::for_git_checkout( + res_git.repo.slice(string_buf), + resolved, + ); + if manager.has_created_network_task(checkout_id, is_required) { + continue; + } + let queued = enqueue::enqueue_git_checkout( + manager, + checkout_id, + repo_fd, + dep_id, + dep_name_handle.slice(string_buf), + &res, + resolved, + None, + ); + manager.task_batch.push(ThreadPoolBatch::from(queued)); + } + } } else { // Resolving! let dependency_list_entry = manager diff --git a/src/install/lockfile/bun.lock.rs b/src/install/lockfile/bun.lock.rs index be1254346f20..4807936daa41 100644 --- a/src/install/lockfile/bun.lock.rs +++ b/src/install/lockfile/bun.lock.rs @@ -3061,7 +3061,8 @@ fn resolve_peer_dep_version_based( // `package_index` is keyed by *real* package names while `dep.name_hash` // may hold an alias, so an `npm:`-aliased peer must be looked up under // the real package name (`dep.realname()`). Mirrors the realname hashing - // in `enqueue_dependency_with_main_and_success_fn`. + // in `enqueue_dependency_with_main_and_success_fn`, including its + // empty-realname alias fallback. let name_hash = match dep.version.tag { DependencyVersionTag::DistTag | DependencyVersionTag::Git @@ -3069,7 +3070,11 @@ fn resolve_peer_dep_version_based( | DependencyVersionTag::Npm | DependencyVersionTag::Tarball | DependencyVersionTag::Workspace => { - StringBuilder::string_hash(dep.realname().slice(string_buf)) + let mut name = dep.realname(); + if name.is_empty() { + name = dep.name; + } + StringBuilder::string_hash(name.slice(string_buf)) } _ => dep.name_hash, }; diff --git a/test/cli/install/bun-lock.test.ts b/test/cli/install/bun-lock.test.ts index 8180f837292a..895c783a9615 100644 --- a/test/cli/install/bun-lock.test.ts +++ b/test/cli/install/bun-lock.test.ts @@ -1012,3 +1012,340 @@ it("optional peer with a non-wildcard range is idempotent with two versions of t await rm(join(packageDir, "node_modules"), { recursive: true, force: true }); await run(["install", "--frozen-lockfile"]); }); + +// Minimal gzipped tarball with a single root folder wrapping the files, the +// shape of both github codeload tarballs and npm pack tarballs. +function makeTarball(rootDir: string, files: Record): Uint8Array { + function tarHeader(name: string, size: number, isDir: boolean): Uint8Array { + const header = new Uint8Array(512); + const encoder = new TextEncoder(); + header.set(encoder.encode(name), 0); + header.set(encoder.encode(isDir ? "0000755 " : "0000644 "), 100); + header.set(encoder.encode("0000000 "), 108); + header.set(encoder.encode("0000000 "), 116); + header.set(encoder.encode(size.toString(8).padStart(11, "0") + " "), 124); + header.set(encoder.encode("00000000000 "), 136); + header.set(encoder.encode(" "), 148); + header[156] = (isDir ? "5" : "0").charCodeAt(0); + header.set(encoder.encode("ustar"), 257); + header.set(encoder.encode("00"), 263); + let checksum = 0; + for (const byte of header) checksum += byte; + header.set(encoder.encode(checksum.toString(8).padStart(6, "0") + "\0 "), 148); + return header; + } + const blocks: Uint8Array[] = []; + blocks.push(tarHeader(`${rootDir}/`, 0, true)); + for (const [name, contents] of Object.entries(files)) { + const bytes = new TextEncoder().encode(contents); + blocks.push(tarHeader(`${rootDir}/${name}`, bytes.length, false)); + blocks.push(bytes); + if (bytes.length % 512 !== 0) blocks.push(new Uint8Array(512 - (bytes.length % 512))); + } + blocks.push(new Uint8Array(1024)); + return Bun.gzipSync(Buffer.concat(blocks)); +} + +// Re-resolving a dirty lockfile used to re-download every github and remote +// tarball dependency the lockfile had already resolved: the enqueue looked the +// package up under its real package name, which for these dependency types is +// only learned from the first extract and is still empty on a fresh parse, so +// the in-memory lookup always missed and scheduled a new download. +it("re-resolving reuses github and remote tarball packages from the lockfile instead of re-downloading", async () => { + const { packageDir, packageJson } = await registry.createTestDir(); + + const ghTarball = makeTarball("testowner-testrepo-aaaaaaa", { + "package.json": JSON.stringify({ name: "gh-dep", version: "1.0.0" }), + "index.js": "module.exports = 'gh';\n", + }); + const tdTarball = makeTarball("package", { + "package.json": JSON.stringify({ name: "td-dep", version: "1.0.0" }), + "index.js": "module.exports = 'td';\n", + }); + + let githubDownloads = 0; + let tarballDownloads = 0; + await using server = Bun.serve({ + port: 0, + fetch(req) { + const { pathname } = new URL(req.url); + if (pathname === "/td-dep.tgz") { + tarballDownloads++; + return new Response(tdTarball, { headers: { "Content-Type": "application/gzip" } }); + } + githubDownloads++; + return new Response(ghTarball, { headers: { "Content-Type": "application/gzip" } }); + }, + }); + + const installEnv = { + ...env, + GITHUB_API_URL: `http://localhost:${server.port}`, + // CI exports BUN_INSTALL_CACHE_DIR; pin it so this test's cache is its own. + BUN_INSTALL_CACHE_DIR: join(packageDir, ".bun-cache"), + }; + async function install() { + await using proc = spawn({ + cmd: [bunExe(), "install"], + cwd: packageDir, + env: installEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [err, code] = await Promise.all([proc.stderr.text(), proc.exited, proc.stdout.text()]); + expect(err).not.toContain("error:"); + expect(code).toBe(0); + } + + await write(packageJson, JSON.stringify({ name: "ws-root", workspaces: ["packages/*"] })); + const memberPackageJson = join(packageDir, "packages", "member", "package.json"); + const memberDeps: Record = { + "gh-dep": "github:testowner/testrepo#aaaaaaa", + "td-dep": `http://localhost:${server.port}/td-dep.tgz`, + }; + await write(memberPackageJson, JSON.stringify({ name: "member", version: "1.0.0", dependencies: memberDeps })); + await write( + join(packageDir, "packages", "member", "dummy", "package.json"), + JSON.stringify({ name: "dummy", version: "1.0.0" }), + ); + + await install(); + expect({ githubDownloads, tarballDownloads }).toEqual({ githubDownloads: 1, tarballDownloads: 1 }); + + // Dirty the lockfile with a change that re-resolves the member's unchanged + // dependencies (a workspace member edit re-parses its whole dependency list). + memberDeps["dummy"] = "file:./dummy"; + await write(memberPackageJson, JSON.stringify({ name: "member", version: "1.0.0", dependencies: memberDeps })); + await install(); + expect({ githubDownloads, tarballDownloads }).toEqual({ githubDownloads: 1, tarballDownloads: 1 }); + + const lock = await file(join(packageDir, "bun.lock")).text(); + expect(lock).toContain("gh-dep@github:testowner/testrepo#aaaaaaa"); + expect(lock).toContain(`td-dep@http://localhost:${server.port}/td-dep.tgz`); + expect(await file(join(packageDir, "node_modules", "gh-dep", "index.js")).text()).toBe("module.exports = 'gh';\n"); + expect(await file(join(packageDir, "node_modules", "td-dep", "index.js")).text()).toBe("module.exports = 'td';\n"); +}); + +// Same bug through the git: dependency path (clone/fetch tasks instead of a +// tarball download). The repo is served over git's dumb HTTP protocol: after +// `git update-server-info`, a bare repo is plain static files. +it("re-resolving reuses a git package from the lockfile instead of re-fetching", async () => { + const { packageDir, packageJson } = await registry.createTestDir(); + + const srcDir = join(packageDir, "git-src"); + const bareDir = join(packageDir, "repo.git"); + // Isolate git from system/global config (e.g. core.autocrlf on Windows). + const gitEnv = { + ...env, + GIT_CONFIG_NOSYSTEM: "1", + GIT_CONFIG_GLOBAL: join(packageDir, "gitconfig"), + GIT_AUTHOR_NAME: "bun-test", + GIT_AUTHOR_EMAIL: "test@bun.sh", + GIT_COMMITTER_NAME: "bun-test", + GIT_COMMITTER_EMAIL: "test@bun.sh", + }; + async function git(args: string[], cwd: string): Promise { + await using proc = spawn({ cmd: ["git", ...args], cwd, env: gitEnv, stdout: "pipe", stderr: "pipe" }); + const [out, err, code] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(err).not.toContain("fatal:"); + expect(code).toBe(0); + return out; + } + + await write(join(packageDir, "gitconfig"), "[core]\n\tautocrlf = false\n"); + await write(join(srcDir, "package.json"), JSON.stringify({ name: "git-dep", version: "1.0.0" })); + await write(join(srcDir, "index.js"), "module.exports = 'git';\n"); + await git(["init", "-q"], srcDir); + await git(["add", "-A"], srcDir); + await git(["commit", "-qm", "init"], srcDir); + const sha = (await git(["rev-parse", "HEAD"], srcDir)).trim(); + await git(["clone", "-q", "--bare", srcDir, bareDir], packageDir); + await git(["update-server-info"], bareDir); + + let gitRequests = 0; + await using server = Bun.serve({ + port: 0, + async fetch(req) { + gitRequests++; + const { pathname } = new URL(req.url); + if (!pathname.startsWith("/repo.git/")) return new Response("not found", { status: 404 }); + const f = file(join(bareDir, pathname.slice("/repo.git/".length))); + return (await f.exists()) ? new Response(f) : new Response("not found", { status: 404 }); + }, + }); + + const installEnv = { + ...gitEnv, + BUN_INSTALL_CACHE_DIR: join(packageDir, ".bun-cache"), + }; + async function install(extraArgs: string[] = []) { + await using proc = spawn({ + cmd: [bunExe(), "install", ...extraArgs], + cwd: packageDir, + env: installEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [err, code] = await Promise.all([proc.stderr.text(), proc.exited, proc.stdout.text()]); + expect(err).not.toContain("error:"); + expect(code).toBe(0); + } + + await write(packageJson, JSON.stringify({ name: "ws-root", workspaces: ["packages/*"] })); + const memberPackageJson = join(packageDir, "packages", "member", "package.json"); + const memberDeps: Record = { + "git-dep": `git+http://127.0.0.1:${server.port}/repo.git#${sha}`, + }; + await write(memberPackageJson, JSON.stringify({ name: "member", version: "1.0.0", dependencies: memberDeps })); + await write( + join(packageDir, "packages", "member", "dummy", "package.json"), + JSON.stringify({ name: "dummy", version: "1.0.0" }), + ); + + await install(); + expect(gitRequests).toBeGreaterThan(0); + + memberDeps["dummy"] = "file:./dummy"; + await write(memberPackageJson, JSON.stringify({ name: "member", version: "1.0.0", dependencies: memberDeps })); + const requestsAfterFirstInstall = gitRequests; + await install(); + expect(gitRequests).toBe(requestsAfterFirstInstall); + + expect(await file(join(packageDir, "bun.lock")).text()).toContain( + `git-dep@git+http://127.0.0.1:${server.port}/repo.git#${sha}`, + ); + expect(await file(join(packageDir, "node_modules", "git-dep", "index.js")).text()).toBe("module.exports = 'git';\n"); + + // Cold cache with the lockfile already resolved: the isolated installer + // must clone and then check out. Its clone-completion path used to lean on + // the dependency resolver missing the in-memory lookup to schedule the + // checkout, which deadlocked once that lookup could hit. + await rm(join(packageDir, "node_modules"), { recursive: true, force: true }); + await rm(join(packageDir, "packages", "member", "node_modules"), { recursive: true, force: true }); + await rm(join(packageDir, ".bun-cache"), { recursive: true, force: true }); + const requestsBeforeColdInstall = gitRequests; + await install(["--linker", "isolated"]); + expect(gitRequests).toBeGreaterThan(requestsBeforeColdInstall); + expect(await file(join(packageDir, "packages", "member", "node_modules", "git-dep", "index.js")).text()).toBe( + "module.exports = 'git';\n", + ); +}); + +// Overrides match dependencies by the name they are declared under. For a +// github dependency that name used to be unknown until the first extract, so +// the override only applied after a wasted tarball download; now it applies on +// the first enqueue and the tarball is never fetched. +it("an override on a github dependency applies without downloading the tarball", async () => { + const { packageDir, packageJson } = await registry.createTestDir(); + + const ghTarball = makeTarball("testowner-testrepo-aaaaaaa", { + "package.json": JSON.stringify({ name: "no-deps", version: "9.9.9" }), + "index.js": "module.exports = 'github';\n", + }); + let githubDownloads = 0; + await using server = Bun.serve({ + port: 0, + fetch() { + githubDownloads++; + return new Response(ghTarball, { headers: { "Content-Type": "application/gzip" } }); + }, + }); + + await write( + packageJson, + JSON.stringify({ + name: "override-root", + dependencies: { "no-deps": "github:testowner/testrepo#aaaaaaa" }, + overrides: { "no-deps": "1.0.0" }, + }), + ); + + await using proc = spawn({ + cmd: [bunExe(), "install"], + cwd: packageDir, + env: { + ...env, + GITHUB_API_URL: `http://localhost:${server.port}`, + BUN_INSTALL_CACHE_DIR: join(packageDir, ".bun-cache"), + }, + stdout: "pipe", + stderr: "pipe", + }); + const [err, code] = await Promise.all([proc.stderr.text(), proc.exited, proc.stdout.text()]); + expect(err).not.toContain("error:"); + expect(code).toBe(0); + + expect(githubDownloads).toBe(0); + expect(await file(join(packageDir, "bun.lock")).text()).toContain('"no-deps": ["no-deps@1.0.0"'); + expect(await file(join(packageDir, "node_modules", "no-deps", "package.json")).json()).toMatchObject({ + name: "no-deps", + version: "1.0.0", + }); +}); + +// `bun update ` must re-fetch git-backed and tarball dependencies rather +// than rebinding them to the lockfile-resolved package. +it("bun update re-fetches github and remote tarball dependencies", async () => { + const { packageDir, packageJson } = await registry.createTestDir(); + + const ghTarball = makeTarball("testowner-testrepo-aaaaaaa", { + "package.json": JSON.stringify({ name: "gh-dep", version: "1.0.0" }), + "index.js": "module.exports = 'gh';\n", + }); + const tdTarball = makeTarball("package", { + "package.json": JSON.stringify({ name: "td-dep", version: "1.0.0" }), + "index.js": "module.exports = 'td';\n", + }); + let githubDownloads = 0; + let tarballDownloads = 0; + await using server = Bun.serve({ + port: 0, + fetch(req) { + const { pathname } = new URL(req.url); + if (pathname === "/td-dep.tgz") { + tarballDownloads++; + return new Response(tdTarball, { headers: { "Content-Type": "application/gzip" } }); + } + githubDownloads++; + return new Response(ghTarball, { headers: { "Content-Type": "application/gzip" } }); + }, + }); + + const installEnv = { + ...env, + GITHUB_API_URL: `http://localhost:${server.port}`, + BUN_INSTALL_CACHE_DIR: join(packageDir, ".bun-cache"), + }; + async function run(args: string[]) { + await using proc = spawn({ + cmd: [bunExe(), ...args], + cwd: packageDir, + env: installEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [err, code] = await Promise.all([proc.stderr.text(), proc.exited, proc.stdout.text()]); + expect(err).not.toContain("error:"); + expect(code).toBe(0); + } + + await write( + packageJson, + JSON.stringify({ + name: "update-root", + dependencies: { + "gh-dep": "github:testowner/testrepo#aaaaaaa", + "td-dep": `http://localhost:${server.port}/td-dep.tgz`, + }, + }), + ); + + await run(["install"]); + expect({ githubDownloads, tarballDownloads }).toEqual({ githubDownloads: 1, tarballDownloads: 1 }); + + await run(["update", "td-dep"]); + expect({ githubDownloads, tarballDownloads }).toEqual({ githubDownloads: 1, tarballDownloads: 2 }); + + await run(["update", "gh-dep"]); + expect({ githubDownloads, tarballDownloads }).toEqual({ githubDownloads: 2, tarballDownloads: 2 }); +});