Skip to content

Commit 02ee60a

Browse files
authored
ci(unit-tests): save each xdist worker's coverage at session finish and keep the process log (#1423) (#1424)
- tests/conftest.py: a pytest_sessionfinish hook saves an xdist worker's auto-started coverage before shutdown, so a worker that dies after reporting its results no longer loses its whole data file. - .github/coverage.toml: sigterm = true, so a SIGTERM saves the data too. - unit-tests.yml: the guard names each missing pid's command line and whether its atexit save ran; coverage-processes.log is now uploaded as the coverage-processes-log artifact on every coverage leg. - docs: unit-tests.yml citations renumbered by line content; the coverage section mentions the save and the new artifact. tests/project: 404 passed, 1 skipped; the 5 new tests fail without the fix.
1 parent 07c74b3 commit 02ee60a

7 files changed

Lines changed: 163 additions & 14 deletions

File tree

‎.github/coverage.toml‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,14 @@
1313
# data before os._exit(), which skips atexit: execnet calls it on
1414
# a worker whose execution outlives shutdown, and pytest-timeout's
1515
# thread method on a hung test (#1387).
16+
# sigterm -- installs a SIGTERM handler that saves the data. `patch =
17+
# ["_exit"]` covers os._exit and atexit covers a clean exit, but
18+
# nothing covered a signal, so a worker SIGTERMed during
19+
# shutdown would leave no file -- one candidate for #1423, whose
20+
# actual cause is not established. Does not help against
21+
# SIGKILL -- execnet's post-timeout kill is `Popen.kill()` --
22+
# which is why `tests/conftest.py` also saves at
23+
# `pytest_sessionfinish` rather than relying on this alone.
1624
# core -- "ctrace", not the sys.monitoring default on 3.12+. Tests
1725
# purge and re-import pcapkit, and sysmon keeps state for every
1826
# generation's code objects: measured on test_http_unit.py
@@ -30,6 +38,7 @@ patch = [
3038
"subprocess",
3139
"_exit",
3240
]
41+
sigterm = true
3342
core = "ctrace"
3443

3544
[tool.coverage.report]

‎.github/workflows/unit-tests.yml‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,16 +172,26 @@ jobs:
172172
# direct children in the log above) left one.
173173
python - <<'PY'
174174
import glob, os, re, sys
175-
parent, cmd = {}, {}
175+
parent, cmd, atexit_ = {}, {}, {}
176176
log = 'coverage-processes.log'
177177
for line in open(log, errors='replace').readlines() if os.path.exists(log) else []:
178178
if m := re.match(r' *(\d+)\.\w+: New process(: cmd| parent pid): (.*)', line):
179179
(parent if m[2] == ' parent pid' else cmd)[m[1]] = m[3]
180+
elif m := re.match(r' *(\d+)\.\w+: (atexit|sigterm): pid: \d+, (.*)', line):
181+
atexit_[m[1]] = f'{m[2]}: {m[3]}'
180182
root = [p for p, c in cmd.items() if "'run', '--rcfile=.github/coverage.toml'" in c]
181183
workers = sorted(p for p, q in parent.items() if root and q == root[0])
182184
saved = set(re.findall(r'\.pid(\d+)\.', ' '.join(glob.glob('.coverage.*'))))
183185
missing = [p for p in root + workers if p not in saved]
184186
print(f'controller {root}, {len(workers)} xdist workers, missing data: {missing}')
187+
# #1423: which process it was, and whether coverage's atexit handler ran
188+
# in it at all -- a pid with no `atexit:` line died on a signal before
189+
# saving, one with such a line reached the save and lost it afterwards.
190+
# Those are different bugs, and the log that distinguishes them is kept
191+
# as an artifact by the step below rather than only read here.
192+
for p in missing:
193+
print(f' pid {p}: cmd {cmd.get(p, "<no cmd line logged>")}')
194+
print(f' pid {p}: {atexit_.get(p, "no atexit/sigterm entry -- died before saving")}')
185195
if len(root) != 1 or not workers or missing:
186196
why = f'no data file from pid {missing}' if missing else 'the process log is missing or unreadable'
187197
with open(os.environ['GITHUB_STEP_SUMMARY'], 'a') as summary:
@@ -227,6 +237,21 @@ jobs:
227237
print('Per-file detail: the `coverage-html` artifact of this run.')
228238
PY
229239
240+
# #1423: the guard above fires on runs whose tests all pass, and the only
241+
# record of what became of the lost process is this log -- it carries each
242+
# measured process's command line, its parent, and the `atexit:` line
243+
# coverage writes as it saves. Reading it off the runner is impossible once
244+
# the job ends, so keep it whenever the leg ran, not only on failure: a run
245+
# that passed is the baseline the failing one has to be read against.
246+
- name: Upload coverage process log
247+
if: ${{ !cancelled() && matrix.coverage }}
248+
uses: actions/upload-artifact@v7
249+
with:
250+
name: coverage-processes-log
251+
path: coverage-processes.log
252+
retention-days: 30
253+
if-no-files-found: warn
254+
230255
- name: Upload coverage HTML report
231256
if: ${{ !cancelled() && matrix.coverage }}
232257
uses: actions/upload-artifact@v7

‎docs/source/contributing/releasing.rst‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ Editing ``__version__`` by hand without also moving :file:`CITATION.cff`'s
4646
(``:494-514``) asserts the two agree, and ``create-release.yml``'s
4747
``unit-tests`` job (``:362-376``) calls ``unit-tests.yml`` with
4848
``gate-only: true``, which runs the **full** suite rather than the tiered
49-
subset an ordinary push runs (its ``gate`` job, ``unit-tests.yml:1189-1258``).
49+
subset an ordinary push runs (its ``gate`` job, ``unit-tests.yml:1214-1283``).
5050
That gate runs whenever a release will -- a moved ``__version__`` is exactly
5151
what makes ``version_check``'s evidence read "not yet published" -- and every
5252
publishing job requires it to have succeeded (``:392,493,573,674``), so a
@@ -56,7 +56,7 @@ Either run the script, or move both fields by hand in the same commit.
5656
This is the **only** edit a person makes to get a release started -- the tag,
5757
the Release, and every upload are the workflow's job from here. Two other
5858
checks run unconditionally on the same path, though neither is a step in *this*
59-
process: the ``changelog`` job (``unit-tests.yml:1049-1070``) fails outright if
59+
process: the ``changelog`` job (``unit-tests.yml:1074-1095``) fails outright if
6060
``CHANGELOG.md`` has drifted from its source entry under
6161
:file:`docs/source/changelog/`, and the release-body step warns, without
6262
failing, if that entry's heading still reads "unreleased"

‎docs/source/contributing/workflows.rst‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ and ``project-tests``, and ``required-checks`` (see `Required Status
245245
Checks`_ below), gated out by their own ``if:`` rather than by having
246246
already run. ``changelog`` carries no
247247
``if:`` at all and runs on every path regardless -- deliberately, per its own
248-
comment (``unit-tests.yml:1037-1043``): ``create-release.yml`` feeds
248+
comment (``unit-tests.yml:1062-1068``): ``create-release.yml`` feeds
249249
``CHANGELOG.md`` to the GitHub Release body, so the release path is exactly
250250
where a drifted file must not go unchecked. Confirmed on run `36210743295
251251
<https://github.com/JarryShaw/PyPCAPKit/actions/runs/36210743295>`__ (a
@@ -352,9 +352,14 @@ and ``patch = ["subprocess"]``, without which only the xdist controller is
352352
measured, not its workers, plus ``core = "ctrace"``: with the suite purging
353353
and re-importing pcapkit, one module's peak RSS was 4.17 GiB under the default
354354
``sys.monitoring`` core against 0.41 GiB under ``ctrace``, and that growth
355-
killed the runner. The leg then writes the total and a per-package table to the
356-
job summary and uploads the HTML report as the ``coverage-html``
357-
artifact (30 days), with the bare numbers as the ``coverage-summary`` artifact.
355+
killed the runner. ``sigterm = true`` and a ``pytest_sessionfinish`` hook in
356+
:file:`tests/conftest.py` that saves each xdist worker's data early keep a
357+
worker that dies during shutdown from losing its file
358+
(`#1423 <https://github.com/JarryShaw/PyPCAPKit/issues/1423>`__). The leg then
359+
writes the total and a per-package table to the job summary and uploads the HTML
360+
report as the ``coverage-html`` artifact (30 days), with the bare numbers as the
361+
``coverage-summary`` artifact and the per-process debug log as
362+
``coverage-processes-log``.
358363

359364
The PR comment comes from **Coverage Comment** (:file:`coverage-comment.yml`),
360365
not from ``unit-tests.yml``. That file is also a reusable workflow, a called
@@ -384,7 +389,7 @@ grepping every workflow file for its name:
384389
- Where
385390
* - ``Required checks passed``
386391
- job ``required-checks``
387-
- ``unit-tests.yml:1346``
392+
- ``unit-tests.yml:1371``
388393
* - ``Compat Python 3.10``
389394
- job ``compatibility``, matrix leg ``3.10``
390395
- ``python-compatibility.yml:31,41``
@@ -402,20 +407,20 @@ grepping every workflow file for its name:
402407
- ``python-compatibility.yml:31,45``
403408

404409
``Required checks passed`` is defined by exactly one job
405-
(``unit-tests.yml:1346``). ``Compat Python`` is defined by two in
410+
(``unit-tests.yml:1371``). ``Compat Python`` is defined by two in
406411
``python-compatibility.yml``: the required ``compatibility`` job (``:31``,
407412
``Compat Python ${{ matrix.python-version }}``, expanding to the five required
408413
legs at ``:41-45``) and the non-required ``compatibility-nightly`` job (``:69``,
409414
``Compat Python 3.15 (scheduled)``, see below). A comment at
410-
``unit-tests.yml:1261`` mentions the string without defining it.
415+
``unit-tests.yml:1286`` mentions the string without defining it.
411416

412417
``Required checks passed`` is an aggregate, not a single check run. The ruleset
413418
requires it and the five ``Compat Python 3.10``-``3.14`` legs, nothing else, as
414-
``unit-tests.yml``'s own comment (``unit-tests.yml:1260-1270``) records. It
419+
``unit-tests.yml``'s own comment (``unit-tests.yml:1285-1295``) records. It
415420
``needs:`` the gating jobs of ``unit-tests.yml`` -- ``test``, ``integration``,
416421
``engine-tests`` and ``pypcap-parity``, plus ``changes`` and ``project-tests``
417422
for the docs-only path -- and checks each result explicitly
418-
(``unit-tests.yml:1345-1399``). Rulesets match check names literally, with no
423+
(``unit-tests.yml:1370-1424``). Rulesets match check names literally, with no
419424
wildcard, so requiring the matrix cells one by one would mean editing the
420425
ruleset whenever a matrix changes; the aggregate keeps the required list fixed.
421426
It accepts the four legs as ``skipped`` only on a docs-only pull request, where
@@ -424,13 +429,13 @@ instead. The ``Compat`` legs stay required separately because they come from
424429
``python-compatibility.yml``, and a job cannot ``needs:`` a job in another
425430
workflow file. This job runs on Unit Tests' own ``push``/``pull_request``
426431
triggers; the ``gate-only: true`` reusable calls documented above skip it via
427-
``if: ${{ always() && inputs.gate-only != true }}`` (``unit-tests.yml:1348``),
432+
``if: ${{ always() && inputs.gate-only != true }}`` (``unit-tests.yml:1373``),
428433
so it is never produced -- and never expected -- on those paths.
429434

430435
``Compat Python 3.10``-``3.14`` are five ordinary matrix legs, not an
431436
aggregate: ``python-compatibility.yml``'s own comment (``:37-40``) and
432437
``unit-tests.yml``'s matching one for its own, differently-matrixed
433-
``test``/``integration`` jobs (``unit-tests.yml:59-62``, ``unit-tests.yml:256``) both say why 3.15 is
438+
``test``/``integration`` jobs (``unit-tests.yml:59-62``, ``unit-tests.yml:281``) both say why 3.15 is
434439
excluded from every *required* matrix in this repository -- the ruleset's
435440
required-checks list stops at 3.14, so a 3.15 leg cannot gate a merge and is
436441
kept advisory (``continue-on-error: true`` in ``python-compatibility.yml``'s

‎tests/conftest.py‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,51 @@ def pytest_sessionstart(session: 'pytest.Session') -> 'None':
168168
_pin_module_snapshot()
169169

170170

171+
def pytest_sessionfinish(session: 'pytest.Session') -> 'None':
172+
"""Write an xdist worker's coverage data out before the process can die (#1423).
173+
174+
The coverage leg of ``unit-tests.yml`` measures each worker through
175+
coverage's ``patch = ["subprocess"]`` hook, and that measurement is saved by
176+
an :mod:`atexit` handler -- so it is written *after* the worker has already
177+
reported its results and told the controller it is finished. Anything that
178+
kills the process in that window takes the whole worker's data with it, and
179+
takes it silently: pytest has its results, so every test still passes, while
180+
``coverage combine`` merges one file fewer and reports the rest as the whole.
181+
That is #1387, and the guard in ``Report coverage`` turns it into a failed
182+
step rather than a quietly low number -- four times on 2026-10-08.
183+
184+
Saving here closes the window instead of policing it. :func:`cov.save` is
185+
safe to call mid-run (coverage's own long-running-process recipe), and the
186+
later ``atexit`` save appends to the same file, so the data file exists from
187+
this point on and a worker that dies during shutdown costs at most the arcs
188+
executed after this hook.
189+
190+
A no-op outside that one leg: it returns immediately unless the process is
191+
an xdist worker (``workerinput``) whose coverage was auto-started by the
192+
subprocess patch.
193+
194+
Args:
195+
session: The pytest session, for its config's ``workerinput``.
196+
197+
"""
198+
if not hasattr(session.config, 'workerinput'):
199+
return
200+
try:
201+
import coverage # pylint: disable=import-outside-toplevel
202+
except ImportError: # pragma: no cover
203+
return
204+
cov = getattr(coverage.process_startup, 'coverage', None)
205+
if cov is None:
206+
return
207+
try:
208+
cov.save()
209+
except Exception: # pragma: no cover # pylint: disable=broad-except
210+
# Deliberately broad, as in `pytest_sessionstart`: a failure to flush
211+
# early must not fail a run whose tests have already passed. The atexit
212+
# save still gets its turn, and the guard still catches a lost file.
213+
pass
214+
215+
171216
@pytest.fixture(autouse=True)
172217
def restore_module_table() -> 'Iterator[None]':
173218
"""Put the :mod:`pcapkit` region of :data:`sys.modules` back to a known-good state.

‎tests/project/test_coverage_data_guard.py‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,13 @@
2626
import sys
2727
import tempfile
2828
import textwrap
29+
import types
30+
import typing
2931
import unittest
32+
import unittest.mock
33+
34+
if typing.TYPE_CHECKING:
35+
import pytest
3036

3137
ROOT = pathlib.Path(__file__).resolve().parents[2]
3238
WORKFLOW = ROOT / '.github' / 'workflows' / 'unit-tests.yml'
@@ -155,6 +161,61 @@ def test_the_child_lookup_allows_a_padded_pid(self) -> None:
155161
"""The real-log test above finds its child under a pid shorter than the padding."""
156162
self.assertEqual(CHILD.search(_entry(9832, 9800, "['-c']"))[1], '9832')
157163

164+
def test_a_missing_worker_is_named_with_its_fate(self) -> None:
165+
"""#1423: the failure says which process it was and whether its atexit save ran."""
166+
log = (_entry(100, 1, CONTROLLER_CMD) + _entry(101, 100, "['-c']")
167+
+ _entry(102, 100, "['-c']") + _entry(103, 100, "['-c']")
168+
+ ' 102.ab12: atexit: pid: 102, instance: <Coverage data_file=x>\n')
169+
self.write(log, [100, 101])
170+
result = self.run_guard()
171+
self.assertIn("pid 102: cmd ['-c']", result.stdout)
172+
self.assertIn('pid 102: atexit: instance: <Coverage data_file=x>', result.stdout)
173+
self.assertIn('pid 103: no atexit/sigterm entry -- died before saving', result.stdout)
174+
self.assert_fails("no data file from pid ['102', '103']")
175+
176+
def test_the_process_log_is_kept_as_an_artifact(self) -> None:
177+
"""The log is the only record of a lost worker's fate once the runner is gone (#1423)."""
178+
text = WORKFLOW.read_text(encoding='utf-8')
179+
step = text.split('- name: Upload coverage process log', 1)[1]
180+
step = step.split('\n - name:', 1)[0]
181+
self.assertIn('if: ${{ !cancelled() && matrix.coverage }}', step)
182+
self.assertIn(f'path: {LOG}', step)
183+
184+
185+
class WorkerSessionFinishSaveTests(unittest.TestCase):
186+
"""#1423: an xdist worker writes its data at session finish, not only at exit."""
187+
188+
def finish(self, *, worker: bool) -> int:
189+
"""Run the conftest hook under a stand-in auto-started coverage; return its saves."""
190+
import coverage # pylint: disable=import-outside-toplevel
191+
192+
from tests import conftest # pylint: disable=import-outside-toplevel
193+
194+
cov = unittest.mock.Mock()
195+
sentinel = object()
196+
before = getattr(coverage.process_startup, 'coverage', sentinel)
197+
coverage.process_startup.coverage = cov # type: ignore[attr-defined]
198+
try:
199+
config = types.SimpleNamespace(**({'workerinput': {}} if worker else {}))
200+
conftest.pytest_sessionfinish(typing.cast('pytest.Session',
201+
types.SimpleNamespace(config=config)))
202+
finally:
203+
if before is sentinel:
204+
del coverage.process_startup.coverage # type: ignore[attr-defined]
205+
else:
206+
coverage.process_startup.coverage = before # type: ignore[attr-defined]
207+
return int(cov.save.call_count)
208+
209+
@unittest.skipUnless(_has_coverage(), 'needs coverage 7.10+ (`[run] patch`)')
210+
def test_a_worker_saves(self) -> None:
211+
"""The file exists before shutdown, so a death there cannot lose it whole."""
212+
self.assertEqual(self.finish(worker=True), 1)
213+
214+
@unittest.skipUnless(_has_coverage(), 'needs coverage 7.10+ (`[run] patch`)')
215+
def test_the_controller_does_not(self) -> None:
216+
"""The controller saves at its own exit; only workers die after reporting."""
217+
self.assertEqual(self.finish(worker=False), 0)
218+
158219

159220
if __name__ == '__main__':
160221
unittest.main()

‎tests/project/test_coverage_rcfile.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,10 @@ def test_the_xdist_workers_are_measured(self) -> None:
4949
self.assertIs(run.get('parallel'), True)
5050
self.assertIn('subprocess', run.get('patch', ()))
5151

52+
def test_a_sigterm_saves_the_data(self) -> None:
53+
"""A worker SIGTERMed during shutdown otherwise leaves no data file (#1423)."""
54+
self.assertIs(self.rcfile['run'].get('sigterm'), True)
55+
5256
def test_the_core_is_ctrace(self) -> None:
5357
"""sysmon grew one module's peak RSS from 0.40 to 4.17 GiB and killed the runner (#1064)."""
5458
self.assertEqual(self.rcfile['run'].get('core'), 'ctrace')

0 commit comments

Comments
 (0)