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.py25 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=3i='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.py4 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.py31 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
@JarryShaw
JarryShaw merged commit b7f5140 into main Sep 24, 2026
23 checks passed
@JarryShaw
JarryShaw deleted the ci-xdist-drop-py315-matrix branch September 24, 2026 04:41
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) review: good-to-go Cross-review at the current head says ready; CI state is separate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant