Repository navigation
Conversation
`get_s3_default_client` cached the client in a per-VM Strong and returned it to every global of the VM. Under `bun test --isolate` the next file got the client of a finished file, and the calls of that client never settled. `Bun.s3` is a lazy property. JSC calls the getter one time for each `Bun` object and keeps the result on that object. Delete the per-VM slot.
|
Status
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 5 seconds. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
S3 client creation and lifecycle src/runtime/api/BunObject.rs, src/jsc/rare_data.rs |
The getter reads credentials through the VM’s mutable environment loader and returns the created client directly. RareData no longer stores or deinitializes the cached client. |
Realm and isolated-file regression tests test/cli/test/isolation.test.ts, test/js/bun/s3/s3.test.ts |
Tests check that clients do not share properties across isolated files or between a ShadowRealm and the current realm. |
Possibly related issues
- bun test --isolate: intermittent segfault in Bun__runDeferredWork at exit after all tests pass (1.4.2, Linux x64, JIT off) #43056 — The issue reports deferred JSC work after an isolated test VM is torn down. This change tests client separation across isolated files, but does not describe changes to deferred-work scheduling.
Suggested reviewers: jarred-sumner, alii
Merge Risk: 
9dac9
-
No actionable merge-blocking risk is established for the per-realm S3 client change. Merge after normal checks pass.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly states the main change: each isolated test file receives its own Bun.s3 client. Description check The description explains the problem, root cause, fix, trade-offs, and verification results. It does not use the exact template headings, but it provides the required change summary and verification i… 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. Full details: Description check
-
Explanation
The description explains the problem, root cause, fix, trade-offs, and verification results. It does not use the exact template headings, but it provides the required change summary and verification information.
-
- 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 checked the deleted Strong/ensure_still_alive against the rooting rules: s3 is registered as PropertyCallback in src/jsc/bindings/BunObject.cpp:1019 (same as redis), so JSC stores the returned client as an own property of each realm's Bun object and get_valkey_default_client already follows this no-cache shape. The error path of get_s3_default_client is byte-for-byte the base's, the raw transpiler.env deref is replaced by the existing env_mut() helper that carries its own SAFETY comment, and no reference to s3_default_client remains anywhere in the repo.
Extended reasoning...
The PR removes the per-VM RareData.s3_default_client Strong and makes the Bun.s3 lazy-property callback build and return a fresh S3Client per realm, plus two tests (isolate/parallel and ShadowRealm). It touches GC rooting of a native wrapper but no auth, injection, or data-exposure surface; the credentials read is unchanged. A test-coverage finding is posted inline, and since the change deletes a GC root a human glance is still reasonable, so this is a defer with a ruled-out note rather than an approve.
The per-VM slot no longer keeps the client. The lazy property of the `Bun` object is what makes two reads in one realm give one object.
There was a problem hiding this comment.
LGTM — the second commit adds the same-realm identity assertions I asked for, and the fix itself is the same shape Bun.redis already uses.
What was reviewed:
- Confirmed
s3is aPropertyCallbackentry on the Bun object (BunObject.cpp:1019), so JSC reifies the returned client onto each realm's ownBunobject;get_valkey_default_clientrelies on the identical mechanism with no VM-wide Strong. - Checked the
to_jsreturn path: the wrapper goes straight from the Rust callback into JSC'sputDirectwith nothing allocating in between, so droppingensure_still_alive/StrongOptionalleaves no window. env_mut()is required byget_s3_credentials(&mut self); the loader borrow ends before any other VM access. No stales3_default_clientreferences remain in the tree.- Both new tests fail on the old code with a wrong value (shared
madeIn/markedBy) rather than a hang, and the--parallel=2case uses the 60s scale-up knob so both files share one worker VM.
Extended reasoning...
The PR removes the VM-wide RareData.s3_default_client Strong and lets the Bun.s3 lazy property hold the client per realm, touching src/jsc/rare_data.rs, src/runtime/api/BunObject.rs, and two test files (72 insertions, 31 deletions). It touches no auth, injection, or data-exposure surface: credential reading from the env loader is unchanged, only where the resulting wrapper is cached moved. The deciding facts for approval were that the mechanism is already proven by Bun.redis in the same file, the returned value is stored by JSC immediately with no GC window, the new tests assert identity within a realm and isolation across realms and fail on the unfixed code for the right reason, and the one prior inline comment was addressed by the second commit. No CODEOWNERS entry covers the changed paths and the hunt ran dry rather than hitting a budget bound.
Problem
bun test --isolateor--parallel, a call onBun.s3never settles from the second test file on.await Bun.s3.file("k").exists()times out after 5000 ms. 1.4.2 passes. 1.4.3, 1.4.4 and main do not.get_s3_default_client(src/runtime/api/BunObject.rs:1374) returns a per-VMStrong,RareData.s3_default_client, to every global. The second file gets the client of the first.Fix
Strong.Bun.s3is a lazy property: JSC calls the getter once for eachBunobject and keeps the result there, as forBun.redis.test/cli/test/isolation.test.tsandtest/js/bun/s3/s3.test.ts. Both fail on main.Background
Bunobject.--isolatemakes one for each test file. AShadowRealmis one.Strongis a GC root. This one also kept the global of the first file alive for the whole run.ShadowRealmstill gets the wrong client, and nothing else reads the slot.Downsides
Bun.s3builds a client, one time: 27,553 instructions and 9 mallocs. Before: 30 instructions.--isolate, a property that a file sets onBun.s3is gone in the next file.Notes
Repro
bun test --isolate ./1.test.ts ./2.test.ts, with no S3 credentials in the environment:ERR_S3_MISSING_CREDENTIALSat once (it has the client of file 1, and the call settles)f7607a3d35)ERR_S3_MISSING_CREDENTIALSat once, from its own clientEach file passes alone on every build.
afd78836a7(#41831) is an ancestor ofbun-v1.4.3and not ofbun-v1.4.2.In a
ShadowRealm: afternew ShadowRealm().evaluate("Bun.s3.madeIn = 'realm'"), main givesBun.s3.madeIn === "realm"in the host realm. With this PR it isundefined, in either read order.Measurements (linux-x64 release builds,
f7607a3d35against this PR)Bun.s3read of a VMBunObject_lazyPropCb_s3Bun.s3read of a second realmvoid Bun.s3heapStats().protectedObjectTypeCountsBun.s3heapStats()after twoBun.gc(true),--isolateand--parallel=2RareData.textsize -A..rodatadoes not change. No new host function.perf, valgrind, strace and bloaty are not installed in the container. The instruction counts come from gdb.
Tests
isolation.test.ts: the second file has its ownBun.s3, under--isolateand inside one--parallelworker.s3.test.ts:Bun.s3in aShadowRealmis a client of that realm.isolation.test.ts(51 pass) ands3.test.ts(308 pass, 270 skip without R2 credentials).Not changed
Other values of one realm that the VM or the process still keeps:
bun:sqlresult callbacks (RuntimeState.sql_rare): bun:sql: keep the query callbacks on the realm that registers them #47494, on top of this PR.require.extensionshandlers (VirtualMachine.commonjs_custom_extensions): bun test --isolate: reset Module._extensions per file and ask it before the source cache answers a require() #47477. On this branch a handler of a finished file still runs for the next file.node:quiccallback holder (RareData.node_quic_callbacks). The file swap clears it.node:clusterchild state (CHILD_SINGLETON,src/runtime/node/node_cluster_binding.rs:28).Self-review
The review covered this PR and #47494 as one diff. It asked for the split: this half fixes a regression and has no design question. Not done: a fix for the JIT plan that can root a finished file's global for a short time (it is not a value that the VM owns), and a move of the retention tests of #47494 into the test table of another open PR.