Fix bun install hang with security scanner and many packages#28116
Fix bun install hang with security scanner and many packages#28116robobun wants to merge 3 commits into
Conversation
|
Updated 2:48 PM PT - Mar 14th, 2026
❌ @autofix-ci[bot], your commit bb32005 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 28116That installs a local version of the PR into your bun-28116 --bun |
|
✅ Verification complete — commit Gate 1 (CI): Build #39663 — all test jobs pass. Two non-blocking failures:
Gate 2 (Classification): Bug fix — test proof required. Gate 3 (Test proof): ✅
Gate 4 (Diff): ✅ No TODOs/FIXMEs. Fix matches root cause: Gate 5 (Bot convergence): ✅ All 9 review threads resolved (coderabbit nitpicks, all noise). Gate 6 (Hygiene): ✅ Branch follows Ready for human review. |
WalkthroughReplaces inline package JSON passing with a temporary-file mechanism: removes the Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/PackageManager/security_scanner.zig (1)
721-730:⚠️ Potential issue | 🟠 MajorEscape backslashes in Windows paths before inserting into TypeScript source.
On Windows,
joinAbsStringBufZ(..., .auto)returns a path with backslash separators (e.g.,C:\Users\Temp\bun-scanner-...). When inserted directly into the TypeScript string literal at line 725, the backslashes will cause invalid escape sequences like\Uand\Tin the resulting code:fs.readFileSync("C:\Users\Temp\...", "utf8") // ❌ Invalid escapesEither escape the backslashes (e.g.,
C:\\Users\\Temp\\...) or use forward slashes when building the path. Consider usingbun.path.joinAbsStringBufZ(..., .posix)to ensure forward slashes, or add escaping logic when inserting the path into the TypeScript source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/install/PackageManager/security_scanner.zig` around lines 721 - 730, The inserted JSON path (json_tmppath) can contain Windows backslashes which produce invalid TypeScript string escapes when appended into temp_source around packages_placeholder; update the insertion logic in the block that builds new_code (symbols: packages_placeholder, temp_source, new_code, json_tmppath) to ensure json_tmppath is safe for a TS string literal — either convert the path to posix-style slashes (use joinAbsStringBufZ(..., .posix) where the path is created) or escape backslashes in json_tmppath (replace "\" with "\\") before calling new_code.appendSlice(json_tmppath); keep the rest of the splice logic intact so the TS source receives a valid quoted path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/PackageManager/scanner-entry.ts`:
- Around line 1-4: Wrap the synchronous read/parse of "__PACKAGES_FILE__" in a
try-catch so failures produce a clear, specific error instead of a generic
throw; specifically, surround the
JSON.parse(fs.readFileSync("__PACKAGES_FILE__", "utf8")) assignment to packages
(and keep scannerModuleName untouched) with a try block, catch the error, call
the existing error exit/handler (e.g., writeAndExit or the same mechanism used
by the outer catch) with a descriptive message like "Failed to read/parse
packages file" plus the caught error, and ensure packages is only used if the
read/parse succeeded.
---
Outside diff comments:
In `@src/install/PackageManager/security_scanner.zig`:
- Around line 721-730: The inserted JSON path (json_tmppath) can contain Windows
backslashes which produce invalid TypeScript string escapes when appended into
temp_source around packages_placeholder; update the insertion logic in the block
that builds new_code (symbols: packages_placeholder, temp_source, new_code,
json_tmppath) to ensure json_tmppath is safe for a TS string literal — either
convert the path to posix-style slashes (use joinAbsStringBufZ(..., .posix)
where the path is created) or escape backslashes in json_tmppath (replace "\"
with "\\") before calling new_code.appendSlice(json_tmppath); keep the rest of
the splice logic intact so the TS source receives a valid quoted path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7195228b-c06b-4932-afa7-9e199ef72658
📥 Commits
Reviewing files that changed from the base of the PR and between 10bdb48 and c4034e108bc47c2f7b1969d639d305d44c17c737.
📒 Files selected for processing (4)
src/install/PackageManager/scanner-entry-globals.d.tssrc/install/PackageManager/scanner-entry.tssrc/install/PackageManager/security_scanner.zigtest/regression/issue/23607.test.ts
💤 Files with no reviewable changes (1)
- src/install/PackageManager/scanner-entry-globals.d.ts
When a project has >~790 packages and a security scanner is configured, `bun install` hangs indefinitely. The packages JSON was embedded inline in the `-e` code argument passed to the scanner subprocess. With many packages, this JSON exceeds the OS per-argument size limit (MAX_ARG_STRLEN = 128KB on Linux), causing posix_spawn to fail silently. Write the packages JSON to a temporary file instead and have scanner-entry.ts read from it via fs.readFileSync. The temp file is cleaned up after the scanner subprocess completes. Fixes #23607
c4034e1 to
511f0c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/PackageManager/security_scanner.zig`:
- Around line 680-683: The temp path generation using tmp_path_buf and tmp_path
(built with FileSystem.RealFS.tmpdirPath() and milliTimestamp()) can collide
across processes; modify the bufPrintZ format call that constructs tmp_path to
include the current process id (PID) alongside
`@bitCast`(std.time.milliTimestamp()) so the string becomes e.g.
"{s}/bun_scan_{x}_{d}" (or similar) and pass std.os.getpid() (or equivalent)
into the varargs, ensuring tmp_path remains checked for PathTooLong as before.
- Around line 680-683: The tmp_path value constructed using
FileSystem.RealFS.tmpdirPath() may contain Windows backslashes which break the
JavaScript string literal; modify the code that builds tmp_path (the
tmp_path_buf/tmp_path construction) to escape backslashes before injecting into
scanner-entry.ts by replacing each "\" with "\\", e.g., transform the
tmpdirPath() result to an escaped string (using std.mem.replace or a small loop)
and then use that escaped string in std.fmt.bufPrintZ so tmp_path contains
double-backslashes and is safe for the JS string literal.
In `@test/regression/issue/23607.test.ts`:
- Line 47: Replace the string-concatenated path in the fs.readFileSync call with
a cross-platform join using the path module: locate the expression that reads
fs.readFileSync(tmpDir + "/" + scanFile, "utf8") (using variables tmpDir and
scanFile) and change it to use path.join(tmpDir, scanFile); ensure the test file
imports or requires the path module (e.g., const path = require("path") or
import path from "path") so the join call is available.
- Around line 39-44: The test currently picks any file matching "bun_scan_*" in
tmpDir which may belong to another concurrent process; after locating scanFile
(tmpFiles.find(...)) open and read the file at path join(tmpDir, scanFile) and
assert its contents include the expected package name "is-number" (or another
deterministic marker produced by this install) so the test verifies the temp
file belongs to this run rather than any concurrent process.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59536a4e-a8b5-4b7f-90f1-dd92318025d5
📥 Commits
Reviewing files that changed from the base of the PR and between c4034e108bc47c2f7b1969d639d305d44c17c737 and ff91148fc00c81519a6bf840b36aff0571516ef1.
📒 Files selected for processing (4)
src/install/PackageManager/scanner-entry-globals.d.tssrc/install/PackageManager/scanner-entry.tssrc/install/PackageManager/security_scanner.zigtest/regression/issue/23607.test.ts
💤 Files with no reviewable changes (1)
- src/install/PackageManager/scanner-entry-globals.d.ts
- Normalize backslashes to forward slashes in tmp_path before embedding in JS string literal (prevents \U, \a, \T etc. being interpreted as escape sequences on Windows) - Use bun.sys.open/write/close instead of File.writeFile for cross-platform temp file creation (File.writeFile expects wide strings on Windows) - Remove accidental double-free of tmp_path_for_js on non-Windows (was freeing a stack buffer pointer, causing ASAN segfault) - Remove duplicate comment block
ff91148 to
194207b
Compare
Summary
bun installhanging indefinitely when a security scanner is configured and the project has >~790 packages-ecode argumentRoot Cause
The security scanner subprocess received package metadata embedded directly in the TypeScript code string passed via
bun -e "...". For large projects (>~790 packages), the JSON payload exceeds the OS per-argument size limit (MAX_ARG_STRLEN= 128KB on Linux), causingposix_spawnto fail silently and the parent process to wait indefinitely.Fix
security_scanner.zig: Writejson_datato a temp file in the system temp directory before spawning the subprocess. Replace the__PACKAGES_FILE__placeholder in the code template with the temp file path (a short string). Clean up the temp file in the defer block after the scanner completes.scanner-entry.ts: Read package data from the temp file path usingfs.readFileSyncinstead of using the inline__PACKAGES_JSON__placeholder.json_datafield fromSecurityScanSubprocesssince data is no longer passed via the struct.Verification
test/regression/issue/23607.test.tsthat verifies the scanner correctly receives packages via the temp file mechanismUSE_SYSTEM_BUN=1fails (old code doesn't have temp file reading),bun bd testpassesFixes #23607
Verification: CI green — all Linux, Windows, macOS, ASAN passed. Two darwin-14-aarch64 test failures verified as pre-existing flakes (both also fail on baked main binary):
dev-server.test.ts(puppeteer timeout) andsnapshot.test.ts(hangs).upload-benchmark.mjsinfra flake. Test proof holds: baked binary fails on regression test, PR binary passes. Diff clean. Bot threads resolved. Suggested reviewer: @dylan-conway.