Repository navigation
Conversation
…S3 server Each of the five leak fixtures called its operation one time: `new Array(100).fill(fn())` fills the array with one promise. A case compared the RSS after one operation with the RSS after one operation, so a leak could not make it fail. The file also ran against Cloudflare R2 only, where one request of 1 MiB sometimes takes longer than the 30 s limit of a case. - One fixture, s3-leak-fixture.ts, replaces the five. It does 30 operations as a warm-up and then measures 100, ten at a time. It fails if the 100 operations did not move 100 MiB. - The fixture runs against the server of test/packages/s3-server. Nothing in the file needs R2. - The fixture reads the RSS until it stops falling. The test uses expectRssDeltaBelow, which now takes the environment of the fixture. - The entry of the file in test/flaky-tests.txt goes away: the flake was the time limit on R2. Co-authored-by: Jax <jaxyang39@gmail.com>
|
Status: the diff is green. CI cannot finish, because the macOS x64 lane has no agent. The next step is a review by a maintainer. CI, build 131737 (head 58b0d75):
How I reproduced the problem:
How I checked the new file:
|
|
Updated 2:55 AM PT - Oct 24th, 2026
❌ @robobun, your commit 58b0d75 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 46686That installs a local version of the PR into your bun-46686 --bun |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Shared fixture operations test/js/bun/s3/s3-leak-fixture.ts, test/js/bun/s3/*-leak-fixture.js |
The shared fixture defines five selectable S3 operations and runs them in batches. The previous operation-specific fixtures were removed. |
RSS measurement test/js/bun/s3/s3-leak-fixture.ts |
The fixture warms up, settles RSS readings, measures ten batches, checks that the operations moved 100 MiB, and prints the RSS delta. |
Local server and test integration test/js/bun/s3/s3.leak.test.ts, test/harness.ts, test/flaky-tests.txt |
The tests configure a local S3 server and run the shared fixture with per-operation RSS limits. The harness accepts a custom child-process environment, and the flaky-test entry is removed. |
Possibly related PRs
- oven-sh/bun#46605: Moves large-transfer cases to a local S3 server instead of relying on Cloudflare R2.
- oven-sh/bun#46581: Also uses RSS-based subprocess leak checks and addresses ASAN quarantine handling.
- oven-sh/bun#46653: Changes S3 blob size resolution, which can affect the read operations used by this fixture.
Suggested reviewers: jarred-sumner
Priority:
Merge Risk: 
95f85
-
The consolidated leak tests have no identified merge-blocking issue; proceed with normal test checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies both primary changes: making the S3 leak test effective and running it against the local S3 server. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description is complete and relevant. It explains the problem, implementation, verification steps, test results, scope, and background. It does not use the exact template headings, but it provides… Full details: Description check
-
Explanation
The description is complete and relevant. It explains the problem, implementation, verification steps, test results, scope, and background. It does not use the exact template headings, but it provides the required information in equivalent sections.
-
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked: S3_* is read ahead of AWS_* in src/dotenv/env_loader.rs, so the fixture's env wins over machine credentials; the top-level spawnServer plus afterAll(stop) shape and the region-bearing clientOptions("buntest") match what s3.test.ts already does; the new defaulted env parameter on expectRssDeltaBelow leaves every other caller unchanged; and writer.end() is typed as the byte count, with the fixture's moved !== 100 * MiB guard throwing rather than passing vacuously if that ever changed.
Extended reasoning...
Test-only change: one consolidated S3 leak fixture replaces five, the test now runs against the in-tree s3-server instead of R2, and the harness helper gains a backward-compatible env parameter. No runtime source or security-sensitive surface is touched. The test previously skipped on every lane without R2 secrets and now asserts RSS thresholds on all CI platforms for the first time, which is why a human look at the first CI run is still worthwhile.
The fixed limit of 30 s was below the limit that the CI runner sets for a test: 90 s, or 270 s on the ASAN lane. Only a debug build needs more than the default limit of `bun test` (5 s): a case takes up to 11 s there.
There was a problem hiding this comment.
I reviewed the updated PR (including the follow-up that limits the 30 s per-test timeout to debug builds) and found no bugs; since the RSS bounds and the per-case timing were measured on Linux x64 only, a human look at the first CI results on the other lanes is still worthwhile.
What was reviewed:
- The claim that
S3_*wins overAWS_*is true:src/dotenv/env_loader.rsget_s3_credentialsreadsS3_*first for all six names, including the session token the test unsets. spawnServerpasses the givenenvstraight toBun.spawn, so theundefinedproxy entries drop those variables; the top-levelawait spawnServer+afterAll(stop)shape and thebuntestbucket matchs3.test.ts.it.concurrentIfexists inbun:testtypes;expectRssDeltaBelowstill applieswithoutAsanQuarantineto the newenvargument, and the file remains listed intest/no-validate-leaksan.txt.- The fixture's 100 MiB "moved" guard would fail loudly rather than silently if
writer.end()or a read returned fewer bytes, and the settle loop is bounded (250 readings).
Extended reasoning...
Test-only change: it rewrites test/js/bun/s3/s3.leak.test.ts to spawn the in-repo s3-server instead of using R2 secrets, replaces five JS fixtures with one parameterized TypeScript fixture that measures settled RSS over 100 one-MiB operations, adds an optional env parameter to expectRssDeltaBelow in test/harness.ts, and removes the file's flaky-tests.txt entry. No production code or security-sensitive surface is touched. The structure mirrors the existing s3.test.ts usage of spawnServer, and the env-precedence and env-passthrough claims check out against src and the s3-server package. Deferring rather than approving because the thresholds and the debug-only 30 s cap (still below the CI runner's 90 s / 270 s ceiling) rest on measurements from one platform, and the PR itself notes CI is the first run elsewhere.
|
The first CI results on the other lanes are in the description now (Notes, "CI results"). In builds 131667 and 131712 the file passed on ten lanes, each time with
macOS x64 has not run the file yet. Its test jobs got no agent in either build. |
Problem
test/js/bun/s3/s3.leak.test.tscannot fail for a leak. Each fixture calls its operation one time:new Array(100).fill(readLargeFile())fills the array with one promise.this test timed out after 30000ms.Fix
s3-leak-fixture.ts, replaces the five. It warms up with 30 operations, then measures 100, ten at a time. It fails if those 100 did not move 100 MiB.test/packages/s3-server(test: replace the MinIO container with an S3 server on Bun.serve #44054), ass3.test.tsdoes. Nothing in this file needs R2.bun bd test test/js/bun/s3/s3.leak.test.tsand the release build of main pass.Background
fetch()share the HTTP client, andfetch-leak.test.tshas TLS cases.Notes
Each case fails with a leak and passes without it
The leak is in a scratch copy of the fixture: each operation keeps its body in an array (the chunks of
stream(), the string oftext(), a freshBufferfor each write). The numbers are the growth in MiB that the test got.stream()text()writer()write()Bun.writebun bd test7d240c1dd367d939d9(2026-09-18)With the leak the result is
0 pass, 5 failon each build, for exampleExpected: < 25, Received: 106.6015625.The growth without a leak
BUN_GARBAGE_COLLECTOR_LEVEL=1,BUN_JSC_randomIntegrityAuditRate=1.0)367d939d9BUN_JSC_useDFGJIT=0(release367d939d9,write()) the RSS stays between 37.0 and 38.1 MiB from the second batch to the twelfth.367d939d9by 2.2 to 6.1 MiB. So there is no growth per operation.The guard on the work
The fixture adds up the bytes that the operations moved, and throws if the sum is not 100 MiB. The sum is made in the function that
Array.fromcalls for each element.new Array(10).fill(promise)) each case fails:error: The 100 operations moved 10485760 bytes, not 100 MiB.error: The 100 operations moved 0 bytes, not 100 MiB.The reading of the RSS
Bun.gc(true)the RSS falls for about 200 ms more, in steps about 100 ms apart. In a sample from the release build of 09-18 it went from 55.7 to 44.8 MiB.ASAN
expectRssDeltaBelowturns the quarantine off. It now takes the environment of the fixture as a third argument, withbunEnvas the default. No other caller changes.test/no-validate-leaksan.txtstill lists the file, so LeakSanitizer stays off for it.The environment of the fixture
Bun.s3readsS3_*beforeAWS_*(src/dotenv/env_loader.rs). The old test setAWS_*, so anS3_BUCKETof the machine won:NoSuchBucket. The test setsS3_*now.InvalidToken. The test clears both names.fetch()followHTTP_PROXY,http_proxy,ALL_PROXYandall_proxy, also for a loopback address. With one of them set, the fixture getsConnectionRefusedand the server does not start (The s3-server process ended before it listened). The test clears the four names for both.HTTPS_PROXYis not read for an http endpoint.One at a time on a debug or ASAN build
The fixtures share one server process. A debug build of the server needs 13 to 27 ms for a request.
bun bd test, this filebun test, 90 s from the CI runner, 270 s on the ASAN lane.What changed in the operations
A. It was 1 MiB of zero bytes, and 1 MiB + 1 byte ins3-write-leak-fixture.js. The client has no limit at 1 MiB: a part is 5 MiB.out.binargument are gone. No fixture read them.stream(),text(),writer(),write(),Bun.writeandunlink()are the same.What the file sends to R2 in one attempt
test/flaky-tests.txtThe entry of the file (
1 of 80 builds; windows) is removed. The three builds above show the cause: the 30 s limit on a request to R2. A flake of the new file is then reported as a new flake.CI results
The bounds come from measurements on Linux x64. CI ran the file on the other platforms for the first time. Each cell is the time of the whole file, with
5 pass, 0 fail.Build 131737 has the limit of the last commit: only a debug build sets one. macOS x64 has not run the file yet: its test jobs got no agent in any of the three builds.
Other pull requests
s3.test.tsoff R2 for the same reason.[auto-merge] gate passed · iteration 2 · 9 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 2
evidence per changed file