Skip to content

tests: no test may perform a real os.kill(pid, 0) on Windows — the rule is enforced in one file, nowhere else #1351

Closed

Description

@argszero

The rule

No test may execute a real os.kill(pid, 0) on Windows. signal.CTRL_C_EVENT is 0 there, and CPython's os_kill_impl routes it to GenerateConsoleCtrlEvent(CTRL_C_EVENT, pid) before the TerminateProcess fallback — so the call is not a probe but a Ctrl+C to that pid's console process group, the whole console included, the runner's pytest included.

Where it is enforced today, and where it is not

  • tests/test_stop_all.py — guarded (PR emrg: one platform-correct liveness probe (#1349) #1350, _POSIX_ONLY + _tests_that_reach_a_real_probe, an AST scan with its own control). The scan caught a real blind spot on its first draft: it matched only attribute calls, so a directly-imported pid_alive(...) would have gone unseen.
  • Everywhere else in tests/ — unguarded. Nothing stops a new test in another file from reaching a real default-argument probe. The class of test that does it is narrow but not hypothetical: it is exactly what PR emrg: one platform-correct liveness probe (#1349) #1350's first CI round did (run 35258286311 — the test passed on windows-2025 and the next test died with KeyboardInterrupt inside subprocess.py:1606, exit 1, 2462 passed, 118 skipped).
  • tests/conftest.py's _NoSignalOs guard (PR emrg: guard the daemon red line at its two uncovered routes #1348) is deliberately scoped to emrg.client.daemon_manager's namespace, and its Popen guard to spawning a daemon stop/restart. Both are about the daemon, not about os.kill in general — and neither reaches a probe made from emrg._stop_all or a test's own os.kill.

Measured exposure right now

grep -rn "os\.kill(" tests/*.py   # real calls, excluding mock/patch mention

At f5ece5ae the only real-call sites are the two _POSIX_ONLY tests in tests/test_stop_all.py, so the suite is currently clean — this issue is about the guard, not about a live defect.

What a fix would look like

A suite-wide version of the same rule, in one place, keyed on the act rather than on a helper's name: on a Windows host, any os.kill(pid, 0) performed by a test (directly, or through a default argument) fails the test loudly instead of signalling the console. The shape that fits this repo's discipline:

Note for whoever picks this up: tests/test_stop_all.py's AST scan is per-file (it reads its own source). A conftest-level rule and that scan would be two instruments answering different questions — this file's tests vs any test — so landing the suite-wide rule does not make the per-file one redundant, but it does make it the weaker of the two.

Evidence

Activity

  1. argszero commented on Sep 18, 2026

    @argszero
    OwnerAuthor

    Recording the state of this one, since the fix in flight matches the shape the issue asks
    for and nothing here said so yet.

    PR #1363 implements the suite-wide rule: tests/signal_probe_guard.py plus an
    autouse fixture in tests/conftest.py that installs RefusingProbe(os.kill, platform=sys.platform) over the module's os.kill. It matches the three properties
    listed above — decided before the delegation (so the refusal cannot itself signal, and
    the controls are safe to write), the platform decision injected as a value rather than read
    (the Windows branch is pinnable on a POSIX runner), and a both-ways control
    (tests/test_signal_probe_guard.py).

    Status measured today (cycle cyc20260918-083832), not quoted from the PR text:

    • head 7e93a2e5, both CI legs green (run 35288203357 — test 3m6s, test-windows 7m4s);
    • the head is stale against master 8b6f0b2e (behind_by=1), so the review was cast on the
      landing tree 3fdb2fbaede0 — three test-only paths, no product path, suite
      3076 passed / 18 skipped on that tree;
    • an independent five-row control on the guard itself: refuses exactly (Windows, signal 0),
      passes win32/15, linux/0, darwin/15, and the real call is provably not reached when
      it refuses → 0 disagreements. pid is deliberately not consulted: the danger is in the
      signal, not in which process is named.

    Two follow-ups already filed, so they are not lost with this issue: #1364 (nothing pins
    that no module under emrg/ captures os.kill at import time — which would silently put
    a module outside this guard's reach; PR #1365) and, as this issue itself notes, the per-file
    AST scan in tests/test_stop_all.py stays as the weaker of the two instruments once the
    suite-wide rule lands.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions