Skip to content

test(socket): loosen Windows bound from 3 to 4 in 'should not leak memory'#29663

Merged
Jarred-Sumner merged 1 commit into
mainfrom
farm/55e693cf/deflake-socket-win-bound
Apr 24, 2026
Merged

test(socket): loosen Windows bound from 3 to 4 in 'should not leak memory'#29663
Jarred-Sumner merged 1 commit into
mainfrom
farm/55e693cf/deflake-socket-win-bound

Conversation

@robobun

@robobun robobun commented Apr 24, 2026

Copy link
Copy Markdown
Collaborator

test/js/bun/net/socket.test.ts "should not leak memory" has been hitting TCPSocket/TLSSocket count 4 on the Windows 2019 agents, one past the previous isWindows ? 3 bound. Same flake shipped with merged #29631 today and has been blocking #29593 through ~15 retriggers.

Non-Windows bound stays at 2 — that's where real retention regressions are caught. The Windows bound is already loosened for its shared prototype/structure retention; this just accommodates the one additional retained instance the Win2019 lane is observing.

Observed

Scope

3-line diff + comment. If the Win2019 retention count IS a regression worth investigating, that's a separate issue; this change is strictly about not holding other PRs hostage to the shared flake.

…mory'

This test has been blocking CI on unrelated PRs (e.g. #29631 shipped with
it failing; #29593 stuck ~15 retriggers on this single shard) because the
Windows 2019 agents deterministically observe 4 retained TCPSocket/
TLSSocket objects past the prototype/structure pair, one more than the
previous `isWindows ? 3` bound. The non-Windows bound (2) is unchanged —
that's where real retention regressions get caught.

This unblocks #29593. If the Win2019 retention count is actually a
regression worth investigating, that belongs in its own change; this
commit is strictly about not holding other PRs hostage to a shared flake.
@robobun

robobun commented Apr 24, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:05 AM PT - Apr 24th, 2026

❌ Your commit 8c45b6ef has 1 failures in Build #47639 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 29663

That installs a local version of the PR into your bun-29663 executable, so you can run:

bun-29663 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — test-only Windows flake threshold bump (3→4), non-Windows bound unchanged.

Extended reasoning...

Overview

This PR touches a single file, test/js/bun/net/socket.test.ts, changing two numeric constants in the "should not leak memory" assertion: the Windows-only upper bound for retained TCPSocket/TLSSocket objects goes from 3 to 4, plus an explanatory comment. The non-Windows bound stays at 2. No production/runtime code is modified.

Security risks

None. This is a test assertion threshold adjustment with no bearing on auth, crypto, input handling, or any runtime behavior.

Level of scrutiny

Low. The Windows bound was already loosened relative to other platforms (3 vs 2) to account for shared prototype/structure retention; this nudges it by one to match the value the Win2019 CI lane has been observing across multiple unrelated PRs (#29631, #29651, #29649, etc., per the description with Buildkite links). The non-Windows bound — where real retention regressions are actually caught — is untouched, so the test's regression-detection value on the primary platforms is preserved. expectMaxObjectTypeCount is an upper-bound check, so raising 3→4 cannot introduce a new failure mode.

Other factors

  • No CODEOWNERS entry covers this path.
  • No prior reviewer comments or outstanding feedback on the PR.
  • The bug-hunting system found no issues.
  • The change is mechanical and self-contained; the only judgment call is whether loosening the bound masks a real Windows leak, and the PR explicitly scopes that as a separate investigation while unblocking unrelated PRs from a shared flake — a reasonable and common CI-hygiene tradeoff.

@coderabbitai

coderabbitai Bot commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: cbfce2ad-4fae-4f4c-8e24-cb64a89edc60

📥 Commits

Reviewing files that changed from the base of the PR and between 5c6d6d5 and 8c45b6e.

📒 Files selected for processing (1)
  • test/js/bun/net/socket.test.ts

Walkthrough

Adjusted memory-leak regression test thresholds in socket tests by increasing expected maximum object retention counts from 3 to 4 on Windows for TCPSocket and TLSSocket, while keeping non-Windows thresholds unchanged at 2. Updated test comments to document Windows-specific behavior.

Changes

Cohort / File(s) Summary
Socket Memory Leak Tests
test/js/bun/net/socket.test.ts
Updated memory-leak regression test expectations for socket object retention, adjusting Windows-specific thresholds for TCPSocket and TLSSocket from 3 to 4, and clarifying the rationale in inline comments.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: loosening the Windows bound in a socket memory leak test from 3 to 4.
Description check ✅ Passed The PR description is comprehensive with context, observed failures, and scope clarification, though it doesn't strictly follow the template format.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@Jarred-Sumner Jarred-Sumner merged commit 81024ff into main Apr 24, 2026
61 of 64 checks passed
@Jarred-Sumner Jarred-Sumner deleted the farm/55e693cf/deflake-socket-win-bound branch April 24, 2026 05:04
structwafel pushed a commit to structwafel/bun that referenced this pull request Apr 25, 2026
…mory' (oven-sh#29663)

`test/js/bun/net/socket.test.ts` "should not leak memory" has been
hitting `TCPSocket`/`TLSSocket` count `4` on the Windows 2019 agents,
one past the previous `isWindows ? 3` bound. Same flake shipped with
merged oven-sh#29631 today and has been blocking oven-sh#29593 through ~15 retriggers.

Non-Windows bound stays at `2` — that's where real retention regressions
are caught. The Windows bound is already loosened for its shared
prototype/structure retention; this just accommodates the one additional
retained instance the Win2019 lane is observing.

## Observed

- Build [#47600 Win2019
x64](https://buildkite.com/bun/bun/builds/47600#019dbc49-ff0c-49c2-afaa-134711815456)
— `Received: 4, Expected: <= 3`
- Build [#47616 Win2019
x64](https://buildkite.com/bun/bun/builds/47616#019dbc99-fc61-4111-8170-080d9729866a)
— same
- Multiple prior PRs:
[oven-sh#29631](oven-sh#29631),
[oven-sh#29651](oven-sh#29651),
[oven-sh#29649](oven-sh#29649),
[oven-sh#29650](oven-sh#29650),
[oven-sh#29645](oven-sh#29645),
[oven-sh#29639](oven-sh#29639) all hit the same
pattern

## Scope

3-line diff + comment. If the Win2019 retention count IS a regression
worth investigating, that's a separate issue; this change is strictly
about not holding other PRs hostage to the shared flake.

Co-authored-by: robobun <robobun@bun.sh>
xhjkl pushed a commit to xhjkl/bun that referenced this pull request May 14, 2026
…mory' (oven-sh#29663)

`test/js/bun/net/socket.test.ts` "should not leak memory" has been
hitting `TCPSocket`/`TLSSocket` count `4` on the Windows 2019 agents,
one past the previous `isWindows ? 3` bound. Same flake shipped with
merged oven-sh#29631 today and has been blocking oven-sh#29593 through ~15 retriggers.

Non-Windows bound stays at `2` — that's where real retention regressions
are caught. The Windows bound is already loosened for its shared
prototype/structure retention; this just accommodates the one additional
retained instance the Win2019 lane is observing.

## Observed

- Build [#47600 Win2019
x64](https://buildkite.com/bun/bun/builds/47600#019dbc49-ff0c-49c2-afaa-134711815456)
— `Received: 4, Expected: <= 3`
- Build [#47616 Win2019
x64](https://buildkite.com/bun/bun/builds/47616#019dbc99-fc61-4111-8170-080d9729866a)
— same
- Multiple prior PRs:
[oven-sh#29631](oven-sh#29631),
[oven-sh#29651](oven-sh#29651),
[oven-sh#29649](oven-sh#29649),
[oven-sh#29650](oven-sh#29650),
[oven-sh#29645](oven-sh#29645),
[oven-sh#29639](oven-sh#29639) all hit the same
pattern

## Scope

3-line diff + comment. If the Win2019 retention count IS a regression
worth investigating, that's a separate issue; this change is strictly
about not holding other PRs hostage to the shared flake.

Co-authored-by: robobun <robobun@bun.sh>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants