Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. WalkthroughThe PR routes all epoll notifications through the readiness callback and adds platform-specific regression tests for FIFO writes and reset errors from piped stdin. ChangesPipe I/O behavior
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change corrects Linux FIFO and socket error reporting with targeted regression coverage. No merge-blocking risk is currently identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:38 PM PT - Sep 5th, 2026
❌ @robobun, your commit 9a9afac has 2 failures in
🧪 To try this PR locally: bunx bun-pr 37852That installs a local version of the PR into your bun-37852 --bun |
|
Status: fix pushed (latest 5dec5cc), waiting on CI. Linux lanes exercise the epoll half; the darwin test lanes are the ones that exercise the Reproduced how: |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of any of the four; they are the neighbours the description already accounts for:
The fixed behaviour here, EPIPE instead of an errno-0 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes epoll dispatch semantics for every ReadFile/WriteFile on Linux, introduces a new blocking-select(2)-on-the-pool-thread pattern for macOS FIFOs (with the acknowledged pool-thread-pinning trade-off), and the macOS half couldn't be exercised locally, a human look is warranted before merge.
Checked: on_update_epoll now treating EPOLLERR as readiness — traced that ReadFile::on_ready/WriteFile::on_ready both re-issue the syscall and record the real errno, and that __bun_io_pollable_on_io_error remains live via the kqueue EV_ERROR path. The do_write loop restructure correctly re-issues write() on continue (the old code re-matched a stale result). block_until_writable's bitmap sizing (fd/32 + 1 words for nfds = fd+1) and EINTR retry look right; Tag::select slots into NAMES[108] correctly. The st_dev != 0 FIFO-vs-pipe(2) discriminator is gated behind could_block, so regular files skip the extra fstat.
Extended reasoning...
Overview
Four files: src/io/lib.rs (Linux epoll dispatch), src/runtime/webcore/blob/write_file.rs (macOS named-pipe wait path, do_write loop restructure, new is_named_pipe/polls_before_writing), src/sys/lib.rs (new select$DARWIN_EXTSN$NOCANCEL import, block_until_writable, Tag::select), and two new tests in bun-write.test.js.
Security risks
None identified. No user-controlled input reaches new parsing or allocation paths; the select fd-set is sized from an already-open fd, not external data.
Level of scrutiny
High. The Linux hunk is small but changes how every EPOLLERR from the IoRequestLoop is dispatched — it now treats it as readiness so the owner's next read()/write() surfaces the real errno instead of a fabricated errno-0 epoll_ctl error. That is correct reasoning (the old get_errno(event.events) on a bitmask was always wrong), but it affects ReadFile too, not just the WriteFile case under test. The macOS half introduces a new pattern: instead of parking on the IO thread's kqueue, a FIFO write blocks a work-pool thread in select(2) for as long as the reader is slow. The PR description states the trade-off explicitly and justifies it well (kqueue provably cannot deliver this event for FIFO vnodes), but pool-thread occupancy vs. correctness is the kind of architectural call a maintainer should sign off on.
Other factors
- The author had no macOS machine; darwin correctness (including the
select$DARWIN_EXTSN$NOCANCELlink name and thest_dev != 0XNU heuristic for distinguishingmkfifoFIFOs frompipe(2)pipes) rests on the CI lanes. - The change coordinates with three or four other open PRs (#37823, #36025, #35953, #37787); the description calls out which hunks are shared and how they merge, and notes a follow-up needed for #37787's trace assertions on macOS.
- Tests: the EPIPE test polls for the observable condition (write refused with EAGAIN) before closing the reader rather than sleeping, and asserts
{ code, syscall }exactly. The drain test is new coverage for a path (Bun.writewaiting on a FIFO) that previously had none, and on macOS now goes throughselectinstead of kqueue. Both look sound; the drain test'sBun.sleep(1)is inside a bounded poll loop, not a fixed wait. - The
do_writerestructure independently fixes a latent spin bug in the non-pollable EAGAIN branch (the oldcontinuere-matched the sameresultwithout re-issuingwrite()).
Given the cross-platform I/O surface, the untested-locally macOS half, and the pool-thread-blocking design decision, this should not be auto-approved.
|
On the two points the review says rest on CI: both darwin build lanes of this PR's build (93282) have passed, so the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Two pushes since the last round, both from going over the diff again:
Description updated to match. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/runtime/webcore/blob/write_file.rs`:
- Around line 473-477: Update the macOS descriptor classification in the write
operation around `sys::fstat` so its error is not swallowed by the `matches!`
expression. Propagate or record the `fstat` failure and terminate the operation,
or otherwise bias the fallback to the safe named-pipe classification instead of
routing failures through the kqueue path; preserve normal `Ok(stat)`
classification via `bun_sys::is_named_pipe`.
In `@src/sys/lib.rs`:
- Around line 7642-7668: Update block_until_writable so it does not wait
indefinitely on a WorkPool thread: use a bounded select timeout and periodically
re-check or return an appropriate timeout/error outcome, while preserving EINTR
handling and writability detection. If the API cannot represent bounded waiting,
document the justification for retaining the unbounded wait and its safety
relative to the prior io-thread registration.
In `@test/js/bun/util/bun-file-read.test.ts`:
- Around line 97-104: Update the child-process assertion around the stdout split
so it validates or compares the raw output before parsing JSON, preserving the
captured stderr and exitCode diagnostics when the child aborts without printing
“reading\n”. Keep the expected structured payload assertion, but avoid calling
JSON.parse on an undefined outcome; anchor the change to the outcome extraction
and combined expect block.
🪄 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: b6936302-87a2-4331-873d-98dce088def7
📒 Files selected for processing (5)
src/io/lib.rssrc/runtime/webcore/blob/write_file.rssrc/sys/lib.rstest/js/bun/io/bun-write.test.jstest/js/bun/util/bun-file-read.test.ts
|
5dec5cc takes two of the three review points: a failing classification |
There was a problem hiding this comment.
The VM-borrow issue I flagged earlier is addressed: run_with_fd now hops to do_write_loop_task before any named-pipe write, so the blocking select() runs outside Job::run_on_pool's borrow, and the new worker-termination test pins that. This pass found no further bugs. A human look is still worthwhile given the macOS half rests on XNU source reading and the darwin test lanes (no local macOS run), the pool-thread-pinning tradeoff is a design call, and the change coordinates with four sibling PRs (#36025, #37823, #35953, #37791).
Also checked this pass and ruled out:
__bun_io_pollable_on_io_erroris not dead after the epoll caller is removed —on_update_kqueuestill uses it forEV_ERROR(which does carry an errno).- The
selectfd_set arithmetic (word + 1words fornfds = fd + 1) and per-iteration re-init are correct for the$DARWIN_EXTSNvariant. - Moving
sys::writeinside the retry loop fixes the pre-existing!could_blockEAGAIN spin as a side effect; no other caller ofdo_writeexists.
Extended reasoning...
Overview
Five files: on_update_epoll in src/io/lib.rs now dispatches every event (including EPOLLERR) as readiness so the owner's retried read()/write() reports the real errno; src/sys/lib.rs adds macOS-only Tag::select, the select$DARWIN_EXTSN$NOCANCEL import, is_named_pipe(&stat) and block_until_writable(fd); write_file.rs adds an is_named_pipe field, classifies via fstat when could_block, skips poll(2) before writing for named pipes, waits in select(2) on EAGAIN, and schedules the write loop as its own pool task so the wait runs outside the VM borrow. The sys::write call moved inside do_write's retry loop so continue re-issues the syscall. Four new FIFO tests in bun-write.test.js and one Linux-only ReadFile EPOLLERR test in bun-file-read.test.ts.
Security risks
None identified. No user-controlled parsing, no auth/crypto/permissions. The new select call uses the unlimited-fd Darwin variant with a correctly-sized bitmap; the fstat failure path now rejects the write instead of silently misclassifying.
Level of scrutiny
High. This changes the epoll error-dispatch semantics for both ReadFile and WriteFile on Linux, and introduces a new blocking wait strategy on macOS with a stated tradeoff (a slow FIFO reader now pins a pool thread where it previously parked on the io thread for free). The macOS half is derived from XNU source and exercised only by CI darwin lanes — the author has no local macOS. The description is thorough and cites kernel sources, and the earlier VM-borrow deadlock I raised was fixed with a test, but the design decision (select on the pool vs. accepting the kqueue blind spot) and the four-way PR coordination are things a maintainer should sign off on.
Other factors
My earlier inline finding (the inline select running under run_on_pool's VM borrow, blocking VmHandle::close) was addressed in 2152a5f: the loop is now scheduled as a plain pool task before the first write, and a new test terminates a worker mid-wait and requires its exit event. CodeRabbit's two actionable points (propagate fstat failure; compare child output as text) were taken in 5dec5cc; its unbounded-wait note was correctly declined. The comment-cop bot's remaining two threads on write_file.rs were answered (three-line comments retained with justification). I verified __bun_io_pollable_on_io_error remains live via the kqueue EV_ERROR path, so removing the epoll caller does not leave dead code. The do_write loop restructure also fixes a pre-existing spin in the !could_block EAGAIN branch. Given the platform-specific reasoning, the design tradeoff, and the interaction with #36025/#37823/#35953/#37791, this should have a human reviewer.
|
Heads-up on an overlap: #40099 adds the same |
|
A second scenario for the Linux The test is in |
|
@robobun rebase |
|
Rebased onto main (50ca71e). The PR is mergeable and ready for review. The Linux fix ( I dropped the macOS half. It waited in The macOS write side belongs on the io thread, as a write set on the |
59a2b20 to
50ca71e
Compare
robobun
changed the title
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
LGTM. The scope reduction to Linux-only drops the write_file.rs select() loop my earlier comment flagged, and what remains — treating EPOLLERR as readiness so the owner's retried syscall reports the real errno — is the correct epoll semantics.
Checked that __bun_io_pollable_on_io_error is still live (the kqueue EV_ERROR path at lib.rs:1637 uses it with a real errno from event.data, which is a genuine changelist failure, not a readiness flag), and that epoll_ctl registration failures still surface synchronously via FileAction::on_error in tick_epoll, so no error path is lost. The new tests use bounded poll loops (Bun.sleep(1) only inside EAGAIN retry), using for FIFO/tempDir cleanup, and comment why each platform skip exists; the ECONNRESET test drains stdout by iteration and stderr afterward, which is fine since the child writes nothing to stderr.
Extended reasoning...
Overview
Since the previous review, the PR was squashed down to three files: src/io/lib.rs (the epoll EPOLLERR dispatch fix), test/js/bun/io/bun-write.test.js (FIFO drain + EPIPE tests), and test/js/bun/util/bun-file-read.test.ts (stdin ECONNRESET test). All the macOS work described in the PR body — block_until_writable, is_named_pipe, the select$DARWIN_EXTSN import, and the write_file.rs pool-thread wait loop — is gone from the diff, and the EPIPE test is now skipIf(isMacOS) with a comment explaining the kqueue write-filter limitation. The PR description is stale, but the code that merges is what matters.
The native change is a net −9 lines: the EPOLLERR branch that called sys::get_errno(event.events as isize) (always 0, since an events bitmask is never −1) and dispatched a fabricated epoll_ctl error is deleted, and every event now goes through __bun_io_pollable_on_ready. This matches epoll(7): EPOLLERR and EPOLLHUP are readiness signals with no errno attached; the fd's actual condition comes out of the next read()/write(). My earlier inline concern (the macOS select() running under a VM Borrow, blocking worker.terminate()) was on write_file.rs, which is no longer touched — removing that code resolves it.
Security risks
None. This is purely internal event-dispatch plumbing on an already-registered fd; no user input parsing, no auth/crypto, no new attack surface. The change removes a fabricated errno rather than adding any trust in external data.
Level of scrutiny
Low-to-moderate. The remaining native change is tiny and mechanically verifiable against epoll semantics. I confirmed the two things the deletion could have broken: (1) __bun_io_pollable_on_io_error still has a caller — the kqueue path at lib.rs:1632–1646, where EV_ERROR genuinely carries an errno in event.data from a rejected changelist entry, so that branch is correct to keep; (2) epoll_ctl failures themselves are still reported — they go through FileAction::on_error synchronously in tick_epoll, as the new comment states. REVIEW.md's "fix at the layer that owns the invariant" is exactly what this does: the errno belongs to the owner's syscall, not the dispatch layer.
Other factors
The tests follow the repo's conventions closely: tempDir + using for cleanup, bunExe/bunEnv for the spawned child, Bun.sleep(1) only inside bounded EAGAIN-retry loops (never as a bare wait), platform gating via describe.skipIf/it.skipIf with a comment naming the reason for each exclusion, and stdout/stderr asserted before exit code. The unresolved github-actions inline threads at write_file.rs:530–531 from the earlier iteration are on files no longer in the diff, so they're moot. A maintainer commented just before the scope-reduction commit landed, so the Linux-only shape appears intentional. Exit reason was dry_streak.
This branch has not been deployed
2 participants
Problem
Bun.write()into a full FIFO waits for the reader. If the last reader closes, the write must reject with EPIPE. On Linux it rejects withUnknown Error, epoll_ctl(code undefined, errno 0).Poll::on_update_epoll(src/io/lib.rs:1665) turns theEPOLLERRevents bitmask into an errno. The bitmask is never-1, so the errno is always 0.EPOLLERRcarries no errno. A waitingBun.stdin.text()on a socket that gets reset takes the same path.Fix
on_update_epolldelivers every event,EPOLLERRincluded, as "ready". The owner retries its syscall and gets the real errno: EPIPE for the write, ECONNRESET for the stdin read.epoll_ctlis reported synchronously at registration intick_epoll.test/js/bun/io/bun-write.test.js("Bun.write() into a FIFO that is full") andtest/js/bun/util/bun-file-read.test.ts(stdin ECONNRESET). Both new cases fail on the released binary. Also the rest ofbun-write.test.js,bun-file-fd-read.test.tsandbun-stdin-slice.test.ts.Background
mkfifo) is a pipe with a filesystem path. A write to a pipe with no reader fails with EPIPE.Bun.write()to anything that is not a regular file runs its write loop on a pool thread. On EAGAIN it hands the fd to the io thread (epoll on Linux) and resumes when told the fd is writable.EPOLLERRandEPOLLHUPare readiness flags, not error codes. The errno for whatever happened comes from the next syscall on the fd.Notes
This revision is the rebase onto main that was requested. It drops the macOS half of the earlier revision. On macOS the kqueue write filter for a FIFO never fires when the reader closes, so the write never settles there. The earlier revision waited in
select(2)on the pool thread instead, and ran that loop outside the job's VM borrow so a worker's teardown did not wait for the reader.That shape cannot work on main any more. Since #38299 a
Jobholds aTicketfor its whole trip, andVmHandle::close_and_waitwaits, without a bound, for every ticket. The only way to end a parkedBun.write()isJobContext::cancel, which unparks a wait on the io thread throughIoParking. Aselect(2)on a pool thread cannot be cancelled that way, so the earlier revision's own worker-teardown test would hang on the darwin lanes.The right place for the macOS write side is the io thread. #40099 adds a
select(2)watcher thread (src/io/fifo_select.rs) for the read side of named pipes and delivers readiness back into the owner's kqueue, soIoParkingcancel keeps working. The write side is a write set on that same thread. I can do that on top of #40099 once it lands.The EPIPE test is
skipIf(isMacOS)for that reason. The drain test runs everywhere. Thest_devand worker-teardown tests from the earlier revision are gone with the macOS code they covered.Another scenario the same hunk fixes: a pty slave parked in
Bun.file(slave).text()whose master closes. The kernel reportsEPOLLIN|EPOLLHUP|EPOLLERRat once andread()then returns 0 with the buffered bytes. On 1.4.1 the call rejects withUnknown Error, epoll_ctland drops the bytes.Fail-before: both new cases fail on the released binary here. With the debug build, the FIFO block passes 10 of 10 reruns. In the full
bun-write.test.jsrun, "copyFileRange is not available > on large files" exceeded its 5 s budget once under concurrent load (6.3 s) and passed alone at 4.7 s. It is a copy path this change does not touch.Original description of the earlier revision, for the macOS analysis and XNU references: see the PR history before the rebase.
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/bun-file-read.test.ts, test/js/bun/io/bun-write.test.js