Skip to content

Commit c20b05e

Browse files
committed
Use pidfd for cancellable _ForkedProc.wait
Two coordinated improvements to the `subint_forkserver` backend: 1. Replace `trio.to_thread.run_sync(os.waitpid, ..., abandon_on_cancel=False)` in `_ForkedProc.wait()` with `trio.lowlevel.wait_readable(pidfd)`. The prior version blocked a trio cache thread on a sync syscall — outer cancel scopes couldn't unwedge it when something downstream got stuck. Same pattern `trio.Process.wait()` and `proc_waiter` (the mp backend) already use. 2. Drop the `@pytest.mark.xfail(strict=True)` from `test_orphaned_subactor_sigint_cleanup_DRAFT` — the test now PASSES after 0cd0b63 (fork-child FD scrub). Same root cause as the nested-cancel hang: inherited IPC/trio FDs were poisoning the child's event loop. Closing them lets SIGINT propagation work as designed. Deats, - `_ForkedProc.__init__` opens a pidfd via `os.pidfd_open(pid)` (Linux 5.3+, Python 3.9+) - `wait()` parks on `trio.lowlevel.wait_readable()`, then non-blocking `waitpid(WNOHANG)` to collect the exit status (correct since the pidfd signal IS the child-exit notification) - `ChildProcessError` swallow handles the rare race where someone else reaps first - pidfd closed after `wait()` completes (one-shot semantics) + `__del__` belt-and-braces for unexpected-teardown paths - test docstring's `@xfail` block replaced with a `# NOTE` comment explaining the historical context + cross-ref to the conc-anal doc; test remains in place as a regression guard The two changes are interdependent — the cancellable `wait()` matters for the same nested- cancel scenarios the FD scrub fixes, since the original deadlock had trio cache workers wedged in `os.waitpid` swallowing the outer cancel. (this patch was generated in some part by [`claude-code`][claude-code-gh]) [claude-code-gh]: https://github.com/anthropics/claude-code
1 parent 9993db0 commit c20b05e

2 files changed

Lines changed: 60 additions & 27 deletions

File tree

tests/spawn/test_subint_forkserver.py

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -446,22 +446,15 @@ def _process_alive(pid: int) -> bool:
446446
return False
447447

448448

449-
@pytest.mark.xfail(
450-
strict=True,
451-
reason=(
452-
'subint_forkserver orphan-child SIGINT hang: trio\'s '
453-
'event loop stays wedged in `epoll_wait` despite the '
454-
'SIGINT handler being correctly installed and the '
455-
'signal being delivered at the kernel level. NOT a '
456-
'"handler missing on non-main thread" issue — post-'
457-
'fork the worker IS `threading.main_thread()` and '
458-
'trio\'s `KIManager` handler is confirmed installed. '
459-
'Full analysis + ruled-out hypotheses + fix directions '
460-
'in `ai/conc-anal/'
461-
'subint_forkserver_orphan_sigint_hang_issue.md`. '
462-
'Flip this mark (or drop it) once the gap is closed.'
463-
),
464-
)
449+
# NOTE: was previously `@pytest.mark.xfail(strict=True, ...)`
450+
# for the orphan-SIGINT hang documented in
451+
# `ai/conc-anal/subint_forkserver_orphan_sigint_hang_issue.md`
452+
# — now passes after the fork-child FD-hygiene fix in
453+
# `tractor.spawn._subint_forkserver._close_inherited_fds()`:
454+
# closing all inherited FDs (including the parent's IPC
455+
# listener + trio-epoll + wakeup-pipe FDs) lets the child's
456+
# trio event loop respond cleanly to external SIGINT.
457+
# Leaving the test in place as a regression guard.
465458
@pytest.mark.timeout(
466459
30,
467460
method='thread',

tractor/spawn/_subint_forkserver.py

Lines changed: 51 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -526,6 +526,18 @@ def __init__(self, pid: int):
526526
self.stdin = None
527527
self.stdout = None
528528
self.stderr = None
529+
# pidfd (Linux 5.3+, Python 3.9+) — a file descriptor
530+
# referencing this child process which becomes readable
531+
# once the child exits. Enables a fully trio-cancellable
532+
# wait via `trio.lowlevel.wait_readable()` — same
533+
# pattern `trio.Process.wait()` uses under the hood, and
534+
# the same pattern `multiprocessing.Process.sentinel`
535+
# uses for `tractor.spawn._spawn.proc_waiter()`. Without
536+
# this, waiting via `trio.to_thread.run_sync(os.waitpid,
537+
# ...)` blocks a cache thread on a sync syscall that is
538+
# NOT trio-cancellable, which prevents outer cancel
539+
# scopes from unwedging a stuck-child cancel cascade.
540+
self._pidfd: int = os.pidfd_open(pid)
529541

530542
def poll(self) -> int | None:
531543
'''
@@ -555,22 +567,40 @@ def returncode(self) -> int | None:
555567

556568
async def wait(self) -> int:
557569
'''
558-
Async blocking wait for the child's exit, off-loaded
559-
to a trio cache thread so we don't block the event
560-
loop on `waitpid()`. Safe to call multiple times;
561-
subsequent calls return the cached rc without
562-
re-issuing the syscall.
570+
Async, fully-trio-cancellable wait for the child's
571+
exit. Uses `trio.lowlevel.wait_readable()` on the
572+
`pidfd` sentinel — same pattern as `trio.Process.wait`
573+
and `tractor.spawn._spawn.proc_waiter` (mp backend).
574+
575+
Safe to call multiple times; subsequent calls return
576+
the cached rc without re-issuing the syscall.
563577
564578
'''
565579
if self._returncode is not None:
566580
return self._returncode
567-
_, status = await trio.to_thread.run_sync(
568-
os.waitpid,
569-
self.pid,
570-
0,
571-
abandon_on_cancel=False,
572-
)
581+
# Park until the pidfd becomes readable — the OS
582+
# signals this exactly once on child exit. Cancellable
583+
# via any outer trio cancel scope (this was the key
584+
# fix vs. the prior `to_thread.run_sync(os.waitpid,
585+
# abandon_on_cancel=False)` which blocked a thread on
586+
# a sync syscall and swallowed cancels).
587+
await trio.lowlevel.wait_readable(self._pidfd)
588+
# pidfd signaled → reap non-blocking to collect the
589+
# exit status. `WNOHANG` here is correct: by the time
590+
# the pidfd is readable, `waitpid()` won't block.
591+
try:
592+
_, status = os.waitpid(self.pid, os.WNOHANG)
593+
except ChildProcessError:
594+
# already reaped by something else
595+
status = 0
573596
self._returncode = self._parse_status(status)
597+
# pidfd is one-shot; close it so we don't leak fds
598+
# across many spawns.
599+
try:
600+
os.close(self._pidfd)
601+
except OSError:
602+
pass
603+
self._pidfd = -1
574604
return self._returncode
575605

576606
def kill(self) -> None:
@@ -584,6 +614,16 @@ def kill(self) -> None:
584614
except ProcessLookupError:
585615
pass
586616

617+
def __del__(self) -> None:
618+
# belt-and-braces: close the pidfd if `wait()` wasn't
619+
# called (e.g. unexpected teardown path).
620+
fd: int = getattr(self, '_pidfd', -1)
621+
if fd >= 0:
622+
try:
623+
os.close(fd)
624+
except OSError:
625+
pass
626+
587627
def _parse_status(self, status: int) -> int:
588628
if os.WIFEXITED(status):
589629
return os.WEXITSTATUS(status)

0 commit comments

Comments
 (0)