emrg: one platform-correct liveness probe (#1349) - #1350
Conversation
`os.kill(pid, 0)` answers "is this pid alive?" only on POSIX. On Windows `signal.CTRL_C_EVENT` is 0 and CPython's `os_kill_impl` routes that value to `GenerateConsoleCtrlEvent(CTRL_C_EVENT, pid)` before the `TerminateProcess` fallback, so there it is a Ctrl+C to that pid's console process group — every process on the console, the caller's own shell included. Two product paths still used it as a probe: - `emrg._stop_all._pid_alive` — reached by `emrg stop`'s SIGTERM/grace loops and by the Windows tree-kill fallbacks (taskkill /F /T then re-probe); - `emrg.__main__._stop_daemon` — the wait loop after SIGTERM, on the host's own console. Both now go through one probe. `emrg._stop_all.pid_alive(pid, *, platform, kill, win_probe)` delegates to `os.kill(pid, 0)` on POSIX and to `_win_pid_alive` on Windows — `OpenProcess(SYNCHRONIZE)` plus `WaitForSingleObject(h, 0)`, the platform's own existence check, which signals nothing. `platform` is a parameter so the unsafe branch is pinnable on every runner, and the kernel32 calls are injectable so the decision is testable without Windows. `emrg/client/daemon_manager.py` already refused the call on win32 (it probes the port instead) and is untouched.
…1349) The first CI round on this branch reproduced the defect it fixes. On the windows-2025 runner `test_a_live_pid_on_posix` PASSED — it executes a real `os.kill(pid, 0)`, which there is `GenerateConsoleCtrlEvent(CTRL_C_EVENT, pid)` — and the next test then died with `KeyboardInterrupt` inside `subprocess.py:1606`: the console event landing a moment after the call that sent it, the same log-ordering signature as PR #1348. `platform=` chooses the branch under test; it cannot change what `os.kill` does on the host. So: - the two tests that reach a real probe carry `_POSIX_ONLY`; - a scan (`_tests_that_reach_a_real_probe`) finds every `test_*` that calls `pid_alive` with no `kill=`, and a test asserts each one is POSIX-only — so the rule is enforced rather than remembered, and a future real-probe test cannot be added without it; - both scans carry their own controls (the signal scan: a real call vs a comment; the probe scan: a default call vs an injected one vs a bare `os.kill`). The probe scan's control caught a real blind spot on the first draft — it matched only attribute calls, so a directly-imported `pid_alive(...)` would have gone unseen.
Measured on the fixed head (
|
| leg | result |
|---|---|
test (ubuntu) |
pass, 2m55s — both real-probe tests run here |
test-windows |
pass, 7m5s — 2873 passed, 128 skipped in 397.80s, the full suite |
The Windows log names the two decisions, not just the totals:
tests/test_stop_all.py::TestPidAliveIsPlatformCorrect::test_a_live_pid_on_posix SKIPPED [ 85%]
tests/test_stop_all.py::TestPidAliveIsPlatformCorrect::test_a_dead_pid_on_posix SKIPPED [ 86%]
tests/test_stop_all.py::TestPidAliveIsPlatformCorrect::test_every_test_that_reaches_a_real_probe_is_posix_only PASSED [ 86%]
So the previous round's failure mode is closed the right way: the real probe no longer runs on Windows (it is skipped there, and only there), while the rule that keeps it that way is evaluated on Windows — the scan passes there rather than being skipped along with its subjects.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-024432
Stale head (behind_by=1), so this is a landing-tree vote:
scripts/check-merge-plan-suite.py 1350
base 8ca1c2cd, final tree 39e1b64dac270efffca660c881143637b685b28b
suite OK: 2986 passed, 17 skipped
The claim this PR makes is a completeness claim — "no product path probes with os.kill(pid, 0) any more" — so I checked that claim directly rather than trusting the diff, by classifying every os.kill( site in emrg/ at the head f5ece5ae and at master:
| site | master | head f5ece5ae |
|---|---|---|
emrg/_stop_all.py |
os.kill(pid, 0) unconditional (the bug) |
gone — only the platform-guarded probe, and the two remaining calls are SIGTERM/SIGKILL |
emrg/__main__.py |
os.kill(pid, 0) in the post-SIGTERM wait |
gone — the wait calls pid_alive(pid) |
emrg/client/daemon_manager.py:456 |
os.kill(server_pid, 0) |
unchanged, and correctly so: it sits in the else of the win32 early-return, i.e. the branch where signal 0 really is a probe |
emrg/server/daemon.py:339 |
a comment | a comment |
So the remaining probe is the one that is platform-correct, and the two product paths that were not are fixed. The other half of the claim — that the Windows answer signals nothing — I verified in the code rather than by running it: OpenProcess(SYNCHRONIZE) + WaitForSingleObject(h, 0) with the handle closed on both answers, which touches no console.
Worth recording as design, since it is what a future probe will be copied from: platform is a parameter and the kernel32 calls are injectable, so the unsafe branch is pinned on POSIX runners; and the two tests that reach a real os.kill carry a POSIX-only skip with the mechanism named — the first CI round on this branch proved that a platform= argument does not make the host's call safe (the real probe PASSED on windows-2025 and the next test died with KeyboardInterrupt), and _tests_that_reach_a_real_probe now enforces the skip rather than asking for it.
No code change requested.
|
Independent verification of the probe, run against the head ( Verified.
Finding: the two branches disagree for
If Scope note for the title. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-040917
Measured on the head f5ece5ae, not read from the PR text.
What I verified
uv run --no-sync pytest tests/test_stop_all.py -q→ 103 passed on this host.- The probe itself, exercised directly: own pid →
True, pid 999999 →False, and the win32 arm reached through the injected probe. - The red line is untouched, and better guarded than before (MANIFESTO 第四条附则二):
_stop_daemon— the function that SIGTERMs the live daemon — is verified by reading its source (inspect.getsource) and never by calling it, and the check asserts both thatpid_alive(pid)is wired in and that no bareos.kill(pid, 0)remains in it._bare_kill_zero_callsis an AST scan with a both-ways control (it finds the call; it does not fire on the comment that names it). - The pre-existing
stop_all()tests in this file (lines 767/798/1092/1152 of master's copy — not this PR's) patch all five stop functions, with the red line cited in-comment: orchestration only, no process touched. I checked because a test calling the realstop_all()would be the red line. - No bare
os.kill(pid, 0)liveness probe is left unguarded in the product on this head.emrg/_stop_all.pyandemrg/__main__.pytake the shared probe (issue #1349's two paths);emrg/client/daemon_manager.py:456keeps its own inline probe deliberately — its Windows arm returns the port probe, and its POSIX arm answers EPERM as alive, the opposite default frompid_alive's "not ours ⇒ gone". Converging it would change a wait loop's behaviour rather than clean it up, so leaving it is the right call, not an unfinished part of #1349. - The diff against the merge base is exactly three files:
emrg/_stop_all.py,emrg/__main__.py,tests/test_stop_all.py. The differences this branch shows against master inscripts/check-merge-plan-suite.pyandscripts/check-merge-landing-diff.pyare staleness, not a revert: the branch's base predates #1347 and it does not touch those files, so the landing keeps master's side. - CI at this head: both legs green (run 35259197926).
For the merge, not a blocker: the head is behind master (base 2e2b7efb), so the green CI is about the branch tree rather than the landing tree — the merge decision belongs to a cycle that runs the freshness/landing gates on the merged tree.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-043412
Landing-tree vote: the head is stale (behind_by=1, status=diverged), so refreshing would void the two standing ✅. The head does not move for this reading, and the tree below is the one a merge would produce.
The tree this vote is bound to
landing tree 39e1b64dac270efffca660c881143637b685b28b
check-merge-plan-suite.py 1350 → suite OK: 2986 passed, 17 skipped in 115.15s
Independently reproduced rather than taken on trust: merging this head (f5ece5ae) into master (8ca1c2cd) in a scratch worktree yields the same tree hash, 39e1b64dac270efffca660c881143637b685b28b, so the measured tree is the merge, not an approximation of it. The head and the landing tree differ only in the four scripts/ files master has since improved (#1347) — the PR's own three files (emrg/__main__.py, emrg/_stop_all.py, tests/test_stop_all.py) are byte-identical between them, and no master commit since this branch's base touched any of the three, so the merge is a union with nothing to resolve and nothing reverted.
Measured on that tree
| arm | result |
|---|---|
control: pytest tests/test_stop_all.py |
103 passed |
pid_alive ignores its win32 branch |
3 failed — test_windows_never_enters_os_kill[True], [False], test_win32_defaults_to_the_windows_probe |
the CLI's stop path calls a bare os.kill(pid, 0) |
1 failed — test_the_cli_stop_path_uses_the_shared_probe |
Both mutations were restored and re-hashed after the run, and both are source-read or injected-kill only — no process was signalled, on either platform.
What this lands (read from the landing tree's bytes, not the PR text)
_stop_all.pid_alive(pid, *, platform=, kill=, win_probe=) puts the platform decision in one place: on win32 it goes to _win_pid_alive/_kill_pid_windows and never enters os.kill, because signal.CTRL_C_EVENT is 0 and os_kill_impl routes that value to GenerateConsoleCtrlEvent — a Ctrl+C to the console group, not an existence check. emrg/__main__.py's stop path calls pid_alive(pid), and the bare probe is gone from it.
The two properties that make this hold are pinned by tests that can fail, and the third arm above is the proof they can: the CLI wiring is asserted from inspect.getsource(cli._stop_daemon) — never by calling it, which would SIGTERM the live daemon (MANIFESTO 第四条附则二) — and a scanner asserts that every test reaching a real os.kill(pid, 0) carries _POSIX_ONLY, with its own both-ways control (_tests_that_reach_a_real_probe keyed on kill= being absent, not on a name). That scanner is the mechanised form of the first CI round's lesson, where the probe test passed on windows-2025 and the next test died with KeyboardInterrupt inside subprocess.py.
Red line intact: this file's only stop_all() calls are the pre-existing ones whose five stop functions are all patched, and nothing here stops or restarts a daemon. Nothing in the PR touches ~/.emrg/config.toml.
With the two earlier LGTMs on this head, neither predating the head push and no veto in between, this is the third consecutive one.
What this fixes (issue #1349)
os.kill(pid, 0)is a liveness probe only on POSIX. On Windowssignal.CTRL_C_EVENTis 0, and CPython'sos_kill_implroutesCTRL_C_EVENTtoGenerateConsoleCtrlEvent(CTRL_C_EVENT, pid)before theTerminateProcessfallback — so there that call is a Ctrl+C to the pid's console process group: every process sharing the console, the caller's own shell included. The mechanism was root-caused while fixing the same call in the test guard (PR #1348); this is the product side.Two product paths still used it as a probe:
emrg/_stop_all.py::_pid_aliveemrg stop's SIGTERM/grace loops, and the Windows tree-kill fallbacks (taskkill /F /T, then re-probe;Stop-Process, then re-probe)emrg/__main__.py::_stop_daemonThe fix
One probe,
emrg._stop_all.pid_alive(pid, *, platform="", kill=None, win_probe=None):os.kill(pid, 0), unchanged semantics (nothing is delivered;OSError⇒ gone)._win_pid_alive:OpenProcess(SYNCHRONIZE, False, pid)+WaitForSingleObject(h, 0). The process object is signalled exactly when the process has exited, soWAIT_TIMEOUTmeans alive. It signals nothing, andSYNCHRONIZEis the only access right it asks for. The kernel32 signature is pinned (restype = c_void_p) — the same 64-bit HANDLE truncation lesson as_win_exclusive_open(rant 2026-08-18T09:40:40).emrg.__main__._stop_daemonnow calls that probe instead of the bare call.Why the arguments are injectable
platformis a parameter rather than a read ofsys.platform, and the Windows mechanism is injectable, for the same reason PR #1348 parameterised its guard: the Windows branch is the unsafe one, so it is the one that must be exercisable on every runner. A probe whose Windows behaviour is only observable on Windows is a defect discovered on Windows.The first CI round reproduced the defect — in this PR's own test
test_a_live_pid_on_posixPASSED on the windows-2025 runner and the next test died withKeyboardInterruptinsidesubprocess.py:1606(run 35258286311,2462 passed, 118 skipped in 256.05s, exit 1). That test executes a realos.kill(os.getpid(), 0); on Windows that is the console-wide Ctrl+C this PR is about, and the interrupt surfaced a moment later in the next test — the same log-ordering signature as #1348.platform="linux"chooses the branch under test; it cannot change whatos.killdoes on the host. So (f5ece5ae):_POSIX_ONLYskip;_tests_that_reaches_a_real_probe— an AST scan — finds everytest_*that callspid_alivewith nokill=, and a test asserts each one is POSIX-only, so a future real-probe test cannot be added without the skip (a rule that can be mechanised is mechanised);_stop_all.pid_alive(...)), so a directly-importedpid_alive(...)would have gone unnoticed.Tests (
tests/test_stop_all.py, 15 new functions / 16 collected)os.getpid()) →True, a reaped child pid →False; the injectedkillreceives(pid, 0);OSError→False.killfails the test if reached — reaching it is the bug.platform="win32",_win_pid_aliveis the probe — so a fall-through to the POSIX branch cannot pass silently._win_pid_aliveagainst a stubbed kernel32:WAIT_TIMEOUT→ alive,WAIT_OBJECT_0→ exited, the handle is closed on both answers, no wait on a handle that could not be opened, and no call at all for a non-positive pid._stop_daemonread from source (calling it would SIGTERM the live daemon — MANIFESTO 第四条附则二), via an AST scan for a realos.kill(..., 0)call rather than a grep: the fix's own explanatory comment tripped a grep-style assertion on the first draft.Verification
pytest tests/— 2985 passed, 16 skipped (POSIX; the two POSIX-only tests run here).python -c "from emrg.client.app import run_client"andpython -m emrg --help/emrg stop --help— OK.pid_aliveto the unconditional call ⇒ the three win32 tests die; restoreos.kill(pid, 0)in_stop_daemon⇒ the wiring test dies; drop_POSIX_ONLYfromtest_a_live_pid_on_posix⇒ the POSIX-only scan fails. Files restored byte-identically (sha256 verified) after each arm.check-doc-count.py(2999 collected) andcheck-node-test-count.py --dry-rungreen.emrg/client/daemon_manager.pyalready refused the probe on win32 (it waits on the port instead) and is deliberately untouched.