Internal AssertionError on stale _finalizers when a pytest_fixture_setup hookimpl raises during setup of a parametrized argument #14800

Problem

If a pytest_fixture_setup hook implementation raises (including raising Skipped via pytest.skip()), the FixtureDef for the parametrized argument is left with a stale entry in self._finalizers. Every subsequent parameter for that same argument then fails during setup with an internal AssertionError at _pytest/fixtures.py:1221 instead of running.

The first parameter reports correctly. Only the ones after it break, so the damage is order-dependent and the reported failures point at the wrong parameters.

This is a regression in 9.1.0. 9.0.0 and 8.4.x are fine, and it still reproduces on main.

Minimal reproducer

No third-party plugins required.

conftest.py:

import pytest


@pytest.hookimpl(tryfirst=True)
def pytest_fixture_setup(fixturedef, request):
    """Resolve a fixture by name from inside pytest_fixture_setup, based on the
    param value. This is what pytest-lazy-fixtures does to implement lf()."""
    param = getattr(request, "param", None)
    if isinstance(param, str) and param.startswith("fixture:"):
        request.param = request.getfixturevalue(param[len("fixture:"):])

test_pure.py:

import pytest


@pytest.fixture
def skipping_base():
    pytest.skip("backend unavailable")


@pytest.fixture
def derived(skipping_base):
    return "derived"


@pytest.mark.parametrize("val", ["fixture:derived", "plain-1", "plain-2"])
def test_thing(val):
    assert isinstance(val, str)

Run pytest.

Expected (and what 8.4.2 / 9.0.0 do): 2 passed, 1 skipped.

Actual on 9.1.0, 9.1.1 and main: 1 skipped, 2 errors, both errors being

    # Register the pytest_fixture_post_finalizer as the first finalizer,
    # which is executed last.
>   assert not self._finalizers
           ^^^^^^^^^^^^^^^^^^^^
E   AssertionError

.../_pytest/fixtures.py:1221: AssertionError

pytest.skip() is not special here — replacing it with raise RuntimeError("boom")
produces the same internal AssertionError on the following parameters (the
first parameter then reports the RuntimeError correctly, as it should).

Version bisect

Cause

Two commits that both landed in 9.1.0 encode contradictory assumptions about when _finalizers may be populated.

96728d56 ("fix pytest_fixture_post_finalizer sometimes called multiple times", #5848) added an early return to FixtureDef.finish() (_pytest/fixtures.py:1146):

def finish(self, request: SubRequest) -> None:
    if self.cached_result is None:
        # Already finished. It is assumed that finalizers cannot be added in
        # this state.
        return

719f1ec9 ("improve handling of pytest_fixture_post_finalizer raising",
#14114) then made FixtureDef.execute() add a finalizer in exactly that state —
cached_result is still None at this point, because the hook that sets it has
not been called yet (_pytest/fixtures.py:1219):

        # Register the pytest_fixture_post_finalizer as the first finalizer,
        # which is executed last.
        assert not self._finalizers
        self.addfinalizer(
            lambda: request.node.ihook.pytest_fixture_post_finalizer(
                fixturedef=self, request=request
            )
        )

        ihook = request.node.ihook
        try:
            result: FixtureValue = ihook.pytest_fixture_setup(
                fixturedef=self, request=request
            )
        finally:
            # Schedule our finalizer, even if the setup failed.
            request.node.addfinalizer(finalizer)

So the stated assumption in finish() — "finalizers cannot be added in this state" — is violated by execute() itself, on every fixture setup.

The failure sequence, confirmed by tracing addfinalizer / finish / execute on the val fixturedef:

1. execute() for param 1: _finalizers is empty, assert passes, the
   pytest_fixture_post_finalizer lambda is appended → len(_finalizers) == 1,
   cached_result still None.
2. ihook.pytest_fixture_setup(...) raises. cached_result is never assigned.
   The finally correctly schedules finish on the node.
3. At teardown, that scheduled finish(request) runs — and hits the early
   return, because cached_result is None. _finalizers is not drained.
4. execute() for param 2: _finalizers still has 1 entry and cached_result
   is None, so the cache branch is skipped and assert not self._finalizers
   trips.

Trace output for the reproducer above, with FixtureDef.addfinalizer,
FixtureDef.finish, FixtureDef.execute and SetupState.addfinalizer
monkeypatched to log calls for argname == "val":

[execute] val: _finalizers=0 cached=False
[addfinalizer] val -> now 1
[SetupState.addfinalizer] node=test_thing[fixture:derived] fin=<bound method finish of <FixtureDef argname='val' ...>>
[finish] val (had 1 finalizers)          <-- early-returns, does not drain
[execute] val: _finalizers=1 cached=False  <-- assert trips

Relationship to #14775 / #14777

#14777 is the same family of bug — a setup failure before cached_result is set
leaves a stale finalizer and the next execute() trips the same assert — but its
fix does not cover this case. That fix moves resolve_fixture_function() and the
async check inside the try/except TEST_OUTCOME in the default
pytest_fixture_setup implementation, so the failure gets cached. Here the
exception comes from a different hookimpl that runs before the default
implementation (tryfirst), so the default implementation never executes and
nothing caches the failure. Verified: the reproducer above still fails on main,
which already contains that work.

Suggested fixes

Either would resolve it; the first seems closer to the intent of both commits.

1. Make finish()'s early return conditional on there being nothing to do, e.g.
   if self.cached_result is None and not self._finalizers: return, so a
   fixture whose setup failed still gets drained. This keeps Fixture with dependencies executes pytest_fixture_post_finalizer multiple times #5848's
   "post_finalizer not called twice" property, since the guard still short-
   circuits the genuinely-already-finished case.
2. Register the pytest_fixture_post_finalizer finalizer only after
   cached_result has been set, restoring the invariant that finish()
   documents.

Either way it would be worth turning the assert not self._finalizers into a
real error path or dropping it, since as written any hook-raised setup failure
converts into an internal assertion with no indication of the real cause.

Real-world impact

Hit via pytest-lazy-fixtures, which implements lf() by resolving fixtures
inside pytest_fixture_setup exactly as the reproducer does. Any
@pytest.mark.parametrize list that mixes lf(...) with other values breaks
under 9.1.x as soon as the lazy fixture's dependency chain skips — which is the
normal case for integration tests that skip when a backing service is absent. In
our suite that turned 16 tests into internal AssertionErrors, attributed to
parameters that had nothing to do with the unavailable service.

Environment

pytest 9.1.1
Python 3.11.10
platform darwin
plugins: none required for the reproducer