fix(install): cleanup omitted top-level node_modules after install #26565
Conversation
WalkthroughThe changes implement a new top-level cleanup mechanism for the package installation process. A public function is added to scan node_modules and schedule deletion of packages not listed in the lockfile. The install command is updated to invoke this cleanup after successful installation when dependency categories are missing. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/cli/install_command.zig`:
- Around line 85-91: The cleanup call
PackageInstall.scheduleTopLevelOmittedCleanup(manager) is currently invoked with
try which will abort the install on allocation/errors; change it to run as
best-effort by catching and handling any error instead of propagating it (e.g.,
replace the try with a catch/if-error pattern that logs or warns the error and
continues), keeping the surrounding condition that checks
manager.options.local_package_features so the cleanup remains optional and
non-fatal to a successful bun install.
In `@src/install/PackageInstall.zig`:
- Around line 1237-1268: The pending-task counter can drift because
candidate_count is computed before a second pass that may fail or find different
entries; change the code to collect actual candidate package paths (e.g., into
an ArrayList or SliceList) during the first pass instead of only incrementing
candidate_count, then call manager.incrementPendingTasks with the collected list
length (list.len) and schedule deletions from that list in the second pass
(using the same collected entries when opening scope_dir or node entries);
alternatively, if you prefer minimal changes, ensure every failure in the second
pass (e.g., scope_dir.openDir, scope_dir.iterate, node_modules_dir.openDir)
decrements the pending counter via manager.decrementPendingTasks so pending
tasks cannot remain >0 — update logic around candidate_count,
manager.incrementPendingTasks, and the second-pass iteration to use the
collected list or perform symmetric decrement on failures.
- Around line 1231-1233: Replace the relative openDir("node_modules") calls that
use cwd with an absolute path built from FileSystem.instance.top_level_dir using
bun.path.joinAbsString(), e.g., construct a single absolute node_modules path
string once and use that value for both openDir/openFile calls (replace usages
of cwd and variable node_modules_dir with the new absolute path) so the code no
longer depends on current working directory; update the variables where
node_modules_dir and subsequent opens are created to use that joined absolute
path.
- Line 1233: Remove the early defer that closes node_modules_dir so we don't
double-close the handle after it is explicitly closed and reopened;
specifically, locate the variable node_modules_dir in PackageInstall.zig and
delete the first defer node_modules_dir.close() that runs before the explicit
node_modules_dir.close() and reassignment, and likewise remove the earlier defer
paired with the later defer at the second occurrence (the block around lines
1271–1273) so only the final close is deferred for the currently-open handle.
- Around line 1221-1227: Remove the inline imports inside
scheduleTopLevelOmittedCleanup: delete the local consts for std, bun,
FileSystem, String, and PackageNameHash and instead reference the existing
file-scope constants; then add a file-scope alias const PackageNameHash =
install.PackageNameHash to the top-level import block so
scheduleTopLevelOmittedCleanup uses the shared PackageNameHash constant rather
than creating duplicates.
- Around line 1294-1341: The DeleteTask structs use public field names and named
type literal initialization; change the field declarations to private by
prefixing with # (e.g., `#absolute_path`, `#task`), update all internal accesses in
methods run and deinit to use self.#absolute_path and this.#absolute_path, and
switch initializations from the named-type literal (DeleteTask{ ... }) to a
declaration literal with private keys (.{ .#absolute_path = abs, .#task = .{
.callback = &DeleteTask.run } }); apply the same changes for the second
DeleteTask instance as well and update the allocator create/assignment sites
that set t.* accordingly.
|
|
||
| if (!manager.options.local_package_features.dev_dependencies | ||
| or !manager.options.local_package_features.optional_dependencies | ||
| or !manager.options.local_package_features.peer_dependencies) | ||
| { | ||
| try PackageInstall.scheduleTopLevelOmittedCleanup(manager); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# View the context around lines 85-91 in install_command.zig
sed -n '75,100p' src/cli/install_command.zigRepository: oven-sh/bun
Length of output: 897
🏁 Script executed:
# Find the definition of scheduleTopLevelOmittedCleanup
fd -e zig | xargs rg -l "scheduleTopLevelOmittedCleanup" | head -5Repository: oven-sh/bun
Length of output: 113
🏁 Script executed:
# Search for the function signature
rg -n "fn scheduleTopLevelOmittedCleanup" src/Repository: oven-sh/bun
Length of output: 166
🏁 Script executed:
# View the implementation of scheduleTopLevelOmittedCleanup
sed -n '1221,1280p' src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 2791
🏁 Script executed:
# Look for similar cleanup patterns in PackageInstall.zig to understand error handling approach
rg -n "Cleanup" src/install/PackageInstall.zig -B 2 -A 2 | head -50Repository: oven-sh/bun
Length of output: 234
🏁 Script executed:
# See more of the implementation to understand all error paths
sed -n '1221,1300p' src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 4121
🏁 Script executed:
# Check how this function is called in other places, if at all
rg -n "scheduleTopLevelOmittedCleanup" src/Repository: oven-sh/bun
Length of output: 265
🏁 Script executed:
# Look for patterns of cleanup operations in PackageManager or PackageInstall to understand error handling style
rg -n "catch \|err\|" src/install/PackageInstall.zig | head -20Repository: oven-sh/bun
Length of output: 1850
Treat omit-cleanup as best-effort to avoid failing successful installs.
try will propagate allocation errors from within the cleanup operation and abort bun install even though the package installation succeeded. Since this is supplementary cleanup for unused packages, it should gracefully degrade on error rather than fail the command.
🔧 Suggested change
- try PackageInstall.scheduleTopLevelOmittedCleanup(manager);
+ PackageInstall.scheduleTopLevelOmittedCleanup(manager) catch |err| {
+ if (PackageManager.verbose_install) {
+ Output.debugWarn("omit cleanup skipped: {s}", .{`@errorName`(err)});
+ }
+ };🤖 Prompt for AI Agents
In `@src/cli/install_command.zig` around lines 85 - 91, The cleanup call
PackageInstall.scheduleTopLevelOmittedCleanup(manager) is currently invoked with
try which will abort the install on allocation/errors; change it to run as
best-effort by catching and handling any error instead of propagating it (e.g.,
replace the try with a catch/if-error pattern that logs or warns the error and
continues), keeping the surrounding condition that checks
manager.options.local_package_features so the cleanup remains optional and
non-fatal to a successful bun install.
| pub fn scheduleTopLevelOmittedCleanup(manager: *PackageManager) !void { | ||
| const std = @import("std"); | ||
| const bun = @import("bun"); | ||
| const FileSystem = bun.fs.FileSystem; | ||
| const String = bun.Semver.String; | ||
| const PackageNameHash = bun.install.PackageNameHash; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "PackageInstall.zig" 2>/dev/null | head -5Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Find the file in the repository
fd -t f "PackageInstall.zig" 2>/dev/null | head -1Repository: oven-sh/bun
Length of output: 85
🏁 Script executed:
# List files in src/install/
ls -la src/install/ | grep -i packageRepository: oven-sh/bun
Length of output: 486
🏁 Script executed:
# Check if the file exists and read the section in question
wc -l src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 90
🏁 Script executed:
# Read the specific section mentioned in the review
sed -n '1221,1227p' src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 352
🏁 Script executed:
# Get broader context around the function
sed -n '1215,1240p' src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 946
🏁 Script executed:
# Check the end of the file for imports
tail -50 src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 1651
🏁 Script executed:
# Look for `@import` statements in the file
rg -n "@import" src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 343
🏁 Script executed:
# Check for file-scope constants like PackageNameHash, FileSystem, String, etc.
rg -n "^const (std|bun|FileSystem|String|PackageNameHash)" src/install/PackageInstall.zig | head -20Repository: oven-sh/bun
Length of output: 262
🏁 Script executed:
# More comprehensive search for file-level constants
rg -n "^const\s+" src/install/PackageInstall.zig | head -30Repository: oven-sh/bun
Length of output: 1255
🏁 Script executed:
# Get the entire end of file (last 100 lines) to see import section
tail -100 src/install/PackageInstall.zig | cat -nRepository: oven-sh/bun
Length of output: 4795
🏁 Script executed:
# Check what const declarations exist near the imports at file end
tail -150 src/install/PackageInstall.zig | grep -n "const"Repository: oven-sh/bun
Length of output: 2160
🏁 Script executed:
# Directly read lines 1221-1227 with line numbers
cat -n src/install/PackageInstall.zig | sed -n '1221,1227p'Repository: oven-sh/bun
Length of output: 401
🏁 Script executed:
# Let's get a larger section and count lines properly
sed -n '1215,1300p' src/install/PackageInstall.zig | cat -nRepository: oven-sh/bun
Length of output: 4807
🏁 Script executed:
# Check the structure of imports at file end and beginning
echo "=== File start (first 50 lines) ===" && head -50 src/install/PackageInstall.zig && echo -e "\n=== File end (last 80 lines) ===" && tail -80 src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 4529
🏁 Script executed:
# Get file information and specific lines
echo "Total lines:" && wc -l src/install/PackageInstall.zig && echo -e "\n=== Lines 1-30 ===" && head -30 src/install/PackageInstall.zig && echo -e "\n=== Lines around 1221 ===" && sed -n '1215,1235p' src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 1603
🏁 Script executed:
# Simple check: look at the actual file content
head -40 src/install/PackageInstall.zig | tail -20Repository: oven-sh/bun
Length of output: 714
🏁 Script executed:
# Try reading with explicit line ranges
awk 'NR>=1 && NR<=30' src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 784
🏁 Script executed:
# Read the function at lines 1221-1235
awk 'NR>=1221 && NR<=1235' src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 693
🏁 Script executed:
# Get the very last lines of the file
awk 'END{print NR}' src/install/PackageInstall.zig && echo "---Last 60 lines---" && awk 'NR>END-60' src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 160
🏁 Script executed:
# Python to read file sections clearly
python3 << 'PYTHON_EOF'
with open("src/install/PackageInstall.zig", "r") as f:
lines = f.readlines()
print(f"Total lines: {len(lines)}\n")
print("=== First 40 lines ===")
for i, line in enumerate(lines[:40], 1):
print(f"{i:4d}: {line}", end="")
print("\n\n=== Lines 1221-1235 ===")
for i, line in enumerate(lines[1220:1235], 1221):
print(f"{i:4d}: {line}", end="")
print("\n\n=== Last 70 lines ===")
for i, line in enumerate(lines[-70:], len(lines)-70+1):
print(f"{i:4d}: {line}", end="")
PYTHON_EOFRepository: oven-sh/bun
Length of output: 5224
Remove inline imports and use file-scope constants.
The @import("std"), @import("bun"), and derived aliases at lines 1222–1226 duplicate declarations already available at file scope (lines 1654–1671). Inline @import violates the Zig style guideline.
Use the existing file-scope constants directly and add PackageNameHash to the file-scope import section:
Changes required
In the function, remove lines 1222–1226:
- const std = `@import`("std");
- const bun = `@import`("bun");
- const FileSystem = bun.fs.FileSystem;
- const String = bun.Semver.String;
- const PackageNameHash = bun.install.PackageNameHash;At the file-scope constants after line 1681, add:
const PackageNameHash = install.PackageNameHash;🤖 Prompt for AI Agents
In `@src/install/PackageInstall.zig` around lines 1221 - 1227, Remove the inline
imports inside scheduleTopLevelOmittedCleanup: delete the local consts for std,
bun, FileSystem, String, and PackageNameHash and instead reference the existing
file-scope constants; then add a file-scope alias const PackageNameHash =
install.PackageNameHash to the top-level import block so
scheduleTopLevelOmittedCleanup uses the shared PackageNameHash constant rather
than creating duplicates.
| var cwd = std.fs.cwd(); | ||
| var node_modules_dir = cwd.openDir("node_modules", .{}) catch return; | ||
| defer node_modules_dir.close(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the actual code at the specified lines
echo "=== Lines 1231-1233 ==="
sed -n '1225,1240p' src/install/PackageInstall.zig
echo -e "\n=== Lines 1272-1273 ==="
sed -n '1266,1280p' src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 1408
🏁 Script executed:
#!/bin/bash
# Search for all openDir calls with "node_modules" in the file
rg -n 'openDir\("node_modules"' src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 216
🏁 Script executed:
#!/bin/bash
# Check if FileSystem.instance.top_level_dir is available and used elsewhere
rg -n 'FileSystem\.instance\.top_level_dir' src/install/PackageInstall.zig | head -20Repository: oven-sh/bun
Length of output: 631
🏁 Script executed:
#!/bin/bash
# Verify bun.openDirA and bun.path.joinAbsString usage patterns in the codebase
echo "=== bun.openDirA usage examples ==="
rg -n 'bun\.openDirA' src/ -A 2 | head -30
echo -e "\n=== bun.path.joinAbsString usage examples ==="
rg -n 'bun\.path\.joinAbsString' src/ -A 2 | head -30Repository: oven-sh/bun
Length of output: 4512
🏁 Script executed:
#!/bin/bash
# Check the context around bun.openDirA calls to see if they truly use absolute paths
echo "=== Context for line 1157 (bun.openDirA usage) ==="
sed -n '1150,1165p' src/install/PackageInstall.zig
echo -e "\n=== Check what dirname is in these contexts ==="
sed -n '1145,1160p' src/install/PackageInstall.zig | grep -B5 "dirname"Repository: oven-sh/bun
Length of output: 1782
🏁 Script executed:
#!/bin/bash
# Check if there's a pattern showing bun.openDirAbsolute vs bun.openDirA
echo "=== Check bun.openDirAbsolute usage pattern ==="
rg -B3 'bun\.openDirAbsolute' src/install/ | head -40Repository: oven-sh/bun
Length of output: 1428
🏁 Script executed:
#!/bin/bash
# Verify: does top_level_dir actually contain "node_modules" subdirectory?
# Check how it's used in nearby lines
sed -n '1180,1295p' src/install/PackageInstall.zig | grep -A2 -B2 "top_level_dir"Repository: oven-sh/bun
Length of output: 815
Replace relative node_modules path with absolute path.
Opening "node_modules" relative to cwd violates the absolute-path requirement and can scan the wrong directory if cwd changes. Use FileSystem.instance.top_level_dir with bun.path.joinAbsString() and reuse it for both opens.
🔧 Suggested change
- var cwd = std.fs.cwd();
- var node_modules_dir = cwd.openDir("node_modules", .{}) catch return;
+ var cwd = std.fs.cwd();
+ const node_modules_path = bun.path.joinAbsString(FileSystem.instance.top_level_dir, &.{ "node_modules" }, .auto);
+ var node_modules_dir = bun.openDirA(cwd, node_modules_path) catch return;
- node_modules_dir = cwd.openDir("node_modules", .{}) catch return;
+ node_modules_dir = bun.openDirA(cwd, node_modules_path) catch return;Also applies to: 1272-1273
🤖 Prompt for AI Agents
In `@src/install/PackageInstall.zig` around lines 1231 - 1233, Replace the
relative openDir("node_modules") calls that use cwd with an absolute path built
from FileSystem.instance.top_level_dir using bun.path.joinAbsString(), e.g.,
construct a single absolute node_modules path string once and use that value for
both openDir/openFile calls (replace usages of cwd and variable node_modules_dir
with the new absolute path) so the code no longer depends on current working
directory; update the variables where node_modules_dir and subsequent opens are
created to use that joined absolute path.
| // Try open top-level node_modules; nothing to do if missing. | ||
| var cwd = std.fs.cwd(); | ||
| var node_modules_dir = cwd.openDir("node_modules", .{}) catch return; | ||
| defer node_modules_dir.close(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get the file size first to determine read strategy
wc -l src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 90
🏁 Script executed:
# Read context around line 1233 and lines 1271-1273
sed -n '1220,1250p' src/install/PackageInstall.zig | cat -nRepository: oven-sh/bun
Length of output: 1550
🏁 Script executed:
# Read context around lines 1271-1273
sed -n '1260,1280p' src/install/PackageInstall.zig | cat -nRepository: oven-sh/bun
Length of output: 1069
🏁 Script executed:
# Search for all node_modules_dir defer statements
rg -n "defer.*node_modules_dir" src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 144
🏁 Script executed:
# Get broader context to understand scope and variable lifecycle
sed -n '1200,1300p' src/install/PackageInstall.zig | cat -nRepository: oven-sh/bun
Length of output: 5418
Remove the first defer node_modules_dir.close() to prevent double-closing the reopened handle.
After the explicit node_modules_dir.close() at line 1258 and reassignment, the first defer (line 1220) remains active and will attempt to close the newly-opened handle at function exit, conflicting with the second defer (line 1260).
Suggested fix
// Try open top-level node_modules; nothing to do if missing.
var cwd = std.fs.cwd();
var node_modules_dir = cwd.openDir("node_modules", .{}) catch return;
- defer node_modules_dir.close();The same issue occurs at lines 1271–1273 where the second defer is added after reassignment.
📝 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.
| defer node_modules_dir.close(); |
🤖 Prompt for AI Agents
In `@src/install/PackageInstall.zig` at line 1233, Remove the early defer that
closes node_modules_dir so we don't double-close the handle after it is
explicitly closed and reopened; specifically, locate the variable
node_modules_dir in PackageInstall.zig and delete the first defer
node_modules_dir.close() that runs before the explicit node_modules_dir.close()
and reassignment, and likewise remove the earlier defer paired with the later
defer at the second occurrence (the block around lines 1271–1273) so only the
final close is deferred for the currently-open handle.
| // PASS 1: count candidates | ||
| var candidate_count: usize = 0; | ||
| var it = node_modules_dir.iterate(); | ||
| while (it.next() catch null) |entry| { | ||
| if (!(entry.kind == .directory or entry.kind == .sym_link)) continue; | ||
| const name = entry.name; | ||
| if (name.len == 0) continue; | ||
| if (name[0] == '.') continue; | ||
|
|
||
| if (name[0] == '@') { | ||
| // scoped packages: check immediate children only | ||
| var scope_dir = node_modules_dir.openDir(name) catch continue; | ||
| defer scope_dir.close(); | ||
| var sit = scope_dir.iterate(); | ||
| while (sit.next() catch null) |inner| { | ||
| if (!(inner.kind == .directory or inner.kind == .sym_link)) continue; | ||
| const buf = try std.fmt.allocPrint(bun.default_allocator, "{s}/{s}", .{ name, inner.name }); | ||
| const hash = String.Builder.stringHash(buf); | ||
| if (std.mem.indexOfScalar(PackageNameHash, name_hashes, hash) == null) candidate_count += 1; | ||
| bun.default_allocator.free(buf); | ||
| } | ||
| } else { | ||
| const hash = String.Builder.stringHash(name); | ||
| if (std.mem.indexOfScalar(PackageNameHash, name_hashes, hash) == null) candidate_count += 1; | ||
| } | ||
| } | ||
|
|
||
| if (candidate_count == 0) return; | ||
|
|
||
| // Increment pending tasks once for the batch | ||
| manager.incrementPendingTasks(@intCast(u32, candidate_count)); | ||
|
|
There was a problem hiding this comment.
Pending-task count can drift from scheduled deletions.
candidate_count is computed before rescan, then incrementPendingTasks is called even if the second open fails or the directory contents change. That can leave the pending counter permanently >0 and keep the installer alive.
Consider collecting candidate paths in pass 1 (ArrayList), incrementing by list.len, and scheduling from that list, or adjust the counter when pass 2 fails.
Also applies to: 1270-1273
🤖 Prompt for AI Agents
In `@src/install/PackageInstall.zig` around lines 1237 - 1268, The pending-task
counter can drift because candidate_count is computed before a second pass that
may fail or find different entries; change the code to collect actual candidate
package paths (e.g., into an ArrayList or SliceList) during the first pass
instead of only incrementing candidate_count, then call
manager.incrementPendingTasks with the collected list length (list.len) and
schedule deletions from that list in the second pass (using the same collected
entries when opening scope_dir or node entries); alternatively, if you prefer
minimal changes, ensure every failure in the second pass (e.g.,
scope_dir.openDir, scope_dir.iterate, node_modules_dir.openDir) decrements the
pending counter via manager.decrementPendingTasks so pending tasks cannot remain
>0 — update logic around candidate_count, manager.incrementPendingTasks, and the
second-pass iteration to use the collected list or perform symmetric decrement
on failures.
| const DeleteTask = struct { | ||
| absolute_path: []const u8, | ||
| task: jsc.WorkPoolTask = .{ .callback = &run }, | ||
|
|
||
| pub fn run(task_ptr: *jsc.WorkPoolTask) void { | ||
| var self: *@This() = @fieldParentPtr("task", task_ptr); | ||
| var debug_timer = bun.Output.DebugTimer.start(); | ||
| defer { | ||
| PackageManager.get().decrementPendingTasks(); | ||
| PackageManager.get().wake(); | ||
| } | ||
| defer self.deinit(); | ||
|
|
||
| const dirname = std.fs.path.dirname(self.absolute_path) orelse { | ||
| Output.debugWarn("Unexpectedly failed to get dirname of {s}", .{ self.absolute_path }); | ||
| return; | ||
| }; | ||
| const basename = std.fs.path.basename(self.absolute_path); | ||
|
|
||
| var dir = bun.openDirA(std.fs.cwd(), dirname) catch |err| { | ||
| if (comptime Environment.isDebug or Environment.enable_asan) { | ||
| Output.debugWarn("Failed to delete {s}: {s}", .{ self.absolute_path, @errorName(err) }); | ||
| } | ||
| return; | ||
| }; | ||
| defer bun.FD.fromStdDir(dir).close(); | ||
|
|
||
| dir.deleteTree(basename) catch |err| { | ||
| if (comptime Environment.isDebug or Environment.enable_asan) { | ||
| Output.debugWarn("Failed to delete {s} in {s}: {s}", .{ basename, dirname, @errorName(err) }); | ||
| } | ||
| }; | ||
|
|
||
| if (Environment.isDebug) { | ||
| _ = &debug_timer; | ||
| debug("deleteTree({s}, {s}) = {f}", .{ basename, dirname, debug_timer }); | ||
| } | ||
| } | ||
|
|
||
| pub fn deinit(this: *@This()) void { | ||
| bun.default_allocator.free(this.absolute_path); | ||
| bun.destroy(this); | ||
| } | ||
| }; | ||
|
|
||
| var t = try bun.default_allocator.create(DeleteTask); | ||
| t.* = DeleteTask{ .absolute_path = abs, .task = .{ .callback = &DeleteTask.run } }; | ||
| manager.thread_pool.schedule(bun.ThreadPool.Batch.from(&t.task)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
sed -n '1294,1341p' src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 2854
🏁 Script executed:
sed -n '1350,1397p' src/install/PackageInstall.zigRepository: oven-sh/bun
Length of output: 2690
🏁 Script executed:
rg -n "DeleteTask" src/install/PackageInstall.zig | head -20Repository: oven-sh/bun
Length of output: 548
🏁 Script executed:
rg -B 3 -A 1 '\.\{.*\.#' src --type zig | head -40Repository: oven-sh/bun
Length of output: 2558
🏁 Script executed:
rg '#\w+.*=' src --type zig -B 2 -A 2 | grep -A 5 'struct {' | head -50Repository: oven-sh/bun
Length of output: 1171
🏁 Script executed:
fd --type f --name "*.zig" -x rg -l '#\w+.*:.*=' {} \; | head -5Repository: oven-sh/bun
Length of output: 305
🏁 Script executed:
# Search for struct field initialization patterns with # prefix
rg '= \.\{' src --type zig -B 5 | grep -E '(#\w+|struct \{)' | head -30Repository: oven-sh/bun
Length of output: 1923
Apply # prefix for private fields and use decl literals in DeleteTask structs.
Both DeleteTask struct definitions (lines 1294-1341 and 1350-1397) are missing the # prefix on private fields and should use decl literals instead of named type literals for initialization. Update field declarations to use #absolute_path and #task, access them as self.#absolute_path and this.#absolute_path within methods, and initialize with .{ .#absolute_path = abs, .#task = ... } instead of DeleteTask{ ... }.
Suggested changes
-const DeleteTask = struct {
- absolute_path: []const u8,
- task: jsc.WorkPoolTask = .{ .callback = &run },
+const DeleteTask = struct {
+ `#absolute_path`: []const u8,
+ `#task`: jsc.WorkPoolTask = .{ .callback = &run },
pub fn run(task_ptr: *jsc.WorkPoolTask) void {
var self: *@This() = `@fieldParentPtr`("task", task_ptr);
- const dirname = std.fs.path.dirname(self.absolute_path) orelse {
- Output.debugWarn("Unexpectedly failed to get dirname of {s}", .{ self.absolute_path });
+ const dirname = std.fs.path.dirname(self.#absolute_path) orelse {
+ Output.debugWarn("Unexpectedly failed to get dirname of {s}", .{ self.#absolute_path });
...
}
pub fn deinit(this: *@This()) void {
- bun.default_allocator.free(this.absolute_path);
+ bun.default_allocator.free(this.#absolute_path);
bun.destroy(this);
}
};
var t = try bun.default_allocator.create(DeleteTask);
-t.* = DeleteTask{ .absolute_path = abs, .task = .{ .callback = &DeleteTask.run } };
+t.* = .{ .#absolute_path = abs, .#task = .{ .callback = &DeleteTask.run } };🤖 Prompt for AI Agents
In `@src/install/PackageInstall.zig` around lines 1294 - 1341, The DeleteTask
structs use public field names and named type literal initialization; change the
field declarations to private by prefixing with # (e.g., `#absolute_path`, `#task`),
update all internal accesses in methods run and deinit to use
self.#absolute_path and this.#absolute_path, and switch initializations from the
named-type literal (DeleteTask{ ... }) to a declaration literal with private
keys (.{ .#absolute_path = abs, .#task = .{ .callback = &DeleteTask.run } });
apply the same changes for the second DeleteTask instance as well and update the
allocator create/assignment sites that set t.* accordingly.
1 participant
Summary
Fix a bug where running
bun install --omit=dev(or--omit=optional/--omit=peer)would omit those dependency types from the new install plan but leave previously-installed
top-level package directories in
node_modules. This PR adds a safe, performant post-installcleanup that removes top-level entries that are no longer present in the final lockfile.
Motivation
Users expect
bun install --omit=...(or production installs) to produce a node_modulestree that reflects the requested dependency features. Previously Bun would stop installing
omitted types but would not remove stale directories that were installed by prior runs,
leading to confusing leftovers on disk and possible behavior mismatches.
What this PR does
scheduleTopLevelOmittedCleanup(manager: *PackageManager)insrc/install/PackageInstall.zig:node_modules:present in the final lockfile (using
name_hashmembership checks).installer thread pool.
name_hasharray for fast membership tests.src/cli/install_command.zigafter a successful install when anyof
local_package_features.{dev,optional,peer}is disabled (e.g. via--omitor production).performance is preserved.
Why this approach
--omit.Files changed
scheduleTopLevelOmittedCleanup(manager)with two-pass candidate discovery and background DeleteTask scheduling.Testing & verification
Manual test plan:
bun installto populatenode_modulesincluding devDependencies.bun install --omit=dev— verify that top-level dev package directories not present in the final lockfile are deleted.@scope/pkg) behavior — immediate children are considered and removed when omitted.node_modulesis missing.Automated:
Performance notes
node_modulesentries and immediate scoped children (no deep recursion).Backward compatibility & risk
--omitflags.