HTMLRewriter: stream the rewrite instead of buffering the whole document

[robobun]

Fixes #19305
Fixes #6068

Problem

HTMLRewriter.transform(response) fully buffers the source body before running lol-html, then does a single write() + end(). The output Response's pending body also has none of the stream callbacks fetch bodies install, so no consumer that wants a stream is ever told about the ByteStream it is reading. Three user-visible consequences, all reproducible with a plain streaming upstream:

// upstream: a Bun.serve that emits "<p>1</p>", waits, emits "<p>2</p>", closes
const rw = () => new HTMLRewriter().on("p", { element(e) { e.setAttribute("x", "1"); } });
const res = rw().transform(await fetch(upstream));

1. Handlers only fire when the source closes, never as chunks arrive. An element handler on the first chunk fires at T = upstream-end, not at T = first-chunk. Memory is proportional to the document, and the TTFB of a transformed proxy response equals the full upstream download time.
2. res.body yields an empty, closed stream: the .body getter creates a ByteStream the producer never learns about (on_readable_stream_available is not set), then done()'s Value::resolve sees the orphan readable and closes it empty. Every .body consumer (a reader, new Response(res.body), S3.write) gets "". That is S3Client writes empty file for HTMLRewriter transformed fetch Response #19305.
3. Bun.serve returning the transformed response hangs forever: the server sees Locked { task: Some }, calls Value::to_readable_stream, and pipes a ByteStream nobody ever writes to. HTMLRewriter + new Response(Bun.file) causes Bun.serve to think a non-Response is returned #6068 is the Bun.file() shape of the same bug (a file-backed body is read asynchronously, so the server always gets the pending body first).

Rebased twice: onto #32927 / #32840, then onto #33048

An earlier revision of this PR also fixed the source-stream error swallowing (ValueBufferer::on_stream_pipe treating StreamResult::Err as a clean zero-byte end), the ValueBufferer half of #31964. #32927 landed that fix first and more completely (it also covers the parked-error short-circuit, the synchronous transform() error shape, and a Value::Error guard on every body reader), so in the rebase main's version wins and this PR's copy is removed. This PR no longer claims #31964, and its two source-stream-error tests are deleted as exact functional duplicates of #32927's merged suite (verified: both pass on post-#32927 main). What remains is purely the streaming change this PR was filed for.

One merged test is adapted, and it is the part of the resolution to review. #32927's a read already pending on .body when the upstream fails rejects starts a single reader.read() before the upstream fails and asserts that that read rejects. That is only true of a fully buffered implementation, where nothing can reach .body until the whole input has arrived. With this PR the bytes that arrive before the failure are rewritten and delivered immediately (that is the point), so the pending read is legitimately satisfied by a chunk of rewritten output first. The test now reads in a loop, which protects the same invariant its author named ("the error must reach the attached stream; a read must never be stranded") and is strictly stronger: it still fails if the stream closes cleanly (the exact bug #32927 fixed), still fails on a hang, and passes only when a read eventually rejects with the upstream error. I verified the invariant holds before changing anything: on this branch a subsequent read does reject with the connection error. Every other test #32927 and #32840 added passes unmodified.

Separately, the post-rebase soak surfaced a real, pre-existing HTTP-client race that is independent of this PR. An async element handler runs inside wait_for_promise (handler_callback has always spun the full event loop for an async handler; that is not introduced here). The two async-handler tests' "independent round trip" fetched the same host:port as the still-streaming chunked source, and about 1 in 20 full-file runs got a Malformed_HTTP_Response whose raw bytes were the previous response's terminal chunked trailer (0\r\n\r\n) prepended to the next response: the source's connection returns to the pool while the nested loop is spinning, before its trailer is drained, and the handler's fetch reuses it. The two tests now give that round trip its own sidecar server so it cannot share a pooled connection with the source under test; their purpose (prove the HTTP-client thread is not parked) is unchanged. After the change: 0 failures in 110 consecutive runs (50 full-file, 60 filtered). The underlying client bug (a chunked keep-alive connection re-pooled with an unconsumed trailer under a re-entrant event loop) is worth a separate issue.

#33048 (requested in review on this PR) then landed, and this diff is now re-expressed on top of its lol_html Rust-crate bindings. The streaming design turned out to be orthogonal to the bindings: of the fourteen functions it adds, eleven never touch lol-html and ported verbatim, and only three places changed shape. write_chunk now destructures the typed per-call Result<(), RewritingError> and builds the JS error from it directly; finish's flush uses the crate's consuming end(self) (null the field, heap::take(rewriter).end()); and Drop uses heap::destroy. Body.rs needed nothing (#33048 never touched it). This is also what resolves the original review concern on this PR: the extern "C" trampolines, the opaque-handle FFI, and lol-html's take-once thread-local error buffer that this diff previously had to reason about are all gone from the code it is built on.

One honest finding from re-verifying on the new bindings: on the asynchronous (mid-stream) path, a throwing handler's JS error value is not recoverable by the time the failure is reported, because the VM's exception-capture slot is only armed for the synchronous transform() window, so the reported message is lol-html's generic one. That is byte-for-byte main's behavior on its own asynchronous path (which formats the same RewritingError display), so it is preserved here, not introduced, and it is why the concurrent-failure test asserts the absence of cross-contamination rather than each handler's literal message. Recovering the thrown value on the asynchronous path is a worthwhile follow-up, independent of streaming.

Fix

Input side (Body.rs): ValueBufferer gains an optional per-chunk callback. When set, body bytes are delivered incrementally and on_finished_buffering only ever signals completion or an error.

HTMLRewriter (html_rewriter.rs): BufferOutputSink registers that callback and feeds each source chunk into HtmlRewriter::write. The output body's PendingValue gets on_start_streaming (hands over any already-rewritten bytes as the new stream's initial buffer) and on_readable_stream_available, the same lazy contract FetchTasklet uses. The second hook stashes the consumer's new ReadableStream into the output Response cell's stream WriteBarrier slot (values: ["stream"] in response.classes.ts, visited by the generated visitChildrenImpl); the sink holds no reference of its own and reads it back through Response::get_body_readable_stream. The cell slot, not the body Value, is the right owner because Bun.serve's do_render_with_body moves the stream out of the body and replaces the Value with Used. Once a consumer streams, OutputSink::write pushes rewritten bytes straight into that stream and done() closes it. With no streaming consumer, done()'s buffered Value::resolve path is unchanged, so .text() / .json() and string / blob inputs behave exactly as before.

The pipe callback must never drive lol-html inline. ValueBufferer's pipe handler runs inside FetchTasklet::on_progress_update, which holds the tasklet's mutex until ByteStream::on_data returns, and the singleton HTTP-client thread takes that same mutex before delivering the next chunk. HtmlRewriter::write can suspend on an async element handler spinning the full event loop (vm().wait_for_promise), which would park the HTTP thread (and every in-flight fetch) on that mutex for the whole await, and deadlock outright if the handler's own await needs it. lol-html is also not re-entrant, so a chunk or source end arriving during that spin must not re-enter it. So:

• The asynchronous delivery paths copy the chunk into a queue on the sink and schedule a drain as an event-loop task (an AnyTask embedded on the sink + VirtualMachine::enqueue_task, the FetchTasklet shape). A task runs on a fresh stack, strictly after the producer's critical section ends, and at the top of an event-loop turn. It must be a task and not a microtask: an async handler's wait_for_promise drains the microtask queue, and doing that from inside a microtask callback would re-enter the microtask drain.
• drive_rewriter sets writing for the duration of its lol-html calls; chunks, the source's end, or an error arriving while a write is suspended are queued / deferred and drained by that suspended call's own loop and tail.
• The synchronous paths (transform(string/blob) and a source that is already complete) drive lol-html directly: no producer lock exists there, which preserves transform()'s synchronous contract for non-streaming inputs. The pre-buffered prefix of a still-streaming source goes through the deferred path like every other pipe delivery: the slice handoff stays synchronous (no user JS runs while it is borrowed), and no &mut ValueBufferer is ever live when a handler spins the event loop and re-enters the pipe.

Lifetime: the sink's only GC reference is a JsRef on the output Response wrapper, strong for the sink's active lifetime (the pattern JsRef documents and ServerWebSocket / UDPSocket use); the cell's visitChildrenImpl keeps everything else reachable. It holds no JSC::Strong. In the other direction, the output body Value (and any ReadableStream a consumer holds on it) can outlive the sink, so Drop for BufferOutputSink nulls every PendingValue back-pointer into the sink (detach_from_output_body) and a late .body / .text() can never invoke on_start_streaming on freed memory.

Three more corrections the per-chunk input made load-bearing:

• done() no longer overwrites a body already in the Error state with a partial InternalBlob.
• The error for a failed write is built at the failing call rather than when the source ends. The typed RewritingError exists only on that call's frame, and the pending JS exception a throwing handler may have left is consume-once, so neither can be read later (at source end) without having been lost or consumed in between. The JS error is carried as a stack value and reported from that same drive_rewriter invocation; it is never stored on the sink.
• end() is skipped when an earlier write already failed (a handler threw mid-stream and the source then ended cleanly). lol-html poisons the rewriter after a failed write and panics (Attempt to use the HtmlRewriter after a fatal error) if it is used again. A source that ends with an error never reaches end() at all; that path is HTMLRewriter: reject the transformed body when the upstream body fails #32927's and is unchanged here.
• A write failure on a still-open source is reported to every consumer immediately instead of when the source eventually ends. A failed write poisons the rewriter and drops every later chunk, so there is nothing left to wait for, and a .text() / .body / Bun.serve consumer of a long or never-ending upstream would otherwise hang. The eventual source end then has nothing left to do; finish on a poisoned sink returns without touching the rewriter.

The task deferral, the re-entrancy guard, the four corrections above, the removal of every JSC::Strong from the sink (in favor of the Response cell's WriteBarrier graph plus one JsRef), and the replacement of the raw-pointer (*sink).field access pattern with three disjoint RefCell domains (so every entry point forms only a shared &Self, and an aliasing violation is a deterministic panic instead of UB) were all surfaced by review on this PR.

Not included: JS / direct ReadableStream sources (#11758, #14216) still return ERR_STREAM_CANNOT_PIPE. That path needs the disabled ArrayBufferJSSink pump, a separate mechanism.

Verification

Ten new tests in test/js/workerd/html-rewriter.test.js, in a describe.concurrent("HTMLRewriter streaming") block plus a sequential describe("HTMLRewriter streaming with async handlers") for the two whose handlers spin the event loop (a nested wait_for_promise is hostile to concurrently running server / fetch tests). Each is gate-driven: the upstream refuses to emit its final chunk until the behavior under test has already been observed, so a buffer-everything implementation deadlocks deterministically rather than flaking, and the gate also guarantees the source is never already complete when transform() runs.

• element handlers fire on the first chunk while the source is provably still open
• .body of a transformed streaming response yields the rewritten output
• Bun.serve streams a transformed response through to the client
• Bun.serve streams a transformed Bun.file() response through to the client (HTMLRewriter + new Response(Bun.file) causes Bun.serve to think a non-Response is returned #6068)
• a Bun.serve proxy delivers rewritten bytes to the client before the upstream ends
• an element handler throwing mid-stream rejects an already-streaming .body reader
• a handler throw rejects consumers WITHOUT waiting for the source to end: the only test whose upstream gate stays shut for the whole assertion, so the immediate failure report is the only thing that can settle the consumer (pre-fix it fails by timing out)
• two concurrent transforms whose handlers both fail mid-stream each report their own error
• an async element handler on a chunk delivered mid-stream does not stall the HTTP client: the chunk containing the matched element is gated until after transform() has returned (forcing the pipe path), and the handler then awaits an independent round trip through the HTTP client, which can never complete if the rewriter is driven from inside the fetch tasklet's critical section
• an async element handler that spans a source chunk boundary still produces correct, in-order output (the re-entrancy guard)

With this change all ten pass, and at the final revision (re-expressed on the #33048 bindings) the full file passes 0 failures across 80 consecutive runs (40 of the whole file, 40 filtered to the streaming describes), on top of the earlier 110 and 80 clean runs at the prior revisions. With src/ reverted to post-#33048 main (the gate's fail-before base) exactly these ten fail, deterministically, by timeout or a missing-first-chunk assertion, and every pre-existing test in the file, including everything #32927, #32840, and #33048 added, still passes.

Existing suite and soaks

Re-run after the rebase, on a debug + ASAN build:

• Full HTMLRewriter suite (5 files, 87 tests): 0 fail, 2 todo (the pre-existing JS-ReadableStream it.todos, intentionally unchanged), 1 skip.
• test/js/web/fetch/{body,body-stream,body-clone,body-mixin-errors,body-async-iterator,blob,content-length}.test.* (this PR's Body.rs change now merges with HTMLRewriter: reject the transformed body when the upstream body fails #32927's Body.rs and body-reader changes): 9489 pass, 0 fail.
• 110 consecutive runs of html-rewriter.test.js (50 of the whole file, 60 filtered to the streaming describes): 0 failures.

Run on the pre-rebase revision; the streaming and lifetime code they exercise is unchanged by the rebase resolution:

• ASAN soak: 200 iterations across 6 consumer shapes (.text(), fully-drained .body, abandoned reader, dropped Response, Bun.serve proxy, mid-transform cancel) with Bun.gc(true) every 5: clean.
• ASAN soak: 160 iterations across the re-entrancy shapes (handler grabs out.body.getReader() from inside lolhtml::write, handler throws mid-stream with a .body reader attached, reader cancelled from inside a handler, handler throws with a .text() consumer): clean.

Related PRs

Related PRs touching the same code. This one was originally written against main without any of them.

• HTMLRewriter: deliver the transformed output to consumers of the response's body stream #32956 registers the same on_readable_stream_available hook and delivers the output as a single terminal chunk at done() time. That fixes the .body-empty and Bun.serve-hang symptoms, but the input is still fully buffered and the output still arrives only at end-of-stream. This PR supersedes that mechanism; the two cannot merge cleanly. HTMLRewriter: deliver the transformed output to consumers of the response's body stream #32956's separate Bun.file() size-sentinel fix (non-seekable or missing files report their length as u64::MAX and trip a lol-html preallocation assert) is a distinct pre-existing bug, is not made more reachable here, and is not duplicated here.
• HTMLRewriter: reject the transformed body when the upstream body fails #32927 (now merged) is the deeper fix for the error swallowing: it also covers the has_received_last_chunk parked-error short-circuit, surfaces the real body error from the synchronous transform() branch, and adds a Value::Error guard to every body reader. The one change both made (the Err arm in on_stream_pipe) is removed from this PR in the rebase in favor of main's; see the rebase section above.
• Rework LinearFifo uninit accessors, replace stream raw-pointer/static-mut forges, thread JS errors through body bufferer #31835 reworks ValueBufferer::run's error return (exception fidelity, a different concern). Semantically independent; textually conflicts with the new ValueBufferer field and init signature.

[coderabbitai]

Walkthrough

Adds lazy streaming output to HTMLRewriter by extending ValueBufferer with a per-chunk callback and rewriting BufferOutputSink to forward rewritten bytes directly into a consumer ByteStream as they arrive, with microtask-based draining, re-entrancy guards, and streaming-aware error propagation. New streaming integration tests cover TTFB, concurrent error isolation, and async handler correctness. Two documentation files receive minor formatting fixes.

Changes

HTMLRewriter lazy streaming output

Layer / File(s)	Summary
ValueBufferer per-chunk delivery contract and routing src/runtime/webcore/Body.rs	Introduces ValueBuffererChunkCallback type and on_chunk field on ValueBufferer, extends init to accept it, adds deliver() helper routing bytes through on_chunk or the finish callback, and updates all body-arrival paths (run, on_finished_loading_file, on_stream_pipe, buffer_locked_body_value, on_receive_value, PipeHandler::on_pipe) to use deliver().
BufferOutputSink state and init wiring src/runtime/api/html_rewriter.rs	Adds output_stream, write_error, writing, drain_scheduled, pending_input, and pending_finish fields to BufferOutputSink; initializes them in init; installs on_start_streaming and on_readable_stream_available hooks on the output Response; wires on_input_chunk_trampoline as the ValueBufferer chunk callback.
Input chunk handling, microtask drain, and rewriter driving loop src/runtime/api/html_rewriter.rs	Implements on_input_chunk_trampoline/on_input_chunk (queue, async-defer, or sync drive), schedule_drain/drain_microtask for microtask-based queue consumption outside producer critical sections, and drive_rewriter/write_chunk as the core lol-html driving loop with poison-on-first-failure semantics.
Terminal completion, error propagation, and output detachment src/runtime/api/html_rewriter.rs	Rewrites on_finished_buffering and finish to defer under re-entrancy via pending_finish; adds fail_output_stream and report_rewriter_failure to reject the consumer ByteStream on error; updates detach_from_output_body/done() for safe teardown; forwards rewritten bytes to ByteStream in write when streaming; calls detach_from_output_body in Drop.
Streaming integration tests test/js/workerd/html-rewriter.test.js	Adds concurrent and sequential HTMLRewriter streaming test suites asserting early element-handler firing, transformed body streaming, Bun.serve proxy TTFB, failure isolation across concurrent transforms, mid-stream handler error rejecting an attached body reader, and async handler correctness across chunk boundaries. Updates the upstream-failure body reader test to use a continuous read loop.

Documentation formatting fixes

Layer / File(s)	Summary
Base64 and Web APIs doc reformatting docs/guides/util/base64.mdx, docs/runtime/web-apis.mdx	Re-renders the btoa/atob legacy example inside an explicit ```ts fenced block and reflows the Web APIs markdown table header/separator without changing content.

Possibly related PRs

• oven-sh/bun#32927: Modifies BufferOutputSink/ValueBufferer buffer-delivery and error propagation so upstream body failure rejects the transformed output rather than closing cleanly — directly adjacent to the streaming pipeline changes here.
• oven-sh/bun#33040: Also modifies docs/guides/util/base64.mdx around the legacy btoa()/atob() warning snippet.

Suggested reviewers

• alii
• cirospaciari

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The docs formatting updates in docs/guides/util/base64.mdx and docs/runtime/web-apis.mdx are unrelated to the HTMLRewriter streaming fix.	Move the documentation-only formatting edits to a separate PR, or remove them from this change set.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly states the main change: HTMLRewriter now streams rewrites instead of buffering the full document.
Linked Issues check	✅ Passed	The changes address both linked issues by streaming transformed responses and exposing the rewritten output to consumers and Bun.serve.
Description check	✅ Passed	The description covers the PR purpose and verification thoroughly, with only non-template headings and extra detail.

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 10:20 PM PT - Jul 1st, 2026}

❌ @robobun, your commit a23c3a4 has 5 failures in Build #67768 (All Failures):

• test/js/bun/terminal/terminal.test.ts - code 1 on 🍎 14 x64
• test/js/bun/resolve/load-same-js-file-a-lot.test.ts - code 1 on 🐧 3.23 aarch64
• test/js/bun/resolve/load-same-js-file-a-lot.test.ts - code 1 on 🐧 3.23 x64
• test/js/node/zlib/zlib.test.js - code 1 on 🐧 25.04 aarch64
• test/js/node/zlib/zlib.test.js - code 1 on 🐧 13 aarch64
• test/js/node/zlib/zlib.test.js - code 1 on 🐧 25.04 x64-baseline
• test/js/node/zlib/zlib.test.js - code 1 on 🐧 25.04 x64
• test/js/node/zlib/zlib.test.js - code 1 on 🐧 3.23 aarch64
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64-baseline
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64
• test/bake/dev/production.test.ts - code 1 on 🪟 2019 x64
• test/bake/dev/production.test.ts - code 1 on 🪟 11 aarch64
• test/bake/dev/production.test.ts - code 1 on 🪟 2019 x64-baseline
• test/bake/dev/production.test.ts - code 1 on 🐧 13 x64-asan
• test/bake/dev/production.test.ts - code 1 on 🐧 25.04 aarch64
• test/bake/dev/production.test.ts - code 1 on 🐧 13 aarch64
• test/bake/dev/production.test.ts - code 1 on 🐧 25.04 x64-baseline
• test/bake/dev/production.test.ts - code 1 on 🐧 13 x64-baseline
• test/bake/dev/production.test.ts - code 1 on 🐧 25.04 x64
• test/bake/dev/production.test.ts - code 1 on 🐧 13 x64
• test/bake/dev/production.test.ts - code 1 on 🐧 3.23 aarch64
• test/bake/dev/production.test.ts - code 1 on 🐧 3.23 x64-baseline
• test/bake/dev/production.test.ts - code 1 on 🐧 3.23 x64
• test/bake/dev/production.test.ts - code 1 on 🍎 14 x64
• test/bake/dev/production.test.ts - code 1 on 🍎 14 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 32988

That installs a local version of the PR into your bun-32988 executable, so you can run:

bun-32988 --bun

[github-actions]

Found 1 issue this PR may fix:

1. HTMLRewriter + new Response(Bun.file) causes Bun.serve to think a non-Response is returned #6068 - HTMLRewriter + new Response(Bun.file) causes Bun.serve to return the default response instead of the transformed one; this PR fixes the streaming output path through Bun.serve

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #6068

🤖 Generated with Claude Code

[github-actions]

This PR may be a duplicate of:

1. HTMLRewriter: deliver the transformed output to consumers of the response's body stream #32956 - Also fixes HTMLRewriter .body returning an empty stream (S3Client writes empty file for HTMLRewriter transformed fetch Response #19305) by delivering transformed output to body stream consumers; HTMLRewriter: stream the rewrite instead of buffering the whole document #32988 explicitly supersedes this PR
2. HTMLRewriter: reject the transformed body when the upstream body fails #32927 - Also fixes HTMLRewriter error swallowing on upstream stream failure (Piped stream errors are swallowed: body buffering resolves with truncated data instead of rejecting #31964) by properly propagating StreamResult::Err in ValueBufferer::on_stream_pipe

🤖 Generated with Claude Code

[robobun]

Thanks, both bot findings are addressed.

The #6068 suggestion was correct, and it also pointed at a gap: a Bun.file() body is a file-backed Blob the bufferer reads asynchronously, a different source path (on_finished_loading_file) from the piped fetch upstreams the other tests cover, and this PR changed that path without exercising it. Added an eighth test (Bun.serve streams a transformed Bun.file() response through to the client) and Fixes #6068 to the description. Verified the repro directly: on main the request times out after the server's idle timeout; with this change it returns the rewritten document in ~56ms.

The duplicate-PR finding matches the "Related PRs" section in the description: #32956 and #32927 each fix one symptom of the same root cause. This PR supersedes #32956's mechanism (but not its separate Bun.file() size-sentinel fix) and overlaps #32927 on exactly one change (the Err arm in on_stream_pipe, required here so the new output streaming cannot forward truncated data as a clean end). Neither does the actual streaming. Cross-linked on both.

[claude]

The microtask-deferral fix and its dedicated test address my last concern; I have nothing further, but given the volume of new unsafe pointer/refcount/re-entrancy machinery here a human pass is still warranted.

Extended reasoning...

Overview

This PR rewrites HTMLRewriter.transform(response) to stream both input and output instead of buffering the whole document. It adds ~400 lines of new logic to src/runtime/api/html_rewriter.rs (lazy output-stream hooks on the pending body, a per-chunk lol-html driver with a re-entrancy guard, microtask-deferred draining to avoid running lolhtml::write inside FetchTasklet's critical section, eager error capture, and Drop-time back-pointer detachment), extends ValueBufferer in src/runtime/webcore/Body.rs with an optional per-chunk callback and proper StreamResult::Err handling, and adds ~500 lines of gate-driven tests.

Security risks

No auth/crypto/permission surface. The risk axis is memory safety: the new code is dense with unsafe, raw *mut Self field accesses across FFI re-entry, manual refcounting (ref_/adopt), JsCell aliasing, and lifetime coupling between the sink and a GC-rooted Response. Four issues in this space (a UAF on the prefix slice, lol-html re-entry, a take-once thread-local error race, and a producer-mutex deadlock) were raised on earlier revisions and have all been addressed in the current diff — the last via the new is_async → schedule_drain → drain_microtask path plus the "does not stall the HTTP client" test.

Level of scrutiny

High. This is a behavioral redesign of a production code path (transform(await fetch(...)), Bun.serve proxying) with cross-thread locking implications and substantial new unsafe Rust. It is not mechanical, and the review history itself surfaced multiple real correctness/safety issues that required follow-up commits.

Other factors

Test coverage is unusually thorough (twelve deterministic gate-driven tests plus ASAN soaks per the description), all four prior inline findings are resolved in the current revision, and the multi-agent bug hunter found nothing new on this pass. The PR also overlaps two other open PRs (#32956, #32927) whose interaction the description calls out. None of that changes the bar: the scope and unsafe density put this well outside what I'd approve without a human reviewer.

[robobun]

Thanks for the thorough review. All four inline findings across the three rounds (the take-once lol-html last-error buffer, end() on a poisoned rewriter, the prefix-slice aliasing under a re-entrant event-loop spin, and the producer-mutex deadlock) are fixed in the current revision, each with a dedicated deterministic test, and the review history is summarized in the description.

Agreed that a human pass is warranted. This is a behavioral redesign of a production code path with a significant amount of new unsafe pointer, refcount, and cross-thread reasoning, and it overlaps two other open PRs. The description's "Related PRs" section lays out exactly how #32956, #32927, and #31835 interact with this one so a maintainer can choose an order.

[robobun]

CI status for a reviewer: the diff is green, and the remaining red is CI infrastructure that is independent of this change. I have used my one retrigger and will not push further ones.

Across the last three full builds of this branch (66265, 66404, 66423), 279-280 jobs pass each time and every HTMLRewriter test passes on every lane that actually ran. The only failures, all three times:

• darwin 26 aarch64 - test-bun: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'. The agent never obtains the binary to test, so zero tests run. It has failed with that exact message on three consecutive builds of three different commits (twice on the latest), including one that was an empty retrigger with no code change. That is a broken agent or artifact-storage path on that fleet; no push from this PR can affect it.
• alpine 3.23 x64 / x64-baseline - test-bun: test/js/sql/sql-mysql.auth.test.ts fails with an "Unhandled error between tests" inside test/docker/index.ts while standing up the MySQL container. This PR touches html_rewriter.rs, the HTMLRewriter-only ValueBufferer in Body.rs, and one test file; it has no path into the SQL subsystem or the docker test harness.
• A bun-install / bun-install-registry flake annotation on Windows each time, already marked flaky and passing on its own retry.

The binary-size check reports this change as within a few KB of main on every target (largest delta +3.0 KB, windows-aarch64).

All four review rounds are addressed with every thread resolved, and the description's Verification and Related PRs sections are current. If someone can retry the darwin lane (or it is known broken fleet-wide), the build should go green aside from the sql-mysql docker flake. Ready for a human pass.

[Jarred-Sumner]

This code uses far too much unsafe. Instead, let's refactor our bindings to use the Rust LOLHTML crate directly instead of going through their C API.

[robobun]

Agreed, and that is the right diagnosis. Every unsafe block this PR (and its four review rounds) has been fighting exists because of the C boundary, not the problem itself: the opaque *mut HTMLRewriter handles, the output_sink_function trampoline that manufactures aliased &muts from userdata, and the take-once thread-local last-error buffer (which caused two of the review findings outright) are all artifacts of Rust calling lol-html's C API, which only exists because the Zig runtime had no other option.

It is also a smaller lift than it looks, because lol_html is already in the dependency graph: bun_lolhtml_sys consumes vendor/lolhtml/c-api as a cargo path dependency, which itself depends on the lol_html crate (v2.7.2, already vendored at vendor/lolhtml). Going direct is a dependency-graph no-op; it is the 1,299-line src/lolhtml_sys/ FFI layer (72 extern "C" declarations) that disappears. Concretely the surface is:

• src/lolhtml_sys/: deleted. HtmlRewriter, Settings, and html_content::{Element, TextChunk, Comment, Doctype, DocumentEnd, EndTag} replace the opaque handles with real types, real lifetimes, Result-typed errors, and Rust-enforced poisoning and non-reentrancy.
• src/runtime/api/html_rewriter.rs: 117 C-API call sites become direct method calls; the builder and handler registration move to Settings + closures. The JS wrapper classes keep their shape (a handle valid only inside its handler is inherent to lol-html's model either way).
• src/bundler/HTMLScanner.rs and bundle_v2.rs use the same bindings and migrate with them.
• scripts/build/deps/lolhtml.ts and the c-api path dependency go away.

I will do this as a behavior-preserving precursor PR (same buffered semantics as main, every existing HTMLRewriter and bundler-HTML test passing unchanged, zero new behavior) so it is reviewable on its own, and then rebase this streaming change on top of it, at which point this one should shrink to a fraction of its current unsafe. Starting on the precursor now; I'll link it here when it is up.

[robobun]

The binding refactor is up: #33048.

It deletes src/lolhtml_sys/ (1,299 lines, 74 unsafe blocks, all 72 extern "C" declarations) and migrates html_rewriter.rs plus the two bundler consumers to the lol_html Rust crate directly. Behavior-preserving: no test was added or changed, and all five HTMLRewriter test files (75 tests) and all three bundler HTML test files (30 tests) pass unmodified. Zero extern "C" remains in either consumer; the take-once thread-local error channel (the direct cause of two of this PR's review findings) is gone in favor of lol_html's typed Results.

Net diff: 23 files, +808 / -2310.

Once #33048 lands I will rebase this PR on top of it. The streaming change here should shed most of its unsafe in the process, because the classes of bug its four review rounds kept finding (the consume-once error buffer, the poisoned-rewriter panic, the FFI trampoline aliasing) are structurally impossible against the Rust crate's API.

[mintlify]

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated (UTC)
bun	🟢 Ready	View Preview	Jun 29, 2026, 5:39 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

[coderabbitai]

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details

{}

[robobun]

Final CI result on the current head a23c3a4e04 (build 67768, now terminal): 268 jobs pass, 18 test-bun lanes are red, and none of the red is this PR's. Exactly five test files fail across all 18 lanes; every one of them is a tracked main-side break or flake with an issue number, and html-rewriter / workerd / Body.rs appear zero times in any annotation. test/js/workerd/html-rewriter.test.js ran and passed on all 18 of those red lanes (including the ASAN and Windows ones) as well as on the 268 green ones.

• test/bake/dev/production.test.ts (15 lanes): bake: "works with sourcemaps - error thrown in React component" times out on every CI build #33214. It times out on every CI build right now; I counted it in the annotations of 16 of the last 19 finished builds across 16 unrelated branches.
• test/js/node/zlib/zlib.test.js (5 lanes): CI: zlib.test.js "streaming encode does not wait for entire input" (brotli) times out on the Linux lanes of most recent builds #33226. zlib.brotli > streaming encode doesn't wait for entire input times out on the Linux lanes of 8 of the last 19 finished builds, again across unrelated branches.
• test/js/node/test/parallel/test-net-connect-memleak.js (2 lanes): CI: test-net-connect-memleak.js fails on half of PR builds on linux-x64-musl since June 28 ~23:00 UTC #33044 (alpine/musl only).
• test/js/bun/terminal/terminal.test.ts (1 lane, darwin 14 x64 only): CI: terminal.test.ts "creates subprocess with terminal attached" times out on darwin-14-x64 on ~13% of builds #33187.
• test/js/bun/resolve/load-same-js-file-a-lot.test.ts (2 lanes, alpine only): the non-alpine lanes pass it.
• The darwin 26 aarch64 lane also dies 2 minutes in with buildkite-agent artifact download timed out after 120s before running a single test, on every build of this branch and on 20 of the last 25 builds pipeline-wide: CI: darwin-26-aarch64 test-bun fails on most PR builds with 'buildkite-agent artifact download timed out after 120s' #33116.

Bun Bake, node:zlib, net, the module loader, and Bun.Terminal share no code with HTMLRewriter. I have already used my one CI retrigger on this PR and will not push more; a retrigger cannot help here, since #33214 and #33226 fail deterministically on every branch until main is fixed.

Review state: @Jarred-Sumner's two changes-requested items are both done and replied to individually (the microtask draining is gone in 4dccae5, the raw-pointer unsafe is gone in a23c3a4, each verified with a 20-plus-run soak), and every review thread is resolved. Verification commands and results are in the description. This PR is ready and needs a maintainer.

[Jarred-Sumner]

I don't think this should use any JSC::Strong? visitChildren should visit the WriteBarrier it needs to keep alive.

[robobun]

Done in 8ab5455. You're right, and it came out strictly simpler: the sink now holds zero JSC::Strong handles. It had three.

• output_stream: readable_stream::Strong is deleted, along with the two functions whose only job was servicing it (on_readable_stream_available as a field-setter, and fail_output_stream's deinit bookkeeping). The stream a consumer attaches is stashed into the output Response cell's stream WriteBarrier slot (values: ["stream"] in response.classes.ts, visited by the generated visitChildrenImpl) and read back through the existing Response::get_body_readable_stream, which already treats that slot as the source of truth. So the cell's visitChildren keeps it alive, exactly as you said.

  One detail mattered and the existing Bun.serve tests caught it when I tried the obvious version (read the stream back off the body's PendingValue.readable): do_render_with_body moves that Strong out of the body and replaces the body Value with Used (RequestContext.rs:3075), so after the server takes over, nothing inside the body Value reaches the stream. The cell's WriteBarrier slot is outside the body Value and survives that, which is why the hook writes there.

• write_error: StrongOptional is deleted. It becomes a failed: bool; the JS error is built on the failing frame, carried as a stack value, and reported from that same drive_rewriter invocation on both the sync and async paths. It is never stored on the sink. This also deleted the idempotent double-report dance its doc comment used to need.

• response_value: StrongOptional (this one pre-dates the PR) becomes response_ref: JsRef, strong only for the sink's active lifetime, the pattern JsRef documents and ServerWebSocket/UDPSocket use. It is the sink's single GC reference, and it is what makes the Response cell (and through its visitChildrenImpl, the stream and the cached body slots) reachable from the sink's source callbacks after transform() has returned.

One thing to flag so it is not a surprise in the diff: report_rewriter_failure still constructs webcore::body::ValueError::JSValue(jsc::strong::Optional::create(..)) and streams::StreamError::JSValue(..) when handing the error to the body and to the stream. Those are Body.rs's own (pre-existing) representations for an error value; they own the handle, the sink does not. If you want those types moved off Strong too, that is a Body.rs change touching every body producer, and I'd rather do it as its own PR than fold it into this one.

Verified with bun bd test test/js/workerd/html-rewriter.test.js: 79 pass, 0 fail, including the three Bun.serve streaming tests. grep Strong src/runtime/api/html_rewriter.rs now matches only one comment.

[robobun]

The walkthrough's "Out of Scope Changes" warning about docs/guides/util/base64.mdx and docs/runtime/web-apis.mdx is stale: neither file is in this diff anymore. They were pushed to an earlier revision of this branch by autofix-ci[bot] (the repo's formatter), and the two rebases since then absorbed them once main picked up the same formatting, so the rebase dropped the patch.

git diff --stat origin/main...HEAD is exactly three files: src/runtime/api/html_rewriter.rs, src/runtime/webcore/Body.rs, and test/js/workerd/html-rewriter.test.js.

[Jarred-Sumner]

We shouldn't be draining microtasks anymore in here. Refactor it to not do that.

Remove all the unsafe usage.

[robobun]

Working on both. Stating my reading first so you can correct it cheaply before I rewrite.

"We shouldn't be draining microtasks anymore in here." I believe the problem is the nesting this PR introduced, not wait_for_promise itself. handler_callback's vm().wait_for_promise(promise) (pre-existing, html_rewriter.rs:1773) spins the event loop, which drains microtasks, from inside a synchronous lol_html::HtmlRewriter::write(). On main that only ever happens on transform()'s own top-level synchronous frame. This PR added schedule_drain, which uses JSGlobalObject::queue_microtask_callback to drive lol-html on a fresh stack after the HTTP producer's critical section ends, so an async element handler now reaches wait_for_promise from inside a microtask callback, which is a re-entrant microtask drain. queue_microtask_callback has exactly two callers in the whole repo, so it was never the right deferral primitive here.

Fix: the deferral becomes an event-loop task instead of a microtask, so wait_for_promise keeps running only from a top-level task frame, the same place it has always run from. If you meant something stronger (that wait_for_promise inside write() has to go entirely), tell me, because lol-html's handler contract is synchronous and that is a much larger change than this PR.

"Remove all the unsafe usage." The file has 107 unsafe sites today; 64 of them are already on main (the #33048 bindings) and 52 are this PR's. They all exist for one reason: BufferOutputSink is a raw heap allocation reached through *mut, because HtmlRewriter::write() re-enters SinkRef::handle_chunk(&mut self) through a stored pointer and runs user JS that can re-enter on_start_streaming, so nothing may hold &mut *sink across a write(). I am restructuring it so the part lol-html's output sink and the body hooks actually need is a separate shared, interior-mutable piece rather than the driver's own state; then the driver holds a plain &mut self and the raw pointers and the intrusive refcount go away with it.

I'll reply here with the result. The branch is also rebased onto current main (it had fallen 8 commits behind, past the new src/parsers crate).

[robobun]

The first one is done in 4dccae5: no more microtask draining, and the reading in my previous comment held up.

schedule_drain used JSGlobalObject::queue_microtask_callback to move the lol-html drive out of the HTTP producer's critical section. That primitive has exactly two callers in the whole tree and it was the wrong one: write runs user element handlers, an async handler blocks the write by spinning the full event loop (wait_for_promise), and that drains the microtask queue, so the streaming path was re-entering the microtask drain from inside a microtask callback. The pre-streaming code never did that because lol-html only ever ran from transform()'s own top-level synchronous frame.

It is now a plain event-loop task, the same AnyTask embedded on the native object + VirtualMachine::enqueue_task shape that FetchTasklet, PasswordObject, and FileResponseStream use. A task runs at the top of an event-loop turn, which is the same kind of frame lol-html has always been driven from, so wait_for_promise never nests. Verified: the full HTMLRewriter suite (79 tests) passes, 0 failures across 20 consecutive runs. I also kept wait_for_promise itself: lol-html 2.7.2's handler contract is Result<(), Box<dyn Error>>, fully synchronous, so it is the only way to support an async element handler on the crate. If you want that gone too, say so, because it is a much larger change than this PR and I do not see a mechanism for it in the crate.

Now doing the second one (remove the unsafe). The shape: everything that lol-html's output sink and the body's streaming hooks touch moves into its own interior-mutable cell on a shared Rc, separate from the cell that holds the HtmlRewriter itself and from the cell with the driver's queueing state, so no borrow is ever held across the re-entrant write() and the whole raw-pointer-plus-intrusive-refcount design goes away. I will reply again when it is pushed.

[robobun]

Both done.

No more microtask draining (4dccae5): covered in my previous comment. The deferral out of the HTTP producer's critical section is an event-loop task now (AnyTask + VirtualMachine::enqueue_task, the FetchTasklet shape), so wait_for_promise only ever runs from a top-level task frame, which is where lol-html has always been driven from.

The unsafe (a23c3a4): every function on BufferOutputSink reached its fields through unsafe { (*sink).field } place expressions, because drive_rewriter cannot hold &mut Self across HtmlRewriter::write() (the write re-enters SinkRef::handle_chunk through the sink pointer, and runs user element handlers that can reach the output body's hooks and, for an async handler, spin the whole event loop). It never needed &mut. The mutable state now lives in three disjoint interior-mutability domains, so every entry point forms only a shared &Self from its *mut Self, which is src/CLAUDE.md's documented borrow = shared mode:

• rewriter: borrowed across write()/end() by drive_rewriter and finish, lol-html's only drivers. Nothing re-entered from inside lol-html touches it.
• driver (the writing/failed flags, the queued input, the deferred terminal): what a source chunk, the source's end, or the drain task touches, possibly while drive_rewriter is suspended on an async handler.
• out (the buffered output bytes, the Response, its JsRef): what handle_chunk and the body's streaming hooks touch from inside a write().

A borrow of one domain is never held across a call that can enter another, and RefCell turns a violation of that into a deterministic panic with a line number instead of aliasing UB. It earned its keep on the literal first test run: panic: RefCell already mutably borrowed at two line numbers, because the if let Some(t) = this.driver.borrow_mut().pending_finish.take() at drive_rewriter's tail was holding the driver borrow across finish() (an if let scrutinee's temporary lives to the end of the whole block). That exact held-too-long shape was structurally invisible in the raw-pointer version. Two-line fix, caught on run one.

The rewriter also moved inline into its cell, so its separate heap allocation and Drop's manual heap::destroy are gone too.

The honest residual, so you can push further if you want: unsafe in the file went 107 to 78 (main is 64). Every raw-pointer place expression is gone. What is left is (a) one let this = unsafe { &*sink }; liveness assertion per entry point, under the refcount contract each of those unsafe fns already stated, (b) init()'s construction (16 of the 78, mostly the *mut Response derefs on the input and output Responses), and (c) the *mut c_void ctx casts at the 4 callback trampolines. I deliberately did NOT convert the intrusive refcount to Rc: that protocol is already correct and reviewed, and Rc::from_raw is exactly as unsafe as what it would replace, so the conversion removes zero unsafe while risking a subtle, working ownership protocol. If you want that, or init() reworked, say so and I will.

Verified: cargo check clean on the first compile of the restructure, 79 pass / 0 fail on the HTMLRewriter suite, and 0 failures across 25 consecutive full-file runs.