From 49213d170eec776671134e2243d28c7d6b464bf6 Mon Sep 17 00:00:00 2001 From: goodboy Date: Tue, 18 Aug 2026 01:58:45 -0400 Subject: [PATCH] Reap `to_actor.run()` children before return Give each `ActorNursery` child its own reap request and completion event. Owned one-shots now wait for process joining and bookkeeping removal before returning. Escalate unacknowledged cancellation with `proc.kill()` after an active debugger releases. Latch nursery-wide teardown for monitors that finish startup late, and snapshot children before cancellation checkpoints permit concurrent removal. Cover immediate managed-nursery cleanup, failed cancel acknowledgements and late monitor registration across Trio TCP/UDS and `mp_spawn`. Caught-during: review remediation Found-via: `/run-tests` test_late_child_reap_registration_is_released Review: PR #481 (copilot-pull-request-reviewer) https://github.com/goodboy/tractor/pull/481#discussion_r3514759131 Prompt-IO: ai/prompt-io/opencode/20260818T031532Z_4151b956_prompt_io.md (this patch was generated in some part by `opencode` using `gpt-5.6-sol` (`openai`)) --- .../20260818T031532Z_4151b956_prompt_io.md | 43 +++++ ...20260818T031532Z_4151b956_prompt_io.raw.md | 74 +++++++++ tests/test_to_actor.py | 79 ++++++++- tractor/runtime/_supervise.py | 152 +++++++++++++++--- tractor/spawn/_mp.py | 9 +- tractor/spawn/_spawn.py | 27 ++-- tractor/spawn/_trio.py | 8 +- tractor/to_actor/_api.py | 11 +- 8 files changed, 351 insertions(+), 52 deletions(-) create mode 100644 ai/prompt-io/opencode/20260818T031532Z_4151b956_prompt_io.md create mode 100644 ai/prompt-io/opencode/20260818T031532Z_4151b956_prompt_io.raw.md diff --git a/ai/prompt-io/opencode/20260818T031532Z_4151b956_prompt_io.md b/ai/prompt-io/opencode/20260818T031532Z_4151b956_prompt_io.md new file mode 100644 index 00000000..e072e2a2 --- /dev/null +++ b/ai/prompt-io/opencode/20260818T031532Z_4151b956_prompt_io.md @@ -0,0 +1,43 @@ +--- +model: openai/gpt-5.6-sol +service: opencode +session: pr481-review-fixes-p1-20260818 +timestamp: 2026-08-18T03:15:32Z +git_ref: 4151b956 +scope: code +substantive: true +raw_file: 20260818T031532Z_4151b956_prompt_io.raw.md +--- + +## Prompt + +Address the approved review findings on PR #481, but work +iteratively: implement and verify one finding at a time, prepare a +separate `/commit-plan` after each fix, and stop for the human commit +before starting the next finding. Begin with the P1 per-child +lifecycle issue. Also publish the already-approved review findings +against the reviewed PR head before editing. + +## Response summary + +Published the approved non-approving review at head `4151b956`, then +implemented only the P1 lifecycle fix. Owned one-shot actors now use +a child-specific cancellation and process-reap handshake, including +hard escalation for unacknowledged cancellation and deterministic +bookkeeping removal before `to_actor.run()` returns. + +## Files changed + +- `tractor/runtime/_supervise.py` - coordinate child-specific cancel + and reap. +- `tractor/spawn/_trio.py` - wait on the Trio child's reap request. +- `tractor/spawn/_mp.py` - wait on the multiprocessing child's reap + request. +- `tractor/spawn/_spawn.py` - publish monitor completion centrally. +- `tractor/to_actor/_api.py` - await owned-child process reaping. +- `tests/test_to_actor.py` - cover cleanup, escalation, and startup + ordering. + +## Human edits + +None - the generated P1 patch remains uncommitted for human review. diff --git a/ai/prompt-io/opencode/20260818T031532Z_4151b956_prompt_io.raw.md b/ai/prompt-io/opencode/20260818T031532Z_4151b956_prompt_io.raw.md new file mode 100644 index 00000000..5b89ac13 --- /dev/null +++ b/ai/prompt-io/opencode/20260818T031532Z_4151b956_prompt_io.raw.md @@ -0,0 +1,74 @@ +--- +model: openai/gpt-5.6-sol +service: opencode +timestamp: 2026-08-18T03:15:32Z +git_ref: 4151b956 +diff_cmd: git diff HEAD~1..HEAD +--- + +Implemented only the P1 lifecycle finding from the approved PR #481 +review, preserving the requested one-fix-at-a-time commit boundary. + +> `git diff HEAD~1..HEAD -- tractor/runtime/_supervise.py` + +Added per-child reap request/completion events to `ActorNursery`, a +shielded child-specific cancel-and-reap operation, late-registration +latching for nursery teardown, and cancellation escalation that waits +for debugger release before using non-ignorable process termination. +The nursery-wide cancellation path snapshots child records before +checkpointing so concurrent one-shot cleanup cannot invalidate its +iteration. + +> `git diff HEAD~1..HEAD -- tractor/spawn/_trio.py` + +Changed Trio child monitors to wait on their per-child reap requests. + +> `git diff HEAD~1..HEAD -- tractor/spawn/_mp.py` + +Changed multiprocessing child monitors to wait on their per-child reap +requests. + +> `git diff HEAD~1..HEAD -- tractor/spawn/_spawn.py` + +Ensured every backend publishes child-reap completion after its process +monitor exits. + +> `git diff HEAD~1..HEAD -- tractor/to_actor/_api.py` + +Changed owned one-shot cleanup to await child-specific process joining +and bookkeeping removal instead of treating the cancel RPC as reaping. + +> `git diff HEAD~1..HEAD -- tests/test_to_actor.py` + +Added regressions for immediate caller-managed nursery cleanup, failed +cancel acknowledgement escalation, and child registration after a +latched nursery-wide teardown request. + +Verification: + +`pytest -q tests/test_to_actor.py tests/test_cancellation.py tests/test_spawning.py tests/discovery/test_multi_program.py` + +Result: `46 passed, 1 xfailed, 3 xpassed`. + +`pytest -q tests/test_to_actor.py --tpt-proto uds` + +Result: `13 passed`. + +`pytest -q tests/test_to_actor.py --spawn-backend mp_spawn --tpt-proto tcp` + +Result: `13 passed`. + +One broad verification run was mistakenly launched in parallel with +the UDS and `mp_spawn` actor suites. It timed out +`test_remote_error_from_caller_nursery`; the node passed immediately +in isolation and the complete broad selection then passed serially. +The failure was classified as concurrent test-session interference, +not accepted as a passing boundary result. + +Ruff, Python compilation, and `git diff --check` passed for the changed +boundary. Ruff's existing `_trio.py` F401 finding was reproduced at the +unmodified PR head and excluded from attribution to this patch. + +No source files were staged, committed, pushed, or used for review +replies. The previously approved top-level review was published before +the fix at reviewed head `4151b956`. diff --git a/tests/test_to_actor.py b/tests/test_to_actor.py index 40ade1c4..deb32bb6 100644 --- a/tests/test_to_actor.py +++ b/tests/test_to_actor.py @@ -97,9 +97,14 @@ async def test_spawn_from_caller_nursery( debug_mode: bool, ): ''' - Pass a caller-managed `an: ActorNursery` for the - spawn; the subactor is still one-shot reaped by the - time the call returns. + Pass a caller-managed `an: ActorNursery` for the spawn. + + Previously `to_actor.run()` treated an actor-runtime cancel ack + as process reaping, so the call returned while the child monitor + and its `ActorNursery._children` record remained alive until the + entire nursery exited. The assertion inside the still-open + nursery proves child-process joining and record removal now + complete before the one-shot call returns. ''' async with tractor.open_nursery() as an: @@ -108,6 +113,74 @@ async def test_spawn_from_caller_nursery( an=an, n=10, ) == 11 + assert not an._children + + +@tractor_test +async def test_cancel_ack_failure_hard_reaps_child( + monkeypatch: pytest.MonkeyPatch, + start_method: str, + debug_mode: bool, +): + ''' + Escalate a failed cancel acknowledgement and reap the child. + + `Portal.cancel_actor()` can return `False` when its transport is + already closed without confirming runtime cancellation. The old + one-shot path ignored that result, released the nursery-wide join + gate and then waited forever for a still-running process. This + test forces that exact result without cancelling the actor, caps + the call to detect the former hang and verifies the child monitor + removes its `ActorNursery._children` record before returning. + + ''' + async def cancel_without_ack( + portal: tractor.Portal, + timeout: float|None = None, + raise_on_timeout: bool = False, + ) -> bool: + assert raise_on_timeout + return False + + monkeypatch.setattr( + tractor.Portal, + 'cancel_actor', + cancel_without_ack, + ) + + async with tractor.open_nursery() as an: + with trio.fail_after(5): + assert await to_actor.run( + add_one, + an=an, + n=20, + ) == 21 + assert not an._children + + +def test_late_child_reap_registration_is_released(): + ''' + Preserve a nursery-wide reap request across child startup. + + A child monitor can checkpoint while connecting to its parent as + the surrounding `ActorNursery` begins teardown. Previously the + nursery signalled only already-registered child events, so a + monitor registering afterward waited forever. This models that + ordering by publishing the nursery-wide request first and proves + the later per-child event inherits its set state immediately. + + ''' + an = object.__new__(tractor.ActorNursery) + an._join_procs = trio.Event() + an._child_reap_requests = {} + an._child_reaped = {} + + an._join_procs.set() + reap_request, _ = an._register_child_reap( + ('late_child', 'uid'), + ) + + assert reap_request.is_set() @tractor_test diff --git a/tractor/runtime/_supervise.py b/tractor/runtime/_supervise.py index 9cb30941..366bb61d 100644 --- a/tractor/runtime/_supervise.py +++ b/tractor/runtime/_supervise.py @@ -90,7 +90,7 @@ async def _try_cancel_then_kill( Sends a graceful actor-runtime cancel-RPC via `Portal.cancel_actor(raise_on_timeout=True)`. If the bounded-wait expires before the peer ack's, `ActorTooSlowError` is raised and - we escalate via `proc.terminate()` (SIGTERM) per SC-discipline: + we escalate via `proc.kill()` per SC-discipline: graceful cancel-req -> bounded wait -> hard-kill @@ -102,11 +102,9 @@ async def _try_cancel_then_kill( the wider write-up. ''' - # XXX, do NOT escalate to `proc.terminate()` while ANY of - # the following are true — SIGTERM-ing a sub would tear - # down its sub-tree including any descendant proxying - # stdio to/from a REPL-locked actor, clobbering the user's - # debug session: + # XXX, delay hard-kill escalation while any debugger guard + # below is active. Killing the sub immediately would tear down + # its tree and clobber an actor proxying a REPL session: # # - `Lock.ctx_in_debug is not None`: most precise — some # actor in the tree is currently REPL-locked. Set in the @@ -122,7 +120,7 @@ async def _try_cancel_then_kill( # child. # # - `debug_mode_active`: this nursery has at least one - # child started with an explicit `debug_mode=` arg + # child started with an explicit `debug_mode=True` arg # (`ActorNursery._at_least_one_child_in_debug`). Catches # the case where root is NOT in debug-mode but a # nursery-direct child opted in. @@ -132,36 +130,57 @@ async def _try_cancel_then_kill( # mutated by per-child `debug_mode=True`). ORing covers # every flavor without false-positively skipping # legitimate hard-kill paths in non-debug trees. - if ( + debug_protected: bool = ( debug.Lock.ctx_in_debug is not None or _state._runtime_vars.get('_debug_mode', False) or debug_mode_active - ): - await portal.cancel_actor() - return + ) try: - await portal.cancel_actor(raise_on_timeout=True) + cancelled: bool = await portal.cancel_actor( + raise_on_timeout=not debug_protected, + ) + if not cancelled: + if debug_protected: + await debug.maybe_wait_for_debugger( + child_in_debug=( + debug_mode_active + or + debug.Lock.ctx_in_debug is not None + ), + header_msg=( + 'Delaying subproc hard-reap while ' + 'debugger locked..\n' + ), + ) + + peer_id: str = portal.channel.aid.reprol() + raise ActorTooSlowError( + f'Peer {peer_id} disconnected before ' + f'acknowledging its `Actor.cancel()` RPC' + ) + except ActorTooSlowError as too_slow: log.error( f'Cancel-ack TIMED OUT for sub-actor\n' f' uid: {subactor.aid.reprol()!r}\n' f' reason: {too_slow}\n' - f'-> escalating to `proc.terminate()` (hard-kill)\n' + f'-> escalating to `proc.kill()` (hard-reap)\n' ) # XXX, the `subint` backend stores an `int` interp-id in the - # `proc` slot (not a `Process`), so it has no `.terminate()`. + # `proc` slot (not a `Process`), so it has no `.kill()`. # Guard here so a cancel-ack timeout doesn't `AttributeError` # once that backend lands; its hard-kill path is a TODO. - if hasattr(proc, 'terminate'): - proc.terminate() + if hasattr(proc, 'kill'): + if proc.poll() is None: + proc.kill() else: log.error( f'Cannot hard-kill sub-actor — backend proc-handle ' f'{proc!r} ({type(proc).__name__!r}) has no ' - f'`.terminate()`!\n' + f'`.kill()`!\n' f' uid: {subactor.aid.reprol()!r}\n' f'TODO: per-backend cancel-escalation.\n' ) @@ -220,6 +239,14 @@ class ActorNursery: ] = {} self._join_procs = trio.Event() + self._child_reap_requests: dict[ + tuple[str, str], + trio.Event, + ] = {} + self._child_reaped: dict[ + tuple[str, str], + trio.Event, + ] = {} self._at_least_one_child_in_debug: bool = False self.errors = errors self._scope_error: BaseException|None = None @@ -284,6 +311,79 @@ class ActorNursery: # self._cancelled_caught ) + def _register_child_reap( + self, + uid: tuple[str, str], + ) -> tuple[trio.Event, trio.Event]: + ''' + Register a child monitor's process-reap events. + + ''' + reap_request = trio.Event() + reaped = trio.Event() + self._child_reap_requests[uid] = reap_request + self._child_reaped[uid] = reaped + if self._join_procs.is_set(): + reap_request.set() + return reap_request, reaped + + def _request_reap_all(self) -> None: + ''' + Release every child monitor into its process-join phase. + + ''' + self._join_procs.set() + for reap_request in tuple( + self._child_reap_requests.values() + ): + reap_request.set() + + def _mark_child_reaped( + self, + uid: tuple[str, str], + ) -> None: + ''' + Publish completed child-process teardown to its waiter. + + ''' + self._children.pop(uid, None) + self._child_reap_requests.pop(uid, None) + reaped: trio.Event|None = self._child_reaped.pop( + uid, + None, + ) + if reaped is not None: + reaped.set() + + async def _cancel_and_reap_child( + self, + portal: Portal, + ) -> None: + ''' + Cancel, join and unregister one nursery-owned child. + + ''' + uid: tuple[str, str] = portal.channel.aid.uid + child_entry = self._children.get(uid) + if child_entry is None: + return + + subactor, proc, _ = child_entry + reap_request: trio.Event = self._child_reap_requests[uid] + reaped: trio.Event = self._child_reaped[uid] + + with trio.CancelScope(shield=True): + try: + await _try_cancel_then_kill( + portal, + proc, + subactor, + self._at_least_one_child_in_debug, + ) + finally: + reap_request.set() + await reaped.wait() + async def start_actor( self, name: str, @@ -333,7 +433,7 @@ class ActorNursery: # allow setting debug policy per actor if debug_mode is not None: _rtv['_debug_mode'] = debug_mode - self._at_least_one_child_in_debug = True + self._at_least_one_child_in_debug |= debug_mode enable_modules = list(enable_modules or []) proc_kwargs = dict(proc_kwargs or {}) @@ -484,7 +584,7 @@ class ActorNursery: # TODO: impl a repr for spawn more compact # then `._children`.. - children: dict = self._children + children: tuple = tuple(self._children.values()) child_count: int = len(children) msg: str = f'Cancelling actor nursery with {child_count} children\n' @@ -503,7 +603,7 @@ class ActorNursery: subactor, proc, portal, - ) in children.values(): + ) in children: # TODO: are we ever even going to use this or # is the spawning backend responsible for such @@ -523,7 +623,9 @@ class ActorNursery: await event.wait() # channel/portal should now be up - _, _, portal = children[subactor.aid.uid] + _, _, portal = self._children[ + subactor.aid.uid + ] # XXX should be impossible to get here # unless method was called from within @@ -570,14 +672,14 @@ class ActorNursery: subactor, proc, portal, - ) in children.values(): + ) in children: log.warning(f"Hard killing process {proc}") proc.terminate() else: self._cancelled_caught # mark ourselves as having (tried to have) cancelled all subactors - self._join_procs.set() + self._request_reap_all() @acm @@ -639,7 +741,7 @@ async def _open_and_supervise_one_cancels_all_nursery( 'Waiting on subactors to complete:\n' f'>}} {len(an._children)}\n' ) - an._join_procs.set() + an._request_reap_all() except BaseException as _inner_err: inner_err = _inner_err @@ -658,7 +760,7 @@ async def _open_and_supervise_one_cancels_all_nursery( # if the caller's scope errored then we activate our # one-cancels-all supervisor strategy (don't # worry more are coming). - an._join_procs.set() + an._request_reap_all() # XXX NOTE XXX: hypothetically an error could # be raised and then a cancel signal shows up diff --git a/tractor/spawn/_mp.py b/tractor/spawn/_mp.py index d0c8af32..ee56b770 100644 --- a/tractor/spawn/_mp.py +++ b/tractor/spawn/_mp.py @@ -170,13 +170,16 @@ async def mp_proc( # any process we may have started. portal = Portal(chan) + reap_request, _ = actor_nursery._register_child_reap( + subactor.aid.uid, + ) actor_nursery._children[subactor.aid.uid] = (subactor, proc, portal) # unblock parent task task_status.started(portal) - # wait for ``ActorNursery`` block to signal that - # subprocesses can be waited upon. + # wait for this child or its `ActorNursery` to signal that + # the subprocess can be joined. # This is required to ensure synchronization # with user code that may want to manually await results # from nursery spawned sub-actors. We don't want the @@ -185,7 +188,7 @@ async def mp_proc( # nursery block closes do we allow subactor results to be # awaited and reported upwards to the supervisor. with trio.CancelScope(shield=True): - await actor_nursery._join_procs.wait() + await reap_request.wait() async with trio.open_nursery() as nursery: if portal in actor_nursery._cancel_after_result_on_exit: diff --git a/tractor/spawn/_spawn.py b/tractor/spawn/_spawn.py index c0218c30..1cee9637 100644 --- a/tractor/spawn/_spawn.py +++ b/tractor/spawn/_spawn.py @@ -446,18 +446,21 @@ async def new_proc( # mark the new actor with the global spawn method subactor._spawn_method = _spawn_method - await target( - name, - actor_nursery, - subactor, - errors, - bind_addrs, - parent_addr, - _runtime_vars, # run time vars - infect_asyncio=infect_asyncio, - task_status=task_status, - proc_kwargs=proc_kwargs - ) + try: + await target( + name, + actor_nursery, + subactor, + errors, + bind_addrs, + parent_addr, + _runtime_vars, # run time vars + infect_asyncio=infect_asyncio, + task_status=task_status, + proc_kwargs=proc_kwargs + ) + finally: + actor_nursery._mark_child_reaped(subactor.aid.uid) # NOTE: bottom-of-module to avoid a circular import since the diff --git a/tractor/spawn/_trio.py b/tractor/spawn/_trio.py index 7d47175a..a2f38b71 100644 --- a/tractor/spawn/_trio.py +++ b/tractor/spawn/_trio.py @@ -161,6 +161,9 @@ async def trio_proc( assert proc portal = Portal(chan) + reap_request, _ = actor_nursery._register_child_reap( + subactor.aid.uid, + ) actor_nursery._children[subactor.aid.uid] = ( subactor, proc, @@ -191,9 +194,10 @@ async def trio_proc( # resume caller at next checkpoint now that child is up task_status.started(portal) - # wait for ActorNursery.wait() to be called + # wait for this child or its `ActorNursery` to request + # process joining. with trio.CancelScope(shield=True): - await actor_nursery._join_procs.wait() + await reap_request.wait() async with trio.open_nursery() as nursery: if portal in actor_nursery._cancel_after_result_on_exit: diff --git a/tractor/to_actor/_api.py b/tractor/to_actor/_api.py index fc15193f..0cb4d60c 100644 --- a/tractor/to_actor/_api.py +++ b/tractor/to_actor/_api.py @@ -103,13 +103,10 @@ async def _invoke_in_subactor( **fn_kwargs, ) finally: - # one-shot semantics: the subactor's lifetime is - # bound to its lone task's completion; the - # cancel-req's bounded wait is shielded - # internally (see `Portal.cancel_actor()`) so - # this reap also runs when the caller's scope - # was itself cancelled. - await portal.cancel_actor() + # Cancel and join this child before returning. The nursery + # helper shields teardown, escalates a missed cancel ack and + # waits for the child monitor to remove its process record. + await an._cancel_and_reap_child(portal) async def run(