[robobun]

Problem

• new HTMLRewriter().transform(new Response(Bun.stdin)).text().then(cb) with a piped stdin exits with code 0 and never runs cb. 1.3.14 ran it. No user reported it.
• FileReader feeds the rewriter from its poll callback (write_chunk_to_sink, on_reader_done, on_reader_error in src/runtime/webcore/FileReader.rs), which runs no microtask checkpoint.

Fix

• FileReader opens an event-loop scope around each such delivery. It checkpoints on exit, after the reader applies the sink's answer.
• attach_sink decides once who owes it: the reader, unless the sink is a FileSink or a fetch request body. A read that a drain ack starts belongs to the ack's caller.
• Verified: test/js/workerd/html-rewriter.test.js, 29 new tests, 26 fail on main 179664e. Notes list 36 more files.
• Self-reviewed: 7 concerns raised, 5 addressed. Rejected: VM::is_entered() as the gate. Open: a Windows FileSink end (Notes).

Background

• A microtask checkpoint runs promise reactions and process.nextTick callbacks. A loop callback owes one for the JS it enters (FileSink: run the microtask checkpoint for JS entered from a writer callback #44250).
• A native sink reads a stream with no JS reader. Only wire_native_sink attaches one.
• Considered a scope in RewriterPipe::end_from_stream (a backpressured chunk loses its jobs), one at the reader's poll entry (no checkpoint between a last chunk and its end), and the JS pump (Notes).

Downsides

• Per chunk, a FileSink pays 8 more instructions (395 to 403), a fetch request body 10. .text grows by 768 bytes.
• A last chunk that the loop delivers runs its jobs before the end handlers. A drain-ack read keeps main's order.
• A drain ack from a callback with no scope runs the jobs a turn late (Bun.serve: run the microtask checkpoint when a writable callback resumes script #46822, Notes).

Notes

All numbers are from Linux x64. A line that names no base is from main 179664e and this branch on top of it. A line marked (fec4a88) is from the base and the code before the rework, and was not measured again. Release builds unless a line says debug. Debug builds have ASAN.

What changed since head 43b227e. Main 179664e is merged in. #46641 removed the variant SinkHandle::ArrayBuffer, and one arm of SinkHandle::leaves_checkpoint_to_source named it: the branch merged as text and did not compile (error[E0599] at src/runtime/webcore.rs:370:27, the only error of a build of the branch alone). A review of the design on the new base kept the owner (the three reader callbacks of FileReader) and asked for two changes to the mechanism, which this head has:

1. leaves_checkpoint_to_source is gone, and webcore.rs is the same as on main. The question it answered on each delivery is settled when wire_native_sink installs the sink, so FileReader::attach_sink answers it once and keeps the answer in one byte (CheckpointOwner). It names the two sinks that opt out and nothing else, so a new SinkHandle variant cannot break it.
2. A FileSink and a fetch request body take main's path plus one compare. The chunk delivery with checkpoints and the read that pull_into_sink starts for a checkpointed sink are out of line and cold.

dc190ea also added a scope in RequestContext::on_writable_byte_stream. e3332aa takes it out again: #46822 scopes that callback for every producer that runs script, and its twin on_writable_response_stream, and the two changes did not merge. #46740 covers a pull() that ends a response.

FileReader.rs and ReadableStream.rs are byte-identical between fec4a88 and 179664e. wire_native_sink has the same callers: the rewriter, FileSink (two) and the fetch request body. FileSink::completion_scope is unchanged. The controller that #46641 added (ReadableSinkController) writes from script or from a native transform, never from a FileReader.

Who is affected. No user has reported this. The repro below is the only report. await, Bun.write(Bun.stdout, out) and for await all keep the loop alive or read through script, and hide it.

Repro (no timer, no top-level await, no beforeExit listener):

(sleep 0.3; echo '<b>x</b>') | bun -e 'new HTMLRewriter().transform(new Response(Bun.stdin)).text().then(t => console.log("then ran:", JSON.stringify(t)))'
# expected: then ran: "<b>x</b>\n"      main prints nothing and exits with code 0

Debug and release builds of main 179664e print nothing in 3 of 3 runs. This branch prints the line in 3 of 3. The pipe can also be a FIFO or a child's stdout. A regular file is read from script, so it was never affected on POSIX.

Why main loses the reaction. The run loop is while vm.is_event_loop_alive() { vm.tick(); vm.auto_tick_active(); } (src/runtime/cli/run_command.rs). auto_tick_active runs the reader's poll callback. The callback feeds the rewriter, the end of the pipe settles the body, and the reaction is queued. The callback returns, nothing is alive, and the loop ends before the next tick(). With a JS reader the same callback settles the read inside StreamResult::fulfill_promise, which holds a scope.

What was lost, and what runs now (fec4a88). A probe with 29 consumers of a slow piped stdin, each in a child process with .then(ok, bad) and nothing else alive:

	main	this branch
no rewriter (16 consumers)	16 run	16 run
rewriter: .text(), .bytes(), .blob(), .arrayBuffer()	lost, promise fulfilled	run
rewriter: out.body.text(), Bun.readableStreamToText(out.body), new Response(out.body).text()	lost, promise pending	run
rewriter: second rewriter on the output, handler that throws, microtask from a document end handler	lost	run
rewriter: getReader() loop, Bun.write(file, out), microtask from an element handler	run	run

10 of 29 lost on main. 0 of 29 lost on this branch, 3 runs.

More shapes, from a later report. The release binaries are from npm, Linux x64, 3 of 3 runs in each cell. "This branch" is its debug build.

Shape	1.3.14	1.4.0 to 1.4.4	this branch	Test
.text().then(cb), the rest of an async function, .blob(), .bytes(), .arrayBuffer()	run	lost	run	"text() runs its reaction", "the rest of an async function", "blob()", "bytes()", "arrayBuffer()"
new Response(Bun.stdin.stream())	crashes, exit code 133	lost	run	"text() of a body made from Bun.stdin.stream()"
a document of 300,000 bytes, also as cat page.html | bun filter.mjs	run	lost	run	"text() of a 300,000 byte document on ..." (2 tests)
process.exitCode = 3, set back to 0 in the callback	exits with 0	exits with 3	exits with 0	none, it is the same reaction

So 1.4.0 is the first release that loses the reaction of a Blob body. A body made from Bun.stdin.stream() was not right in 1.3.14 either.

Bun.spawn(["cat", file]).stdout with the child left ref'd loses the reaction in some runs only. The reaction is lost when nothing is alive after the callback that delivers the end of the stdout. A child that still runs keeps the loop alive for one more tick(), and that tick runs the jobs. A child that was reaped first does not. 100 interleaved runs for each cell, with the child as sleep 0.2; exec cat file, so that a debug build is back in its loop before the child writes (fec4a88):

Document	1.4.4	main (debug)	this branch (debug)
10 bytes	46 lost	78 lost	0 lost
300,000 bytes	51 lost	52 lost	0 lost

The test "the pipe is the stdout of a child that has exited" forces the losing order: the child exits first, and a grandchild holds its stdout. A probe with that order loses the reaction in 30 of 30 runs on main and in 0 of 30 on this branch (fec4a88).

A pipe from cat. Two tests put cat in front of the fixture, as in cat page.html | bun filter.mjs. They close their end as soon as the document is written. busybox cat copies with sendfile(). While that call waits for more input, the kernel holds the lock of the pipe it writes to. The fixture then cannot read the bytes that are already in the pipe: it sleeps in pipe_read. So a test must not wait for the fixture to see a chunk before it closes its end. With a cat that makes the same calls, each of the two tests passes 25 of 25 runs on this branch (fec4a88).

Each clause is needed (debug, one clause switched off at a time, the whole test file). The table is from 1ede34a, which has the same delivery code: since then read_for_drain_caller lost only its pin and its test of the state.

Switched off	Failing tests
nothing	0
scope around a chunk	5: order with the end on the last chunk, handler that throws, handler that waits for the output reader, jobs of a backpressured chunk, document larger than the pipe
scope around the end that comes with the last chunk	3: the three "the end arrives with the last chunk" tests
scope in on_reader_done	18
scope in on_reader_error	1: read error
the DrainCaller state	2: both "a read driven by script" tests
one scope for chunk and end (no checkpoint between)	1: order with the end on the last chunk
the FileSink and fetch opt-out	0. It is a cost only: filesink.test.ts and bun-write.test.js give the same results with and without it.
the pin around the read that a drain ack starts (removed in 4d9a5c6)	0, and no ASAN report in the file, in html-rewriter-leak.test.ts or in four probes that drop a rewrite and collect garbage while it runs

On main 179664e, 3 of the 29 new tests pass: the two "read driven by script" tests and the test for the second read of self.sink. They guard the new code and have nothing to fail on without it.

Checkpoints per reader delivery (debug logs, outermost enter() between one onReadChunk / onReaderDone and the next):

Wire	main	this branch
pipe to rewriter, .text()	0	1
pipe to rewriter, Bun.write(file, out)	0 per chunk, 1 at the end	1
pipe to rewriter, for await over out.body	1	1
pipe to FileSink (Bun.write(file, new Response(proc.stdout)))	0 per chunk, 1 at the end	same
pipe to fetch request body	0	0
pipe to a getReader() loop	1	1

Cost. Instruction counts are from gdb stepi on release builds with symbols, from the entry of the function to its return, without the callee that a row names. The first call of a run is left out. The counts are identical over every other call counted (3 or more), except in the row that gives two.

	main	this branch
on_read_chunk, 64 KiB into a FileSink	395	403
pull_into_sink, the drain ack of a FileSink, without the read it starts	73	73
on_read_chunk into a fetch request body, without SinkHandle::write	95	105
pull_into_sink, the drain ack of a fetch request body, without the read	73	73
on_read_chunk for a for await reader, without Pending::run	249	249
rewriter over a regular file: the two calls of pull_into_sink, without the read	55, 57	67, 69
rewriter over a regular file: on_read_chunk, without SinkHandle::write	77	85
.text (size)	107,582,646	107,583,414
size_of::<FileReader>()	344	344

The head before the rework paid 23 per chunk into a FileSink (395 to 418), 17 per drain ack (73 to 90) and 2 per chunk for a for await reader. Of the instructions that a FileSink chunk still pays, 2 are the compare and its branch. The others are moves and a different branch order in the same code.

Calls of drainMicrotasks for 64 chunks (fec4a88, ranges over runs, they depend on timing):

	main	this branch
into Bun.write("/dev/null")	73 to 78	69 to 77
for await over proc.stdout	132 to 136	131 to 140
rewriter with await out.text()	68 to 75	146 to 159
rewriter with for await over out.body	144, 145	144, 150

Nothing in src/io changes.

Why only a read that the event loop dispatched. pull_into_sink is the sink's drain ack. Its caller can be script, and script can run with the event loop not entered: an 'exit' listener does (until #43741). A scope there runs jobs in the middle of the caller's statement. The two "read driven by script" tests fail without the check. So the rule is: a read that a drain ack starts is checkpointed by the caller of the ack. on_start reads before a sink is attached, and on_pull and set_flowing belong to a JS reader, so pull_into_sink is the only read a FileReader starts that can deliver to a sink.

read_for_drain_caller needs self after the read, to set the owner back. It holds no pin for that. Whoever acks a drain roots the stream (the rewriter's cell is protected or reachable, and a FileSink, a fetch and a server response each hold a strong handle on the stream they read). The read pins the wrapper for its own frames, and the ref of the wrapper outlives the read.

The per-chunk scope depends on #46021. A job that runs in the chunk's checkpoint can resume the reader from inside its own read loop. That read has no scratch buffer and takes 16 KiB. Before #46021, read_state labelled such a read as the end once the writer had hung up, and a 100,684-byte document from cat came out as 81,920 bytes. The test "a document larger than the pipe, from a writer that has exited" covers this through a kernel pipe. Bun.spawn({ stdin: "pipe" }) gives the child a socket pair, so the other tests do not read a pipe.

Callers of a drain ack that hold no scope today. Their handler jobs run a loop turn late. Nothing is lost there, because the sink keeps the loop alive.

• RequestContext::on_writable_byte_stream and on_writable_response_stream: Bun.serve: run the microtask checkpoint when a writable callback resumes script #46822.
• The start of a rewritten Bun.serve response when the fetch handler did not suspend (do_render_with_body wakes the producer under the dispatcher of the request, which drains at points of its own): 574 of 2,500 handler jobs in a probe, with the writable callback scoped. No PR yet.
• FileSink::on_auto_flush acks from inside the deferred task queue, where the exit of a scope runs no checkpoint: only the event loop can close that one. With cat page.html | bun filter.mjs | slow reader and Bun.write(Bun.stdout, out), this branch ran 16 of 2,000 jobs late in 1 of 8 runs (main: 16 to 48 in 8 of 8).
• FileSink::on_ready (the Windows writable slot) and RewriterPipe::cancel_from_output from an aborted request: read, not run.

A FileSink owes its own checkpoints, so attach_sink leaves it out. One of its ends does not hold one: on Windows, for a fd that the sink does not own (Bun.stdout, Bun.file(fd)), end_writer settles the piped stream with no completion_scope. That is on main too, it is read and not run, and it is not fixed here.

Order of the jobs of the last chunk. When the event loop delivers the last chunk, its jobs run before the end handlers, whether the end comes with that chunk or in a later callback. With a 'beforeExit' listener that prints, and the end on the last chunk (Bun.stdin.slice(0, n)), main prints element, end, beforeExit, element microtask, end microtask and this branch prints element, element microtask, end, end microtask, beforeExit. Node runs the jobs of a data callback before end too. This is not the order of a read that a drain ack starts: it delivers the chunk and the end on its caller's stack, so the end handlers run first, as on main. The order test pins the first case only.

Other shapes considered.

• A scope in RewriterPipe::end_from_stream only. A chunk answered with backpressure pauses the reader and the process exits with that chunk's jobs queued: end_from_stream is never called there.
• A scope at each entry of the rewriter pipe. The checkpoint then runs before FileReader has applied a Backpressure answer, so a job that resumes the pipe finds the reader not yet paused.
• A scope at the reader's poll entry in src/io/PipeReader.rs. It gives one checkpoint per wakeup: no job can run between a last chunk and an end in the same wakeup, and every reader parent pays a test.
• No native wiring for a FileReader under a rewriter, so that the JS pump reads the pipe under script. It runs the reaction too. For a 16 MiB document through cat, the whole run retires 33.5 million instructions (32.7 to 34.2 over 5 runs), against 28.9 million (28.9 to 29.1) on this branch and 28.8 million (28.7 to 29.0) on main: about 16% more. It also gives up the native path for every pipe, and on Windows for every file. Only a release build of it was run, with no tests.
• VM::is_entered() in place of the DrainCaller state. It is false when a native callback with no scope acks a drain, so the reader would run its checkpoint inside ByteStream::resume, which holds no ref on its stream and reads it again after the producer returns.
• A tick before 'beforeExit'. It turns lost into late at exit only, and every process pays it.
• A drain in the run loop. FileSink: run the microtask checkpoint for JS entered from a writer callback #44250 refused it: every loop turn pays.

Not run here. Windows and macOS: CI ran them (build 132505 passed on dc190ea, which has the same FileReader paths apart from the pin). bun run rust:check-all passes on this head for x86_64-pc-windows-msvc and aarch64-apple-darwin. The read-error test needs a Unix socket that resets when its peer closes with unread bytes, which only Linux does. The whole-file "read driven by script" test is skipped on Windows, where the event loop reads a file. No test separates a checkpoint per chunk from one per poll callback: that needs two deliveries from one callback, and one read takes everything a socket holds.

Self-review. The head before the rework (43b227e): 13 concerns raised, 10 addressed, 3 not:

1. No gate test survives process: run beforeExit, exit and error listeners with the event loop entered #43741, which makes 'exit' listeners run with the loop entered.
2. No test separates per-chunk from per-callback checkpoints (above).
3. Windows regular files were not run.

The rework (dc190ea): 7 concerns raised, 5 addressed, 2 not.

1. The Bun.serve commit was a second copy of Bun.serve: run the microtask checkpoint when a writable callback resumes script #46822 and did not merge with it. Taken out.
2. The pin and the test of the state in read_for_drain_caller were dead, and the SAFETY comment said what does not hold. Both removed.
3. The statement that the jobs of the last chunk run before the end handlers was too wide. The body and the test comment now say when.
4. and 5. Two points about the Bun.serve test left with that test. What they found went to Bun.serve: run the microtask checkpoint when a writable callback resumes script #46822.
5. Not fixed here: the Windows end of a FileSink over a fd it does not own (above). The comment in attach_sink no longer says that a FileSink always opens its scope.
6. Rejected: VM::is_entered() in place of the DrainCaller state (above).

Suites on the debug build of this head (4d9a5c6), in this container. html-rewriter passes (231). Of 36 more files, 31 pass: html-rewriter-leak (49), html-rewriter-end-error, filesink (83), arraybuffersink (16), bun-stdin-slice, readablestream-helpers, spawnsync-no-microtask-drain, spawn-stdin-readable-stream (4 files), spawn-streaming-stdin, spawn-streaming-stdout, spawn-stream-serve, spawn-stdout-filereader-gc-uaf, spawn-pipe-read-error-leak, spawn-stdout-iterate-leak, spawn-unread-stdout-gc, spawn-socketpair-shutdown, spawn (182), body-stream (9,086), body-stream-excess, fetch-abort-stream-body, stream-fast-path, fetch-backpressure (127), streams (727), sync-pull-fast-path, native-source-onclose-leak, bun-serve-file (176), serve-direct-readable-stream (175), bun-server (86). None of the 37 logs has an ASAN report.

The other 5 fail on the debug build of main 179664e too, in the same container and from the same test directory: bun-write ("Bun.file(0) survives GC", past its 5 s limit), bun-write-leak ("Memory usage is too high" under ASAN), spawnsync-isolated-event-loop (the same 2 tests, past 5 s), fetch.stream ("Content-Length response works (multiple parts)", past 5 s, for 6 kinds of compression here and 7 on main) and serve ("should use correct error when using a root range port(#7187)": the container runs as root).

---

no test proof · iteration 1 · 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

[robobun]

Status of #46557

• Reproduced on main 179664e (Linux x64, debug and release builds). This prints nothing and exits with code 0:

  (sleep 0.3; echo '<b>x</b>') | bun -e 'new HTMLRewriter().transform(new Response(Bun.stdin)).text().then(t => console.log("then ran:", JSON.stringify(t)))'

• bun bd test test/js/workerd/html-rewriter.test.js: 26 of the 29 new tests fail on main 179664e. All 231 tests of the file pass on this branch (head 4d9a5c6), which has that main merged in.
• Release binaries from npm: 1.3.14 runs the callback. 1.4.0 to 1.4.4 do not.
• This head no longer changes Bun.serve: Bun.serve: run the microtask checkpoint when a writable callback resumes script #46822 owns the writable callbacks of a response.
• Not run locally: Windows and macOS.

[robobun]

^{Updated 2:50 PM PT - Oct 25th, 2026}

✅ @robobun, your commit 4d9a5c64d863f3add5f78d8349848ff487e539ab passed in Build #132760! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 46557

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

bun-46557 --bun

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: oven-sh/bun
• Branch: robobun/c5fae459/filereader-sink-checkpoint
• File: .coderabbit.yaml
• Commit: 4d9a5c6
• Layers: Repository YAML
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 18ffd045-44cd-4edf-b49c-d90f17a81e5c

Commits

Reviewing files that changed from the base of the PR and between dc190ea and 4d9a5c6.

Files selected for processing: 2

• src/runtime/webcore/FileReader.rs
• test/js/workerd/html-rewriter.test.js

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

Walkthrough

FileReader now tracks checkpoint ownership for attached sinks and applies event-loop scopes to reader-owned sink deliveries. ReadableStream uses the new sink attachment method. HTMLRewriter tests cover pipe-driven delivery, backpressure, cancellation, errors, and synchronous reads.

Changes

Checkpoint-aware stream delivery

Layer / File(s)	Summary
Sink ownership and read routing src/runtime/webcore/FileReader.rs, src/runtime/webcore/ReadableStream.rs	FileReader assigns checkpoint ownership when sinks attach and routes reads for reader-owned sinks through a drain-caller ownership state. ReadableStream now calls attach_sink.
Checkpoint-scoped delivery and validation src/runtime/webcore/FileReader.rs, test/js/workerd/html-rewriter.test.js	FileReader applies checkpoint scopes to eligible chunk writes, EOF, completion, and errors. HTMLRewriter tests cover pipe delivery, backpressure, cancellation, errors, and synchronous reads.

Possibly related issues

• oven-sh/bun#46558 — This PR adds checkpoint scopes to FileReader sink deliveries. The issue reports a similar undrained-microtask symptom in a separate setImmediate callback path.

Suggested reviewers: dylan-conway

Priority: Normal

Merge Risk: Minimal · up to 4d9a5

No actionable merge-blocking risk is established; merge after normal checks.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title "HTMLRewriter: run the microtask checkpoint for loop-dispatched pipe reads" names the main change: FileReader now opens an event-loop scope and runs a microtask checkpoint for reader-owned s…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The description follows the repository template with both required sections, "What does this PR do?" and "How did you verify your code works?". It gives a detailed problem statement, the fix, a root-c…

Full details: Title check

Explanation

The title "HTMLRewriter: run the microtask checkpoint for loop-dispatched pipe reads" names the main change: FileReader now opens an event-loop scope and runs a microtask checkpoint for reader-owned sink deliveries (write_chunk_to_sink, on_reader_done, on_reader_error) that feed HTMLRewriter from a pipe. It matches the raw summary and the objectives. The title omits the drain-ack distinction and the checkpoint ownership model, but it does not need to cover every detail. The objectives also name Bun.serve drains, but the current PR no longer changes RequestContext, so omitting that is accurate.

Full details: Description check

Explanation

The description follows the repository template with both required sections, "What does this PR do?" and "How did you verify your code works?". It gives a detailed problem statement, the fix, a root-cause explanation, the repro command, the test counts, and a per-clause breakdown of which tests fail when each part is removed. The verification is thorough for Linux x64. Windows and macOS were not run locally, and the description states this. The bot evidence note flags platform-specific tests that are deferred to CI. The description is long, and the Notes section is dense, but the required information is present and complete.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[claude]

I reviewed this PR and didn't find any bugs. Because it widens the window where user JS runs inside the pipe reader's delivery path (microtasks now run between a chunk and its end, and between a backpressure answer and the next read), a human look at the re-entrancy and ordering implications would still be worthwhile.

What was reviewed:

• Lifetime across the new checkpoints: on_read_chunk runs under the read loop's ref_/deref pin, and on_reader_done/on_reader_error hold their own _pin, so the second self.sink.get() after the chunk scope and the field writes after sink.end() land on a live box; the pull_into_sink pin covers the pulling_into_sink reset after a read that may drop the last ref.
• Scope placement: each scope drops after the sink's Backpressure/Err/Done answer is applied, and the nested-call guard on pulling_into_sink resets only in the frame that set it.
• Sibling delivery sites: the buffered write in pull_into_sink is the one path still delivering to the rewriter with no checkpoint, which the description names as a known one-turn-late case; FileSink and fetch body opt-outs match the scopes those sinks already own.
• Tests: fixtures spawn without timers or top-level awaits, drain stdout/stderr concurrently, and assert {stdout, stderr, exitCode} together; the kernel-pipe test is skipIf(isWindows) and the ECONNRESET test skipIf(!isLinux).

Extended reasoning...

The change adds a SinkHandle::leaves_checkpoint_to_source() classifier in src/runtime/webcore.rs and, in src/runtime/webcore/FileReader.rs, a pulling_into_sink flag plus an RAII event-loop scope around the four places the pipe reader's callback delivers chunks, EOF, or errors to a native sink, with 24 new subprocess tests in test/js/workerd/html-rewriter.test.js. It touches no security-sensitive surface (no auth, crypto, or input parsing), and no CODEOWNERS entry covers the changed paths. The Rust diff is 45 lines, but it changes when user JS can run relative to the reader's state (microtask checkpoints now fire mid-delivery), which is a subtle re-entrancy and ordering change in a shared streams path with raw-pointer lifetime reasoning, so a human maintainer should still weigh it; the hunt ran dry with no findings and the six ruled-out candidates all concerned that same window.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline nit, I also checked two liveness questions in src/runtime/webcore/FileReader.rs and ruled them out: the re-read of self.sink after the chunk scope exits in write_chunk_to_sink runs under the BufferedReader vtable's ref_/deref pin that already brackets every event-loop read loop, and pull_into_sink only resets pulling_into_sink on the frame that set it (a nested call sees replace(true) return true and takes neither pin nor reset), so the flag and the pin balance on re-entry. The "No FileReader is wired to these" comment on leaves_checkpoint_to_source matches the callers: only the fetch body, FileSink and HTMLRewriter call wire_native_sink, and the S3 client attaches to ByteStream only.

Extended reasoning...

The change adds an event-loop scope around each FileReader delivery into a native sink that enters JS (HTMLRewriter), gated by a new SinkHandle predicate and a pulling_into_sink flag that suppresses the scope for reads driven from the drain-ack path, plus a RefPtr pin in pull_into_sink and ~460 lines of subprocess tests. It touches no auth, crypto, or input-parsing surface. The code since my prior review is test-only (two commits), and the one finding this run is a serial-vs-concurrent test nit; the Rust change is small but sits on unsafe refcount and microtask-checkpoint ordering in a shared stream source, so a human look at the ordering of scope exit versus reader pause/unpause is still worthwhile.

[claude]

The read-error test is now concurrent with its siblings, which addresses the one nit I had; I re-ran the review on this push and found no bugs, but since the change alters when microtask checkpoints run for every pipe-fed native sink, a maintainer's look at that design is still worthwhile.

What was reviewed:

• pull_into_sink: the init_ref pin is taken only on the branch that sets pulling_into_sink, the flag is cleared before the pin drops, and both callers (on_ready, the hookup in ReadableStream.rs) touch nothing after the call.
• write_chunk_to_sink: the chunk scope drops before self.sink is re-read for the end path, so a job that cancels the output cannot leave a stale sink in use; on_reader_done/on_reader_error keep their existing _pin across the new scope.
• leaves_checkpoint_to_source: FileSink really does open completion_scope where it enters JS, so the opt-out is consistent; the remaining variants default to opening a scope.
• Tests: pipes drained via Promise.all, conditions awaited via stderr readiness lines rather than timers, combined {stdout, stderr, exitCode} asserted, socket pair closed in finally.

Extended reasoning...

The PR adds 45 lines to src/runtime/webcore.rs and src/runtime/webcore/FileReader.rs that open an event-loop scope around each event-loop-dispatched delivery from a FileReader to an HTMLRewriter/S3Upload sink, plus a 469-line test block in test/js/workerd/html-rewriter.test.js. It touches no auth, crypto, or input-parsing surface. The Rust change is unchanged since the first commit of the PR; the only new commit since the last review makes the read-error test concurrent, addressing the earlier nit. Deferring rather than approving because the change shifts microtask checkpoint timing and adds a per-chunk scope on a stream hot path, involves an unsafe refcount pin around a re-entrant read, and the PR itself documents a remaining one-turn-late case for drain-resumed reads that a maintainer should weigh.

[claude]

I re-reviewed the current head (the concurrent-test change and the post-merge SinkHandle fixup) and found no bugs; the earlier serial-test note is addressed. Because this changes when the microtask checkpoint runs inside the pipe reader's own read-loop callback, a maintainer's look is still worthwhile before merge.

What was reviewed:

• leaves_checkpoint_to_source() is exhaustive over the current SinkHandle variants after the ArrayBuffer removal, and only FetchRequestBody/FileSink/HTMLRewriter can actually be wired to a FileReader (FileSink opens its own completion_scope, so the opt-out is consistent).
• Every sink_delivery_scope site drops before on_close()/decrement_count, and event_loop is set at every FileReader construction site and in on_start, so entered() never sees the null sentinel.
• The pulling_into_sink pin: PosixBufferedReader::read refs the parent only for its own duration, so the extra RefPtr::init_ref is needed for the flag reset after the read; the outermost-only reset handles nested pull_into_sink.
• The one surface worth a human eye: a checkpoint at the end of write_chunk_to_sink's backpressure path can re-enter IOReader::read while the outer read_loop frame is still live. The scratch-buffer claim guard and the post-callback is_done() re-check appear to cover it, and the "handler that waits for the reader" / "larger than the pipe" tests exercise that path, but it is new re-entrancy on a hot I/O path.

Extended reasoning...

The PR adds an RAII event-loop scope around each FileReader delivery to a JS-entering native sink (HTMLRewriter today) so queued promise reactions and nextTick jobs run before the loop can exit, plus a pulling_into_sink flag that suppresses the scope for reads driven on a caller's stack; it touches src/runtime/webcore.rs, src/runtime/webcore/FileReader.rs, and appends ~470 lines of subprocess tests to test/js/workerd/html-rewriter.test.js. No security-sensitive surface (no auth, crypto, or untrusted parsing) is involved. Since the prior review the only changes are making the Linux socket-pair test concurrent, a merge with main, and dropping the removed SinkHandle::ArrayBuffer arm; the core logic is unchanged and no CODEOWNERS entry covers these files. Deferring rather than approving because the change introduces microtask execution (and possible re-entrant IOReader::read) inside the reader's poll callback, which is subtle event-loop/lifetime behavior rather than a mechanical change.

[claude]

I reviewed the latest push (the attach_sink rework and the new on_writable_byte_stream scope) and found no bugs, but since this changes when microtask checkpoints run inside the pipe reader's poll callback and the Bun.serve drain path, a human look is still worthwhile.

What was reviewed:

• Liveness across the new checkpoints in write_chunk_to_sink: IOReader::read/on_poll hold ref_parent() (which increment_counts the source) across on_read_chunk, and the EOF/error paths keep their own _pin, so the self.sink.get() re-read after the chunk scope drops is on live memory.
• read_for_drain_caller: nested enter() only drains at entered_event_loop_count == 1, so a drain-driven read on a JS caller's stack cannot checkpoint mid-statement; the owner reset is guarded by the re-check and self is pinned for it.
• on_writable_byte_stream: pinned is declared before _exit so it is dropped after the checkpoint; the early return true matches the non-rewriter return value and the is_aborted_or_ended guard runs before it. Only one FileReader sink-wiring site exists (ReadableStream.rs), so no caller bypasses attach_sink.
• Tests await observable stderr lines rather than time, drain stdout/stderr/exit concurrently, and the one serial test states its reason.

Extended reasoning...

The change touches src/runtime/webcore/FileReader.rs (new CheckpointOwner state, event-loop scopes around chunk, EOF and error deliveries to a native sink, and a drain-caller flip around the synchronous read in pull_into_sink), src/runtime/webcore/ReadableStream.rs (one call switched to attach_sink), src/runtime/server/RequestContext.rs (a pinned event-loop scope around resuming an HTMLRewriter-backed response body on socket drain), and ~530 lines of new subprocess tests. It touches no auth, crypto, or input-parsing surface. It was not approved because the fix alters microtask-checkpoint timing inside native I/O callbacks for every FileReader sink except FileSink and fetch bodies (including ServerResponse, S3 and HTTP sinks fed from pipes), which is a runtime-semantics and perf tradeoff a maintainer should weigh, and the RequestContext.rs scope is new since the previous review.

[robobun]

Update: this is settled. e3332aa drops dc190ea here, so this PR no longer changes src/runtime/server/RequestContext.rs. The head of this PR (4d9a5c6) and the head of #46822 (23870f7) merge with no conflict as text. I did not build the merged tree.

The earlier note, for the record:

This PR and #46822 changed the same lines of RequestContext::on_writable_byte_stream. A merge of the two heads (dc190ea and c5c5713) conflicted in src/runtime/server/RequestContext.rs.

• This PR, since dc190ea, opened an event-loop scope there when the producer of the response body is an HTMLRewriter, with the request context pinned.
• Bun.serve: run the microtask checkpoint when a writable callback resumes script #46822 opens a scope there for every producer that runs script (ByteStream::producer_runs_script()), and also in on_writable_response_stream.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.