Skip to content

ci: parallelise pytest with xdist and drop 3.15 from the blocking matrix - #725

Merged
JarryShaw merged 2 commits into
mainfrom
ci-xdist-drop-py315-matrix
Sep 24, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
ci-xdist-drop-py315-matrix

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 23, 2026 •

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • ci — workflows or build tooling

Description of your pull request and other information

Two of #715's four items, plus three review findings. Rebased onto #727 (main bf57b4542); its integration-job selection rewrite conflicted with this PR's -n auto --dist load addition to the same step — resolved by keeping #727's fixture_tier_paths() selection/error-checking and appending -n auto --dist load to its final pytest line.

Change Detail
pytest-xdist -n auto --dist load on test/integration/gate; pinned >=3.6.1 (3.0.2-3.5.0 lack the workerinput forwarding the tier-guard fix needs, and PyPI yanked 3.6.0 itself)
Drop 3.15 Removed from unit-tests.yml's blocking matrices; moved to a schedule-only job in python-compatibility.yml

Findings fixed:

Tests: test_tier_guard.py + test_tier_guard_xdist.py, 42 passed/57 subtests under real xdist (40 passed/2 skipped/57 subtests in the repo's own no-xdist venv). Race reproduced 10/10 and 14/32 with the mutex disabled, 0 failures across 350+ repeated concurrent trials with it enabled, including an independent cross-model review that reran the same toggle experiment. test_module_isolation.py: 6 passed.

Related to #715; item 1 (integration partitioning) is done by #727, item 2 (gate dedup) remains.

@JarryShaw JarryShaw added perf Pull requests that improve performance (perf: subject prefix) ci Pull requests that change CI or workflow configuration (ci: subject prefix) labels Sep 23, 2026
@JarryShaw
JarryShaw force-pushed the ci-xdist-drop-py315-matrix branch from 21de5a2 to e975b9e Compare September 23, 2026 22:23
@JarryShaw

Copy link
Copy Markdown
Owner Author

❌ NEEDS CHANGES — cross-review on Opus of Sonnet-authored work at head e975b9e6e: -n auto destroys the tier guard's entire diagnostic under xdist (serial exit 4 + full message becomes exit 3 + INTERNALERROR with the message absent), and the --dist load comment's stated reason is stale because #720 is already fixed in this PR's own merge base.

@JarryShaw

Copy link
Copy Markdown
Owner Author

❌ NEEDS CHANGES — cross-review on Opus of Sonnet-authored work; head e975b9e6e. No prior verdict on this PR (0 reviews / 0 issue comments / 0 review threads), so nothing is superseded.

Own evidence: repo venv 3.14.7 with pytest 9.1.1, PYTHONSAFEPATH=1, pcapkit.__file__ asserted inside this worktree; xdist 3.8.0 installed to a private --target only, never the shared venv. tests/test_tier_guard.py → 25 passed, 15 subtests, serial and under -n auto --dist load.

1. Blocking — -n auto destroys the tier guard's diagnostic

tests/conftest.py raises pytest.UsageError from pytest_collection_modifyitems, and its docstring makes the message the entire point ("one loud, explanatory message at the top of the output rather than a red test buried in a summary"). Reproduced with a real untracked tier violation, since deleted:

run exit guard message in output
serial 4 full diagnostic — file:line, capture name, the CI command, two named remedies
-n auto --dist load 3 0 occurrences across 78 lines; INTERNALERROR … dsession.py:217 assert not crashitem names the test as a crashed item

CI still goes red, so the gate holds as a gate — but whoever trips it gets an xdist assertion instead of the instructions. Cheapest fix that preserves intent: emit the findings to stderr / terminalreporter before raising, so they survive however the controller renders a worker-side collection error.

2. Blocking — the --dist load comment's reason is stale

It asserts, present tense, that load "is the only xdist distribution mode that is safe on this suite today" because loadfile/loadscope "surface a real order-dependent sys.modules leak (#720)". #720 is fixed in this PR's own merge base 4391dc77b. Measured at this head on the #720 reproduction set (tests/protocols/test_dispatch_registry_unit.py + tests/project, -n 2): loadfile → exit 0, 133 passed; loadscope → exit 0, 133 passed. Keep the pin — pinning the mode is right and load is the measured one — but the reason as written is untrue and will mislead.

Attacked and held

  • The pin is unoverridable. No pytest.ini/setup.cfg/tox.ini; [tool.pytest.ini_options] carries only testpaths/python_files, no addopts. Precedence measured anyway: ini addopts = --dist loadfile + PYTEST_ADDOPTS=--dist loadscope + CLI --dist load → resolved dist='load'; no-CLI control → dist='loadfile', proving the ini was live and genuinely overridden.
  • xdist does not undermine tests: an unrestored setUpClass module purge leaks sys.modules state across files, blocking --dist loadfile #720's fix — the item nobody had checked. pytest_sessionstart fires in every worker before collection: 4 workers + controller, distinct pids, each pinning size=315 with pcapkit_present=True, all under module key tests.conftest. The warm-up import runs per worker, so none pins a cold table, and the pin precedes any setUpClass in each. Safe because the design is per-process and an xdist worker is a process.
  • Subtest fidelity holds. Deliberate failing subtest: serial and -n 2 --dist load both exit 1 with 1 failed, 2 passed, 5 subtests passed and a full traceback. Cosmetic only — ids stringify (i=3 → i='3') and subtests render as u in the progress line.
  • compatibility-nightly is reachable — cron '0 4 * * 6' genuinely exists and the if: matches it. lint.yml is comment-only — YAML parse identical before/after, 0 non-comment diff lines. timeout-minutes: 45 untouched on all three pytest jobs, and all three install the test extra, so there is no -n auto without xdist present.

Notes, not blockers

❌ NEEDS CHANGES — head e975b9e6e.

@JarryShaw
JarryShaw force-pushed the ci-xdist-drop-py315-matrix branch from e975b9e to 7d1f011 Compare September 24, 2026 01:05
@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE @ 7d1f011cf — the guard still hard-fails the run with its full diagnostic under all 9 xdist distribution modes (exit 2, zero INTERNALERROR) and 12/12 stress repeats, and all 15 required contexts still have producers; one judgement call for you: pytest-xdist>=3 should be >=3.6.1, because the mechanism the fix relies on does not exist below it.

@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE @ 7d1f011cf — gate still hard-fails with the full diagnostic under all 9 distribution modes; one judgement call: pytest-xdist>=3 should be >=3.6.1.

Supersedes the ❌ NEEDS CHANGES verdict at e975b9e6e — both its blockers are fixed. Cross-review on Opus of Sonnet-authored work. Repo venv 3.14.7 + xdist 3.8.0/pytest 9.1.1 exposed via a PYTHONPATH symlink overlay (venv-from-venv --system-site-packages inherits the base site-packages, not the parent venv's, so /tmp/xd725v cannot import pcapkit at all); pcapkit.__file__ asserted inside my worktree, captures built, repo venv left with no xdist/execnet.

The gate holds — real violation, guard ACTIVE (guard_unavailable_reason() → None)

invocation exit diagnostic INTERNALERROR crashitem
serial 4 (ERROR:) 1 0 0
load / loadfile / loadscope / loadgroup / worksteal / each / no / -n auto 2 (Interrupted:) 1 0 0

12/12 repeats at -n 4, -n auto, -n 16, worksteal over all of tests/protocols (hundreds of real items to schedule): every one exit 2, no flakiness. No false positives either — a clean selection under -n 4 --dist load is 31 passed, exit 0.

Mechanism — confirmed, and isolated to the one added line

Single-variable A/B, -n 4 --dist load, 5 repeats per arm, only the session.shouldfail assignment differing:

arm exit diagnostic INTERNALERROR crashitem
without 3 0 39 2 — dsession.py:217 assert not crashitem, naming a real scheduled id
with 2 1 0 0

Chain is as the comment says: _pytest/main.py:872 modifyitems inside the try:, :879 pytest_collection_finish in the finally:; xdist/remote.py:141-149 copies shouldfail into workeroutput; dsession.py:206-212's for…else breaks past the assert at :217. One refinement — the worker's own shouldfail check (remote.py:207) never runs, since collection raised before pytest_runtestloop, so the entire effect is controller-side. Pre-fix was a genuine race; post-fix the break is unconditional on shouldfail's truthiness, so it cannot fire on any interleaving.

Counts

  • tests/test_tier_guard_xdist.py — 4 passed with xdist; 2 passed, 2 skipped on the repo venv, skipping for pytest-xdist not installed, not for absent captures.
  • tests/test_tier_guard.py + tests/project/test_module_isolation.py — 31 passed, 15 subtests, serial and under -n 4 --dist load.

The one change I would ask for

>=3 permits xdist 3.0.2–3.5.0, none of which forward shouldfail to the controller at all — there the fix silently reverts to the INTERNALERROR bug, and this PR's own test_xdist_run_reports_the_diagnostic_and_fails goes red rather than skipping, since xdist 3.5 imports fine. The mechanism arrives in 3.6.0 (xdist #1024), which is yanked, so the honest floor is >=3.6.1. It cannot bite this CI (no lockfile, so pip resolves 3.8.0), hence a judgement call and not a blocker. pytest>=8 is safe: the shouldfail setter is identical in 8.0.0 and 9.1.1 and its unset-guard never fires for a non-empty string.

Attacked and held

  • Both e975b9e6e blockers fixed. grep -rn "720" .github/workflows/ → nothing; the sole loadfile/loadscope mention is the future-proofing sentence, which is accurate — xdist/plugin.py:318-320 already defaults dist=load under -n, so the pin guards exactly the future default change it claims. CLI beats PYTEST_ADDOPTS beats ini addopts (measured), so it is the strongest pin available.
  • Serial unchanged: one conftest.py in the repo, no hookwrappers/pytest_plugins/entry points, the single call site keyword-only; session is a declared hookspec arg (_pytest/hookspec.py:268) and pluggy binds by name. Message string is character-identical to pre-change.
  • hasattr(config, 'workerinput') is pytest core's own idiom (cacheprovider.py:417, stepwise.py:64, junitxml.py:424); xdist/remote.py:424 is the only assignment site anywhere.
  • 3.15 drop is safe: ruleset 23497679 is the only gate (branches/main/protection 404s), and all 15 required contexts are already reporting on this PR's own run with 2 green. No required context ever came from a continue-on-error leg. All three pytest jobs — gate included — install .[test], so -n auto never runs without xdist.

Notes, not blockers

  • Interrupted: vs ERROR: framing — content is fully intact (file:line, the CI command, both remedies). But "unfixable from a conftest" is overstated: pytest_keyboard_interrupt is a documented conftest hook (hookspec.py:1214) and terminal.py:1011-1023 is what writes the ! … ! banner, so it could be relabelled. Nothing is lost, so I would not spend the code on it — your call.
  • Unchanged from last review: make test/Pipfile still carry no xdist, so CI runs a distribution local dev cannot reproduce; compatibility-nightly's cron is weekly (Sat 04:00 UTC) with no workflow_dispatch.
  • lint.yml's "newest non-experimental version in the test matrix" is now vestigial — after this PR nothing in that matrix is experimental. True, just redundant.

✅ GOOD TO MERGE @ 7d1f011cf.

@JarryShaw
JarryShaw force-pushed the ci-xdist-drop-py315-matrix branch 2 times, most recently from 11fc9a6 to 391a1b3 Compare September 24, 2026 02:55
…blocking matrix

Contention, not job duration, was the bottleneck (#703). Changes:

- Add pytest-xdist>=3.6.1 (not >=3: 3.0.2-3.5.0 lack the workerinput
  forwarding the fix below needs, and PyPI has yanked 3.6.0); run
  `-n auto --dist load` in all three pytest jobs.
- Drop Python 3.15 from unit-tests.yml's blocking matrices; its
  compileall/import check moves to a schedule-only job in
  python-compatibility.yml.
- Fix the tier guard's UsageError diagnostic under xdist (swallowed by a
  controller race, `assert not crashitem` in dsession.py) by setting
  session.shouldfail inside a worker before raising.
- Close a second race: XdistSubprocessTests' probe module and
  SuiteIsCleanTests' filesystem scan can land in different xdist workers
  and overlap. tests/conftest.py adds a readers-writer flock mutex around
  both, without touching test_tier_guard.py.

Verified: test_tier_guard.py and test_tier_guard_xdist.py pass serial and
under xdist; the probe/scan race reproduced in 14/32 trials pre-fix, 0/139
post-fix over repeated runs.
@JarryShaw
JarryShaw force-pushed the ci-xdist-drop-py315-matrix branch from 391a1b3 to 5ac52f5 Compare September 24, 2026 03:00
@JarryShaw JarryShaw added the review: pending No verdict for the current head - never reviewed, or the head moved since the last one label Sep 24, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE @ 5ac52f567 — reproduced the race myself: lock off, 18/32 trials hit the exact CI failure (32/32 failed overall); lock on, 0/60 across ~2,520 reader acquisitions.

@JarryShaw

Copy link
Copy Markdown
Owner Author

✅ GOOD TO MERGE @ 5ac52f567 — reproduced the race myself: lock off, 18/32 trials hit the exact CI failure (32/32 failed overall); lock on, 0/60 across ~2,520 reader acquisitions.

Cross-review on Opus (author: Sonnet). Supersedes the ✅ at 7d1f011cf, which predates the mutex. Read-only: no repo file was edited — xdist came from a symlink PYTHONPATH overlay and the lock was toggled by shadowing fcntl, which is conftest's own documented no-op path, so the repo venv stayed without xdist throughout.

claim verdict my evidence
race real, mutex closes it ✅ 92 trials of -n auto --dist load on 16 cores: off 18/32 SuiteIsCleanTests scan failures (author: 14/32), on 0/60
mutex covers gate ✅ 181 modules under tests/: 150 reader-locked, 30 not, 1 self-managing; all 3 modules that scan TESTS_ROOT are locked — 0 holes
pytest-xdist>=3.6.1 floor ✅ shouldfail absent from remote.py in 3.0.2–3.5.0, present in 3.6.1/3.7.0/3.8.0; PyPI confirms 3.6.0 yanked
3.15 safe to drop ✅ ruleset 23497679 requires 15 checks, exactly 3.10–3.14 × {Python, Integration Python, Compat Python}; each still maps to a surviving job of the same name, so nothing becomes unsatisfiable
#727 structure intact ✅ both guards, the echo, shell: bash and the unquoted $selection all present at :184–193; the flags are the only change
documented deadlock gap ✅ acceptable test_module_isolation is the only unit-tier nested-pytest spawner (the other four subprocess sites are git ls-files or plain python -c, which load no conftest); its three selections are literal file lists holding neither racing file; and run_pytest's timeout=600 bounds even a future regression to a loud TimeoutExpired rather than a silent hang

Two things the description undersells. tests/project/test_capture_tracking.py is a second scanner that also races, and it is covered only because the fixture keys on is_unit_tier rather than naming SuiteIsCleanTests — that choice is load-bearing, not stylistic. And Linux flock grants a new shared hold while an exclusive one waits (measured), so the outer-shared/inner-shared shape gate genuinely does produce cannot deadlock.

Unmeasured before, now measured — and your call, not mine: on the post-#727 integration selection (20 entries, 173 tests) -n auto is worth ~8×, alternated arms, 161 passed / 12 skipped / 243 subtests identical in all four runs — serial 146.5s and 139.8s, xdist 18.9s and 17.3s. Host load 2.8–8.6 on 16 cores, so treat the ratio as approximate; it also confirms the fixture tier is xdist-safe.

Caveats, none blocking:

  • XdistSubprocessTests.setUp's assertFalse(probe_path.exists()) sits outside the hold (:171 vs the acquire at :182). Lock off it fired 32/32; lock on, 0/60 — but it is protected incidentally, by other workers stalling inside the reader fixture, not structurally. Moving that assertion inside the hold would make it structural.
  • Writer starvation in gate is unmeasured: with no writer preference the exclusive request is granted only when the reader count reaches zero, and gate never runs per PR push. Bounded by timeout-minutes: 45.
  • Two pytest sessions in one checkout now serialise on the lock file. Irrelevant in CI (a fresh runner per job); visible to anyone running an IDE and a terminal run at once. Per-checkout keying works — I saw two distinct lock files for two worktrees.
  • Nit: the pyproject.toml comment cites remote.py:146-147, which are 3.8.0's line numbers; in 3.6.1, the pinned floor, the same statement is at :118.

✅ GOOD TO MERGE @ 5ac52f567

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Pull requests that change CI or workflow configuration (ci: subject prefix) perf Pull requests that improve performance (perf: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant