Skip to content

emrg: a suite run cannot signal the daemon through the client kill routes - #1360

Closed
argszero wants to merge 7 commits into
masterfrom
fix/a-suite-run-cannot-signal-the-daemon
Closed

argszero wants to merge 7 commits into
masterfrom
fix/a-suite-run-cannot-signal-the-daemon

Conversation

@argszero

@argszero argszero commented Sep 17, 2026

Copy link
Copy Markdown
Owner

Closes #1337 (part 2: the subprocess/client restart routes).

A pytest run must not be able to reach a live daemon through the client's kill routes. The guard is a shim, not a stub: it interposes on the module's os name, so a test that patches the module's own os.kill keeps working (that is the shape tests/test_daemon_manager.py uses), and os.kill(pid, 0) probes pass through.

Contents

  1. conftest::_guard_live_daemon_signals — the autouse tripwire on both kill-carrying modules (emrg/client/daemon_manager.py, emrg/__main__.py), with the two escape hatches above.
  2. Six tests in tests/test_hermeticity_guard.py, each route driven through the real code path in both states: the route is refused before the signal lands (a real child process named in a refused kill is still alive afterwards), the liveness probe still reaches the real os, and a kill-isolating test still runs.
  3. Prose in tests/test_stop_all.py / tests/test_cli_failure_reporting.py recording why those routes are now guarded.

Follow-up pushed after the first CI round (why the head moved)

The first version failed both legs, for two independent reasons — both found by CI, neither by the local suite:

  • Windows: the negative control did not drive the probe its platform asks. _old_pid_alive() asks the pid on POSIX (os.kill(pid, 0)) but the port file on Windows (is_running(), because signal 0 is CTRL_C_EVENT there — issue stop/cli: os.kill(pid, 0) is a Ctrl+C on Windows, and two product paths still use it as a liveness probe #1349). The fake answered only the POSIX question, so on Windows the wait loop exhausted and the test quietly drove the 10-second force-kill fallback instead of the path it meant to. It now answers whichever probe the platform asks, and asserts the loop settled on the SIGTERM — so the drift cannot come back silently.
  • The fallback it stumbled into names a signal Windows does not have: os.kill(server_pid, signal.SIGKILL) raises AttributeError on Windows, and nothing in the restart path catches it — worse than the fallback merely failing, because ensure_connected() dies instead of respawning the daemon. It now asks for whatever force the platform has (getattr(signal, "SIGKILL", signal.SIGTERM)); Windows' SIGTERM is an immediate hard kill there, which is what the fallback is for.
  • The test leg failed because the branch was based before emrg: a config.toml edit never restarts the daemon again #1355 removed _get_config_mtime and its guard tests monkeypatched it; CI tests the merge with master. Fixed by merging master and dropping the two patches (one prose mention kept, now describing it in the past tense).

Master was merged again in this round, so the head is the landing tree — no landing-tree measurement is needed to vote.

Verification

  • Landing tree (90f1c2a6, master merged in): uv run pytest tests/ -q3049 passed, 16 skipped.
  • Windows-branch replay on this machine (probe on stdin, sys.platform forced to win32 in that process): with the daemon dying, one SIGTERM, one port probe, 0.2 s, no fallback — the shape the fixed test drives.
  • The product fix in both directions: with signal.SIGKILL deleted (the Windows state) the fixed line runs the fallback and returns; the pre-fix line raises AttributeError: module 'signal' has no attribute 'SIGKILL' — the CI failure verbatim.
  • uv run python -c "from emrg.client.app import run_client" → ok; uv run python -m emrg --help → usage prints.

…oved

CI tests the pull request *merged with master*, not the branch alone, and
master removed `_get_config_mtime` together with the config-mtime restart
branch (#1355). Two tests in tests/test_hermeticity_guard.py monkeypatched
that helper, so both legs failed with

    AttributeError: <module 'emrg.client.daemon_manager'> has no attribute
    '_get_config_mtime'

while the same suite was green on this branch locally: the branch was cut
from 951c19f, before #1355, so the attribute still existed there.

The two patches are dropped — source mtime is the whole restart trigger on
master — and the docstrings now say so, including why a config-mtime patch
would name something that no longer exists.

Re-verified on the merged tree:
- mutation arm (guard removed from conftest, sha16 e113d88b1a3f14af ->
  b4fb6cdca30a25c0): the same 4 tests red, both negative controls green;
  conftest restored byte-identically.
- full suite: 2994 passed, 16 skipped; the live daemon (pid 86187) alive
  before and after.
@argszero

Copy link
Copy Markdown
Owner Author

Closing as a duplicate of #1348, which is the same guard for the same acceptance (issue #1337, item 2) and is the copy that covers all three routes the issue names. Nothing here is lost — the two things this PR had and #1348 did not have been carried over, listed below.

Measured, not asserted. scripts/check-merge-pairs.py --repo argszero/emrg 1348 13600 clean and healthy, 2 blocked by a conflict. Both PRs add an autouse daemon-signal guard to tests/conftest.py and their controls to tests/test_hermeticity_guard.py; both merge orders conflict, so neither can land after the other and one of them has to go.

Why #1348 is the survivor. #1337's acceptance names three routes — an in-process call, a subprocess, and the client-side restart. #1348 guards all three: _guard_stop_all_hermeticity (in-process), _GuardedPopen on subprocess.Popen (a child that runs the stopper), and the _NoSignalOs shim (the client's restart). This PR guards two of the three and records the third as out of reach in its own docstring — "Not covered, and not coverable from inside the process: a child process that runs the stopper".

What was carried over, and where.

  1. The one route this PR covered and emrg: guard the daemon red line at its two uncovered routes #1348 did not: emrg/__main__.py::_stop_daemon, whose SIGTERM fallback signals the daemon without passing through emrg._stop_all's five stop functions, so _guard_stop_all_hermeticity never saw it. emrg: guard the daemon red line at its two uncovered routes #1348's shim now stands in both modules that carry a kill, pinned in both directions by test_guard_refuses_the_cli_stop_fallback_signal and test_the_cli_shim_still_answers_everything_else.
  2. This PR's unrelated production fix: Windows has no signal.SIGKILL, and the restart path's force-kill fallback named it outright — the AttributeError left check_and_restart_if_stale() and ensure_connected() died instead of respawning the daemon. It is in emrg: guard the daemon red line at its two uncovered routes #1348 as commit fd9793df, with a platform simulation that reddens when the fallback names SIGKILL outright (test_the_force_kill_asks_for_a_signal_the_platform_has).

And what this PR's CI round found, which is worth more than the PR. Its test-windows leg failed on run 35283518915 (job 105410618420) with TypeError: is_running() takes 0 positional arguments but 1 was given, raised inside emrg._stop_all.pid_alive on the first iteration of the wait loop. That is a real defect on master, introduced by #1358 when the probe was delegated: is_running answers for the port and takes no pid, while pid_alive's win_probe contract is probe(pid). Nothing on POSIX reaches it, which is why local runs were green. Fixed in #1348 as commit 6b520341, pinned by a stand-in with is_running's real arity — the test it replaces patched in a MagicMock, which accepts any call and therefore stayed green over the crash.

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.

tests: a suite run writes into ~/.emrg/logs, and a daemon was SIGTERMed mid-run (the cycle it broke was forced read-only)

1 participant