Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughA fix for file descriptor leaks in ChangesFileReader Error Handling and FD Leak Prevention
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 2/5 reviews remaining, refill in 25 minutes and 56 seconds. Comment |
|
Updated 3:06 PM PT - May 4th, 2026
❌ @robobun, your commit 73ff63d has 4 failures in
🧪 To try this PR locally: bunx bun-pr 30116That installs a local version of the PR into your bun-30116 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
FileReader.onStart() increments the parent Source refcount and sets
waiting_for_onReaderDone=true so the native source outlives the JS
wrapper while I/O is in flight. The paired decrementCount() lived only
in onReaderDone(). On POSIX, when a read syscall fails (ECONNRESET,
EIO, EBADF, ...), PosixBufferedReader.onError invokes only
vtable.onReaderError and never calls done()/onReaderDone, so the
refcount was never released. JS finalize drops it from 2->1, deinit
never runs, and the dup'd fd + poll + buffered bytes leak forever. The
leaked poll's keep-alive ref also prevented the event loop from
exiting.
Mirror onReaderDone in the error path: after rejecting the pending
pull, clear waiting_for_onReaderDone and call parent().decrementCount()
so the final JS finalize reaches 0 and deinit runs.
Test: Bun.listen server calls socket.terminate() (SO_LINGER{1,0} ->
RST) so the client's recv() on the dup'd fd returns ECONNRESET,
reliably hitting PosixBufferedReader.onError -> FileReader.onReaderError.
50 iterations leaked 50 fds before, 0 after.
isPosix includes FreeBSD, but the fixture's countFds relies on /proc/self/fd which is not mounted by default there. Match harness getFDCount() and skip on anything other than Linux/macOS.
75efa4d to
250a6e5
Compare
There was a problem hiding this comment.
LGTM — the FreeBSD gating nit was addressed and the Windows onStart() sibling path is acknowledged as #29440's scope; the 4-line fix mirrors onReaderDone() exactly under the same flag guard.
Extended reasoning...
Overview
The Zig change adds a 4-line guarded decrementCount() at the tail of FileReader.onReaderError (src/runtime/webcore/FileReader.zig), mirroring the identical block already present in onReaderDone(). The test change adds a Linux/macOS-gated regression test that triggers ECONNRESET via socket.terminate() and asserts fd count doesn't grow across 50 iterations.
Security risks
None. This is a resource-leak fix (releasing a refcount that was previously leaked). The decrement is guarded by waiting_for_onReaderDone, which is cleared before decrementing, so it's idempotent with onReaderDone() and cannot double-free. The decrement is the final statement in the function, so even if it drives the refcount to 0 and frees the parent Source, this is not touched afterward — same ordering as onReaderDone(). decrementCount() (ReadableStream.zig:511) panics in debug on underflow, providing additional safety.
Level of scrutiny
Refcount/lifecycle code in core runtime I/O normally warrants careful review, but this change is mechanical: it copies the exact guard+decrement pattern from onReaderDone() into the sibling error callback. The PR description provides a complete refcount trace, cites precedent in Terminal.onReaderError / FileResponseStream.onReaderError / #30055 (subprocess PipeReader), and includes a regression test that verifies both that the error path is exercised (errored > iterations/2) and that fds don't leak.
Other factors
I left two inline comments on earlier revisions: (1) the isPosix/FreeBSD gating nit — fixed in 75efa4d by gating on isLinux || isMacOS; (2) the related Windows-only onStart() synchronous-.err leak — author confirmed it's dead on POSIX and deferred to #29440 for Windows, which is reasonable scoping. Both threads are resolved. No CODEOWNERS match for this path. No new bugs found in this run.
…h no agent, main 50956 same)
There was a problem hiding this comment.
LGTM — the flag-guarded decrementCount() exactly mirrors onReaderDone, and both earlier review threads are resolved.
Extended reasoning...
Overview
This PR touches two files: a 4-line logic addition to FileReader.onReaderError in src/runtime/webcore/FileReader.zig (clear waiting_for_onReaderDone and call parent().decrementCount(), mirroring the tail of onReaderDone immediately above it), plus a ~100-line POSIX regression test in test/js/bun/util/bun-file-fd-read.test.ts that provokes ECONNRESET via Bun.listen + socket.terminate() and counts fds before/after 50 iterations.
Security risks
None. This is a resource-leak fix on an error path; no auth, parsing, or untrusted-input handling is involved. The test runs in a spawned subprocess with hard-coded localhost endpoints.
Level of scrutiny
Moderate — refcount/lifecycle code in the native runtime can UAF if wrong. I checked the specifics: the new block is identical to onReaderDone's tail, is guarded by the waiting_for_onReaderDone flag (set only in onStart, cleared before decrementing, so no double-decrement even under re-entrancy via pending.run()), and decrementCount() is the last statement in the function so a 1→0 transition that triggers deinit cannot dereference this afterward. decrementCount itself (ReadableStream.zig:511) panics in debug on underflow, which would have caught any miscount during the included test.
Other factors
Both of my earlier inline comments are resolved: the FreeBSD /proc/self/fd nit was fixed by gating the test on isLinux || isMacOS, and the Windows synchronous-.err sibling leak was explicitly flagged as pre-existing/non-blocking and deferred to #29440. No CODEOWNERS rule covers these paths. The PR description includes a verified before/after refcount trace and test output (50 leaked → pass), and the bug-hunting system found nothing on this revision.
|
Closing: this fix targets |
1 participant
What
FileReader.onStart()increments the parentSourcerefcount and setswaiting_for_onReaderDone = trueso the native source outlives its JS wrapper while I/O is in flight. The paireddecrementCount()lived only inonReaderDone().On POSIX, when a read syscall fails (ECONNRESET, EIO, EBADF, …),
PosixBufferedReader.onErrorinvokes onlyvtable.onReaderErrorand never follows up withdone()/onReaderDone.FileReader.onReaderErrorrejected the pending pull but never clearedwaiting_for_onReaderDoneor calleddecrementCount(). After the stream errors and JS drops the wrapper,finalize()drops the refcount from 2→1, it never reaches 0, soFileReader.deinitnever runs — the dup'd fd, itsFilePoll, and any buffered bytes leak forever. The leaked poll's keep-alive ref also prevents the event loop from exiting.Refcount trace:
onStart()→parent().incrementCount()(1→2),waiting_for_onReaderDone = trueonReaderDone()→decrementCount()(2→1) → JSfinalize()→decrementCount()(1→0) →deinitonReaderError()→ (nothing) → JSfinalize()→decrementCount()(2→1) → stays at 1 foreverTerminal.onReaderErrorandFileResponseStream.onReaderErroralready release their reader ref in the error path;FileReaderwas the outlier. Same class of bug as #30055 (subprocessPipeReader).Fix
Mirror
onReaderDonein the error path: after rejecting the pending pull, clearwaiting_for_onReaderDoneand callparent().decrementCount().Repro / test
Bun.listenserver callssocket.terminate()on the accepted connection (closes withSO_LINGER{1,0}→ RST), so the client's nextrecv()on the fd dup'd byBun.file(fd).stream().getReader()returnsECONNRESET, reliably hittingPosixBufferedReader.onError→FileReader.onReaderError.(
net.Socket.resetAndDestroy()was tried first but does not actually send RST in Bun — it uses.fast_shutdown, which surfaces as clean EOF on the peer.)Verification
The
FileReader.onReaderErrorchange here is the same as item 6 in #29440 (which bundles it with several WindowsBufferedReaderfixes and theclose_jsvalueStrong-cycle change). This PR adds a POSIX regression test for the leak; #29440 has no test/ coverage for this path. Whichever lands first, the other reduces to a trivial merge.