From e20ee37e563a0ea9d3045c8b5d1a26875f3fa001 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 23 Oct 2026 01:43:28 -0400 Subject: [PATCH] test(conftest): skip the tier-guard lock under an ancestor's shared hold (#2082) A unit-tier test that runs a nested pytest holds the probe/scan lock shared for the whole run. Since the writer gate (#1977), a writer arriving meanwhile holds the gate while it waits on that hold, so the nested run's own shared requests queue behind it and the outer hold waits on the nested run: a deadlock only subprocess.run's 600 s timeout ended. On GraalPy's experimental leg, test_module_isolation failed this way in three runs, with ProbeWriteLockTests and another reader stalled ~590 s alongside it. The reader hold now exports PCAPKIT_TEST_TIER_GUARD_HELD, naming the lock file, for its duration; a process that inherits it for the same lock file skips the request, since the ancestor's hold already keeps the writer out. ProbeLockNestedRunTests reproduces the deadlock with a real child process (it times out at the gate without the fix) and pins the variable's scope. --- tests/conftest.py | 49 ++++++++++++++++-- tests/test_tier_guard_xdist.py | 92 ++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 5 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 345b6122b2..997968f4a9 100644 --- a/tests/conftest.py +++ b/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,7 +709,7 @@ 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) @@ -678,5 +717,5 @@ def _tier_guard_reader_lock(request: 'pytest.FixtureRequest') -> 'Iterator[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 diff --git a/tests/test_tier_guard_xdist.py b/tests/test_tier_guard_xdist.py index a07a7c79d1..92a95d51dc 100644 --- a/tests/test_tier_guard_xdist.py +++ b/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))