test: measure memory via harness rss() instead of process.memoryUsage.rss() - #36429
Conversation
….rss() On macOS, RSS includes MADV_FREE_REUSABLE pages the process has already released, so leak tests report the high-water mark rather than what is actually retained, and the deltas drift with allocator/GC timing. This makes RSS-delta leak tests flaky on darwin. Add an rss() export to test/harness.ts that uses Bun.unsafe.memoryFootprint() (phys_footprint) on darwin and falls back to process.memoryUsage.rss() elsewhere, and switch the leak tests over to it. Test files import it from harness; spawned fixtures and inline -e scripts define the same helper locally, with a typeof Bun guard where the fixture can also run under Node. No-Verification-Needed: test-only change
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical test-only sweep. The inline nit on setInterval-leak-fixture.js (missing typeof Bun guard) only affects manual Node runs on darwin, not CI, so it's non-blocking.
Extended reasoning...
Overview
This PR adds a single rss() export to test/harness.ts that returns Bun.unsafe.memoryFootprint() on darwin and process.memoryUsage.rss elsewhere, then applies it across ~100 leak-test files and fixtures. In-process tests import from harness; spawned fixtures and inline -e scripts get a local one-liner ternary; fixtures that also run under Node additionally guard on typeof Bun !== "undefined". No runtime/production code is touched.
Security risks
None. Test-only, no user-facing surface, no external inputs, no auth/crypto/permissions.
Level of scrutiny
Low per-file, moderate in aggregate for consistency. Each hunk is the same 1-4 line substitution; the risk is a missed rename shadowing the harness import or a fixture whose Node-compat guard was omitted. I spot-checked the non-trivial rename sites: sql.test.ts (rss→before, log key preserved), bun-serve-static-helpers.ts (rss→rssMB/finalRSS), structured-clone-fastpath.test.ts (rss→rss1), socketaddress.spec.ts (object→scalar, assertion updated to match), body-leak-test-fixture.ts (replaced the pre-existing ?? process.memoryUsage.rss() fallback with the standard ternary). All correct.
Other factors
- The one finding is a consistency nit:
setInterval-leak-fixture.jslacks thetypeof Bun !== "undefined"guard its two sibling timer fixtures received. CI only spawns it viabunExe(), so nothing breaks in the merge path. - Thresholds are unchanged everywhere — since
phys_footprint≤ RSS on darwin, existing upper-bound assertions remain valid or become slightly less flaky, which is the point. - The PR description explicitly excludes
test/js/node/test/parallel/**and theprocess.test.jsassertion on thememoryUsage.rss()API itself, which is the right scoping. - Already landed on main (commit
65848d23).
| const usage = | ||
| process.platform === "darwin" && typeof Bun.unsafe.memoryFootprint === "function" | ||
| ? Bun.unsafe.memoryFootprint | ||
| : process.memoryUsage.rss; |
There was a problem hiding this comment.
🟡 This fixture is written to be Node-compatible (it polyfills Promise.withResolvers and guards typeof Bun !== "undefined" in gc()), but the new usage const evaluates typeof Bun.unsafe.memoryFootprint without a typeof Bun !== "undefined" guard — under Node on darwin this throws ReferenceError: Bun is not defined at load. The two sibling timer fixtures updated in this PR (setinterval-cancel-fixture.js, setTimeout-clear-in-callback-leak-fixture.js) both received the guard; this one was missed. CI only spawns this via bunExe() so nothing breaks there — just a consistency nit.
Extended reasoning...
What
setInterval-leak-fixture.js carries explicit Node-compat scaffolding: it polyfills Promise.withResolvers (line 13) and its gc() helper checks typeof Bun !== "undefined" before calling Bun.gc (line 23). The intent is that the fixture can be run under Node for comparison. This PR replaces the old function usage() { return process.memoryUsage.rss(); } with:
const usage =
process.platform === "darwin" && typeof Bun.unsafe.memoryFootprint === "function"
? Bun.unsafe.memoryFootprint
: process.memoryUsage.rss;Unlike the sibling fixtures, there is no typeof Bun !== "undefined" && before typeof Bun.unsafe.memoryFootprint.
Why it fails under Node on darwin
typeof <bare identifier> is safe on an undeclared name, but typeof Bun.unsafe.memoryFootprint is typeof applied to a member expression — evaluating it must first resolve Bun, then read .unsafe, then read .memoryFootprint. Under Node, Bun is not declared, so the very first step throws ReferenceError: Bun is not defined.
Step-by-step on Node/darwin:
- Module loads; the
const usage = ...initializer runs at top level. process.platform === "darwin"→true, so&&proceeds to the right operand.- Right operand is
typeof Bun.unsafe.memoryFootprint. To compute it, the engine evaluates the MemberExpressionBun.unsafe.memoryFootprint, starting with the referenceBun. Bunis an unresolvable reference in Node →ReferenceError: Bun is not definedis thrown beforetypeofever runs.- The fixture aborts at load time; nothing after line 8 executes.
On non-darwin Node the process.platform === "darwin" check is false and && short-circuits, so the fixture still works there.
Why existing code doesn't prevent it
The file's other Node guards (gc() at line 23, globalThis.Bun at line 88) run after line 8, so they never get a chance. Nothing above line 8 defines or guards Bun.
Missed sibling
The PR description's own table says: "Fixtures that also run under Node — Same, plus typeof Bun !== "undefined" guard." Both other timer fixtures touched in this PR received exactly that:
setinterval-cancel-fixture.jsline 5-8:process.platform === "darwin" && typeof Bun !== "undefined" && typeof Bun.unsafe.memoryFootprint === "function" ? ...setTimeout-clear-in-callback-leak-fixture.jsline 15-18: same pattern.
Per REVIEW.md "Fix the whole class in the same PR — grep for every sibling site sharing the pattern", this is the one sibling that was missed.
Impact
CI is unaffected: setInterval.test.js only spawns this fixture via bunExe(), where Bun is always defined. The only breakage is manual node setInterval-leak-fixture.js runs on macOS, which worked before this PR. Hence nit severity — nothing in the merge-blocking path breaks.
Fix
Add the same guard used in the sibling fixtures:
const usage =
process.platform === "darwin" && typeof Bun !== "undefined" && typeof Bun.unsafe.memoryFootprint === "function"
? Bun.unsafe.memoryFootprint
: process.memoryUsage.rss;
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 51 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 (103)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ... # Conflicts: # src/jsc/bindings/BunDebugger.cpp
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ...
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ... # Conflicts: # src/js/internal/debugger.ts
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ...
1 participant
What
RSS on macOS keeps freed-but-lazily-unmapped (
MADV_FREE_REUSABLE) pages, so leak tests that diffprocess.memoryUsage.rss()measure a high-water mark and flake. This switches every RSS-based leak test toBun.unsafe.memoryFootprint()(phys_footprint) on darwin.test/harness.tsrss()export:memoryFootprinton darwin,process.memoryUsage.rsselsewhere*.test.ts/js(in-process)import { rss } from "harness"; shadowingrsslocals renamed-escriptsconst rss = process.platform === "darwin" && typeof Bun.unsafe.memoryFootprint === "function" ? … : process.memoryUsage.rsstypeof Bun !== "undefined"guardUntouched on purpose:
test/js/node/test/parallel/**(upstream Node tests) andprocess.test.js's assertion on thememoryUsage.rss()API itself.Verify
Ran the affected files with the system Bun (1.4.0, which already has
memoryFootprint) on darwin — all pass.