Repository navigation
AbortSignal: emit write barrier when storing abort reason - #32747
Conversation
AbortSignal::markAborted and AbortSignal::jsReason stored m_reason via JSValueInWrappedObject::setWeakly without emitting a write barrier. JSAbortController::visitChildren visits signal().reason() but had no visitOutputConstraints, so once the concurrent collector had scanned a controller, a later controller.abort(err) on the mutator thread was invisible to the GC: the controller was never rescanned and the reason Error was left unmarked. BUN_JSC_verifyGC=1 catches this as 'cell ... was not marked' and without the verifier the reason is collected while the controller is still reachable, so signal.reason silently becomes undefined (and any downstream consumer of it sees a stale or freed cell). Match upstream WebKit (https://bugs.webkit.org/show_bug.cgi?id=236353): store m_reason via set() so a write barrier fires on the AbortSignal wrapper, and give JSAbortController visitOutputConstraints so the DOMGCOutputConstraint re-visits it after the mutator has run. Also null check the owner in JSValueInWrappedObject::set, since wrapper() is null before the JS wrapper is created.
|
Updated 3:48 AM PT - Jun 26th, 2026
❌ @Jarred-Sumner, your commit 1598cf2 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32747That installs a local version of the PR into your bun-32747 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAbortSignal now stores abort reasons with context-aware GC barriers, JSAbortController adds GC visitation hooks for ChangesAbortController GC reason handling
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/js/web/abort/abort-controller-gc-reason.test.ts`:
- Around line 13-18: Shorten the explanatory comment in the abort-controller GC
test to fit the 3-line comment limit. Collapse the GC-verifier rationale into a
tighter summary, or move the extra background about setWeakly(),
visitOutputConstraints, and the scanned-after-abort case into the PR description
instead. Keep the remaining comment focused on the key reason the test exists
and reference the abort reason/Error marking behavior in
abort-controller-gc-reason.test.ts.
🪄 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: a9f63604-df27-45de-81ae-285e4d6a8edd
📒 Files selected for processing (5)
src/jsc/bindings/webcore/AbortSignal.cppsrc/jsc/bindings/webcore/JSAbortController.cppsrc/jsc/bindings/webcore/JSAbortController.hsrc/jsc/bindings/webcore/JSValueInWrappedObject.htest/js/web/abort/abort-controller-gc-reason.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues, but this touches JSC concurrent-GC write-barrier / output-constraint plumbing (and JSValueInWrappedObject explicitly says changes need a JavaScriptCore reviewer), so this should get a human look.
Extended reasoning...
Overview
This PR fixes a missing GC write barrier when storing AbortSignal::m_reason. It changes AbortSignal::markAborted / jsReason to call JSValueInWrappedObject::set(vm, wrapper(), value) instead of setWeakly, adds visitOutputConstraints / visitAdditionalChildrenInGCThread to JSAbortController so its IsoSubspace is registered into outputConstraintSpaces() (verified in BunClientData.h:213-216), null-checks owner in JSValueInWrappedObject::set, and adds a verifyGC/collectContinuously regression test.
Security risks
None — this is GC marking correctness, no parsing, auth, or external input handling. The risk profile here is memory-safety / use-after-free if the barrier logic is wrong in either direction, which is a stability concern rather than an attack surface.
Level of scrutiny
High. JSValueInWrappedObject.h itself carries the comment "This class includes a lot of GC related subtle things… Any changes on this class must be reviewed by JavaScriptCore reviewers too." The change follows the exact same pattern as existing classes (JSMessagePort, JSCustomEvent, JSEventEmitter) and matches the upstream WebKit fix referenced in the description, so it looks correct, but concurrent-GC barrier semantics are subtle enough that a human familiar with Bun's JSC integration should sign off.
Other factors
The header/cpp additions mirror established codegen patterns; subspaceForImpl<T> automatically picks up the new visitOutputConstraints to enroll the subspace in the DOMGCOutputConstraint set. The new test follows repo conventions (combined toEqual assertion, 120s timeout for collectContinuously, Windows skip). CodeRabbit's only nit (comment length) was addressed in 007db39. CI shows build-rust failures across all Linux/FreeBSD/Android shards, which look like an unrelated infra/toolchain issue rather than something caused by these C++ changes.
There was a problem hiding this comment.
I didn't find any issues, but this touches concurrent-GC write barriers and JSValueInWrappedObject (which is explicitly flagged as "must be reviewed by JavaScriptCore reviewers"), so it warrants a human look.
Extended reasoning...
Overview
This PR fixes a missing GC write barrier when storing AbortSignal::m_reason. It changes four C++ files in src/jsc/bindings/webcore/ (AbortSignal.cpp, JSAbortController.{h,cpp}, JSValueInWrappedObject.h) and adds a regression test. markAborted/jsReason now call m_reason.set(vm, wrapper(), reason) instead of setWeakly(), JSAbortController gains visitOutputConstraints/visitAdditionalChildrenInGCThread so its IsoSubspace is registered in outputConstraintSpaces() (verified in BunClientData.h:213-216), and JSValueInWrappedObject::set null-checks owner before the barrier.
Security risks
This is memory-safety-adjacent: the bug being fixed is a concurrent-GC marking miss that can lead to a freed cell being read back via signal.reason. The fix itself follows the upstream WebKit pattern (https://bugs.webkit.org/show_bug.cgi?id=236353) and looks correct — the write barrier targets the JSAbortSignal wrapper, the output-constraint registration covers the controller-only case, and the owner null-check degrades to the prior setWeakly behavior when no wrapper exists yet. I don't see a new vulnerability introduced, but the consequence of any subtle error here (missed mark, premature collection, UAF) is severe.
Level of scrutiny
High. JSValueInWrappedObject.h carries an in-source warning: "This class includes a lot of GC related subtle things, and changing this class easily causes GC crashes. Any changes on this class must be reviewed by JavaScriptCore reviewers too." Concurrent-GC write-barrier and output-constraint changes are exactly the class of edit that benefits from a second pair of expert eyes, even when the diff looks right and mirrors upstream.
Other factors
- The new
visitOutputConstraintsfollows the same pattern asJSMessageEvent,JSErrorEvent,JSEventTarget, etc. in the same directory, andsubspaceForImplwill pick it up automatically via the function-pointer comparison. - A targeted regression test is included (verifyGC + collectContinuously + parallel markers) and the PR description reports it fails 5/5 pre-fix and passes 5/5 post-fix.
- CI shows
build-rustfailures across all Linux/FreeBSD/Android shards on the latest run; these look like unrelated infrastructure breakage (this PR touches no Rust), but the build isn't green yet. - CodeRabbit's only comment (3-line comment limit) was addressed in 007db39.
|
CI on build 64914 is red on three lanes, none related to this diff. For reference:
Nothing in this PR touches HTTP serving, sendfile, MySQL, or the install proxy. All 23 |
Searched before reporting: this is oven-sh/bun#34636, closed as completed, fixed by oven-sh/bun#32747 — `AbortSignal::markAborted` stored the reason without a GC write barrier, so a concurrent collector that had already scanned the signal wrapper could take the reason while the signal stayed alive. No issue filed; it would have duplicated a fixed one. The fix landed after v1.3.14 and is not in the pinned toolchain, so the accessor stays — with the condition for deleting it written down instead of left for someone to rediscover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-silent closure (#982) Fixes the failure modes behind one audited chat session: **7 runs, $4.68, and nothing delivered to the user.** A `report.html` was produced two minutes after the turn was killed and announced nowhere. Two defects of modelling explain almost all of it: 1. **A chat turn was treated as a synchronous transaction** while orchestrating jobs longer than itself. 2. **A run was treated as returning a value** (results transit the LLM's context) **when it produces an artifact** (results should transit by reference). ## What changed | Area | Change | | --- | --- | | Turn closure | A turn killed by its deadline no longer ends in silence: a real persisted text part + a new `finishReason: "deadline"`. Enforced on **both** engines. | | Step cap | `CHAT_MAX_STEPS` was reported and never applied on the Pi engine. It now stops the tool loop and spends the last step on one tool-less answer. | | Turn budget | `run_and_wait` derives its wait from an absolute turn deadline instead of silently taking a 30-minute default three times longer than the turn. A launch with too little budget left is refused — nothing created, nothing spent. | | Fan-in | `context_documents` mounts previous runs' `document://` URIs by reference. A prompt naming a document the input does not mount is still refused (400) — explicitly, not repaired. | | Prompt | Fan-out/fan-in contract, effort ceilings, deliver-before-continuing, and `integration_not_active` recovery. **+7 net lines** (`+9 / -2`). | | Integrations | An `integration_not_active` 412 is routed to `activateIntegration` instead of a retry — the platform's RBAC decides who may clear it. | | Runner | An agent that ends without calling `output` gets exactly one corrective turn while its context is still alive. | | Orphan runs | A run finishing after its turn posts a notice into the chat session. | ## Notes for the reviewer **Auto-activation on connect was built, then removed** (`92be77616` reverts `88df5ee54`). It first shipped with a privilege escalation — a plain member (holding `integrations:connect`) could install an integration application-wide (admin-only `integrations:install`) — fixed by threading the capability as a fail-closed claim across the two session-less connect surfaces. That fix was correct but exposed the real cost. The threading is intrinsic, not accidental: the decision point is authenticated (the `/connect/*` request) while the action point is not (the OAuth callback is stateless; the hosted Connect portal has no org member at all), so any auto-activation must carry the capability across that gap — and re-deriving it there *was* the escalation. 278 lines of production code across 8 files plus a signed token claim, to save one API call. Its reach was also narrower than it looked: env-backed SYSTEM integrations are already active without an install row, so it only ever covered org-installed ones. And the obvious cheaper alternative — refuse the connection until the integration is active — is not viable, because `integrations:connect` is a member right while activation is admin-only; it would make member self-connect impossible until an admin acted first. What replaces it is strictly better: `activateIntegration` is already in the platform's operation surface, so the prompt sends the model there and **the platform's own RBAC decides** — an admin clears the 412 in one step, a member gets a 403 and is told to ask one. Same one-step UX where the right exists, zero capability pre-computed or transported, and the escalation class is structurally gone rather than guarded. **An adversarial security pass found three more.** Two of them turned out to be arguments against the code that produced them, and that code is now gone (see below): an unbounded ACL-probe fan-out in the prompt-URI repair path (~8 300 concurrent queries from one request), and a run-result spill locatable by name and therefore impersonable by an agent. The third — agent-chosen document names rendered unquoted into a persisted `assistant` message — is fixed and stays. **Two claims in this branch were falsified during review and corrected rather than re-documented.** Namespacing the spill document's name did not prevent collision — the dedup key is `(run_id, sha256, name)`, so a decoy and the real spill would coexist. And the budget note advertised a launch threshold the gate does not apply. **Two features were built, then removed** (`f6ec12465`), on the same test their author should have applied first: what does this cost, and what does the evidence say it buys? - **Auto-repair of `document://` URIs pasted into a prompt.** It turned inert prompt text into a mount primitive — its own comment said "prompt injection can now cause a mount" — and needed that ACL fan-out bounded. What it replaced was a recoverable 400 that already existed and is still wired. The reason a model pasted URIs into prose is precisely what `context_documents` plus the prompt paragraph fix, so the branch was paying twice: shipping the instruction that prevents the mistake AND the machinery that papers over it. - **The run-result spill document.** The 32 KB truncation stays — nothing else bounds how much of `runs.result` reaches a chat context. The COPY goes: the full payload is already durable in `runs.result` and already readable through `getRun`, so a dedicated `agent_output` duplicate bought only a `document://` handle, and locating it by name is what opened the impersonation hole that then needed a publish-boundary refusal to close. Removing the spill also fixed a defect in it: truncation was conditional on the spill existing, and the spill was best-effort (org quota, per-run cap) — so the guard disarmed itself exactly when a result was large enough to matter. It is unconditional now, with a test for the run-published-nothing case the old path skipped. Accepted regression: a >32 KB result no longer produces a downloadable chip. Deliverables belong in the run's `outputs/`, which the prompt already mandates. **One diagnostic in the originating audit was wrong.** `stepCount: 20` vs `maxSteps: 16` was not evidence of 20 model calls: the counter counted messages, and pi emits `message_start` for the user prompt and every tool result. The turn ran out of time, not steps. The counter is now split. **Two SDK behaviours forced the design and are worth knowing:** - `streamText` never publishes a `finish` part on abort — it enqueues `{type:"abort"}` and closes. Metadata alone could not carry the outcome, so the notice and the finish chunk are injected in-band via a `TransformStream.flush()`. - `signal.reason` can read back `undefined` while `signal.aborted` stays `true` in the pinned Bun. Reading it later would have silently downgraded the deadline to an untagged stop — reintroducing the exact silence this PR removes. This is [oven-sh/bun#34636](oven-sh/bun#34636), already fixed upstream by [#32747](oven-sh/bun#32747) (a missing GC write barrier in `AbortSignal::markAborted`) in a release later than the one pinned here. The trigger is simply that nothing else holds the reason object — not the timer, not a forced GC; a synchronous `abort(new Error())` loses it too. The workaround carries a removal condition in the code. ## Deliberately not built No retry for a notice missed when a newer turn happens to be generating at the instant an orphan finalizes — that needs a queue, a worker or a table. The deliverable stays reachable from the run page and the documents gallery; the limit is documented in the module header. The 32 KB result truncation is a **context safety net, not the fan-in fix** — the audited results were 9-11.6 KB and would not have tripped it. It earns its place by bounding an otherwise unbounded surface, not by fixing the incident. The fan-in fix is `context_documents` plus the prompt's reference rule. ## Verification - `bun run check` — 33/33 tasks, OpenAPI all checks passed, type-coverage 99.04% - `bun test` — **9049 pass, 46 skip, 0 fail** across 693 files - CI **Integration tests** and **E2E tests** both run on this PR (labels `integration` + `e2e`), not just post-merge ### Live smoke test (done) Run against a local instance on this branch, real models, both engines — the gap the previous revision of this section flagged as advisable before merge. | Path | Result | | --- | --- | | ai-sdk turn with tools (deepseek) | `finishReason: "stop"`, `stepCount: 3`, correct answer from real MCP tool output | | subscription turn with tools (Pi / claude-code) | `finishReason: "stop"`, `stepCount: 2`, `lastToolName: invoke_operation` | | A5 budget note — ai-sdk | model quotes it verbatim: `10m00s left … step 0/16 … needs at least 2m15s` | | A5 budget note — subscription | rides the tool result, `step 1/16` — the split counter reads correctly | | **Turn deadline (the blocker fix)** | deadline forced to 30 s: turn cut at 33 s, `finishReason: "deadline"`, notice injected, **both persisted in `chat_messages`** | | `run_and_wait` refusal | non-error payload, model relays it, **zero `runs` rows created** | **The smoke test found a real bug**, fixed in 56eb8c1: the refusal message carried the same wrong threshold this PR had already fixed in the budget note, and rendered the post-reserve run budget as the turn's remaining time — contradicting `remaining_ms` in its own payload. The arithmetic now lives in one exported constant both texts read. Two observations, deliberately not changed: the subscription model treated the `[turn budget]` line as a possible prompt injection before complying (it rides an unlabelled tool result — prompt design, not a defect); and `turnDeadlineNoticeText` renders "1 minutes" via `Math.max(1, …)`, unreachable at the 10-minute production constant. Still not covered by automated tests: the live subscription path (needs the Pi SDK, a real token and a model). The step cap's `terminate` mechanism remains verified against the SDK source and a fake session. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Problem
AbortSignal::markAbortedstoredm_reasonviaJSValueInWrappedObject::setWeakly()without emitting a write barrier (the old code had aFIXMEadmitting it).JSAbortController::visitChildrenvisitssignal().reason()but had novisitOutputConstraints, so once the concurrent collector had already scanned a controller, a latercontroller.abort(err)on the mutator thread was invisible to the GC: the controller was never rescanned and theErrorpassed as the reason was left unmarked.JSC's GC verifier catches this directly:
Without the verifier the observable effect is that
signal.reasonsilently becomesundefinedwhile the controller is still reachable (theWeak<>inJSValueInWrappedObjectis cleared). Any downstream consumer that holds the controller then reads a freed-and-possibly-reused cell.Reproduction
On current
mainthis either asserts insideHeap::verifyGCor printslost > 0.Fix
Match upstream WebKit (https://bugs.webkit.org/show_bug.cgi?id=236353):
AbortSignal::markAborted/AbortSignal::jsReason: storem_reasonviaset(vm, wrapper(), reason)so a write barrier fires on theJSAbortSignalwrapper.JSAbortController: addvisitOutputConstraints/visitAdditionalChildrenInGCThreadso its subspace is registered as an output-constraint space and the DOMGCOutputConstraint re-visits the controller after the mutator has run. This covers the case where noJSAbortSignalwrapper exists yet (only the controller).JSValueInWrappedObject::set: null-checkownerbefore the barrier, matching upstream, sincewrapper()is null before a JS wrapper is created.Verification
bun bd test test/js/web/abort/: 13 pass / 0 failbun bdbuild (verifier asserts) and passes 5/5 with the fix.