check for nursery misnesting on task exit (#3307)

* check for nursery misnesting on task exit ... not working

* forcefully abort children of unclosed nurseries to avoid internalerror

* call Runner.task_exited on aborted tasks to get errors for aborted tasks and handle nested scenarios (needs test)

* new approach

* suppress Cancelled if a CancelScope is abandoned by misnesting

* add example of losing exception from abandoned task

* add newsfragment

Author: jakkdl

newsfragments/3307.misc.rst

@@ -0,0 +1 @@
+When misnesting nurseries you now get a helpful :exc:`RuntimeError` instead of a catastrophic :exc:`TrioInternalError`.

src/trio/_core/_run.py

@@ -678,6 +678,9 @@ def _close(self, exc: BaseException | None) -> BaseException | None:
             exc is not None
             and self._cancel_status.effectively_cancelled
             and not self._cancel_status.parent_cancellation_is_visible_to_us
+        ) or (
+            scope_task._cancel_status is not self._cancel_status
+            and self._cancel_status.abandoned_by_misnesting
         ):
             if isinstance(exc, Cancelled):
                 self.cancelled_caught = True
@@ -1261,6 +1264,9 @@ def _child_finished(
         outcome: Outcome[object],
     ) -> None:
         self._children.remove(task)
+        if self._closed and not hasattr(self, "_pending_excs"):
+            # We're abandoned by misnested nurseries, the result of the task is lost.
+            return
         if isinstance(outcome, Error):
             self._add_exc(
                 outcome.error,
@@ -1321,7 +1327,7 @@ def aborted(raise_cancel: _core.RaiseCancelT) -> Abort:
                 self._add_exc(exc, reason=None)
 
         popped = self._parent_task._child_nurseries.pop()
-        assert popped is self
+        assert popped is self, "Nursery misnesting detected!"
         if self._pending_excs:
             try:
                 if not self._strict_exception_groups and len(self._pending_excs) == 1:
@@ -2007,6 +2013,17 @@ async def python_wrapper(orig_coro: Awaitable[RetT]) -> RetT:
         return task
 
     def task_exited(self, task: Task, outcome: Outcome[object]) -> None:
+        if task._child_nurseries:
+            for nursery in task._child_nurseries:
+                nursery.cancel_scope._cancel(
+                    CancelReason(
+                        source="nursery",
+                        reason="Parent Task exited prematurely, abandoning this nursery without exiting it properly.",
+                        source_task=repr(task),
+                    )
+                )
+                nursery._closed = True
+
         # break parking lots associated with the exiting task
         if task in GLOBAL_PARKING_LOT_BREAKER:
             for lot in GLOBAL_PARKING_LOT_BREAKER[task]:
@@ -2017,7 +2034,8 @@ def task_exited(self, task: Task, outcome: Outcome[object]) -> None:
             task._cancel_status is not None
             and task._cancel_status.abandoned_by_misnesting
             and task._cancel_status.parent is None
-        ):
+        ) or task._child_nurseries:
+            reason = "Nursery" if task._child_nurseries else "Cancel scope"
             # The cancel scope surrounding this task's nursery was closed
             # before the task exited. Force the task to exit with an error,
             # since the error might not have been caught elsewhere. See the
@@ -2026,7 +2044,7 @@ def task_exited(self, task: Task, outcome: Outcome[object]) -> None:
                 # Raise this, rather than just constructing it, to get a
                 # traceback frame included
                 raise RuntimeError(
-                    "Cancel scope stack corrupted: cancel scope surrounding "
+                    f"{reason} stack corrupted: {reason} surrounding "
                     f"{task!r} was closed before the task exited\n{MISNESTING_ADVICE}",
                 )
             except RuntimeError as new_exc:

src/trio/_core/_tests/test_run.py

@@ -8,7 +8,13 @@
 import time
 import types
 import weakref
-from contextlib import ExitStack, contextmanager, suppress
+from contextlib import (
+    AsyncExitStack,
+    ExitStack,
+    asynccontextmanager,
+    contextmanager,
+    suppress,
+)
 from math import inf, nan
 from typing import TYPE_CHECKING, NoReturn, TypeVar
 from unittest import mock
@@ -761,7 +767,7 @@ async def enter_scope() -> None:
     assert scope.cancel_called  # never become un-cancelled
 
 
-async def test_cancel_scope_misnesting() -> None:
+async def test_cancel_scope_misnesting_1() -> None:
     outer = _core.CancelScope()
     inner = _core.CancelScope()
     with ExitStack() as stack:
@@ -771,6 +777,8 @@ async def test_cancel_scope_misnesting() -> None:
                 stack.close()
         # No further error is raised when exiting the inner context
 
+
+async def test_cancel_scope_misnesting_2() -> None:
     # If there are other tasks inside the abandoned part of the cancel tree,
     # they get cancelled when the misnesting is detected
     async def task1() -> None:
@@ -828,6 +836,8 @@ def no_context(exc: RuntimeError) -> bool:
     )
     assert group.matches(exc_info.value.__context__)
 
+
+async def test_cancel_scope_misnesting_3() -> None:
     # Trying to exit a cancel scope from an unrelated task raises an error
     # without affecting any state
     async def task3(task_status: _core.TaskStatus[_core.CancelScope]) -> None:
@@ -844,6 +854,130 @@ async def task3(task_status: _core.TaskStatus[_core.CancelScope]) -> None:
         scope.cancel()
 
 
+# helper to check we're not outputting overly verbose tracebacks
+def no_cause_or_context(e: BaseException) -> bool:
+    return e.__cause__ is None and e.__context__ is None
+
+
+async def test_nursery_misnest() -> None:
+    # See https://github.com/python-trio/trio/issues/3298
+    async def inner_func() -> None:
+        inner_nursery = await inner_cm.__aenter__()
+        inner_nursery.start_soon(sleep, 1)
+
+    with pytest.RaisesGroup(
+        pytest.RaisesExc(
+            RuntimeError, match="Nursery stack corrupted", check=no_cause_or_context
+        ),
+        check=no_cause_or_context,
+    ):
+        async with _core.open_nursery() as outer_nursery:
+            inner_cm = _core.open_nursery()
+            outer_nursery.start_soon(inner_func)
+
+
+def test_nursery_nested_child_misnest() -> None:
+    # Note that this example does *not* raise an exception group.
+    async def main() -> None:
+        async with _core.open_nursery():
+            inner_cm = _core.open_nursery()
+            await inner_cm.__aenter__()
+
+    with pytest.raises(RuntimeError, match="Nursery stack corrupted") as excinfo:
+        _core.run(main)
+    assert excinfo.value.__cause__ is None
+    # This AssertionError is kind of redundant, but I don't think we want to remove
+    # the assertion and don't think we care enough to suppress it in this specific case.
+    assert pytest.RaisesExc(
+        AssertionError, match="^Nursery misnesting detected!$"
+    ).matches(excinfo.value.__context__)
+    assert excinfo.value.__context__.__cause__ is None
+    assert excinfo.value.__context__.__context__ is None
+
+
+async def test_asyncexitstack_nursery_misnest() -> None:
+    # This example is trickier than the above ones, and is the one that requires
+    # special logic of abandoned nurseries to avoid nasty internal errors that masks
+    # the RuntimeError.
+    @asynccontextmanager
+    async def asynccontextmanager_that_creates_a_nursery_internally() -> (
+        AsyncGenerator[None]
+    ):
+        async with _core.open_nursery() as nursery:
+            await nursery.start(started_sleeper)
+            nursery.start_soon(unstarted_task)
+            yield
+
+    async def started_sleeper(task_status: _core.TaskStatus[None]) -> None:
+        task_status.started()
+        await sleep_forever()
+
+    async def unstarted_task() -> None:
+        await _core.checkpoint()
+
+    with pytest.RaisesGroup(
+        pytest.RaisesGroup(
+            pytest.RaisesExc(
+                RuntimeError, match="Nursery stack corrupted", check=no_cause_or_context
+            ),
+            check=no_cause_or_context,
+        ),
+        check=no_cause_or_context,
+    ):
+        async with AsyncExitStack() as stack, _core.open_nursery() as nursery:
+            # The asynccontextmanager is going to create a nursery that outlives this nursery!
+            nursery.start_soon(
+                stack.enter_async_context,
+                asynccontextmanager_that_creates_a_nursery_internally(),
+            )
+
+
+def test_asyncexitstack_nursery_misnest_cleanup() -> None:
+    # We guarantee that abandoned tasks get to do cleanup *eventually*, but exceptions
+    # are lost. With more effort it's possible we could reschedule child tasks to exit
+    # promptly.
+    finally_entered = []
+
+    async def main() -> None:
+        async def unstarted_task() -> None:
+            try:
+                await _core.checkpoint()
+            finally:
+                finally_entered.append(True)
+                raise ValueError("this exception is lost")
+
+        # rest of main() is ~identical to the above test
+        @asynccontextmanager
+        async def asynccontextmanager_that_creates_a_nursery_internally() -> (
+            AsyncGenerator[None]
+        ):
+            async with _core.open_nursery() as nursery:
+                nursery.start_soon(unstarted_task)
+                yield
+
+        with pytest.RaisesGroup(
+            pytest.RaisesGroup(
+                pytest.RaisesExc(
+                    RuntimeError,
+                    match="Nursery stack corrupted",
+                    check=no_cause_or_context,
+                ),
+                check=no_cause_or_context,
+            ),
+            check=no_cause_or_context,
+        ):
+            async with AsyncExitStack() as stack, _core.open_nursery() as nursery:
+                # The asynccontextmanager is going to create a nursery that outlives this nursery!
+                nursery.start_soon(
+                    stack.enter_async_context,
+                    asynccontextmanager_that_creates_a_nursery_internally(),
+                )
+        assert not finally_entered  # abandoned task still hasn't been cleaned up
+
+    _core.run(main)
+    assert finally_entered  # now it has
+
+
 @slow
 async def test_timekeeping() -> None:
     # probably a good idea to use a real clock for *one* test anyway...

test-requirements.in

@@ -1,5 +1,5 @@
 # For tests
-pytest >= 5.0         # for faulthandler in core
+pytest >= 8.4         # for pytest.RaisesGroup
 coverage >= 7.2.5
 async_generator >= 1.9
 pyright