test(conftest): skip the tier-guard lock under an ancestor's shared hold (#2082)

tests/conftest.py

@@ -45,6 +45,7 @@
 import gc
 import hashlib
 import importlib
+import os
 import pathlib
 import sys
 import tempfile
@@ -529,9 +530,10 @@ def pytest_collection_modifyitems(session: 'pytest.Session', config: 'pytest.Con
 #: shared hold (spanning the whole test, including the blocking
 #: ``subprocess.run``) would wait on an inner exclusive request that cannot be
 #: granted until the outer test finishes: the same deadlock shape as above, one
-#: level removed. Today it never selects either, so nothing here triggers it;
-#: a future change to that selection should keep it that way, or add the
-#: module in question here too.
+#: level removed. Today it never selects either; a future change to that
+#: selection should keep it that way, or add the module in question here too.
+#: The nested run's own *shared* holds are a different matter, and
+#: :data:`TIER_GUARD_HELD_ENV` covers them.
 _LOCK_MANAGED_ELSEWHERE = frozenset({PROBE_MODULE_NAME, 'test_tier_guard_xdist.py'})
 
 #: Whether :func:`_tier_guard_flock` polls with ``LOCK_NB`` instead of blocking
@@ -550,6 +552,17 @@ def pytest_collection_modifyitems(session: 'pytest.Session', config: 'pytest.Con
 #: GraalPy as on CPython.
 TIER_GUARD_LOCK_POLL = 0.05
 
+#: Set, to :func:`_tier_guard_lock_path`, while :func:`_tier_guard_reader_hold`
+#: holds the lock shared, so that a nested :program:`pytest` the test spawns
+#: inherits it and does not ask for that lock again (#2082). The writer's gate
+#: makes a second shared request wait behind any writer already queued, and the
+#: writer waits on the outer hold, which waits on the nested run: a deadlock
+#: that only the nested run's ``subprocess.run`` timeout ends, as
+#: :mod:`tests.project.test_module_isolation` hit on GraalPy, whose slow start
+#: leaves the writer time to queue. The outer hold already keeps the writer out
+#: for as long as the nested run lasts, so skipping the inner one loses nothing.
+TIER_GUARD_HELD_ENV = 'PCAPKIT_TEST_TIER_GUARD_HELD'
+
 
 def _tier_guard_lock_path() -> 'pathlib.Path':
     """Where the probe/scan mutex lives, one file per checkout.
@@ -659,6 +672,32 @@ def _tier_guard_lock(exclusive: 'bool') -> 'Iterator[None]':
             fcntl.flock(handle.fileno(), fcntl.LOCK_UN)
 
 
+@contextlib.contextmanager
+def _tier_guard_reader_hold() -> 'Iterator[None]':
+    """A test's shared hold, unless an ancestor process already has one (#2082).
+
+    Takes :func:`_tier_guard_lock` shared and sets :data:`TIER_GUARD_HELD_ENV`
+    for the block, putting the variable back as it was afterwards. Where the
+    variable already names this lock file, the hold is an ancestor's that spans
+    this whole process, so the lock is not asked for again.
+
+    """
+    held = str(_tier_guard_lock_path())
+    if os.environ.get(TIER_GUARD_HELD_ENV) == held:
+        yield
+        return
+    with _tier_guard_lock(exclusive=False):
+        previous = os.environ.get(TIER_GUARD_HELD_ENV)
+        os.environ[TIER_GUARD_HELD_ENV] = held
+        try:
+            yield
+        finally:
+            if previous is None:
+                os.environ.pop(TIER_GUARD_HELD_ENV, None)
+            else:
+                os.environ[TIER_GUARD_HELD_ENV] = previous
+
+
 @pytest.fixture(autouse=True)
 def _tier_guard_reader_lock(request: 'pytest.FixtureRequest') -> 'Iterator[None]':
     """Hold the probe/scan mutex shared, for every unit-tier test but the two
@@ -670,13 +709,13 @@ def _tier_guard_reader_lock(request: 'pytest.FixtureRequest') -> 'Iterator[None]
     :class:`~tests.test_tier_guard.SuiteIsCleanTests`, so the protection is
     structural and covers a similarly-scanning test written later without this
     fixture changing at all -- and it never has to touch that module's own
-    source to do it.
+    source to do it. :func:`_tier_guard_reader_hold` takes the hold.
 
     """
     location = getattr(request.node, 'path', None) or getattr(request.node, 'fspath', None)
     path = pathlib.Path(str(location)) if location is not None else None
     if path is None or path.name in _LOCK_MANAGED_ELSEWHERE or not is_unit_tier(path):
         yield
         return
-    with _tier_guard_lock(exclusive=False):
+    with _tier_guard_reader_hold():
         yield

tests/test_tier_guard_xdist.py

@@ -703,3 +703,95 @@ def test_a_real_leftover_still_fails_and_releases_the_lock(self) -> None:
 
         self.assertTrue(self.probe_path.exists(), 'setUp removed a probe it did not write')
         self._assert_lock_free()
+
+
+#: What :class:`ProbeLockNestedRunTests` runs in a child, as a nested
+#: :program:`pytest` would: take a test's shared hold on the lock file named by
+#: ``sys.argv[1]``, under the environment the parent's hold handed down.
+_NESTED_READER = '''
+import pathlib, sys, unittest.mock
+from tests import conftest
+with unittest.mock.patch.object(conftest, '_tier_guard_lock_path',
+                                return_value=pathlib.Path(sys.argv[1])):
+    with conftest._tier_guard_reader_hold():
+        print('IN', flush=True)
+'''
+
+
+@unittest.skipUnless(_conftest.fcntl is not None, 'fcntl unavailable on this platform')
+class ProbeLockNestedRunTests(unittest.TestCase):
+    """A process under a test's shared hold does not ask for the lock again (#2082).
+
+    :mod:`tests.project.test_module_isolation` runs a nested :program:`pytest`
+    inside its shared hold. A writer arriving meanwhile holds the gate while it
+    waits on that hold, so a nested shared request waited behind it, and the
+    hold waited on the nested run: on GraalPy the run's ``subprocess.run``
+    timeout, 600 s, was what broke it, with the writer and every other reader
+    stalled as long.
+
+    """
+
+    def setUp(self) -> None:
+        _redirect_tier_guard_lock(self, 'pcapkit-2082-')
+        patcher = unittest.mock.patch.object(_conftest, 'TIER_GUARD_LOCK_POLL', 0.01, create=True)
+        patcher.start()
+        self.addCleanup(patcher.stop)
+        environ = unittest.mock.patch.dict(os.environ)
+        environ.start()
+        self.addCleanup(environ.stop)
+        os.environ.pop(_conftest.TIER_GUARD_HELD_ENV, None)
+
+    def _held(self, suffix: 'str', operation: 'int') -> 'bool':
+        """Whether ``operation`` on the lock file plus ``suffix`` would block."""
+        fcntl = _conftest.fcntl
+        path = _conftest._tier_guard_lock_path()
+        with open(path.with_name(path.name + suffix), 'a', encoding='utf-8') as handle:
+            try:
+                fcntl.flock(handle.fileno(), operation | fcntl.LOCK_NB)
+            except BlockingIOError:
+                return True
+            fcntl.flock(handle.fileno(), fcntl.LOCK_UN)
+            return False
+
+    def test_a_nested_run_does_not_queue_behind_a_waiting_writer(self) -> None:
+        """Fails on the code before this change: the child times out at the gate."""
+        order = []  # type: list[str]
+
+        def writer() -> None:
+            with _conftest._tier_guard_lock(exclusive=True):
+                order.append('W')
+
+        with _conftest._tier_guard_reader_hold():
+            thread = threading.Thread(target=writer, daemon=True)
+            thread.start()
+            self.addCleanup(thread.join, 10)
+            end = time.monotonic() + 10
+            while not self._held('.gate', _conftest.fcntl.LOCK_SH):
+                self.assertLess(time.monotonic(), end, 'the writer never took the gate')
+                time.sleep(0.01)
+            try:
+                child = subprocess.run(  # nosec: B603
+                    [sys.executable, '-c', _NESTED_READER, str(_conftest._tier_guard_lock_path())],
+                    cwd=str(_tiers.ROOT), env=_child_environ(), capture_output=True, text=True,
+                    timeout=120, check=False,
+                )
+            except subprocess.TimeoutExpired:
+                self.fail('the nested reader queued behind the writer the outer hold keeps waiting')
+            self.assertEqual((child.returncode, child.stdout), (0, 'IN\n'), child.stderr)
+            self.assertEqual(order, [], 'the writer got in under the outer hold')
+        thread.join(10)
+        self.assertEqual(order, ['W'], 'the writer never got the lock once the hold ended')
+
+    def test_the_variable_names_the_hold_only_while_it_lasts(self) -> None:
+        """Set for the hold and put back after; another lock file's value is no hold."""
+        name = _conftest.TIER_GUARD_HELD_ENV
+        for before in (None, '/elsewhere/tier-guard.lock'):
+            with self.subTest(before=before):
+                if before is not None:
+                    os.environ[name] = before
+                with _conftest._tier_guard_reader_hold():
+                    self.assertEqual(os.environ.get(name), str(_conftest._tier_guard_lock_path()))
+                    self.assertTrue(self._held('', _conftest.fcntl.LOCK_EX),
+                                    'the hold did not take the lock')
+                self.assertEqual(os.environ.get(name), before)
+                self.assertFalse(self._held('', _conftest.fcntl.LOCK_EX))