cli: honor --preserve-symlinks/-main for __filename, make SIGUSR1 inert by default - #35782
cli: honor --preserve-symlinks/-main for __filename, make SIGUSR1 inert by default#35782robobun wants to merge 8 commits into
Conversation
…rt, implement --heapsnapshot-signal Three Node.js CLI flags that bun accepted silently but did not honor: --preserve-symlinks / --preserve-symlinks-main: the flags were parsed and plumbed to the resolver, but finalize_result() unconditionally rewrote the resolved path to its realpath, so a required symlink still reported the realpath as __filename (Node.js reports the symlink path). Gate the file-level realpath in finalize_result on !preserve_symlinks. For -main, the fast entry-point path in maybe_open_with_bun_js called get_fd_path() (realpath) and the bun:main wrapper re-resolved the entry without the flag; both now honor preserve_symlinks_main. SIGUSR1: Node.js reserves SIGUSR1 for the debugger and never lets it terminate the process. bun left it at SIG_DFL so ops tooling that signals a node-compatible process would kill it. Install a no-op handler at startup (not SIG_IGN, so the disposition resets to SIG_DFL across exec() for children), and keep it installed when the last JS listener is removed. --heapsnapshot-signal=<SIG>: was swallowed entirely, so the configured signal killed the process instead of writing a heap snapshot. Declare the flag, route it through pre_execution to a process.on(sig) handler that calls v8.writeHeapSnapshot(), matching Node.js.
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesThe PR adds effective Main-entry symlink preservation
SIGUSR1 handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
None of the four suggested issues are directly closed by this PR:
Leaving them out of the PR description so they are not auto-closed. |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 11:05 PM PT - Jul 25th, 2026
❌ @robobun, your commit bb90f37 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35782That installs a local version of the PR into your bun-35782 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/internal/process/pre_execution.ts:341-346— Theprocess.platform !== "win32"gate skips bothvalidateSignalNameand theprocess.onregistration on Windows, but Node'sinitializeHeapSnapshotSignalHandlershas no platform gate — on Windows,node --heapsnapshot-signal=NOTREALstill fails withERR_UNKNOWN_SIGNAL, and valid Windows signals (SIGBREAK,SIGINT) get a working handler via libuv. Consider dropping the gate (Bun'sprocess.on(signal)already works on Windows viauv_signal_t), or at least movingvalidateSignalNameoutside it; the 'rejects an unknown signal name' test is insidedescribe.skipIf(isWindows)so this divergence is currently untested.Extended reasoning...
What the bug is
The new
--heapsnapshot-signalbootstrap block insrc/js/internal/process/pre_execution.ts:341-346is gated onprocess.platform !== "win32":if (heapsnapshotSignal !== null && process.platform !== "win32") { require("internal/validators").validateSignalName(heapsnapshotSignal); process.on(heapsnapshotSignal as NodeJS.Signals, () => { require("node:v8").writeHeapSnapshot(); }); }
On Windows this skips both the
validateSignalNamecall and theprocess.onregistration. Node.js'sinitializeHeapSnapshotSignalHandlersinlib/internal/process/pre_execution.jshas no such gate — it unconditionally callsvalidateSignalName(signal)andprocess.on(signal, ...)on every platform.Code path that triggers it
On Windows,
bun --heapsnapshot-signal=NOTREAL -e 0:Arguments.rsparses the flag intoexecArgv.VirtualMachine.rssees--heapsnapshot-signalinis_bootstrap_flagand loadspre_execution.ts.- The execArgv scan sets
heapsnapshotSignal = "NOTREAL". - The
process.platform !== "win32"check is false, so the block is skipped entirely. - The script runs with exit code 0.
Under Node.js on Windows, step 4 would instead call
validateSignalName("NOTREAL"), which throwsERR_UNKNOWN_SIGNAL, and the process exits nonzero before the entry script runs.Similarly,
bun --heapsnapshot-signal=SIGBREAK script.json Windows silently installs no handler, whereas Node registers a workingprocess.on('SIGBREAK', ...)via libuv (and Bun's ownprocess.on(signal)supports this on Windows viaBun__UVSignalHandle__init— seeBunProcess.cpp:1181).Why existing code doesn't prevent it
The test that asserts rejection of a bad signal name (
'rejects an unknown signal name'intest/cli/run/node-cli-flags.test.ts) is insidedescribe.skipIf(isWindows)("--heapsnapshot-signal", ...), so nothing exercises the Windows path.Impact
Windows-only Node-compat divergence on a diagnostic flag. In practice the primary use case (
SIGUSR1/SIGUSR2) doesn't exist on Windows, so the feature itself is niche there; the observable differences are (a) a bad flag value is silently accepted instead of erroring, and (b) the few Windows-deliverable signals (SIGBREAK,SIGINT,SIGHUP) don't get a snapshot handler. Nothing breaks if merged as-is — this is a strict improvement over the prior state where the flag did nothing on any platform.How to fix
Drop the platform gate to match Node exactly:
if (heapsnapshotSignal !== null) { require("internal/validators").validateSignalName(heapsnapshotSignal); process.on(heapsnapshotSignal as NodeJS.Signals, () => { require("node:v8").writeHeapSnapshot(); }); }
Bun's
process.on(signal)already routes throughuv_signal_ton Windows, so the handler registration will work for Windows-supported signals andvalidateSignalNamewill reject the rest. If there's a reason to keep the handler POSIX-only, at minimum movevalidateSignalName(heapsnapshotSignal)outside the platform check so the error-reporting matches Node. The'rejects an unknown signal name'test could then be lifted out of theskipIf(isWindows)block.Step-by-step proof
- On Windows, run
node --heapsnapshot-signal=NOTREAL -e 0. Node'sinitializeHeapSnapshotSignalHandlerscallsvalidateSignalName('NOTREAL')with no platform check → throwsTypeError [ERR_UNKNOWN_SIGNAL]: Unknown signal: NOTREAL, exit code 1. - On Windows, run
bun --heapsnapshot-signal=NOTREAL -e 0with this PR.heapsnapshotSignal = "NOTREAL", butprocess.platform === "win32"so theifat line 341 is false → no validation, no error, exit code 0. - Divergence: Node rejects, Bun accepts.
…sult, replace spawn-based SigIgn test with /proc/self/status check
robobun
changed the title
… does) test-require-symlink.js Worker case: with --preserve-symlinks but not --preserve-symlinks-main, the main entry is still realpath'd in Node.js. Move the preserve decision out of finalize_result (which cannot tell main from non-main) into _resolve, which picks .pretty (the pre-realpath spelling set_realpath stashed) when the applicable flag is set. Also extract preserve_symlinks_main_effective() for the three call sites.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/run/node-cli-flags.test.ts`:
- Around line 74-84: The existing `.cjs` test only covers direct file opening;
extend the relevant CLI test suite with separate symlink cases for extensionless
`bun run link` resolution and `NODE_PRESERVE_SYMLINKS_MAIN=1`. Use the
appropriate command/environment setup and assert each case preserves the
original link spelling, including successful exit and clean stderr.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4f39b24-070f-43c4-ab34-92435f90d581
📒 Files selected for processing (8)
src/bundler/transpiler.rssrc/jsc/VirtualMachine.rssrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/c-bindings.cppsrc/options_types/context.rssrc/resolver/options.rssrc/runtime/cli/run_command.rstest/cli/run/node-cli-flags.test.ts
…ty in slow-path entry resolve
|
CI status at bb90f37: every test lane that built is green for this diff (
|
…egative-control cases
…selection below is what honors -main)
There was a problem hiding this comment.
No new findings on bb90f37 — all four prior inline concerns were addressed. This touches the runtime module-resolve hot path (VirtualMachine::_resolve) and installs a process-wide SIGUSR1 handler at startup, so worth a human look before merging.
What was reviewed: the .pretty-vs-.text selection in _resolve and both entry paths (fast maybe_open_with_bun_js cwd-join and the bun run resolver fallback); SIGUSR1 install uses a real handler (not SIG_IGN) with SA_RESTART, and onDidChangeListeners restores to the no-op rather than SIG_DFL; the uncalled LoaderHooks::resolve sibling in jsc_hooks.rs — confirmed dead, author intentionally excluded; test hermeticity — NODE_PRESERVE_SYMLINKS{,_MAIN} now cleared for negative controls.
The one open inline comment is comment-cop lint on the 3-line explanatory comment at run_command.rs:2567-2569.
Extended reasoning...
Overview
Two Node-compat behaviors: (1) --preserve-symlinks / --preserve-symlinks-main now affect the path returned from module resolution (previously parsed but ignored — _resolve always returned the realpath'd .text), and (2) SIGUSR1 is made non-fatal by installing a no-op handler in bun_initialize_process. Eight files: the resolve tail in VirtualMachine::_resolve, three entry-point sites in run_command.rs, a new preserve_symlinks_main field on resolver BundleOptions (plus its projection and default), a shared preserve_symlinks_main_effective() helper, the SIGUSR1 handler in c-bindings.cpp / restore path in BunProcess.cpp, and a 9-case test file.
Security risks
None identified. The preserve-symlinks change only selects between two paths the resolver already produced (realpath .text vs. pre-realpath .pretty); it does not relax any containment check. The SIGUSR1 handler is a genuine no-op with SA_RESTART, not SIG_IGN, so exec()'d children revert to SIG_DFL — the SigIgn-mask test verifies this on Linux.
Level of scrutiny
High. VirtualMachine::_resolve is on every require()/import path, and returning .pretty instead of .text changes the module-cache key (intended under --preserve-symlinks, matching Node). bun_initialize_process runs unconditionally at startup, so the SIGUSR1 disposition change applies to every bun invocation including bun install, bun test, etc. Neither is a config tweak or mechanical refactor; both are user-visible behavioral changes to core paths.
Other factors
The PR went through five review iterations: my earlier 🔴 (bun run slow path still realpath'd) and three 🟡 nits (dead jsc_hooks.rs sibling, non-hermetic negative-control env, stale saved_preserve comment) were all addressed or explicitly declined with rationale. CodeRabbit's slow-path/env-var coverage request was addressed in ef71c79. The bug-hunting system found nothing on the current head. Test coverage is solid (flag on/off, entry vs. required, env-var form, bun vs. bun run, listener add/remove, SigIgn mask), and the author reports test-require-symlink.js stays green. The remaining open thread is automated comment-cop lint on a 3-line comment, not a substantive review. Given the surface area — module resolution identity + process-wide signal disposition — I'm deferring rather than approving.
2 participants
Two Node.js CLI flag behaviors bun accepted (echoed in
execArgv) without the effect.Reproduction
Cause
--preserve-symlinkswas parsed and stored onresolver.opts.preserve_symlinks, but the runtime module-resolve path always returnedresult_path.text(the realpath written byPath::set_realpathinfinalize_result), so a required symlink's__filenamewas always the realpath.--preserve-symlinks-mainhad two more gaps: the fast entry-point path (maybe_open_with_bun_js) derived the script path viaget_fd_path(fd), which is a realpath; and the syntheticbun:mainwrapper re-resolved the entry through_resolvewithout the flag applied.SIGUSR1 had no default handler, so the POSIX default action (terminate) applied. Node.js installs its debugger-activation handler at startup so the signal is never fatal.
Fix
VirtualMachine.rs_resolve:finalize_resultalready stashes the pre-realpath spelling in.prettywhen it rewrites.text. Return.prettywhenpreserve_symlinks(for required modules) orpreserve_symlinks_main(for the entry resolved frombun:main) is set. This keeps the main entry realpath'd under--preserve-symlinksalone, matching Node.js and keepingtest-require-symlink.jsgreen.run_command.rs: whenpreserve_symlinks_mainis set, resolve the entry target against cwd instead ofget_fd_path; propagate the flag toresolver.opts.preserve_symlinks_main. Shared flag+env precedence extracted toRuntimeOptions::preserve_symlinks_main_effective().c-bindings.cpp/BunProcess.cpp: install a no-op SIGUSR1 handler inbun_initialize_process(a handler rather thanSIG_IGN, so the disposition resets toSIG_DFLacrossexec()for children); restore to the no-op instead ofSIG_DFLwhen the last JS listener is removed.Verification
test/cli/run/node-cli-flags.test.ts(7 cases): required symlink__filenamewith and without the flag,--preserve-symlinksalone does not apply to the entry, entry__filenameunder-main, SIGUSR1 survives by default and after removing the last listener, SIGUSR1 is handled (notSIG_IGN) per/proc/self/statusSigIgn. 4/7 fail on stock bun and all pass with this change.test/js/node/test/parallel/test-require-symlink.jsalso passes.Related
process.reportmisalignment (excludeEnvreading"SIGUSR2", missingsignal/excludeNetwork) is covered by process: fix report.excludeEnv copy-paste, add signal/excludeNetwork, honor in getReport() #34400.--heapsnapshot-signalis covered by node:v8 profiling APIs, real perf_hooks nodeTiming, --diagnostic-dir and --heapsnapshot-signal (+11 tests) #35390 (along with--diagnostic-dirand v8 profiling).preserveSymlinks) and process: preserve the symlink path in process.argv[1] #35469 (process.argv[1]) touch adjacent resolver/entry code for different observables.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/node-cli-flags.test.ts