Repository navigation
Foundation: make the sandbox kill helper and docs-example timeout portable to Windows (#302) - #325
Conversation
…imeout portable to Windows (#302) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Opus reviewer (independent review 1 of 2): Claude Opus 5.5 ( Reviewed head Blocking1.
I measured it on this machine (Windows 11, CPython 3.14.3) with a verbatim copy of the Windows branch, the limit lowered to 0.05 s, and a hung body (
With Fix: record the firing in the callback and classify on that record, not on thread liveness. Take a lock so the timer never fires after the body has finished: lock = threading.Lock()
state = {"done": False, "fired": False}
def fire():
with lock:
if state["done"]:
return
state["fired"] = True
_thread.interrupt_main()
timer = threading.Timer(seconds, fire)
timer.start()
try:
yield
except KeyboardInterrupt:
if not state["fired"]:
raise # a real Ctrl-C
raise _Timeout("docs/evaluation.md example exceeded its timeout") from None
finally:
with lock:
state["done"] = True
timer.cancel()Residual window, present both in the PR and in this fix: if the body finishes within microseconds of the deadline, after Do not try to "absorb" an in-flight interrupt in If you prefer, a race-free alternative for the non-POSIX branch is Please also correct the PR description's "a finished body leaves no stray interrupt": it is true after the Suggestions (non-blocking)
Linux unchanged: verified
Checks run (fresh venv, Windows 11, Python 3.14.3,
|
|
Opus reviewer (independent review 2 of 2) — Claude Opus 5.5 ( Reviewed head The BlockingB1. timer = threading.Timer(seconds, _thread.interrupt_main)
...
except KeyboardInterrupt:
if timer.is_alive():
raise # a real Ctrl-C, not our timer
What I measured, on Windows 11 with CPython 3.14.3, importing the PR's own
pytest re-raises The same design has a second, narrower race. There is also a related gap. If SIGINT is ignored in the process ( Change: do not reuse SIGINT. Use a signal that only this helper handles, and an "armed" flag that the armed = True
def _on_timer(_signum, _frame):
if armed:
raise _Timeout("docs/evaluation.md example exceeded its timeout")
previous = signal.signal(signal.SIGBREAK, _on_timer)
timer = threading.Timer(seconds, _thread.interrupt_main, args=(signal.SIGBREAK,))
timer.start()
try:
yield
finally:
armed = False
timer.cancel()
timer.join()
signal.signal(signal.SIGBREAK, previous)I tested this variant on this machine:
It also does not depend on how SIGINT is set up. Suggestions (non-blocking)S1.
That supports the issue's "Linux behaviour unchanged and still tested" with a test rather than by reading the code. (The gap existed before this PR, but the fallback is what makes it worth pinning.) S2. S3. A blocking C call inside the body (for example Checks run (fresh venv, Windows 11, CPython 3.14.3,
|
…l signal (#302 fix round 1) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Opus reviewer (independent review 2 of 2, re-review) — Claude Opus 5.5 ( Reviewed head Scope of this pass:
Previous findingsB1 (Windows timer timeout misreports a timeout as Ctrl-C and can deliver a late interrupt): resolved. I re-ran my probes against the new
S1 (no test pinned the Linux kill signal): resolved.
S2 (stale S3 (document that blocking calls are not interrupted): resolved. The New race-shape test
The test is real evidence for the fix, but only a probabilistic guard against a revert. It also never runs in CI, which is Linux. That is acceptable for a Windows-only helper, so I am not blocking on it. See N1 for a cheap deterministic complement. BlockingNone. Suggestions (non-blocking)N1. def test_windows_time_limit_leaves_a_real_ctrl_c_alone():
with pytest.raises(KeyboardInterrupt):
with _time_limit(10):
_thread.interrupt_main() # SIGINT, as a real Ctrl-C delivers it
for _ in range(10**6):
pass
def test_windows_time_limit_ignores_a_trip_after_the_block():
with _time_limit(10):
pass
_thread.interrupt_main(signal.SIGBREAK) # a timer that fired just as the block ended
for _ in range(10**6):
pass # must not raiseI checked both on this machine. Both pass on the PR's N2. The reflowed docstring lines in Checks run (fresh venv from round 0 on this machine, re-checked out at
|
|
Opus reviewer (independent review 1 of 2, re-review): Claude Opus 5.5 ( Reviewed head Changes since
Findings from round 0Blocking 1 (racy
A real Ctrl-C is never touched. A trip still pending when the handler is restored is also safe. CPython's I re-ran my probes against the real
For comparison, the round-0 code gave a raw Suggestion ( Suggestion (pin
New in this roundNo blocking findings. On the timeout's limit: the Windows Suggestions (non-blocking)
Linux unchanged: verified
Checks run (venv, Windows 11, Python 3.14.3,
|
…ic Windows timeout tests (#302 fix round 2) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Opus reviewer (independent review 2 of 2, re-review 2) — Claude Opus 5.5 ( Reviewed head What changed since
|
| Probe | Result at dc7cfff |
|---|---|
300 × _time_limit(0.01) on while True: pass |
300 _Timeout, 0 KeyboardInterrupt |
| 400 bodies ending within ±1 ms of a 20 ms deadline (any exception after the block counts as a stray) | 400 clean, 0 strays |
SIGINT set to SIG_IGN, 0.5 s limit, 3 s busy body |
_Timeout |
Real Ctrl-C (_thread.interrupt_main()) inside a 10 s block, 100 runs |
100 × KeyboardInterrupt, 0 _Timeout. SIGBREAK is back to SIG_DFL afterwards |
| 2000 fast bodies under a 10 s limit | 0 strays |
time.sleep(3) under 0.5 s |
_Timeout after 3.0 s, as the docstring says |
The three Windows-only tests: do they fail on a broken version?
I ran each test function from the PR's file against the real helper and against three deliberately broken _time_limit variants:
_time_limit used |
…always_raises_timeout_for_a_busy_loop |
…leaves_a_real_ctrl_c_alone |
…ignores_a_trip_after_the_block |
|---|---|---|---|
PR (dc7cfff) |
0/20 failed | pass | pass |
Round-0 is_alive() guess |
2/20 failed (Failed, via pytest.fail) |
pass | pass |
Neither the armed check nor the handler restore |
pass | pass | fails (_Timeout) |
Treats any KeyboardInterrupt in the block as a timeout |
pass | fails (_Timeout) |
pass |
The two deterministic tests each catch the break they target, every time. The busy-loop test is still probabilistic: it caught 2 of 20 runs here and 4 of 20 last round. I said last round that this is acceptable, and the deterministic pair now covers the properties that matter.
Two limits, for the record:
- The late-trip test fails only when both safeguards are gone. Either one alone neutralises a late trip, so that is the strongest pin this test can be.
- All three tests are skipped on Linux CI, because the Windows path they test does not exist there.
Blocking
None.
Suggestions
None new. N1 and N2 are both applied.
Checks run (same fresh venv, re-checked out at dc7cfff, Windows 11, CPython 3.14.3, .[dev], no TYPESAFE_API_KEY)
pytest tests/test_docs_examples.py tests/test_notebook_ci_sandbox.py: 36 passed, 1 skipped (NO_PROCFS). That includes all three Windows_time_limittests, both kill-signal tests and the docs-example executions.pytest(full suite): 5299 passed, 49 skipped, 50 deselected, 31 xfailed, 0 failed (6m02s). The kernel-start flake from my last pass did not recur, and neither did the twosignal.SIGKILLfailures from Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302.ruff check .: All checks passed.ruff format --check .: 397 files already formatted.python tools/render_catalog.py --check: exit 0.python tools/check_hygiene.py: passed, 606 files scanned.python tools/check_recipe_assets.py, the new hygiene-workflow step from Foundation: image-file contract, checker, scope rule and notebook image policy (#307) #322: passed, 461 files underrecipes/checked.python tools/notebook_ci.py fixtures: 51 recipe folders, 0 failed.- Notebook execution: not run. The PR touches no notebook, recipe or fixture, and the executor is unchanged since round 1 (its only code change is
_KILL_SIGNAL). - PR CI (
gh pr checks 325, at review time): all 60 checks pass (Linux).
Acceptance criteria (#302)
-
pytestpasses on Windows from a fresh clone, with no test skipped to hide the platform difference: met; see the pytest lines above. The skips are genuine:- The three new
_time_limittests are skipped only on POSIX, where the Windows path they test does not exist. - The
NO_PROCFSskip is pre-existing and needs a real/proc.
- The three new
- Linux behaviour of
tools/execute_notebook.pyis unchanged and still tested: met._KILL_SIGNAL == signal.SIGKILLis pinned whereverSIGKILLexists, and the kill test asserts the signal actually sent.
OPUS-REVIEW: APPROVE dc7cfff
|
Opus reviewer (independent review 1 of 2, re-review 2): Claude Opus 5.5 ( Reviewed head What changed since
|
| body | _Timeout |
finished normally | raw KeyboardInterrupt / other |
handler leaked | stray after the with |
|---|---|---|---|---|---|
| busy-loop hang, 600 runs | 600 | 0 | 0 | 0 | 0 |
| deadline edge (ends 0-200 µs before the limit), 3000 runs | 3 | 2997 | 0 | 0 | 0 |
time.sleep(0.2), 20 runs |
20 (after the sleep returned, as documented) | 0 | 0 | 0 | 0 |
The real Ctrl-C test checks the property it names: SIGINT is never rebound, and the KeyboardInterrupt propagates through both finally blocks.
Suggestion (non-blocking)
tests/test_docs_examples.py:304-310, test_windows_time_limit_ignores_a_trip_after_the_block: the test passes, but it does not exercise the armed flag. It fires interrupt_main(SIGBREAK) after the with block, when the handler has already been restored to SIG_DFL. Per the docs, interrupt_main does nothing for an unhandled signal, so the test passes whether or not armed exists. Mutation check: I replaced if armed: with if True:, and all 21 tests in the file still passed.
The case armed actually guards is a trip that lands while on_timer is still installed but the body has finished. A fake Timer whose join() calls interrupt_main(SIGBREAK) reproduces that case deterministically. I checked it with a probe in the same shape (monkeypatch threading.Timer; start/cancel no-ops; join fires the trip):
- at
dc7cfff: "late trip ignored; handler restored: True" - with
if armed:mutated toif True:: "late trip RAISED _Timeout"
Either rename the existing test to say what it checks (that a trip after restore is a no-op), or add this fake-Timer test so the flag is pinned. This doesn't block approval: the flag is correct by inspection and by the probes above, and the late-trip window is microseconds wide.
Linux unchanged: verified
The POSIX SIGALRM branch of _time_limit is byte-identical to round 1's. All three added tests are skipif(_HAS_SIGALRM), which is legitimate: the Windows timer branch is unreachable on POSIX. _KILL_SIGNAL is still pinned to SIGKILL by an unskipped test. CI ran at dc7cfff0 (gh run list: CI, Notebooks, Hygiene, Scope all success), and gh pr checks 325 shows 60 of 60 passing:
- Lint and Catalog
- Tests on py3.10 and py3.14
- Hygiene, which now includes the image-asset check
- Fixtures and Scope
- Notebooks discover/execute and all 51
Notebook (…)jobs
Checks run (venv, Windows 11, Python 3.14.3, pip install -e ".[dev]" at this head, no TYPESAFE_API_KEY)
ruff check .: All checks passed.ruff format --check .: 397 files already formatted.python tools/render_catalog.py --check: rc 0.python tools/check_hygiene.py: passed, 606 files.python tools/check_recipe_assets.py(new on main): passed, 461 files.python tools/notebook_ci.py fixtures: 51 recipe folders, 0 failed.- Full
pytest: 5299 passed, 49 skipped, 50 deselected, 31 xfailed, 0 failed (404 s). - Race probes and mutations are described above. Each mutation was applied in the working tree and reverted;
git statusis clean.
Acceptance (#302)
SIGKILLsandbox tests pass on Windows with no skip or loosened assertion: met.- Full pytest on Windows with 0 failures: met.
- Linux behaviour unchanged: met.
- No skip hides a platform difference: met.
- The Windows timeout reports a hang as
_Timeout, leaves a real Ctrl-C alone, and cannot leave a stray interrupt, leak its handler or hang for the pure-Python bodies it guards: met. The blocking-call limit is documented.
OPUS-REVIEW: APPROVE dc7cfff
Orchestrator: proposed foundation-only manual ruling for an immaterial
|
|
Opus reviewer (independent review 1 of 2): immaterial main advance Claude Opus 5.5 ( I checked the delta myself:
I accept the OPUS-REVIEW: APPROVE dc7cfff |
|
Opus reviewer (independent review 2 of 2): immaterial main advance — Claude Opus 5.5 ( I accept the advance of
The delta neither touches nor meaningfully exercises anything this PR changes, so my approval of OPUS-REVIEW: APPROVE dc7cfff |
|
Merge note (orchestrator). This merge departs from the checklist's currency requirement under the foundation-only manual ruling for an immaterial |
Closes #302
What this adds
Two minimal, Windows-only portability fixes. Linux behaviour is unchanged.
tools/execute_notebook.py(CI gate):signal.SIGKILL->_KILL_SIGNAL = getattr(signal, "SIGKILL", signal.SIGTERM). On Linux (CI) this is exactlysignal.SIGKILL, so theos.killcall is identical. The helper only signals anything whenin_an_isolated_pid_namespace()is true (needs/proc, Linux), so the SIGTERM fallback is never used on a real process; it just lets the unit tests (which mockos.kill) run on Windows. No test is skipped or loosened.tests/test_docs_examples.py:test_block_executesandtest_block_four_equivalence_holdsskipped on Windows (noSIGALRM). A_time_limit()context manager keeps theSIGALRMpath byte-for-byte equivalent on POSIX and uses athreading.Timer+_thread.interrupt_mainelsewhere, so these now run on Windows with a 10 s limit. (Superseded by fix round 1 below: the Windows path now uses a dedicatedSIGBREAKhandler.)Not touched: other executor hardening (PR #316's issue).
docs/notebook-ci.mdneeds no change.The one remaining skip in the sandbox tests (
NO_PROCFS, reads real/proc/self/stat) is pre-existing and genuinely Linux-only.Recipe checklist
Skipped: foundation work.
How it was verified
Fresh venv on Windows 11,
pip install -e ".[dev]":pytest: 4937 passed, 49 skipped, 50 deselected, 31 xfailed, 0 failed (previously 2 failures intests/test_notebook_ci_sandbox.py).pytest tests/test_docs_examples.py tests/test_notebook_ci_sandbox.py: 32 passed, 1 skipped (NO_PROCFS).ruff check .: all checks passed.ruff format --check .: 395 files already formatted.execute_notebook.pyis a one-name substitution resolving tosignal.SIGKILLon Linux; Linux CI runs the same sandbox tests. Not run on Linux locally.render_catalog --check,check_hygiene, fixture validator, notebook execution (no recipe, catalog or notebook touched).Fix round 1 (head 1f1c2cd)
Both independent reviews of 4c6dbb3 asked for changes. Resolutions:
timer.is_alive()could not tell the timer's interrupt from a real Ctrl-C, so a few percent of hangs escaped as a bareKeyboardInterrupt._time_limitintests/test_docs_examples.pynow follows review 2's design on Windows: the timer calls_thread.interrupt_main(signal.SIGBREAK), a handler installed only by this helper raises_Timeoutwhile anarmedflag is set, andfinallyclears the flag, cancels and joins the timer, then restores the old handler. A real Ctrl-C (SIGINT) is untouched. No wait-it-out loop. POSIXSIGALRMpath unchanged.test_windows_time_limit_always_raises_timeout_for_a_busy_loopruns 100 limits of 0.01 s on a busy loop and requires every one to raise_Timeout(about 3 s; passed 5 of 5 separate runs). Skipped on POSIX with a stated reason: it exercises theSIGBREAKpath, which exists only on Windows._Timeoutcan still be raised at the edge. This is documented in the_time_limitdocstring. These blocks take milliseconds against a 10 s limit. I did not re-measure it._KILL_SIGNAL; a new test asserts_KILL_SIGNAL == signal.SIGKILLwhereverSIGKILLexists.execute_notebook.pycomment no longer claims the fallback "lets the module import"; it says it lets the helper's unit tests (which mockos.kill) run on Windows.tests/test_docs_examples.pydescribes both timeout paths; the_time_limitdocstring notes the Windows path cannot interrupt a blocking call such astime.sleep.Verification at this head (Windows 11):
ruff check .andruff format --check .clean;render_catalog --checkrc 0;check_hygienepassed; fixture validator 51 recipe folders, 0 failed;tests/test_docs_examples.py+tests/test_notebook_ci_sandbox.py34 passed, 1 skipped (existing/procskip).Full
pyteston Windows: one run, 1 failed, 5077 passed, 49 skipped, 31 xfailed. The failure wastests/test_new_recipe.py::test_a_looping_cell_hits_the_timeout_with_one_line_and_leaves_the_file, a kernel-startup test this PR does not touch. I did not capture its error text; review 2 saw the same test fail once withzmq.error.ZMQError: Address in use, which is the likely cause but is not confirmed for my run. Re-running the whole file passed (84 passed). The orchestrator has filed this flake as a separate issue. I do not have a single clean 0-failure full run at this head.Fix round 2 (head dc7cfff)
Main advanced to 327b0a0 (#322), so the branch took a signed merge of
origin/main. Taken from the approving reviews' non-blocking suggestions, nothing else:_time_limit(Windows path) now nests the try/finally so the previousSIGBREAKhandler is always restored, even if the timer trips between the end of the body andarmed = False.KeyboardInterruptintopytest.fail(...). Added review 2's two deterministic Windows-only tests (a real Ctrl-C passes through; a timer trip after the block has no effect), skipped on POSIX with a stated reason.tests/test_docs_examples.py.Verification (Windows 11): full
pytest5299 passed, 49 skipped, 50 deselected, 31 xfailed, 0 failed, in one run (no #328 kernel flake appeared this time);ruff check .andruff format --checkclean;render_catalog --checkrc 0;check_hygienepassed (606 files);check_recipe_assetspassed (461 files); fixture validator 51 recipe folders, 0 failed; the two touched test files 36 passed, 1 skipped (existing/procskip).🤖 Generated with Claude Code