Skip to content

emrg: the client asks the one liveness probe instead of spelling it twice - #1358

Merged
argszero merged 1 commit into
masterfrom
fix/one-spelling-of-the-liveness-probe
Sep 17, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/one-spelling-of-the-liveness-probe

Conversation

@argszero

Copy link
Copy Markdown
Owner

Closes the residual of #1349: the client was the last place that asked the liveness question in its own words.

emrg/_stop_all.pid_alive was made the one platform-correct probe in #1350, and emrg server stop now asks it. The client's restart path kept a second copy — _old_pid_alive read sys.platform itself and called a bare os.kill(server_pid, 0) on POSIX. That copy happened to be correct, but it was a second spelling of a rule whose Windows half is dangerous to get wrong: signal.CTRL_C_EVENT is 0, so on Windows that call is a Ctrl+C delivered to the pid's whole console process group. A rule you can only verify on the platform where getting it wrong hurts is a rule worth having in exactly one place.

What changed

  • emrg/client/daemon_manager.py: new _old_daemon_alive(pid, *, platform="", kill=None, win_probe=None) delegates to emrg._stop_all.pid_alive, passing is_running as this caller's Windows probe (Windows SIGTERM is an immediate hard kill, so a port probe is what answers there — unchanged behaviour). The closure in check_and_restart_if_stale is now a one-line call. No sys.platform read remains in the file.
  • tests/test_daemon_manager.py: TestTheOldDaemonProbe — 5 tests, pinning both answers on any runner, plus a source scan.

emrg/_stop_all.py is pure stdlib with no emrg imports, so the import is a local one inside the helper, the same shape emrg/__main__.py already uses.

One reading changes, named rather than discovered: the deleted copy read EPERM as alive (the process exists, we may not signal it); pid_alive reads any OSError as gone. The pid here is the daemon this client just SIGTERMed — its own child, same user — so EPERM cannot arise for it, and one answer for the class is the point of asking instead of spelling. It is stated in the helper's docstring.

Verification (local)

  • Full suite 2992 passed / 16 skipped. The delta is exactly the 5 new tests: master in the same environment is 2987 passed / 16 skipped (measured in a scratch worktree at 951c19f3; that worktree reports one extra skip only because it has no node_modules, so test_check_node_test_count.py skips there and runs in the main tree — the adjustment is what makes 2986 there and 2987 here the same number).
  • uv run python -c "from emrg.client.app import run_client" → ok; uv run python -m emrg --help → ok.

Mutation arms

Both arms restore the source byte-identically (daemon_manager.py sha256[:16] c8ee5af8c054d381 before and after):

  • full revert (the deleted copy comes back, the helper goes away) → all 5 new tests red, 33 passed;
  • respell with behaviour kept (delegation stays, but the platform verdict is read locally again in the old shape) → only the source scan fails, 37 passed. That is the arm that shows the scan is not redundant with the behaviour it guards: a respelling that behaves identically is still caught.

The scan is checked in both directions — its positive half asserts text the file must still contain, so a scan that cannot read the file fails instead of passing. It pins the mechanism (sys.platform, a signal-0 probe) rather than the word win32, which this file legitimately carries in win32_no_window_kwargs.

Not in scope here: the test-side half of this rule is #1351 (no test may perform a real os.kill(pid, 0) on Windows), which is a suite-wide conftest guard and a separate change.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260918-051533

Landed-tree vote: master moved twice today (951c19f35f3ee4d7, this PR's own #1355 among them), so the head c8ec7eaf is behind_by=1 and its CI verdict is about a base that can no longer be merged. Landing tree 4656d510eb3c6162bf6c5995d265fd6ea84ecb75, materialised and measured this cycle: suite on it 2992 passed / 17 skipped, and tests/test_daemon_manager.py 39 passed (33 at master + 1 from #1355 + 5 added here).

What this cycle's reading adds over the head measurement is the co-existence question, which nothing had asked: #1355 touches emrg/client/daemon_manager.py too — the same file, and its docstring edit sits in the function immediately below the helper this PR adds. Merged, they do not collide: the helper is defined at :374, the delegation from emrg._stop_all import pid_alive is at :399, the call site is at :480, and sys.platform appears 0 times in the file — so the deletion this PR is about survived a second landing in the same file rather than being silently undone by it. The 5 tests added here pass on the merged tree, not just on the branch.

The claim, for a reader who has not seen the earlier reading: emrg/_stop_all.pid_alive is the one platform-correct liveness probe (#1350), and this PR removes the last respelling of that rule — the client's own sys.platform == "win32" branch plus a bare os.kill(server_pid, 0) — by delegating to it, with is_running as this caller's Windows probe (behaviour unchanged). The arms for that were run on the branch last cycle and are stated on the PR; the one reading that changes (EPERM: the deleted copy called it alive, pid_alive calls it gone) is named in the helper's docstring rather than left for a later reader to discover.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260918-054009

Second vote, measured on the tree this merge would land rather than on the head alone, because check-merge-freshness.py reports #1358 STALE (head c8ec7eaf, base 951c19f3, behind_by=2 — CI's verdict is about a base that can no longer be merged). The head was left alone so the standing vote survives:

  • scripts/check-merge-plan-suite.py 1358 → base b706e2ec (refs/remotes/origin/master), final tree 137c2033f1d64de8f2304a20a2f44dafffaae3ea, suite OK: 3047 passed, 17 skipped.

The change itself asked one question — is the probe now asked in exactly one place? — and the head answers it:

  • The delegation matches the authority's signature. _old_daemon_alive forwards platform / kill / win_probe to emrg._stop_all.pid_alive, whose own signature is pid_alive(pid, *, platform: str = "", kill=None, win_probe=None) — the same three seams, so both answers stay pinnable on every runner. win_probe defaults to this module's own is_running, which is the correct Windows probe for this caller.
  • The second spelling is gone, not merely bypassed. sys.platform occurs 0 times in emrg/client/daemon_manager.py at this head (it was the branch that answered the probe). import sys is still needed — sys.executable at the spawn site — so nothing dead was left behind.
  • The remaining os.kill(...) calls are sends, not probes: signal.SIGTERM / signal.SIGKILL on the restart path, which is what a restart is supposed to do once it has decided to restart. The liveness question is now asked only through pid_alive.
  • The one behaviour change is disclosed rather than discovered: the removed copy read EPERM as alive, pid_alive reads any OSError as gone. The docstring names it and gives the reason (the pid is the child this client just signalled, same user, so EPERM cannot arise for it), which is the honest way to land a unification.

No test starts, stops or restarts a daemon; the guards added by #1360 are on master and the suite above ran under them.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260918-063417

Third vote, on the tree this merge would land (master has not moved since the second vote, so
137c2033f1d6 is the same tree: check-merge-plan-suite.py 1358 → final tree 137c2033f1d64de8f2304a20a2f44dafffaae3ea, 3047 passed / 17 skipped). The head c8ec7eaf is behind_by=2, so the head's own CI verdict is about a tree that can no longer be merged; the head does not move and the two standing votes already cover the head and this same landing tree.

Reviewed the diff at code level rather than counting the standing votes, and re-derived every claim from the head's own files:

  • The delegation is real, not a rename: daemon_manager._old_daemon_alive (head :412-414) does from emrg._stop_all import pid_alive and returns pid_alive(pid, platform=platform, kill=kill, win_probe=is_running …). Read pid_alive on master (emrg/_stop_all.py:230) — the signature and the platform branch match, so emrg server stop and this restart path now ask the same function.
  • The respelling is gone: git show c8ec7eaf:emrg/client/daemon_manager.py | grep -c sys.platform0. The old inner _old_pid_alive (which decided the platform here, a second copy of #1349's rule) is replaced by a one-line delegate at :484-485, while the SIGTERM at :480 — which the new test pins as the instrument's positive control — stays.
  • Both answers are pinned on every runner, which is the part that matters: the Windows branch is the unsafe one (signal.CTRL_C_EVENT == 0os.kill(pid, 0) is a Ctrl+C to that console process group), and a probe whose Windows behaviour is only observable on Windows is a defect discovered on Windows. test_windows_never_enters_os_kill makes reaching os.kill on that branch itself the failure, and test_the_platform_decision_is_not_respelled_here carries a positive half (assert "_old_daemon_alive" in src, assert "os.kill(server_pid, signal.SIGTERM)" in src) so a scan that cannot read the file it reads fails instead of passing — an instrument, not a grep.
  • The one reading that changes is named, not hidden: the old copy read EPERM as alive, pid_alive reads any OSError as gone. For this call site the pid is the daemon this client just SIGTERMed — its own child, same user — so an EPERM cannot arise for it, and the docstring says so rather than leaving it to be discovered.

Pair check before merging, since #1360 edits the same function: check-merge-pairs.py 1358 1360 → both ordered pairs clean and healthy, check-merge-order.py0 of 1 pairs conflict, and merging #1358 dirties nothing (it does not touch the signal.SIGKILL line at :495 that #1360 is about, so neither PR absorbs the other's defect).

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.

1 participant