From 358513b8a076f00f25dc5740404f7e4c8c855cce Mon Sep 17 00:00:00 2001
From: jakkdl <h6+github@pm.me>
Date: Mon, 18 Mar 2024 15:11:51 +0100
Subject: [PATCH] Don't reregister subfixture finalizer in parent fixture if
 value is cached.

---
 changelog/12135.bugfix.rst |  1 +
 src/_pytest/fixtures.py    | 18 ++++++--
 testing/python/fixtures.py | 85 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+), 4 deletions(-)
 create mode 100644 changelog/12135.bugfix.rst

diff --git a/changelog/12135.bugfix.rst b/changelog/12135.bugfix.rst
new file mode 100644
index 000000000..d77dbace9
--- /dev/null
+++ b/changelog/12135.bugfix.rst
@@ -0,0 +1 @@
+Fix subfixtures adding their finalizer multiple times to parent fixtures, causing incorrect teardown ordering in some instances.
diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py
index 48429a023..f5f3ab398 100644
--- a/src/_pytest/fixtures.py
+++ b/src/_pytest/fixtures.py
@@ -1035,13 +1035,18 @@ class FixtureDef(Generic[FixtureValue]):
             raise BaseExceptionGroup(msg, exceptions[::-1])
 
     def execute(self, request: SubRequest) -> FixtureValue:
-        finalizer = functools.partial(self.finish, request=request)
-        # Get required arguments and register our own finish()
-        # with their finalization.
+        # Ensure arguments (parent fixtures) are loaded.
+        # If their cache has been invalidated it will finish itself and subfixtures,
+        # which will set our self.cached_result = None.
+        # If/when parent fixture parametrization is included in our cache key this
+        # can be moved after checking our cache key and not require saving in a list.
+        parent_fixtures_to_add_finalizer = []
         for argname in self.argnames:
             fixturedef = request._get_active_fixturedef(argname)
             if not isinstance(fixturedef, PseudoFixtureDef):
-                fixturedef.addfinalizer(finalizer)
+                # save fixture as one to add our finalizer to, if we're not cached
+                # resolves #12135
+                parent_fixtures_to_add_finalizer.append(fixturedef)
 
         my_cache_key = self.cache_key(request)
         if self.cached_result is not None:
@@ -1060,6 +1065,11 @@ class FixtureDef(Generic[FixtureValue]):
             self.finish(request)
             assert self.cached_result is None
 
+        finalizer = functools.partial(self.finish, request=request)
+        # add finalizer to parent fixtures
+        for parent_fixture in parent_fixtures_to_add_finalizer:
+            parent_fixture.addfinalizer(finalizer)
+
         ihook = request.node.ihook
         try:
             # Setup the fixture, run the code in it, and cache the value
diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py
index 1e22270e5..2dc8460d5 100644
--- a/testing/python/fixtures.py
+++ b/testing/python/fixtures.py
@@ -4751,3 +4751,88 @@ def test_scoped_fixture_teardown_order(pytester: Pytester) -> None:
     )
     result = pytester.runpytest()
     assert result.ret == 0
+
+
+def test_subfixture_not_queue_multiple_finalizers(pytester: Pytester) -> None:
+    """Make sure subfixtures don't needlessly requeue their finalizer multiple times in
+    parent fixture.
+
+    Regression test for #12135
+    """
+    pytester.makepyfile(
+        """
+        import pytest
+
+        def _assert_len(request):
+            assert len(request._get_active_fixturedef('fixture_1')._finalizers) == 1
+
+
+        @pytest.fixture(scope="module")
+        def fixture_1():
+            ...
+
+        @pytest.fixture(scope="module")
+        def fixture_2(request, fixture_1):
+            yield
+
+        def test_1(request, fixture_2):
+            _assert_len(request)
+
+        def test_2(request, fixture_2):
+            _assert_len(request)
+        """
+    )
+    result = pytester.runpytest()
+    assert result.ret == 0
+
+
+def test_subfixture_teardown_order(pytester: Pytester) -> None:
+    """
+    Make sure fixtures don't re-register their finalization in parent fixtures multiple
+    times, causing ordering failure in their teardowns.
+
+    Regression test for #12135
+    """
+    pytester.makepyfile(
+        """
+        import pytest
+
+        execution_order = []
+
+        @pytest.fixture(scope="class")
+        def fixture_1():
+            ...
+
+        @pytest.fixture(scope="class")
+        def fixture_2(fixture_1):
+            execution_order.append("setup 2")
+            yield
+            execution_order.append("teardown 2")
+
+        @pytest.fixture(scope="class")
+        def fixture_3(fixture_1):
+            execution_order.append("setup 3")
+            yield
+            execution_order.append("teardown 3")
+
+        class TestFoo:
+            def test_initialize_fixtures(self, fixture_2, fixture_3):
+                ...
+
+            # This would previously reschedule fixture_2's finalizer in the parent fixture,
+            # causing it to be torn down before fixture 3.
+            def test_reschedule_fixture_2(self, fixture_2):
+                ...
+
+            # Force finalization directly on fixture_1
+            # Otherwise the cleanup would sequence 3&2 before 1 as normal.
+            @pytest.mark.parametrize("fixture_1", [None], indirect=["fixture_1"])
+            def test_finalize_fixture_1(self, fixture_1):
+                ...
+
+        def test_result():
+            assert execution_order == ["setup 2", "setup 3", "teardown 3", "teardown 2"]
+        """
+    )
+    result = pytester.runpytest()
+    assert result.ret == 0