io: leave the fd with the caller when a POSIX pipe writer fails to start

Merged
RO
2mo ago
+316-38

Problem

  • ASAN test workers (bun test --test-worker --isolate) die with panic: assertion failed: err.is_none() in <bun_core::util::Fd as bun_sys::fd::FdExt>::close (src/sys/fd.rs:73), called from bun_io::closer::Closer on a thread pool thread: the deferred close(2) got EBADF, so the fd number had already been closed by someone else. Seen three times across seven PR builds (for example build 94781); the worker's in-flight file is reported as the crash and passes on retry, so it never fails a build.
  • Cause: PosixStreamingWriter::start (src/io/PipeWriter.rs) stores the fd in a freshly created poll and then registers it. When epoll_ctl fails it returns Err with the poll still holding the fd. FileSink::setup (src/runtime/webcore/FileSink.rs:679) closes the fd on that Err, and Blob.writer() then releases the sink, whose writer Drop hands the same fd number to Closer. Debug builds trip the assertion; release builds close whatever fd has been given that number in the meantime (reproduced: a /dev/null fd opened right after came back EBADF).
  • What makes the registration fail in CI is the EEXIST from #37968: under --isolate each file's process.stdout/stderr sink leaves a stale epoll entry keyed on a dup number that a later dup lands on. Since internal/fs/streams wraps Bun.file(fd).writer() in a try/catch, that failure is silent and only the double close is visible. #38008 removes that trigger; this PR fixes the double close, which follows any registration failure (ENOSPC when max_user_watches is exhausted, for instance).
  • PosixBufferedWriter::start has the same shape. Its callers happened to rely on the opposite convention (the writer closes the fd on failure), as did Bun.Terminal and the subprocess stdin FileSink, so the two conventions coexisted and the callers disagreed about who owns the fd after a failed start().

Fix

  • A failed start() on either POSIX writer releases the poll it created (PollOrFd::close_without_closing_fd, new) and leaves the fd with the caller. A poll left over from an earlier start() is not touched: it still holds that earlier fd, which the writer does own (the shell re-arms its writer this way).
  • Why this direction: it makes start() have no effect on failure, matches the Windows writer (a failed open never adopts the handle), and makes the fd's owner the same on every path: whoever passed it in closes it on Err, the writer closes it after Ok. FileSink::setup is already written this way and is now correct without changes.
  • Callers that relied on the writer closing the fd on failure now close it themselves: Bun.Terminal closes its pty write fd on every platform (the Windows branch already did), Writable::init closes the stdin pipe after releasing the sink, StaticPipeWriter::start closes its stdio_result. The shell IOWriter's EINVAL/EPERM retry no longer needs to tear the poll down itself; it owns its fd separately and is otherwise unchanged.
  • The same callers also relied on the writer's later close() reporting on_close, which an empty writer no longer does. For Bun.spawn with a buffer stdin that report was what retired the Writable::Buffer slot; left in place it counts as pending activity and pins the Subprocess wrapper forever (caught in review). The spawn bindings now retire the slot on that error path themselves (on_close_io(Stdin)), on POSIX only since Windows adopts the pipe before start() and cannot fail there. The shell drops its subprocess outright on this path and Bun.Terminal destroys itself, so they needed nothing.
  • close_impl on Windows ignored its close_fd argument; it now honors it so the new helper means the same thing on every platform (no live Windows caller passes false; PollOrFd only backs the POSIX reader and writers).
  • Verification:
    • test/js/bun/util/filesink.test.ts ("whose registration fails closes the dup exactly once"): recreates the CI condition on a socketpair fd (close a sink's dup from under it, the next dup gets EEXIST), with the thread pool parked so the stray close has to land on the fd that reused the number. Unfixed debug and release builds report reusedStillOpen: false; with the fix it stays open. Linux only, as kqueue drops registrations with the fd.
    • test/js/bun/spawn/spawn-pipe-start-error.test.ts: an LD_PRELOAD shim fails every writable epoll_ctl registration (Bun issues them through syscall(2)), and fixtures for stdin: "pipe", a buffer stdin (memfd disabled so the pipe writer path is taken) and new Bun.Terminal() assert that the fd count and the number of live Subprocess/Terminal wrappers return to baseline with an empty stderr. All three pass on bun 1.4.0 and with this PR; with the three caller-side closes removed each fixture reports leakedFds: 1, and without the bindings change the buffer fixture reports leakedWrappers: 1. 30/30 runs stable on the debug build.
    • Also run: filesink, bunshell, shell file-io/epipe/output, spawn, spawnSync, terminal, spawn stdin stream suites; cargo fmt --check and cargo clippy on bun_io/bun_spawn; cargo check of bun_io, bun_spawn and bun_runtime for x86_64-pc-windows-msvc.

Background

  • PollOrFd is the handle inside the POSIX pipe readers and writers: either a bare fd or a FilePoll, the event loop's epoll/kqueue registration, which records the fd it watches. Closing the handle unregisters the poll and then closes the fd, normally through Closer, which performs the close(2) on the thread pool so the event loop does not block on it. That delay is why the second close lands after the fd number has been reused.
  • Fd::close asserts that close(2) did not return EBADF in debug and ASAN builds, because for an internally owned fd EBADF means a double close and therefore a possible close of an unrelated fd. That assertion is the crash in the report and is intentionally left in place.
  • FileSink is the native writer behind Bun.file(...).writer(), process.stdout/stderr and a piped subprocess stdin; StaticPipeWriter writes a Buffer/Blob stdin into a child; both own a PosixStreamingWriter/PosixBufferedWriter from src/io/PipeWriter.rs.
  • A Subprocess holds a strong reference to its own JS wrapper while it has pending activity: a child that has not exited, or a stdio slot still in use. Each slot is retired through on_close_io, which the stdio object reports when it closes; once the slots are retired and the child has exited, the reference is downgraded and the wrapper becomes collectable.
  • Registering a pollable fd can fail in production with ENOSPC (fs.epoll.max_user_watches) or, as in #37968, EEXIST when epoll still holds an entry for the same open file under the same fd number; epoll keys entries on (open file, fd number), so an entry outlives an fd number that was closed while another fd kept the file open.
Bugs noticed on the way, tracked separately
  • ShellSubprocess::abort_after_failed_start leaks the not-yet-started stdout/stderr pipe fds when the buffered stdin writer fails to start (visible with the shim from the new test). Handed off.
  • The security scanner's handling of a failed StaticPipeWriter::start releases one ref too many; #37898 already changes that site.

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/filesink.test.ts, test/js/bun/spawn/spawn-pipe-start-error.test.ts