Treat unregistering an already-removed poll registration as success#31821
Conversation
A FilePoll's EV_DELETE (kqueue) or EPOLL_CTL_DEL can fail with ENOENT/EBADF when the kernel-side registration is already gone. Two routine producers: - the fd was closed while the poll was still registered: close() removes an fd's kevents, and epoll drops closed fds automatically - on macOS, closing a pty master marks the slave's knotes EV_EOF|EV_ONESHOT, so the kernel deletes them when the hangup event is delivered; the reader's teardown EV_DELETE then finds nothing. This happens on every terminal window/tab close while a tty is polled (e.g. raw-mode stdin) Both mean the kernel state already matches what unregistration wants, so count them as success instead of returning an error. The error return also skipped the flag-clearing at the end of unregister_with_fd_impl, leaving the poll claiming to be registered and re-issuing doomed deletes on later teardown calls. libuv ignores the same errnos for its kqueue/epoll delete operations. Follow-up to #31701, which fixed the errno decoding for these entries but still surfaced them as errors. Verified with a pty harness (bun under script(1) polling raw stdin, then SIGHUP + revoke + pty master kill): teardown is clean, stdin delivers end/close, and the process exits normally. Existing coverage: spawn-pipe-stale-fd-unregister.test.ts exercises the modified macOS kevent and Linux epoll branches.
WalkthroughUpdated POSIX event loop deregistration error handling to treat EBADF and ENOENT as successful outcomes. Added a documented helper function and applied the concept across Linux, macOS, and FreeBSD unregister paths to tolerate expected teardown races. ChangesDeregistration error handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the change is small, well-reasoned, and matches libuv's handling — but since it alters error semantics in the core POSIX event loop deregistration path across three platforms, it's worth a maintainer's quick look.
Extended reasoning...
Overview
This PR modifies FilePoll::unregister_with_fd_impl in src/io/posix_event_loop.rs so that ENOENT/EBADF from EPOLL_CTL_DEL / EV_DELETE are treated as successful deregistration rather than errors. A new deregistration_already_gone helper encodes the predicate, and each of the three platform branches (Linux epoll, macOS kevent64, FreeBSD kevent) now falls through to the flag-clearing tail instead of early-returning. The macOS per-entry EV_ERROR check is also refactored from two unrolled conditionals into a loop over 0..rc.min(2) — semantically equivalent (the rc < 0 global-failure case is already handled above, so the usize::try_from cannot fail).
Security risks
None. This is internal event-loop bookkeeping with no user-facing input, auth, crypto, or data-exposure surface. Treating already-gone registrations as success cannot grant new capabilities; at worst it would mask a programming error, but the PR description and libuv precedent make a convincing case that these errnos are routine teardown outcomes.
Level of scrutiny
Moderate-to-high. The diff is small (~40 lines net) and the logic is straightforward, but it lives in the core POSIX event loop and changes error-path control flow on every supported Unix platform. The key behavioral effect — that the PollReadable/PollWritable/etc. flags now get cleared even when the kernel-side registration was already gone — is the whole point and looks strictly beneficial, but a maintainer familiar with the FilePoll lifecycle should confirm there's no caller that relies on the old error return to detect a stale fd.
Other factors
- All in-tree callers reportedly discard the unregister result, so the only observable change is the flag clearing (avoids redundant doomed
EV_DELETEs on later teardown). - Matches libuv's handling of the same errnos on the same syscalls.
- Existing tests (
spawn-pipe-stale-fd-unregister.test.ts,run-crash-handler.test.ts) exercise the modified branches; no new test added since the change has no JS-observable surface. - No CODEOWNERS entry covers this file.
- No prior reviews or outstanding reviewer comments on the PR.
There was a problem hiding this comment.
I didn't find any issues with the logic, but this touches core event-loop fd teardown across three platform branches and the CI run shows a terminal.test.ts failure on macOS 14 x64 (the exact pty/terminal teardown scenario this PR discusses), so it's worth a human look before merging.
Extended reasoning...
Overview
This PR modifies FilePoll::unregister_with_fd_impl in src/io/posix_event_loop.rs so that ENOENT/EBADF from EPOLL_CTL_DEL (Linux), EV_DELETE via kevent64 (macOS), and EV_DELETE via kevent (FreeBSD) are treated as successful deregistration rather than errors. A new deregistration_already_gone helper encodes the errno set, and the macOS branch is refactored from two unrolled index checks into a bounded loop over 0..rc.min(2) that skips already-gone entries before falling back to kevent_change_error. The rationale (kernel already removed the registration; libuv does the same; early-return was leaving stale poll_* flags set) is well-researched and clearly documented.
Security risks
None identified. This is errno-classification logic in the teardown path; no untrusted input, auth, crypto, or data exposure surface is involved.
Level of scrutiny
High. This is the core POSIX event-loop fd-lifecycle code that every pipe/socket/pty/process poll in the runtime flows through. Swallowing errnos in this layer is correct here per the libuv precedent and the kernel-behavior analysis in the PR description, but mistakes in this area can manifest as hangs, leaked registrations, or interactions with fd reuse that are hard to reproduce. The macOS branch also includes a small structural refactor (loop + usize::try_from(rc.min(2))), not just an added guard. This is not a mechanical or config-level change.
Other factors
- The CI build shows
test/js/bun/terminal/terminal.test.tsfailing on macOS 14 x64. The PR's motivating scenario is specifically macOS pty/terminal teardown, so this failure may be related and should be checked rather than assumed flaky. (The widespreadbunx.test.tsfailures, including on Windows where this file doesn't compile, look unrelated.) - No new test is added; the author notes the change has no JS-observable surface since callers discard the unregister result, and points to the existing
spawn-pipe-stale-fd-unregister.test.tsfor coverage of the modified branches. - No CODEOWNERS entry covers this path.
Given the criticality of the code path and the possibly-related macOS terminal test failure, I'm deferring to a human reviewer rather than approving.
2 participants
What
FilePoll::unregister_with_fd_implreturns an error when itsEV_DELETE(kqueue) orEPOLL_CTL_DELfails with ENOENT/EBADF. Those errnos mean the kernel-side registration is already gone — which is a routine teardown outcome, not a failure. This PR counts them as successful deregistration on all three platform branches (macOS kevent, Linux epoll, FreeBSD kevent).Why
Two ordinary ways a registration disappears underneath a live
FilePoll:close()removes an fd's kevents, and epoll drops closed fds automatically. The forced unregister during reader teardown then gets EBADF/ENOENT. This is the scenario from Fix panic when FilePoll unregister fails on macOS #31701.EV_EOF|EV_ONESHOT, so the kernel deletes them when the hangup event is delivered (verified: the delivered event carriesflags=0x8095, i.e.EV_EOF|EV_ONESHOT|EV_DISPATCH|EV_ENABLE|EV_ADD, even though we registered withEV_DISPATCH). The reader's teardownEV_DELETEthen finds nothing → ENOENT. This fires on every terminal window/tab close while a tty is polled, e.g. raw-mode stdin.#31701 fixed the errno decoding for these entries (they previously fed an errno value through the −1-sentinel decoder and panicked on the unwrap), but still surfaces them as errors. That has a real cost: the early error return skips the registration-flag clearing at the end of
unregister_with_fd_impl, so the poll still claimspoll_readableand a later teardown call re-issues another doomedEV_DELETE(observed via instrumentation: the same poll failing the delete twice during one pty teardown). Every in-tree caller discards the unregister result, so nothing recovers the state.libuv handles this the same way:
uv__kqueue_delete(src/unix/kqueue.c) returns silently for exactly EBADF/ENOENT onEV_DELETE(and aborts on anything else), and the epoll side ignoresEPOLL_CTL_DELfailures for closed fds (uv__platform_invalidate_fddiscards the return value; the io_uring batch path comments "Failed submissions are either EPOLL_CTL_DEL commands for file descriptors that have been closed ... Ignore the former").How verified
script(1)polling raw-mode stdin (plus subprocess churn), then SIGHUP +revoke(2)+ pty-master SIGKILL — 5/5 runs tear down cleanly, stdin deliversend/close, process exits normallytest/js/bun/spawn/spawn-pipe-stale-fd-unregister.test.tspasses (exercises the modified macOS kevent and Linux epoll branches)test/cli/run/run-crash-handler.test.tspassesNo new test: callers discard the unregister result, so the change has no JS-observable surface; the modified branches are covered by the existing stale-fd fixture above.