Repository navigation
test(install): drop stale kB-to-bytes scaling of resourceUsage().maxRSS - #36586
Conversation
Subprocess.resourceUsage().maxRSS has been normalised to bytes on every platform since f68e504 (#36087). The test added in b4aa3a0 (#36541) was authored before that merged and still multiplied by 1024 on Linux/Windows, so the assertion compared a value 1024x too large and failed on every non-macOS lane. Also tighten the lower bound from > 0 to > 1 MiB so a regression back to kilobytes trips the lower bound instead of vacuously passing the upper.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe streaming extraction test removes an unused ChangesStreaming extraction test
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — straightforward test-only fix for the CI failure on non-macOS lanes.
What was reviewed:
- Verified
RusageFields::maxrssinsrc/spawn_sys/spawn_process.rsnormalises to bytes on all platforms (×1024 on non-Apple Unix,PeakWorkingSetSizebytes on Windows), so the dropped scaling is correct. - The new
> 1 MiBlower-bound unit guard mirrors the existing pattern intest/harness.tsrunFixtureMaxRSS. isMacOSimport removal — no other uses in the file.
Extended reasoning...
Overview
This PR removes a stale per-platform * (isMacOS ? 1 : 1024) scaling of Subprocess.resourceUsage().maxRSS in one test (buffered extract does not hold the decompressed local tarball in memory) in test/cli/install/bun-install-streaming-extract.test.ts. The scaling was added in #36541 assuming raw ru_maxrss semantics, but #36087 had already landed hours earlier normalising maxRSS to bytes on every platform. The double-scaling made the received value ~325 GB on Linux/Windows, failing the < 512 MB upper bound. The fix drops the scaling, removes the now-unused isMacOS import, tightens the lower bound from > 0 to > 1 MiB as a unit guard, and updates the comment.
Security risks
None. Test-only change to an RSS assertion; no src/ changes, no untrusted-input handling.
Level of scrutiny
Low. This is a mechanical fix to a CI-red test with a well-documented root cause. I verified the underlying claim directly: RusageFields::maxrss() in src/spawn_sys/spawn_process.rs:200 multiplies by 1024 on non-Apple Unix and returns PeakWorkingSetSize (bytes) on Windows via WinRusage, so maxRSS is indeed bytes everywhere and the test-side scaling was double-counting. Dividing the CI failure value (333065486336) by 1024 gives ~325 MB, well under the 512 MB bound and consistent with the ASAN baseline the comment cites.
Other factors
- The
> 1 MiBunit guard is copied verbatim from the establishedrunFixtureMaxRSShelper intest/harness.ts:332, so it follows an existing convention rather than inventing a new threshold. - The upper bound (
< 2 * PAYLOAD_SIZE= 512 MB) is unchanged, so the regression the test guards against (~780 MB release / ~1 GB ASAN with the old pre-decompress path) still fails it. - No prior reviews or outstanding comments on the PR timeline.
- PR states
bun bd testpasses locally with 10/10.
Fixes
test/cli/install/bun-install-streaming-extract.test.tsred on main (every non-macOS lane, build 86466).Failure
Cause
#36541 (b4aa3a0) added
buffered extract does not hold the decompressed local tarball in memory, which scaledSubprocess.resourceUsage().maxRSSby 1024 on Linux/Windows under the assumption that it reports kilobytes there (rawru_maxrsssemantics). That assumption was already stale when it merged: #36087 (f68e504, merged ~11h earlier the same day) had normalisedresourceUsage().maxRSSto bytes on every platform (RusageFields::maxrssmultiplies by 1024 on non-Apple Unix; Windows storesPeakWorkingSetSizeundivided). #36541 was branched before #36087 landed and was not rebased over it.Dividing the CI values by the extra 1024 gives ~80-90 MB on release lanes and ~325 MB on the x64-asan lane, well under the 512 MB bound and consistent with the comment's stated baselines.
Fix
Drop the per-platform scaling:
resourceUsage().maxRSSis already bytes everywhere. Tighten the existing> 0lower bound to> 1 MiBso a future regression back to kilobytes trips that bound instead of making the upper bound vacuously pass (matches the unit guard intest/harness.ts'srunFixtureMaxRSS).The test's regression guard is unchanged: the old pre-decompress path that #36541 removed would still produce ~780 MB release / ~1 GB debug+ASAN peak RSS and fail the
< 512 MBupper bound.Test-only change; no src/ diff for fail-before.
Verification