Conversation
A poll wakeup that reports a hangup does not mean that one read took every byte. read_state labelled every delivery Eof under a hangup, also when fill_scratch or fill_buffer stopped because the delivery was large enough. A native sink ended there: Bun.write(file, proc.stdout) wrote 262144 of the bytes and resolved, a Bun.serve file response sent 262144 and ended, and a read error behind the delivery was dropped. read_state now returns Eof only for a 0-byte read, a used-up limit or a used-up byte budget, the cases in which the reader is closed before the delivery. The end of any other stream comes through on_reader_done.
|
Status: ready for review. Merge #37082 first, this PR is stacked on its branch. How to reproduce on main (Linux, no timing needed):
On 1.4.1, 1.4.2 and main the promise resolves and the file has 262144 bytes. With this PR the file has all 270000 bytes. The rows in The rows pass on Linux and on macOS (x64 and aarch64). The defect without the fix was measured on Linux only. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the remaining received_hup handling in read_loop (src/io/PipeReader.rs:858-883) and close_if_final against the new read_state: Eof is now only labelled where close_if_final has already closed the reader, and the hung-up-pipe drain path still terminates through a 0-byte read (Stop::Eof) or on_reader_done, so no path is left without an end-of-stream signal. The Windows libuv reader (WindowsBufferedReader::on_read, ~:1881) never derived Eof from a hangup, so it is not affected by the same defect.
Extended reasoning...
The Rust change is a three-hunk edit to PosixBufferedReader in src/io/PipeReader.rs that drops the received_hup heuristic from read_state so partial deliveries are labelled Progress instead of Eof; the rest of the diff is socketpair-based regression tests in three existing test files. It touches no security-sensitive surface. Inline findings were reported (chunked-vs-Content-Length behaviour change for Bun.serve, a wmem_max-dependent test precondition, duplicated test helpers), so a human decision on the Bun.serve wire-format trade-off is needed rather than approval.
The rows in spawn.test.ts, bun-serve-file.test.ts and streams.test.js take their sockets, their constants and their payload from test/socketpair.ts. The spawn fixtures are files now, so that they can import it. The module does not import the harness: a fixture process of a debug build takes seconds to load it. The rows skip on a host whose limit for a socket buffer cannot hold their bytes.
|
Updated 8:52 AM PT - Sep 26th, 2026
✅ @robobun, your commit 0261ed02d3d4200d15f16842f033d998005e5ceb passed in 🧪 To try this PR locally: bunx bun-pr 44036That installs a local version of the PR into your bun-44036 --bun |
|
Thank you for the review. Commit 814a129 takes two of the three findings.
The source of the PR is the same as before. |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The follow-up commit addresses two of my earlier notes: the socketpair helper now lives in one place (test/socketpair.ts), and the holds() probe skips the hangup rows on a host whose socket-buffer limit is clamped instead of failing on queued.
Beyond the inline findings, I also checked the pushback path the relabel newly reaches: a sink that returns Backpressure pauses the reader, yet under a hangup the loop at src/io/PipeReader.rs:858 keeps reading and delivering. That continuation is pre-existing (unchanged by this diff), the sinks queue the extra writes rather than drop them, and the unix-listener and suspending-HTMLRewriter rows exercise it.
Extended reasoning...
The production change is three lines in src/io/PipeReader.rs (read_state drops received_hup, so a size-bounded delivery is labelled Progress and the real end arrives via on_reader_done/on_error); the rest is a shared test helper and new rows in three test files. No security-sensitive surface is touched. It stays at defer because the core pipe reader feeds streams, subprocess stdio, the shell and file responses, and an earlier inline note on the Content-Length-to-chunked behavior change remains open.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
The socketpair helper asks for a socket buffer of 1 MiB. macOS before 14.4 refuses a size above kern.ipc.maxsockbuf with ENOBUFS. The helper threw then, and three test files call it while they load, so each file failed as a whole. - The helper does not check the result of setsockopt(). A socket that keeps its default size takes fewer bytes. - limitIsBelow() replaces holds(). It never throws. It is true when a socket takes only a part of the bytes, and the rows skip then. - The helper opens libc at the first call. - The client of the file response rows and the spawn fixture pass the type check.
|
Answer to the second review.
One more item is in Downsides now. Main has a use-after-free in |
There was a problem hiding this comment.
The latest push resolves the points I raised earlier (the load-time setsockopt throw on macOS and the spawn rows failing rather than skipping on a low socket-buffer host) and I found no bugs in this version; because the one-line labelling change alters user-visible behavior of Bun.serve file responses and native sinks, a maintainer's look at that tradeoff would still be worthwhile.
What was reviewed:
read_stateand bothread_loopcall sites in src/io/PipeReader.rs: withstop == Nonethe loop already continues reading until a 0-byte read yieldsStop::Eof, so the real end still arrives viadone(); no otherreceived_hup-based labelling remains (read_intoderivesEoffrom the read result).- test/socketpair.ts
limitIsBelow(): no longer throws fromdescribe.skipIf, and the spawn child raises its buffer the same way the in-process probe does, so a clamped host skips instead of failing. - The nested-reader rows in streams.test.js: the outer reader is cancelled only after the
resolvesassertion, so a failing row leaks it into later tests in the file (a nit, not a correctness issue).
Extended reasoning...
The only production change is src/io/PipeReader.rs read_state, which drops the received_hup parameter so a delivery is labelled Eof only on a 0-byte read, a used-up limit, or a used-up byte budget; the other 500 lines are new tests plus a shared bun:ffi socketpair helper registered as a tsconfig path alias. It touches no auth, crypto, or input-parsing surface. The change is small and the tests are byte-exact and skip safely, but it changes observable behavior (file responses of 128-256 KiB under hangup go chunked instead of Content-Length, native sinks now reject on a trailing read error, and it is stacked on another PR), and my earlier inline thread at PipeReader.rs:902 on that tradeoff is still open with only an author reply, so approval is a maintainer's call rather than mine. No CODEOWNERS entry covers the changed files and there are no third-party objections in the timeline.
The child fixture opened its FIFO with a blocking open. A parent that was killed before it read the FIFO, as in a row that timed out, left the child in that open with no end. - The child opens the FIFO without blocking until the parent reads it. It gets the pid of the parent and ends when that process is gone. - A nested-reader row of streams.test.js cancels the outer reader in a finally block. A row that fails does not leave that reader to the tests after it.
|
Answer to the third review.
|
The ASAN lane runs spawn.test.ts with leak checks on. The child fixture
opens libc with bun:ffi, and after a dlopen the exit of a process takes
seconds there, because the leak check symbolizes the leaks that the
suppression file names. The parent waits for that exit.
spawn.test.ts also runs itself once more with the default limit of 5 s
per test ("with BUN_FEATURE_FLAG_FORCE_WAITER_THREAD"). The six rows
took 4.1 to 5.0 s each in that run, and one row hit the limit.
- The child ends by SIGKILL after its report.
- The shell row does not throw for a command that a signal ended.
|
@robobun wake up!! |
|
@robobun wake up!! |
|
I am here. The state of this PR:
Tell me if you want one of these:
|
This branch has not been deployed
2 participants
Problem
Bun.write(file, stream)andHTMLRewriterresolve with 262144 of 270000 bytes that are unread at the hangup. ABun.servefile response sends 262144 with200.read_state(src/io/PipeReader.rs:903) labels every deliveryEof, also one that filled the buffer.Fix
read_statereturnsEofonly for a 0-byte read, a used-up limit or byte budget.Eof, assrc/io/pipes.rs:148documents.on_reader_doneneeds itsfinish().spawn.test.ts,bun-serve-file.test.ts,streams.test.js. 18 fail without the fix. Self-reviewed: 16 concerns raised, 12 addressed, 4 in part.Background
PosixBufferedReaderreads pipes and sockets for streams, subprocess stdio and file responses. It labels each deliveryProgress,DrainedorEof.Bun.write.SOCK_SEQPACKETmessages.Downsides
recv()for 1. ABun.servebody of 131072 to 262144 bytes goes out chunked, 15send()for 9. No hangup: no change.Bun.serveresets where it sent a complete200.Notes
Preconditions. POSIX only. A native sink is attached:
Bun.writeof a stream or aResponse,HTMLRewriter.transform,Bun.spawnstdin, afetchbody, aBun.servefile response. JS readers (getReader(),for await,.text()) end onon_reader_doneand are not changed.PipeReader.rs:784)Bun.file(fd).stream()andBun.stdin.stream(): the reader starts on its poll, so the first wakeup has the bytes and the hangup. Forproc.stdoutand a started file response the bytes and the hangup must come in one wakeup.:805), used while another read loop holds the shared oneReach. A child on a default Linux socket cannot leave more than 262144 bytes unread: the socket holds about 219000. The first row needs
SO_SNDBUFraised,net.core.wmem_defaultof 262144 or more, or an fd that the user passes in. The second row happens with default sockets. Bun gives the stdio sockets of a child 512 KiB on macOS (src/spawn_sys/spawn_process.rs:867), so a plain child can reach the first row there. The defect was not measured on macOS: no macOS host was available.Affected versions (a socket that hung up with 270000 bytes before the sink attached, linux x64):
Bun.write(file, stream)HTMLRewriternew Response(stream).arrayBuffer()[object ReadableStream][object ReadableStream]Node and libuv. Node v26.3.0 (libuv 1.52.1) with the same child: 262145, 270000 and 400000 bytes unread at the hangup all arrive, then
end. libuv 1.52.0 takes EOF from a hangup only when the event has noPOLLIN(uv__stream_io,src/unix/stream.c). libuv 1.51.0 took it only after a partial read.Tests. Every row compares bytes, not lengths. The payload repeats a unit of 251 bytes. No row sleeps. The sockets, their constants and the payload come from one module,
test/socketpair.ts. It does not import the harness, because the child fixtures use it and a debug build takes seconds to load the harness. The rows skip on a host whose limit for a socket buffer cannot hold their bytes. Linux cuts a request abovenet.core.wmem_max. macOS cuts it too, and refuses it withENOBUFSbefore 14.4 (kern.ipc.maxsockbuf). So the helper does not check the result ofsetsockopt(). It asks a probe socket how many bytes it takes (limitIsBelow()), and that probe never throws, because a test file calls it while it loads.streams.test.jsbun-serve-file.test.tsspawn.test.tsBun.writeof the stream, of aResponse,HTMLRewriterfetchbody, shell captureThe child rows block the parent in
readFileSync(fifo)until the child has closed its stdout. The child opens the FIFO without blocking, again and again until the parent reads it, and it ends when the parent is gone. With a blocking open, a row that timed out left its child behind. The child ends by SIGKILL after its report. It opens libc withbun:ffi, and with leak checks on (the ASAN lane) the exit of a process takes seconds after adlopen: the leak check symbolizes the leaks thattest/leaksan.suppnames. The parent waits for that exit. The child closes fd 1 before it reports, because the exit of a process does not close its descriptors in a useful order. The reset rows are Linux only: a unix socket whose peer closes with unread input fails the next read withECONNRESET. There is no row for a static file route. A static route refuses an fd, and a FIFO cannot hold 262144 bytes withoutF_SETPIPE_SZ, which fails in a container withoutCAP_SYS_RESOURCE.Measured (release builds of the base and of this PR, linux x64, LD_PRELOAD loggers and a ptrace tracer.
strace,perfandvalgrindare not installed):recv()per stream, sink path under a hangup, N = 1000 / 200000 / 262144 / 270000 / 400000: base 2 / 1 / 1 / 1 / 1, PR 2 / 2 / 2 / 3 / 3. The pull path makes 2 / 2 / 2 / 3 / 3 on both.recv()per stream, no hangup, N = 1000 / 200000 / 600000: base 5 / 5 / 7, PR 5 / 5 / 7 (10 of 12 runs each at 600000, the others took 9 to 12 on both builds).Bun.write(out, Bun.stdin.stream())): base 1 poll and 1 read, PR 2 and 2. The pull path makes 2 and 2 on both.Bun.servefile response, source hung up with 200000 bytes before the first read: baseContent-Length, 9send(), 200120 wire bytes. PR chunked, 15send(), 200138 wire bytes. Bodies under 131072 bytes: 9send()on both. Each header part is its ownsend()because the file response writes from reader callbacks without a cork. That is so on main too.read_loop: base 2607 bytes, 588 instructions, 68 conditional branches. PR 2614, 591, 68. Release text size: 80856331 bytes on both.Bun.writeresolves 160000 on the base, and rejects withECONNRESETafter it wrote 160000 bytes on the PR.Bun.file(fd).text()rejects on both.FileReader::on_read_chunk, pull path, first hung-up delivery of 262144 bytes: 109 on both, the same sizes in the same order.Read errors, by consumer.
Bun.servefile response: the server answers a failed body with a reset. That is the rule on main for an error behind fewer than 131072 bytes. It now applies behind more bytes too. Measured with 160000 bytes: the base sends a complete message with the whole body. The PR sends a part of the body with no last chunk, then the reset (ECONNRESETat the client). The close drops what the kernel had not sent yet. The server calls noerror()handler and logs nothing:on_file_stream_errorignores its error argument (src/runtime/server/RequestContext.rs:1657).Self-review. 16 concerns, all about the tests and this text. None asked for a change to the source.
HTMLRewriterhandler), a response that starts empty, and the read error forHTMLRewriterandBun.serve. The text names the preconditions, the reach and the versions.Bun.file(fd).stream()and for the file response, not forproc.stdout.Review of the first push. Three optional findings.
test/socketpair.tsnow.queuedon a host with a lownet.core.wmem_max. They skip there now.Bun.serve(Downsides, bullet 1). The proposal was one more read before the delivery when the hangup is known. Measured on a release build with that change: it keepsContent-Lengthand 9send()for hung-up bodies up to about 245760 bytes, with the samerecv()count as this PR. It reads into the rest of a partly filled buffer, and that cuts a message on a message-oriented socket:SOCK_SEQPACKET, hung up, intoBun.writeSo this PR keeps the label change alone. The 6 more
send()come from writes that the file response makes without a cork, one per header part. A cork there helps every pollable body, on main too. It is not in this PR.Review of the second push. One optional finding. On macOS before 14.4 with a lowered
kern.ipc.maxsockbuf,setsockopt()fails withENOBUFS. The helper threw then, and three test files failed while they loaded. Changed in ef4611e, see Tests. Checked on Linux with a library in place of libc whosesetsockopt()always fails: the old helper failsstreams.test.jswhile it loads, the new one skips its 11 rows.Review of the third push. No bug found. One nit: a nested-reader row of
streams.test.jsthat fails left its outer reader to the tests after it. The row cancels that reader in afinallyblock now (0ce4644). The same commit has the change to the child fixture, see Tests. Checked with the rows held to 2 CPUs: 5 of 6 rows timed out, and no fixture process was left. Before the change, rows that timed out left their children behind: 6 of them were on this host.CI of the third push (build 120963).
spawn.test.tswas red on the x64-asan lane, 4 of 4 attempts. Two causes. One was a row of this PR, see Suites. The other is not from this PR: "an idle reader stopped at the highwater mark does not keep the process alive (trickling writer)" fails there because the stderr of its child has a LeakSanitizer warning (ptrace appears to be blocked). The annotations of 37 other builds since 2026-09-25, on branches without this change, name the same test. It was red again in build 120970 (b6ee5db), with the "saturating writer" row of the same test beside it, and it is the only red test there. Of 54 builds that finished after 2026-09-26T06:00Z, 12 have it red and 27 have it green after a retry.Left for a follow-up.
read_loopignoreskeep_going == falseunder a hangup (&& !received_hup,PipeReader.rs:858). That exception made up for the old label: a parent that answeredfalsetoEofleft bytes in the kernel. It stays in this PR. Without it,pause()is honoured under a hangup, which changes more than this bug.Found outside this change.
A heap-use-after-free on main (Downsides, bullet 3). The source of a file response hung up before the response starts. The stream then ends inside
FileResponseStream::startand is freed at its end (src/runtime/server/FileResponseStream.rs:280). The poll of its reader stays registered, because the stream owns the fd and the reader returns its poll only when it owns the fd. The poll fires later, andbegin_readreads the freed reader (src/io/PipeReader.rs:655). One fresh process per run, 30 runs per cell, debug builds with ASAN:One more run of the PR build made no report and did not end in 60 s. Release builds did not crash in this probe. In some runs the process spins in the event loop and the client has no answer after 8 s: 10 of 100 runs on main (367d939), 8 of 100 on the base, 15 of 100 on this PR (two socket cells, the builds interleaved). These counts are too small to tell the builds apart. Bun.serve: release the FIFO file reader when the client disconnects #41874 makes the reader return its poll when it goes away, and
finish()unregisters it. That is from its diff. It was not built for this PR.Bun.servedrops the errno of a failed file body, see above.The comment at
src/uws_sys/libuwsockets.cpp:1182namesFileResponseStream::finishas a caller ofend_without_body. Bun.serve: terminate FIFO/pipe file responses at EOF #37082 removes that call.Suites on the debug build of this PR. At 814a129:
streams.test.js635 pass, and inbun-serve-file.test.tsandspawn.test.tsevery new row passes. At earlier commits of this branch with the same source change:spawn.test.ts,bun-serve-file.test.ts,bunshell.test.ts,html-rewriter.test.js,bun-serve-static.test.ts,spawn-stdin-readable-stream.test.ts,spawn-stdio-syscall-error.test.ts,shell-blocking-pipe.test.ts,process-stdin-stale-hup.test.ts,filesink.test.ts,body-clone.test.ts,terminal.test.ts,filter-workspace.test.tspass.bun run rust:check-all: 12 ok. On the last two commits this host was overloaded (load average above 300). Tests that start child processes hit the local default of 5 s in some runs, on a debug build of main too. So the new rows ran with--timeout 60000on the last commits (0ce4644 forstreams.test.jsandspawn.test.ts, ef4611e forbun-serve-file.test.ts): 26 pass on a release and a debug build of this PR, 18 fail on a release build of the base and on a debug build of main. CI sets--timeout=90000, and 270000 with ASAN (scripts/runner.node.ts:2166). Butspawn.test.tsruns itself once more with the default limit of 5 s ("with BUN_FEATURE_FLAG_FORCE_WAITER_THREAD", Linux only). In that run on the x64-asan lane the six child rows took 4.0 to 4.9 s each in build 120928 and 4.1 to 5.0 s in build 120963, where one row hit the limit. On two release lanes they took 37 to 82 ms. The cause was the exit of the child, see Tests. b6ee5db changes it. In build 120970, with that change, the six rows took 273 to 368 ms each in that run (4 attempts). With the environment of that run on a debug build here, a row took 6.9 to 9.7 s before and 2.2 to 4.3 s after (4 runs each, interleaved, this host overloaded).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/web/streams/streams.test.js, test/js/bun/spawn/spawn.test.ts, test/js/bun/http/bun-serve-file.test.ts