Repository: python-trio/trio

Issue:
raise_single_exception_from_group causes SystemExit to have no exit status
`raise_single_exception_from_group` was introduced as part of https://github.com/python-trio/trio/pull/3240 and https://github.com/python-trio/trio/pull/3197, but handles `KeyboardInterrupt` and `SystemExit` by wrapping the original exception in an exception of the same type:

```python
# immediately bail out if there's any KI or SystemExit
for e in eg.exceptions:
    if isinstance(e, (KeyboardInterrupt, SystemExit)):
        raise type(e) from eg
```

Notably, this causes the resulting `SystemExit` to have no exit status, so `system.exit(1)` (or really any other exit status) does not properly set the exit status. I do not see a good reason why the exception should not be raised directly:

```python
# immediately bail out if there's any KI or SystemExit
for e in eg.exceptions:
    if isinstance(e, (KeyboardInterrupt, SystemExit)):
        raise e from eg
```

Was there a reason for this handling?

Discussion hints:
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/python-trio/trio/pull/3280?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 99.98443%. Comparing base [(`efd785a`)](https://app.codecov.io/gh/python-trio/trio/commit/efd785a20721707b52a6e2289a65e25722b30c96?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio) to head [(`669e30e`)](https://app.codecov.io/gh/python-trio/trio/commit/669e30ee0c7843c2a4d1fb9ca26bd45881f408de?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio).

:x: Your project check has failed because the head coverage (99.98443%) is below the target coverage (100.00000%). You can increase the head coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage.

<details><summary>Additional details and impacted files</summary>


```diff
@@                 Coverage Diff                  @@
##                 main       #3280         +/-   ##
====================================================
- Coverage   100.00000%   99.98443%   -0.01557%     
====================================================
  Files             127         127                 
  Lines           19257       19262          +5     
  Branches         1301        1302          +1     
====================================================
+ Hits            19257       19259          +2     
- Misses              0           1          +1     
- Partials            0           2          +2     
```

| [Files with missing lines](https://app.codecov.io/gh/python-trio/trio/pull/3280?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio) | Coverage Δ | |
|---|---|---|
| [src/trio/\_tests/test\_util.py](https://app.codecov.io/gh/python-trio/trio/pull/3280?src=pr&el=tree&filepath=src%2Ftrio%2F_tests%2Ftest_util.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vX3Rlc3RzL3Rlc3RfdXRpbC5weQ==) | `100.00000% <100.00000%> (ø)` | |
| [src/trio/\_util.py](https://app.codecov.io/gh/python-trio/trio/pull/3280?src=pr&el=tree&filepath=src%2Ftrio%2F_util.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio#diff-c3JjL3RyaW8vX3V0aWwucHk=) | `100.00000% <100.00000%> (ø)` | |

... and [1 file with indirect coverage changes](https://app.codecov.io/gh/python-trio/trio/pull/3280/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=python-trio)

</details>
<details><summary> :rocket: New features to boost your workflow: </summary>

- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
CheViana: > Thanks for doing this!

you're welcome 

please re-review @A5rocks , fixed requested things

failed tests on py3.13 seem unrelated to my changes, some typing mismatch in some other files
A5rocks: > failed tests on py3.13 seem unrelated to my changes, some typing mismatch in some other files

It may have to do with the pytest version bump. I can't find anything named `RaisesContext` now so... maybe they removed it?

I still think we can avoid having to increase the minimum required pytest version (though if you want, you can bump in `test-requirements.txt`) and I actually feel somewhat conflicted about whether we should preserve any `SystemExit#code` mutations (which don't get reflected in `e.args`) now that I learned that exists.

I don't really know what review status to mark a review comment as though, so this is just a regular comment.
CheViana: > > failed tests on py3.13 seem unrelated to my changes, some typing mismatch in some other files
> 
> It may have to do with the pytest version bump. I can't find anything named `RaisesContext` now so... maybe they removed it? (EDIT: to fix it, replace `RaisesContext` with `pytest.RaisesExc`)
> 
> I still think we can avoid having to increase the minimum required pytest version (though if you want, you can bump in `test-requirements.txt`) and I actually feel somewhat conflicted about whether we should preserve any `SystemExit#code` mutations (which don't get reflected in `e.args`) now that I learned that exists.
> 
> I don't really know what review status to mark a review comment as though, so this is just a regular comment.


ok fixed it to work without pytest upgrade, as you suggested, thanks for comments! 
also updated error code handling, it's now just `type(e)(*e.args)` and seems to work ok
CheViana: > This looks good to me! As stated before I'm conflicted about whether to handle mutation of `SystemExit`'s `code` attribute, but I'm fine if we don't. Users probably don't do that?

I don't think one's supposed to instantiate SystemExit class directly at all, or to mutate their fields, one is supposed to use sys.exit. However for the purposes of, pardon, a bit weird things happening in `raise_single_exception_from_group` (presumably for backward compat 3.9?) , that's probably a change for better to reraise not empty exception of same type, but exception with same code/message as original

I suppose this PR resolves reporter's issue of missing error code so that's a small change for good

don't think I can merge it though so maybe somebody else can merge?
A5rocks: > don't think I can merge it though so maybe somebody else can merge?

Yeah let's make sure others agree (and they can merge it).
A5rocks: @seowalex I assume this works for you?
A5rocks: Well if there's any issues, we can make a follow up PR.

Solution patch:
diff --git a/newsfragments/3275.bugfix.rst b/newsfragments/3275.bugfix.rst
new file mode 100644
index 00000000..b418b26d
--- /dev/null
+++ b/newsfragments/3275.bugfix.rst
@@ -0,0 +1,1 @@
+Handle unwrapping SystemExit/KeyboardInterrupt exception gracefully in utility function ``raise_single_exception_from_group`` that reraises last exception from group.
diff --git a/src/trio/_util.py b/src/trio/_util.py
index 106423e2..54d324ca 100644
--- a/src/trio/_util.py
+++ b/src/trio/_util.py
@@ -398,7 +398,7 @@ def raise_single_exception_from_group(
     # immediately bail out if there's any KI or SystemExit
     for e in eg.exceptions:
         if isinstance(e, (KeyboardInterrupt, SystemExit)):
-            raise type(e) from eg
+            raise type(e)(*e.args) from eg
 
     cancelled_exception: trio.Cancelled | None = None
     noncancelled_exception: BaseException | None = None


Test patch:
diff --git a/src/trio/_tests/test_util.py b/src/trio/_tests/test_util.py
index 4182ba5d..3a57b9cf 100644
--- a/src/trio/_tests/test_util.py
+++ b/src/trio/_tests/test_util.py
@@ -323,25 +323,32 @@ async def test_raise_single_exception_from_group() -> None:
         [
             ValueError("foo"),
             ValueError("bar"),
-            KeyboardInterrupt("this exc doesn't get reraised"),
+            KeyboardInterrupt("preserve error msg"),
         ],
     )
-    with pytest.raises(KeyboardInterrupt, match=r"^$") as excinfo:
+    with pytest.raises(
+        KeyboardInterrupt,
+        match=r"^preserve error msg$",
+    ) as excinfo:
         raise_single_exception_from_group(eg_ki)
+
     assert excinfo.value.__cause__ is eg_ki
     assert excinfo.value.__context__ is None
 
-    # and same for SystemExit
+    # and same for SystemExit but verify code too
     systemexit_ki = BaseExceptionGroup(
         "",
         [
             ValueError("foo"),
             ValueError("bar"),
-            SystemExit("this exc doesn't get reraised"),
+            SystemExit(2),
         ],
     )
-    with pytest.raises(SystemExit, match=r"^$") as excinfo:
+
+    with pytest.raises(SystemExit) as excinfo:
         raise_single_exception_from_group(systemexit_ki)
+
+    assert excinfo.value.code == 2
     assert excinfo.value.__cause__ is systemexit_ki
     assert excinfo.value.__context__ is None