Skip to content
Closed
Show file tree
Hide file tree
Changes from 6 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
12 changes: 12 additions & 0 deletions src/install/PackageManager.zig
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,16 @@ global_link_dir: ?std.fs.Dir = null,
global_dir: ?std.fs.Dir = null,
global_link_dir_path: string = "",

/// Names of packages registered via `bun link`, read from
/// `<globalLinkDir>/` once per install. Populated on the main thread
/// before any install worker starts; after that it's read-only and can
/// be accessed lock-free from worker threads. Lookups return early
/// when the set is empty (no active links) — the common case on dev
/// machines without `bun link` configured, and unconditional on CI.
/// See `populateLinkedNamesCache` / `linkedPackagePath`.
linked_names: bun.StringHashMapUnmanaged(void) = .{},
linked_names_populated: bool = false,

onWake: WakeHandler = .{},
ci_mode: bun.LazyBool(computeIsContinuousIntegration, @This(), "ci_mode") = .{},

Expand Down Expand Up @@ -1177,6 +1187,8 @@ pub const getTemporaryDirectory = directories.getTemporaryDirectory;
pub const globalLinkDir = directories.globalLinkDir;
pub const globalLinkDirAndPath = directories.globalLinkDirAndPath;
pub const globalLinkDirPath = directories.globalLinkDirPath;
pub const linkedPackagePath = directories.linkedPackagePath;
pub const populateLinkedNamesCache = directories.populateLinkedNamesCache;
pub const isFolderInCache = directories.isFolderInCache;
pub const pathForCachedNPMPath = directories.pathForCachedNPMPath;
pub const pathForResolution = directories.pathForResolution;
Expand Down
117 changes: 117 additions & 0 deletions src/install/PackageManager/PackageManagerDirectories.zig
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,123 @@ pub fn globalLinkDirAndPath(this: *PackageManager) struct { std.fs.Dir, []const
return .{ dir, this.global_link_dir_path };
}

/// Read the global link dir once and populate
/// `this.linked_names` with every registered package name (including
/// scoped names as `@scope/name`). Must be called on the main thread
/// before any install worker touches `linkedPackagePath`; after that
/// the map is read-only and lock-free.
///
/// Safe to call repeatedly; subsequent calls are no-ops.
pub fn populateLinkedNamesCache(this: *PackageManager) void {
if (this.linked_names_populated) return;
this.linked_names_populated = true;

// Best-effort: for users who have never run `bun link`, the global
// link dir may not exist (or may be unreadable). `globalLinkDirPath`
// would `Global.exit(1)` on setup failure — treat that same failure
// here as "no links on this machine" instead, and leave the cache
// empty so `linkedPackagePath` short-circuits to null. If a later
// code path really does need the global dir (e.g. `bun link`
// itself), `globalLinkDirPath` will be called on that path and
// surface the error there.
const dir_path = dir_path: {
if (this.global_link_dir_path.len != 0) break :dir_path this.global_link_dir_path;
var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return;
const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return;
this.global_dir = global_dir;
this.global_link_dir = link_dir;
var buf: bun.PathBuffer = undefined;
const path_slice = bun.getFdPath(.fromStdDir(link_dir), &buf) catch return;
this.global_link_dir_path = bun.handleOom(Fs.FileSystem.DirnameStore.instance.append([]const u8, path_slice));
Comment on lines +434 to +440

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid caching a half-initialized global link dir.

If bun.getFdPath() fails here, populateLinkedNamesCache() returns with this.global_link_dir already set but this.global_link_dir_path still empty. A later globalLinkDirPath() call then skips re-initialization and can build linked-package paths from "". Move the field assignments until after the path lookup succeeds, and close the local dirs on the early-return path.

Suggested fix
-        var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return;
-        const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return;
-        this.global_dir = global_dir;
-        this.global_link_dir = link_dir;
+        var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return;
+        errdefer global_dir.close();
+        const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return;
+        errdefer link_dir.close();
         var buf: bun.PathBuffer = undefined;
         const path_slice = bun.getFdPath(.fromStdDir(link_dir), &buf) catch return;
+        this.global_dir = global_dir;
+        this.global_link_dir = link_dir;
         this.global_link_dir_path = bun.handleOom(Fs.FileSystem.DirnameStore.instance.append([]const u8, path_slice));
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return;
const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return;
this.global_dir = global_dir;
this.global_link_dir = link_dir;
var buf: bun.PathBuffer = undefined;
const path_slice = bun.getFdPath(.fromStdDir(link_dir), &buf) catch return;
this.global_link_dir_path = bun.handleOom(Fs.FileSystem.DirnameStore.instance.append([]const u8, path_slice));
var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return;
errdefer global_dir.close();
const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return;
errdefer link_dir.close();
var buf: bun.PathBuffer = undefined;
const path_slice = bun.getFdPath(.fromStdDir(link_dir), &buf) catch return;
this.global_dir = global_dir;
this.global_link_dir = link_dir;
this.global_link_dir_path = bun.handleOom(Fs.FileSystem.DirnameStore.instance.append([]const u8, path_slice));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/install/PackageManager/PackageManagerDirectories.zig` around lines 434 -
440, The code sets this.global_dir and this.global_link_dir before calling
bun.getFdPath, which can fail and leave this.global_link_dir_path empty; move
the assignments to this.global_dir and this.global_link_dir until after
bun.getFdPath and the call to Fs.FileSystem.DirnameStore.instance.append(...)
succeed so the object is never left half-initialized; if bun.getFdPath or the
path append fails, close the opened local dir handles (the values returned by
Options.openGlobalDir and global_dir.makeOpenPath) before returning to avoid
resource leaks; ensure populateLinkedNamesCache() and globalLinkDirPath() will
see either all three fields set or none set.

break :dir_path this.global_link_dir_path;
};
const root_fd = switch (bun.openDirForIteration(bun.FD.cwd(), dir_path)) {
.result => |fd| fd,
// Dir missing / unreadable → empty set. Every linkedPackagePath
// lookup will short-circuit to null with no further syscalls.
.err => return,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
};
defer root_fd.close();

var iter = bun.DirIterator.iterate(root_fd, if (bun.Environment.isWindows) .u16 else .u8);
while (iter.next().unwrap() catch null) |entry| {
const name = entry.name.slice();
if (name.len == 0) continue;

// Scope dirs (`@scope`) contain the actual links nested one
// level deeper; flatten to `@scope/name` in the cache.
if (name[0] == '@' and entry.kind == .directory) {
if (comptime bun.Environment.isWindows) {
// WTF-16 name; skip scope flattening on Windows for now.
// Falls through to the lstat path in linkedPackagePath.
continue;
}
const scope_fd = switch (bun.openDirForIteration(root_fd, name)) {
.result => |fd| fd,
.err => continue,
};
defer scope_fd.close();

var scope_iter = bun.DirIterator.iterate(scope_fd, .u8);
while (scope_iter.next().unwrap() catch null) |scope_entry| {
const sub_name = scope_entry.name.slice();
if (sub_name.len == 0) continue;
const full = bun.handleOom(std.fmt.allocPrint(this.allocator, "{s}/{s}", .{ name, sub_name }));
bun.handleOom(this.linked_names.put(this.allocator, full, {}));
}
continue;
}

if (comptime bun.Environment.isWindows) continue;
const dup = bun.handleOom(this.allocator.dupe(u8, name));
bun.handleOom(this.linked_names.put(this.allocator, dup, {}));
}
}

/// If `<globalLinkDir>/<pkg_name>` exists (typically a symlink created by
/// `bun link` from the producer dir), write its absolute path into `buf` and
/// return it. Otherwise `null`. Scoped names (`@scope/name`) are handled
/// because `joinAbsStringBufZ` preserves the `/`.
///
/// Performance: when `linked_names` has been populated (via
/// `populateLinkedNamesCache` at install start), this is a single
/// hashmap check with no syscalls. When the cache is empty (no active
/// links on this machine), it returns immediately. Falls back to the
/// per-call `lstat` when the cache has not been populated (e.g. on
/// Windows, or if a caller runs outside the isolated-install flow).
pub fn linkedPackagePath(
this: *PackageManager,
pkg_name: []const u8,
buf: *bun.PathBuffer,
) ?[:0]const u8 {
if (pkg_name.len == 0) return null;

const use_cache = this.linked_names_populated and !bun.Environment.isWindows;
if (use_cache) {
if (this.linked_names.count() == 0) return null;
if (!this.linked_names.contains(pkg_name)) return null;
const dir_path = this.globalLinkDirPath();
return bun.path.joinAbsStringBufZ(dir_path, buf, &.{pkg_name}, .auto);
}

const dir_path = this.globalLinkDirPath();
const joined = bun.path.joinAbsStringBufZ(dir_path, buf, &.{pkg_name}, .auto);
if (comptime bun.Environment.isWindows) {
const attrs = bun.sys.getFileAttributes(joined) orelse return null;
return if (attrs.is_directory or attrs.is_reparse_point) joined else null;
}
return switch (bun.sys.lstat(joined)) {
.result => |st| brk: {
const mode: u32 = @intCast(st.mode);
if (std.posix.S.ISDIR(mode) or std.posix.S.ISLNK(mode)) {
break :brk joined;
}
break :brk null;
},
.err => null,
};
}

pub fn pathForCachedNPMPath(
this: *PackageManager,
buf: *bun.PathBuffer,
Expand Down
9 changes: 9 additions & 0 deletions src/install/PackageManager/WorkspacePackageJSONCache.zig
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,15 @@ pub const GetResult = union(enum) {
};

map: Map = .{},
/// `getWithPath` / `getWithSource` mutate `map` (which invalidates
/// previously-returned `*MapEntry` pointers on grow) and call
/// `initializeStore()` + the JSON parser — none of which are
/// thread-safe. Multi-threaded callers (currently only the isolated
/// installer's `Task.run`) must hold this mutex across both the call
/// *and* any use of the returned entry pointer, since a concurrent
/// call would otherwise grow the map and invalidate the pointer.
/// Single-threaded callers can ignore it.
lock: bun.Mutex = .{},

/// Given an absolute path to a workspace package.json, return the AST
/// and contents of the file. If the package.json is not present in the
Expand Down
1 change: 1 addition & 0 deletions src/install/PackageManager/patchPackage.zig
Original file line number Diff line number Diff line change
Expand Up @@ -835,6 +835,7 @@ fn overwritePackageInNodeModulesFolder(
.fromStdDir(cached_package_folder),
src_path,
dest_subpath,
&.{},
ignore_directories,
);
defer copier.deinit();
Expand Down
30 changes: 30 additions & 0 deletions src/install/isolated_install.zig
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ pub fn installIsolatedPackages(
) OOM!PackageInstall.Summary {
bun.analytics.Features.isolated_bun_install += 1;

// Populate the linked-names cache once, on the main thread, before
// any install worker calls `manager.linkedPackagePath()`. Replaces a
// per-dependency `lstat` with a single readdir of the global link
// dir; short-circuits every subsequent lookup when nothing is
// linked (the common case on CI and dev machines without active
// links). See populateLinkedNamesCache in
// PackageManager/PackageManagerDirectories.zig.
manager.populateLinkedNamesCache();

const lockfile = manager.lockfile;

const store: Store = store: {
Expand Down Expand Up @@ -1577,6 +1586,17 @@ pub fn installIsolatedPackages(
// write the new project-local tree through the link into
// the shared cache). Treat the stale link as
// needs-install so `link_package` detaches and rebuilds.
// An active `bun link` for this package name means the
// producer dir is the source of truth — override the
// cache-based materialization regardless of whether the
// store dir already exists. The existence check below
// would otherwise short-circuit and leave the body stale.
// Only fires when the user didn't opt into the
// symlink-only backend.
var link_buf: bun.PathBuffer = undefined;
const has_active_link = PackageInstall.supported_method != .symlink and
manager.linkedPackagePath(pkg_name.slice(string_buf), &link_buf) != null;

const has_stale_gvs_link = !uses_global_store and stale: {
if (installer.global_store_path == null) break :stale false;
var local: bun.Path(.{ .sep = .auto }) = .initTopLevelDir();
Expand All @@ -1599,6 +1619,7 @@ pub fn installIsolatedPackages(
// should still take the cheap symlink-only path.
(is_new_bun_modules and !uses_global_store) or
has_stale_gvs_link or
has_active_link or
patch_info == .remove or
needs_install: {
var store_path: bun.AbsPath(.{}) = .initTopLevelDir();
Expand Down Expand Up @@ -1657,6 +1678,15 @@ pub fn installIsolatedPackages(
continue;
}

// `link_package` will source from the producer dir via
// `linkedPackagePath`; skip the cache-fetch dance entirely
// (mirrors how `.folder` is handled — no registry traffic
// needed when the body comes from an on-disk producer).
if (has_active_link) {
installer.startTask(entry_id);
continue;
}

var pkg_cache_dir_subpath: bun.RelPath(.{ .sep = .auto }) = .from(switch (pkg_res_tag) {
.npm => manager.cachedNPMPackageFolderName(pkg_name.slice(string_buf), pkg_res.value.npm.version, patch_info.contentsHash()),
.git => manager.cachedGitFolderName(&pkg_res.value.git, patch_info.contentsHash()),
Expand Down
3 changes: 2 additions & 1 deletion src/install/isolated_install/FileCopier.zig
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ pub const FileCopier = struct {
src_dir: FD,
src_path: bun.AbsPath(.{ .sep = .auto, .unit = .os }),
dest_subpath: bun.Path(.{ .sep = .auto, .unit = .os }),
skip_filenames: []const bun.OSPathSlice,
skip_dirnames: []const bun.OSPathSlice,
) OOM!FileCopier {
return .{
Expand All @@ -16,7 +17,7 @@ pub const FileCopier = struct {
var w = try Walker.walk(
src_dir,
bun.default_allocator,
&.{},
skip_filenames,
skip_dirnames,
);
w.resolve_unknown_entry_types = true;
Expand Down
3 changes: 2 additions & 1 deletion src/install/isolated_install/Hardlinker.zig
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ pub fn init(
folder_dir: FD,
src: bun.AbsPath(.{ .sep = .auto, .unit = .os }),
dest: bun.Path(.{ .sep = .auto, .unit = .os }),
skip_filenames: []const bun.OSPathSlice,
skip_dirnames: []const bun.OSPathSlice,
) OOM!Hardlinker {
return .{
Expand All @@ -19,7 +20,7 @@ pub fn init(
var w = try Walker.walk(
folder_dir,
bun.default_allocator,
&.{},
skip_filenames,
skip_dirnames,
);
w.resolve_unknown_entry_types = true;
Expand Down
Loading