Remove stale signal.is_dead() assertion in pipe_readable_stream_to_blob - #32657
Conversation
The controllerDetached hook added in #32120 clears the sink signal when controller.end()/.close() is called. When a source stream finishes synchronously inside assignToStream (for example an S3 file with no credentials, which throws in pull()), the controller detaches before assignToStream returns and the signal is legitimately dead. The assertion predates that hook; RequestContext.rs already dropped its equivalent assertion with the same reasoning.
|
Updated 7:09 PM PT - Jun 24th, 2026
✅ @Jarred-Sumner, your commit 56b0ec0e81acbd8683f2799b18c41d82cd7d0e37 passed in 🧪 To try this PR locally: bunx bun-pr 32657That installs a local version of the PR into your bun-32657 --bun |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
WalkthroughRemoves a debug assertion in ChangesFileSink synchronous close handling
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…3 error does not trip this test
There was a problem hiding this comment.
LGTM — the test now accepts either resolve or reject per the earlier feedback, and the Rust change is a debug-assert removal with no release-build impact.
Extended reasoning...
Overview
This PR removes a single debug_assert!(!signal.get().is_dead()) in pipe_readable_stream_to_blob (src/runtime/webcore/Blob.rs) and replaces it with a comment explaining why the signal may legitimately be dead after assignToStream returns — since #32120, synchronous stream completion runs __controllerDetached which clears the signal back to dead before control returns. It also adds a regression test (test/js/bun/s3/s3-write-to-file-sync-close.test.ts) that reproduces the fuzzilli-found panic via Bun.file().write(Bun.S3Client.file(...)) with no credentials.
Security risks
None. The only runtime change is removing a debug-build-only assertion; release builds are byte-identical in behavior. The test spawns a subprocess with S3/AWS credential env vars explicitly unset, so it cannot accidentally hit a real bucket.
Level of scrutiny
Low. The Rust change is purely subtractive in debug builds and a no-op in release. The code paths immediately following the removed assertion (error → rejected promise; promise Pending/Fulfilled/Rejected branches) already handle all states without referencing the signal, and RequestContext.rs already dropped its equivalent post-assignToStream assertion for the same reason — only the pre-call is_dead() check remains there, mirroring what Blob.rs now has.
Other factors
I left an inline comment on the earlier revision noting the test was pinning a pre-existing swallowed-error bug as the contract. Commit fb1212a addressed that exactly as suggested (Option B): the fixture now wraps the write() in try/catch, the assertion accepts either resolved or rejected:ERR_S3_MISSING_CREDENTIALS, and the file header documents the swallowed-error situation so a future fix to ByteStream::on_start() won't trip this test. No CODEOWNERS cover these paths. The robobun musl build failures reference the older commit (617613c) and are infra (scripts/build/ci.ts) failures unrelated to a comment-only Rust change and a new test file.
There was a problem hiding this comment.
LGTM — thanks for updating the test to accept either resolve or reject.
Extended reasoning...
Overview
This PR removes a single debug_assert!(!signal.get().is_dead()) in pipe_readable_stream_to_blob (src/runtime/webcore/Blob.rs) and adds a regression test (test/js/bun/s3/s3-write-to-file-sync-close.test.ts). The assertion was checking that C++ wrote the controller cell into the sink signal during assignToStream, but since #32120 introduced __controllerDetached, a synchronously-completing source stream (e.g., S3 with no credentials) can legitimately end the sink and clear the signal back to dead before assignToStream returns. The code immediately following the removed line already handles all outcomes (error, fulfilled, rejected, pending promise), and RequestContext.rs already dropped the equivalent assertion for the same reason.
Security risks
None. This is a debug_assert! — it compiles out of release builds entirely, so no production behavior changes. The test only spawns a child Bun process with credential env vars unset; no network/auth/crypto surface is touched.
Level of scrutiny
Low. A 3-line deletion of a debug-only assertion, with clear precedent in the codebase and a well-reasoned PR description tracing the exact code path that invalidates the invariant. The new test is isolated and self-contained.
Other factors
I previously flagged that the original test assertions locked in a pre-existing swallowed-error bug as the contract. The author addressed this in commit fb1212a: the fixture now wraps the write() in try/catch, the test accepts either resolved or rejected:ERR_S3_MISSING_CREDENTIALS, and a header comment documents that ByteStream::on_start() currently swallows the error so a future fix won't trip this test. That inline comment is now resolved. No bugs were found in the current revision and there are no outstanding reviewer comments.
2 participants
Fuzzilli found a debug assertion failure:
panic: assertion failed: !signal.get().is_dead()atsrc/runtime/webcore/Blob.rs:1653.Minimal repro:
What was happening
pipe_readable_stream_to_blobsets the sink signal to a dead sentinel, callsFileSink__assignToStream, then asserts the signal is no longer dead (i.e. that C++ wrote the controller cell into it).C++ does always write the controller into
signal.ptrbefore running the JSassignToStreambuiltin. But since #32120 added__controllerDetached, callingcontroller.end()/.close()during that JS clears the signal back to dead. When the source stream'spull()fails synchronously (an S3 file with no credentials throwsERR_S3_MISSING_CREDENTIALS), the sink is ended beforeassignToStreamreturns, the signal is cleared, and the assertion trips.The code after the assertion already handles this correctly (error/promise-status branches), and
RequestContext.rsalready dropped its equivalent post-call assertion for the same reason. This change drops the stale assertion here and documents why.How did you verify your code works?
New test in
test/js/bun/s3/s3-write-to-file-sync-close.test.tsspawns a child that writes an S3 file (no credentials) to a file Blob. Fails on the current debug build with the assertion panic; passes with this change.