Skip to content

test: make the time_limit hang guard robust to slow runners (#1058) - #1061

Merged
JarryShaw merged 2 commits into
mainfrom
test/1058-time-limit-flake
Oct 6, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
test/1058-time-limit-flake

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) — N/A, no pcapkit/ change; isort -l100 clean on touched files
  • A test case covers the change; the 12 affected modules and tests/project pass one module per process (full make test not run)
  • Added a changelog entry — N/A, test-only

What is the purpose of your pull request?

  • test — tests only

Description

Closes #1058. time_limit now defaults to 30 s (was 5 s, which bodies of ~5 ms still overran on a loaded runner). The 19 time_limit(5) sites take the default, and the three CASE_TIMEOUTs rise from 10 s to 30 s, matching EXTRACT_TIMEOUT/DEADLINE. PCAPKIT_TEST_TIMEOUT_SCALE multiplies every deadline, including DEADLINE's subprocess timeout. A malformed value raises; it is not ignored. CI does not set it (this PR doesn't touch .github/), so the 30 s default has to be safe without it. That bound is about 4000× a working body, and still well under #1059's 900 s per-test pytest-timeout.

ABC growth: real, but it does not explain the overrun locally. Over 40 purge_modules(['pcapkit']) cycles, the collections.abc.Mapping subclass tree grows from 950 to 2822–3758, oscillating rather than climbing because gc reclaims old generations. A gc.collect() per cycle holds it at 2822. An uncached isinstance(x, Mapping) scales with it, from 0.8 ms to 3–4 ms. In a 7-module internet slice in one process, the tree reached 5633, yet the slowest time_limit body was 6.9 ms (the issue's test). One module alone: 4.8 ms. The 5 s CI overrun is about 700× that, so runner stall is the likelier cause. UNVERIFIED: not reproduced.

- time_limit defaults to 30s, not 5s; the 19 call sites that passed 5 now take
  the default, and the three CASE_TIMEOUT constants rise from 10 to 30
- PCAPKIT_TEST_TIMEOUT_SCALE multiplies every deadline via scale_timeout,
  including the DEADLINE subprocess timeout in test_extraction_no_eof; a
  malformed value raises instead of being ignored
- TimeLimitTests pins the new default, the scaling, rounding and refusal
@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one test Pull requests that add or correct tests (test: subject prefix) labels Oct 6, 2026
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
@JarryShaw JarryShaw added 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 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on e29a90f14: NEEDS CHANGES (ran on Sonnet; author Opus).

Required. A huge PCAPKIT_TEST_TIMEOUT_SCALE (1e9 or more) leaks the SIGALRM handler. scale_timeout accepts it, and then signal.alarm(seconds) at tests/_support.py:132 raises OverflowError: Python int too large to convert to C int. That line comes after signal.signal(SIGALRM, expire) at :126 but before the try at :134, so the previous handler is never restored. Move the arming inside the try, or clamp/reject oversized values in scale_timeout, and add a test for it.

Optional. tests/project/test_release_gates.py:2177 and :2320 still pass a fixed timeout=10 to subprocesses. These are the last tight bounds that ignore scale_timeout.

Confirmed:

  • A busy loop fails at 30.0 s, and at 3.0 s with scale 0.1.
  • 0, -1, nan, inf and abc raise before arming.
  • Nested deadlines still restore correctly with a scaled value.
  • No 5 s or 10 s time_limit call remains.
  • The setUp env patch can't leak.
  • ci: bound unittest-leg reimport growth and add stall diagnostics (#1052) #1059's pytest-timeout uses thread mode at 900 s, so it doesn't conflict.
  • test_support_helpers 30 passed; test_ipv4_unit 26 passed.

@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 6, 2026
…he try (#1058)

- scale_timeout refuses a deadline scaled past MAX_TIMEOUT (one day); 1e9
  overflowed signal.alarm's C int and 1e308 the float product
- time_limit arms the alarm inside the try, so a failure while arming restores
  the previous handler, and leaves an enclosing alarm pending
- test_release_gates' two subprocess timeouts go through scale_timeout
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on e834a2921: GOOD TO GO (ran on Sonnet; author Opus)

  • scale_timeout refuses a scaled deadline above the 86400 s ceiling before rounding. Scales 1e9, 1e308, nan and inf raise ValueError; exactly 86400 is accepted.
  • signal.alarm is now armed inside the try. With alarm forced to raise, the previous handler came back and an enclosing 50 s alarm was still pending.
  • Against the old helper, the new tests in tests/test_support_helpers.py give 4 failed, 31 passed. At this head the module gives 32 passed and 21 subtests passed, re-run independently.
  • Nit, not blocking: signal.signal(SIGALRM, expire) still runs before the try. Only an exception in that gap (a KeyboardInterrupt, say) could leave expire installed.

@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 6, 2026
@JarryShaw
JarryShaw merged commit 03f3fac into main Oct 6, 2026
74 checks passed
@JarryShaw
JarryShaw deleted the test/1058-time-limit-flake branch October 6, 2026 03:12
@JarryShaw JarryShaw moved this to Done in PyPCAPKit Oct 6, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 6, 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.

tests: 5s wall-clock time_limit flakes under xdist (test_ipv4_unit truncated option)

1 participant