Bun.serve: stop FIFO file responses from keeping the process alive - #37083
Bun.serve: stop FIFO file responses from keeping the process alive#37083robobun wants to merge 6 commits into
Conversation
FileResponseStream held its in-flight read ref until on_reader_done or on_reader_error fired, but an abort while the read is parked on a poll that will never fire (a FIFO with no writer) never reaches those, so the stream leaked and the armed FilePoll kept the event loop referenced forever. Release the ref in finish(). The stream also clears the reader's CLOSE_HANDLE flag because auto_close owns the fd, which makes the posix reader's own teardown skip the FilePoll; its event-loop active ref then leaked even when the response completed at EOF. Unregister and free the poll in Drop without closing the fd, the same way the shell IOReader does.
|
Status: reproduced on Linux with the FIFO repro in the description (process hangs after |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 16 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 (3)
WalkthroughChangesFile reader cleanup now preserves buffers used by pending operations. File response teardown releases pending reader references and Unix poll handles. Shell I/O cleanup uses shared poll-release logic. FIFO subprocess tests cover parked-read abort and EOF cleanup. FIFO response cleanup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/http/bun-serve-file.test.ts`:
- Around line 1172-1173: The two independent subprocess tests currently run
sequentially; make both concurrent. In test/js/bun/http/bun-serve-file.test.ts
at lines 1172-1173, update the abort test to use
test.concurrent.skipIf(isWindows), and at line 1231 update the EOF test the same
way.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 753dc422-fd4e-41b9-8e94-e4023c64be4d
📒 Files selected for processing (2)
src/runtime/server/FileResponseStream.rstest/js/bun/http/bun-serve-file.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/http/bun-serve-file.test.ts`:
- Line 1289: Remove the explicit 15_000 per-test timeout from the
hang-regression test, leaving the test to use the existing test runner timeout.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9333870b-2ae3-4518-87e7-82f6a7379e62
📒 Files selected for processing (1)
test/js/bun/http/bun-serve-file.test.ts
CI hit a heap corruption segfault on Windows (build 89813, serve.test.ts): freeing a FileResponseStream while its uv_fs_read was still running on the threadpool let the op write into the freed reader buffer, since uv_cancel cannot stop an op that already started. close_impl now moves the buffer into the detached File box, which the fs callback reclaims after the op. Also from review: trim two comments, run the new FIFO tests concurrently without per-test timeouts, and restrict the EOF test to Linux, where closing the last FIFO writer reliably wakes the armed read poll (macOS kqueue does not deliver that event; the EOF there arrives via idleTimeout).
… poll-release idiom The abort fixture closed its FIFO writer before exiting, which delivered an EOF that released the parked read through the reader-done path; the test passed even without the finish() release. Keep the writer open so the test fails if either half of the teardown is lost. Add the same abort-while-parked test through a static route: FileRoute is the other FileResponseStream entry point (and the only on_abort: None caller), and it had the identical hang. Replace the five hand-rolled matches!-plus-close_impl(None, None, false) sites with PollOrFd::release_poll_keep_fd(), so the FilePoll teardown for CLOSE_HANDLE-cleared owners lives in bun_io next to the invariant it maintains.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/io/pipes.rs`:
- Around line 131-140: Gate the public method PollOrFd::release_poll_keep_fd
with a non-Windows configuration so it is unavailable on Windows, where
close_impl cannot preserve the file descriptor. Keep its existing Poll-state
check and teardown behavior unchanged on supported targets.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2c2db33-a33c-45b0-9e94-1d7ea162deaf
📒 Files selected for processing (5)
src/io/pipes.rssrc/runtime/server/FileResponseStream.rssrc/runtime/shell/IOReader.rssrc/runtime/shell/IOWriter.rstest/js/bun/http/bun-serve-file.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ight FileResponseStream::Drop queued Closer::close (an async uv_fs_close on the libuv threadpool) before the reader detach could even attempt uv_cancel, so with a read already running on a worker the fd could be closed, and its CRT slot recycled, out from under the in-flight ReadFile. This is the fd half of the hazard whose buffer half 3e82ce6 fixed. When an op is pending, Drop now sets close_after_operation on the File source instead of closing the fd itself; complete() already honors that flag after the op's callback, and on_close_complete reclaims the box.
There was a problem hiding this comment.
The two issues I raised earlier are addressed — 305f243 defers the Windows fd close until the in-flight uv_fs_read completes (close_fd_after_pending_op → complete() honors close_after_operation → on_close_complete reclaims the box + orphaned_buffer), and the abort fixtures now deliberately never close writerFd so the test pins the finish() clause on its own. I have nothing further; deferring to a human for the refcount / cross-thread fd-lifetime changes.
What was reviewed:
- Traced the Windows abort Drop ordering:
close_fd_after_pending_opruns before the field-drop'sdetach_borrowed_fd, both on the loop thread, soclose_after_operationis set before the box is handed off andon_file_read's null-parent branch takes thestate == Closingarm. release_poll_keep_fdat all five call sites is a mechanical extraction of the pre-existingif Poll { close_impl(None, None, false) }idiom; the#[cfg(not(windows))]gate keeps the contract honest.deinit()reordering:_buffer = Vec::new()now runs afterclose_impl, so the in-flight-op branch can move the live allocation intoorphaned_bufferfirst.
Extended reasoning...
Overview
Fixes two process-keepalive leaks in FileResponseStream when serving a FIFO body: (1) finish() now releases the in-flight read ref so an aborted stream whose read is parked on a poll that will never fire can reach Drop, and (2) Drop explicitly frees the FilePoll on Unix (start() cleared CLOSE_HANDLE, so the reader's own teardown skips it). The pre-existing hand-rolled poll-release in shell IOReader/IOWriter is factored into PollOrFd::release_poll_keep_fd(). On Windows, making Drop reachable mid-uv_fs_read opened a buffer-UAF and an fd-close race; WindowsBufferedReader::close_impl now moves _buffer into the detached File box, and FileResponseStream::Drop defers the fd close to the box via close_fd_after_pending_op() when an op is in flight. Three new subprocess fixture tests cover abort (fetch handler + static route) and clean-EOF exit.
Security risks
None. No parsing of untrusted input, no auth/crypto, no path handling. The changes are lifetime/refcount bookkeeping on already-open server-owned fds.
Level of scrutiny
High — this is squarely in REVIEW.md's "memory safety (the most-blocked category)": intrusive-refcount balance across every terminal path, a new reachable Drop while a threadpool op is running, and fd ownership hand-off between the parent and a detached libuv box. The Windows path in particular is a real cross-thread hazard (uv_cancel cannot stop a running op; Closer::close is an async uv_fs_close on the same pool). A human maintainer should sign off on the ownership story.
Other factors
I raised two findings on an earlier revision (the Windows fd-close race and the abort fixture's closeSync(writerFd) masking the finish() clause); both are resolved in the current diff and the threads are marked resolved. The release_poll_keep_fd extraction in IOReader/IOWriter is byte-for-byte equivalent to what was there. Tests follow harness conventions (tempDir, await using proc, concurrent pipe-drain, test.concurrent, no per-test timeouts, hang-guard asserts exitCode === 0). No outstanding human review comments.
1 participant
What
A
Response(Bun.file(fifo))body whose read is parked on a poll that will never fire keeps the bun process alive forever. After the client aborts andserver.stop(true)runs, the process never exits:While reproducing this I found the success path leaks too: a FIFO response that completes normally at EOF also leaves the process unable to exit (body delivered,
server.stop(true)returns, event loop never goes idle).Cause
Two leaks in
FileResponseStream, one per scenario:start()takes a ref for the in-flight read (hold_read_ref), released only bytake_read_ref()inon_reader_done/on_reader_error/ the backpressure arm ofon_read_chunk. When the client aborts while the read is parked on a poll that will never fire (FIFO with no writer), none of those ever run, so the stream's refcount never reaches zero:Dropnever runs, the fd leaks, and the registeredFilePollkeeps the event loop referenced for the life of the process.start()clears the reader'sCLOSE_HANDLEflag becauseauto_closeowns the fd, and the posixPosixBufferedReaderteardown (close_without_reporting, reached from itsDrop) skips the handle entirely in that mode. The armedFilePoll, which holds the event loop's active ref, is never unregistered or freed even when the refcount does reach zero, which is why the clean-EOF case hangs as well.Fix
finish()now releases the in-flight read ref if it is still held: once the stream is finished, no reader callback is coming to adopt it. This is a no-op on the reader-callback paths, which already took the ref, and every entry point intofinish()holds its own guard ref, so the free still lands on that guard's drop.Dropnow unregisters and frees theFilePollexplicitly without closing the fd (whichauto_closestill owns). Unix only; on Windows the reader's ownDropalready hands its libuv source back to the loop. This unregister-without-closing-the-fd teardown already existed hand-rolled in the shellIOReader/IOWriter(the otherCLOSE_HANDLE-cleared owners), so it is now a namedPollOrFd::release_poll_keep_fd()inbun_ioand all five sites use it.Windows follow-up
The first CI round (build 89813) segfaulted in serve.test.ts on Windows x64 with heap corruption:
Releasing the read ref means an aborted stream can now actually be freed while its
uv_fs_readis still running on the libuv threadpool (before this PR it just leaked, so the window was unreachable).uv_cancelcannot stop an op that has already started, and the op'siovpoints into the reader's_buffer, so the read completed into freed memory and corrupted the allocator freelist; the next same-size allocation (another response's reader buffer) crashed.WindowsBufferedReader::close_implnow moves_bufferinto the detachedFilebox when an op is in flight, and the fs callback reclaims both together after the op completes.Verification
Three new tests in
test/js/bun/http/bun-serve-file.test.ts, each spawning a fixture process that must exit on its own:fetch()handler (posix): client reads the first body chunk (proving the server's read is parked on its poll), aborts,server.stop(true), process must exit 0. The fixture deliberately never closes its FIFO writer: an explicit close would deliver an EOF that releases the parked read through the reader-done path and mask a missing abort-time release, so the test isolates both halves of the fix.routes:, theFileRouteentry point, which had the identical hang.server.stop(true), process must exit 0. Restricted to Linux because macOS kqueue does not reliably wake an armed FIFO read filter when the last writer closes (the workaround note inio/pipes.rsdocuments this), so the late EOF there arrives viaidleTimeoutinstead of the poll.All three hang (test timeout, dangling process killed) on bun 1.4.0 without the fix and pass with it. Full
bun-serve-file.test.ts,bun-serve-static.test.ts,serve-file-slice-read-error.test.ts, andtls-bunfile-leak.test.tssuites pass on Linux, and the shellfile-io.test.ts/shell-blocking-pipe.test.tssuites pass with therelease_poll_keep_fdmigration; on Windows,serve.test.tsandbun-serve-file.test.tspass and a 1000-iteration abort-mid-file-response stress runs clean with the buffer hand-off fix.serve.test.tson Linux has 4 pre-existing environment failures (IPv6, privileged ports) that fail identically on the released bun.Related but distinct: #37082 fixes the wire framing of the late-EOF completion in this same function (
end_without_bodyleaving a chunked body unterminated). This PR does not touch framing; the new EOF test tolerates either framing so the two do not depend on each other.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/bun/http/bun-serve-file.test.ts