Skip to content

Bun.file(fifo).bytes(): wait for a named pipe's EOF with select(2) on macOS - #37823

Open
robobun wants to merge 7 commits into
mainfrom
farm/855d1d4b/macos-fifo-read-eof
Open

Bun.file(fifo).bytes(): wait for a named pipe's EOF with select(2) on macOS#37823
robobun wants to merge 7 commits into
mainfrom
farm/855d1d4b/macos-fifo-read-eof

Conversation

@robobun

@robobun robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • On macOS, Bun.file(fifo).bytes() / .text() / .arrayBuffer() / .json() on a named pipe, and Bun.stdin.text() when stdin is a named pipe, never resolve. Every byte the writer sends arrives, then the read sits forever after the writer closes; Linux resolves at the close.
  • This is why the FIFO test in io: hand ReadFile/WriteFile to the io thread through the owner's pointer, not &mut self.io_request #37787 ran to its 90 s timeout on the darwin lanes and is skipped there. main behaves the same, so io: hand ReadFile/WriteFile to the io thread through the owner's pointer, not &mut self.io_request #37787 did not cause it.
  • Cause: both ways the reader waits on a pipe (a poll(2) pre-check, then a kqueue EVFILT_READ wait on the io thread) wake for a FIFO only while bytes are buffered, and XNU reports the last writer closing through neither. Once a read drains the pipe the EOF wake-up never comes; only reads whose writer was gone before the first read() finish today.
  • pipe(2) pipes are unaffected: XNU gives them their own filter that reports EV_EOF, which is why stdin from an ordinary pipe already works on macOS.

Fix

  • macOS only: a whole-file read of a named pipe (fstat says S_IFIFO with a non-zero st_dev; pipe(2) pipes report st_dev == 0) no longer polls or parks on the io thread. It runs as its own pool task that blocks in select(2) before the first read and again each time a read drains the pipe, then continues the existing read loop; everything else, on every platform, takes the old path unchanged.
  • This is correct because XNU's select on a FIFO consults the FIFO's socket, which does flag the last writer's close, and read() then returns 0. Waiting before the first read also makes a FIFO with no writer yet wait for one, as Linux does today, instead of reading as empty.
  • The wait is a separate pool task rather than inline in the job because the job runs under a borrow of the VM that worker teardown waits for; inline, a worker with a FIFO read pending against an idle writer could not be terminate()d.
  • Verification: six new tests (payload delivered in pieces, writer connecting late, writer closing without writing, FIFO as stdin, fd above 1024, worker terminate during the wait). They pass on Linux with and without the change; on macOS the first five hang on main, so this PR's darwin CI lanes are the fail-to-pass evidence. The author had no macOS machine and did not run them there before CI.

Background

  • A named pipe (FIFO) is a filesystem entry made with mkfifo; a reader's read() returns 0 only once nothing is buffered and every writer has closed. A pipe(2) pipe is the anonymous kind behind cmd | bun and subprocess stdio. Both fstat as S_IFIFO; on XNU only the named one has a non-zero st_dev.
  • On macOS, poll(2) is implemented on top of kqueue, so poll and kqueue see the same events for an fd, and for a FIFO that event is only "bytes are buffered". select(2) goes through the FIFO's underlying socket, which also reports "no more writers". Linux reports FIFO EOF through epoll, so it never had this bug. Go hit the same kernel behaviour (runtime: closing Darwin FIFO writer does not trigger kqueue event for reader golang/go#24164).
  • ReadFile is the off-thread half of a whole-file read such as Bun.file(x).bytes(). For anything that is not a regular file it polls, and when not ready registers the fd with the io thread's kqueue and finishes when woken; this diff adds a third path beside those two.
  • A pool task is a job on bun's blocking work pool, where fs.readFile already does its blocking reads on every platform. Unlike JobContext::run, a pool task holds no VM borrow, so a thread sleeping in one does not block a worker's teardown.
  • select$DARWIN_EXTSN is the _DARWIN_UNLIMITED_SELECT flavour of select: the read set is a bitmap of fd / 32 + 1 words with no FD_SETSIZE (1024) cap. The plain select rejects nfds > FD_SETSIZE with EINVAL, which is what the fd-above-1024 test pins.

no test proof · iteration 3 · 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

Original description

What

On macOS, Bun.file(fifo).bytes() / .text() / .arrayBuffer() / .json() on a named pipe, and Bun.stdin.text() when stdin is a named pipe, never resolve once the reader has to wait for anything after receiving data. The read delivers every byte the writer sends and then sits forever after the writer closes.

// mkfifo /tmp/p; then, in another process: cat some-file > /tmp/p
const bytes = await Bun.file("/tmp/p").bytes(); // macOS: never resolves; Linux: resolves at the writer's close

This is how the FIFO test added in #37787 behaved on its darwin lanes (build 93007: the child stayed alive until the 90 s timeout, while every Linux lane passed), which is why that PR skips it on macOS. Nothing in #37787 caused it; the same thing happens on main.

Cause

Both ways ReadFile (src/runtime/webcore/blob/read_file.rs) finds out whether a pipe is readable are blind to a named pipe's EOF on macOS:

  • The wait itself is a kqueue EVFILT_READ registration on the io thread. XNU attaches that filter for a FIFO to the vnode (vn_kqfilter), and filt_vnode_common activates it only when fifo_charcount() is non-zero, i.e. while bytes are buffered; fifo_close_internal only calls socantrcvmore() on the FIFO's socket and posts nothing to the vnode. So a reader parked in kqueue is never woken up by the last writer closing.
  • The bun_core::is_readable pre-check is poll(2), which XNU implements by registering the same kqueue filter (poll_nocancel in sys_generic.c), so it reports not-ready for a drained FIFO whether or not the writers are gone.

Together they make the hang deterministic rather than a race: after any successful read the loop polls (not ready), hands off to kqueue, and the EOF event never comes. The only reads that complete today are the ones where read() itself returns 0 because the writer was already gone before the first read. pipe(2) pipes are not affected: they have their own filter (filt_piperead_common sets EV_EOF), which is why the Bun.stdin-from-a-pipe tests pass on macOS. Go ran into the same kernel behaviour and stopped using kqueue for FIFOs on darwin (golang/go#24164).

What does work is select(2): fifo_select goes to soo_select on the FIFO's read socket, and soreadable() includes SS_CANTRCVMORE, which is exactly what the last writer's close sets (and socantrcvmore does the select wakeup). fifo_read then returns 0 when there are no writers and nothing buffered.

XNU references (apple-oss-distributions/xnu, current main)
  • bsd/vfs/vfs_vnops.c: vn_kqfilter (FIFO knotes attach to v_knotes), vnode_readable_data_count (fifo_charcount), filt_vnode_common (activate = (data != 0) for EVFILT_READ; EV_EOF only on NOTE_REVOKE), vn_select -> VNOP_SELECT.
  • bsd/miscfs/fifofs/fifo_vnops.c: fifo_write/fifo_read post to the vnode (lock_vnode_and_post), fifo_close_internal does not; fifo_select -> soo_select; fifo_read returns 0 with fi_writers < 1 and an empty buffer.
  • bsd/kern/sys_socket.c soo_select, bsd/kern/uipc_socket2.c soreadable / socantrcvmore.
  • bsd/kern/sys_generic.c poll_nocancel: poll is kevent_register(EVFILT_READ | EV_POLL) per fd.
  • bsd/kern/sys_pipe.c: pipeclose sets PIPE_EOF and KNOTEs the peer; pipe_stat leaves st_dev 0 ("Left as 0: st_dev, ...").
  • libsyscall/wrappers/select-base.c and libsyscall/Platforms/MacOSX/{arm64,x86_64}/syscall.map: select$DARWIN_EXTSN$NOCANCEL exists on both architectures and maps straight to __select_nocancel; the non-EXTSN variants reject nfds > FD_SETSIZE with EINVAL.

Fix

  • bun_sys::block_until_readable(fd) (macOS only): blocks in select(2) on one fd, EINTR-retried. It links select$DARWIN_EXTSN$NOCANCEL next to the other $NOCANCEL imports, so there is no FD_SETSIZE limit (the read set is a bitmap of fd / 32 + 1 words; both darwin build lanes link it), and gets a Tag::select appended to the syscall tag table.
  • ReadFile records is_named_pipe from the fstat it already does: S_IFIFO with a non-zero st_dev. pipe(2) pipes report st_dev == 0 (see pipe_stat above) and keep the existing io-thread path, so cmd | bun stdin and subprocess pipes are untouched.
  • For a named pipe, run_async_with_fd does not poll or hand the request to the io thread; it schedules read_named_pipe_task on the pool and returns. That task waits in select, runs the existing read loop, and whenever a read drains the pipe waits in select again and carries on (the poll(2) pre-check is skipped there, since it answers nothing select does not); a failed select is reported through the usual errno / system_error path, and on_finish closes the fd and posts the completion exactly as after an io-thread wake-up today. Scheduling a task rather than waiting inline matters because JobContext::run executes under a VM borrow (VmHandle::borrow: "jobs that could block indefinitely on an external party must own their memory instead") and a worker's teardown waits for outstanding borrows, so a wait inside run would have made a worker with a Bun.file(fifo).text() pending against an idle or absent writer impossible to terminate(). The rescheduled task holds no borrow, and ReadFile is the job's owned off-thread part, so a read that finishes after its VM is gone is released the same way a late io-thread wake-up is.
  • Waiting before the first read as well is what makes a FIFO whose writer has not connected yet wait for that writer instead of reading as empty, which is what Linux does today (poll on a never-written FIFO is not ready there, and read() would return 0 on both systems).

Why this shape: kqueue cannot be made to deliver this event, so some thread has to sit in a call that the kernel does wake up for. Waiting on the pool thread is what fs.readFile(fifo) already does on every platform (a blocking read() on the pool), it is confined to FIFO vnodes on macOS, and every read it slows down is one that never completed before. Anonymous pipes, sockets, character devices and every other platform still go through the io thread exactly as before. If pinning a pool thread per in-flight FIFO read ever matters, the next step would be a select-based wait on the io thread; that is a much larger change to src/io for a path that is currently non-functional.

Composition with #37787 (open; restructures the same two functions and adds a macOS-skipped copy of the first test below): the two conflict textually and whichever lands second rebases. If that is this PR, the named-pipe branch goes into its prepare_read / read_until_blocked exits and its skipped FIFO test is replaced by this describe block; if #37787 lands second, its FIFO test should be dropped in favour of the one here, which is the same scenario without the macOS skip. Noted on #37787 as well.

Out of scope, same kernel behaviour, reported separately: WriteFile's EVFILT_WRITE wait on a FIFO cannot see the reader going away either (a parked Bun.write to a FIFO hangs instead of getting EPIPE; #37852), and the streaming readers (Bun.file(fifo).stream(), FIFO responses in Bun.serve, the area #37090 is working in) register the same vnode filter on the main loop's kqueue, so their EOF on macOS depends on the writer having closed before the drain loop's last read().

Tests

test/js/bun/util/bun-file-read.test.ts, "reading a named pipe", six concurrent cases:

  • a 256 KiB payload pushed through a blocking writer, so the child drains and re-waits several times (the io: hand ReadFile/WriteFile to the io thread through the owner's pointer, not &mut self.io_request #37787 scenario; traced on Linux with the WriteFile debug scope, 300 KB is five waitForReadable round trips there);
  • a writer that connects only after the child opened the FIFO;
  • a writer that connects and closes without writing (must resolve to "");
  • a FIFO passed to the child as stdin, read through Bun.stdin.text() (the fd-based classification);
  • a FIFO opened in the child after 1024 other descriptors and read through Bun.file(fd), so the select set needs a second word and nfds > FD_SETSIZE; this is what pins the $DARWIN_EXTSN variant (the plain one returns EINVAL here, which would surface as a rejected read);
  • a worker whose Bun.file(fifo).text() is waiting on a connected but idle writer must still be terminate()-able (stdout reading / terminated, natural exit). This one pins the pool-task shape: it hangs if the wait happens inside JobContext::run, and passes on main, where the read parks on the io thread.

All of them pass on Linux with and without this change, since Linux reports FIFO EOF through epoll; macOS is where the first five distinguish the two trees, so the darwin lanes of this PR are the fail-to-pass evidence, with build 93007 of #37787 as the failing-before data point for the first case. The other four hang on main for the same reason (data or a close is delivered, then the poll pre-check is blind and the kqueue wait never fires). I have no macOS machine in this environment, so I could not run them there myself before CI.

Verification

Debug build on Linux: the file above (11 pass), test/regression/issue/07500, bun-stdin-slice, bun-file-fd-read, and test/internal/source-lints (82 pass). cargo clippy -p bun_sys -p bun_runtime is clean natively and for aarch64-apple-darwin (the only kind of target where the new code is compiled); cargo check of the same for x86_64-apple-darwin, and of bun_sys for x86_64-unknown-freebsd and x86_64-pc-windows-msvc (the tag table is shared); cargo fmt and prettier are clean. Both darwin build lanes pass on every revision, which covers the new symbol's linkage, and the :darwin: 14 aarch64 test lane of build 93455 ran the file above against fc757cd: 11 pass, 0 fail, Ran 11 tests across 1 file. [657.00ms] (the four-case revision 8bf43ec had passed the same way in build 93193).

XNU attaches a FIFO's EVFILT_READ filter to the vnode, where it only
activates while bytes are buffered; closing the last writer posts nothing,
and poll(2) is implemented on top of kqueue. So once ReadFile had drained a
named pipe and handed itself to the io thread it never learned about EOF,
and Bun.file(fifo).bytes()/text() (or Bun.stdin with a FIFO as stdin) never
resolved. select(2) reaches the FIFO's socket, whose readability includes
the state set by the last writer's close, so on macOS a ReadFile on a FIFO
vnode (S_IFIFO with a non-zero st_dev; pipe(2) pipes report 0 and keep
using kqueue) now waits in select on the pool thread instead, including
before its first read so a writer that has not connected yet is waited for
as on Linux.
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

The macOS runtime now detects named FIFOs and waits for readability before and between reads. A Darwin select binding provides the blocking helper. Tests cover byte, text, empty-writer, delayed-writer, and inherited-stdin FIFO reads.

macOS FIFO reads

Layer / File(s) Summary
Darwin select readability helper
src/sys/lib.rs
Adds the macOS select syscall tag, binding, and blocking helper with EINTR retry and error reporting.
ReadFile FIFO read flow
src/runtime/webcore/blob/read_file.rs
Tracks named FIFOs, detects them from metadata, and waits for readability before initial and subsequent reads.
FIFO read validation
test/js/bun/util/bun-file-read.test.ts
Adds FIFO tests for chunked bytes, delayed writers, empty writers, and inherited stdin.

Suggested reviewers: jarred-sumner, dylan-conway

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the macOS named-pipe EOF fix for Bun.file(fifo).bytes().
Description check ✅ Passed The description explains the problem, implementation, scope, tests, and verification results in detail.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review; the diff is final (code unchanged since fc757cd, dc62568 only renamed the worker test's spawned files to *.fixture.ts).

CI: build 93635 (dc62568) finished with every test lane green, including all four darwin test lanes (179 jobs passed, flaky-only warnings); its one red is the linux aarch64-android build lane failing to download lol-html from github.com, which nothing in this diff touches (the previous build's reds were the same download problem on other lanes). I am not going to push again just to re-roll that.

macOS evidence: build 93455, :darwin: 14 aarch64 - test-bun, bun test v1.4.0-canary.1 (fc757cdd3): test/js/bun/util/bun-file-read.test.ts reported 11 pass, 0 fail, 23 expect() calls, Ran 11 tests across 1 file. [657.00ms]. That covers all six named-pipe cases on a real Mac, including the descriptor above FD_SETSIZE (the unlimited select variant with a two-word set) and terminating a worker whose read is parked on an idle writer (the wait runs in its own pool task, outside the job's VM borrow). The earlier revision 8bf43ec had likewise passed its four cases on darwin 14 in build 93193 (9 pass ... [350.00ms]). On main the same scenarios hang until the 90 s timeout (build 93007 on #37787). The darwin build lanes pass on every revision, which covers the select$DARWIN_EXTSN$NOCANCEL linkage on x64 and aarch64.

Reproduced how: no macOS machine in this environment, so the diagnosis is from the XNU sources listed in the description (FIFO EVFILT_READ knotes live on the vnode and only activate on buffered bytes; fifo_close_internal posts nothing; poll(2) is built on the same filter; select(2) goes through the FIFO's socket and does see the close; pipe_stat leaves st_dev 0, which is what keeps pipe(2) pipes on the existing path). The tests pass on Linux with and without the change, as expected, since Linux reports FIFO EOF through epoll; macOS is where they distinguish the two trees, per the lanes above.

For the reviewer: the one design call in here is waiting on a pool thread (the same thing fs.readFile(fifo) does on every platform) rather than teaching the io thread to select(); the description's "Why this shape" paragraph has the trade-off, and the composition note covers the textual conflict with #37787. Siblings with the same kernel cause are #37852 (write side) and the streaming readers (reported separately).

Comment thread src/runtime/webcore/blob/read_file.rs Outdated
Comment thread src/runtime/webcore/blob/read_file.rs Outdated
Comment thread src/runtime/webcore/blob/read_file.rs Outdated
Comment thread src/runtime/webcore/blob/read_file.rs Outdated
Comment thread src/sys/lib.rs Outdated
Comment thread src/sys/lib.rs Outdated
@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 9:02 PM PT - Aug 12th, 2026

@robobun, your commit dc62568 has 1 failures in Build #93635 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 37823

That installs a local version of the PR into your bun-37823 executable, so you can run:

bun-37823 --bun

Comment thread test/js/bun/util/bun-file-read.test.ts Outdated
Comment thread src/runtime/webcore/blob/read_file.rs Outdated
Comment thread src/sys/lib.rs Outdated
Comment thread src/sys/lib.rs Outdated
Comment thread src/sys/lib.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/util/bun-file-read.test.ts`:
- Around line 98-109: Update openWriterOnceChildIsReading to use a named retry
limit or deadline instead of an unbounded loop with fixed arbitrary sleeps.
Preserve the ENXIO retry behavior, but fail once the bound is exceeded with an
error containing the FIFO path and the child’s current exit/signal state.
🪄 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: 20be47b1-a0f6-46a3-8438-83bd678b4925

📥 Commits

Reviewing files that changed from the base of the PR and between 9a543cc and 7afcfad.

📒 Files selected for processing (3)
  • src/runtime/webcore/blob/read_file.rs
  • src/sys/lib.rs
  • test/js/bun/util/bun-file-read.test.ts

Comment thread test/js/bun/util/bun-file-read.test.ts

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and the automated bug hunt found nothing. All prior feedback (fd cleanup via using, bounded ENXIO retry loop, comment length) has been addressed in 64e6b54–8bf43ec. That said, a human look is warranted: the fix pins a pool thread in a blocking select() per in-flight FIFO read (an architectural tradeoff the description explicitly flags), relies on the st_dev != 0 heuristic and the select$DARWIN_EXTSN$NOCANCEL linkage that the author could not verify locally on macOS, and CI on the darwin lanes is the only fail-to-pass evidence.

What was reviewed:

  • block_until_readable: fd_set bitmap sizing (fd/32 + 1 words), EINTR retry, error path via Tag::select — looks correct for the EXTSN variant.
  • do_read_loop named-pipe branch: on select failure the buffer is restored before on_finish(), and retry/EOF handling is preserved for non-FIFO fds.
  • Test 1's childDied rejection after the race resolves — Promise.race attaches a handler, so no unhandled rejection.
Extended reasoning...

Overview

The PR fixes Bun.file(fifo).bytes() / .text() and Bun.stdin.text() hanging forever on macOS when reading a named pipe: XNU's kqueue EVFILT_READ filter for FIFO vnodes only activates on buffered bytes and never fires for the last writer closing, and poll(2) on macOS is built on the same filter. The fix adds bun_sys::block_until_readable (a blocking select(2) via select$DARWIN_EXTSN$NOCANCEL) and has ReadFile on macOS classify named pipes via S_IFIFO && st_dev != 0, then wait in select on the pool thread instead of handing off to the kqueue io thread. Four subprocess tests cover the multi-chunk drain, late-connecting writer, empty writer, and FIFO-as-stdin cases.

Security risks

None identified. The new code is macOS-only, gated on an fstat result, and the select fd_set is sized to the single fd being waited on. No user-controlled input reaches the new syscall path beyond the fd itself.

Level of scrutiny

High. This changes core I/O behavior in ReadFile::do_read_loop and run_async_with_fd, adds a new FFI import with a Darwin-specific symbol name, and the author explicitly states they have no macOS machine — the diagnosis is derived entirely from XNU source reading and the darwin CI lanes are the only verification. The architectural choice to block a pool thread per FIFO read (rather than integrating a select-based wait into the io thread) is called out in the description as a deliberate tradeoff that may need revisiting; a maintainer should confirm that shape is acceptable.

Other factors

  • All bot feedback (comment-cop on comment length, my earlier nit on test fd cleanup, CodeRabbit on bounding the ENXIO retry loop) is addressed and resolved in the timeline.
  • The st_dev != 0 discriminator between pipe(2) pipes and FIFO vnodes is justified by an XNU pipe_stat citation but is the kind of heuristic a macOS-experienced reviewer should sanity-check.
  • Tests are well-constructed (concurrent, hermetic, using-guarded fds, bounded polling), but they pass on Linux with or without the change, so darwin CI results are load-bearing.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

The write-side counterpart mentioned under "out of scope" is #37852. Its select$DARWIN_EXTSN$NOCANCEL import and Tag::select hunks in src/sys/lib.rs are written identically to the ones here, so whichever lands second should merge cleanly.

…e job's VM borrow

JobContext::run holds a VM borrow for the duration of the call and VM teardown
waits for borrows, so blocking in select() there kept a worker with a read
waiting on a FIFO writer from ever being terminated. run_async_with_fd now
schedules read_named_pipe_task and returns; the task does the waits and the
read loop with no borrow held, like the io-thread hand-off does.

Tests: a worker parked on an idle FIFO writer can be terminated; a FIFO read
through a descriptor above FD_SETSIZE, which is what the unlimited select
variant and the hand-built bitmap are for.
Comment thread src/runtime/webcore/blob/read_file.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR again after fc757cd — the earlier fd-cleanup nit and CodeRabbit's unbounded-retry point are both addressed. No bugs found in this pass. Because this adds a new Darwin $NOCANCEL FFI binding, changes pool-thread scheduling around VM borrows, and the author had no macOS machine (darwin CI lanes are the only fail→pass evidence), a maintainer look is still worthwhile.

What was reviewed:

  • block_until_readable: fd_set sizing (fd/32 + 1 words), EINTR retry, error path via Tag::select — checked the high-fd test covers the >1024 case the $DARWIN_EXTSN variant exists for.
  • is_named_pipe classification (S_IFIFO && st_dev != 0) keeps pipe(2) pipes on the existing kqueue path; the stdin-as-FIFO test covers the fd-based entry.
  • read_named_pipe_task scheduling as a separate WorkPool task so the blocking select() runs outside JobContext::run's VM borrow — the new worker-terminate test exercises exactly that.
  • The mid-loop block_until_readable() in do_read_loop restores self.buffer on both continue and break paths (buffer is moved back after the loop / via on_finish).

Note: comment-cop still has one open flag on the read_named_pipe_task doc comment (read_file.rs:490).

Extended reasoning...

Overview

Three files: src/sys/lib.rs gains a macOS-only select$DARWIN_EXTSN$NOCANCEL binding, a Tag::select entry, and block_until_readable(fd) that builds a heap-allocated bitmap read set and loops on EINTR. src/runtime/webcore/blob/read_file.rs records is_named_pipe from the existing fstat, and on macOS routes named-pipe reads to a separate WorkPool task that blocks in select() before the first read and between drained reads, instead of the kqueue EVFILT_READ path that XNU never wakes for a FIFO's writer-close. Tests add six it.concurrent cases in bun-file-read.test.ts covering large payloads, late-connecting writer, empty-writer close, FIFO-as-stdin, fd > FD_SETSIZE, and worker termination while a read is parked in select().

Security risks

None identified. The new code only reads from an already-open fd; the select() bitmap is sized from fd.native() (a valid non-negative fd, debug-asserted) so index / 32 cannot underflow. No user-controlled data reaches the new syscall path beyond the fd itself.

Level of scrutiny

High. This is platform-specific I/O in core runtime code with new FFI (linking a Darwin-specific libsyscall symbol), a change to which thread a read blocks on, and an interaction with VM teardown (JobContext::run holds a borrow that terminate() waits for — fc757cd moved the blocking wait out of it and added a test for exactly that). The author explicitly could not run any of it on macOS; correctness rests on XNU source reading and the darwin CI lanes. The design choice to pin one pool thread per in-flight FIFO read is called out in the description as a deliberate tradeoff — that's the kind of architectural call a maintainer should ratify.

Other factors

  • My previous nit (fd cleanup via using) was addressed in 64e6b54 with an openFd() disposable helper.
  • CodeRabbit's bounded-retry request was addressed in 8bf43ec (10s deadline + child-state error message in openWriterOnceChildIsReading).
  • One comment-cop flag remains unresolved on the 4-line doc comment for read_named_pipe_task; the comment explains the VM-borrow constraint and reads as necessary rather than as workaround justification, but that's a maintainer call.
  • Tests are well-structured (concurrent, subprocess-isolated, drain all pipes, assert combined objects, deadline-bounded polling) and cover the variant matrix including the fd > 1024 case that motivates the $DARWIN_EXTSN symbol choice.
  • The Tag name-table change is append-only (index 108 → "select"), so existing err.syscall strings are unaffected.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR again after the fd-cleanup and pool-task changes and didn't find any bugs. Because it adds a new macOS-only FFI binding and a third async-read path (blocking a pool thread in select(2)) that the author could not run locally, and because it interacts with the VM-borrow/worker-teardown machinery, a maintainer should still look at the design before this lands.

What was reviewed:

  • block_until_readable's fd_set sizing (fd/32 + 1 words) and the $DARWIN_EXTSN linkage against the fd-above-1024 test — the plain select would EINVAL there.
  • The st_dev != 0 discriminator keeps pipe(2) pipes on the existing io-thread path; only FIFO vnodes take the new branch.
  • The pool-task shape releases the job's VM borrow before blocking, and on_finish runs on both the select-failed and read-loop exits — no path leaves io_task unset.
  • Test fds are now held via using openFd(...) per my earlier note, and openWriterOnceChildIsReading is deadline-bounded.
Extended reasoning...

Overview

This PR fixes Bun.file(fifo).bytes() / .text() etc. never resolving on macOS when reading a named pipe. It adds a macOS-only select$DARWIN_EXTSN$NOCANCEL FFI import and bun_sys::block_until_readable helper in src/sys/lib.rs, and in src/runtime/webcore/blob/read_file.rs adds an is_named_pipe flag (S_IFIFO with non-zero st_dev) that routes named-pipe reads to a new pool task which blocks in select(2) before the first read and again whenever a read drains the pipe. Six new tests in test/js/bun/util/bun-file-read.test.ts cover multi-chunk payloads, late-connecting writers, empty writers, FIFO-as-stdin, fd > FD_SETSIZE, and worker terminate() while a FIFO read is parked.

Security risks

None identified. The new code path is macOS-only, gated on an fstat result the reader already performs, and only changes which syscall the reader waits in. The select read-set is sized from the fd number (heap-allocated vec[0u32; fd/32 + 1]), so there is no fixed-size buffer to overrun. No user-controlled data reaches the new FFI call beyond the fd itself.

Level of scrutiny

High. This is platform-specific native code touching async I/O, threading (WorkPool vs io thread), and VM-lifetime interactions (the pool-task shape exists specifically so a worker's terminate() is not blocked by a parked FIFO read). The author explicitly had no macOS machine and relied on CI for verification. The design pins one pool thread per in-flight FIFO read, which the author acknowledges as a tradeoff a maintainer should accept or reject. It also textually conflicts with open PR #37787.

Other factors

  • All prior review threads (comment-cop on comment length, CodeRabbit on the retry bound, my earlier fd-leak nit) are marked resolved and the fixes are visible in the current diff.
  • The bug-hunting system found no issues.
  • The XNU-source-level reasoning in the description is thorough and the st_dev == 0 discriminator for pipe(2) vs FIFO vnodes is well-cited, but this is exactly the kind of kernel-behaviour claim a macOS-experienced maintainer should confirm.
  • The change is confined behind #[cfg(target_os = "macos")] so Linux/Windows behaviour is unchanged by construction; still, adding a third read path to ReadFile is an architectural decision, not a mechanical fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant