jsc: remove AbortSignal::unref/detach; add abort_signal::PendingActivityRef as the owned hold on a signal - #37609
jsc: remove AbortSignal::unref/detach; add abort_signal::PendingActivityRef as the owned hold on a signal#37609robobun wants to merge 9 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe change replaces raw ChangesAbort signal ownership
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
|
Updated 9:05 AM PT - Aug 12th, 2026
❌ @robobun, your commit 00b27e5 has some failures in 🧪 To try this PR locally: bunx bun-pr 37609That installs a local version of the PR into your bun-37609 --bun |
|
Status: reproduced on main as an API-level hole (the snippet in the description compiles with no Merge order relative to the in-flight PRs, none of which blocks this one:
CI: the diff is green. Build 93117 (head 00b27e5, a re-run of 94e81cc with no code change) ran 179 jobs and every one that ran passed; the build is marked failed only because the two |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it restructures AbortSignal refcount release across six memory-safety-critical subsystems (spawn, http2, Bun.serve, WebSocket upgrade, fetch, FetchTasklet) and coordinates with three in-flight PRs, a human look would still be worthwhile.
Checked: release order vs. clean_native_bindings/pending_activity_unref is preserved at every site; the one intentional swap (h2 SignalRef::Drop now derefs parser before the signal field drops) is benign since the listener is already removed and ~AbortSignal runs no callbacks. Verified Cell<Option<AbortSignalRef>> on ServerWebSocket is only ever .take()n (no Copy-requiring .get()), JsCell::set/replace on Subprocess.abort_signal correctly drops the prior None, and the spawn scopeguard removal is covered by AbortSignalRef's own Drop on every early return. Each unsafe { adopt(...) } site's SAFETY comment traces to the matching ref_()/new().
Extended reasoning...
Overview
This PR deletes the safe AbortSignal::unref(&self) and AbortSignal::detach(&self, ctx) methods and routes every release of a WebCore::AbortSignal ref through AbortSignalRef (ExternalShared<AbortSignal>)'s Drop. Ten files: the FFI handle itself, six runtime call sites (http2 stream signals, Bun.spawn option parsing + Subprocess storage, Bun.serve request-context release, WebSocket upgrade handover + ServerWebSocket storage, fetch's SignalRef guard, FetchTasklet::clear_abort_signal), and a new source-lint test that pins each of four release shims to a single declaration + a single call inside its owner's release hook.
Security risks
None user-facing. The change is purely about who is allowed to call the release shim from Rust; no attacker-reachable input shapes any of these paths differently than before. The risk category is memory safety (double-free / UAF / leak) if a release site's ownership proof is wrong.
Level of scrutiny
High. This is the repo's most-blocked review category (native memory safety, reference-count balance across FFI). Field types change on two GC-finalized structs, one drop ordering is intentionally swapped, and every converted site now carries an unsafe { adopt(...) } whose SAFETY comment states the +1 provenance. I traced each site against ExternalShared::{adopt, Drop, Deref} and JsCell::{get, set, replace} semantics: the spawn early-return path (scopeguard branch removed) is covered by the local Option<AbortSignalRef> dropping; ServerWebSocket.signal as Cell<Option<AbortSignalRef>> is sound because only Cell::new/Cell::take are used (no .get()); Subprocess.abort_signal_ref() returning a borrow tied to &self is fine at its one caller (spawnSync timeout probe) which does not re-enter; RequestContext::signal_release and FetchTasklet::clear_abort_signal preserve the original pending_activity_unref → unref / clean_native_bindings → pending_activity_unref → unref orderings via scope-exit drop. The h2 SignalRef::Drop reorder (parser deref now runs before the signal field's implicit drop) matches the PR description's justification and is not observable — the abort listener was removed on the line above and ~AbortSignal on the C++ side does not call back into Rust.
Other factors
The PR description is unusually thorough (per-site rationale, verification against the abort-signal leak fixture and every affected test suite under debug+ASAN, cross-platform cargo check). The new source-lint test is a reasonable ratchet and correctly guards against an empty scan. That said, three coordination points (#32009, #37549, #37577) mean a maintainer should confirm merge order, and the sheer breadth of refcount-lifecycle edits across core runtime paths puts this outside what I'd auto-approve without a human maintainer's sign-off.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Should we instead have a AbortSignal::PendingActivityRef as an Option? And that holds both the signal ref / unref and cleaning native bindings?
|
Yes, that is a better shape. What the native holders actually own is a bundle: the ref, a pending-activity count, and (for the ones that listen rather than fire) one native listener keyed by a ctx pointer. This PR made only the first of those RAII and left the other two as hand-written sequences at each release site, which is the same class of thing it set out to remove. Reworking it as One thing that fell out of picking the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
|
Pushed the rework described above (f713940, plus a comment trim in 87848df); title and description updated to match.
The remaining multi-line comments flagged by the comment lint are the rustdoc for the new type, the note on why |
There was a problem hiding this comment.
Both earlier inline findings are addressed in fb968d4 — the dead second abort_signal.take() block on the spawnSync path is gone, and the new spawn-signal test now asserts signal.aborted === false after proc.exited instead of branching on it. This pass found no further issues. Given the scope — a new PendingActivityRef abstraction with load-bearing Drop ordering, threaded through six AbortSignal holders across FFI (Subprocess, RequestContext → ServerWebSocket, Response, fetch/FetchTasklet, h2) — a human look is still worthwhile before merge.
What was reviewed:
- Refcount balance at each converted holder: every prior
unref()/pending_activity_unref()/clean_native_bindings()sequence maps to aPendingActivityRefdrop or anAbortSignalRef::adoptat the same program point. set_signal_abortedtaking its ownsignal_ref()clone so re-entrant abort listeners emptying the cell don't invalidate the borrow.- The h2
SignalRefdrop-order change (parser deref now afterclean_native_bindings, before theAbortSignalReffield drop) — neither release observes the other. - The source-lint test's exact-count assertions and the tracked-files/realpath guards.
Extended reasoning...
Overview
The PR removes AbortSignal::unref(&self) and AbortSignal::detach(&self, ctx) — safe methods on an opaque ZST handle that could release a ref the caller did not own — and routes all releases through AbortSignalRef's Drop (ExternalSharedDescriptor::ext_deref). It introduces abort_signal::PendingActivityRef, an owned bundle of (ref, pending-activity count, one native listener) whose Drop releases all three in a fixed order, and converts Subprocess.abort_signal, RequestContext.signal → ServerWebSocket.signal, and Response::BodyAbortListener to hold it as Option<PendingActivityRef>. FetchTasklet, fetch's SignalRef guard, and the h2 SignalRef are minimally updated to AbortSignalRef::adopt at their existing release points. Two tests are added: a source lint pinning that each release shim is called only from its RAII owner's release hook, and a spawn test for the one observable change (an AbortSignal.timeout() handed to spawn keeps its timer after the child exits).
Security risks
None identified. This is internal refcount/lifetime plumbing; no new untrusted-input parsing, no auth/crypto/permission surface. The change reduces the surface for accidental double-release by removing the safe unref() escape hatch.
Level of scrutiny
High. This is native memory-safety code — intrusive C++ refcounts across FFI, GC pending-activity accounting, and Drop ordering that affects whether an AbortSignal.timeout() timer is cancelled — spread across six holders in production paths (spawn, Bun.serve request/WebSocket, fetch Response, http2). REVIEW.md flags this as the most-blocked category. The design was already steered by a maintainer (the PendingActivityRef shape), and verification was thorough (ASAN debug build, the abortsignal-leak fixture, multiple targeted test suites), but the breadth and the interaction with three in-flight PRs (#37549, #37420, #32009, plus the #37577 lint overlap) make it inappropriate to approve without a human look.
Other factors
My two prior nits (a dead code block in spawn_maybe_sync and a vacuous-pass window in the new test) were both fixed in fb968d4. The comment-cop bot flags were all resolved (trimmed or justified). The remaining unsafe uses are narrowly scoped AbortSignalRef::adopt calls at documented +1 transfer points, each with a SAFETY comment naming where the ref came from.
…tSignalRef `AbortSignal::unref(&self)` and `detach(&self, ..)` released the C++ intrusive refcount from a shared borrow. `AbortSignal` is an opaque ZST handle, so `&AbortSignal` is reachable from any pointer and from every `AbortSignalRef`'s Deref; safe code could therefore release a ref it did not own (double release on the owner's Drop). Delete both methods so the only release path is dropping an `AbortSignalRef`, and adopting a raw `+1` into one stays `unsafe`. Callers: - h2 `SignalRef` and `Subprocess.abort_signal` / the spawn locals now hold an `AbortSignalRef`; `ServerWebSocket.signal` too, adopting the request context's ref at the upgrade hand-off. - `RequestContext::signal_release`, fetch's `SignalRef` guard and `FetchTasklet::clear_abort_signal` adopt the raw `+1` they already own at the release point. Add a source lint pinning release shims of this kind (`WebCore__AbortSignal__unref`, `JSC__ArrayBuffer__deref`, `NapiEnv__deref`, `CookieMap__deref`) to their owner's release hook.
…umers keep on a signal Bundles what a native operation holds on an AbortSignal while in flight: the ref, the pending-activity count, and the native listener it registered (keyed by ctx). Drop removes the listener, then drops the pending-activity count, then releases the ref. Holders store it as Option<PendingActivityRef> and release by taking it out. Converted: Subprocess.abort_signal, RequestContext.signal (the WebSocket upgrade now moves the hold into ServerWebSocket.signal instead of adopting a raw pointer), and Response's BodyAbortListener, which was this struct written out by hand. AbortSignal::new() returns an AbortSignalRef since every caller wrapped it. The Drop order removes the listener before dropping pending activity. Subprocess did the reverse, which let cleanNativeBindings see an unobserved signal and cancel an AbortSignal.timeout() the caller was still holding; spawn now behaves like fetch here, with a test.
…condition The abort listener is attached unconditionally before the sync-only tail of spawn_maybe_sync, so the second attach inside watch_or_reap's Ok arm could never see a signal. The new spawn-signal test now asserts the child exited before the timer fired instead of silently skipping the wait.
fb968d4 to
e11190c
Compare
|
Rebased onto current main: the only conflicts were the upgrade hand-off lines in server_body.rs against #37477's |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/api/bun/subprocess.rs`:
- Around line 364-368: Update abort_signal_ref to return an owned cloned
AbortSignalRef rather than a borrowed &AbortSignal, cloning the value obtained
from JsCell::get() and adjusting its signature and callers as needed while
preserving None when no signal is attached.
In `@test/js/bun/spawn/spawn-signal.test.ts`:
- Around line 140-147: Increase the timeout window used by both spawn signal
tests, selecting a larger TIMEOUT_MS when isASAN or isDebug is enabled and
retaining the existing default otherwise. Import isASAN and isDebug from harness
alongside bunEnv and bunExe, and use AbortSignal.timeout(TIMEOUT_MS) in both
tests. Keep expectTimeoutAfterExit’s exitCode and signal.aborted preconditions
unchanged.
🪄 Autofix
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: df8a4e53-69ff-4b78-85b2-fbe04d403225
📒 Files selected for processing (13)
src/jsc/AbortSignal.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/runtime/api/bun/subprocess.rssrc/runtime/server/RequestContext.rssrc/runtime/server/ServerWebSocket.rssrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/runtime/webcore/Response.rssrc/runtime/webcore/fetch.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/internal/source-lints/refcount-release-owner.test.tstest/js/bun/spawn/spawn-signal.test.ts
There was a problem hiding this comment.
I re-reviewed after fb968d4/e11190c and both earlier nits are addressed (dead second attach block deleted; the timeout test now asserts signal.aborted === false before waiting and covers spawnSync). No new issues found this run.
Given the scope — refcount/pending-activity/listener lifetime rework across Subprocess, RequestContext→ServerWebSocket, Response, h2, and the fetch adopt sites, plus an intentional Drop-order behavior change and merge-order coupling with #37577/#37549/#32009/#37420 — a human pass over the ref balance at each converted holder is still worthwhile.
What was reviewed
PendingActivityRefDrop order vs.eventListenersDidChangetimer cancel; matches the fetch order and is pinned by the new spawn/spawnSync tests.- Each converted holder releases at the same program point as before (take-then-drop replaces the hand-written unref sequences);
set_signal_abortedclones a ref before firing so re-entranton_abort/finalize_without_deinitemptying the cell can't UAF. ServerWebSocket.signalmoved fromCell<Option<NonNull>>toCell<Option<PendingActivityRef>>— checked thaton_close'stake()+ scopeguard +as_deref()reads andfinalize's box drop cover all release paths.- The source-lint test's exact-count assertion and
FN_HEADERscan look correct for the four shims listed.
Extended reasoning...
Overview
This PR removes the safe AbortSignal::unref(&self) / detach(&self, ctx) methods (which allowed safe code to release a ref it did not own) and replaces the hand-written ref/pending-activity/listener release sequences at each native holder with a new RAII type, abort_signal::PendingActivityRef. Converted holders: Subprocess.abort_signal, RequestContext.signal (moved into ServerWebSocket.signal on upgrade), and Response::BodyAbortListener. FetchTasklet, fetch's SignalRef, and h2's SignalRef are updated to adopt into AbortSignalRef at the release point but not fully converted (documented follow-ups pending #37549/#37420). AbortSignal::new() now returns AbortSignalRef directly. Three NonNull shim helpers in RequestContext.rs are deleted. A dead second attach block in the spawnSync tail (my prior finding) is removed. Two tests are added: a source lint asserting the four release shims are called only from their RAII owner's release hook, and a spawn/spawnSync case for the one behavior change (timeout signal still fires after child exit).
Security risks
None identified. This is internal refcount plumbing; no user-input parsing, auth, or crypto is touched. The change closes a soundness hole (safe double-release) rather than opening one.
Level of scrutiny
High. Per REVIEW.md, native memory safety around intrusive refcounts across FFI is the most-blocked category. Every converted site must release exactly the same counts (ref, pending-activity, listener) at exactly the same program points as before, on every path including error/abort/re-entry. The Drop order in PendingActivityRef is load-bearing (swapping the two lines regresses the new test). The set_signal_aborted change from borrowing the cell to cloning a ref before firing is specifically there because abort listeners can synchronously re-enter and empty the cell — that's the kind of subtlety a human should confirm across all the fire-then-drop sites (on_abort, finalize_without_deinit, ServerWebSocket::on_close).
Other factors
- The design shape (
PendingActivityRef) was suggested by a maintainer and iterated on in-thread; the final revision hasn't had a human review pass. - Merge-order interactions with four in-flight PRs are documented but require whoever merges to be aware of them.
- My two prior nit-level findings were addressed exactly as suggested; the follow-up commit e11190c extended the test to spawnSync.
- Verification in the description is thorough (ASAN debug build, leak fixture with per-request signal creation, the affected test suites) but self-reported.
- The comment-cop bot flagged several multi-line comments; the author trimmed most and justified the four that remain (rustdoc, load-bearing order note, the "why no unref" note). Those justifications read as correct to me.
Use a child that does not boot a JS VM, give debug/ASAN builds a 3s window instead of 1s, and run the two cases concurrently so the wider window is paid once.
There was a problem hiding this comment.
Thanks — 94e81cc addresses the timeout-window concern well (the --version child + 3s debug/ASAN window + describe.concurrent is a better fix than what I'd proposed, since it stays under the 5s per-test ceiling). No further findings this run. Given the scope — refcount/pending-activity/listener lifecycle rewired across spawn, RequestContext, ServerWebSocket, http2, Response and fetch, plus one intentional behavior change to the Drop order — a maintainer look is still worthwhile.
What was reviewed:
PendingActivityRef::dropordering vs.eventListenersDidChangetimer cancellation, and that every converted holder releases at the same program point as before.- The upgrade hand-off (
RequestContext.signal→ServerWebSocket.signal) andset_signal_abortedtaking its ownsignal_ref()so re-entrant abort listeners emptying the cell can't dangle. abort_signal_ref()now returning an ownedAbortSignalRef(dfa1b41) — the spawnSync caller no longer holds aJsCellborrow across anything that could clear it.- The h2
SignalRef::dropreorder (parser deref before signal ref drop) — neither release observes the other.
Extended reasoning...
Overview
This PR removes AbortSignal::unref(&self) / detach(&self, ctx) — safe methods that could double-release a C++ intrusive refcount — and replaces the hand-balanced (ref, pending-activity count, native listener) triple at each holder with a new RAII type, abort_signal::PendingActivityRef, whose Drop releases all three in a fixed order. Converted holders: Subprocess.abort_signal, RequestContext.signal (moved to ServerWebSocket.signal on upgrade), and Response::BodyAbortListener. FetchTasklet, fetch's SignalRef, and h2's SignalRef are minimally adapted (adopt-at-release) pending in-flight PRs that replace those fields. AbortSignal::new() now returns AbortSignalRef directly. A new source-lint test enforces that the four intrusive-release FFI shims are only called from their RAII owners' release hooks.
Security risks
None identified. This is an internal memory-ownership refactor with no user-input parsing, auth, or network-protocol surface changes. The one behavior change (spawn's AbortSignal.timeout() now keeps running after the child exits, matching fetch) is a bug fix, not a security-relevant change.
Level of scrutiny
High. Per REVIEW.md, native memory safety is the most-blocked review category, and this PR rewires refcount / pending-activity / listener lifecycle across six subsystems (spawn, server request context, server WebSocket, http2, Response, fetch). Each converted site must release at exactly the same program points as before, on every path including error/abort/finalize. The unified Drop order is an intentional behavior change whose correctness depends on eventListenersDidChange semantics on the C++ side. This is not a mechanical change a bot should approve alone.
Other factors
All three of my prior inline findings (dead second attach block, vacuous test precondition, tight timeout window) were addressed across fb968d4 / dfa1b41 / ad5b7ec / 94e81cc, and both CodeRabbit threads are resolved. The PR has strong test coverage (source lint fails on main, spawn-signal timeout cases time out on main, leak fixture verified with balanced counts under ASAN), the design was suggested by a maintainer per the description, and the verification section lists the relevant abort/leak/upgrade suites as passing. The bug hunting system found nothing this run. Deferring rather than approving because the breadth of memory-lifecycle changes across critical paths warrants a human maintainer's sign-off.
There was a problem hiding this comment.
All three rounds of prior feedback are addressed (94e81cc's --version child + 3s debug/ASAN window + describe.concurrent is the right fix — my 10s suggestion would have overshot the 5s per-test ceiling), and this run found no bugs. Given the scope — refcount/pending-activity/listener lifecycle rewired across Subprocess, RequestContext→ServerWebSocket, Response, http2 and fetch, a new public PendingActivityRef, and one intentional observable change to AbortSignal.timeout drop ordering — a maintainer sign-off is still worthwhile.
What was reviewed:
PendingActivityRef::droporder (listener → pending-activity → ref) and that every converted holder releases at the same program point as before.- The RequestContext→ServerWebSocket signal move on upgrade, and
set_signal_abortedcloning its own ref before firing listeners that can empty the cell. - http2
SignalRef::dropreordering (parser deref now precedes signal drop) — neither release observes the other. - The new source-lint's declaration/owner-call counting against the four release shims.
Extended reasoning...
Overview
Removes AbortSignal::unref/detach (safe &self methods that could double-release) and routes the FFI release shim exclusively through AbortSignalRef's Drop. Introduces abort_signal::PendingActivityRef — an owned bundle of (ref, pending-activity count, one native listener) whose Drop releases all three in a fixed order — and converts Subprocess, RequestContext/ServerWebSocket, and Response::BodyAbortListener to hold Option<PendingActivityRef>. FetchTasklet, fetch's SignalRef, and http2's SignalRef are minimally adapted (adopt-then-drop) pending sibling PRs. Adds a source-lint test pinning each release shim to its single owner, and two spawn/spawnSync tests for the observable drop-order fix.
Security risks
None identified. This is an internal memory-ownership refactor; no user-controlled input parsing, auth, or boundary validation is touched.
Level of scrutiny
High. This is native memory-safety code in REVIEW.md's most-blocked category: intrusive C++ refcounts balanced across FFI, GC pending-activity, and listener lifecycle, spread across 11 source files with a hand-off (RequestContext→ServerWebSocket on upgrade) and one intentional behavior change. The design was maintainer-suggested (Jarred-Sumner), so a maintainer confirming the converted holders and the deferred ones (fetch/fs left for #37549/#37420) match intent is appropriate.
Other factors
Three prior automated review rounds each surfaced a nit that was fixed: the unreachable second spawnSync attach block was deleted (fb968d4), the timeout tests gained a hard precondition assertion (fb968d4), and the timing window was widened for debug/ASAN with a lighter child and concurrent execution (94e81cc). CodeRabbit's two findings (owned abort_signal_ref return, timeout scaling) were also addressed (dfa1b41, 94e81cc). The author ran the leak fixture (7,650 requests, balanced counts) and the full spawn/server/websocket/http2/fetch abort test set on a debug+ASAN build. All review threads are resolved. No bugs were found in this run.
2 participants
What
bun_jsc::AbortSignal(src/jsc/AbortSignal.rs) is anopaque_ffi!zero-sized handle to aWebCore::AbortSignal, which isWTF::RefCounted;WebCore__AbortSignal__unrefisderef()and frees the object at count zero. The handle exposed that as two safe&selfmethods:A
&AbortSignalproves nothing about owning a ref: it is whatAbortSignalRef(ExternalShared<AbortSignal>) hands out throughDeref, and for a ZST handle it is also obtainable from any non-null pointer via the safeAbortSignal::opaque_ref. So this compiles with nounsafeanywhere and is a double release (theAbortSignalRefreleases the same ref again when it drops):Nothing in tree did this; every caller released a ref it had taken with
ref_()/new(). But none of those sites had to say so, because the API did not ask. The same was true of the two things the callers release alongside the ref: the pending-activity count and the native abort listener were balanced by hand at every site.Fix
unrefanddetachare deleted. The release shim is now reached only fromExternalSharedDescriptor::ext_deref, i.e. fromAbortSignalRef'sDrop, the shapeJSCArrayBuffer,NapiEnvandCookieMapRefalready use: taking a ref is safe, releasing one means dropping the owner, and turning a raw+1into an owner (AbortSignalRef::adopt) is the oneunsafestep.ref_documents this so the method does not grow back;AbortSignal::new()returns anAbortSignalRefsince every caller wrapped it.Per @Jarred-Sumner's suggestion, what the native holders keep is modelled as one owned value,
abort_signal::PendingActivityRef(the Rust spelling ofAbortSignal::PendingActivityRef): it owns anAbortSignalRef, takes the pending-activity count innew, records the ctx of the one native listener registered through itsadd_listener, derefs to&AbortSignal, and itsDropremoves the listener, drops the pending-activity count and releases the ref. Holders storeOption<PendingActivityRef>and release by taking it out. Converted:Subprocess.abort_signal; the spawn code parses the option into anAbortSignalRef(ref_from_js; its ownDropcovers the early returns, replacing a scopeguard branch) and wraps it when the listener is attached.clear_abort_signalis adrop. The second attach block in the spawnSync tail was unreachable (the first one runs unconditionally and empties the option) and is deleted.RequestContext.signal, created fromAbortSignal::new()withsignal_ref()giving theRequestits copy;on_abort/finalize_without_deinittake it out, fire it, and drop it, and the WebSocket upgrade moves it intoServerWebSocket.signal, whichon_closefires and drops (whatever is left goes with the box infinalize). The threeNonNullshims in RequestContext.rs go away.set_signal_abortedfires through asignal_ref()of its own because the listeners it runs can end the request and empty the cell.Response'sBodyAbortListener, which was this struct written out by hand.Holders left as they are, each with a one-line reason:
FetchTasklet/ fetch'sSignalRefguard adopt the+1they already own at the release point, because #37549 is replacing those fields (on top of this PR itsunref()deletion is a no-op conflict) and they becomeOption<PendingActivityRef>in one line once it lands; the twonode:fsholders are the same story with #37420; the http2SignalRefandfs.watch'sFSWatcher(untouched by this PR, it never usedunref()) hold anAbortSignalRefplus a listener but take no pending activity today (a native listener already counts as a timeout observer), so giving them aPendingActivityRefwould be a GC-behavior change and is left for a follow-up. Once those are converted,pending_activity_ref/unrefandclean_native_bindingscan stop being public.Behavior
Everything releases at the same program points as before. The one observable change is the unified
Droporder.cleanNativeBindingsrunseventListenersDidChange, which cancels anAbortSignal.timeout()timer if the signal has no observers left at that moment;Subprocessdropped its pending-activity count first and removed its listener second, so on main this never aborts:PendingActivityRef::dropremoves the listener while the pending-activity count still counts as an observer (the orderFetchTaskletandResponsealready used), so spawn and spawnSync now match fetch;spawn-signal.test.tsgets a case for each, both of which time out on main (fetch's own order is unchanged, so a fetch case would pass on both sides; it is worth adding whenFetchTaskletconverts). The eager cancel itself also hits a held timeout signal whose last JS listener is removed, and is being fixed separately on the C++ side. The http2SignalRefnow releases its parser ref before the signal ref rather than after; neither release can observe the other.Tests
test/internal/source-lints/refcount-release-owner.test.tslists the release shims whose pointee has an RAII owner (WebCore__AbortSignal__unref,JSC__ArrayBuffer__deref,NapiEnv__deref,CookieMap__deref) and asserts each is declared once and called only from that owner's release hook, with exact counts so a renamed shim surfaces instead of dropping out. On main it fails withsrc/jsc/AbortSignal.rs:167 (in fn unref): WebCore__AbortSignal__unref(self)and the other three entries pass; with this change all pass. Complementary to bun_url: make freeing a whatwg::URL an unsafe raw-pointer operation #37577's lint, which allowlists this instance; whichever lands second drops that line.test/js/bun/spawn/spawn-signal.test.ts: the two timeout cases above, run concurrently. Each asserts the child exited before the timer fired (the child isbun --version, which does not boot a JS VM, and debug/ASAN builds get a 3s window instead of 1s) and then waits for the abort event; on a released bun both time out after the preconditions pass.Verification
Rebased onto current main (the upgrade hand-off now sits next to #37477's
UpgradeState); the rebased branch was rebuilt and the runs below repeated on it.cargo check/clippy/fmt --checkclean forbun_jscandbun_runtime, also forx86_64-pc-windows-msvc; debug (ASAN) build. Against it:spawn-signal(incl. the spawnSyncAbortSignal.timeoutcase, which reads the hold throughabort_signal_ref()),spawn-maxbuf,websocket-upgrade-signal-gc(the ctx to socket move),websocket-serverclose/upgrade cases,bun-server.test.ts(only the threelocalhostconnection tests fail, identically with the released binary in this sandbox),serve-pending-promise-abort-leak,serve-async-stream-client-abort,node-http2.test.js -t "abort|signal",fetch-abort-queued,fetch-abort-stream-bodypass.abortsignal-leak-fixture.tsrun directly (7,650 server requests creating and releasing aPendingActivityRefeach, 5,100 of them aborted) ends with 1 / 2 / 1 liveAbortSignals after a full GC, so the counts balance. Itsbun testwrapper andfetch-abort-socket-close-raceonly exceed their 5s ceilings here because this container is running at load average ~70; the latter passed at 4.1s earlier on a build whose fetch code was byte-identical.[review] gate passed · iteration 0 · 13 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file