From eb3253d344b6285fb86ce9335bc1073309dfcdc5 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 15 Oct 2026 00:29:51 -0400 Subject: [PATCH] test(tiers): keep the xdist probe out of recursive collection (#1756) A worker that dies mid-way through XdistSubprocessTests never runs its cleanup, so tests/protocols/test_zzz_xdist_guard_probe_unit.py stays on disk. xdist starts a replacement worker, which collects tests/ afresh; its pytest_collection_modifyitems audit finds the probe, sets session.shouldfail, and the controller ends the session with xdist.dsession.Interrupted, exit 2 (GraalPy job 116215824320, gw10). - tests/conftest.py: collect_ignore the probe's reserved path. pytest checks collect_ignore only while recursing, never for an initial path, so the probe's own nested run, which names it explicitly, still collects and audits it. SuiteIsCleanTests still scans it. - tests/test_tier_guard_xdist.py: ProbeWorkerCrashTests runs the real probe test under -n 2 with a -p plugin that kills the worker after the probe is written. It holds the exclusive lock throughout, then removes the stale probe. Skips without xdist, like its neighbours. Fails on main (Interrupted, exit 2), passes here; tier-guard, queue and docs-claims tests pass in the repo venv and under -n 2. Closes #1756 --- tests/conftest.py | 13 +++++ tests/test_tier_guard_xdist.py | 99 ++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index e1d5f107c9..3bf13614aa 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -418,6 +418,19 @@ def pytest_collection_modifyitems(session: 'pytest.Session', config: 'pytest.Con #: loads this conftest but never collects ``test_tier_guard_xdist.py`` at all. PROBE_MODULE_NAME = 'test_zzz_xdist_guard_probe_unit.py' +#: Keeps the probe out of every session that does not name it on the command +#: line (GitHub issue #1756). A worker that dies mid-probe never runs the +#: probe's cleanup, and xdist's replacement worker then collects ``tests/`` +#: afresh: without this, its collection-time audit finds the stale probe, sets +#: ``session.shouldfail``, and the controller interrupts the whole session. +#: pytest consults ``collect_ignore`` only while recursing into a directory, +#: never for an initial path (``Dir.collect`` skips the check for +#: ``session.isinitpath``), so the probe's own nested run, which passes the +#: probe's path explicitly, still collects and audits it. Only this one +#: reserved path is ignored, and :class:`tests.test_tier_guard.SuiteIsCleanTests` +#: still scans it on disk like any other module. +collect_ignore = [f'protocols/{PROBE_MODULE_NAME}'] + #: Module names whose own tests manage :func:`_tier_guard_lock` themselves and #: must not also take the shared hold :func:`_tier_guard_reader_lock` puts on #: every other unit-tier test. diff --git a/tests/test_tier_guard_xdist.py b/tests/test_tier_guard_xdist.py index 2b78cbce1d..966ec6ee15 100644 --- a/tests/test_tier_guard_xdist.py +++ b/tests/test_tier_guard_xdist.py @@ -37,6 +37,11 @@ ``SuiteIsCleanTests``, without that module's own source changing at all. :class:`ProbeWriteLockTests` pins the mutex itself, independently of xdist. +A third (#1756): a worker that dies mid-probe never runs its cleanup, and the +replacement worker xdist starts collects the stale probe. ``collect_ignore`` in +:mod:`tests.conftest` keeps the probe out of every recursive collection, and +:class:`ProbeWorkerCrashTests` kills a real worker mid-probe to pin that. + Kept separate from :mod:`tests.test_tier_guard`, which another change is already modifying: this module touches none of its code, only :mod:`tests.conftest`. @@ -45,8 +50,10 @@ import importlib.util import os +import shutil import subprocess import sys +import tempfile import textwrap import threading import unittest @@ -237,6 +244,98 @@ def test_xdist_run_reports_the_diagnostic_and_fails(self) -> None: self.assertNotIn('crashitem', output) +#: Module name of the ``-p`` plugin :class:`ProbeWorkerCrashTests` loads into +#: its throwaway session; written to a temporary directory, never to the tree. +_CRASH_PLUGIN_NAME = 'pcapkit_tier_guard_probe_crash' + +#: Kills a worker mid-probe, as CI's GraalPy leg lost one (job 116215824320): +#: after :class:`XdistSubprocessTests` has written the probe and before its +#: cleanup can remove it. The lock becomes a no-op because the test +#: that launched the session holds the exclusive hold on the probe's behalf. +_CRASH_PLUGIN_SOURCE = textwrap.dedent(f""" + import contextlib + import os + import subprocess + + _run = subprocess.run + + + def _crashing_run(args, *posargs, **kwargs): + if any(str(arg).endswith({_conftest.PROBE_MODULE_NAME!r}) for arg in args): + os._exit(1) + return _run(args, *posargs, **kwargs) + + + def pytest_runtest_setup(item): + if hasattr(item.config, 'workerinput'): + from tests import conftest + conftest._tier_guard_lock = lambda exclusive: contextlib.nullcontext() + subprocess.run = _crashing_run + """).lstrip('\n') + + +@unittest.skipUnless(importlib.util.find_spec('xdist') is not None, + 'pytest-xdist not installed') +class ProbeWorkerCrashTests(unittest.TestCase): + """A worker dying mid-probe fails that one test, not the session (#1756). + + xdist replaces the dead worker, and the replacement collects ``tests/`` + afresh while the probe's cleanup never ran. Before ``collect_ignore`` in + :mod:`tests.conftest`, that collection audited the stale probe and the + controller ended the session with ``xdist.dsession.Interrupted``, exit 2. + + """ + + def setUp(self) -> None: + reason = _tiers.guard_unavailable_reason() + if reason is not None: + self.skipTest(f'git cannot answer here: {reason}') + + self.probe_path = _tiers.TESTS_ROOT / 'protocols' / _conftest.PROBE_MODULE_NAME + # Exclusive for the whole throwaway session and until the stale probe is + # gone, so a concurrent SuiteIsCleanTests never sees it; LIFO cleanups + # unlink while still holding the lock, as in XdistSubprocessTests. + lock = _conftest._tier_guard_lock(exclusive=True) + lock.__enter__() + self.addCleanup(lock.__exit__, None, None, None) + self.assertFalse( + self.probe_path.exists(), + f'{self.probe_path} already exists -- a previous run did not clean up', + ) + self.addCleanup(self.probe_path.unlink, missing_ok=True) + + self.plugin_dir = tempfile.mkdtemp(prefix='pcapkit-1756-') + self.addCleanup(shutil.rmtree, self.plugin_dir, ignore_errors=True) + with open(os.path.join(self.plugin_dir, f'{_CRASH_PLUGIN_NAME}.py'), 'w', + encoding='utf-8') as handle: + handle.write(_CRASH_PLUGIN_SOURCE) + + def test_a_worker_crash_mid_probe_does_not_interrupt_the_session(self) -> None: + """Fails on main: exit 2 and the probe's diagnostic as the interrupt reason.""" + protocols = self.probe_path.parent + environ = _child_environ() + environ['PYTHONPATH'] = os.pathsep.join([self.plugin_dir, environ['PYTHONPATH']]) + probe_test = ('tests/test_tier_guard_xdist.py::XdistSubprocessTests' + '::test_xdist_run_reports_the_diagnostic_and_fails') + completed = subprocess.run( + [sys.executable, '-m', 'pytest', '-p', 'no:cacheprovider', '-p', _CRASH_PLUGIN_NAME, + '-q', '-n', '2', '--dist', 'load', '--max-worker-restart=2', probe_test, + # The directory the probe lives in, as CI collects it, minus every + # module but test_z*: only the probe could be left there. + 'tests/protocols', f'--ignore-glob={protocols}/[!t]*', + f'--ignore-glob={protocols}/t[!e]*', f'--ignore-glob={protocols}/test_[!z]*'], + cwd=str(_tiers.ROOT), env=environ, capture_output=True, text=True, + timeout=300, check=False, + ) + output = completed.stdout + completed.stderr + + self.assertIn(f"crashed while running '{probe_test}'", output) + self.assertTrue(self.probe_path.exists(), 'the crash did not leave the probe behind') + self.assertNotIn('Interrupted', output) + self.assertNotIn(_DIAGNOSTIC_MARKER, output) + self.assertEqual(completed.returncode, 1, output) + + class ProbeWriteLockTests(unittest.TestCase): """:func:`tests.conftest._tier_guard_lock` actually excludes a concurrent reader.