[codex] Fix S3 arrayBuffer native buffer leak#29776
Closed
sorenbs wants to merge 2 commits into
Closed
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
WalkthroughThis PR refactors Blob lifetime handling to prevent retained buffers during JSON and FormData conversions, particularly fixing RSS growth when downloading from S3. It also introduces a Docker-based leak detection test that validates RSS bounds during repeated S3 ArrayBuffer operations. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/s3/s3.leak.test.ts`:
- Around line 53-75: Remove the unconditional empty-stderr assertion for the
Bun.spawnSync call: instead of asserting stderr.toString() === "" after running
the subprocess (the spawn in this block that returns exitCode and stderr), rely
on checking exitCode to detect regressions; update the test to drop or guard the
expect(stderr.toString()).toBe("") line and only assert expect(exitCode).toBe(0)
(or, if you need optional warnings, conditionally allow known ASAN startup
warnings from stderr before failing).
🪄 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: 36a6940b-709f-4955-a34e-9ddf1cf19c09
📒 Files selected for processing (3)
src/bun.js/webcore/Blob.zigtest/js/bun/s3/s3-arraybuffer-leak-fixture.jstest/js/bun/s3/s3.leak.test.ts
Comment on lines
+53
to
+75
| const { exitCode, stderr } = Bun.spawnSync( | ||
| [bunExe(), "--smol", path.join(dir, "s3-arraybuffer-leak-fixture.js")], | ||
| { | ||
| env: { | ||
| ...bunEnv, | ||
| BUN_JSC_gcMaxHeapSize: "503316", | ||
| AWS_ACCESS_KEY_ID: minioOptions!.accessKeyId as string, | ||
| AWS_SECRET_ACCESS_KEY: minioOptions!.secretAccessKey as string, | ||
| AWS_ENDPOINT: minioOptions!.endpoint as string, | ||
| AWS_BUCKET: "buntest", | ||
| PAYLOAD_MIB: "1", | ||
| WARMUP_ITERATIONS: "8", | ||
| ITERATIONS: "80", | ||
| MAX_ALLOWED_RSS_INCREMENT_MB: "64", | ||
| }, | ||
| stderr: "pipe", | ||
| stdout: "inherit", | ||
| stdin: "ignore", | ||
| }, | ||
| ); | ||
|
|
||
| expect(stderr.toString()).toBe(""); | ||
| expect(exitCode).toBe(0); |
Contributor
There was a problem hiding this comment.
Drop the unconditional empty-stderr assertion for this subprocess.
This child runs JS via bunExe(), so ASAN/debug builds can emit the known startup warning on stderr even with bunEnv. That makes Line 74 a spurious failure while the actual regression signal here is exitCode.
Suggested change
- const { exitCode, stderr } = Bun.spawnSync(
+ const { exitCode } = Bun.spawnSync(
[bunExe(), "--smol", path.join(dir, "s3-arraybuffer-leak-fixture.js")],
{
env: {
...bunEnv,
BUN_JSC_gcMaxHeapSize: "503316",
@@
- expect(stderr.toString()).toBe("");
expect(exitCode).toBe(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/bun/s3/s3.leak.test.ts` around lines 53 - 75, Remove the
unconditional empty-stderr assertion for the Bun.spawnSync call: instead of
asserting stderr.toString() === "" after running the subprocess (the spawn in
this block that returns exitCode and stderr), rely on checking exitCode to
detect regressions; update the test to drop or guard the
expect(stderr.toString()).toBe("") line and only assert expect(exitCode).toBe(0)
(or, if you need optional warnings, conditionally allow known ASAN startup
warnings from stderr before failing).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Created by Codex.
Fixes #29083.
Summary
This fixes the native RSS leak when reading S3-backed blobs with
arrayBuffer()and other body materialization paths.BlobFormData path using.cloneso existing blob storage is not freed accidentally.Bun.s3.file().arrayBuffer()RSS retention.Root Cause
doReadFromS3()passed downloaded response bytes to the body conversion functions with lifetime.clone.For
arrayBuffer(), that produced the expected JavaScript result, but the source HTTP response body buffer remained allocated natively. Since that source buffer was not attached to the resulting JSArrayBuffer, JS heap, external memory, andarrayBuffersstayed bounded while anonymous RSS grew with every S3 download.The fixed path marks S3 download bytes as
.temporary, which lets the existing conversion helpers consume/free those response bytes after producing the JS value.Reproduction
I used MinIO in Docker and a temporary Linux container harness with an 8 MiB S3 object and a 1 GiB memory limit.
Original Bun (
oven/bun:1.3.11) was OOM-killed with exit137:The patched Linux release build continued well past that point:
The original run still had only one visible 8 MiB JS
ArrayBufferbefore the OOM kill, which matches this being a native response-buffer lifetime issue rather than JS heap growth.Validation
USE_SYSTEM_BUN=1 bun test test/js/bun/s3/s3.leak.test.tsfails the new regression on installed Bun 1.3.13 as expected.bun bd --asan=off test test/js/bun/s3/s3.leak.test.ts -t "arrayBuffer"bun bd --asan=off test test/js/bun/s3bun bd --asan=off test test/js/web/fetch/body.test.ts test/js/web/fetch/blob.test.ts test/js/web/fetch/body-stream.test.ts test/js/web/fetch/body-mixin-errors.test.tsgit diff --checkI also attempted
bun bd --asan=off test; that run stopped intest/v8/v8.test.tswhilenode-gypwas downloading/building Node 24.3.0 headers and the hook timed out. That failure was unrelated to the S3 change.