Skip to content

Bound the recursion in exception group split and subgroup - #8584

Merged
youknowone merged 1 commit into
RustPython:mainfrom
luantaraschi:exception-group-deep-recursion
Aug 22, 2026
Merged

Bound the recursion in exception group split and subgroup#8584
youknowone merged 1 commit into
RustPython:mainfrom
luantaraschi:exception-group-deep-recursion

Conversation

@luantaraschi

@luantaraschi luantaraschi commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Summary

BaseExceptionGroup.split and .subgroup recurse into nested groups through vm.call_method, which pushes no Python frame. sys.setrecursionlimit therefore never sees the nesting, and a group nested deeply enough runs the native stack out:

from test.support import exceeds_recursion_limit

e = TypeError(1)
for _ in range(exceeds_recursion_limit()):   # 150000
    e = ExceptionGroup("eg", [e])
e.split(TypeError)

CPython answers RecursionError. RustPython dumps core, and so does the test module that covers it:

0:00:00 load avg: 2.86 [1/1] test_exception_group
timeout: the monitored command dumped core

Lib/test/test_exception_group.py says as much in two skip reasons, TODO: RUSTPYTHON; Segfault, on test_deep_split and test_deep_subgroup. Both are unskipped here.

Fix

The two recursive vm.call_method calls now sit inside vm.with_recursion, which is the Py_EnterRecursiveCall counterpart already used for comparison, repr, __length_hint__ and __subclasscheck__. It measures the native stack rather than counting frames, which is the right budget for recursion that never reaches the evaluator.

Measurements

Deep case, one process each:

CPython 3.14 before after
split at depth 150000 RecursionError core dumped RecursionError
subgroup at depth 150000 RecursionError core dumped RecursionError

Shallow behaviour is untouched, checked case by case against CPython: split and subgroup on a flat group, on a nested one, with a predicate instead of a type, with no match at all, and at depth 100 where neither implementation is anywhere near the limit. Every one agrees.

test_exception_group ran three times after unskipping, 52 tests, SUCCESS each time, and test_except_star, test_traceback, test_asyncio.test_taskgroups, test_baseexception and test_exceptions are green too. Also cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher --exclude rustpython-capi (41 binaries, no failures), cargo test in crates/capi, cargo fmt --check, the clippy invocation from CI, and scripts/check_redundant_patches.py on the test file.

Summary by CodeRabbit

  • Bug Fixes
    • Added recursion protection when processing nested exception groups.
    • Prevented excessive recursion from causing native stack overflows while preserving existing subgroup and split behavior.

Both walk nested groups by calling themselves back through
vm.call_method, and neither pushes a Python frame, so the frame limit
never sees the nesting and the native stack runs out first. A group
nested past the recursion limit segfaults:

    e = TypeError(1)
    for _ in range(exceeds_recursion_limit()):
        e = ExceptionGroup("eg", [e])
    e.split(TypeError)      # dumps core

vm.with_recursion is the guard for exactly this, the same one comparison
and repr already use, and it turns the crash into the RecursionError
CPython raises. test_deep_split and test_deep_subgroup were skipped on
the segfault and now pass.

Assisted-by: Claude Code:claude-opus-5
@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: c139a7fb-c655-404c-9a59-f2cbf34d4cf0

📥 Commits

Reviewing files that changed from the base of the PR and between cc1e55e and 811b8e1.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_exception_group.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • crates/vm/src/exception_group.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

ExceptionGroup.subgroup and ExceptionGroup.split now wrap recursive nested-group calls with vm.with_recursion. The operations report their context and propagate recursion errors.

Changes

ExceptionGroup recursion protection

Layer / File(s) Summary
Guard recursive subgroup and split calls
crates/vm/src/exception_group.rs
subgroup and split now protect recursive nested-group calls with vm.with_recursion. Recursion errors propagate with operation context.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 811b8

The change bounds deep exception-group recursion and restores RecursionError behavior, with the reported tests and standard checks passing. No actionable merge-blocking risk remains beyond normal review.

Suggested reviewers: youknowone

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding recursion bounds to exception group split and subgroup operations.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[ ] test: cpython/Lib/test/test_exceptions.py (TODO: 22)
[ ] test: cpython/Lib/test/test_baseexception.py
[x] test: cpython/Lib/test/test_except_star.py (TODO: 1)
[ ] test: cpython/Lib/test/test_exception_group.py (TODO: 3)
[x] test: cpython/Lib/test/test_exception_hierarchy.py (TODO: 2)
[x] test: cpython/Lib/test/test_exception_variations.py

dependencies:

dependent tests: (no tests depend on exception)

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thank you!

@youknowone
youknowone enabled auto-merge (squash) August 22, 2026 23:34
@youknowone
youknowone merged commit bf7c173 into RustPython:main Aug 22, 2026
29 checks passed
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