Skip to content

test(support): re-arm time_limit's alarm so a swallowed timeout cannot hang a worker (#1892) - #1893

Merged
JarryShaw merged 1 commit into
mainfrom
test/1892-time-limit-rearm
Oct 17, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
test/1892-time-limit-rearm

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

tests/_support.time_limit armed one SIGALRM and raised TimeoutError once. As #1891 found for the generator's deadline, that single raise can be lost. A finaliser sends it to sys.unraisablehook, and an except OSError swallows it because TimeoutError is an OSError. The body then runs until pytest-timeout's 900s backstop kills the xdist worker.

Fix. The handler re-arms alarm(1) each time it raises. The first thing the exit path does is set a flag that makes any due repeat a no-op. After that it cancels the alarm, restores the previous handler, and restores the outer alarm minus the body's seconds, as before.

Tests. New TimeLimitTests cases cover:

  • the re-arm;
  • a first raise lost in a real __del__, gated with sys.version_info on 3.8+;
  • a first raise caught by except OSError;
  • nothing left armed after a normal, timed-out or exceptional exit;
  • an outer alarm restored after repeats.

Every waiting body gives up after 10s, which includes the two existing unbounded while True: pass loops. Against main's _support.py, 5 of the new tests fail with "did not interrupt the body within 10s" or 5 != 1. The test file passed 20/20 runs.

Not done: sending pytest-timeout's stack dump somewhere visible under xdist. It could not be checked without xdist and pytest-timeout installed here.

Closes #1892

@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 3c38d2690. The re-arm works. A Sonnet cross-review confirmed:

  • the outer alarm is restored correctly after repeats (30 → 28 after a swallowed timeout plus a timeout);
  • the new tests fail against main's _support.py without hanging;
  • no caller catches TimeoutError inside the with;
  • test_tier_guard.py passes.

Required: no test pins the leaving flag. Each of these mutations still passes all 42 tests:

  • removing the handler's if leaving: return;
  • never setting leaving;
  • setting it only after the cancel and restore.

The flag is the exit-path race fix carried over from #1891's review, so it needs a test that fails without it. The reviewer validated one. It patches signal.alarm so that alarm(0) first does os.kill(os.getpid(), signal.SIGALRM), runs with time_limit(5): pass, and then asserts that nothing is left behind. That test passes on head and fails under all three mutations.

Not blocking: a fire between the body ending and the generator resuming, inside contextlib's __exit__, can still raise into the caller. That window cannot be closed in pure Python. Mention it in the docstring.

@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
…t hang a worker (#1892)

- time_limit's handler re-arms the alarm for one second before each raise,
  so a TimeoutError dropped by a finaliser or an `except OSError` is
  followed by another until the body leaves the `with`.
- A flag set first on the way out turns a repeat already due into a no-op;
  the repeat is cancelled and the previous handler and pending alarm are
  restored on every exit, as before. The docstring names the window inside
  contextlib's `__exit__` that pure Python cannot close.
- TimeLimitTests pins the repeat through a finaliser and an `except OSError`,
  that a repeat landing on the way out does nothing, that nothing stays armed
  after any exit, and that an enclosing alarm survives a repeat.
- Every body there that waits for the deadline is bounded and fails by name,
  so a lost deadline fails in seconds instead of hanging the worker.

Closes #1892
@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/1892-time-limit-rearm branch from 3c38d26 to a525629 Compare October 17, 2026 06:49
@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 a525629f1. The Sonnet reviewer re-checked the amend.

  • The new test_a_repeat_due_as_the_block_is_left_does_nothing fails under each leaving mutation: the check removed, the flag never set, and the flag set after the restore. In each case TimeoutError escapes the with and the test fails rather than hangs. It also asserts the signal was actually sent.
  • The docstring's new paragraph on the contextlib window matches what was measured in the first round, and it states no count.
  • tests/test_support_helpers.py passed 5 of 5 runs with 43 passed; test_tier_guard.py passes.

@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
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage: 90.06% (unit tier, Python 3.15, a525629f1, 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 cf7cd1c into main Oct 17, 2026
56 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/1892-time-limit-rearm branch October 17, 2026 13:28
@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(support): time_limit's one-shot alarm can be swallowed and hang an xdist worker

1 participant