Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/install/isolated_install.zig
Original file line number Diff line number Diff line change
Expand Up @@ -831,6 +831,8 @@ pub fn installIsolatedPackages(
.supported_backend = .init(PackageInstall.supported_method),
.is_new_bun_modules = is_new_bun_modules,
};
defer installer.deinit();
defer installer.blocked_entries.deinit(lockfile.allocator);
Comment thread
robobun marked this conversation as resolved.

for (tasks, 0..) |*task, _entry_id| {
const entry_id: Store.Entry.Id = .from(@intCast(_entry_id));
Expand Down
35 changes: 20 additions & 15 deletions src/install/isolated_install/Installer.zig
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,13 @@ pub const Installer = struct {

trusted_dependencies_from_update_requests: std.AutoArrayHashMapUnmanaged(TruncatedPackageNameHash, void),

pub fn deinit(this: *const Installer) void {
/// Tracks which entries are currently blocked, avoiding O(N) iteration over all entries
/// in resumeUnblockedTasks. Only modified from the main thread.
blocked_entries: std.AutoArrayHashMapUnmanaged(Store.Entry.Id, void) = .empty,

// Note: blocked_entries is freed via a targeted defer at the call site
// in isolated_install.zig, not here. See the defer ordering there.
pub fn deinit(this: *Installer) void {
this.trusted_dependencies_from_update_requests.deinit(this.lockfile.allocator);
}
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.

Expand Down Expand Up @@ -215,6 +221,7 @@ pub const Installer = struct {

// .monotonic is okay because the task isn't running right now.
this.store.entries.items(.step)[entry_id.get()].store(.blocked, .monotonic);
bun.handleOom(this.blocked_entries.put(this.lockfile.allocator, entry_id, {}));
}

/// Called from both the main thread (via `onTaskBlocked` and `resumeUnblockedTasks`) and the
Expand Down Expand Up @@ -298,32 +305,30 @@ pub const Installer = struct {
this.installed.set(pkg_id);
}

// This function runs only on the main thread. The installer tasks threads
// will be changing values in `entry_step`, but the blocked state is only
// set on the main thread, allowing the code between
// `entry_steps[entry_id.get()].load(.monotonic)`
// and
// `entry_steps[entry_id.get()].store(.symlink_dependency_binaries, .monotonic)`
// This function runs only on the main thread. `blocked_entries` is only
// modified from the main thread (added in `onTaskBlocked`, removed here),
// so no concurrent modification can occur. The two-phase collect-then-remove
// pattern avoids mutating `blocked_entries` during iteration.
pub fn resumeUnblockedTasks(this: *Installer) void {
const entries = this.store.entries.slice();
const entry_steps = entries.items(.step);

var parent_dedupe: std.AutoArrayHashMap(Store.Entry.Id, void) = .init(bun.default_allocator);
defer parent_dedupe.deinit();

for (0..this.store.entries.len) |id_int| {
const entry_id: Store.Entry.Id = .from(@intCast(id_int));

// .monotonic is okay because only the main thread sets this to `.blocked`.
const entry_step = entry_steps[entry_id.get()].load(.monotonic);
if (entry_step != .blocked) {
continue;
}
// Collect entries to unblock first since we can't modify blocked_entries while iterating.
Comment thread
robobun marked this conversation as resolved.
var to_unblock: std.ArrayListUnmanaged(Store.Entry.Id) = .empty;
defer to_unblock.deinit(bun.default_allocator);

for (this.blocked_entries.keys()) |entry_id| {
if (this.isTaskBlocked(entry_id, &parent_dedupe)) {
continue;
}
bun.handleOom(to_unblock.append(bun.default_allocator, entry_id));
}

for (to_unblock.items) |entry_id| {
_ = this.blocked_entries.swapRemove(entry_id);
// .monotonic is okay because the task isn't running right now.
entry_steps[entry_id.get()].store(.symlink_dependency_binaries, .monotonic);
this.startTask(entry_id);
Expand Down
83 changes: 83 additions & 0 deletions test/regression/issue/28422.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import { expect, test } from "bun:test";
import { existsSync } from "fs";
import { bunEnv, bunExe, tempDir } from "harness";
Comment thread
claude[bot] marked this conversation as resolved.
import { join } from "path";

const PKG_COUNT = 10;
const APP_COUNT = 3;

// Regression test for https://github.com/oven-sh/bun/issues/28422
// Isolated install was O(N²) due to resumeUnblockedTasks iterating all store
// entries on every task completion. With many workspace packages and deps,
// this caused extreme slowdowns (50x+ slower than hoisted).
test("isolated install with workspace monorepo completes without hanging", async () => {
const files: Record<string, string> = {
"bunfig.toml": `[install]\nlinker = "isolated"\n`,
"package.json": JSON.stringify({
name: "monorepo-root",
workspaces: ["packages/*", "apps/*"],
}),
};

// Generate PKG_COUNT packages with chained dependencies: pkg-i depends on pkg-(i-1),
// and for i>=2 also on pkg-0, creating cross-workspace connectivity.
for (let i = 0; i < PKG_COUNT; i++) {
const deps: Record<string, string> = {};
if (i > 0) {
deps[`pkg-${i - 1}`] = "workspace:*";
}
if (i > 1) {
deps["pkg-0"] = "workspace:*";
}
files[`packages/pkg-${i}/package.json`] = JSON.stringify({
name: `pkg-${i}`,
version: "1.0.0",
...(Object.keys(deps).length > 0 ? { dependencies: deps } : {}),
});
}

// Generate APP_COUNT apps, each depending on a spread of packages
for (let a = 0; a < APP_COUNT; a++) {
const deps: Record<string, string> = {};
// Each app depends on every 5th package plus its neighbors, creating high connectivity
for (let i = 0; i < PKG_COUNT; i += 5) {
deps[`pkg-${i}`] = "workspace:*";
}
// Plus a couple unique ones per app (offset by 1 to avoid collisions with every-5th set)
deps[`pkg-${(a * 3 + 1) % PKG_COUNT}`] = "workspace:*";
deps[`pkg-${(a * 7 + 2) % PKG_COUNT}`] = "workspace:*";

files[`apps/app-${a}/package.json`] = JSON.stringify({
name: `app-${a}`,
version: "1.0.0",
dependencies: deps,
});
}
Comment thread
robobun marked this conversation as resolved.

using dir = tempDir("isolated-perf", files);
const packageDir = String(dir);

await using proc = Bun.spawn({
cmd: [bunExe(), "install"],
cwd: packageDir,
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).not.toContain("error:");

// Verify workspace symlinks were created for a sample of packages
for (let a = 0; a < APP_COUNT; a++) {
expect(existsSync(join(packageDir, "apps", `app-${a}`, "node_modules", "pkg-0"))).toBeTrue();
}

// Verify chained workspace dependencies resolve
for (let i = 1; i < PKG_COUNT; i++) {
expect(existsSync(join(packageDir, "packages", `pkg-${i}`, "node_modules", `pkg-${i - 1}`))).toBeTrue();
}

expect(exitCode).toBe(0);
Comment thread
robobun marked this conversation as resolved.
});
Loading