Restore higher-scoped teardown when a teardown error rules out a re-run

[teddytennant]

Follow-up to #351, the first of the two leaks I mentioned there.

pytest_runtest_teardown takes the module, class and session finalizers off the setup stack when it expects a re-run. Whether the error a teardown raised is terminal is only worked out afterwards, in pytest_runtest_makereport, so that decision cannot see it. When --rerun-except or --only-rerun classifies that error as one not to re-run on, the protocol then declines to re-run, _restore_suspended_finalizers is never reached, and the fixture's teardown never runs again for the rest of the session:

@pytest.fixture(scope="module", autouse=True)
def module_fixture():
    yield
    print("module teardown")

@pytest.fixture
def broken_fixture():
    yield
    raise ValueError("teardown error")

@pytest.mark.flaky(reruns=2, rerun_except=["ValueError"])
def test_fail(broken_fixture):
    assert False

module teardown is not printed. As with #351 the damage is not local, since suspended_finalizers is a module-level global: run that file alongside another and the first module's fixture stays un-torn-down past the end of its own module.

Adding a term to the gate does not work here, because the error does not exist yet when the gate runs. So the hook becomes a wrapper. On the way in it decides exactly as before. On the way out it looks at the error the teardown phase actually raised, and if that error rules out a re-run it puts the finalizers back and calls teardown_exact. Running them there rather than just restoring them matters: a restore alone leaves a stale collector on the stack, and the next item's SetupState.setup asserts on that.

If a higher-scoped teardown then fails itself, its error is chained onto the one that got us there instead of replacing it. Replacing it would change what the re-run decision is made on, and the test would start re-running again.

Tests cover class, module and session scope, the cross-module case, and a higher-scoped teardown that fails while being run this way. Two are guards rather than reproductions: an error the filters do allow a re-run on still re-runs and still tears the module fixture down exactly once, and the plain --reruns paths are untouched.

The remaining leak, premature teardown when a call phase fails through subtests only, is a separate PR.

[icemac]

Reviewed against real repro cases (pytest 9.1.1) and the full suite with and without xdist + subtests (179 passed).

The first comment below is a regression against this PR's own goal, so it is the one that matters; the rest are on the new error-chaining path.

Also checked and clean: module/class fixture preservation when the next item shares the scope, terminal teardown error on the second attempt, and stack ordering after stack.update(suspended_finalizers). test_run_session_teardown_once_after_reruns fails under pytest 8.2, but identically on master — pre-existing, not from this PR.

— Comment created by Claude

[icemac]

This drops the report.outcome != "failed" guard, so non-failing teardown exceptions trigger a premature teardown + re-setup on every rerun.

_should_hard_fail_on_error only consults _is_terminal_error when report.outcome == "failed". This wrapper calls _is_terminal_error on the raw exception with no such guard, so the two disagree: a teardown exception that pytest does not report as a failure is terminal here, while pytest_runtest_makereport sets _terminal_errors["teardown"] = False and the protocol still reruns the test — after this hook has already restored and run the higher-scoped finalizers.

@pytest.fixture(scope="module", autouse=True)
def module_fixture():
    print("MODULE SETUP"); yield; print("MODULE TEARDOWN")

@pytest.fixture
def skipping_teardown():
    yield
    pytest.skip("skip in teardown")

@pytest.mark.flaky(reruns=2, only_rerun=["AssertionError"])
def test_fail(skipping_teardown):
    assert False

master: MODULE SETUP once, MODULE TEARDOWN once. This branch: setup and teardown three times — the exact leak class this series removes.

Exit and KeyboardInterrupt diverge the same way: pytest re-raises them out of the teardown hook (CallInfo.from_call(reraise=(Exit, KeyboardInterrupt))) and never turns them into a failed report.

— Comment created by Claude

[teddytennant]

Confirmed, and pytest.xfail() in a teardown goes the same way, since skipping.py turns that into a skipped report too. I could not write a guard here I trusted: the wrapper runs before the report exists, so any list of exception types in it is a copy of pytest's classification that will drift.

So the restore and the teardown_exact moved into pytest_runtest_makereport, where the report and _terminal_errors already exist, and _is_terminal_error is gone. One derivation instead of two, and the two cannot disagree by construction. Your repro and the xfail one both tear the module fixture down once again, same as master.

[icemac]

except BaseException swallows Exit and KeyboardInterrupt raised by the higher-scoped finalizers.

A module fixture whose teardown calls pytest.exit("EXIT FROM TEARDOWN"), reached through this path, is silently ignored: the session continues and the next module's test runs (1 failed, 1 passed, 1 error). Ctrl-C during that teardown is discarded the same way.

Catching Exception, or re-raising Exit/KeyboardInterrupt before the chaining, avoids this.

— Comment created by Claude

[teddytennant]

Fixed. The extra teardown runs inside CallInfo.from_call with the same reraise=(Exit, KeyboardInterrupt) pytest uses, so both get out and stop the session. For the record except Exception on its own would not have covered it either, Exit subclasses Exception.

[icemac]

Attaching the secondary error via __context__ loses it whenever the primary exception suppresses its context.

Assigning __context__ does not clear __suppress_context__ or __cause__, so if the original teardown exception was raised with from None (or from something) the chained error never reaches the report. With raise ValueError("teardown error") from None in the function-scoped fixture, the module fixture's RuntimeError("MODULE TEARDOWN ERROR") vanishes from the output entirely. It also clobbers any pre-existing __context__ on the primary exception.

Minor, related: because the later error is installed as the context of the earlier one, the chained report reads backwards — RuntimeError: module teardown error, then During handling of the above exception, another exception occurred:, then the ValueError that actually happened first.

— Comment created by Claude

[teddytennant]

Good catch, the __context__ assignment is gone. When both teardowns fail the two errors go into a BaseExceptionGroup("errors during test teardown") now, which is what pytest itself builds for several failing finalizers, so from None is no longer a factor and the order reads the way pytest's does.

[icemac]

Minor: this return True sits outside the if item in item.session._setupstate.stack: block, so a True return does not imply the docstring's "they were taken off the stack".

When the item is not on the stack, the caller still treats the finalizers as suspended and, on a terminal teardown error, runs _restore_suspended_finalizers (splicing in whatever stale globals are left from a previous item) plus a redundant teardown_exact.

I could not construct a case where this actually misbehaves today — the gate's not any(item._terminal_errors.values()) rules out the collector-setup-failure paths — so this is a contract mismatch rather than a live bug.

— Comment created by Claude

[teddytennant]

Tightened. The flag is set on the line that deletes from the stack now, so it is true only if something actually came off. Still no test for it, since I could not reach that state either.

[teddytennant]

Pushed as a new commit. It reads smaller than it sounds: pytest_runtest_teardown is back to master's plain hookimpl plus three lines recording what it held back, and pytest_runtest_makereport does the work once the real report exists.

The exception group needs from exceptiongroup import BaseExceptionGroup on 3.10. pytest already requires exceptiongroup there and _pytest/runner.py imports it the same way, so I left it transitive rather than touching pyproject.toml in a bugfix. Say the word if you would rather have it declared.

183 passed on 3.13 with pytest 9.1.1, same under -n 2, and 175 passed 8 skipped on 3.10 with pytest 8.2.1. Four new tests, all of which fail on the previous commit.