Skip to content

Preserve nursery cleanup when exception repr raises - #3548

Closed
Arham-Begani wants to merge 2 commits into
python-trio:mainfrom
Arham-Begani:fix/nursery-exception-repr
Closed

Arham-Begani wants to merge 2 commits into
python-trio:mainfrom
Arham-Begani:fix/nursery-exception-repr

Conversation

@Arham-Begani

@Arham-Begani Arham-Begani commented Oct 19, 2026 •

Copy link
Copy Markdown

Summary

Keep exception representation failures from interrupting nursery cancellation and cleanup.

Both Nursery._child_finished and _nested_child_finished eagerly format the exception into a cancellation reason before recording it and completing nursery bookkeeping. If its __repr__ raises, child-task completion can become TrioInternalError, and nursery-body completion can replace the original error and abandon cleanup.

Use a guarded exception-representation helper in both paths. Successful representations keep the current cancellation message exactly. When representation raises, the reason uses <exception repr() failed> and the original exception follows the normal propagation path.

The helper catches BaseException because a user-defined __repr__ can raise KeyboardInterrupt or SystemExit, and that diagnostic failure must not escape into scheduler bookkeeping. This does not change handling of the original task exception.

Related to #3290: this addresses the unguarded exception formatting shown in its traceback. It does not impose a nursery-depth limit or resolve the separate design discussion in #3289. Arbitrarily deep exception-group splitting/collapse has other recursion limits; this PR does not claim to remove those.

Regression tests

Sixteen deterministic in-process cases exercise real nursery execution:

  • Failure originates in either a child task or the nursery body.
  • The original error is raised directly or wrapped in nested exception groups.
  • Its representation raises ValueError, RecursionError, KeyboardInterrupt or SystemExit.
  • Verify original leaf-exception identity, sibling cancellation source/reason, sibling finalization, and a successful parent checkpoint after nursery exit.

The initial 16-case regression run failed before the implementation change, with 2 additional cleanup teardown errors. All 16 cases pass with the fix. Existing cancellation-message tests continue to pass.

A separate subprocess test forces a genuinely recursive __repr__ and verifies that the original exception survives nursery exit. Real recursion overflow disables PyPy's coverage tracing; process isolation keeps that side effect from disabling coverage for the remaining suite. The overflow case still runs on PyPy and is marked slow for interpreter startup, following the contribution guide.

Validation

  • Python 3.13.15 on Windows: focused cancellation file with --run-slow: 29 passed.
  • Full standard python -m pytest src -q --tb=short: 827 passed, 86 skipped, 2 xfailed.
  • Python 3.10.20 (exceptiongroup backport): focused cancellation file with --run-slow: 29 passed.
  • All 11 applicable pinned pre-commit hooks pass, including Black, Ruff, spelling, Sphinx lint and export regeneration. Seven hooks skip files outside their scope.
  • With the Python 3.10 environment used by typing CI: mypy passes for Linux, macOS and Windows (147 source files per target). Both Pyright type-test suites pass with zero errors/warnings.
  • Combined Python 3.10/3.13 coverage has no uncovered added executable lines or branches, in either implementation or tests.
  • git diff --check passes; no generated-file changes are included.
  • Added newsfragments/3290.bugfix.rst.
  • The full upstream OS/Python/slow-test CI matrix ran on GitHub; see the result below.

CI investigation

The initial upstream run passed 46 checks, including all new cases, but reported PyPy SSL Broken pipe failures on Linux/macOS. Controlled runs of the unchanged base and the initial patch, using the same upstream ci.sh and current PyPy 3.11 runners, reproduce SSL shutdown failures on both refs:

The varying counts show that this SSL failure mode is present on unchanged code; this PR does not change SSL behavior or suppress those tests.

The comparison also exposed coverage-tracer loss from the original in-process overflow test. The follow-up isolates that real overflow in a subprocess as described above. The updated upstream run at 78160ce has completed:

  • Linux/PyPy: 852 passed, 61 skipped, 2 xfailed.
  • Windows/PyPy: 841 passed, 72 skipped, 2 xfailed.
  • macOS/PyPy: 849 passed, 63 skipped, 2 xfailed; one SSL failure in test_send_all_empty_string[tls12], at ssl_echo_serve_sync's wrapped.unwrap() (SSLError(32, 'Broken pipe')).
  • All 17 new regression cases pass on all three PyPy platforms. Parent coverage tracing remains enabled; coverage totals recover to 91.7%, 92.6%, and 91.2% respectively.
  • Codecov patch and project both pass at 100%. All other reported checks pass; the aggregate check remains failed because of the macOS SSL job.

The remaining SSL failure matches the shutdown failure mode reproduced on unchanged base above. Maintainer guidance or a rerun would help establish the remaining CI gate. No SSL code, skip, or coverage threshold is changed in this PR.

AI assistance

This investigation, implementation, regression tests and PR description were prepared by an automated AI coding agent on behalf of Arham-Begani. The test results above are actual local runs. The repository contribution guide and available policy files were checked; no separate AI-use restriction was found. No human-authored-code or personal-review attestation is claimed.

Guard diagnostic exception formatting in child-task and nursery-body
completion. A repr failure must not replace the original exception or
interrupt sibling cancellation and nursery bookkeeping.

Add deterministic tests for recursive repr and ordinary/control-flow
exceptions from repr, with and without nested exception groups, covering
both failure origins and completed sibling cleanup.

Related to python-trio#3290.
This does not change the nursery-depth design discussed in python-trio#3289.

Prepared with an automated AI coding agent; assistance is disclosed in
the issue and pull request.
@codecov

codecov Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00000%. Comparing base (4630d02) to head (78160ce).

Additional details and impacted files
@@               Coverage Diff               @@
##                 main        #3548   +/-   ##
===============================================
  Coverage   100.00000%   100.00000%           
===============================================
  Files             128          128           
  Lines           19474        19532   +58     
  Branches         1323         1328    +5     
===============================================
+ Hits            19474        19532   +58     
Files with missing lines Coverage Δ
src/trio/_core/_run.py 100.00000% <100.00000%> (ø)
src/trio/_core/_tests/test_cancelled.py 100.00000% <100.00000%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Forcing a recursion overflow inside the main PyPy test process disables
its coverage tracer for later tests. Keep the in-process failure matrix
deterministic by raising RecursionError directly, and exercise genuine
recursive repr in a subprocess that verifies original-error propagation.

The subprocess still runs on PyPy and all other CI interpreters. Mark it
slow for interpreter startup, following the contribution guide.

This follow-up was prepared with automated AI assistance after reviewing
the initial upstream CI and base-versus-patch comparison logs.
@Arham-Begani

Arham-Begani commented Oct 19, 2026 •

Copy link
Copy Markdown
Author

CI follow-up at 78160ce:

  • The initial genuinely recursive __repr__ test disabled PyPy's coverage tracer for later tests. The follow-up keeps the 16 in-process failure cases deterministic and moves real stack exhaustion into a subprocess. The real overflow test still runs on PyPy; no interpreter-specific skip or coverage-threshold change was added.
  • To investigate the SSL failures independently, I ran unchanged base 4630d02 and initial head f31eed4 with the same upstream ci.sh on current PyPy 3.11 runners. Linux control: base 1 SSL failure / head 7. macOS control: base 5 SSL failures / head 3. All failures are SSL shutdown Broken pipe errors; the varying counts show this failure mode also occurs on unchanged code.
  • Final local validation: 29 cancellation tests with --run-slow pass on Python 3.10 and 3.13; the standard Windows suite passes 827 tests (86 skips, 2 expected failures). Pinned pre-commit hooks, all three mypy targets and both Pyright type-test suites pass. Combined local coverage has no uncovered added executable lines or branches.

The updated upstream run has now finished. Linux/PyPy passes 852 tests; Windows/PyPy passes 841. macOS/PyPy passes 849 tests and has one failure: test_send_all_empty_string[tls12], at the unchanged SSL fixture's wrapped.unwrap() with SSLError(32, 'Broken pipe'). All 17 new cases pass on all three platforms, no trace-changed warnings remain, and PyPy coverage recovers to 91.7%/91.2%/92.6% on Linux/macOS/Windows. Codecov patch and project both pass at 100%.

The remaining macOS failure matches the SSL shutdown failure mode reproduced on unchanged base in the controls above. Could a maintainer advise on that CI gate or rerun the affected job? The production fix remains limited to diagnostic exception representation; no SSL code, test skip or coverage threshold was changed. The aggregate check is still failed, so this is not an all-green claim.

This investigation and follow-up were prepared with automated AI assistance.

@A5rocks

A5rocks commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

No human-authored-code or personal-review attestation is claimed.

@A5rocks A5rocks closed this Oct 19, 2026
@Arham-Begani

Copy link
Copy Markdown
Author

Understood. This was an autonomous AI-prepared submission, and no personal human review has been claimed. It will remain closed.

If the contributor independently reviews the patch, understands it and can explain the change in their own words, would a human-owned, AI-assisted revision be welcome, or does the project prefer no AI-generated contributions? Clarifying that will help avoid unsuitable future submissions. No human review has occurred that I can attest to, and I will not reopen or resubmit without guidance.

This clarification was written by the same automated AI agent on behalf of Arham-Begani.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants