Skip to content

test(protocols): keep the #1889 spin's first raise inside its try (#1897) - #1901

Merged
JarryShaw merged 1 commit into
mainfrom
test/1897-swallow-race
Oct 17, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
test/1897-swallow-race

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 17, 2026 •

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

The fix is in the test harness. examples/generators/options.py behaves correctly in both cases below.

  • The CI failure, on CPython 3.13: the backward jump closing _Spin's inline loop falls outside its except TimeoutError range in the exception table. A SIGALRM handled at that jump went straight past the swallow, so the case was still TIMEOUT but swallowed stayed 0. 3.12's table has the same gap, but an alarm handled at that jump there resumes inside the try, so only 3.13 fails naturally; 3.14 and 3.15 cover the jump. The spin now waits in _Spin.wait, called from inside the try.

  • A stall before the try: the deadline armed on entry, so a one-second stall before the spin delivered the raise outside it. spin_roundtrip now holds back the deadline's own first signal.alarm until wait arms it. Re-arms and the final cancel still reach the real alarm.

  • New tests: one raises at the spin's first backward jump through sys.monitoring (CPython 3.12+), and one stalls before the spin. Each fails with its part of the fix reverted.

  • Loops of the target test, 200 runs each:

    Interpreter Before, idle Before, under load After
    3.13.16 19 failed 26 failed 0 failed
    3.14 0 failed 0 failed 0 failed

    tests/project, tests/test_support_helpers.py and the tier guard pass.

Closes #1897

@JarryShaw JarryShaw added test Pull requests that add or correct tests (test: subject prefix) 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
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 61230f43c. The fix holds; one wording change is required. Three places overstate the 3.12 mechanism: the _Spin.wait docstring (:151), the monitoring test's docstring (:351), and this PR's body. The 3.12 exception table does leave the loop-closing jump outside the try. But a naturally handled alarm there is attributed to the jump target, which is inside. In 175 natural first alarms on the old 3.12 spin, 25 were handled at that jump and all 25 were still swallowed. Only a raise injected through sys.monitoring lands on the jump itself.

Everything else the Sonnet cross-review checked holds:

  • 3.13 cause: on the old spin, all 9 alarms handled at offset 292, the closing jump, left swallowed at 0. The other 31 were handled at the call and swallowed.

  • Real re-arm: removing options.py's re-arm signal.alarm(1) fails the swallow test, so the re-arm is still what the test proves.

  • Settle paths: dropping either settle path fails a test.

  • New tests: each fails when its part of the fix is reverted. The monitoring test skips cleanly on 3.8–3.11.

  • Loop counts:

    Tree Python Idle Loaded
    Old 3.13 8 of 50 failed 8 of 50 failed
    New 3.12–3.15 0 failed 0 failed
  • No leaks: after every test, the alarm, the handler and the sys.monitoring tool ids come back as they were.

@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
)

- Wait in `_Spin.wait`, called from inside the `try` that drops a raise. On
  CPython 3.13 the backward jump closing the inline loop sat outside that
  `except`'s range, so a SIGALRM handled there went straight through and
  nothing was dropped: the `0 != 1` on CI.
- Hold the deadline's own `signal.alarm` back until `wait` arms it, through
  `_Spin.alarm` and a new `spin_roundtrip` helper. Armed on entry, a stall
  of a second before the `try` delivered the raise outside it as well.
- `clear` now drops the held deadline rather than cancelling the alarm.
- Check the swallow outcome in one comparison that also shows the status and
  the seconds armed.
- Add a test that raises at the spin's first backward jump through
  `sys.monitoring` (CPython 3.12+), and one that stalls before the spin.

Closes #1897
@JarryShaw
JarryShaw force-pushed the test/1897-swallow-race branch from 61230f4 to a22a43c Compare October 17, 2026 18:25
@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 17, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at a22a43c56. The amend changes only docstrings. With docstrings stripped, the file's AST matches the reviewed head.

A Sonnet re-check found the new wording accurate in the docstrings, the PR body and the commit message:

  • 3.12's table has the gap, but an alarm handled at the jump resumes inside the try.
  • Only 3.13 fails naturally.
  • The monitoring test exercises the gap on 3.12 and 3.13, and is a guard that cannot fail from 3.14.

The re-check confirmed the jump offsets with dis on 3.12.15, 3.13.16 and 3.14.8. The functional findings from the review of 61230f43c still hold.

This PR's Python 3.13 CI leg gates readiness: the re-check's own ad hoc run of the monitoring test on 3.13 hung in a scratch copy and was stopped. Two nits, neither blocking:

  • the _Spin.wait docstring does not say the gap is version-specific;
  • "each fails with its part of the fix reverted" holds for the monitoring test on 3.12 and 3.13 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

Copy link
Copy Markdown
Contributor

Coverage: 90.07% (unit tier, Python 3.15, a22a43c56, 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 2504 70 792 36 96.54%
pcapkit/dumpkit 276 3 100 3 98.40%
pcapkit/foundation 2916 164 1016 34 93.59%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 17411 224 4580 189 98.10%
pcapkit/toolkit 872 177 298 8 77.18%
pcapkit/utilities 522 2 158 4 99.12%
pcapkit/vendor 4431 2341 1018 156 43.64%

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

@JarryShaw
JarryShaw merged commit fcf09cf into main Oct 17, 2026
57 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/1897-swallow-race branch October 17, 2026 22:06
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 18, 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 #1889 swallowed-raise test can see the deadline arrive without the swallow

1 participant