Skip to content

test(protocols): bound the #1428 deadline test so a lost alarm cannot hang the worker (#1889) - #1891

Merged
JarryShaw merged 1 commit into
mainfrom
test/1889-bounded-deadline-test
Oct 17, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
test/1889-bounded-deadline-test

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

What happened on #1882's job 117297215943. The #1428 deadline test spun until pytest-timeout's 900s backstop ended the xdist worker. The traceback was lost because an xdist worker's stdout is /dev/null. Another test that waits on the tier-guard lock took 882s on the same job, so the spin held that lock the whole time.

Why the alarm can be lost (reproduced locally, not proven on CI). The generator's deadline is a one-shot SIGALRM. If its single TimeoutError is swallowed, nothing fires again. Two things swallow it:

  • a finaliser, which routes the raise to sys.unraisablehook, for example after a long garbage collection;
  • an except OSError, because TimeoutError is an OSError. Extractor.__del__ catches exactly that.

Fix.

  • examples/generators/options.py: _deadline re-arms itself until roundtrip marks the case settled. The option captures regenerate byte-identical.
  • The test's spin gives up after 10s, so a lost alarm fails quickly with a clear message rather than hanging a worker.

Tests. New tests cover a swallowed raise (still recorded as 'TIMEOUT'), the repeat stopping once settled, and a cleared alarm. With the generator fix reverted they fail in about 11s instead of hanging. On main, with a finaliser swallowing the first raise, the CI worker crash reproduces exactly. The file passes 20/20 serially and 20/20 under -n 4 on Python 3.12.

Closes #1889

@JarryShaw JarryShaw added test Pull requests that add or correct tests (test: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at ccc1e469e. The approach is sound, and two small gaps need closing. A Sonnet cross-review confirmed:

  • the lost-raise mechanism hangs on base and is handled on head;
  • no stray alarm or handler is left behind on the TIMEOUT, CONSTRUCT and KeyboardInterrupt paths it ran;
  • the option captures regenerate byte-identical (sha256 of all five matches base);
  • reverting options.py fails the new tests in about 11s rather than hanging;
  • the other option-generator tests and test_tier_guard.py pass.

Required:

  1. Non-timeout exits. settled is set only in the except TimeoutError clauses and after the last step, not on the CONSTRUCT, PARSE or RECONSTRUCT returns. If the alarm fires just before __exit__ cancels it on one of those paths, expire re-arms and the handler is never restored. Injected with signal.raise_signal at the top of __exit__, that left a recurring alarm raising TimeoutError in unrelated code. Base leaks only the handler. Set limit.settled = True first in those except clauses too, or have __exit__ settle before cancelling.
  2. No test pins the settled stores. Removing all of them in roundtrip still passes the suite. Add a deterministic test: fire SIGALRM via signal.raise_signal during exit, check roundtrip still returns its outcome, and check that no alarm or foreign handler is left. Show that it fails without the stores.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026
@JarryShaw JarryShaw removed the review: needs-changes Cross-review at the current head says changes are required; see the verdict comment label Oct 17, 2026
@JarryShaw
JarryShaw force-pushed the test/1889-bounded-deadline-test branch from ccc1e46 to 69ea9b3 Compare October 17, 2026 06:40
@JarryShaw JarryShaw added the review: running A cross-review is in flight against the current head - no verdict yet label Oct 17, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 69ea9b3e6. The amend closes the exit-path window on the CONSTRUCT, PARSE and RECONSTRUCT returns, and the new test pins those stores and the with order. Reordering the with changes no outcome: all 326 outcomes are identical to before, and the option captures are byte-identical.

But a SIGALRM delivered at the very top of _deadline.__exit__, before its first statement, still leaks on two paths:

  • The OK path, which is a regression. Dropping the end-of-body store means settled is False until __exit__ runs.
  • A BaseException such as KeyboardInterrupt, which was already leaking.

On both, TimeoutError escapes roundtrip, an alarm stays pending and the handler is not restored. The new comment's "before anything else, on every way out" is therefore not quite true.

Required:

  • Wrap the with body in try: … finally: limit.settled = True. The reviewer checked this in a scratch copy: all eight ways out come back clean.
  • Add a fire point at the very top of __exit__ to the test, covering OK and the BaseException exit.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026
… hang the worker (#1889)

- examples/generators/options.py: each raise of _deadline re-arms the alarm
  for one more second until the deadline is settled. roundtrip enters it
  after catch_warnings, so it is torn down first, and settles it before
  __exit__ is entered on every way out: a `finally` around the body, and the
  first statement of each `except` clause, since a clause builds its outcome
  before the `finally` runs. __exit__ settles it too, as a backstop. A raise
  landing in a finaliser run by the cyclic collector, or in an
  `except OSError`, is dropped, and the single alarm then left the case
  spinning until pytest-timeout ended the xdist worker.
- tests/protocols/test_option_generator_deadline_scale_1428_unit.py: _Spin
  gives up after SPIN_LIMIT, and the hang test then fails by name instead of
  hanging; new tests pin the repeat, the settled no-op, a lost alarm, SIGALRM
  fired on every way out of roundtrip, and the backstop.
- The option captures regenerate byte-identical.

Closes #1889
@JarryShaw JarryShaw removed the review: needs-changes Cross-review at the current head says changes are required; see the verdict comment label Oct 17, 2026
@JarryShaw
JarryShaw force-pushed the test/1889-bounded-deadline-test branch from 69ea9b3 to 437e5ad Compare October 17, 2026 06:58
@JarryShaw JarryShaw added the review: running A cross-review is in flight against the current head - no verdict yet label Oct 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 437e5ad73. The Sonnet reviewer's third pass found nothing left to change.

  • A SIGALRM handled at the entry to _deadline.__exit__, before its first statement, now leaves nothing behind on all eight ways out, OK and BaseException included: no pending alarm, the handler restored, and no stray TimeoutError.
  • Each guard is pinned. Dropping the finally fails 2 tests, dropping the per-clause stores fails 6, and dropping __exit__'s backstop fails 1.
  • Apart from the try/finally and comment rewording, the generator's syntax tree is unchanged. All 326 outcomes are identical, and the option captures are byte-identical to main.
  • The test file passed 5/5 runs; the other option-generator tests and test_tier_guard.py pass.

Unverified, as the PR says: that the lost raise is what hung CI. That was reproduced locally only.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 17, 2026
@github-actions

github-actions Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 90.06% (unit tier, Python 3.15, 437e5ad73, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 2 20 2 96.77%
pcapkit/const 19595 881 2618 867 92.11%
pcapkit/corekit 2462 69 780 35 96.55%
pcapkit/dumpkit 276 3 100 3 98.40%
pcapkit/foundation 2914 164 1016 34 93.59%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 17406 224 4580 189 98.10%
pcapkit/toolkit 872 177 298 8 77.18%
pcapkit/utilities 512 2 158 4 99.10%
pcapkit/vendor 4429 2341 1018 156 43.62%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 4c91ded into main Oct 17, 2026
86 of 88 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 17, 2026
@JarryShaw
JarryShaw deleted the test/1889-bounded-deadline-test branch October 17, 2026 13:27
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Pull requests that add or correct tests (test: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

test(protocols): the #1428 deadline test can hang and take down its xdist worker

1 participant