HTMLRewriter: read the input body through ResumableSink instead of ValueBufferer - #35324
HTMLRewriter: read the input body through ResumableSink instead of ValueBufferer#35324robobun wants to merge 13 commits into
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (18)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The Stacked Borrows concern is resolved — dropping the back-pointer entirely (20789b1) is cleaner than either fix I suggested, and the init() ref_count=1 path leaves the sink owned solely by its own pipe-ref/JS-wrapper lifecycle. No further issues found, but this rewires enough unsafe refcounting and provenance-sensitive code that a human should sign off.
What was reviewed:
- Refcount balance on
BufferOutputSinkacross everystart_reading_inputarm (Used/Empty/Error/Blob/file/Locked) — the in-flight +1 is consumed exactly once byon_input_endon Ok, or released by the caller on Err. ResumableHTMLRewriterSinkownership after the back-pointer removal: no reverse edge fromBufferOutputSink::Dropintoend_pipe's live&mut self.is_asyncderivation fromtmp_sync_error.is_none()and its nulling afterstart_reading_inputreturns — matches the oldValueBufferersync/async split.- Deleted
ValueBufferer/NativePromiseContext::BodyValueBufferer/error variants have no remaining callers.
Extended reasoning...
Overview
This PR replaces HTMLRewriter's bespoke ValueBufferer (the only caller of ~430 lines in Body.rs) with the existing ResumableSink primitive that fetch() request-body uploads and S3 multipart uploads already use. A third codegen'd monomorphization ResumableHTMLRewriterSink is added (one-line entries in .classes.ts, generated.rs, generated_classes_list.rs, ResumableSink.rs), and BufferOutputSink implements ResumableSinkContext to receive chunks. start_reading_input now dispatches on the body Value variant directly instead of delegating to ValueBufferer::run. All ValueBufferer-only machinery is deleted: the struct, its promise host-fns, its NativePromiseContext::Tag entry, three crate::Error variants, and JSSink<ArrayBufferSink>::detach_self. Net -36 lines across 16 files.
Security risks
None identified. This is internal plumbing between a Response body and lol-html; no new user-controlled input parsing, no auth/crypto, no filesystem/network surface. The chunk-copy into input_buffer before running the rewriter (tested by "does not rewrite out of the source buffer a handler can detach") is a memory-safety improvement over reading a user-mutable buffer.
Level of scrutiny
High. The core of the change is unsafe Rust with raw *mut BufferOutputSink pointers, intrusive Cell<u32> refcounts, and explicit Stacked Borrows provenance management (the this self-pointer field, the "recover root provenance" pattern in write_end_request, the raw-pointer-only accesses in init()). My previous review found a real SB violation on the reverse edge (BufferOutputSink::Drop re-entering ResumableSink while end_pipe held &mut self); the author fixed it by removing the back-pointer entirely, which is correct — the ResumableSink allocation is now owned solely by its own lifecycle (pipe ref on the Bytes path, JS wrapper Strong on the JS-pump path), and BufferOutputSink never touches it after init() returns. I re-traced the refcount on BufferOutputSink through every start_reading_input arm and every terminal path lands at exactly one on_input_end (which adopts the in-flight +1) or one explicit deref on Err.
Other factors
- Test coverage is thorough: 18 new cases plus 2 un-todo'd, covering sync/async delivery, error before/after
transform()returns, GC-under-load, buffer detachment mid-scan, and theBun.serveshape from #11758. All fail on main withERR_STREAM_CANNOT_PIPEand pass on the PR (verified in the description under ASAN andvalidateExceptionChecks). - The PR explicitly supersedes #33310, which a maintainer blocked on the grounds that HTMLRewriter should stop buffering — this is the direction that review pointed at, so a maintainer confirming the architectural choice is appropriate.
- The acknowledged pre-existing limitations (#19305 output-side, and the
ResumableSinkabandoned-transform leak shape shared with fetch/S3) are documented as.todo/ not-changed-here rather than silently carried; that's the right call for scope but worth a maintainer nod. - CI shows one unrelated flake (
test/js/bun/spawn/spawn.test.ts) on x64 lanes; not touched by this PR.
Given the scope of unsafe code and the explicit relationship to a previously-blocked PR, this should get a human review even though I found nothing further to flag.
Ready for reviewAt 7353853 (rebased onto current main). Two consecutive automated review passes with no findings; all ten substantive review threads (three from @Jarred-Sumner, seven across three bot rounds) are resolved. The open threads are comment-cop lints on doc-comment length; the comment-cop check itself passes. CI #81817: Shape after the rework (see the updated PR body for the full writeup):
Net -285 lines of native code vs. main. Deferring to a maintainer for the shared |
Why
|
| Field | Concern | Touched by SinkRef? |
|---|---|---|
bytes |
output accumulator | yes (write) |
response, response_value |
output Response |
yes (done) |
global |
JS context | yes (done) |
rewriter, context |
lol-html state | no (but rewriter owns SinkRef) |
input_buffer |
input accumulator | no |
tmp_sync_error |
sync-error plumbing (points at a stack local in init()) |
no |
ref_count, this |
lifecycle / provenance scaffolding | no |
The only reason SinkRef reaches back into BufferOutputSink is that the output target lives on the same struct as the rewriter. In every native transform precedent in the codebase (fetch response body → ByteStream, DecompressionStream, TextDecoderStream), the processor writes to an object it does not also own. BufferOutputSink is the only place that conflates processor and output.
The wait_for_promise constraint
handler_callback (html_rewriter.rs:1438) nests the event loop inside HtmlRewriter::write() to wait for an async element handler. That is only sound because write() is called once with the whole buffered body: nothing can deliver another input chunk during the nested drain, so write()'s &mut TransformStream is never re-entered. A streaming input (calling write() per chunk) would let the nested event loop pump the next chunk and re-enter write() while it's already borrowed. That is the real blocker for streaming and why #33243 exists.
The correct shape
Separate the output target from the rewriter's owner. The output Response body should be a ByteStream from the start, and the rewriter's OutputSink should write to that ByteStream, not to a field of its owner:
RewriteSession (renamed BufferOutputSink)
├── rewriter: Box<HtmlRewriter<'static, SinkRef>>
│ └── SinkRef { out: *mut ByteStream } ← separate allocation
├── context: Rc<RefCell<LOLHTMLContext>>
├── response_value: Strong ← roots output Response
├── ref_count, global
- Output:
transform()creates the outputResponsewith aLockedbody whoseon_start_streaminghands back theByteStream(the same hookfetch()response bodies use, Body.rs:782-841).SinkRef::handle_chunkcallsByteStream::on_datafor non-empty chunks and closes it on the final empty chunk. Nobytesfield; theByteStreamis the accumulator. - Input:
ResumableSinkContext::write_request_data(&mut self, chunk)appends to an input buffer (whilewait_for_promisestays) or calls(*self.rewriter).write(chunk)directly once HTMLRewriter: suspend async content handlers instead of nesting the event loop #33243 lands. - No self-reference: driving the rewriter touches
ByteStream(separate heap allocation), neverRewriteSession.write_end_requestcan be a plain&mut selfmethod;run_output_sinkcan take&mut self; thethisfield, the raw-pointer field access pattern ininit(), and the "do not materialise&mut *sink" comments all disappear.
This also fixes #19305 (.body of a transformed response yields an empty stream) as a direct consequence: the ByteStream is response.body from the moment transform() returns, so .body.getReader() and Bun.serve both work regardless of whether the input has settled.
tmp_sync_error
This field points at a stack local in init() and is Some only while init() is on the stack. It exists so a handler error that occurs synchronously during the one-shot write() can be thrown from transform() instead of rejecting the body. With the ByteStream output, the sync/async split goes away: a handler error is delivered as ByteStream::on_data(StreamResult::Err(..)), which errors the body's stream, and transform() always returns a Response. An already-errored stream is observable synchronously if anyone cares (workerd does the same). The field and the unhandled_rejection_scope dance in init() are then dead.
What to actually do with this PR
The ResumableSink input side and the ValueBufferer deletion are correct and independent of the output shape. The this field and input_buffer are the part that papers over the self-reference. Two options:
- Redo the output side here: replace
bytes/response/tmp_sync_error/thiswith aByteStreamtarget as above. Keeps the singlewrite()-at-end shape (so no HTMLRewriter: suspend async content handlers instead of nesting the event loop #33243 dependency), removes the self-reference, and fixesS3Clientwrites empty file forHTMLRewritertransformed fetch Response #19305. More churn, but the struct comes out clean. - Land as-is, follow up: the
thisfield is ugly but no worse thanmain's*mut Self-parameter threading, and the JS-stream fix is real. File theByteStreamoutput rework as the follow-up that also closesS3Clientwrites empty file forHTMLRewritertransformed fetch Response #19305.
Happy to do (1) here if that's the call; it's roughly the same size as what's already in this PR.
…chunk Addresses review on #35324: - write_request_data now drives lol_html::HtmlRewriter::write() per chunk instead of buffering the whole body first. A spill buffer covers the one re-entrant path (an async handler's wait_for_promise draining the next Source::Bytes pipe chunk while write() is on the stack); the JS pump cannot re-enter (JSResumableSinkPumpOperation::m_reading guards the drain loop and the next read is not issued until sink.write() returns). - The output Response body is a ByteStream from the start and SinkRef writes to it, not back into BufferOutputSink. That removes the self-reference (the reason run_output_sink took *mut Self and the this: *mut Self field existed) and makes .body.getReader() / Bun.serve work regardless of whether the input has settled (#19305). - ResumableSinkContext now takes *mut Self (borrow = ptr per src/CLAUDE.md), since HTMLRewriter's write_request_data can re-enter the sink. FetchTasklet and S3UploadStreamWrapper impls dereference once. - The in-flight ref on BufferOutputSink is RAII (ScopedRef::new / forget / adopt) instead of manual ref_() / deref(). - start_reading_input collapses to the materialised-body fast path (so transform(String | ArrayBuffer) keeps its synchronous contract) plus to_readable_stream() + ResumableSink::init for everything else; the per-variant file-read adapter is gone. Dropped fields: bytes, response, response_value, input_buffer, this, tmp_sync_error. Dropped methods: write_tmp_sync_error, on_file_read, on_input_end, run_output_sink, done, write. Handler errors are captured per write() (Self::rewrite) so the original JS error surfaces on async chunks too, not just inside init().
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// Drives one `HTMLRewriter.transform()` call: pulls input chunks from the | ||
| /// source body via `ResumableSink`, feeds them to lol-html, and delivers the | ||
| /// rewritten output to a `ByteStream` that backs the returned `Response` body. | ||
| /// | ||
| /// The rewriter's `OutputSink` writes to that `ByteStream` (a separate | ||
| /// allocation), not back into this struct, so driving the rewriter never | ||
| /// re-enters its owner. lol-html itself is still borrowed exclusively during | ||
| /// `write()`/`end()`; `writing` + `pending_*` guard the one path | ||
| /// (`wait_for_promise` inside a handler on the `Source::Bytes` native pipe) | ||
| /// that can deliver the next input chunk while a `write()` is on the stack. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// GC root for the output `ByteStream`'s JS wrapper; `SinkRef` writes to | ||
| /// its `context` payload via [`output_bytes`](Self::output_bytes). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The output Response body is a ByteStream from the start so `SinkRef` | ||
| // never reaches back into this struct and every consumer path | ||
| // (`.text()`, `.body.getReader()`, `Bun.serve`) reads the same stream. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// refcount > 0; `(*sink).rewriter` must be set. The +1 taken for the | ||
| /// in-flight reader in `init()` is consumed by `finish` on every path. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Materialised bodies run the rewrite synchronously so that | ||
| // `transform(String | ArrayBuffer)` (which reads the output | ||
| // body back as a blob before returning) keeps its synchronous | ||
| // contract. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The in-flight +1 on `BufferOutputSink` keeps `context` valid until | ||
| // `write_end_request` fires; the sink's own lifecycle (pipe ref / JS | ||
| // wrapper) governs its allocation. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Feed one input chunk to lol-html. | ||
| /// | ||
| /// A re-entrant call (handler `wait_for_promise` draining the next | ||
| /// `Source::Bytes` pipe chunk) spills into `pending_input`; the outer call | ||
| /// drains it after `write()` returns. The JS-pump path cannot re-enter | ||
| /// (`JSResumableSinkPumpOperation::m_reading` guards the drain loop and the | ||
| /// next `resumableIssueRead` is not issued until `sink.write()` returns). | ||
| /// | ||
| /// # Safety | ||
| /// `sink` must be a live `BufferOutputSink` heap allocation (refcount > 0). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Drive one `HtmlRewriter::write()` under a handler-error capture scope so | ||
| /// a thrown / rejected handler surfaces its original JS error instead of | ||
| /// the generic "rewriter has been stopped" lol-html wrapper. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Close the transform: consume the rewriter with `end()` (emits the final | ||
| /// empty chunk, which `SinkRef` forwards as `Done`), or push an upstream | ||
| /// error to the output stream. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Writes to the output `ByteStream` (rooted via `BufferOutputSink::output`), | ||
| /// not back into its owner, so driving the rewriter never re-enters | ||
| /// `BufferOutputSink`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// | ||
| /// Both methods take `*mut Self` (not `&mut self`) per the "borrow = ptr" | ||
| /// dispatch rule in src/CLAUDE.md: HTMLRewriter's `write_request_data` drives | ||
| /// `lol_html::HtmlRewriter::write`, which runs user async handlers via | ||
| /// `vm.wait_for_promise`; on the `Source::Bytes` native-pipe path that nested | ||
| /// event loop can deliver the next chunk and re-enter `on_write` on the same | ||
| /// context. A `&mut self` receiver would be aliased on the re-entrant call. | ||
| /// Impls that do not re-enter (FetchTasklet, S3) dereference once at the top. | ||
| /// | ||
| /// # Safety | ||
| /// `this` is the live heap allocation stored in [`ResumableSink::context`]; | ||
| /// callers only invoke these via [`ResumableSink::on_write`] / | ||
| /// [`ResumableSink::on_end`]. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // on the stack via ensure_still_alive above). `to_js` migrated the | ||
| // output ByteStream to the wrapper's cached stream slot. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Drives one `HTMLRewriter.transform()` call: pulls input chunks from the | ||
| /// source body via `ResumableSink`, feeds them to lol-html, and delivers the | ||
| /// rewritten output to a `ByteStream` that backs the returned `Response` body. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// First error pushed via [`fail`](Self::fail), so a synchronous handler | ||
| /// error inside `init()` still makes `transform()` throw. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Skip the `Source::Bytes` native-pipe fast path and always drive the | ||
| /// stream through the JS pump. Set by contexts whose `write_request_data` | ||
| /// nests the event loop (HTMLRewriter's `wait_for_promise`), which could | ||
| /// re-enter the native pipe callback; the JS pump's `m_reading` guard and | ||
| /// read-after-write sequencing prevent that. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `err_ptr` is a stack Cell owned by `HandlerErrorScope`'s | ||
| // caller; that frame is conservatively scanned until | ||
| // `create_lolhtml_error` reads it back, so no `protect()`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `fail()` destroyed the rewriter; stop the pump so the source | ||
| // is cancelled and `write_end_request` fires. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
No new issues on c055503 — the earlier findings (Response leak on clone_this error, unbalanced gcProtect in handler_callback, input pump not cancelled on handler failure, require nit) are all addressed. Deferring to a human: this is a structural rewrite of BufferOutputSink's ownership/refcount graph plus a behaviour change to the shared ResumableSink::js_write Done arm that FetchTasklet and S3 also flow through, so it warrants a maintainer pass.
What was reviewed:
- Refcount balance on
BufferOutputSinkacross every terminal path (sync blob,Value::Error, stream end, handler throw,start_reading_input?-propagation) and the pairedoutputStrong /failedStrong lifetimes. - The new
js_writeDone → cancel(undefined)arm against FetchTasklet/S3: both only returnDoneafter their own abort paths have setstatus = Done, socancel()'s status guard makes it a no-op there. SinkRef(*mut ByteStream)liveness — rooted byBufferOutputSink::outputfor the rewriter's whole lifetime;feed's owned copy vs. handler-mutated source buffers.HandlerErrorScopesave/restore ordering and the droppedexc_value.protect()(stack-local capture cell is conservatively scanned).
Extended reasoning...
Overview
Restructures HTMLRewriter.transform() to drive lol-html per chunk via a new ResumableHTMLRewriterSink (third monomorphization of ResumableSink) and write output to a ByteStream-backed Response body, deleting the ~430-line ValueBufferer and its FFI/NativePromiseContext surface. Touches 16 files: the core rewrite in html_rewriter.rs, a shared behaviour change in ResumableSink.rs (AVOID_NATIVE_PIPE const + js_write Done now cancels), codegen plumbing for the new sink class, and mechanical deletions of the bufferer's C++/Rust hooks. ~340 lines of new tests.
Security risks
None identified. Input is HTML bytes fed to lol-html (unchanged parser); no auth/crypto/permissions. The feed copy-before-write closes a handler-can-mutate-source-buffer window that would otherwise be a memory-safety concern.
Level of scrutiny
High. This is native Rust with manual refcounting (CellRefCounted, ScopedRef::adopt/new/forget), raw *mut fields whose liveness depends on GC Strong roots, an RAII guard that mutates VM-global state across a nested event-loop drain, and a change to shared ResumableSink code that fetch and S3 upload also use. Three prior review rounds each surfaced a real defect (leak-on-error-path, unbalanced gcProtect, uncancelled input pump), all fixed — which is exactly why a human should do the final pass on the ownership graph and the js_write Done semantics change.
Other factors
- Test coverage is thorough (20+ new cases including GC-stress, leak counting via
heapStats, source cancellation, buffer-detach mid-scan,Bun.serveintegration) and the two pre-existing.todocases are un-skipped. - Several
comment-copbot warnings about long justification comments remain open; these are style-lint, not correctness, but a maintainer may want them tightened before merge. - The one candidate raised and refuted this run (
js_writeDone → cancelreaching a non-abortedFetchTasklet) was traced: FetchTasklet'swrite_request_datareturnsDoneonly whenis_body_finished(), at which point its own cleanup has already setstatus = Done, socancel()early-returns. - The acknowledged pre-existing limitation (unrelated unhandled rejections swallowed during
wait_for_promiseunderHandlerErrorScope) is left as a follow-up per the resolved thread; not a regression.
| // A body backed by a ReadableStream has no producer to | ||
| // fire `on_receive_value`; read the stream ourselves. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// `.then` reaction for a `Locked` body that already has a readable: the | ||
| /// body was read via `readableStreamToBytes`; wrap the resolved bytes as | ||
| /// an `InternalBlob` and hand them to [`then`](Self::then). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
…lueBufferer
HTMLRewriter.transform() rejected any Response whose body was a JS-created
ReadableStream (start/enqueue source, TransformStream readable, async
generator, type: 'direct') with ERR_STREAM_CANNOT_PIPE. The
ValueBufferer::buffer_locked_body_value match on the stream source kind had:
Source::JavaScript | Source::Direct => {
// this is broken right now
// return self.create_js_sink(stream);
return Err(crate::Error::UnsupportedStreamType);
}
ValueBufferer was a bespoke 'read a whole body into one slice' helper whose
only caller was HTMLRewriter, and it never finished implementing two of the
five stream source kinds. The runtime already has the primitive it wanted to
be: ResumableSink (src/runtime/webcore/ResumableSink.rs), a two-method
write_request_data / write_end_request trait that handles every source kind
(Source::Bytes via native Pipe, JavaScript/Direct/Blob/File via the JS pump
Bun__assignStreamIntoResumableSink). fetch() request-body uploads and S3
multipart uploads are built on it.
This swaps BufferOutputSink from ValueBufferer to ResumableSink:
- A third codegen'd sink variant ResumableHTMLRewriterSink sits alongside
ResumableFetchSink / ResumableS3UploadSink.
- BufferOutputSink::start_reading_input does the Value dispatch ValueBufferer
used to: materialised bodies (string/Blob/buffer/empty) run the rewrite
immediately, a file-backed Blob schedules an async read, and any body
carrying a ReadableStream goes to ResumableSink::init.
- impl ResumableSinkContext for BufferOutputSink buffers each chunk and runs
the rewrite once in write_end_request. This keeps the single write()+end()
shape the nested-event-loop async-handler path still relies on; moving the
write into write_request_data is the one-line follow-up for true streaming
once that constraint is lifted.
Deleted, all dead once ValueBufferer is gone: the ValueBufferer struct/impls
(~430 lines of Body.rs), Bun__BodyValueBufferer__onResolveStream/onRejectStream
and their PromiseFunctions enum entries (ZigGlobalObject.{h,cpp}, headers.h,
headers-cpp.h), the NativePromiseContext BodyValueBufferer tag (Rust and C++),
JSSink<ArrayBufferSink>::detach_self, the ArrayBufferJSSink alias, and the
crate::Error variants UnsupportedStreamType/StreamAlreadyUsed/InvalidStream.
Tests land in test/js/workerd/html-rewriter.test.js: 18 new cases covering
single/multi/mixed-chunk JS streams, direct streams, streams that produce
after transform() returns, every promise-returning reader, handlers observing
the document, upstream errors before/after transform() returns, bad chunk
types, source reuse, handler-mutated source buffers, GC while the source is
in flight, and the Bun.serve shape from #11758. The two pre-existing
it.todo('works with payload of type direct'/'default') cases in that file were
todo for exactly this reason and now pass. All 20 fail on main with
ERR_STREAM_CANNOT_PIPE.
Fixes #14216
Fixes #11758
BufferOutputSink::Drop ran inside ResumableSink::end_pipe's &mut self on the Source::Bytes path (end_pipe -> on_end -> write_end_request -> on_input_end -> ScopedRef drop -> BufferOutputSink::Drop -> clear_input_sink), and clear_input_sink re-derived a fresh &mut ResumableSink from the stored root pointer, popping end_pipe's Unique tag before end_pipe read self.js_this.is_strong(). No runtime consequence (the allocation was still live and detach_js was a no-op), but it broke the Stacked Borrows discipline the rest of this change is careful about. BufferOutputSink never needed to own a ref on the ResumableSink in the first place: the in-flight +1 on BufferOutputSink keeps the context pointer valid until write_end_request fires, and the ResumableSink's own lifecycle (pipe ref or JS wrapper) governs its allocation. Dropping to init() (ref_count 1) and not storing the pointer removes the reverse edge entirely.
…chunk Addresses review on #35324: - write_request_data now drives lol_html::HtmlRewriter::write() per chunk instead of buffering the whole body first. A spill buffer covers the one re-entrant path (an async handler's wait_for_promise draining the next Source::Bytes pipe chunk while write() is on the stack); the JS pump cannot re-enter (JSResumableSinkPumpOperation::m_reading guards the drain loop and the next read is not issued until sink.write() returns). - The output Response body is a ByteStream from the start and SinkRef writes to it, not back into BufferOutputSink. That removes the self-reference (the reason run_output_sink took *mut Self and the this: *mut Self field existed) and makes .body.getReader() / Bun.serve work regardless of whether the input has settled (#19305). - ResumableSinkContext now takes *mut Self (borrow = ptr per src/CLAUDE.md), since HTMLRewriter's write_request_data can re-enter the sink. FetchTasklet and S3UploadStreamWrapper impls dereference once. - The in-flight ref on BufferOutputSink is RAII (ScopedRef::new / forget / adopt) instead of manual ref_() / deref(). - start_reading_input collapses to the materialised-body fast path (so transform(String | ArrayBuffer) keeps its synchronous contract) plus to_readable_stream() + ResumableSink::init for everything else; the per-variant file-read adapter is gone. Dropped fields: bytes, response, response_value, input_buffer, this, tmp_sync_error. Dropped methods: write_tmp_sync_error, on_file_read, on_input_end, run_output_sink, done, write. Handler errors are captured per write() (Self::rewrite) so the original JS error surfaces on async chunks too, not just inside init().
…cy guard Addresses the five review findings on 15f3adb: - The Source::Bytes native-pipe path could re-enter ResumableSink's own &mut self chain (Wrap::pipe -> on_stream_pipe -> end_pipe) when a handler's wait_for_promise nested the event loop, not just the context layer. Rather than propagating *mut Self through PipeHandler, ResumableSinkContext gains AVOID_NATIVE_PIPE (default false, true for BufferOutputSink) so HTMLRewriter always goes through the JS pump, whose m_reading guard and read-after-write sequencing already prevent re-entry. The trait reverts to &mut self and the writing / pending_input / pending_end fields (and the unrooted JSValue they stored) are gone. - rewrite() shadowed init()'s capture scope, so a sync handler error no longer made transform() throw. rewrite() is gone; create_lolhtml_error already reads vm.unhandled_pending_rejection_to_capture. A HandlerErrorScope RAII guard installs the capture slot around every feed/finish call (init()'s sync path, write_request_data, write_end_request), and a strong::Optional failed field on BufferOutputSink lets init() throw what fail() stored. - finish() now runs under the same capture scope as feed(), so an onDocument.end handler error on the async path surfaces the user's error. - The output Response is wrapped in a scopeguard so a clone_this? failure no longer leaks it and the Strong on its body stream. Other fallout of the ByteStream output landing: - transform(String | ArrayBuffer) reads the output back via get_body_readable_stream().to_any_blob(): Response::to_js migrates Locked.readable to the wrapper's cached stream slot, so use_as_any_blob_allow_non_utf8_string found an empty Strong. - start_reading_input handles Value::Error synchronously so transform() of an already-failed body still throws. - feed() copies the chunk before HtmlRewriter::write(): lol-html parses the first chunk straight from the input slice, and a handler can otherwise mutate bytes it has not tokenized yet. - The 'transform rejects when the upstream body fails' tests are adapted for streaming: chunks delivered before the failure now reach .body, so the assertions read to completion instead of expecting a single rejected read. - The '.body of a transform whose source is still pending' test is un-skipped (#19305 is fixed), and getReader / readableStreamToText are back in the 'every way of reading' suite.
…tion handler_callback wrote the thrown exception into the HandlerErrorScope capture cell and then gcProtect()ed it; create_lolhtml_error reads that slot back and zeros it without unprotecting. The capture cell is a stack local on a conservatively-scanned frame that lives from the store through the read, so the protect provided no GC safety and leaked one Exception (and its Error) per transform on the throwing-handler path. Verified by the new leak test: 200 iterations pinned exactly 200 Error instances pre-fix.
write_request_data returned WantMore unconditionally, so after fail() destroyed the rewriter the JS pump kept reading the source stream into a dead context until the source closed on its own. For a never-closing pull()-based source that means write_end_request never fires and the in-flight +1 (plus the sink's self-rooting js_this Strong, the reader and the source) stay alive forever after .text() has already rejected. js_write now treats ResumableSinkBackpressure::Done as a request to cancel the pump (cancel() releases the reader, fires on_end, and detaches the JS wrapper). write_request_data returns Done once the rewriter is gone; finish() early-returns if fail() already ran so the cancel-driven write_end_request is a no-op. FetchTasklet and S3 already return Done only after their own cancel()/abort paths have set status = Done, so the new js_write behaviour is idempotent for them. Covered by the new 'cancels the source stream once a handler throws' test, which hung forever pre-fix. Also moved the heapStats import to module scope per test/CLAUDE.md.
Bun.write(path, response) with a Response whose body was Locked with a
readable (Locked { readable: Strong(stream) }) set on_receive_value and
waited for a producer to call Value::resolve(). For bodies that are streams
from the start (new Response(readableStream), and now
HTMLRewriter.transform()'s ByteStream output), nothing ever fires that
callback and the write hangs forever. Reproducible on main with
Bun.write(path, new Response(new ReadableStream(...))).
When the Locked body already has a readable (cached on the JS wrapper or in
locked.readable), read it via readableStreamToBytes and feed the resolved
bytes to WriteFileWaitFromLockedValueTask::then as an InternalBlob. A
rejected stream reaches then() as Value::Error.
Fixes the 'Bun.write(output.html, HTMLRewriter.transform(Bun.file))' test
that regressed when HTMLRewriter's output body became a ByteStream.
readable_stream_to_bytes enters JS and can return JSValue::ZERO with a pending exception (or a non-promise value). The previous revision called .then() on the result unconditionally, which in debug asserts on .asCell() and in release silently no-ops, leaking the heap-allocated WriteFileWaitFromLockedValueTask (its JSPromiseStrong root and file_blob) with a promise that never settles. Wrap the call in from_js_host_call and, if the result is not a promise, reject the task's promise with the exception (or the returned value) and free the task.
arguments_old::<N>() was removed on main.
6728c1f to
c08a95e
Compare
There was a problem hiding this comment.
No new issues found in this pass, but this restructures memory-safety-critical native code (BufferOutputSink refcounting, GC rooting of the output ByteStream, a shared ResumableSink::js_write behavior change that also affects FetchTasklet/S3, and a tangential Bun.write Locked-body arm), so it warrants a human look.
What was reviewed:
- Refcount balance on
BufferOutputSinkacross the sync/error/ResumableSinkpaths and the newDone → cancel()mapping injs_write(idempotent for Fetch/S3 via thestatus == Doneguard). HandlerErrorScopesave/restore ofunhandled_pending_rejection_to_captureand the removed.protect()inhandler_callback— the leak test covers it.- The
Bun.writereadable_stream_to_bytesarm: exception + non-promise result now free the task and reject its promise; a non-promise resolved bytes value was considered and ruled out (rejects with the value, which is odd but not a leak). - Input-pump cancellation when a handler throws on a never-closing source.
Extended reasoning...
Overview
This PR restructures HTMLRewriter.transform() to drive input through the existing ResumableSink machinery (adding a third codegen'd variant ResumableHTMLRewriterSink) and to write output to a standalone ByteStream instead of back into the sink itself. It deletes ~430 lines of the bespoke ValueBufferer in Body.rs plus its FFI/NativePromiseContext::Tag/PromiseFunctions surface, and adds a new Bun.write arm for stream-backed Locked bodies (with new Bun__WriteFileLocked__onStream{Resolved,Rejected} promise reactions). It also changes shared behavior in ResumableSink::js_write: ResumableSinkBackpressure::Done now maps to this.cancel(undefined) + return false, which affects the FetchTasklet and S3 sink monomorphizations as well.
Security risks
None identified. The change surface is stream plumbing and lol-html invocation; the input-buffer copy in feed() defends against a handler mutating/transferring the source buffer mid-parse (covered by a test).
Level of scrutiny
High. This is native Rust with intrusive refcounting (CellRefCounted), raw *mut field access, GC roots (readable_stream::Strong, jsc::strong::Optional, JsRef), an RAII guard that mutates VM-global state (HandlerErrorScope), and a nested-event-loop path (wait_for_promise inside feed). Several prior review rounds surfaced real memory-safety issues (an unbalanced gcProtect, an uncancelled input pump leaking the sink and its Strong root, and an unchecked-exception path in the new Bun.write arm) — all now addressed with tests, but the density of fixes is itself a signal that this code needs a maintainer's eye. The js_write Done change is cross-cutting to two other sink contexts and, while argued idempotent, deserves confirmation.
Other factors
Test coverage is good (20+ new cases including GC-stress, leak, cancellation, and Bun.serve shapes; two pre-existing .todo cases un-skipped). Several comment-cop bot comments about paragraph-length justification comments remain unresolved on the thread. The Bun.write change is somewhat tangential to the HTMLRewriter fix and expands the PR's blast radius. Net -285 lines and the removal of the self-referential *mut Self output-sink pattern are structural improvements, but the amount of unsafe pointer choreography in init()/start_reading_input() and the SinkRef(*mut ByteStream) lifetime tie to output: Strong are exactly the kind of thing REVIEW.md flags for human sign-off.
…andlers instead of wait_for_promise (#36733) ### What `HTMLRewriter.transform()` now streams: input body chunks flow through `lol-html` into an output `ByteStream` as they arrive, with backpressure propagated end-to-end via the `SinkHandle`/`SourceHandle` pattern from #36087. Async content handlers no longer nest the event loop. ``` input body ──► SinkHandle::HTMLRewriter ──► lol_html::HtmlRewriter ──► output ByteStream ▲ │ └─────────── SourceHandle::HTMLRewriter (producer) ◄────────────────────┘ ``` ### Why `BufferOutputSink` fully buffered the source body via `ValueBufferer` before a single `rewriter.write()` + `end()`, and `handler_callback` spun `vm.wait_for_promise()` six native frames deep inside lol-html for any handler that returned a Promise. That meant handlers fired only at source-end (TTFB = full download), `.body` / `Bun.serve` saw an empty stream (#6068, #19305), JS-backed `ReadableStream` inputs were rejected outright (#11758, #14216), and the nested loop was a known hazard class (deadlocks, ready-poll clobbering, pending-exception leaks). lol-html could not be suspended before because its tokens are stack locals borrowing a stack-local lexeme; returning from `write()` destroys them, and an async handler must be able to mutate the element after its `await`. ### lol-html fork (`oven-sh/lol-html`, branch `bun`) A handler returns `Err(SuspensionRequest)` to suspend. The in-flight unit is deep-copied onto the heap, `write()`/`end()` return the non-poisoning `RewritingError::Suspended`, and `HtmlRewriter::resume()` continues from a `StateMachineBookmark`. The pending captured-text flush is hoisted from `Dispatcher::handle_tag` into the lexer actions so every suspension point has a uniform shape. `Arena::shift` advances a start offset instead of memmoving the tail (a suspended rewrite re-feeds its unconsumed tail on every resume). 23 in-crate tests added; the new `.github/workflows/lolhtml.yml` runs the fork's own `cargo test` at the pinned commit, gated on `scripts/build/deps/lolhtml.ts`. The fork is a `github-archive` source (no patch file), so rebasing onto a new upstream tag is a `git rebase --onto` in the fork plus a commit bump here. ### Bun side (`RewriterPipe` replaces `BufferOutputSink`) - **Ownership**: a generated `HTMLRewriterTransform` JS cell (not user-visible) owns the pipe; its GC finalizer frees it, and nothing pins anything. Liveness is plain GC edges: the output Response's `transform` WriteBarrier slot and the `.then()` context of a suspended handler's (or the JS pump's) promise reach the cell, the cell's five slots root the Response, the input/output streams, the pending flush promise, and a captured handler error, and a wired native source's `owner` WriteBarrier slot (new on the generated NewSource cells) points back at the cell, so I/O that roots the source (a FetchTasklet, a FileReader's read refs, a reader on the output stream) roots the rewrite for exactly the window the raw `SinkHandle`/producer backrefs are wired. A handler promise collected without settling lets `finalize` defer to an event-loop task (`abandon_suspension`) that rejects the body before freeing; the pipe holds one native `+1` on the Response (released in `Drop`) so that task can still reach the body. No intrusive refcount, no `Strong` fields on the pipe; `finish()` frees the boxed lol-html state machine eagerly, and `fail()`/output-cancel close the upstream producer instead of draining it to EOF. - **Output**: the returned `Response`'s body is `Locked(PendingValue { task: pipe, on_start_streaming, on_readable_stream_available, producer: SourceHandle::HTMLRewriter })`, the `FetchTasklet::to_body_value` shape. The `ByteStream` is created lazily when a consumer reads `.body` or a body-mixin method; until then, rewriter output buffers in a `Vec<u8>` handed over as `DrainResult::Owned`. - **Input**: mirrors `FetchTasklet::start_request_stream`. Native `ByteStream`/`FileReader` sources wire `byte_stream.sink = SinkHandle::HTMLRewriter` + `lock_native` + `drain()`; other stream kinds go through `JSSink::<RewriterPipe>::assign_to_stream` (new `HTMLRewriterSink` codegen entry). Materialized bodies (`InternalBlob`/bytes `Blob`/`WTFStringImpl`) feed synchronously. - **Backpressure**: `RewriterPipe::write` feeds one chunk through `rewriter.write()` (output chunks push via `ByteStream::on_data`). If the output is paused or a handler suspended, `write` returns `Writable::Backpressure`; `ByteStream::resume` → `SourceHandle::HTMLRewriter::on_ready` → `pipe.resume()` drains `pending_input` then `input_source.ready()`. - **Async handlers**: `handler_callback` returns `HandlerOutcome::{Continue, Stop, Suspend}`. On a pending Promise it runs one microtask checkpoint (`process.nextTick` then promise jobs, never the loop); a genuinely pending Promise suspends. The JS wrapper is retargeted at the heap-parked token so post-`await` mutations land where they should. The `.then()` context is the Transform cell itself (the reactions recover the pipe via `from_js`), so a handler promise collected without settling abandons the parked rewrite instead of leaking it. - **Error handling**: `handler_error` on the pipe replaces the stack `Cell` + `unhandled_pending_rejection_to_capture` override. A handler error on a streaming input now rejects the body with the real error instead of `The rewriter has been stopped.`. - `AttributeIterator` holds a backref to the `Element` plus an index instead of a boxed `slice::Iter`, so `for (const [k, v] of el.attributes) { await ... }` keeps working across a suspension. ### Deletions `ValueBufferer` (~430 lines of `Body.rs`) and its host-fn exports; `SinkHandle::ValueBufferer` + `SinkWriteFn`; `JSSink<ArrayBufferSink>::detach_self`; `crate::Error::{StreamAlreadyUsed, InvalidStream, UnsupportedStreamType}`; `NativePromiseContext::Tag::BodyValueBufferer` (ordinal 4 reused for `HTMLRewriterSuspension`); the two `Bun__BodyValueBufferer__*` `PromiseFunctions` (slots reused for `Bun__HTMLRewriter__onHandler{Resolve,Reject}`). ### Behavior changes 1. **Error channel is decided by the overload, not by timing.** `transform(string)` / `transform(ArrayBuffer)` throw from `transform()`. Every `Response` input rejects its output body instead. Five existing tests that pinned the old timing-dependent split are updated. Input-body errors (already-failed or aborted body) still throw synchronously from `transform()`. 2. A handler whose Promise needs the event loop to turn makes `transform(string)` / `transform(ArrayBuffer)` throw a `TypeError` (`pass a Response and await its body`) instead of spinning. A Promise that settles within a microtask checkpoint still works. 3. A rejection a handler neither awaits nor returns reaches `unhandledRejection` instead of being captured and thrown from `transform()`. 4. `transform()` types corrected: `Bun.BufferSource` returns `ArrayBuffer` (it always has at runtime); `Blob` removed from the overload (it threw at runtime). 5. `Bun.serve` with an HTMLRewriter-produced response body defers status/headers until the first body byte or clean end, so a handler that fails before emitting any bytes is routed to the server'''s `error()` hook instead of committing `200 OK` then force-closing the connection. Headers never preceded the first byte on this path before either (the old implementation buffered the whole rewrite). All other native-ByteStream bodies (proxied `fetch()`, S3, spawn stdout) keep sending status/headers immediately, and JS `ReadableStream` bodies (`do_render_stream`) are unchanged. `docs/runtime/html-rewriter.mdx` and `packages/bun-types/html-rewriter.d.ts` cover 1-4. ### Verification - `test/js/workerd/html-rewriter.test.js`: 107 pass, 0 fail under ASAN debug and under `BUN_JSC_validateExceptionChecks=1`. New coverage: async element/text/comment/doctype/onEndTag/document-end handlers, `Bun.gc(true)` while an element is heap-parked across an `await`, re-suspension by a second handler on the same element, nested `transform()` inside a suspended handler, strict document-order across 8 awaiting handlers, `Bun.serve` with a live client, client abort mid-suspension, the consumer matrix for a pending output body, JS-backed and `type:'direct'` `ReadableStream` inputs. - `test/js/workerd/html-rewriter-leak.test.ts`: protected-object + `Response` count regression over 120 suspending rewrites; a never-settling handler rejects the body instead of leaking. - Fail-before: `works with payload of type direct` → `ERR_STREAM_CANNOT_PIPE` on the released bun. - `bun-types` green, `cargo clippy -p bun_runtime` clean. - `vendor/lolhtml/.ref` re-fetch of the fork verified through the real ninja edge. Fixes #11758 Fixes #14216 Fixes #6068 Fixes #19305 Closes #33243 (same lol-html fork, different input layer), Closes #35324 (ResumableSink, deleted in #36087), Closes #32988 (per-chunk ValueBufferer callback). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 20 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/io/bun-write.test.js test/js/node/process/process.test.js test/js/workerd/html-rewriter-leak.test.ts <!-- robobun:evidence:end -->
…andlers instead of wait_for_promise (oven-sh#36733) `HTMLRewriter.transform()` now streams: input body chunks flow through `lol-html` into an output `ByteStream` as they arrive, with backpressure propagated end-to-end via the `SinkHandle`/`SourceHandle` pattern from ``` input body ──► SinkHandle::HTMLRewriter ──► lol_html::HtmlRewriter ──► output ByteStream ▲ │ └─────────── SourceHandle::HTMLRewriter (producer) ◄────────────────────┘ ``` `BufferOutputSink` fully buffered the source body via `ValueBufferer` before a single `rewriter.write()` + `end()`, and `handler_callback` spun `vm.wait_for_promise()` six native frames deep inside lol-html for any handler that returned a Promise. That meant handlers fired only at source-end (TTFB = full download), `.body` / `Bun.serve` saw an empty stream (oven-sh#6068, oven-sh#19305), JS-backed `ReadableStream` inputs were rejected outright (oven-sh#11758, oven-sh#14216), and the nested loop was a known hazard class (deadlocks, ready-poll clobbering, pending-exception leaks). lol-html could not be suspended before because its tokens are stack locals borrowing a stack-local lexeme; returning from `write()` destroys them, and an async handler must be able to mutate the element after its `await`. A handler returns `Err(SuspensionRequest)` to suspend. The in-flight unit is deep-copied onto the heap, `write()`/`end()` return the non-poisoning `RewritingError::Suspended`, and `HtmlRewriter::resume()` continues from a `StateMachineBookmark`. The pending captured-text flush is hoisted from `Dispatcher::handle_tag` into the lexer actions so every suspension point has a uniform shape. `Arena::shift` advances a start offset instead of memmoving the tail (a suspended rewrite re-feeds its unconsumed tail on every resume). 23 in-crate tests added; the new `.github/workflows/lolhtml.yml` runs the fork's own `cargo test` at the pinned commit, gated on `scripts/build/deps/lolhtml.ts`. The fork is a `github-archive` source (no patch file), so rebasing onto a new upstream tag is a `git rebase --onto` in the fork plus a commit bump here. - **Ownership**: a generated `HTMLRewriterTransform` JS cell (not user-visible) owns the pipe; its GC finalizer frees it, and nothing pins anything. Liveness is plain GC edges: the output Response's `transform` WriteBarrier slot and the `.then()` context of a suspended handler's (or the JS pump's) promise reach the cell, the cell's five slots root the Response, the input/output streams, the pending flush promise, and a captured handler error, and a wired native source's `owner` WriteBarrier slot (new on the generated NewSource cells) points back at the cell, so I/O that roots the source (a FetchTasklet, a FileReader's read refs, a reader on the output stream) roots the rewrite for exactly the window the raw `SinkHandle`/producer backrefs are wired. A handler promise collected without settling lets `finalize` defer to an event-loop task (`abandon_suspension`) that rejects the body before freeing; the pipe holds one native `+1` on the Response (released in `Drop`) so that task can still reach the body. No intrusive refcount, no `Strong` fields on the pipe; `finish()` frees the boxed lol-html state machine eagerly, and `fail()`/output-cancel close the upstream producer instead of draining it to EOF. - **Output**: the returned `Response`'s body is `Locked(PendingValue { task: pipe, on_start_streaming, on_readable_stream_available, producer: SourceHandle::HTMLRewriter })`, the `FetchTasklet::to_body_value` shape. The `ByteStream` is created lazily when a consumer reads `.body` or a body-mixin method; until then, rewriter output buffers in a `Vec<u8>` handed over as `DrainResult::Owned`. - **Input**: mirrors `FetchTasklet::start_request_stream`. Native `ByteStream`/`FileReader` sources wire `byte_stream.sink = SinkHandle::HTMLRewriter` + `lock_native` + `drain()`; other stream kinds go through `JSSink::<RewriterPipe>::assign_to_stream` (new `HTMLRewriterSink` codegen entry). Materialized bodies (`InternalBlob`/bytes `Blob`/`WTFStringImpl`) feed synchronously. - **Backpressure**: `RewriterPipe::write` feeds one chunk through `rewriter.write()` (output chunks push via `ByteStream::on_data`). If the output is paused or a handler suspended, `write` returns `Writable::Backpressure`; `ByteStream::resume` → `SourceHandle::HTMLRewriter::on_ready` → `pipe.resume()` drains `pending_input` then `input_source.ready()`. - **Async handlers**: `handler_callback` returns `HandlerOutcome::{Continue, Stop, Suspend}`. On a pending Promise it runs one microtask checkpoint (`process.nextTick` then promise jobs, never the loop); a genuinely pending Promise suspends. The JS wrapper is retargeted at the heap-parked token so post-`await` mutations land where they should. The `.then()` context is the Transform cell itself (the reactions recover the pipe via `from_js`), so a handler promise collected without settling abandons the parked rewrite instead of leaking it. - **Error handling**: `handler_error` on the pipe replaces the stack `Cell` + `unhandled_pending_rejection_to_capture` override. A handler error on a streaming input now rejects the body with the real error instead of `The rewriter has been stopped.`. - `AttributeIterator` holds a backref to the `Element` plus an index instead of a boxed `slice::Iter`, so `for (const [k, v] of el.attributes) { await ... }` keeps working across a suspension. `ValueBufferer` (~430 lines of `Body.rs`) and its host-fn exports; `SinkHandle::ValueBufferer` + `SinkWriteFn`; `JSSink<ArrayBufferSink>::detach_self`; `crate::Error::{StreamAlreadyUsed, InvalidStream, UnsupportedStreamType}`; `NativePromiseContext::Tag::BodyValueBufferer` (ordinal 4 reused for `HTMLRewriterSuspension`); the two `Bun__BodyValueBufferer__*` `PromiseFunctions` (slots reused for `Bun__HTMLRewriter__onHandler{Resolve,Reject}`). 1. **Error channel is decided by the overload, not by timing.** `transform(string)` / `transform(ArrayBuffer)` throw from `transform()`. Every `Response` input rejects its output body instead. Five existing tests that pinned the old timing-dependent split are updated. Input-body errors (already-failed or aborted body) still throw synchronously from `transform()`. 2. A handler whose Promise needs the event loop to turn makes `transform(string)` / `transform(ArrayBuffer)` throw a `TypeError` (`pass a Response and await its body`) instead of spinning. A Promise that settles within a microtask checkpoint still works. 3. A rejection a handler neither awaits nor returns reaches `unhandledRejection` instead of being captured and thrown from `transform()`. 4. `transform()` types corrected: `Bun.BufferSource` returns `ArrayBuffer` (it always has at runtime); `Blob` removed from the overload (it threw at runtime). 5. `Bun.serve` with an HTMLRewriter-produced response body defers status/headers until the first body byte or clean end, so a handler that fails before emitting any bytes is routed to the server'''s `error()` hook instead of committing `200 OK` then force-closing the connection. Headers never preceded the first byte on this path before either (the old implementation buffered the whole rewrite). All other native-ByteStream bodies (proxied `fetch()`, S3, spawn stdout) keep sending status/headers immediately, and JS `ReadableStream` bodies (`do_render_stream`) are unchanged. `docs/runtime/html-rewriter.mdx` and `packages/bun-types/html-rewriter.d.ts` cover 1-4. - `test/js/workerd/html-rewriter.test.js`: 107 pass, 0 fail under ASAN debug and under `BUN_JSC_validateExceptionChecks=1`. New coverage: async element/text/comment/doctype/onEndTag/document-end handlers, `Bun.gc(true)` while an element is heap-parked across an `await`, re-suspension by a second handler on the same element, nested `transform()` inside a suspended handler, strict document-order across 8 awaiting handlers, `Bun.serve` with a live client, client abort mid-suspension, the consumer matrix for a pending output body, JS-backed and `type:'direct'` `ReadableStream` inputs. - `test/js/workerd/html-rewriter-leak.test.ts`: protected-object + `Response` count regression over 120 suspending rewrites; a never-settling handler rejects the body instead of leaking. - Fail-before: `works with payload of type direct` → `ERR_STREAM_CANNOT_PIPE` on the released bun. - `bun-types` green, `cargo clippy -p bun_runtime` clean. - `vendor/lolhtml/.ref` re-fetch of the fork verified through the real ninja edge. Fixes oven-sh#11758 Fixes oven-sh#14216 Fixes oven-sh#6068 Fixes oven-sh#19305 Closes oven-sh#33243 (same lol-html fork, different input layer), Closes oven-sh#35324 (ResumableSink, deleted in oven-sh#36087), Closes oven-sh#32988 (per-chunk ValueBufferer callback). <!-- robobun:evidence:begin --> --- **no test proof** · iteration 20 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/io/bun-write.test.js test/js/node/process/process.test.js test/js/workerd/html-rewriter-leak.test.ts <!-- robobun:evidence:end -->
3 participants
Fixes #14216
Fixes #11758
Fixes #19305
Repro
transform()throws for any Response whose body is a JS-created ReadableStream. The same body without the rewriter reads fine, and the same rewriter accepts string / Blob /blob.stream()/fetch()bodies. The broken composition is the documented streaming-SSR and middleware shape: build or wrap an HTML body in JS, rewrite it on the way out.Cause
ValueBufferer::buffer_locked_body_valuematched on the stream source kind and rejectedSource::JavaScript | Source::Directoutright.ValueBuffererwas a bespoke "read a whole body into one slice" helper whose only caller was HTMLRewriter, and it never finished implementing the JS-driven source kinds. More fundamentally,BufferOutputSinkowned the lol-html rewriter and was its output target:SinkRefwrote toself.bytesand calledself.done(), so every driver of the rewriter had to holdBufferOutputSinkas a root*mutto avoid re-entering its own&mut. That is the reasonrun_output_sinkonmaintakes*mut Selfandinit()is peppered with "do not hold&mut *sink" notes.Fix
BufferOutputSinkis restructured so the rewriter's output target is a separate allocation and the input is driven per chunk:Output =
ByteStream. The outputResponsebody is aByteStreamfrom the start;SinkRefwrites chunks to it viaon_data, never back intoBufferOutputSink. The self-reference is gone, sofeed/finish/failtake&selfand the raw-pointer field-access pattern ininit()is gone. This also fixesS3Clientwrites empty file forHTMLRewritertransformed fetch Response #19305:.body.getReader(),Bun.readableStreamToText(body)andBun.servereturning a transformed response all read the sameByteStreamregardless of whether the input has settled.Input =
ResumableSink. A third codegen'd sink variantResumableHTMLRewriterSinksits alongsideResumableFetchSink/ResumableS3UploadSink.start_reading_inputdoesto_readable_stream()+ResumableHTMLRewriterSink::initfor stream bodies (includingSource::JavaScript/Source::Direct/ file-backed blobs /Source::Bytes), and a short synchronous path for materialised bodies sotransform(String | ArrayBuffer)still returns a value synchronously.Value::Erroris handled synchronously sotransform()of an already-failed body still throws.Per-chunk
rewriter.write().write_request_datacallsHtmlRewriter::writeper chunk.ResumableSinkContext::AVOID_NATIVE_PIPE(true forBufferOutputSink, default false) routesSource::Bytesthrough the JS pump instead of the native pipe: the pump'sm_readingguard and read-after-write ordering inBunStreamSource.cppprevent the nestedwait_for_promisein an async handler from delivering the next chunk whilewrite()is on the stack, so no re-entrancy guard is needed.feedcopies the chunk beforewrite()because lol-html parses the first chunk straight from the input slice and a handler could otherwise mutate bytes it has not yet tokenized.Error handling.
HandlerErrorScopeis an RAII guard that pointsvm.unhandled_pending_rejection_to_captureat a local cell and installs the quiet rejection handler. Everyfeed/finishcall runs under one, socreate_lolhtml_errorrecovers the original JS error a handler threw on both sync and async paths.fail()stores the error in astrong::Optionalfield so a sync handler error insideinit()still makestransform()throw, and pushes the error to the outputByteStreamso.text()/.bodyreject.Deletions
Net -285 lines.
ValueBufferer(~430 lines ofBody.rs) and its FFI /NativePromiseContext::Tag/PromiseFunctions/crate::Errorsurface.BufferOutputSinkfieldsbytes,response,response_value,body_value_bufferer,tmp_sync_errorand methodson_finished_buffering,run_output_sink,done,write,write_tmp_sync_error.JSSink<ArrayBufferSink>::detach_selfand theArrayBufferJSSinkalias.crate::Error::{UnsupportedStreamType, StreamAlreadyUsed, InvalidStream}.Verification
Tests land in
test/js/workerd/html-rewriter.test.js. 20 new cases cover single/multi/mixed-chunk JS streams,type: "direct", a stream that only produces aftertransform()returns, every consumption path (.text(),.arrayBuffer(),.bytes(),.blob(),.json(),.body.getReader(),Bun.readableStreamToText), handlers observing the document, upstream errors before and aftertransform()returns, a bad chunk type surfacing itsTypeError, reuse of a consumed source, a handler mutating/transferring the source buffer mid-scan, aggressive GC while the source is in flight,.bodyof a transform whose source is still pending, and theBun.serveshape from #11758. The two pre-existingit.todo('works with payload of type direct' / 'default')cases were todo for this reason and now pass.The
transform rejects when the upstream body failssuite is adapted for streaming: chunks delivered before the failure now reach.body, so the assertions read to completion instead of expecting a single rejected.read(); the second-.text()assertion is removed because the body has been consumed (spec behavior).All 21 fail on
mainwithERR_STREAM_CANNOT_PIPE.Runs
Relationship to #33310
#33310 routed the same arm through
readableStreamToArrayBufferand was blocked on the grounds that HTMLRewriter should stop buffering. This PR is the direction that review pointed at: the rewriter runs per chunk, the output is a stream from the start, and the bespoke bufferer is deleted. It supersedes #33310; the test coverage from that PR is carried over here.[review] gate passed · iteration 9 · 18 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 2 rejected · iteration 9
evidence per changed file