Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions src/jsc/bindings/bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3538,12 +3538,23 @@ CPP_DECL uint8_t JSC__JSValue__pinArrayBuffer(JSC::EncodedJSValue v)
// Only for a value `pinStorage` answered `Pinned` for: that buffer still exists (pinned buffers are not detached).
CPP_DECL void JSC__JSValue__unpinArrayBuffer(JSC::EncodedJSValue v)
{
// Reached from finalizers during GC sweep, where classInfo() (and so any
// dynamicDowncast) is forbidden; dispatch on JSType like JSC::Weak<T>::get().
Comment on lines +3541 to +3542

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

auto value = JSC::JSValue::decode(v);
if (!value.isCell())
return;
JSC::JSCell* cell = value.asCell();
JSC::JSType type = cell->type();
JSC::ArrayBuffer* buf = nullptr;
if (auto* jb = dynamicDowncast<JSC::JSArrayBuffer>(value))
buf = jb->impl();
else if (auto* view = dynamicDowncast<JSC::JSArrayBufferView>(value); view && view->hasArrayBuffer())
buf = view->possiblySharedBuffer();
if (type == JSC::ArrayBufferType)
buf = static_cast<JSC::JSArrayBuffer*>(cell)->impl();
else if (type == JSC::DataViewType)
buf = static_cast<JSC::JSDataView*>(cell)->possiblySharedBuffer();
else if (JSC::isTypedArrayType(type)) {
auto* view = static_cast<JSC::JSArrayBufferView*>(cell);
if (JSC::isWastefulTypedArray(view->mode()))
buf = view->butterfly()->indexingHeader()->arrayBuffer();
}
if (buf && !buf->isShared())
buf->unpin();
}
Expand Down
40 changes: 40 additions & 0 deletions test/js/bun/s3/s3-stream-error-gc.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,46 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, normalizeBunSnapshot } from "harness";

test.concurrent("collecting file blobs with Buffer paths does not crash during GC sweep", async () => {
// The S3/file blob store keeps the pinned path buffer until the wrapper is
// finalized inside the GC sweep; releasing the pin must not reach
// JSCell::classInfo() there (validateIsNotSweeping assert in debug builds).
const fixture = `
const enc = new TextEncoder();
for (let i = 0; i < 50; i++) {
new Bun.S3Client({}).file(Buffer.from("key-" + i));
new Bun.S3Client({}).file(new DataView(enc.encode("dv-key-" + i).buffer));
new Bun.S3Client({}).file(enc.encode("uint8-key-" + i));
Bun.file(Buffer.from("/tmp/buffer-path-" + i));
Bun.file(enc.encode("/tmp/uint8-path-" + i));
Bun.file(enc.encode("/tmp/enc-path-" + i).buffer);
Bun.gc(true);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
console.log("ok");
`;

await using proc = Bun.spawn({
cmd: [bunExe(), "-e", fixture],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect({
stdout: normalizeBunSnapshot(stdout),
stderr: normalizeBunSnapshot(stderr),
exitCode,
}).toMatchInlineSnapshot(`
{
"exitCode": 0,
"stderr": "",
"stdout": "ok",
}
`);
Comment on lines +31 to +41

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.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Do not require empty stderr in this GC subprocess test.

Bun.gc(true) can produce benign debug or ASAN diagnostics. The stderr: "" snapshot can fail although the regression succeeds. Continue to drain stderr, but assert stdout and exitCode only.

Based on learnings: avoid stderr: "" assertions in aggressive-GC subprocess tests.

Proposed assertion change
-  const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+  const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

   expect({
     stdout: normalizeBunSnapshot(stdout),
-    stderr: normalizeBunSnapshot(stderr),
     exitCode,
   }).toMatchInlineSnapshot(`
     {
       "exitCode": 0,
-      "stderr": "",
       "stdout": "ok",
     }
   `);
🤖 Prompt for 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.

In `@test/js/bun/s3/s3-stream-error-gc.test.ts` around lines 29 - 39, Update the
subprocess assertion in the GC test to stop snapshotting or requiring an empty
stderr value, while continuing to drain stderr. Assert only the normalized
stdout and exitCode, preserving the existing expected values and success
behavior.

Source: Learnings

});

test("S3 stream error parked before consumption survives GC", async () => {
const fixture = `
const stream = Bun.S3Client.file("some-key").stream();
Expand Down
Loading