Windows: release FileSink keep-alive ref for Bun.file(fd).writer() on a borrowed fd - #36107
Windows: release FileSink keep-alive ref for Bun.file(fd).writer() on a borrowed fd#36107robobun wants to merge 7 commits into
Conversation
… a borrowed fd On Windows the two FileSink-creation blocks in Blob.rs (get_writer and pipe_readable_stream_to_blob) set owns_fd = false for a caller-supplied fd and called start(fd, true), which opens a uv_pipe_t/uv_tty_t that adopts the underlying HANDLE. The first write returns Pending (uv writes are async) and FileSink takes a must_be_kept_alive_until_eof self-ref. On end(), WindowsStreamingWriter::end() early-returned for !owns_fd, so close() never ran, on_close never fired, and the self-ref was never released: one native FileSink (plus its Box<uv::Pipe>) leaked per writer for the process lifetime. For pipe/tty fds, dup the borrowed fd before start() so the writer owns its own handle; end() then runs close(), on_close fires, the keep-alive ref is released, and the caller's fd stays open. For regular-file fds, let end() call close() even with !owns_fd: the File close path already takes detach_borrowed_fd() and leaves the fd open, it just never reached there. On a start() error the dup (or a path-opened fd) is now closed; before, a path-opened fd leaked on that error path.
WalkthroughChangesWindows borrowed fd ownership
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:30 AM PT - Jul 27th, 2026
❌ @robobun, your commit 28bc03d has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36107That installs a local version of the PR into your bun-36107 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I also checked that the end() change is safe for borrowed file fds — BaseWindowsPipeWriter::close() routes Source::File/SyncFile through detach_borrowed_fd() when !owns_fd (PipeWriter.rs:1149), so the caller's fd is not closed; the Pipe/Tty arms there do unconditionally uv_close, which is why the dup is needed. FileSink::init is still handed the original fd (not the dup), so the JS-visible .fd keeps reporting the caller's descriptor.
Extended reasoning...
Confirmed the PR's stated invariant that the File close path already honours !owns_fd: close() at PipeWriter.rs:1144-1154 branches on owns_fd() and calls detach_borrowed_fd() for the borrowed case, so letting File/SyncFile through the new end() guard reaches on_close_source() (releasing the keep-alive ref) without closing the caller's fd. The Pipe/Tty arms at 1157-1172 have no such check and hand the handle straight to uv_close, which matches the PR's rationale for duping borrowed pipe/tty fds. Also traced FileSink::init(fd, ...) — it stores the original fd in self.fd while start(writer_fd, ...) gets the dup, so get_fd() still returns what the user passed. Deferring to a human on the broader Windows handle-lifecycle reasoning and the stated overlap with #35993.
There was a problem hiding this comment.
No issues found, but this touches Windows fd/HANDLE ownership and libuv lifecycle in WindowsStreamingWriter::end() and both Blob sink-creation paths, and overlaps with #35993 — worth a human look at the ownership transitions.
Checked: the end() relaxation only reaches WindowsStreamingWriter (shell IOWriter uses WindowsBufferedWriter, unaffected); close() for borrowed File/SyncFile routes through detach_borrowed_fd() so the caller's fd is not closed; start_sync (stdout/stderr) produces SyncFile, so it's covered by the new match arm rather than the pipe early-return. FileSink::init(fd, ...) still receiving the original (not dup'd) fd is fine — it's only read back for the fd-number getter. Both earlier nits (temp-file cleanup, cp.on('error', reject)) are addressed.
Extended reasoning...
Overview
Windows-only leak fix: Bun.file(fd).writer() on a borrowed pipe/tty/file fd never fired on_close, so the must_be_kept_alive_until_eof self-ref taken on the first pending write was never released and each writer leaked a native FileSink for the process lifetime. The fix is two-pronged: (1) a new dup_borrowed_pipe_for_uv() helper DuplicateHandles a borrowed pipe/tty fd so the writer owns the handle libuv adopts and can safely uv_close it, and (2) WindowsStreamingWriter::end() is relaxed to let close() run for borrowed File/SyncFile sources, where close() already honours !owns_fd via detach_borrowed_fd(). Both Windows sink-creation sites in Blob.rs (get_writer and pipe_readable_stream_to_blob) now use the helper and additionally close the fd if start() itself fails. New Windows-only subprocess tests assert liveCount() doesn't grow and the caller's fd stays open.
Security risks
None. No untrusted input parsing; the change is fd-ownership bookkeeping on Windows.
Level of scrutiny
High. This is native memory-safety-adjacent code — fd/HANDLE ownership, libuv handle lifecycle, and refcount release ordering on Windows, which REVIEW.md calls out as the most-blocked category. The end() change alters behaviour for every WindowsStreamingWriter with !owns_fd and a File/SyncFile source; I verified the only setter of owns_fd = false on this type is the two Blob sites updated here (shell IOWriter uses WindowsBufferedWriter, whose end() is untouched), and that the stdout/stderr start_sync path lands in SyncFile and so is covered. But a maintainer familiar with the Windows pipe lifecycle should confirm the dup-then-uv_pipe_open pattern doesn't interact badly with any existing consumer, and decide sequencing with #35993 which touches the same get_writer hunk.
Other factors
The PR has good test coverage (both pipe and file variants, asserting both no-leak and fd-stays-open), and the author reports the relevant existing suites pass on Windows x64 debug plus rust:check-all on all targets. Both prior review nits (temp-file cleanup in the child, wiring cp.on('error', reject)) have been addressed in follow-up commits. The explicit overlap with in-flight #35993 is another reason for a human to sequence the merges.
There was a problem hiding this comment.
Both prior nits are addressed — nothing further from the bug-hunting pass. Deferring to a human familiar with the Windows libuv path: this changes fd-ownership semantics in WindowsStreamingWriter::end() and adds a HANDLE dup ahead of uv_pipe_open, which is squarely in the native memory-safety / lifetime category and only verifiable on Windows CI. Also flagging the stated overlap with #35993 on the get_writer hunk so the two land in a coordinated order.
What was reviewed:
end()now callsclose()for borrowedSource::File/SyncFile; confirmedBaseWindowsPipeWriter::close()routes that case throughdetach_borrowed_fd()and does not close the caller's fd.dup_borrowed_pipe_for_uvusesbun_sys::dup+make_lib_uv_owned_for_syscall(.., CloseOnFail), so a failed uv-wrap doesn't leak the dup.FileSink::initstill receives the originalfd(JS-visible viaget_fd()), only the writer'sstart()gets the dup — checked this split is intentional.- New
writer_fd.close()onstart()failure is gated onowns_fd, so a caller-borrowed non-pipe fd is not closed.
Extended reasoning...
Overview
The PR fixes a Windows-only native FileSink leak when Bun.file(fd).writer() is created on a borrowed (caller-supplied) fd. Three files: a one-line semantic change to WindowsStreamingWriter::end() in src/io/PipeWriter.rs letting close() run for borrowed File/SyncFile sources; a new dup_borrowed_pipe_for_uv() helper in src/runtime/webcore/Blob.rs applied at both Windows sink-creation sites (get_writer and pipe_readable_stream_to_blob) so borrowed pipe/tty handles are DuplicateHandle'd before uv_pipe_open adopts them; and a new Windows-only leak-regression test in filesink.test.ts covering both pipe and file borrowed fds.
Security risks
None identified. No untrusted-input parsing, no auth/crypto surface. The change is confined to internal fd-ownership bookkeeping and libuv handle lifecycle on Windows.
Level of scrutiny
High — this is exactly the class REVIEW.md flags as the most-blocked category (native memory safety, ref-count balance, fd ownership). The fix reasons about who owns the HANDLE that uv_pipe_open adopts, when on_close fires vs. early-returns, and whether close() for each Source variant honours !owns_fd. I traced BaseWindowsPipeWriter::close() and confirmed the File/SyncFile arm branches on owns_fd() into detach_borrowed_fd(), and that the Pipe/Tty arm would uv_close the handle unconditionally (justifying both the early-return remaining for that case and the dup making it unreachable from Blob). The dup helper composes bun_sys::dup with make_lib_uv_owned_for_syscall(CloseOnFail), matching existing usage in src/sys/tmp.rs. The new close-on-start()-failure paths are correctly gated on owns_fd. Nothing looked wrong, but this is subtle enough — and Windows-only, so the author could not run the tests locally — that a maintainer who knows this layer should sign off.
Other factors
Both nits I raised earlier (temp-file hermeticity, wiring cp.on('error', reject)) were addressed in 97d0227 and bf0facf. The author explicitly calls out overlap with #35993 on the identical get_writer hunk and the end() condition; a human should decide the merge order. The test asserts both the leak fix (liveCount() doesn't grow) and the negative contract (fstatSync(fd) still succeeds → caller's fd stays open), and follows the existing bounded-poll pattern in the same file. CI build #83426 for the latest commit is still in progress at the time of this review.
|
CI status: all Windows lanes (x64 and aarch64, build + test) passed on both builds #83414 and #83426, which is where this diff actually runs. The remaining red is unrelated to this Windows-only change:
Ready for review. |
1 participant
Reproduction
Cause
Both Windows sink-creation blocks in
Blob.rs(get_writerandpipe_readable_stream_to_blob) setwriter.owns_fd = falsefor a caller-supplied fd and then calledwriter.start(fd, true), which callsSource::openand for a named-pipe/tty fd creates auv_pipe_t/uv_tty_tthat adopts the underlying HANDLE. Everyuv_writeis async so the first write returnsPendingandFileSink::to_resulttakes themust_be_kept_alive_until_eofself-ref. Onend():close()never runs,Parent::on_closenever fires, andclear_keep_alive_refis never called: one nativeFileSink(and itsBox<uv::Pipe>) leaks for the process lifetime per writer. A borrowed regular-file fd leaks the same way via theSource::Filepath.Fix
dup_borrowed_pipe_for_uv()helper: for a borrowed pipe/tty fd,DuplicateHandle+ wrap as a uv fd, and pass that tostart()withowns_fd = true.end()then runsclose(),on_closefires, the keep-alive ref is released, anduv_closeonly closes the dup so the caller's fd stays open. Bothget_writerandpipe_readable_stream_to_blobcall the helper; a path-opened fd (or the dup) is now also closed ifstart()itself fails, which it was not before.WindowsStreamingWriter::end()now letsclose()run for a borrowedSource::File/SyncFile: the File close path already routes throughdetach_borrowed_fd()and leaves the fd open, it just never reached there. Pipe/Tty with!owns_fdstill early-return, but the dup above makes that state unreachable from the Blob paths.Verification
New Windows-only tests in
test/js/bun/util/filesink.test.tsspawn a child withstdio[3] = 'pipe'and a temp-file fd, run 8Bun.file(fd).writer()/write/enditerations, and assertfileSinkInternals.liveCount()did not grow andfstatSync(fd)still succeeds.Received: 8Received: 8filesink.test.ts,bun-write.test.js,spawn-stdin-readable-stream.test.ts,child-process-stdio.test.js,process-stdio.test.tsall pass on Windows x64 debug.bun run rust:check-allis clean on all 10 targets.Related
#35993 applies the same dup to
get_writeras part of backingcreateWriteStreamwith a FileSink; this PR coverspipe_readable_stream_to_blobas well (reachable today via S3Bun.writeto a pipe fd, and via plainBun.write(Bun.file(pipeFd), readableStream)once #31689 lands). Overlap with #35993 is the identicalget_writerhunk and theend()condition.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/filesink.test.ts