Repository navigation
HTMLRewriter: cancel a still-open JS stream input when the rewrite fails or its output is cancelled - #42130
Conversation
…ils or its output is cancelled Since #41894 a JSSink controller's detach() no longer runs a direct stream's cancel(): only JSSinkController__onClose (the destination went away) does. RewriterPipe::detach_input_source only detached a JS controller, so a type: "direct" input kept producing into a dead pipe after the output reader cancelled, a handler threw, or the HTTP client behind a transformed Bun.serve response disconnected, and its next write() threw instead of returning 0. Close the controller with a reason before the detach on those paths: the handler's error for a failed rewrite, an AbortError for a cancelled output. A default ReadableStream input, which the pump already cancelled with undefined, gets the same reason.
|
Status: reproduced on main (e3b6d63, debug build) and fixed. 1.4.3 behaves differently again (it called Reproduction: // bun repro.mjs
const events = [];
const hold = Promise.withResolvers();
const input = new Response(
new ReadableStream({
type: "direct",
async pull(c) {
c.write("<p>one</p>");
c.flush();
await hold.promise;
try {
events.push("write after -> " + c.write("<p>two</p>"));
} catch (e) {
events.push("write after threw: " + e.message.slice(0, 45));
}
},
cancel(reason) {
events.push("cancel(" + (reason?.name ?? reason) + ")");
},
}),
);
const reader = new HTMLRewriter().on("p", { element() {} }).transform(input).body.getReader();
await reader.read();
await reader.cancel();
hold.resolve();
await new Promise(r => setImmediate(r));
console.log(events);main: The same holds for a handler that throws (the source's Test: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughChangesThe HTML rewriter now propagates abort and rewrite failure reasons to JavaScript input sources. Native sources retain close behavior. Tests cover stream cancellation, handler failures, client disconnects, pending pulls, and post-cancellation writes. HTML rewriter cancellation
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This change propagates rewrite failures and output cancellation to open HTML rewriter input streams, including cancellation reasons. No concrete merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds a new user-JS re-entry point inside RewriterPipe's terminal paths (the cancel(reason) callback now fires while the pipe is tearing down), a human look at the lifecycle ordering would still be worthwhile.
What was reviewed:
cancel_from_outputreordering —done/phaseare now set beforedetach_input_source, andinput_sourceis replaced withNonebeforeupstream.cancel()runs, so theend_from_js→end_from_stream(None)re-entry is a no-op under the caller'sPipePin; this now matches the orderingfail()already had.fail()callingerr.to_js()before the output receives it —ValueError::to_jscaches, so the input'scancel()and the output error see the same object.SourceHandle::cancel— samehas_exception()guard as theclose/abortit was extracted from.- Tests await real conditions (
settledresolver,once(socket, "data"),pendingRequestspoll), useport: 0, assert exact event arrays, and cover reader-cancel / handler-throw / client-disconnect / default-stream variants.
Extended reasoning...
Overview
This PR restores the cancel(reason) callback for a still-open JS ReadableStream input to HTMLRewriter.transform() when the rewrite terminates abnormally (output reader cancels, handler throws, or HTTP client disconnects). The Rust change is ~40 lines across two files: detach_input_source gains an Option<JSValue> reason and closes a JSController source with it before detaching; cancel_from_output moves its done/phase writes before the detach so the new re-entry through end_from_js short-circuits; fail() passes the handler error as the reason; and SourceHandle grows a cancel(reason: JSValue) sibling of abort(). Four new tests in html-rewriter.test.js cover the direct-stream variant matrix plus the default-stream reason.
Security risks
None. This is stream-lifecycle plumbing between HTMLRewriter and its input ReadableStream; no auth, crypto, filesystem, or network-parsing surface is touched. The only new user-controlled value flowing through is the handler's own thrown error, which was already surfaced to the output and is now also handed to the input's cancel().
Level of scrutiny
Medium-high. The change is small and mirrors the existing fail() ordering, but it introduces a user-JS callback (cancel(reason)) mid-teardown, which is exactly the re-entrancy hazard REVIEW.md calls out ("anything that can run user JS can synchronously free your state"). I traced the re-entry: input_source is replaced to None before upstream.cancel() fires, done is set, and the PipePin is held, so the recursive end_from_stream(None) sees done and only runs an idempotent release_input_roots(SourceHandle::None). That looks correct, but someone who owns this file confirming there is no other observer of the old detach_input_source → done.set ordering in cancel_from_output would be valuable.
Other factors
Test coverage is solid and follows the harness rules (port 0, Promise.withResolvers, awaited conditions rather than sleeps, exact .toEqual on ordered event arrays, using server). The HTTP-disconnect test polls server.pendingRequests via setImmediatePromise() rather than sleeping. No CODEOWNERS entries match the changed paths. The bug hunt ran to dry_streak with no candidates. The PR description's re-entrancy walkthrough matches what I read in the code, and the extensive local suite list (including BUN_JSC_validateExceptionChecks=1) suggests the exception-scope discipline was checked.
|
Updated 2:29 AM PT - Sep 9th, 2026
✅ @robobun, your commit 35826e715a9b5216fa36db1271ee2cbfc24d4e72 passed in 🧪 To try this PR locally: bunx bun-pr 42130That installs a local version of the PR into your bun-42130 --bun |
|
On the
On main that |
|
One related item from reviewing this, recorded here since nothing else tracks it:
Both are one-line changes plus a reason assertion in the fetch upload and S3 stream tests. Not done here to keep this PR to the HTMLRewriter regression. |
…w into it (#44862) ### Problem - When the consumer of `new Response(asyncIterable)` leaves, the iterator can stay open for good. `Bun.serve`, the client hangs up: `return() calls: 0; listeners left on the emitter: 5`. 1.4.2 returns all five. - The cancel hook picks `reason ? iterator.throw?.(reason) : iterator.return?.()` (`src/jsc/bindings/webcore/streams/BunAsyncIterableSource.cpp:462`). The error tail calls `iterator.throw?.(error)` (`:184`). Since #41456, #41757, #41894 and #42130 (none released) consumers send a reason. - An iterator with no `throw()` gets no call. A generator that catches the throw never runs `finally`. ### Fix - Every cancel calls `return()`, the rule of #13687 and `docs/guides/http/sse.mdx:19`. - The error tail follows `for await`: a failed write calls `return()`, a failed iterator gets no call. A write that fails at once takes this path too. - Correct because a body cannot catch `return()`. node and `ReadableStream.from()` never call `throw()`. - Verified: 120 new cells in 5 test files, 115 fail on main. Self-reviewed: 14 concerns raised, 13 addressed. ### Background - The body is a direct `ReadableStream` pumped from `iterator.next()`. The cancel hook runs when the consumer leaves, the error tail when the pump fails. - Considered `return()` only where `throw()` is missing: a generator that catches stays open. A reason-less close at `streams.rs:2015` misses `FileSink::on_close`. ### Downsides - Reverses #41757 (unreleased): `cancel(reason)` no longer throws into a generator. Released change: `catch` no longer sees a failed write, and an iterator with only `throw()` gets no call. - Not closed: a generator that never reaches a `yield` again. - Cost: a refused chunk takes one more reaction (14 to 15 cells). Per chunk: none. Release text: -1,280 B. <details><summary>Notes</summary> **Who sends a reason (all end in the same cancel hook)** | Producer | Since | Reason | | --- | --- | --- | | `HTTPServerWritable::abort` (`streams.rs:2015`): client hang-up, idle timeout, `server.stop(true)` | #41894 | connection-closed `AbortError` | | `FileSink::on_close`: a failed write on `Bun.spawn` stdin or `Bun.write` to a pipe or file | #41456 | the write error | | HTMLRewriter input cancel | #42130 | the handler error or an `AbortError` | | `reader.cancel(reason)`, `pipeTo` error or abort, `tee()`/`clone()`, `Readable.fromWeb().destroy(err)`, `fetch()` abort | #41757 | the caller's reason | The producers are not changed. Direct and default streams still get the reason in `cancel(reason)`. **Rulings this follows** - #13687 (in every release since 1.1.27) wrote the abort arm: "undefined === Abort. We don't want to throw here because it will almost inevitably become an uncatchable exception." - `docs/guides/http/sse.mdx:19`: "If the client disconnects, the generator's `finally` block runs so you can clean up." - #41894 lists "Async generator and `Response` bodies" under "Not changed". - #33020 and #33167 set the rule for pipes: a sink that closes under an async iterable returns the generator. #36087 made the pump wait on a FileSink write, which put the error tail in front of that close. **What it reverses** - #41757 stated "`cancel(reason)` throws the reason into the generator (as `Bun.serve` already does on abort)". At that commit `Bun.serve` called `cancel(undefined)`, which returned the generator. Its test `cancel(reason) on an async generator body throws the reason into the generator` now expects `finally` and no `catch`. - #41894's test `async generator body: client abort between yields throws the connection-closed error into the generator once, then finally` now expects `["finally"]`, which is what 1.4.2 gives. - The error tail has called `iterator.throw?.(error)` since 1.0.27. No existing test observed it. - Not reversed: what `return()` itself throws still replaces the body's error (#39913). A rejected `return()` promise does not. **Results** (debug ASAN build unless a build is named) | Check | main bd599f5 | this PR | 1.4.2 | | --- | --- | --- | --- | | The 8 ledger scripts at exit 0 | 0 of 8 | 8 of 8 | 7 of 8 | | Twelve ways a JS consumer leaves, iterator not returned | 7 of 12 | 0 of 12 | 12 of 12 | | A body that ends its own controller, then yields: shapes left open (of 4) | 2 | 0 | 0 | | `Bun.write` to `/dev/full`, 64 KiB chunks | pulls for ever | rejects `ENOSPC` after 1 pull, body closed | pulls for ever | node v26.3.0 returns the iterator in 12 of 12 ways and never calls `throw()`. New tests on release builds of main and of this PR: | File | main | this PR | | --- | --- | --- | | `test/js/web/fetch/body-async-iterator.test.ts` | 10 pass, 40 fail | 50 pass | | `test/js/bun/http/async-iterator-stream.test.ts` | 97 pass, 42 fail | 139 pass | | `test/js/workerd/html-rewriter.test.js` (new rows) | 10 fail | 10 pass | | `test/js/bun/spawn/spawn-stdin-readable-stream.test.ts` (new rows) | 10 fail | 10 pass | | `test/js/bun/io/bun-write.test.js` (new rows) | 14 fail | 14 pass | | `test/js/bun/http/serve-direct-readable-stream.test.ts` (rewritten pin) | 1 fail | 1 pass | The 5 new cells that pass on main are the `reader.cancel()` controls with no reason. Each cell runs five body shapes from `test/js/web/streams/async-iterable-body-shapes.ts`: an iterator with `next()` and `return()`, one with `throw()` too, a generator with try/catch/finally, a generator whose loop catches, and a node `Readable` with `autoDestroy: false`. **The two commits** - Commit 1 changes only the cancel hook. Built alone, the 8 ledger scripts exit 0. It leaves the end-then-yield route open for 2 of 4 shapes, and `/dev/full` with 64 KiB chunks still pulls for ever. - Commit 2 changes the error tail and adds the rejected-write branch. Both routes above need it. **Still open, for a maintainer** - A generator acts on `return()` at a `yield`. One that waits in an `await` is returned when that `await` settles (the same on 1.4.2). The docs paragraph says so and shows `req.signal`. - A generator that writes through the yielded controller in a loop and never yields again is not stopped. 1.4.2 stopped it because `write()` threw once the client left. Since #41894 `write()` returns 0. The question is which of the two a write through the yielded controller must do after the consumer left. - A `return()` that throws synchronously while `Bun.serve` aborts the body is an uncaught exception, as on 1.4.2. - Open PR #44181 adds three tests that expect a refused chunk's error in a generator's `catch` (`thrown: outOfMemory`). They need a `finally` flag so that neither PR turns main red. - Not run: Windows and macOS. The stdin rows are skipped on Windows because `fs.closeSync(0)` does nothing there. HTTPS, HTTP/2 and HTTP/3 share the abort path by reading, and only HTTP/1.1 was run. **Measurements** (release builds of main bd599f5 and of this PR) - Release text (`size`): 88,771,341 to 88,770,061 B (-1,280 B). Reaction handlers per global: 98 to 98. - Cells per chunk (`heapStats` slope, GC off, 2,000 to 6,000 chunks, 3 runs): 3.000 to 3.000 on `Bun.serve` and `.bytes()`. - Per client abort of a generator body (N=200): Promise 10 to 9, reactions 6 to 5. - `body.cancel(err)`: an iterator with `throw()` Promise 4 to 3. An iterator with `return()` only Promise 2 to 3, because `return()` is now called. - Error tail per failing body under `.text()` (N=1000): a generator that throws 13 to 9 cells and 4 to 3 microtask turns. A refused chunk with a generator 14 to 15 cells and 6 to 7 turns. A refused chunk with `{next, return}` 9 to 13 cells, because `return()` is now called. - End of a body whose `return()` gives a plain object and not a promise: 10 to 12 cells. The result is now awaited, so that a thenable or a lazy `Bun.sql` promise runs. - Static (`llvm-objdump`): the four iteration-protocol exception checks in the pump go from 3 instructions to 2. A `write()` that returns a promise gets one more status compare. - Not measured: dynamic instruction counts. `perf` hardware counters fail in the build VM and valgrind could not be installed. **Suites run** on the debug ASAN build: the six files above, `streams.test.js`, `body.test.ts`, `body-stream.test.ts`, `fetch-args.test.ts`, `serve-stream-body-error.test.ts`, and the two main files with `BUN_JSC_validateExceptionChecks=1`. In a CPU-capped container the subprocess tests of `serve-direct-readable-stream`, `spawn-stdin-readable-stream`, `bun-write`, `streams` and `serve-stream-body-error` time out at the default concurrency. All pass with `--max-concurrency 4 --timeout 60000`. Two older chunk-count tests in `async-iterator-stream.test.ts` (`async generator function`, `[Symbol.asyncIterator]`) fail in 2 of 6 debug runs with the new tests filtered out. **Self-review, the one concern not taken:** deleting the `lastResort` overload of `atStreamsBoundary`. The error tail still uses it, because what `return()` throws still replaces the error. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/workerd/html-rewriter.test.js, test/js/bun/spawn/spawn-stdin-readable-stream.test.ts, test/js/bun/io/bun-write.test.js, test/js/bun/http/serve-direct-readable-stream.test.ts <!-- robobun:evidence:end -->
2 participants
Problem
type: "direct"input toHTMLRewriter.transform()no longer getscancel()when the output reader cancels, a handler throws, or the client behind a transformedBun.serveresponse disconnects. The source keeps producing, and its nextwrite()throws instead of returning0. 1.4.3 calledcancel(undefined).RewriterPipe::detach_input_source(src/runtime/api/html_rewriter.rs) only detached a JS controller. Since Bun.serve direct streams: flush on controller.close(), end the response once, call cancel() only on a client abort #41894detach()does not callcancel()(it is also the source's own close). OnlyJSSinkController__onClose, the destination-went-away signal, does.Fix
detach_input_sourcecloses a JS controller with a reason before the detach: the handler's error fromfail(), anAbortErrorfromcancel_from_output(). That isJSSinkController__onClose, socancel(reason)runs once and later writes return0.cancel_from_output()marks the pipe done before the detach, asfail()does, because closing the controller re-entersend_from_js.SourceHandle::cancel(reason)is the arbitrary-reason form ofabort().test/js/workerd/html-rewriter.test.js(4 new cases fail on 1.4.3 and on main), plus the suites in Notes.Background
HTMLRewriter.transform(response)pipes the input body into aRewriterPipesink. A JS stream input goes through a JSSink controller, which a direct stream'spull()writes into.detach()(the source closed, or the owner released a finished sink) andJSSinkController__onClose(reason)(the destination went away). Only the second runs the source'scancel(reason)and makes laterwrite()/flush()return0.ReadableStreaminput is pumped byreadStreamIntoSink, whose close handler cancels the stream on either signal, before withundefined, now with the reason.Notes
The review of #41894 flagged this path after the merge. Checked on main at e3b6d63 with a debug build: for
reader.cancel()on the output, a throwing handler, and a client disconnect on aBun.serveresponse that servesrewriter.transform(directStreamResponse), the direct source'scancel()never ran and thepull()that resumed afterwards threw the "already been closed" message from its nextwrite(). With this change each case records onecancel(reason)and the nextwrite()returns0, the same contract #41894 gave the HTTP sink for a peer that went away.The reason for a cancelled output is
CommonAbortReason::UserAbort("The operation was aborted."). The JS reason passed toreader.cancel(reason)does not travel through the outputByteStream's native cancel, and the output already hands the sameAbortErrorto a native sink wired to it (ByteStream::on_cancel). A client disconnect reaches the pipe through that sameByteStreamcancel, so it also reads asUserAbortrather thanConnectionClosed.Re-entrancy: closing the controller runs
directStreamOnClose/pumpOnClose, which call the controller'send(), which callsRewriterPipe::end_from_js→end_from_stream(None). Both callers setdonefirst, so that pass is a no-op apart from an idempotentrelease_input_roots(None), and the caller'sPipePinkeeps the pipe alive across the controller'srelease_pump_ref. The sameend()re-entry already happened throughdetach()before this change; only thecancel(reason)call andm_destinationClosedare new.fail()now materializes the error's JS value before the output receives it;ValueError::to_jscaches it, so the output and the source'scancel()see the same object.Suites run locally on the debug ASAN build:
test/js/workerd/html-rewriter.test.js(184/184, also withBUN_JSC_validateExceptionChecks=1for the direct-stream cases),html-rewriter-leak.test.ts,html-rewriter-end-error.test.ts,test/js/web/streams/streams.test.js,test/js/bun/http/serve-direct-readable-stream.test.ts,test/js/bun/stream/direct-readable-stream.test.tsx,test/js/bun/http/serve-stream-body-error.test.ts.