Repository navigation
Conversation
…form cell, not as a GC root text(), json(), bytes(), arrayBuffer(), blob() and formData() on the Response that transform() returns, before its body is read, waited on a promise that PendingValue::set_promise protected until the rewrite ended. Nothing but its input keeps a rewrite alive, so when script dropped an input that had not ended, the promise stayed a root with all that its reactions reached: the waiting closure, a disposed Bun.ModuleGraph, a retired realm of bun test --isolate. set_promise now asks the body's producer before it roots the promise (SourceHandle::hold_read_promise). RewriterPipe holds it in the transform cell's outputConsumer slot (the renamed outputStream slot, which such a body never uses) and roots it only to settle it and while a handler is parked. Value::resolve releases the root on every exit: a node:vm timeout that landed in the settle left it protected. Value::reset frees the decoder of a formData() read that was never settled.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughHTMLRewriter now coordinates pending response-body read promises with the rewrite lifecycle. Body and source handling update promise ownership and cleanup. New tests cover promise collection, settlement, interruption, and response body state. ChangesPending HTMLRewriter read promise ownership
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the main change: pending HTMLRewriter body reads move into the transform cell instead of remaining GC roots. |
| Description check | |
The description provides a detailed problem statement, implementation summary, verification results, test coverage, known limitations, and follow-up work. It does not use the exact template headings, … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description provides a detailed problem statement, implementation summary, verification results, test coverage, known limitations, and follow-up work. It does not use the exact template headings, but it contains the required information and is substantially complete.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
|
Updated 3:13 PM PT - Oct 21st, 2026
✅ @robobun, your commit 4d4441f2bf24de6a9fac5e29c97f3ac0f11531bc passed in 🧪 To try this PR locally: bunx bun-pr 46118That installs a local version of the PR into your bun-46118 --bun |
|
Status How the leak was reproduced:
PR: #46118 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked two ways the outputConsumer slot could lose the parked promise and found neither reachable: reading .body while a .text() is pending returns the in-use stream from locked_to_native_stream (has_consumer() is true from action), so on_readable_stream_available never overwrites the slot; and is_output_locked on a promise in the slot is a dynamicDowncast<JSReadableStream> miss, so it answers false rather than misbehaving. The protect/unprotect moves across root_read_promise/unroot_read_promise/fail/finish/Value::resolve pair up on each interleaving I traced (suspended-at-read, finish-or-fail during resume_rewrite, .body read first).
Extended reasoning...
The change reroutes the pending-body read promise of an HTMLRewriter output from a GC root into the Transform cell's visited slot, touching Body.rs, streams.rs and html_rewriter.rs plus three leak tests; no security-sensitive surface. Two findings were posted inline, so a human look is already signalled; this note records the slot-overwrite and double-protect paths that were traced and ruled out.
…iter-pending-body-promise # Conflicts: # src/runtime/api/html_rewriter.classes.ts # src/runtime/api/html_rewriter.rs # test/cli/test/isolation.test.ts
The row read with text() and took its label from bodyUsed, which is true from the moment the read begins. It now reads with json(), so the parse makes the settle most of the read, and it sets the timeout to land in the middle of the settle. A run counts only when the document's end handler ran after the script began and json() did not return.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| while (settled < 40 && kilobytes < 256 << 10) { | ||
| kilobytes *= 2; | ||
| rewritten = 0; | ||
| settled = Infinity; | ||
| for (let sample = 0; sample < 2; sample++) { | ||
| const { at, output } = rewrite(kilobytes); | ||
| const began = performance.now(); | ||
| const read = output.json(); | ||
| const returned = performance.now(); | ||
| rewritten = Math.max(rewritten, at.end - began); | ||
| settled = Math.min(settled, returned - at.end); | ||
| await read; | ||
| } | ||
| } | ||
|
|
||
| const before = protectedCount("Promise"); | ||
| const runs: string[] = []; | ||
| for (let i = 0; i < 4; i++) { | ||
| const cut = ((globalThis as any).rewriteCutShort = { ...rewrite(kilobytes), returned: false }); | ||
| const began = performance.now(); | ||
| try { | ||
| vm.runInThisContext("rewriteCutShort.output.json(); rewriteCutShort.returned = true;", { | ||
| timeout: Math.ceil(rewritten + settled / 2), | ||
| }); | ||
| runs.push("returned"); | ||
| } catch (error: any) { | ||
| if (error.code !== "ERR_SCRIPT_EXECUTION_TIMEOUT") throw error; | ||
| // Between the call of `end` and the return of json() there is only the settle. | ||
| runs.push(`cut short ${cut.returned ? "after" : cut.at.end > began ? "in" : "before"} the settle`); | ||
| } finally { | ||
| delete (globalThis as any).rewriteCutShort; | ||
| } | ||
| } | ||
| expect(runs).toContain("cut short in the settle"); |
There was a problem hiding this comment.
🟡 nit (optional): maintainers can get a spurious CI failure from this test, because its pass condition is a wall-clock race rather than an observable signal. The assertion at test/js/workerd/html-rewriter-leak.test.ts:797 needs at least one of 4 runs where the vm.runInThisContext deadline lands inside the settle window. That deadline is Math.ceil(rewritten + settled / 2) (line 786), measured from two warm-up samples, while each timed run first rewrites a multi-megabyte document whose duration shifts with GC pauses from the previous run's parsed array. Fix: make the classification independent of timing, e.g. loop the timed run until one lands "in" (bounded by attempts, not by a fixed 4) or drive the termination from a deterministic hook.
Why this was flagged
The test at test/js/workerd/html-rewriter-leak.test.ts:740-799 grows a JSON document until settled (the time between the document-end handler and json() returning) is at least 40 ms, then runs vm.runInThisContext 4 times with timeout: Math.ceil(rewritten + settled / 2) (line 786) and requires runs to contain "cut short in the settle" (line 797). Whether a run is classified "in" depends on where the watchdog deadline falls relative to at.end (line 791), which depends on how long the rewrite of the same document took in that run. Each timed run follows a run that parsed an array of millions of numbers; a full collection or a slower rewrite in any of the 4 runs moves the deadline out of the settle window ("before" or "after"), and if all 4 miss the assertion fails although the code under test is correct. The base branch does not have this test; the prior comment at line 799 (resolved) asked for a classification that proves the termination landed in the settle, and this timing loop is how it was answered, so the race is a consequence of that remedy that the comment did not weigh.
Verification: nit. The pass condition at line 797 (expect(runs).toContain("cut short in the settle")) requires at least one of the 4 runs (line 781) to have the watchdog deadline fire after the end handler ran and before json() returned. The deadline is a wall-clock timer: vm.runInThisContext(..., { timeout: Math.ceil(rewritten + settled / 2) }) at line 786.
Problem
.text()and the other body readers of anHTMLRewriter.transform()output stay a GC root when the input stream never ends. That keeps 6 of 6 disposedBun.ModuleGraphs (root(ProtectedValues) Promise -> FullPromiseReaction -> Object -storage-> ModuleGraph) and one realm per file underbun test --isolate.PendingValue::set_promise(src/runtime/webcore/Body.rs:456) callsprotect()until the producer settles the body. Only the collector keeps aRewriterPipealive.Fix
set_promiseasks the producer first (SourceHandle::hold_read_promise).RewriterPipeholds the promise in the transform cell'soutputConsumerslot (wasoutputStream). It roots it only to settle it, and while a handler is parked.Value::resolvereleases the root on every exit.Value::resetfrees the decoder of an unsettledformData()read.test/js/workerd/html-rewriter-leak.test.ts,module-graph-gc.test.ts,isolation.test.ts(40 new, 19 fail on main). Self-reviewed: 42 concerns raised, 31 addressed, 4 handed off, 7 not taken.Background
gcProtectmakes a value a GC root. A visited slot (internalFieldsinhtml_rewriter.bind.ts) is an edge that the collector traces.HTMLRewriterTransform) is the GC owner of onetransform(). HTMLRewriter: hold onEndTag callbacks in a visited slot, and stop using the streams of a collected rewrite #43379 listed this promise as left.dispose()(built, removed: Notes).Downsides
fetch()andBun.servereads: +2 instructions inset_promise. A waiting rewriter read: +2 slot writes, +1 slot read. Binary text: +624 bytes..bodywas read, and a read whose handler is parked on a promise that the waiting code keeps.Notes
Report. There is no GitHub issue. A leak fuzzer found it: a
Bun.ModuleGraphwhose code waits onnew HTMLRewriter().transform(response).text()while the input stream is open is never collected aftergraph.dispose(). The same holds with no graph. Merged #43379 recorded it under "Not changed": "A pending.text()leaves 100 protected Promises (promise_value.protect(),src/runtime/webcore/Body.rs:453)". It is not a regression: 1.4.3 has it.Who can hit it. Code that rewrites a stream of its own and waits for the whole result, when the stream is dropped before it ends. The weight depends on what the promise's reactions reach: a disposed
Bun.ModuleGraph(all of its modules), a retired realm ofbun test --isolateor--parallel(one per test file), a generation ofbun --hot(not measured here).The reporter's scripts (disposed graphs, or waiting objects, still alive after 40 forced collections, on release builds with
--lto=off):text(),arrayBuffer(),await text()text()through a rewritertext(), no rewriterThose scripts count with
WeakRef. The 1 of 6 is the last rewrite that the script made, and no root holds it. Taken inside the no-graph script's lastBun.gc(true), on the release build of this PR, with the script unchanged (a preload wrapsBun.gc):The first block is
generateHeapSnapshotForDebugging(). The second is a read of the blocked process's stack (/proc/<pid>/mem, frame-pointer walk,llvm-symbolizer). The two words are slots ofrunInternalMicrotaskthat the resume of the script's async function does not write: they still hold the controller of the last rewrite's input, an argument of an earlier microtask. The same count reads 0 of 6 on a debug build. The tests here use the heap snapshot (Lifetimes.stillAlive) or a bound of N / 4.Shapes (N = 6 each, judged with the heap snapshot):
text(),json(),blob(),bytes(),arrayBuffer(),formData()text(), the callback also holds the output Responseread()pending onout.bodynew Response(out.body).text()Bun.write(file, out)Protected Promises per 1,000 pending reads, after a full collection: main +1000 for each of the six readers, this PR +0. With a handler parked on a promise that script keeps: +1000 on both. After those handlers resolve, with the input still open: main +1000, this PR +0.
How the promise moves.
hold_read_promiseputs it in the cell unless the rewrite is over or a handler is parked. The body is thenLockedwith itsactionset and nopromise, sobodyUsed, a second read,.body,clone()andBun.writeanswer as before.root_read_promisemoves it intoPendingValue.promiseand protects it in three places: at the start offail(), infinish()before the body is replaced, and inbegin_suspension(). The last one exists becauseabandon_suspension()has to reject the read after the cell is collected.unroot_read_promisemoves it back when the handler has resumed and no other handler is parked.Why the cell is alive whenever the rewrite can end. A native input (a fetch body, a file) roots its stream wrapper while it has a sink, and the wrapper's
sinkOwnerslot holds the cell. The JS pump's reaction holds the cell. A queued background pull protects it. The context of a parked handler's promise holds it, and in that window the promise is a root anyway.Measurements (release builds of main 91e7e3b and of this PR,
--lto=off, linux-x64, withllvm-objdump,llvm-size, andgdbhit counts on the stripped binary). They are from before the merge with main 2d6d445 and were not taken again. That merge moved the cell's declaration tohtml_rewriter.bind.ts(#46128), where a slot read is an inline load and not a call:PendingValue::set_promise, instructionsBun.servebodycmpb,jneon the producer tag)Value::resolve, instructionsValue::to_error_instanceBun.serveawait req.text()x 10,000: protect / unprotectbegin_transform(atransform()that nothing reads)RewriterPipeallocationHTMLRewriterTransformcell (snapshot stride)RewriterPipe::on_start_bufferingResponse::destroy, body not pendingRewriterPipe::fail/finish, instructionsData and bss do not change.
valgrind,perf,straceandbloatyare not installed on the machine that measured, so there are no instruction counts of whole runs.Tests. 40 new. On main's source 19 fail (a release build of 91e7e3b, and a debug build of 2d6d445): the 11 "is not a GC root" rows (six readers, a handler that parked and resumed, four reads begun inside the rewrite's own handlers), the
node:vmtimeout row, theBun.ModuleGraphtest (13 graphs, each withroot(ProtectedValues) Promise), and the sixbun test --isolaterows (8 live globals, the bound is 4). The other 21 guard what must stay and pass before and after: a read is still settled when only the input is held, by a handler that parked and resumed, by the abandonment of a handler that parked after the read began or after another one resumed, by a failure whose inputcancel()begins the read, by a file, by a fetch in flight, when one of the rewrite's own handlers begins it, and through the stream once.bodywas read.The
node:vmrow reads withjson(), so the parse makes the settle most of the read, and it sets the timeout to land in the middle of the settle. A run counts only when the document'sendhandler ran after the script began andjson()did not return: between those two there is only the settle. With the guard inValue::resolveremoved and the rest of this PR in place, the row fails as on main (Expected 0, Received 4), and 8 of 8 runs of the same code are cut short in the settle.Suites run on the debug (ASAN) build, again after the merge with main 2d6d445:
html-rewriter-leak.test.ts(82 pass, 1 skip),module-graph-gc.test.ts(38) and the leaked-handles group oftest/cli/test/isolation.test.ts(15), each under the environment of the ASAN lane (detect_leaks=1,BUN_DESTRUCT_VM_ON_EXIT=1,BUN_JSC_validateExceptionChecks=1).html-rewriter.test.js,html-rewriter-end-error.test.ts,html-rewriter-doctype.test.ts,htmlrewriter-additional-bugs.test.ts,text-chunk-null-access.test.ts(212 together).fetch/body.test.ts,body-mixin-errors.test.ts,body-clone.test.ts,body-async-iterator.test.ts,FormData.test.ts(1100 together),body-stream.test.ts(9086),serve-stream-body-error.test.ts(34).module-graph-isolation.test.tsandworker-terminate-lifetime.test.ts, andtest/internal/source-lints(182).html-rewriter-leak.test.ts(6.6 s to 7.1 s each), and now and then one test ofmodule-graph-gc.test.ts. They pass with a longer timeout, as the ASAN lane sets.The two changes in Body.rs beside
set_promise.Value::resolvetook the promise and unprotected it only on its last line, after eight?. A termination takes one of them, and anode:vm{ timeout }termination is withdrawn afterwards, so the VM lives on with the root. On main, 4 of 4 reads that a timeout cut short stay protected. The guard isjsc::js_value::Protected::adopt, as instreams.rs.Response::destroyresets the body and frees the Response without dropping the body, so the decoder of aformData()read that was never settled was leaked (LeakSanitizer: 640 bytes in 40 objects for the newformData()row).Value::resetnow frees it. The release is a cold function, soresetis still inlined intoResponse::destroy.The first version had a second half, which is removed. It armed an
AbortHandlefor a read thatBun.ModuleGraphcode started, and ended the rewrite when that graph's context stopped. A review of it found, and reruns confirmed:dispose(). A host that kept only the promise ofgraph.run(() => out.text())then got its input stream cancelled and the promise pending for ever. Main fulfils it.cancel()callback of the graph's own stream ran insidedispose()on this path.fail()made a rewrite that failed twice report its first error. Main reports the second. The tail offail()is main's text again.cancel()insidefail()went to the cell afterfail()had taken the promise, and stayed pending (8 of 8).hold_read_promisenow refuses once the rewrite is over, and 8 tests pin it.Not changed.
Bun.ModuleGraphit keeps the graph (6 of 6 in 4 of 9 shapes). Ending such a read with its graph belongs to the read inBody.rs, for fetch andBun.servebodies too, and only fordispose(). That is a separate change..bodywas read, a pendingread(),new Response(out.body).text()andBun.readableStreamToText(out.body): their roots are the output stream's pull promise andBufferAction.promiseinByteStream. Fetch relies on the first.Bun.write(file, out): the root is the file sink's controller, and it reproduces with no rewriter.JSPromiseStrong.finish(). This is on main and does not need a pending read.The second, third and last of these are handed off as separate work.
Self-review, what was not taken. Holding the promise in place through a weak handle, and asking the producer at settle time in
Value::resolve, were both proposed to close the read-inside-fail()window. Thephase == Donetest inhold_read_promisecloses it with no new handle and no change to the settle path. Adebug_assertfor that invariant was not added. A Body-level stop for all producers and a reader context onPendingValueare the separate change named above. The review also noted that a host can wrap the output asnew Response(out.body, out)today to avoid the graph form, and that open PRs touch the same lines (below).Overlap with open PRs. #44259 makes
PendingValue.producera private type:self.producer.hold_read_promise(..)inset_promiseand thematches!(locked.producer, ..)inroot_read_promiseneed a small rebase in whichever lands second. The rewrite ofbegin_suspensionfrom #44759 reached main in #46021. This branch is merged with it: the one call that this PR adds there follows main'sdebug_assert. The tail offail(), which #43521 and #44259 rewrite, is untouched here.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/workerd/html-rewriter-leak.test.ts