Skip to content

Foundation: make the sandbox kill helper and docs-example timeout portable to Windows (#302) - #325

Merged
CompleteDotTech merged 6 commits into
mainfrom
foundation/windows-portable-kill
Oct 21, 2026
Merged

CompleteDotTech merged 6 commits into
mainfrom
foundation/windows-portable-kill

Conversation

@CompleteDotTech

@CompleteDotTech CompleteDotTech commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

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 exactly signal.SIGKILL, so the os.kill call is identical. The helper only signals anything when in_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 mock os.kill) run on Windows. No test is skipped or loosened.
  • tests/test_docs_examples.py: test_block_executes and test_block_four_equivalence_holds skipped on Windows (no SIGALRM). A _time_limit() context manager keeps the SIGALRM path byte-for-byte equivalent on POSIX and uses a threading.Timer + _thread.interrupt_main elsewhere, so these now run on Windows with a 10 s limit. (Superseded by fix round 1 below: the Windows path now uses a dedicated SIGBREAK handler.)

Not touched: other executor hardening (PR #316's issue). docs/notebook-ci.md needs 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 in tests/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.
  • Linux behaviour: diff in execute_notebook.py is a one-name substitution resolving to signal.SIGKILL on Linux; Linux CI runs the same sandbox tests. Not run on Linux locally.
  • Not run: 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:

  • Windows timeout race (review 1 blocking 1, review 2 B1): timer.is_alive() could not tell the timer's interrupt from a real Ctrl-C, so a few percent of hangs escaped as a bare KeyboardInterrupt. _time_limit in tests/test_docs_examples.py now follows review 2's design on Windows: the timer calls _thread.interrupt_main(signal.SIGBREAK), a handler installed only by this helper raises _Timeout while an armed flag is set, and finally clears the flag, cancels and joins the timer, then restores the old handler. A real Ctrl-C (SIGINT) is untouched. No wait-it-out loop. POSIX SIGALRM path unchanged.
  • Regression test for the old race shape: test_windows_time_limit_always_raises_timeout_for_a_busy_loop runs 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 the SIGBREAK path, which exists only on Windows.
  • Residual window (correcting the earlier "no stray interrupt" claim): if a body finishes within microseconds of the deadline, a _Timeout can still be raised at the edge. This is documented in the _time_limit docstring. These blocks take milliseconds against a 10 s limit. I did not re-measure it.
  • Pin the Linux kill signal (review 1 S2, review 2 S1): the kill test now records the signal and asserts it equals _KILL_SIGNAL; a new test asserts _KILL_SIGNAL == signal.SIGKILL wherever SIGKILL exists.
  • Comment fix: the execute_notebook.py comment no longer claims the fallback "lets the module import"; it says it lets the helper's unit tests (which mock os.kill) run on Windows.
  • Docs (review 2 S2, S3): the module docstring of tests/test_docs_examples.py describes both timeout paths; the _time_limit docstring notes the Windows path cannot interrupt a blocking call such as time.sleep.

Verification at this head (Windows 11): ruff check . and ruff format --check . clean; render_catalog --check rc 0; check_hygiene passed; fixture validator 51 recipe folders, 0 failed; tests/test_docs_examples.py + tests/test_notebook_ci_sandbox.py 34 passed, 1 skipped (existing /proc skip).

Full pytest on Windows: one run, 1 failed, 5077 passed, 49 skipped, 31 xfailed. The failure was tests/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 with zmq.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 previous SIGBREAK handler is always restored, even if the timer trips between the end of the body and armed = False.
  • The race-shape test converts a bare KeyboardInterrupt into pytest.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.
  • Rewrapped the two over-100-character module-docstring lines in tests/test_docs_examples.py.

Verification (Windows 11): full pytest 5299 passed, 49 skipped, 50 deselected, 31 xfailed, 0 failed, in one run (no #328 kernel flake appeared this time); ruff check . and ruff format --check clean; render_catalog --check rc 0; check_hygiene passed (606 files); check_recipe_assets passed (461 files); fixture validator 51 recipe folders, 0 failed; the two touched test files 36 passed, 1 skipped (existing /proc skip).

🤖 Generated with Claude Code

CompleteDotTech and others added 2 commits October 20, 2026 18:36
…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>
@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 1 of 2): Claude Opus 5.5 (claude-opus-5-5)

Reviewed head 4c6dbb302ec36fb028b1b04d7dd971df8d7aa1ab against base 83dfc48040736d47e8b83cf3159a198abbe5bd04 (= origin/main). git diff --raw -M origin/main...HEAD: two files, both 100644 -> 100644 modifications (tests/test_docs_examples.py, tools/execute_notebook.py). No mode or type changes, no symlinks or submodules, and nothing outside foundation scope.

Blocking

1. tests/test_docs_examples.py:250-259: the Windows timeout decides "real Ctrl-C vs our timer" with Timer.is_alive(), which is racy. A genuine timeout sometimes surfaces as a raw KeyboardInterrupt, and pytest treats that as a user abort of the whole session.

threading.Timer.run calls interrupt_main() and only afterwards does finished.set() and lets the thread exit. The main thread can take the interrupt while the timer thread is still alive. When that happens, if timer.is_alive(): raise treats our own timer as a real Ctrl-C and re-raises KeyboardInterrupt. pytest then stops the session with !!! KeyboardInterrupt !!! and exit code 2, instead of reporting _Timeout as a failure of that one test. The failure is still loud, so nothing passes silently. But the one job this helper has (report a hang as a test failure) is unreliable, and it is a pattern readers will copy.

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 (while True: pass):

variant _Timeout raw KeyboardInterrupt
PR as written, run 1 296 / 300 4 / 300
PR as written, run 2 596 / 600 4 / 600
flag set by the timer callback (below) 600 / 600 0 / 600

With while True: time.sleep(1) as the body, all 300 of 300 runs gave _Timeout. The race is in the busy, pure-Python case, which is the realistic hang for a doc block.

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 fire() but before the interrupt is delivered, the KeyboardInterrupt lands in contextlib's __exit__ and escapes the with as a raw interrupt. With the body finishing in a 0–200 µs band before the deadline, I measured 2 / 2000 runs for the PR and 6 / 3000 for the fix. In neither variant did an interrupt leak past the end of the with statement into later code (0 strays across all runs). Hang or stray-interrupt behaviour after the block did not occur: no run hung, and timer.cancel() plus the lock leave nothing pending once done is set. These blocks take milliseconds against a 10 s limit, so the residual window is acceptable if a one-line comment states it.

Do not try to "absorb" an in-flight interrupt in finally with a wait loop. I tried that variant, and it hung indefinitely in the near-deadline case.

If you prefer, a race-free alternative for the non-POSIX branch is faulthandler.dump_traceback_later(seconds, exit=True) / faulthandler.cancel_dump_traceback_later(). It never raises into the main thread, so nothing can leak or be misclassified. The cost is that a hang ends the pytest process (with all thread tracebacks dumped) instead of failing one test. I did not test that variant here.

Please also correct the PR description's "a finished body leaves no stray interrupt": it is true after the with statement exits, but not at its exit, per the residual window above.

Suggestions (non-blocking)

  • tools/execute_notebook.py:322-325: the comment says the fallback "lets the module import". The module already imported on Windows, because signal.SIGKILL was only evaluated at call time inside the helper. What failed was the call in the two unit tests. Suggest "lets the helper's unit tests (which mock os.kill) run there".
  • tests/test_notebook_ci_sandbox.py:154 (unchanged) records only PIDs and ignores the signal (lambda pid, _sig: ...). Neither before nor after this PR does any test pin that Linux sends SIGKILL: mutating _KILL_SIGNAL to signal.SIGTERM passes every test. A cheap guard with no skip would be to record _sig and assert sig == getattr(signal, "SIGKILL", signal.SIGTERM). It pins SIGKILL wherever it exists, CI included, and still runs on Windows.

Linux unchanged: verified

  • execute_notebook.py: when the attribute exists, getattr(signal, "SIGKILL", signal.SIGTERM) is signal.SIGKILL, the same object, so the os.kill call is identical. Anything gets signalled only when in_an_isolated_pid_namespace() is true, and that requires /proc and PID 1. All 51 Notebook (<recipe>) jobs at this head (run 39510436519, pull_request, headSha 4c6dbb3) passed, including the pid namespace: active grep, so the kill path ran on Linux with the new name.
  • test_docs_examples.py POSIX branch: same sequence as before: install the SIGALRM handler, alarm(10), body, alarm(0), restore handler. _Timeout propagates unchanged through @contextmanager. In test_block_executes, plt.close("all") still runs after the alarm is cancelled. Linux CI Tests (py3.14) at this head: 5016 passed, 44 skipped, 0 failed. Tests (py3.10) passed too.
  • No skip hides a platform difference: two skipif(not _HAS_SIGALRM) markers were removed and none added. The only skip left in the two touched areas is the pre-existing NO_PROCFS one at tests/test_notebook_ci_sandbox.py:118, which reads the real /proc/self/stat and is genuinely Linux-only. That file is untouched by this PR.

Checks run (fresh venv, Windows 11, Python 3.14.3, pip install -e ".[dev]", no TYPESAFE_API_KEY)

  • ruff check .: All checks passed. ruff format --check .: 395 files already formatted.
  • python tools/render_catalog.py --check: rc 0. python tools/check_hygiene.py: passed, 604 files.
  • python tools/notebook_ci.py fixtures: 51 recipe folders, 0 failed.
  • Full pytest, run 1: 4949 passed, 49 skipped, 31 xfailed, 1 failed: tests/test_new_recipe.py::test_a_failed_start_leaves_no_exit_time_noise, zmq.error.ZMQError: Address in use on the retried kernel's port. The test passed on origin/main and passed 3 / 3 reruns at this head, and nothing in the PR touches kernel start. It is a port-collision flake from other concurrent jobs on this machine, not this change. Full pytest, run 2: 4950 passed, 49 skipped, 50 deselected, 31 xfailed, 0 failed.
  • pytest tests/test_docs_examples.py tests/test_notebook_ci_sandbox.py -rs: 32 passed, 1 skipped (NO_PROCFS). The two Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302 SIGKILL failures are gone, and the two docs-example tests now run on Windows.
  • Offline execution of recipes/01-sentiment-classification from a clean kernel via tools/execute_notebook.py (netguard on PYTHONPATH), then check_notebook_fresh.py: matches a fresh offline run, and the checkout is unchanged. CI executed all 51 notebooks at this head.
  • Timeout race measurements: the scripts above, a verbatim copy of the PR's Windows branch with the limit lowered, run in separate processes.
  • gh pr checks 325: all green (Lint, Catalog, Tests py3.10 and py3.14, Hygiene, Fixtures, Scope, Notebooks discover/execute, and 51 Notebook (…) jobs).

Acceptance (#302)

  • signal.SIGKILL sandbox tests pass on Windows with no skip or loosened assertion: met.
  • Full pytest on Windows with 0 failures: met (run 2). Run 1's single failure is the unrelated zmq port flake described above.
  • Linux behaviour unchanged: met (identical signal object, identical SIGALRM sequence, Linux CI green including the PID-namespace notebook jobs).
  • No skip hiding a platform difference: met.
  • The new Windows timeout reliably reports a hang as a test failure and cannot leak an interrupt or hang: not met. Its Ctrl-C classification is racy (blocking 1). After the fix, the residual microsecond window needs a comment stating it.

OPUS-REVIEW: CHANGES-NEEDED 4c6dbb3

@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 2 of 2) — Claude Opus 5.5 (claude-opus-5-5)

Reviewed head 4c6dbb302ec36fb028b1b04d7dd971df8d7aa1ab against base 83dfc48040736d47e8b83cf3159a198abbe5bd04 (issue #302). I did not read the other review before writing this one.

The execute_notebook.py change is correct. I am asking for changes for one reason: on Windows, the new timer-thread timeout in tests/test_docs_examples.py misreports a real timeout as a Ctrl-C. When it does, pytest aborts the whole session. The interrupt can also land after the block has finished, though I could not trigger that. The fix is small, and I tested one below.

Blocking

B1. tests/test_docs_examples.py:250-259: on Windows, about 2% of real timeouts abort the whole pytest session instead of failing the test.

timer = threading.Timer(seconds, _thread.interrupt_main)
...
except KeyboardInterrupt:
    if timer.is_alive():
        raise  # a real Ctrl-C, not our timer

timer.is_alive() is the wrong test for "did our timer fire". Timer.run calls interrupt_main(), then finished.set(), and only after that does the thread exit. The main thread can take the GIL and handle the pending KeyboardInterrupt between interrupt_main() and the thread's exit. In that window is_alive() is still True, so a timeout is re-raised as a bare KeyboardInterrupt.

What I measured, on Windows 11 with CPython 3.14.3, importing the PR's own _time_limit from tests/test_docs_examples.py, over 300 runs of with _time_limit(0.01): while True: pass:

  • 293 runs raised _Timeout.
  • 7 runs raised KeyboardInterrupt.

pytest re-raises KeyboardInterrupt from a test call and stops the session. I confirmed this on this machine: a test that raises KeyboardInterrupt gives !!! KeyboardInterrupt !!! and no tests ran, and the tests after it never run. So a hanging docs example on Windows sometimes appears as "interrupted", not as a failed test_block_executes[N], and the rest of the suite's results are lost.

The same design has a second, narrower race. timer.cancel() does not stop a Timer that has already passed its finished.is_set() check. If a body finishes within microseconds of the deadline, interrupt_main() still runs after the with has exited. The KeyboardInterrupt then lands in the test's own code, its finally, or pytest teardown, and aborts a session that was passing. I did not trigger this in 400 runs placed within ±1 ms of a 20 ms deadline. The bodies run in about 1 s against a 10 s limit, so it is improbable in practice, but nothing prevents it.

There is also a related gap. If SIGINT is ignored in the process (SIG_IGN), _thread.interrupt_main() does nothing (documented for 3.10+). The timeout then silently stops existing. I measured this too: with SIGINT ignored, a 0.5 s limit let the body run for its full 3.0 s.

Change: do not reuse SIGINT. Use a signal that only this helper handles, and an "armed" flag that the finally clears before it cancels and joins the timer. Then a late trip is a no-op whether it is handled before or after the previous handler is restored, because a non-callable SIG_DFL is skipped. A real Ctrl-C passes through untouched, with no guessing. The non-SIGALRM branch only runs on Windows, so signal.SIGBREAK is available:

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:

  • 300 of 300 runs raised _Timeout, with 0 KeyboardInterrupt.
  • 400 runs placed within ±1 ms of the deadline gave 398 clean, 2 _Timeout inside the block, and 0 stray interrupts after it.

It also does not depend on how SIGINT is set up. interrupt_main(signum) exists from 3.10, the package floor. If you keep SIGINT instead, at minimum replace is_alive() with an Event that the timer callback sets before it calls interrupt_main, and join() the timer in finally. That fixes the misclassification but not the late-interrupt race.

Suggestions (non-blocking)

S1. tools/execute_notebook.py:326 / tests/test_notebook_ci_sandbox.py:154,184: no test pins the signal sent on Linux. Both kill tests discard the signal argument (lambda pid, _sig: ...). The new getattr(signal, "SIGKILL", signal.SIGTERM) adds a way to break Linux silently that signal.SIGKILL did not have: a typo in the attribute string would send SIGTERM in CI, and every test would still pass. Two changes would close this:

  • In test_kills_every_non_zombie_process_and_returns_once_none_remain, record the signal and assert it is execute_notebook._KILL_SIGNAL.
  • Add assert execute_notebook._KILL_SIGNAL == signal.SIGKILL under if hasattr(signal, "SIGKILL").

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. tests/test_docs_examples.py:20: the module docstring still says the blocks are measured "in a SIGALRM timeout". It should mention the Windows timer path too.

S3. A blocking C call inside the body (for example time.sleep(3) under a 0.5 s limit) is not interrupted on Windows. _Timeout arrives only when the call returns (measured: 3.0 s). That is fine for these pure-Python blocks. One clause in the _time_limit docstring would stop someone reusing it for I/O-bound code without knowing.

Checks run (fresh venv, Windows 11, CPython 3.14.3, pip install -e ".[dev]", no TYPESAFE_API_KEY)

  • pytest (full suite): 4950 passed, 49 skipped, 50 deselected, 31 xfailed, 0 failed (8m22s, with nothing else running on the machine). The two signal.SIGKILL failures from Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302 are gone, and test_docs_examples.py::test_block_executes[*] and test_block_four_equivalence_holds now run and pass on Windows. Two earlier full runs did not come out clean, and neither problem was in code this PR touches:
    • Run 1 ran alongside my CPU-heavy race probes. It hung around test 1504, in the kernel-spawning tests in tests/test_new_recipe.py, and I stopped it after 30 min.
    • Run 2 failed one test, test_new_recipe.py::test_a_looping_cell_hits_the_timeout_with_one_line_and_leaves_the_file, with zmq.error.ZMQError: Address in use during kernel startup. The most likely cause is a port collision with run 1's orphaned kernels or another agent's pytest on this machine. That test passes on its own (12 s), and run 3 is clean. This is not a finding against this PR, but the zmq port-collision flake on Windows may be worth an issue of its own.
  • pytest tests/test_docs_examples.py tests/test_notebook_ci_sandbox.py: 32 passed, 1 skipped. The one skip is test_process_state_of_self_is_a_recognisable_running_state, which reads the real /proc/self/stat, and that file does not exist on Windows.
  • ruff check .: All checks passed. ruff format --check .: 395 files already formatted.
  • python tools/render_catalog.py --check: exit 0. python tools/check_hygiene.py: passed, 604 files scanned.
  • python tools/notebook_ci.py fixtures: 51 recipe folders, 0 failed.
  • Notebook execution: not run. No notebook, recipe, fixture or catalog file is touched. The executor change is a no-op off Linux, and the Linux notebook jobs exercise it in CI.
  • Scope: git diff --raw -M origin/main...HEAD lists only tests/test_docs_examples.py and tools/execute_notebook.py, both 100644 -> 100644 M. No symlink, submodule, or mode or type change.
  • POSIX path of _time_limit: same calls in the same order as before (signal.signal, alarm(N), body, alarm(0), restore). In test_block_executes, plt.close("all") still runs in an outer finally. The two removed skipifs were Windows-only, so Linux CI is unaffected.
  • execute_notebook.py: getattr(signal, "SIGKILL", signal.SIGTERM) resolves to signal.SIGKILL wherever it exists. os.kill is reached only after in_an_isolated_pid_namespace(), which needs PID 1 plus /proc/self, so the fallback can never signal a real process on Windows. No test was skipped or loosened.
  • Race probes are listed under B1 above.
  • PR CI (gh pr checks 325, at review time): all 60 checks pass. Those checks run on Linux, so they cannot exercise the Windows branch B1 is about.

Acceptance criteria (#302)

  • pytest passes on Windows from a fresh clone, with no test skipped to hide the platform difference: met (0 failed; see above). The two docs-example tests that were skipped on Windows now run.
  • Linux behaviour of tools/execute_notebook.py is unchanged and still tested: unchanged (it is the same signal.SIGKILL) and the existing tests still run there. See S1 for the signal not being asserted.

The only blocker is B1. It is not in the issue's criteria, but it is new code in this PR, and it would be copied as the Windows timeout pattern.

OPUS-REVIEW: CHANGES-NEEDED 4c6dbb3

CompleteDotTech and others added 2 commits October 20, 2026 19:43
…l signal (#302 fix round 1)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 2 of 2, re-review) — Claude Opus 5.5 (claude-opus-5-5)

Reviewed head 1f1c2cdb1dd5ee358f09217c84ab17ffca691be0 against base 7fa806ea2e588ff3db24063d05abb002c3eee3c5. Previous review: 6304309335 at 4c6dbb30. I did not read the other reviewer's comments first.

Scope of this pass:

  • Everything changed since 4c6dbb30: the fix commit 1f1c2cd, plus the signed merge 0994181 of origin/main.
  • Apart from the merge, the PR diff against the new base is still only three files, all 100644 -> 100644 M: tests/test_docs_examples.py, tests/test_notebook_ci_sandbox.py and tools/execute_notebook.py.
  • The other six files in git diff 4c6dbb30 HEAD (CONTRIBUTING.md, docs/recipe-template.md, docs/simulation.md, src/jev_cookbook/simulation.py, tests/test_recipe_guard.py, tests/test_simulation.py) come from Foundation: shared review-queue/action-log fill helpers and a side-effect invocation guard (#210, #212) #319 on main. None of them appears in the PR's own diff.
  • Both new commits are signed (G).

Previous findings

B1 (Windows timer timeout misreports a timeout as Ctrl-C and can deliver a late interrupt): resolved. _time_limit now uses the dedicated SIGBREAK handler with an armed flag. The finally clears armed, then calls cancel() and join(), then restores the previous handler, in that order. That is what I asked for. A late trip is either handled while the handler is disarmed, which is a no-op, or after restore, where SIG_DFL is not callable and is skipped. armed is a closure cell, so the finally's assignment is what on_timer reads.

I re-ran my probes against the new _time_limit, imported from the PR's file, on Windows 11 with CPython 3.14.3:

Probe Round 0 (4c6dbb30) Round 1 (1f1c2cd)
300 × _time_limit(0.01) on while True: pass 293 _Timeout, 7 KeyboardInterrupt 300 _Timeout, 0 KeyboardInterrupt
400 bodies ending within ±1 ms of a 20 ms deadline, any exception after the block counted as a stray 0 strays observed (race open in principle) 400 clean, 0 strays (closed by design)
SIGINT set to SIG_IGN, 0.5 s limit, 3 s busy body no timeout, the body ran its full 3.0 s _Timeout
Real Ctrl-C (_thread.interrupt_main(), SIGINT) 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 0 strays

S1 (no test pinned the Linux kill signal): resolved.

  • test_kills_every_non_zombie_process_and_returns_once_none_remain now records (pid, sig) and asserts that each kill used execute_notebook._KILL_SIGNAL.
  • The new test_the_kill_signal_is_sigkill_wherever_it_exists asserts _KILL_SIGNAL == signal.SIGKILL wherever SIGKILL exists, which includes Linux CI. A typo in the getattr string, or a changed fallback, now fails there.
  • On Windows the second test passes without asserting anything, and that is correct, because there is no SIGKILL there to compare against.
  • tools/execute_notebook.py changed only in its comment.

S2 (stale SIGALRM-only wording in the module docstring): resolved. Lines 20-22 now describe both paths.

S3 (document that blocking calls are not interrupted): resolved. The _time_limit docstring says so. I measured it again: time.sleep(3) under a 0.5 s limit raises _Timeout only after 3.0 s, which matches the docstring.

New race-shape test

test_windows_time_limit_always_raises_timeout_for_a_busy_loop (100 × 0.01 s busy loop, skipped where SIGALRM exists) passes on the new code. I also mutation-checked it: I swapped the round-0 is_alive() implementation back in and ran the test 20 times. It caught the regression in only 4 of 20 runs on this machine, which was mostly idle.

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.

Blocking

None.

Suggestions (non-blocking)

N1. tests/test_docs_examples.py: add two deterministic Windows-only checks next to the probabilistic one. Each pins one property of the design without relying on timing:

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 raise

I checked both on this machine. Both pass on the PR's _time_limit. A mutant with neither the armed check nor the handler restore fails the late-trip test with _Timeout. Either safeguard alone is enough to neutralise a late trip, so the test fails only when both are gone.

N2. The reflowed docstring lines in tests/test_docs_examples.py are 146 characters (line 22) and 102 characters (line 20), against the repository's 100. ruff does not flag docstrings, so this is cosmetic.

Checks run (fresh venv from round 0 on this machine, re-checked out at 1f1c2cd, Windows 11, CPython 3.14.3, pip install -e ".[dev]", no TYPESAFE_API_KEY)

  • pytest tests/test_docs_examples.py tests/test_notebook_ci_sandbox.py (this PR's tests): 34 passed, 1 skipped (NO_PROCFS). That includes the two docs-example tests, the new race-shape test and both kill-signal tests.

  • pytest (full suite), run twice at this head:

    • Run A: 5 failed, 5073 passed, 49 skipped, 50 deselected, 31 xfailed (12m57s).
    • Run B: 2 failed, 5076 passed, 49 skipped, 50 deselected, 31 xfailed (12m27s).

    I do not count these as a finding against this PR, for these reasons:

    • The failures are kernel-start failures. All seven are tests that start a Jupyter kernel through tools/execute_notebook.py: four in test_new_recipe.py, two in test_notebook_ci_netguard.py, one in test_style_notebook_display.py. The errors were ZMQError: Address in use, Kernel died before replying to kernel_info and Kernel didn't respond in 180 seconds. This matches the port-clash flake the orchestrator says is tracked separately.
    • A different set failed each time, and every failed test passes when re-run on its own (6 passed in 52 s, then 2 passed in 23 s).
    • This PR cannot cause it. The PR's only executor change is _KILL_SIGNAL. That code is reached only inside a Linux PID namespace and is never executed on Windows, and this round's executor diff changes a comment only.
    • The same executor code was clean on Windows in round 0. The executor is identical there, and its full run was 4950 passed, 0 failed, with nothing else running on the machine. The full suite took about 13 min this time against 8 min then, which shows the machine was under heavier load during this pass.

    The two signal.SIGKILL failures that Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302 is about did not occur in any run.

  • ruff check .: All checks passed. ruff format --check .: 395 files already formatted.

  • python tools/render_catalog.py --check: exit 0. python tools/check_hygiene.py: passed, 604 files scanned. 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 change is comment-only this round.

  • Race probes and the mutation check are listed above.

  • PR CI (gh pr checks 325, at review time): all 60 checks pass (Linux).

Acceptance criteria (#302)

  • pytest passes on Windows from a fresh clone, with no test skipped to hide the platform difference: met for everything this PR owns. Round 0's full run had 0 failures. At this head, the only failures were the load-dependent kernel-start flake described above. The two docs-example tests run on Windows. The new race-shape test is skipped only on POSIX, where the Windows path it tests does not exist. The NO_PROCFS skip is pre-existing and genuine.
  • Linux behaviour of tools/execute_notebook.py is unchanged and still tested: met, and the kill signal is now pinned by a test.

OPUS-REVIEW: APPROVE 1f1c2cd

@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 1 of 2, re-review): Claude Opus 5.5 (claude-opus-5-5)

Reviewed head 1f1c2cdb1dd5ee358f09217c84ab17ffca691be0 against base 7fa806ea2e588ff3db24063d05abb002c3eee3c5 (= origin/main). Previous review: comment 6304216055 at 4c6dbb30. I did not read the other reviewer's comments.

Changes since 4c6dbb30:

  • Signed merge 0994181 of origin/main. git diff 7fa806e 0994181 touches only this PR's two original files, so the merge resolved cleanly and changed nothing else.
  • Signed commit 1f1c2cd: the SIGBREAK timer, a pinned kill signal, and a comment fix.
  • git diff --raw -M origin/main...HEAD: three files, all 100644 -> 100644 modifications: tests/test_docs_examples.py, tests/test_notebook_ci_sandbox.py, tools/execute_notebook.py. No mode or type changes, symlinks or submodules.

Findings from round 0

Blocking 1 (racy is_alive() classification, raw KeyboardInterrupt on a real hang): resolved. The Windows branch no longer goes through SIGINT or KeyboardInterrupt:

  • It installs its own SIGBREAK handler and fires it with _thread.interrupt_main(signal.SIGBREAK).
  • The handler raises _Timeout only while armed is set.
  • finally clears armed, cancels and joins the timer, then restores the old handler.

A real Ctrl-C is never touched. A trip still pending when the handler is restored is also safe. CPython's signal.signal runs pending handlers before swapping, so the trip reaches our disarmed handler and does nothing. If it somehow arrived later, a SIG_DFL/SIG_IGN handler is skipped for a simulated signal.

I re-ran my probes against the real _time_limit, imported from tests/test_docs_examples.py at this head, on Windows 11 / CPython 3.14.3 with the limit at 0.05 s. After each run I checked whether SIGBREAK was still bound to our handler, then spun 60 ms to catch any late interrupt:

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 12 2988 0 0 0
time.sleep(0.2), 20 runs 20 (after the sleep returned) 0 0 0 0

For comparison, the round-0 code gave a raw KeyboardInterrupt in 4 of 600 busy runs and 13 of 3000 edge runs. The edge timeouts above are correct: those bodies did reach the limit. They now surface as _Timeout, not as an interrupt that aborts the pytest session.

Suggestion (execute_notebook.py comment wording): resolved.

Suggestion (pin SIGKILL): resolved.

  • test_kills_every_non_zombie_process_and_returns_once_none_remain now records (pid, sig) and asserts _KILL_SIGNAL for both PIDs.
  • test_the_kill_signal_is_sigkill_wherever_it_exists asserts _KILL_SIGNAL == signal.SIGKILL wherever SIGKILL exists, with no skip.
  • Changing _KILL_SIGNAL to SIGTERM on Linux now fails CI.

New in this round

No blocking findings.

On the timeout's limit: the Windows SIGBREAK handler runs between bytecodes, so a body blocked in a C call (time.sleep, I/O) gets _Timeout only once the call returns. Under round 0's SIGINT approach, time.sleep was woken early; under this design it is not. This is stated in both the docstring and the module docstring, and I confirmed it above (the sleep body timed out on return). It is acceptable here: these doc blocks are pure Python, offline and millisecond-scale. On POSIX, SIGALRM behaviour is unchanged.

Suggestions (non-blocking)

  1. tests/test_docs_examples.py:268-273: restore the old handler even if the trip lands at the very top of finally. If the trip lands after the body's last bytecode but before armed = False runs, _Timeout is raised from inside finally. cancel/join/signal.signal(..., previous) are then skipped, and on_timer stays installed for SIGBREAK, still armed. The classification is still _Timeout, which is correct. The only effect is that a later real Ctrl-Break raises _Timeout instead of the default action. I saw 0 leaks in 3600 runs, so the window is tiny. A nested try: ... finally: armed = False inside an outer finally that does the cancel, join and restore would close it.
  2. tests/test_docs_examples.py:279-285: the new regression test catches the round-0 race only some of the time, and as a session abort rather than a test failure. I put the old is_alive branch back into the file and ran this test 6 times. It passed 4 times; the other 2 runs ended with pytest's "no tests ran", because the raw KeyboardInterrupt aborted the session. I restored the file afterwards. Wrapping the loop body in except KeyboardInterrupt: pytest.fail("timer escaped as KeyboardInterrupt") would make a regression fail this one test. The skip on POSIX is legitimate: that branch is unreachable there, and the SIGALRM path is covered by the two docs-example tests.
  3. tests/test_docs_examples.py:20-22: two module-docstring lines are 102 and 146 characters long; the file's own limit is 100. ruff doesn't flag them, because E501 is not selected. Please rewrap them to match the rest of the docstring.

Linux unchanged: verified

  • _KILL_SIGNAL is signal.SIGKILL on Linux, and the new test now pins that.
  • The POSIX SIGALRM branch of _time_limit is byte-identical to round 0's: handler, alarm(seconds), body, alarm(0), restore.
  • No new skip touches Linux behaviour. The only added marker skips the Windows-only test on POSIX, where its code path cannot run. The pre-existing NO_PROCFS skip is unchanged.
  • CI at this head: gh pr checks 325 is all green. That covers Lint, Catalog, Tests on py3.10 and py3.14, Hygiene, Fixtures, Scope, Notebooks discover/execute and all 51 Notebook (…) jobs, which run the kill helper in the PID namespace.

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 .: 395 files already formatted.
  • python tools/render_catalog.py --check: rc 0. python tools/check_hygiene.py: passed, 604 files. python tools/notebook_ci.py fixtures: 51 recipe folders, 0 failed.
  • Full pytest at this head:
    • Final run: 5078 passed, 49 skipped, 50 deselected, 31 xfailed, 0 failed (367 s).
    • Earlier runs: two runs under heavy machine load (755-780 s, with my CPU-bound probe running alongside in one of them) each had 4 kernel-start failures in tests/test_new_recipe.py, plus one in tests/test_notebook_ci_netguard.py.
    • Those failures are load flakes, not this PR. All of those tests passed together in isolation (test_new_recipe.py + test_notebook_ci_netguard.py: 102 passed, 1 skipped; with the sandbox and docs-example tests too, 118 passed, 1 skipped). The PR touches neither kernel start nor netguard. This matches the separately tracked flake.
  • Full pytest at base 7fa806e for comparison: 2 failed (exactly the two Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302 SIGKILL tests), 5065 passed, 58 skipped. So the PR fixes the Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows #302 failures and runs 9 more tests on Windows: the 8 executed doc blocks plus the block-4 equivalence test, which were skipped before. The new Windows-only test explains the rest of the count difference.
  • pytest tests/test_docs_examples.py tests/test_notebook_ci_sandbox.py tests/test_new_recipe.py -rs: 118 passed, 1 skipped (NO_PROCFS).
  • Race probes and mutation check: described above. The probe imports the real helper; the mutation was applied to the working tree and reverted (git status clean).

Acceptance (#302)

  • SIGKILL sandbox tests pass on Windows with no skip or loosened assertion: met. The assertion is now stronger: it checks the signal.
  • Full pytest on Windows with 0 failures: met (final run). The other runs' failures are the tracked kernel-start load flake, unrelated to this change.
  • Linux behaviour unchanged: met.
  • No skip hiding a platform difference: met.
  • The new Windows timeout cannot leave a stray interrupt or hang, and reports a hang as a test failure: met for the pure-Python bodies it guards. Measured above: 0 raw interrupts, 0 strays, 0 leaked handlers. Its blocking-call limit is documented. Suggestion 1 closes a residual microsecond window for handler restoration that never occurred in 3600 runs.

OPUS-REVIEW: APPROVE 1f1c2cd

CompleteDotTech and others added 2 commits October 20, 2026 21:23
…ic Windows timeout tests (#302 fix round 2)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 2 of 2, re-review 2) — Claude Opus 5.5 (claude-opus-5-5)

Reviewed head dc7cfff077489515a552745cb604baec0e09ee0c against base 327b0a0ed5da928b898fb27d886316c728d266a9. Previous reviews: 6304309335 at 4c6dbb30 and 6305436911 at 1f1c2cdb. I did not read the other reviewer's comments first.

What changed since 1f1c2cdb

  • ef62125 (signed G): merge of origin/main at 327b0a0 (Foundation: image-file contract, checker, scope rule and notebook image policy (#307) #322). git diff 327b0a0 ef62125 --stat shows only this PR's three files differing from main, so the merge brought nothing else in. The four lines Foundation: image-file contract, checker, scope rule and notebook image policy (#307) #322 added to .github/workflows/hygiene.yml (the check_recipe_assets step) arrive unchanged from main.
  • dc7cfff (signed G): tests/test_docs_examples.py only. Four changes:
    1. The docstring is rewrapped. My N2 is resolved: no line in the file is over 100 characters now.
    2. The _time_limit Windows branch is split into nested try/finally blocks. The inner finally clears armed, then calls cancel() and join(). The outer finally always restores the previous SIGBREAK handler.
    3. The busy-loop test now catches a KeyboardInterrupt and calls pytest.fail.
    4. My two deterministic tests from N1 are added: Ctrl-C passthrough, and a trip after the block.
  • The PR diff against the new base (git diff --raw -M origin/main...HEAD) is still exactly tests/test_docs_examples.py, tests/test_notebook_ci_sandbox.py and tools/execute_notebook.py, all 100644 -> 100644 M. There is no symlink, submodule, or mode or type change.

Nested try/finally

This is a real improvement over round 1. A trip can land in the last moment of the inner finally, before armed = False, which means the body did run out its time. In round 1, the _Timeout that trip raised skipped the rest of the single finally, so the handler was never restored. A Python SIGBREAK handler with armed still True stayed installed for the rest of the process.

Now the outer finally restores the handler regardless. Skipping cancel()/join() in that case is harmless, because the timer has already fired and its thread ends on its own. The same holds if a real Ctrl-C interrupts timer.join(). I found no new race.

pytest.fail in the busy-loop test

Also an improvement. If the round-0 regression comes back, the test now fails like a normal test instead of aborting the session. I checked this directly: against the round-0 is_alive() implementation, the test now ends in pytest's Failed, not a bare KeyboardInterrupt.

Race probes, re-run against the new _time_limit (Windows 11, CPython 3.14.3, imported from the PR's file)

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_limit tests, 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 two signal.SIGKILL failures 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 under recipes/ 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)

  • pytest passes 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_limit tests are skipped only on POSIX, where the Windows path they test does not exist.
    • The NO_PROCFS skip is pre-existing and needs a real /proc.
  • Linux behaviour of tools/execute_notebook.py is unchanged and still tested: met. _KILL_SIGNAL == signal.SIGKILL is pinned wherever SIGKILL exists, and the kill test asserts the signal actually sent.

OPUS-REVIEW: APPROVE dc7cfff

@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 1 of 2, re-review 2): Claude Opus 5.5 (claude-opus-5-5)

Reviewed head dc7cfff077489515a552745cb604baec0e09ee0c against base 327b0a0ed5da928b898fb27d886316c728d266a9 (= origin/main). Previous review: comment 6305666977 at 1f1c2cdb. I did not read the other reviewer's comments.

What changed since 1f1c2cdb

  • ef62125, signed merge of origin/main (Foundation: image-file contract, checker, scope rule and notebook image policy (#307) #322): git diff 327b0a0 ef62125 touches only this PR's three files. git diff 1f1c2cdb ef62125 on those files is empty. The merge is clean, and Foundation: image-file contract, checker, scope rule and notebook image policy (#307) #322's changes (check_recipe_assets.py, hygiene.yml) do not overlap with this PR.
  • dc7cfff, signed: only tests/test_docs_examples.py changes.
    • _time_limit now uses a nested try/finally. The inner block clears armed, cancels and joins the timer. The outer block always restores the previous SIGBREAK handler.
    • The busy-loop test turns a bare KeyboardInterrupt into pytest.fail.
    • Two new Windows-only tests: one checks that a real Ctrl-C passes through, the other that a trip after the block has no effect.
    • Two docstring lines are rewrapped.
    • Nothing else changes.
  • Full diff against origin/main: git diff --raw -M origin/main...HEAD shows three 100644 -> 100644 modifications: tests/test_docs_examples.py, tests/test_notebook_ci_sandbox.py, tools/execute_notebook.py. No mode or type changes, symlinks or submodules. tools/execute_notebook.py and tests/test_notebook_ci_sandbox.py are unchanged from what I approved at 1f1c2cdb.

Blocking

None.

Round-1 suggestions

  1. Restore the old handler even if the trip lands at the top of finally: resolved. If _Timeout lands in the inner finally before armed = False, the outer finally still runs signal.signal(SIGBREAK, previous). In that case the timer has already fired, so skipping cancel/join is harmless.
  2. Regression test catches the old race only as a session abort: resolved. I put the old is_alive branch back and ran -k windows 5 times. It produced 1 real FAILED and 4 passes. It no longer aborts the session. Detection is still probabilistic, roughly 1 in 5 runs, which is fine as a canary next to the deterministic tests.
  3. Docstring lines over 100 characters: resolved. No line in the diff exceeds 100 characters. The two remaining long lines, tests/test_notebook_ci_sandbox.py:242 and tools/execute_notebook.py:194, are pre-existing and outside this diff.

Race probes, re-run against the real _time_limit at dc7cfff

These were run on Windows 11 / CPython 3.14.3 with the limit at 0.05 s, importing _time_limit from the test module. After each run I checked the SIGBREAK handler, then spun 60 ms to catch any late interrupt.

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 to if 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 status is clean.

Acceptance (#302)

  • SIGKILL sandbox 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

@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Orchestrator: proposed foundation-only manual ruling for an immaterial main advance

Per docs/merge-readiness.md § manual decision for an immaterial main advance and #80 ledger update 6.

  • Reviewed head: dc7cfff077489515a552745cb604baec0e09ee0c, approved by both independent reviewers (6306004727, 6305924651) against base 327b0a0.
  • Current main pinned: e998eb59d78ee06b32c8346c9932a09e6605a45e (Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321). Its CI, Hygiene and Notebooks are all success.
  • Delta 327b0a0..e998eb5: docs/backends.md, docs/vision.md, src/jev_cookbook/backends.py, src/jev_cookbook/vision.py, tests/test_multimodal_backends.py, tests/test_vision.py. No workflow, no required-check change.
  • Effect on this PR: none of these files is touched by this PR. tests/test_docs_examples.py executes only docs/evaluation.md (unchanged by the delta). tools/execute_notebook.py doesn't import vision or backends. Required checks: the delta changes no check definition. Every required check is success on this head and on current main; Scope (recipe pull requests) is head-only.

The merge needs each reviewer's explicit acceptance of this delta on the same head. If either reviewer doesn't accept, the branch takes a signed integration and is reviewed again.

@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 1 of 2): immaterial main advance

Claude Opus 5.5 (claude-opus-5-5). Head dc7cfff077489515a552745cb604baec0e09ee0c, which I approved in 6306004727 against base 327b0a0. Current main is e998eb59d78ee06b32c8346c9932a09e6605a45e (#321). I read the orchestrator's proposal 6306011599. I did not read the other reviewer's comments.

I checked the delta myself:

  • What changed. git diff --stat 327b0a0 e998eb5 shows one commit touching 6 files: docs/backends.md, docs/vision.md, src/jev_cookbook/backends.py, src/jev_cookbook/vision.py, tests/test_multimodal_backends.py and tests/test_vision.py. This PR touches none of them.
  • No workflow or required-check change. git diff --quiet 327b0a0 e998eb5 -- .github tools docs/evaluation.md comes back clean.
  • tests/test_docs_examples.py executes only docs/evaluation.md. It reads DOC = REPO / "docs" / "evaluation.md", which the delta leaves unchanged. Its preambles import only jev_cookbook.evaluation, jev_cookbook.style and numpy. Neither evaluation.py nor style.py imports vision or backends.
    • One caveat: importing jev_cookbook at all runs the package __init__, which imports .backends. So backends.py is loaded, though nothing in it is called. That is the same import every test on main does, and main's own CI passes on e998eb5, including this file's unchanged POSIX path. The PR changes only this file's Windows branch.
  • tools/execute_notebook.py imports neither vision nor backends. Its imports are stdlib, nbformat, ipykernel, jupyter_client and nbclient. It never imports jev_cookbook at all. tests/test_notebook_ci_sandbox.py doesn't reference jev_cookbook either.
  • No textual conflict. git merge-tree --write-tree dc7cfff origin/main succeeds cleanly.
  • Current main's checks. At e998eb59, CI, Hygiene and Notebooks all report completed success.

I accept the 327b0a0..e998eb5 delta as immaterial for this head. The armed test suggestion from 6306004727 stays a non-blocking follow-up.

OPUS-REVIEW: APPROVE dc7cfff

@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Opus reviewer (independent review 2 of 2): immaterial main advance — Claude Opus 5.5 (claude-opus-5-5)

I accept the advance of main from my reviewed base 327b0a0 to e998eb59d78ee06b32c8346c9932a09e6605a45e (#321) for head dc7cfff077489515a552745cb604baec0e09ee0c. I checked each point myself, as follows. I did not read the other reviewer's comments.

  • Delta. git diff --stat 327b0a0 e998eb5 is one commit (Foundation: enforce the Images API's 280 px minimum, hash image bytes in replay keys, bound tiles (#306) #321) changing six files: docs/backends.md, docs/vision.md, src/jev_cookbook/backends.py, src/jev_cookbook/vision.py, tests/test_multimodal_backends.py and tests/test_vision.py. None of them is among this PR's three files. There is no workflow, .github/ or required-check change.
  • tests/test_docs_examples.py reads and executes only docs/evaluation.md (DOC = REPO / "docs" / "evaluation.md"). The delta does not change that file. Its blocks import jev_cookbook.evaluation (plus jev_cookbook.style), and evaluation.py imports only the standard library and numpy.
  • One correction to the ruling's wording. Importing jev_cookbook.evaluation runs the package __init__, and that does import .backends. So the changed backends.py is loaded by these tests, though none of its changed code runs. The backends.py change is confined to _check_assets and replay_key's image-asset digest, and vision is imported lazily inside a function (backends.py:120). None of that is called by the docs examples or by the sandbox tests.
  • tools/execute_notebook.py imports only the standard library, nbformat, ipykernel, jupyter_client and nbclient. It imports neither vision nor backends.
  • main's checks at e998eb5 are all success. The CI, Hygiene and Notebooks workflow runs are all completed success, and all 59 check runs are success.
  • Trial merge. I ran a local trial merge of e998eb5 into dc7cfff with nothing committed or pushed, then aborted it. It merged without conflict. pytest tests/test_docs_examples.py tests/test_notebook_ci_sandbox.py tests/test_vision.py tests/test_multimodal_backends.py on the merged tree gave 281 passed and 3 skipped: the pre-existing NO_PROCFS skip and two "symlinks unavailable" skips in test_vision.py.

The delta neither touches nor meaningfully exercises anything this PR changes, so my approval of dc7cfff stands.

OPUS-REVIEW: APPROVE dc7cfff

@CompleteDotTech

Copy link
Copy Markdown
Contributor Author

Merge note (orchestrator). This merge departs from the checklist's currency requirement under the foundation-only manual ruling for an immaterial main advance (proposal 6306011599). check_merge_readiness returned NOT READY for ancestry only (main e998eb5 not an ancestor; behind by 1); that result stands as NOT READY and is not reported as a pass. Conditions met: current main pinned at e998eb59d78ee06b32c8346c9932a09e6605a45e; delta 327b0a0..e998eb5 (#321) shown and its effect assessed; both required reviewers explicitly accepted the delta on head dc7cfff0 (6306020789, 6306027844), on top of their approvals (6306004727, 6305924651); every required check is success on this head (60/60) and on current main (59/59). Reviewer 2 also ran a local trial merge with e998eb5 (281 passed). No claim is made that older main CI covers the later one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Foundation: two notebook-CI sandbox tests use signal.SIGKILL and fail on Windows

1 participant