Fix regressions introduced when hiding Nursery.start implementation-detail nursery

[gschaffner]

proposed alternative fix and tests for #2611. fixes #2844.

note that these tests are parametrized:

For the tests, there are currently enough separate cases that I can imagine missing that something is backwards. Could we write something parameterized, such that it obviously does each possible set of operations, and then the asserts only depend on whether we're in strict mode? (I expect this to be harder to read individually, but the confidence in consistency is what I'm after) --- #2826 (comment)

[codecov]

Codecov Report

Merging #2845 (588558c) into master (6c50dc2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2845   +/-   ##
=======================================
  Coverage   99.18%   99.18%           
=======================================
  Files         115      115           
  Lines       17594    17596    +2     
  Branches     3142     3143    +1     
=======================================
+ Hits        17451    17453    +2     
  Misses         99       99           
  Partials       44       44           

Files	Coverage Δ
trio/_core/_run.py	100.00% <100.00%> (ø)
trio/_core/_tests/test_run.py	100.00% <100.00%> (ø)

[gschaffner]

note: if having start internally depend on strict_exception_groups=False is unwanted, an alternative patch could be

click to expand

diff --git a/trio/_core/_run.py b/trio/_core/_run.py
index e07d56d6..c2928c87 100644
--- a/trio/_core/_run.py
+++ b/trio/_core/_run.py
@@ -1103,13 +1103,6 @@ class Nursery(metaclass=NoPublicConstructor):

         popped = self._parent_task._child_nurseries.pop()
         assert popped is self
-
-        # don't unnecessarily wrap an exceptiongroup in another exceptiongroup
-        # see https://github.com/python-trio/trio/issues/2611
-        if len(self._pending_excs) == 1 and isinstance(
-            self._pending_excs[0], BaseExceptionGroup
-        ):
-            return self._pending_excs[0]
         if self._pending_excs:
             try:
                 return MultiError(
@@ -1218,15 +1211,25 @@ class Nursery(metaclass=NoPublicConstructor):
             raise RuntimeError("Nursery is closed to new arrivals")
         try:
             self._pending_starts += 1
-            async with open_nursery() as old_nursery:
-                task_status: _TaskStatus[StatusT] = _TaskStatus(old_nursery, self)
-                thunk = functools.partial(async_fn, task_status=task_status)
-                task = GLOBAL_RUN_CONTEXT.runner.spawn_impl(
-                    thunk, args, old_nursery, name
-                )
-                task._eventual_parent_nursery = self
-                # Wait for either TaskStatus.started or an exception to
-                # cancel this nursery:
+            try:
+                async with open_nursery(strict_exception_groups=True) as old_nursery:
+                    task_status: _TaskStatus[StatusT] = _TaskStatus(old_nursery, self)
+                    thunk = functools.partial(async_fn, task_status=task_status)
+                    task = GLOBAL_RUN_CONTEXT.runner.spawn_impl(
+                        thunk, args, old_nursery, name
+                    )
+                    task._eventual_parent_nursery = self
+                    # Wait for either TaskStatus.started or an exception to
+                    # cancel this nursery:
+            except BaseExceptionGroup as nursery_exc_grp:
+                if len(nursery_exc_grp.exceptions) == 1:
+                    # Hide the implementation-detail nursery; see #2611.
+                    raise nursery_exc_grp.exceptions[0] from None
+                else:
+                    # Unfortunately multiple tasks can be in the nursery if they
+                    # were injected via current_task().parent_nursery; see
+                    # #1599.
+                    raise
             # If we get here, then the child either got reparented or exited
             # normally. The complicated logic is all in TaskStatus.started().
             # (Any exceptions propagate directly out of the above.)

[jakkdl]

This looks good to me, and the fix is definitely cleaner in attacking the actual root of the problem.

The test is quite dense and hard to read, one could maybe add some more comments, but I think I managed to grok it in the end.

Thanks for fixing my mistake!

[jakkdl]

I think it's also good to mention in particular that this led to double-wrapped exceptions if used together with trio.Nursery.start with strict_exception_groups set to True or None.

[gschaffner]

hmm, i think i'm struggling to phrase it clearly without making the newsfragment quite long. i had changed the newsfragment to avoid mentioning double-wrapping because i felt it would be clearer to focus on what was broken (start doing wrapping) and not go into how that could interact with other things working correctly (open_nursery(strict=True).aexit doing wrapping).

i've just changed part of the newsfragment to make clear that the thing that was doing the bad wrapping (and the only thing that has changed) was start—not nursery exit. what do you think of this new phrasing?

[gschaffner]

currently my attempt at mentioning that this could lead to double-wrapped exceptions is

diff --git a/newsfragments/2611.bugfix.rst b/newsfragments/2611.bugfix.rst
index a39faa2d..b259210a 100644
--- a/newsfragments/2611.bugfix.rst
+++ b/newsfragments/2611.bugfix.rst
@@ -2,4 +2,11 @@ When a starting function raises before calling :func:`trio.TaskStatus.started`,
 :func:`trio.Nursery.start` will no longer wrap the exception in an undocumented
 :exc:`ExceptionGroup`. Previously, :func:`trio.Nursery.start` would incorrectly
 raise an :exc:`ExceptionGroup` containing it when using ``trio.run(...,
-strict_exception_groups=True)``.
+strict_exception_groups=True)``. (Note that the behavior of nursery exit has not
+changed, i.e. a nursery that sets/inherits `strict_exception_groups=True` will
+still wrap all exception(s) in an :exc:`ExceptionGroup`. The difference is that
+:func:`trio.Nursery.start` will now raise the pre-``started()`` exception
+instead of raising an :exc:`ExceptionGroup` containing it, i.e. the
+pre-``started()`` exception will no longer get incorrectly double-wrapped by
+both :func:`trio.Nursery.start` (incorrectly) and the strict nursery exit
+(correctly).)

but i am not super happy about its phrasing/length

[Zac-HD]

This looks great to me - thanks so much @gschaffner!