Report failed promise settles in event-loop completions (subprocess, streams, blob, s3, valkey) - #37072
Report failed promise settles in event-loop completions (subprocess, streams, blob, s3, valkey)#37072robobun wants to merge 4 commits into
Conversation
The subprocess exit path, ByteStream cancel, Bun.write completions (copy_file, write_file, and the locked-body wait task), S3 multipart buffering, and the valkey timeout timers all discarded the result of settling a promise (or of a completion callback) with `let _ =`. A failed settle leaves a JS exception pending on the VM, and these run as event-loop completions with no host-call boundary to surface it, so the stale exception rode the tick into unrelated JS: debug/ASAN builds hit JSC's exception-state assertions and release builds misattribute the error. Check the settle result instead and report a pending non-termination exception through the VM's unhandled-exception path, the same idiom the socket, IPC, and http2 completions use. The error label cannot be trusted to distinguish a thrown exception from termination (the FFI wrappers collapse both to the terminated sentinel), so the VM state decides: termination exceptions stay pending as the event loop expects. Tests pin the touched completions (subprocess exit-code and signal settles, locked-body and file-to-file Bun.write) under a hostile Object.prototype.then accessor, the prototype-pollution state the fuzzer runs scripts in.
WalkthroughChangesThe runtime now checks promise settlement and client failure results. Failed operations report active JavaScript exceptions instead of discarding them. Regression tests cover subprocess, streaming-write, and file-copy paths with a throwing Promise settlement handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… copyfile count The Windows copyfile completion resolves 0 when the size is not known up front (uv_fs_copyfile reports no byte count), so the expectation is platform-aware. The fixture directory now comes from the harness so it is removed on every exit path.
The resolved count on Windows depends on the backend: uv_fs_copyfile reports none (so the promise resolves 0) while the fallback loop used under BUN_FEATURE_FLAG_DISABLE_UV_FS_COPYFILE reports the real count, and this file reruns itself with that flag. The destination content check is the meaningful assertion.
The POSIX halves of the Bun.write completions (WriteFilePromise::run, CopyFile::then/reject) return into the task dispatch channel, whose error type is the terminated sentinel; a thrown exception from a failed settle was mislabeled as termination, stopped the tick drain early, and stayed pending. Consult the VM via report_error_or_terminate instead: report a pending non-termination exception and return Ok, keep real termination as the Err that unwinds the tick loop. WriteFilePromise::run is the completion callback on both platforms, so the Windows write path now reports there too instead of inside run_from_js_thread.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/webcore/blob/write_file.rs (1)
1364-1373: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPreserve
TerminationExceptionin all three callbacks.report_active_exception_as_unhandledclears termination throughtryClearException. Do not ignore theErrfromreport_error_or_terminate; propagateJsTerminatedor leave termination pending at the non-returning callback boundary.🤖 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 `@src/runtime/webcore/blob/write_file.rs` around lines 1364 - 1373, Preserve termination across all three callback error paths: in src/runtime/webcore/blob/write_file.rs:1364-1373 and src/runtime/webcore/blob/copy_file.rs:1659-1664 and 1751-1757, replace the report_active_exception_as_unhandled handling around the relevant callback settlement methods with logic that does not clear TerminationException, propagates JsTerminated from report_error_or_terminate, or leaves termination pending at the non-returning callback boundary.
🤖 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.
Outside diff comments:
In `@src/runtime/webcore/blob/write_file.rs`:
- Around line 1364-1373: Preserve termination across all three callback error
paths: in src/runtime/webcore/blob/write_file.rs:1364-1373 and
src/runtime/webcore/blob/copy_file.rs:1659-1664 and 1751-1757, replace the
report_active_exception_as_unhandled handling around the relevant callback
settlement methods with logic that does not clear TerminationException,
propagates JsTerminated from report_error_or_terminate, or leaves termination
pending at the non-returning callback boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c0206d00-eea8-4458-8f4b-b041f5485a86
📒 Files selected for processing (3)
src/runtime/webcore/blob/copy_file.rssrc/runtime/webcore/blob/write_file.rstest/js/bun/io/bun-write.test.js
|
Re the review note about preserving TerminationException in the fire-and-forget callbacks (then_wrap, CopyFileWindows::throw/resolve_promise): the premise does not hold. report_active_exception_as_unhandled takes the exception via JSGlobalObject__tryTakeException, which calls ExceptionScope::tryClearException, and that function refuses to clear a termination exception (vendor/WebKit/Source/JavaScriptCore/runtime/ExceptionScope.h:120: it returns false without calling clearException when vm.isTerminationException(exception)). The helper then sees is_termination_exception() and skips reporting, so a TerminationException stays pending on the VM at these boundaries, which is the convention the event loop expects. These three callbacks return () and are not in the task dispatch channel, so there is no Err to propagate; the channel-returning completions (WriteFilePromise::run, CopyFile::then/reject) do propagate JsTerminated via report_error_or_terminate. This is also the same idiom the socket, IPC, and http2 completions already use. |
There was a problem hiding this comment.
Both earlier inline findings are addressed (harness tempDir in the parent, typeof wroteCopy for the backend-dependent count) and no new issues turned up this pass, but I'd like a human to sign off: this changes native error routing across six subsystems, swaps ?-propagation for report_error_or_terminate in the POSIX write/copy task completions, and the description explicitly asks maintainers to weigh the per-site idiom against a single JSC__JSPromise__resolve guard.
What was reviewed:
- Confirmed
report_error_or_terminateandreport_active_exception_as_unhandledboth consult VM state (not the erased error label) and leave termination pending, matching the PR's rationale for passingJsError::Thrown. - Checked the subprocess
_ =>catch-all arm now yieldsOk(())in release sosettledbinds; thethen_wrapchange copiesglobal_this(aCopyBackRef) before the consuming call. - Verified the new bun-write pollution test survives the Windows
BUN_FEATURE_FLAG_DISABLE_UV_FS_COPYFILE=1self-rerun and cleans up viausing dir.
Extended reasoning...
Overview
The PR replaces let _ = ...; // TODO discards on promise-settle results with explicit reporting at eight completion sites across subprocess.rs, ByteStream.rs, blob/copy_file.rs (POSIX + Windows), blob/write_file.rs (POSIX WriteFilePromise::run, Windows uv callbacks, and the locked-body then_wrap), s3/multipart.rs, and js_valkey.rs. Fire-and-forget completions call report_active_exception_as_unhandled; completions that return into the task-dispatch channel call report_error_or_terminate. Two new subprocess-fixture tests pin the write and spawn completions under a hostile Object.prototype.then accessor.
Security risks
None identified. The change surfaces already-pending exceptions instead of silently discarding them; no new user input is parsed and no trust boundary is crossed.
Level of scrutiny
Moderate-to-high. Each individual edit is small and follows an established idiom (#37004), but collectively they touch native completion paths in six subsystems, and the POSIX WriteFilePromise::run / CopyFile::then/reject edits change semantics: what previously ?-propagated as JsTerminated (because the FFI wrapper erases Thrown to the terminated sentinel) is now routed through report_error_or_terminate, which probes VM state to distinguish real termination from a leaked Thrown. I read both helpers in src/jsc/Task.rs and src/jsc/JSGlobalObject.rs and they match the PR's description — take_exception + is_termination_exception() decide, and termination is left pending / returned as Err respectively — but the correctness of that swap across every task-dispatch caller is a maintainer-level judgment.
Other factors
- The description explicitly surfaces a design alternative (single guard at
JSC__JSPromise__resolve) for maintainers to weigh against the per-site idiom; that's a decision for a human, not a bot. - The PR is candid that the new tests are hardening pins, not fail-before reproductions (the fixed branch is only reachable with an already-dirty VM). REVIEW.md normally requires a fail-before test; the justification is thorough but should be accepted by a maintainer, not by me.
- My two prior inline findings on the bun-write test (temp-dir leak; Windows self-rerun byte-count mismatch) were both fixed in 15cf7ba and 32a2fb7, and the current diff reflects those fixes. Nothing else surfaced in this pass.
1 participant
What
Fuzzing (Fuzzilli) keeps hitting a flaky debug assertion whose trigger moves between modules:
It fires when a native
get()runs while a JS exception is already pending. The producer is a recurring defect class in event-loop completion code: a native completion settles a promise (or runs a completion callback), discards the result withlet _ =and a// TODO: properly propagate exception upwards, and the pending exception rides the tick until some later native property read trips the assert. On release builds the stale exception instead gets misattributed to whatever JS runs next. #37004 (open) covers this class in the DNS drains; this PR covers the following sites from the same audit:subprocess.rsexit path: theexitedpromise settle (exit code, signal, waitpid error) runs right before theonExitcallback in the same task, so a failed settle fed its exception into the user's callback invocation.ByteStream.rson_cancel: the buffer-action reject (pendingtext()/arrayBuffer()on a native stream).blob/copy_file.rsandblob/write_file.rs, both platforms:CopyFile::then/reject,WriteFilePromise::run) run inside the task dispatch channel, whose error type is the terminated sentinel; the promise FFI wrappers collapse every settle failure to that sentinel, so a thrown exception was mislabeled as termination, stopped the tick drain early, and stayed pending. These now consult the VM viareport_error_or_terminate(the dispatcher's designed reporter, previously unreachable because the label was erased upstream): a pending non-termination exception is reported and the task returnsOk, while real termination still unwinds the tick loop.CopyFileWindows::throw/resolve_promise, and theWriteFileWindowsuv callbacks, which funnel through the sameWriteFilePromise::run) are fire-and-forget; the copyfile ones report viareport_active_exception_as_unhandled.then_wrap) reports the same way.s3/multipart.rsprocess_buffered: the single-request and multipart upload starts.valkey_jsc/js_valkey.rsconnection/idle timeout timers (the drain rewrite in redis: harden connection lifecycle (Handshake enum, promise-settlement fixes, -1251 LOC) #34829 covers the socket callbacks but not these two timer arms).Fix
Check the settle result and, on failure, report the pending non-termination exception instead of discarding it:
report_active_exception_as_unhandledin fire-and-forget completions (the idiom the socket, IPC, and http2 completions already use),report_error_or_terminatein completions that return into the task dispatch channel. The error label is never trusted, because the promise FFI wrappers collapse every failure to the terminated sentinel; the VM state decides, and termination exceptions stay pending as the event loop expects.Scope
This PR does not exhaust the class. Sites with the same shape that remain, deliberately out of scope here and candidates for follow-ups in the same idiom: the
streams.rsserver-sink family (flush_promise/fulfill_promisediscards inHTTPServerWritable/NetworkSink/Writable),ipc.rs:753,node_fs.rs:1746,node_fs_watcher.rs:877,ArrayBufferSink.rs:55, and thesocket_body.rssites (owned by a separate in-flight fix).Sites audited and deliberately left alone because they are not in the class:
Sink.rsjs_close/js_end_with_sink(the generated JSSink.cpp callers checkscope.exception()immediately after, so the exception does propagate),ReadableStream.rsStrong::get/to_any_blob(held values are always real stream cells, which take the throw-free tag path), and theJSValue::thenregistrations inFileSink.rs/bun_test.rs(the wrapper debug-asserts no non-termination exception internally).An alternative worth noting for the maintainers: a single guard at
JSC__JSPromise__resolveentry (bindings.cpp) converting a resolve-with-pending-exception into a rejection would close the whole class at once, at the cost of masking caller bugs. This PR keeps the per-site idiom #37004 established instead.Why there is no failing-first test
A settle can only fail here with the VM already in an exceptional state, and none of these sites can be driven there from plain JS:
resolvePromiseonly consults user code (thenlookup) for object resolutions; when that lookup throws, JSC catches it and converts it into a rejection of the same promise (JSPromise.cpp:693), leaving the VM clean, so hostileObject.prototype.then/Promise.prototype.thenaccessors cannot make these settles return an error.Empirically: running the touched flows (subprocess exit and signal settles, locked-body
Bun.write, file-to-file copy) under a throwingObject.prototype.thenaccessor behaves identically on the unfixed release build and this branch's debug+ASAN build, settling correctly with no assert. The new tests pin that behavior so these completions stay healthy under the prototype-pollution state fuzzer processes run in; they are hardening pins, not fail-before reproductions, because the fixed branch is unreachable without an already-dirty VM.Verification
bun bd(debug+ASAN) andcargo check --release;rust:check-all(all six target combos) for the platform-gated code inwrite_file.rs/copy_file.rs.test/js/bun/spawn/spawn.test.ts,test/js/bun/io/bun-write.test.js(including the two new pollution tests, in both copyfile backends: default andBUN_FEATURE_FLAG_DISABLE_UV_FS_COPYFILE=1, on Linux and Windows),test/js/web/streams/streams.test.js,test/js/bun/s3/s3-connection-close.test.ts,test/js/valkey/reliability/connection-failures.test.tsall pass on the debug build.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/io/bun-write.test.js test/js/bun/spawn/spawn.test.ts