test(tls): compare per-round peak RSS in the getPeerCertificate leak test - #35322
test(tls): compare per-round peak RSS in the getPeerCertificate leak test#35322robobun wants to merge 2 commits into
Conversation
…test Since #34181 mimalloc returns freed pages to the OS on a background scavenger thread, so a single post-GC process.memoryUsage.rss() sample can land in a transient scavenger dip. When the baseline sample catches such a dip and the after sample does not, the test reads 30+ MB of spurious growth (34.6 MB on debian 13 x64 in build 78939, 34.5 MB in 78653, 34.7 MB on Windows 11 aarch64 in 76807) against the 32 MB threshold #34163 set the day before #34181 merged. Sample RSS after every round and compare the peak over the measurement phase to the peak over the warmup phase, as #34012 did for spawn-pipe-leak. The scavenger can only lower RSS, so the peak over N rounds is immune to its timing; a real per-call leak still raises the peak every round. Peak growth sits within +/-3 MB across 64 local runs on debian 13 x64 (release canary, plain / CI env / 6x concurrent) and a simulated ~800-byte/call leak reads +100 MB, so the 32/40 MB threshold is unchanged.
|
Warning Review limit reached
Next review available in: 7 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 (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only de-flake that switches from single-sample RSS to per-round peak RSS.
What was reviewed:
- The workload (8+15 rounds × 5000 iters × 2 calls) and 32/40 MB thresholds are unchanged, so the assertion still guards the same invariant.
- Verified the test still fails on the leak it protects — PR description shows a simulated ~800 B/call leak drives peak growth to +100 MB, well over the threshold.
- Peak-over-N is sound against the #34181 scavenger behavior: the background thread can only lower RSS, so a dip in the baseline sample is what caused the false positive; peaks are immune.
Extended reasoning...
Overview
This PR touches a single test file, test/js/node/tls/node-tls-getpeercert-leak.test.ts. It changes how RSS is sampled in the leak test: instead of taking one process.memoryUsage.rss() reading after warmup and one after the measurement phase, it now tracks the peak RSS across each round in both phases and compares those peaks. The workload, round counts, per-round iteration count, GC calls, and the 32 MB (40 MB ASAN) threshold are all unchanged.
Security risks
None. This is a test-only change with no production code, no new inputs, and no network/filesystem surface beyond what the test already used.
Level of scrutiny
Low. This is a de-flake of a leak regression test, following the same pattern already applied in #34012 to spawn-pipe-leak for the same underlying cause (#34181 moving mimalloc page return off-thread). The key REVIEW.md concern for de-flakes — "keep asserting the property the original assertion protected" — is satisfied: the PR description includes a verification table showing that a simulated ~800 B/call leak still fails the test at +100 MB, well above the unchanged 32 MB threshold, while 20+ non-leak runs sit within ±3 MB. The test is not being weakened; only the sampling methodology is being made robust to background scavenger timing.
Other factors
- The reasoning is well-documented both in the PR description and in the updated code comment, explaining why peak-over-N is immune to scavenger dips (the scavenger can only lower RSS, so peaks are unaffected).
- The change is mechanically simple: the
spinhelper is renamed tospinRounds, absorbs the outer round loop, and returnsMath.maxof RSS samples. No new dependencies, no timing/sleep logic added. - The bug hunting system found no issues.
- No prior reviewer comments to address.
|
Updated 7:12 PM PT - Jul 23rd, 2026
❌ @robobun, your commit eeee96d has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35322That installs a local version of the PR into your bun-35322 --bun |
Matches spawn-pipe-leak.test.ts and the other leak tests so the next CI failure shows the absolute peaks, not just the delta.
There was a problem hiding this comment.
LGTM — test-only de-flake that swaps single-sample RSS for peak-over-rounds, following the same pattern as #34012.
What was reviewed:
- Confirmed the workload (8+15 rounds × 5000 iters) and 32/40 MB threshold are unchanged; only the sampling strategy moved from two point-in-time reads to per-phase peaks.
- Checked the assertion still protects the original invariant: the simulated-leak verification in the description shows +100 MB peak growth, well above the 32 MB bound, so a real
SSL_get_peer_certificate/computeRawregression would still fail. - The added
console.logof peak readings before the assertion is useful for future flake triage and matches how other leak tests report.
Extended reasoning...
Overview
This PR touches a single test file, test/js/node/tls/node-tls-getpeercert-leak.test.ts, to fix a flake on the debian 13 x64 and windows aarch64 CI lanes. The change replaces two single-point RSS samples (one after warmup, one after measurement) with per-round peak-RSS tracking across each phase, then compares peak-to-peak. The workload — 8 warmup rounds + 15 measurement rounds, each 5000 iterations of two getPeerCertificate() calls plus double Bun.gc(true) — is byte-for-byte identical to before, as is the 32 MB (40 MB under ASAN) growth threshold. A diagnostic console.log of the warmup/measured peaks and delta was added ahead of the assertion.
Security risks
None. This is a test-only change with no production code, no new inputs, and no changes to TLS setup or certificate handling. The fixture certs and server/client configuration are untouched.
Level of scrutiny
Low. Test-only de-flake following an already-landed pattern (#34012 for spawn-pipe-leak). The key REVIEW.md concern for de-flakes — "keep asserting the property the original assertion protected" — is satisfied: the PR description shows a simulated ~800 B/call leak drives peak growth to +100-104 MB and fails all 3 runs, while the fixed build stays within ±3 MB across 20+ runs. Since the mimalloc scavenger can only lower RSS, taking the peak over N rounds is strictly less noisy than a single sample without loosening the bound against real monotonic leaks.
Other factors
The threshold still branches on isASAN per the leak-test guidance, the skipIf(isDebug) guard is preserved, and the 180s per-test timeout is unchanged. No prior human or bot review comments to address. The change is small, mechanical, and well-verified (64 passing runs across plain/CI-env/concurrent scenarios plus a negative check that the leak still trips it).
|
CI status on eeee96d (build 79015): The remaining red is unrelated to this diff (which only touches the one TLS test file):
This is ready to merge. |
1 participant
Fixes
test/js/node/tls/node-tls-getpeercert-leak.test.tsgoing red on the:debian: 13 x64lane in build 78939 and build 78653, and flaking on:windows: 11 aarch64in build 76807.Failure
Cause
#34163 set the release threshold at 32 MB on 2026-07-15. #34181 merged the next day and moved mimalloc's freed-page return to a background scavenger thread:
Bun.gc(true)no longer synchronously hands pages back to the OS, so a singleprocess.memoryUsage.rss()sample taken right after GC can land in a transient scavenger dip. When the baseline sample catches such a dip and the end-of-run sample does not, the test reads 30+ MB of spurious "growth".On a local debian 13 x64 release canary the per-round RSS trace shows the dips directly (typical round ~78 MB, occasional dips to ~57 MB), and the test's single-sample measurement ranges from -3 MB to +19.7 MB across ten runs. Against the Jul 2 build that predates #34181 the same probe sits within ±4 MB.
There is no regression in the
SSL_get_peer_certificate/computeRawpaths this test guards: RSS is flat round-to-round once the scavenger dips are ignored.Fix
Sample RSS after every round and compare the peak over the 15 measurement rounds to the peak over the 8 warmup rounds, the same pattern #34012 applied to
spawn-pipe-leakfor the same reason. The scavenger can only lower RSS, so the peak over N rounds is unaffected by when it fires; a real per-call leak still raises the peak every round. The workload (8+15 rounds × 5000 iterations × twogetPeerCertificatecalls) and the 32/40 MB threshold are unchanged.Verification
Local debian 13 x64, release canary
892b1dabc:BUN_GARBAGE_COLLECTOR_LEVEL=1etc.)raw)bun bd testskips (as before, the test isskipIf(isDebug)).Culprit: #34181 (off-thread page return changed the RSS-sampling contract #34163 relied on).
[stamp-90s] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file