s3: S3File.writer().end(error) aborts the upload instead of committing - #33681
s3: S3File.writer().end(error) aborts the upload instead of committing#33681robobun wants to merge 9 commits into
Conversation
The NetworkSink backing S3File.writer() documented end(error?: Error) but the error argument never reached the sink: js_end in Sink.rs did not read frame.argument(0), and NetworkSink.end_from_js called self.end(None) unconditionally. Passing an Error flushed the buffered tail, sent CompleteMultipartUpload, and resolved, publishing a truncated object under the real key with no way for the caller to prevent it. Plumb the error argument through JsSinkType::end_from_js. NetworkSink now rejects both the end and flush promises with the caller's error and drives MultiPartUpload.fail(), which cancels queued parts and issues AbortMultipartUpload when an upload id exists. This matches the behavior of the Response(ReadableStream) upload path.
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe sink end_from_js contract now accepts a JS error value and propagates it through host-fn wiring and sink implementations. NetworkSink uses that value to trigger abort/reject handling for multipart uploads. Multipart upload parsing and rollback behavior were updated, and a test covers S3 writer end(error) outcomes. ChangesSink end_from_js error propagation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:01 PM PT - Jul 7th, 2026
❌ @robobun, your commit 48f520f has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33681That installs a local version of the PR into your bun-33681 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/runtime/webcore/streams.rs`:
- Around line 2444-2458: The returned promise in the `end_promise`/`value` flow
can be collected too early because `self.end_promise.reject(...)` clears the
Strong handle before the later `task.fail(...)` path finishes. In
`src/runtime/webcore/streams.rs`, keep the `value` from
`self.end_promise.value()` rooted for the entire abort/failure path by ensuring
it stays alive before calling `reject()` and before any
`task_mut().unwrap().fail(...)` work, so the promise cannot be dropped mid-path.
In `@test/js/bun/s3/s3-writer-end-error.test.ts`:
- Line 148: The strict empty-stderr check in the S3 writer error test is too
brittle for debug/ASAN runs. Update the test around the stderr assertion to stop
requiring exact emptiness on the happy path, and instead only report stderr in
failure diagnostics or make the check conditional; use the existing test flow in
s3-writer-end-error.test.ts to keep the assertion from flaking while preserving
useful output when the test fails.
- Around line 106-114: The `single.bin` assertion is using a fixed
`Bun.sleep(50)` to prove no request was sent, which is flaky. In the
`s3-writer-end-error.test.ts` flow around `settle()` for the `single.bin` case,
replace that one-off sleep with the same bounded polling approach used by
`waitFor` in the other cases, repeatedly confirming `reqs.length === 0` stays
true for the window. Keep the check tied to `settle`, `summary`, and the
`single.bin` writer path so the negative assertion remains stable under slow CI.
🪄 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: dc4179ba-8cb9-4683-916d-e8392dbe2251
📒 Files selected for processing (5)
src/runtime/webcore/ArrayBufferSink.rssrc/runtime/webcore/FileSink.rssrc/runtime/webcore/Sink.rssrc/runtime/webcore/streams.rstest/js/bun/s3/s3-writer-end-error.test.ts
…ct stderr check; bounded poll for negative case
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
MultiPartUpload::fail() synchronously invokes the wrapper callback with callback_context = *mut NetworkSink, which would form a second &mut NetworkSink while fail_from_js's &mut self is live (a callback_ctx contract violation). Detach self.task first and replace the callback with a no-op so fail() cannot re-enter the sink; release the sink's ref explicitly afterwards. Also disable LSan in the test subprocess: the -e fixture triggers pre-existing transpiler/sourcemap leaks that are suppressed in the outer runner via leaksan.supp but not in the child.
When fail() runs while InitiateMultipartUpload is still in flight (state == MultipartStarted), start_multi_part_request_result used to early-return on state == Finished without reading the UploadId the server just assigned, leaving the upload orphaned. Parse the id and issue AbortMultipartUpload on that path so end(error) called in the same turn as the write that dispatched the init still cleans up. A new race test case covers this.
Deduplicate the <UploadId> extraction and validation between the Finished and Success branches of start_multi_part_request_result.
|
The diff is green. The new test The remaining CI red across the last three builds is unrelated to this change:
Ready for review/merge. |
…ToSync
NetworkSink::end_from_js now gates on is_any_error() instead of
!is_empty_or_undefined_or_null(), so non-Error argument shapes (e.g.
option bags) are ignored and the upload commits, matching the
documented end(error?: Error) contract.
node:stream/iter pipeTo was passing {signal} to writer.end() on the
success path while the sibling pipeToSync passes no argument; align
the async path so neither trips a sink that interprets the first
argument. A new test case confirms end({signal}) commits.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/js/internal/streams/iter/pull.ts`:
- Around line 955-957: Recheck the abort signal in the close/commit path of the
iterator pull flow so a late abort isn’t skipped after the last write completes.
In the logic around writer.endSync() and writer.end?.() in pull.ts, add a final
signal check before proceeding to close/return, and route that case through the
same abort handling used by the catch path instead of committing normally.
🪄 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: 44ad2c1e-9678-47d2-99ac-8cbc59d37524
📒 Files selected for processing (3)
src/js/internal/streams/iter/pull.tssrc/runtime/webcore/streams.rstest/js/bun/s3/s3-writer-end-error.test.ts
JSDOMException uses JSC::ObjectType rather than ErrorInstanceType, so is_any_error() returned false for it even though its prototype chains through Error.prototype and DOMException instanceof Error is true. AbortController.abort() produces a DOMException as signal.reason, so end(signal.reason) was falling through to the commit path. Extend JSC__JSValue__isAnyError to also match WebCore::JSDOMException. This is semantically correct for every existing caller (expect matchers, napi_is_error, promise wrapping, handler-returned-error checks) since Node treats DOMException as a native Error too. A new test case covers end(AbortSignal.reason).
There was a problem hiding this comment.
No new issues found on this pass, but the unsafe task-detach/callback-swap/deref_ sequence in fail_from_js and the widening of JSC__JSValue__isAnyError to include DOMException (which changes semantics for every existing is_any_error() caller — napi, expect matchers, etc.) are both worth a human sanity check before merge.
Extended reasoning...
Overview
This PR wires the previously-ignored end(error?) argument through the JsSinkType::end_from_js trait to NetworkSink, adds NetworkSink::fail_from_js (which detaches the multipart task, swaps its callback to a no-op, drives fail() via a raw pointer, and manually releases the sink's ref), extends MultiPartUpload::start_multi_part_request_result to roll back an UploadId that arrives after fail(), extracts a shared parse_upload_id helper, extends JSC__JSValue__isAnyError in bindings.cpp to also recognize WebCore::JSDOMException, drops the {signal} argument from pipeTo's success-path writer.end?.(), and adds a 263-line local-stub S3 test covering six scenarios.
Security risks
None identified. The change is data-integrity oriented (don't commit truncated S3 objects). No auth, crypto, or input-validation surface is touched.
Level of scrutiny
High. This went through five prior bug-finding rounds on this PR, each surfacing a real issue (Stacked-Borrows/noalias UB from re-entrant &mut NetworkSink, orphaned upload when init races with fail(), pipeTo regression, DOMException falling through the is_any_error() gate). All were addressed, and the current bug-hunting pass found nothing new. But the final shape still contains:
- An
unsafeblock infail_from_jsthat manually manages aMultiPartUploadrefcount (self.task.take()→ mutate fields via raw ptr →fail()→deref_), replacing whatwrapper_callback → detach_writableused to do. Refcount balance across theNotStarted/MultipartStarted/MultipartCompleted/Finishedstate matrix is subtle and exactly the class CLAUDE.md flags as most-blocked. - A cross-cutting semantic change:
isAnyErrornow returns true forDOMException. The author asserts this is correct for every existing caller (napi_is_error, expect matchers, promise wrapping, handler-returned-error checks), which is plausible givenDOMException instanceof Error === true, but it's a global behavior change riding on an S3 fix and deserves a maintainer nod.
Other factors
All prior review threads (mine and CodeRabbit's) are resolved. Test coverage is thorough for the S3 path (multipart mid-flight, init race, DOMException, sub-part, no-error control, non-Error-arg control). The pull.ts change is a behavioral no-op (the arg was never read before this PR) and the author correctly declined CodeRabbit's out-of-scope throwIfAborted suggestion. Given the memory-safety surface and the isAnyError widening, I'm deferring rather than approving.
1 participant
What
S3File.writer().end(new Error(...))now aborts the in-progress multipart upload and rejects with the caller's error. Previously the error argument was dropped, the buffered tail was flushed,CompleteMultipartUploadwas sent, and the promise resolved, silently publishing a truncated object under the final key.Reproduction
The sibling
s3.write(key, new Response(readableStream))path already aborted on stream error; this brings the.writer()path to the same behavior.Cause
JSSink::js_endinSink.rsnever readframe.argument(0), so the documentedend(error?: Error)argument was lost.NetworkSink::end_from_jsthen calledself.end(None)unconditionally, sending EOF (commit) regardless.Fix
JsSinkType::end_from_jsnow receives the JS error argument;js_endreadsframe.argument(0)and passes it through.ArrayBufferSink,FileSink, andHTTPServerWritableignore it as before.NetworkSink::end_from_jsroutes a non-null error to a newfail_from_js, which rejects any pendingend/flushpromise with the caller's error and drivesMultiPartUpload::fail().fail()cancels queued parts and issuesAbortMultipartUploadwhen an upload id has been received.Verification
New
test/js/bun/s3/s3-writer-end-error.test.tsruns a local S3 stub and asserts, for both the mid-multipart and pre-upload cases, thatend(error)rejects with the caller's message, noCompleteMultipartUploador single-filePUTis sent, and the object is never committed. A control case confirmsend()with no error still commits.