Honor absolute paths for heap and CPU profiler --*-dir/--*-name options - #32570
Honor absolute paths for heap and CPU profiler --*-dir/--*-name options#32570robobun wants to merge 4 commits into
Conversation
build_output_path appended config.dir onto a path already rooted at the current directory via AutoAbsPath::init_top_level_dir(). For a rooted absolute path, Path::append trims the input as relative and strips the leading separator, so an absolute --heap-prof-dir was resolved under CWD instead of at the given location. Use Path::join, which resets the accumulated path when a segment is absolute, matching the CPU profiler.
|
Updated 11:25 AM PT - Jun 21st, 2026
✅ @robobun, your commit 6a9a9a5f80e825fa05b80278efd8f38bb0ff457b passed in 🧪 To try this PR locally: bunx bun-pr 32570That installs a local version of the PR into your bun-32570 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Added |
|
Warning Review limit reached
More reviews will be available in 6 minutes and 26 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Walkthrough
ChangesAbsolute path handling in profiler output directories
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line append → join swap matching the CPU profiler, with a regression test.
Extended reasoning...
Overview
This PR fixes --heap-prof-dir to honor absolute paths. The change is a single method swap in src/jsc/BunHeapProfiler.rs (path.append(config.dir) → path.join(&[config.dir])) plus an explanatory comment, and a new regression test in test/cli/heap-prof.test.ts that asserts the snapshot lands in an absolute target directory rather than under CWD.
Security risks
None. The path being constructed is for a heap snapshot output file, derived from a CLI flag the user themselves supplies on their own machine. No injection, auth, or data-exposure surface is touched.
Level of scrutiny
Low. This is a one-line bug fix that brings the heap profiler into exact parity with the sibling CPU profiler — src/jsc/BunCPUProfiler.rs:185 already uses path.join(&[config.dir]) for the same purpose, so this is conforming to an established pattern rather than introducing new logic. Path::join resets the accumulated path on absolute segments while still resolving relative segments under CWD, which is the documented and desired semantics here.
Other factors
The new test mirrors the structure of the existing --heap-prof-dir tests (same harness helpers, spawn pattern, and assertion style) and additionally asserts nothing leaks under CWD. The bug-hunting system found no issues. No outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — one-line append → join to match the CPU profiler, with a regression test.
Extended reasoning...
Overview
This PR changes a single functional line in src/jsc/BunHeapProfiler.rs: path.append(config.dir)? becomes path.join(&[config.dir])?, so that an absolute --heap-prof-dir replaces the CWD-rooted base path instead of being stripped of its leading separator and appended as a relative segment. A clarifying comment explains the append vs join semantics. The accompanying test in test/cli/heap-prof.test.ts spawns bun from one temp dir with --heap-prof-dir pointing at a second absolute temp dir and asserts the snapshot lands in the target and not under CWD.
Security risks
None. The flag already lets users choose where to write a heap snapshot; this change just makes absolute paths behave as documented instead of being silently relativized. There is no privilege boundary, no injection surface, and no new I/O — the write path is unchanged.
Level of scrutiny
Low. This is a targeted one-line bugfix in a developer-facing profiling utility (not hot-path runtime code), and it brings BunHeapProfiler.rs into exact alignment with the existing, working pattern in BunCPUProfiler.rs:185 (path.join(&[config.dir])). The semantics of Path::join resetting on absolute segments are well-established in src/paths/Path.rs, and the PR description correctly diagnoses the debug_assert!(!is_input_absolute(input)) misuse in append.
Other factors
- The new test is well-constructed (separate CWD vs target dirs, positive assertion on target, negative recursive-glob assertion on CWD) and follows the same harness conventions as adjacent tests in the file.
- The one inline nit (absolute
--heap-prof-namestill hitsappend) is pre-existing, shared withBunCPUProfiler.rs:189, and out of scope for this fix — non-blocking. - The single CI failure (
next-pages/dev-server.test.tson macOS aarch64) is unrelated to heap profiling or path resolution. - No prior human review comments are outstanding.
The profiler filename was appended with Path::append, which trims an absolute input as relative (stripping the leading separator in release, tripping a debug_assert in debug) the same way the directory did. Use Path::join for the filename in both the heap and CPU profilers so an absolute --heap-prof-name / --cpu-prof-name is honored and both profilers stay consistent.
robobun
changed the title
|
Expanded the fix per the review: the same |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/jsc/BunCPUProfiler.rs`:
- Around line 181-192: The issue is that the retry error handling path only
creates the config.dir directory, but when config.dir is empty and the filename
is an absolute path with non-existent parent directories, those parent
directories are never created, causing ENOENT to remain unrecoverable. When
handling the retry logic after the path.join calls fail (in the error handling
path not shown in the diff), ensure that if config.dir is empty, extract and
create the parent directory path from the absolute filename instead of skipping
directory creation entirely. This way, whether the absolute path comes from
config.dir or from the filename parameter, the necessary parent directories will
be created before attempting to write the profile.
In `@test/cli/heap-prof.test.ts`:
- Around line 176-185: Move the exitCode assertion to the end of the test.
Currently the expect(exitCode).toBe(0) assertion appears before the filesystem
side-effect validations. Relocate this assertion to be the last check in the
test, after the cwdFiles validation and all other assertions, following the
coding guideline that exit code checks should come last to ensure more specific
assertions appear first for better error reporting.
In `@test/cli/run/cpu-prof.test.ts`:
- Around line 151-159: The test currently only validates the case where the
absolute target directory exists, which misses regressions in parent directory
creation. Modify the test to use a non-existent absolute parent directory for
the target path (e.g., create a path with a parent that doesn't exist yet), and
reorder the assertions so that the filesystem validation checks on
Bun.file(target).size and the cwdFiles array come before the final
expect(exitCode).toBe(0) assertion, since subprocess tests should assert
filesystem output first and exit code last.
🪄 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: 2eca6b29-599b-4e8a-ade9-02c70df89692
📒 Files selected for processing (4)
src/jsc/BunCPUProfiler.rssrc/jsc/BunHeapProfiler.rstest/cli/heap-prof.test.tstest/cli/run/cpu-prof.test.ts
The CPU profiler write-retry only created config.dir, so an absolute --cpu-prof-name pointing at a non-existent parent failed with ENOENT even after the join fix. Derive the directory from the final output path (like the heap profiler) so the parent is created before retrying. Strengthen the profiler-name tests to use a non-existent absolute parent and assert the exit code last.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/jsc/BunCPUProfiler.rs:192— The ENOENT retry path here still createsconfig.dir(and skips the retry entirely whenconfig.diris empty), but with this PR an absolute--cpu-prof-nameresets the output path viajoin, so the directory that actually needs to exist isdirname(output_path), notconfig.dir.bun --cpu-prof --cpu-prof-name /not/yet/foo.cpuprofilewill fail withWriteFailedinstead of creating/not/yet.BunHeapProfiler.rsalready does this right viaresolve_path::dirname(path_buf.slice())— worth mirroring that here while you're touching this file.Extended reasoning...
What
write_profile_to_fileinBunCPUProfiler.rshas an ENOENT/EPERM/EACCES retry path that creates the output directory before retrying the write. That retry is gated onif !config.dir.is_empty()and createsconfig.dirviaFd::cwd().make_path(config.dir). Before this PR that was fine:config.namewas effectively a bare filename appended underconfig.dir, soconfig.dirwas always the directory that needed creating.This PR changes
build_output_pathto callpath.join(&[filename])so that an absolute--cpu-prof-nameis honored —joinresets the accumulated path when the segment is absolute. After the reset, the actual output directory isdirname(--cpu-prof-name), which has nothing to do withconfig.dir. The retry path was not updated to match.Step-by-step
Take the case the PR explicitly intends to support — an absolute
--cpu-prof-namewith no--cpu-prof-dir:bun --cpu-prof --cpu-prof-name /not/yet/foo.cpuprofile script.js(where/not/yetdoes not exist).config.name = b"/not/yet/foo.cpuprofile",config.dir = b"".build_output_path:pathstarts at CWD,config.diris empty so the dir-join is skipped, thenpath.join(&[b"/not/yet/foo.cpuprofile"])resetspathto/not/yet/foo.cpuprofile.- First
write_file_os_path(Fd::cwd(), "/not/yet/foo.cpuprofile", ...)→ENOENT. - Retry branch:
errno == ENOENTmatches, butconfig.dir.is_empty()is true → falls into theelse { return Err(ProfilerError::WriteFailed); }arm. Nomake_pathis attempted for/not/yet.
A second variant makes the divergence more visible:
bun --cpu-prof --cpu-prof-dir relative/profiles --cpu-prof-name /abs/target/foo.cpuprofile script.js.build_output_path:path.join(&["relative/profiles"])→$CWD/relative/profiles, thenpath.join(&["/abs/target/foo.cpuprofile"])resets to/abs/target/foo.cpuprofile.- First write →
ENOENT. - Retry:
config.dir = "relative/profiles"is non-empty, soFd::cwd().make_path("relative/profiles")creates$CWD/relative/profiles— the wrong directory. - Retry write to
/abs/target/foo.cpuprofilestill hitsENOENT→WriteFailed, plus a stray empty directory left under CWD.
Why nothing prevents it
The retry logic predates this PR and assumed
config.dirwas always the parent of the output file. The PR breaks that invariant by letting--cpu-prof-namereset the path, but only updatedbuild_output_path, not the retry. The new test incpu-prof.test.tsdoesn't catch this because it pre-createstargetDirviatempDir("cpu-prof-name-abs-target", {}), so the first write succeeds and the retry path is never exercised.Sibling already does it right
BunHeapProfiler.rs(lines ~67–69) handles the identical situation by deriving the directory from the resolved output path:let dir_path = resolve_path::dirname::<bun_paths::platform::Auto>(path_buf.slice()); if !dir_path.is_empty() { let _ = Fd::cwd().make_path(dir_path); ... }
That works regardless of whether the final path came from
--heap-prof-dir,--heap-prof-name, or both.Impact
Edge case (absolute
--cpu-prof-namepointing into a not-yet-existing directory) and the failure is a cleanWriteFailedrather than a crash or silent corruption, so non-blocking. But the PR's stated goal is to make absolute names a supported input for both profilers, the sibling file already shows the correct pattern, and CLAUDE.md's "fix the whole class in the same PR" applies — the CPU profiler's retry should mirror the heap profiler'sdirname(path_buf.slice())approach.
|
@claude the CPU profiler retry is already handled in 6a9a9a5: |
There was a problem hiding this comment.
LGTM — the filename join and CPU-profiler retry-dirname follow-ups address everything raised; thanks.
Extended reasoning...
Overview
This PR fixes #32568: absolute paths passed to --heap-prof-dir, --heap-prof-name, and --cpu-prof-name were being treated as relative (leading separator stripped) because Path::append trims its input as relative when the accumulated path is already rooted. The fix swaps append for join (which resets on absolute segments) in build_output_path for both BunHeapProfiler.rs and BunCPUProfiler.rs, and aligns the CPU profiler's ENOENT-retry directory creation with the heap profiler's by deriving the parent via resolve_path::dirname on the final output path instead of only config.dir. Three new tests in heap-prof.test.ts and cpu-prof.test.ts cover absolute --heap-prof-dir, absolute --heap-prof-name (with non-existent parent), and absolute --cpu-prof-name (with non-existent parent).
Security risks
None. This affects only where the user's own profiler output file is written, to a path the user explicitly passed on the CLI. No new input parsing, no privilege boundaries crossed.
Level of scrutiny
Low–medium. The core change is a one-method swap (append → join) whose semantics are documented inline and already used for --cpu-prof-dir before this PR. The retry-dir change mirrors the existing heap-profiler code path verbatim. The non-Windows slice_z() re-derivation is a mechanical borrow-checker accommodation (slice_z takes &mut self, conflicting with the new immutable path_buf.slice() borrow for dirname).
Other factors
I left one inline comment on an earlier revision (the filename append had the same bug) — addressed in 66cadce. CodeRabbit raised the retry-dir gap and two test-ordering nits — all addressed in 6a9a9a5 and confirmed resolved. The bug-hunting system found no issues on the current revision. New tests follow the existing patterns in their files (same tempDir/Bun.spawn/glob-scan structure) and assert the regression directly (file lands at absolute target, nothing under CWD). No CODEOWNERS coverage for these paths.
…ir instead of panicking (#36881) ### What `--cpu-prof-dir`, `--cpu-prof-name`, `--heap-prof-dir`, or `--heap-prof-name` with a value whose length pushes the output path past `PATH_MAX` crashed the whole process at exit and lost the profile: ``` panic: range end index 4106 out of range for slice of length 4095 panic: index out of bounds: the len is 4096 but the index is 4096 oh no: Bun has crashed. This indicates a bug in Bun, not your code. ``` A value that is merely too long for the filesystem but still fits the buffer (e.g. 3000 bytes on Linux) already produced the `WriteFailed: Failed to write CPU profile` warning and exited cleanly; the panic only fired at the `PATH_MAX` boundary. ### Repro ```sh bun --cpu-prof --cpu-prof-dir="$(printf 'd%.0s' {1..4085})" -e 'const e=Date.now()+60; while(Date.now()<e);' ``` ### Cause `build_output_path` in both `src/jsc/BunCPUProfiler.rs` and `src/jsc/BunHeapProfiler.rs` writes the dir and filename into an `AutoAbsPath`, which is `CheckLength::ASSUME`: over-long input does not return `Err(MaxPathExceeded)` but panics inside `PooledBuf::append` slice indexing. The in-code `// Err arm is unreachable` annotation assumed the inputs were bounded, but both `config.dir` and `config.name` are raw CLI bytes. ### Fix Add a public `bun_paths::AutoAbsPathChecked` alias (`CheckLength::CheckForGreaterThanMaxPath`) and make `Path::join` honor `CheckLength` by routing through `join_abs_string_buf_checked` in that mode, so `from`/`append`/`append_fmt`/`join` all return `Err(MaxPathExceeded)` on overflow. `ASSUME`-mode callers (the default, and all existing users) are unchanged. Both profilers switch to `AutoAbsPathChecked` and propagate the error with `?`, dropping the `.expect("unreachable")` unwraps. The overflow surfaces as the existing `FilenameTooLong: Failed to write CPU profile` / `MaxPathExceeded: Failed to write heap profile` warning and the process exits cleanly. ### Verification New parameterized cases in `test/cli/run/cpu-prof.test.ts` and `test/cli/heap-prof.test.ts` pass 5000-byte `--*-prof-dir` / `--*-prof-name` values, plus a 2500+2500 combined case, and assert the warning text, `exitCode === 0`, and `signalCode === null`. They fail before (SIGABRT with the panic above) and pass after. Skipped on Windows, where the path buffer is ~98 KB and the `CreateProcess` command-line limit (~32 KB) prevents delivering an overflowing argument. The `--cpu-prof-dir=inner --cpu-prof-name=/abs/x` debug-assert case (absolute name appended onto a rooted path) is already covered by #32570, which switches `append` to `join` for the filename; this PR is only about the `PATH_MAX` overflow. <!-- robobun:evidence:begin --> --- **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/heap-prof.test.ts test/cli/run/cpu-prof.test.ts <!-- robobun:evidence:end -->
…ir instead of panicking (oven-sh#36881) ### What `--cpu-prof-dir`, `--cpu-prof-name`, `--heap-prof-dir`, or `--heap-prof-name` with a value whose length pushes the output path past `PATH_MAX` crashed the whole process at exit and lost the profile: ``` panic: range end index 4106 out of range for slice of length 4095 panic: index out of bounds: the len is 4096 but the index is 4096 oh no: Bun has crashed. This indicates a bug in Bun, not your code. ``` A value that is merely too long for the filesystem but still fits the buffer (e.g. 3000 bytes on Linux) already produced the `WriteFailed: Failed to write CPU profile` warning and exited cleanly; the panic only fired at the `PATH_MAX` boundary. ### Repro ```sh bun --cpu-prof --cpu-prof-dir="$(printf 'd%.0s' {1..4085})" -e 'const e=Date.now()+60; while(Date.now()<e);' ``` ### Cause `build_output_path` in both `src/jsc/BunCPUProfiler.rs` and `src/jsc/BunHeapProfiler.rs` writes the dir and filename into an `AutoAbsPath`, which is `CheckLength::ASSUME`: over-long input does not return `Err(MaxPathExceeded)` but panics inside `PooledBuf::append` slice indexing. The in-code `// Err arm is unreachable` annotation assumed the inputs were bounded, but both `config.dir` and `config.name` are raw CLI bytes. ### Fix Add a public `bun_paths::AutoAbsPathChecked` alias (`CheckLength::CheckForGreaterThanMaxPath`) and make `Path::join` honor `CheckLength` by routing through `join_abs_string_buf_checked` in that mode, so `from`/`append`/`append_fmt`/`join` all return `Err(MaxPathExceeded)` on overflow. `ASSUME`-mode callers (the default, and all existing users) are unchanged. Both profilers switch to `AutoAbsPathChecked` and propagate the error with `?`, dropping the `.expect("unreachable")` unwraps. The overflow surfaces as the existing `FilenameTooLong: Failed to write CPU profile` / `MaxPathExceeded: Failed to write heap profile` warning and the process exits cleanly. ### Verification New parameterized cases in `test/cli/run/cpu-prof.test.ts` and `test/cli/heap-prof.test.ts` pass 5000-byte `--*-prof-dir` / `--*-prof-name` values, plus a 2500+2500 combined case, and assert the warning text, `exitCode === 0`, and `signalCode === null`. They fail before (SIGABRT with the panic above) and pass after. Skipped on Windows, where the path buffer is ~98 KB and the `CreateProcess` command-line limit (~32 KB) prevents delivering an overflowing argument. The `--cpu-prof-dir=inner --cpu-prof-name=/abs/x` debug-assert case (absolute name appended onto a rooted path) is already covered by oven-sh#32570, which switches `append` to `join` for the filename; this PR is only about the `PATH_MAX` overflow. <!-- robobun:evidence:begin --> --- **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/heap-prof.test.ts test/cli/run/cpu-prof.test.ts <!-- robobun:evidence:end -->
|
Triage note after #32568 was closed. Checked against a debug build of main (165dc9f) and the 1.4.0 canary (da3851e): Three of the four changes here landed in #34660, which switched both profilers to The CPU profiler's retry still does The two heap tests from this PR also fail against main, but only because of unrelated drift since June: the default heap profile is now written with a Leaving this open; it should be rebased down to the |
1 participant
Fixes #32568
What
An absolute
--heap-prof-dirwas resolved relative to the current working directory: the leading separator was stripped and the snapshot landed under$CWDinstead of the given directory. The same bug affected--heap-prof-nameand the CPU profiler's--cpu-prof-name(an absolute name stripped its leading separator in release and tripped adebug_assertin debug builds).--cpu-prof-dirwas already handled correctly.Repro
Expected the file in
/tmp/abs-heap-target; it was written to$CWD/tmp/abs-heap-target.Cause
build_output_pathappended the directory and filename onto a path already rooted at CWD (AutoAbsPath::init_top_level_dir()):For a rooted absolute
Path,Path::appendtrims its input as relative and strips the leading separator (it evendebug_assert!(!is_input_absolute(input))s, so an absolute segment here is misuse).Path::joinresets the accumulated path when a segment is absolute, which is the behavior that was already used for--cpu-prof-dir.Fix
Use
Path::joininstead ofPath::appendfor both the directory and the filename, in bothsrc/jsc/BunHeapProfiler.rsandsrc/jsc/BunCPUProfiler.rs. Absolute--heap-prof-dir/--heap-prof-name/--cpu-prof-nameare now honored; relative values still resolve under CWD.Verification
test/cli/heap-prof.test.tsandtest/cli/run/cpu-prof.test.tsgain cases that run from one directory and write to a separate absolute--*-dir/--*-name, asserting the profile lands at the absolute target and nothing is written under CWD. They fail before this change (under a debug build the stripped-absoluteappendtrips thedebug_assertatsrc/paths/Path.rs:961) and pass after. The existing relative-path tests still pass.