test(tls): deduplicate and deflake the getPeerCertificate() RSS leak test - #34163
Conversation
…test The server-side getPeerCertificate() leak test exists in two places: the standalone node-tls-getpeercert-leak.test.ts added by #29881 and an identical copy appended to node-tls-cert.test.ts by #29916 the same day. The second copy runs after 30 other TLS tests in the same process and its 12MB RSS threshold has been flaking on the macOS tart runners (21.8MB observed in build 72915 on darwin-26-aarch64, 21.0MB on darwin-14-x64 in build 72900), while the standalone file passes on the same commit in its own process. Remove the duplicate from node-tls-cert.test.ts and leave a pointer to the standalone file. Bring the standalone file in line with the refinements the duplicate had picked up (skipIf(isDebug) so the ASAN quarantine cannot mask the signal), lengthen the warmup from 4 to 8 rounds so the allocator's high-water mark settles before the baseline is taken, lengthen the measured window to 15 rounds, and raise the release threshold from 12MB to 32MB (40MB under ASAN). The unpatched BIO/X509 leak would now produce ~60MB of growth over the measured window, so the regression the test guards against remains well above the threshold.
|
Updated 5:08 PM PT - Jul 14th, 2026
⏳ @robobun, your commit 81bb980 is still building in |
The two NODE_EXTRA_CA_CERTS negative tests spawned 3 and 2 debug-build child processes sequentially; at ~1.8s per debug+ASAN child the 3-spawn case exceeds the 5s default timeout. Spawn them concurrently and drain stderr alongside exited so the test stays under 2s on debug builds.
WalkthroughChangesThe TLS certificate tests now run selected TLS certificate and leak testing
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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/js/node/tls/node-tls-getpeercert-leak.test.ts`:
- Around line 21-27: Update the skip condition for the “server-side
getPeerCertificate() should not leak” test to gate specifically on the ASAN
build indicator rather than isDebug. Preserve execution in plain debug builds
while continuing to skip ASAN-instrumented debug builds whose quarantine
invalidates the RSS leak threshold.
🪄 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: c003e542-d59d-4a2e-8a2b-5291a308d2d1
📒 Files selected for processing (2)
test/js/node/tls/node-tls-cert.test.tstest/js/node/tls/node-tls-getpeercert-leak.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only deflake with well-justified threshold math.
What was reviewed:
- Duplicate leak test removal: the deleted copy in
node-tls-cert.test.tswas alreadyskipIf(isDebug), so porting that gate to the standalone file is not a net coverage loss (re: CodeRabbit's note). - Threshold retune: 32MB sits above the observed ~21MB macOS noise and well below the ~60MB+ the guarded leak would produce over 15×5k rounds.
- Concurrent NODE_EXTRA_CA_CERTS spawn refactor:
await using, concurrent stderr/exit drain, and stderr-before-exitCode ordering all match repo conventions.
Extended reasoning...
Overview
Test-only PR touching two files under test/js/node/tls/. It (1) removes a duplicated RSS leak test from node-tls-cert.test.ts in favor of the standalone node-tls-getpeercert-leak.test.ts, (2) retunes the standalone copy with skipIf(isDebug), longer warmup (4→8 rounds), longer measurement window (10→15 rounds), and a higher threshold (12MB→32MB, 40MB under ASAN), and (3) refactors two NODE_EXTRA_CA_CERTS fixture-spawn tests to run their subprocess variants concurrently.
Security risks
None. No production code is modified; the tests spawn local fixtures against a 127.0.0.1 TLS server on port 0 and read repo-local PEM fixtures.
Level of scrutiny
Low. This is a CI deflake of a memory-growth heuristic test. The key question is whether the raised threshold could mask the regression the test guards against, and the PR description does the math: the unpatched BIO/X509 leak is ~800 bytes/call, which over 75k measured iterations is ≥60MB — comfortably above the new 32MB bar, while the observed benign macOS allocator noise (~21MB) sits below it. The skipIf(isDebug) gate is not new behavior: it was already on the now-removed duplicate (via #31155) and is simply being ported to the surviving copy, so CodeRabbit's concern about lost debug coverage doesn't represent a regression from main.
Other factors
The concurrent-spawn refactor follows the repo's own test guidance: await using proc, Promise.all([stderr.text(), exited]) to avoid pipe deadlock, and asserting stderr content before exit code. Assertions are unchanged. The PR description includes 8/8 pass verification on the flaking macOS runner and both bun bd and USE_SYSTEM_BUN=1 runs. No prior human review comments to address.
|
CI status: the two changed files ( Remaining red is unrelated to this diff:
Ready for review. |
Matches the sibling tests in this file which use plain const proc; the map callback already awaits proc.exited so no disposal guard is needed.
There was a problem hiding this comment.
LGTM — test-only deflake with well-justified threshold math.
What was reviewed:
- Duplicate leak test removal: confirmed the standalone file retains identical coverage;
skipIf(isDebug)is carried forward from the removed copy (not a new coverage reduction). - Threshold retune: 32MB sits between the ~21MB observed macOS noise floor and the ~60MB leak signal at 75k calls, so the guarded regression stays detectable.
- Parallelized
NODE_EXTRA_CA_CERTSspawns: same assertions, now stderr-before-exitCode; concurrent clients against onetls.createServeris fine. - The
isDebugvsisASANgating question raised in-thread was checked and resolved — release ASAN lane still runs.
Extended reasoning...
Overview
Test-only change touching two files under test/js/node/tls/. It (1) deletes a duplicate copy of the getPeerCertificate() RSS leak test from node-tls-cert.test.ts (the same test already lives in node-tls-getpeercert-leak.test.ts), (2) retunes the surviving standalone copy with longer warmup (4→8 rounds), longer measurement window (10→15 rounds), skipIf(isDebug), and a raised threshold (12→32MB, 16→40MB under release ASAN), and (3) parallelizes two NODE_EXTRA_CA_CERTS subprocess loops via Promise.all while reordering assertions to stderr-before-exitCode. Unused isASAN/isDebug imports are dropped from the file that no longer needs them.
Security risks
None. No production code is touched. The changes are to test infrastructure only — RSS thresholds, warmup counts, and spawn concurrency in test files.
Level of scrutiny
Low-to-moderate. The main question for a deflake PR that raises a threshold is whether the test can still catch the regression it guards. The PR description does the arithmetic explicitly: the unpatched BIO/X509 leak is ~800 bytes/call, and the new 15-round × 5000-call window is 75k calls ≈ 60MB of leak growth, well above the new 32MB (40MB ASAN) threshold, which in turn is well above the ~21MB benign noise ceiling observed on the macOS runners. That margin is comfortable in both directions. The skipIf(isDebug) gate is not new — it was already applied to the now-removed duplicate in #31155 and is simply being carried to the surviving copy; the release ASAN lane (isDebug=false, isASAN=true) continues to run with its own threshold branch.
Other factors
- CI build 72937 shows both changed files passing on every lane, including the darwin-26-aarch64 and darwin-14-x64 shards where the flake was originally observed.
- The one inline review concern (CodeRabbit's
isASANvsisDebuggating suggestion) was answered in-thread and withdrawn; I independently agreeskipIf(isDebug)is correct here since gating onisASANwould drop the release ASAN lane. - The concurrent-spawn refactor is mechanical: each child still gets its own
Bun.spawn, stderr and exit are collected viaPromise.all, and the assertion loop preserves the sametoContain/toBechecks. Multiple TLS clients hitting onetls.createServerconcurrently is standard. - No CODEOWNERS cover these paths. The bug-hunting system found no issues.
There was a problem hiding this comment.
LGTM — test-only deflake with well-reasoned threshold math.
Checked that the raised 32MB/40MB threshold still sits below the ~60MB leak signal at the new 75k-call window, so the regression guard remains effective.
Verified the removed duplicate in node-tls-cert.test.ts was byte-equivalent coverage; skipIf(isDebug) is ported from the duplicate (not new coverage loss) and the release-ASAN lane still runs.
The concurrent NODE_EXTRA_CA_CERTS spawns share one server safely (each child is an independent tls.connect) and assertions are preserved with stderr now checked before exitCode.
Extended reasoning...
Overview
Test-only change touching two files under test/js/node/tls/. It (1) deletes a duplicate copy of the getPeerCertificate() RSS leak test from node-tls-cert.test.ts, (2) retunes the surviving copy in node-tls-getpeercert-leak.test.ts (8 warmup rounds, 15 measured rounds, 32MB/40MB threshold, skipIf(isDebug)), and (3) converts two sequential for-loop subprocess spawns in the NODE_EXTRA_CA_CERTS tests to Promise.all concurrent spawns. No runtime source is touched.
Security risks
None. Test code only; no changes to TLS implementation, cert validation, or any shipped code path.
Level of scrutiny
Low-to-moderate. The main risk with a leak-test threshold raise is silently defeating the regression guard. The PR description does the arithmetic: the original BIO/X509 leak is ~800 bytes/call, and 15 rounds × 5000 calls = 75k calls → ~60MB growth if the leak returns, comfortably above the new 32MB (release) / 40MB (release-ASAN) thresholds while clearing the ~21MB macOS allocator noise observed in builds 72900/72915. The skipIf(isDebug) gate is not new — it was already applied to the now-removed duplicate in #31155 and is being ported to the surviving copy; the CodeRabbit thread on isDebug vs isASAN was resolved with a correct explanation (release-ASAN lane must keep running, and it does). The concurrent-spawn refactor is mechanical: same env, same fixture, same assertions, now with stderr asserted before exit code per repo convention.
Other factors
- CI: author reports both changed files green across builds 72937 and 73004 including all macOS lanes where the flake originated; remaining red is pre-existing/unrelated.
- The
isDebug ? 2_500 : 5_000branch inperRoundis now dead (test skips on debug), so simplifying to5_000is correct. - Removed
isASAN/isDebugimports fromnode-tls-cert.test.tsmatch the deleted usage. - No prior human review comments outstanding; the one bot comment was withdrawn.
2 participants
Problem
test/js/node/tls/node-tls-cert.test.tswent red on:darwin: 26 aarch64in build 72915:The same assertion also flaked in
node-tls-getpeercert-leak.test.tson:darwin: 14 x64in build 72900 (21008384 bytes, passed on retry).Cause
The
server-side getPeerCertificate() should not leaktest exists twice: #29881 landed it as the standalonenode-tls-getpeercert-leak.test.ts, and #29916 (merged a few hours later) appended an identical copy tonode-tls-cert.test.ts. Both copies baseline RSS after four 5k-call warmup rounds and assert less than 12MB of growth afterwards.On the macOS tart runners the allocator's high-water mark can climb by ~21MB over that window with no leak present, while a probe on
darwin-test-arm64-3with the current canary shows RSS flat within ~1MB once it has had a couple more rounds to settle, and the full test file passes 8/8 on that box. The copy insidenode-tls-cert.test.tsis the worse of the two because it runs after 30 other TLS tests in the same process; in build 72915 the standalone file passed on the same commit in the same lane.There is no regression in the underlying
SSL_get_peer_certificate/computeRawpaths: the growth is the same for 5 and 10 measurement rounds, which rules out the per-call leak the test guards against.Fix
node-tls-cert.test.tsand leave a pointer to the standalone file.node-tls-getpeercert-leak.test.ts:skipIf(isDebug)(the refinement net,tls: port Node.js net/tls compatibility tests and fix the gaps they surface — half-open/reset/write semantics, server TLSSocket wrap, session/keylog, SNICallback/ALPNCallback, pfx, OpenSSL error shapes, addCACert, local binding (+305 tests) #31155 had applied only to the duplicate) so the ASAN quarantine cannot mask the signal on debug builds,The unpatched BIO/X509 leak is ~800 bytes per call; over the 75k calls now measured that is ~60MB of growth, so the regression the test exists to catch remains well above the new threshold, while the ~21MB noise ceiling observed on the macOS runners sits comfortably below it.
Verification
Ran the full
node-tls-cert.test.tseight times ondarwin-test-arm64-3(macOS 26) against canarycc0c1e835; 8/8 pass.[stamp-90s] gate passed · iteration 8 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 8
evidence per changed file