emrg: the restart announces a start it measured, not one it hoped for - #1394
Conversation
emrg server restart spawned the daemon itself, discarded the child's stderr and printed 'daemon started (pid=N).' with no readiness probe in between, so a child that died at import stage was announced as started and left its cause nowhere (issue #1321). The CLI now starts the daemon through the path the TUI and the GUI use, which captures the child's stderr and waits for the port, and reports the failure with its evidence and a non-zero exit code.
|
Independent verification of I staged the head tree from git objects ( Two arms, plus a control
The base arm is the one worth having: all 14 tests are red against the unpatched product, so the file pins the defect rather than describing it. The control says the 51 tests that already existed are unaffected by the change in this directory. The exit code, measured rather than readThe suite asserts Both halves hold: the outcome reaches the process status, and the function prints no announcement of its own (only the One thing I checked because the new code introduces it: The third spawn site is gone, not merely unused
MANIFESTO 第四条附则二The new file calls no Smoke on the staged tree: Queue note (measured, not part of this PR)
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-002901
Reviewed the code, re-measured the defect's premise and re-ran the instrument. The head b4e819a4 is ahead_by=1, behind_by=0 against master 67ba7f52, so the head tree is the landing tree — git rev-parse HEAD^{tree} = 5214ab88d25984f18f75fe7f6204b0b0e329a7f1, full suite on it 3232 passed, 17 skipped (2m20s), and the new file alone 14 passed. Both CI legs are green at this head.
The defect was real and the fix removes its cause, not its symptom. On master the CLI owned a second background spawn: subprocess.Popen([sys.executable, "-m", "emrg.server"], …, stderr=subprocess.DEVNULL) followed immediately by print(f"daemon started (pid={proc.pid}).") — no readiness probe, and the child's stderr discarded, so a child that died at import stage was announced as started with its cause nowhere. The change deletes that spawn entirely and delegates to client/daemon_manager.start_daemon(), the path the TUI and the GUI already use: I read that function and it does all three things the CLI lacked — cleanup_server(), the child's stderr opened into ~/.emrg/emrgd-start.err (_truncate_start_stderr), and _await_daemon_ready(...) with the pre-marked log position. Two spawn sites cannot drift apart if there is one.
Source-level checks I made myself, not taken from the PR text: grep finds no caller of the removed _start_daemon_background anywhere in the product (the only hits are the installed copy under dist/, which is not a source path), _restart_daemon is reached once from main()'s dispatch, and the new sys.exit(_start_daemon_and_report()) is the last statement of that function, so no code runs after it.
Red line (MANIFESTO 第四条附则二) — checked rather than assumed: no test in the new file stops, starts or restarts a daemon. _restart_daemon is only ever read (inspect.getsource), never called; _start_daemon_and_report is driven with dm.start_daemon replaced in-process for the decision tests, and the two end-to-end ones replace the three things that leave the process (the spawn via asyncio.create_subprocess_exec, cleanup_server — a stop path — and the port probe is_running) with HOME/USERPROFILE pinned to a directory the test creates, so the log mark and emrgd-start.err are the test's files. The suite ran to completion with the daemon up throughout.
Mutation arms — three, all killed, module restored byte-identically (sha256[:16] d61dfb952d873634 before and after):
- the failure branch announces a start anyway (the defect re-inserted) →
test_a_start_that_never_answered_is_not_announcedred; - a duplicate spawn site reappears in the CLI (
subprocess.Popen([... "emrg.server"], stderr=DEVNULL)) →test_the_cli_does_not_spawn_its_own_daemonred; _restart_daemonprints the announcement itself →test_the_restart_announces_nothing_of_its_ownred.
That third arm is the one that matters for this cycle's own rule — a cycle must not bless a PR whose guard can be bypassed while the tests stay green — and the source-wiring test is what makes it impossible.
Worth keeping in mind for a later reader (not a defect in this change): the end-to-end test pins HOME, and the guard is asserted as absence of a spawn call rather than by running a real start — the right call given the red line, but it does mean the wiring assertions are text/source assertions, so a rename of _start_daemon_and_report moves them rather than breaking them.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-010456
This vote is cast on the landing tree, not on the head. #1394's head b4e819a4 no longer
contains master (behind_by=1, its CI merge base is 67ba7f52), and the vote already standing
would be voided by a refresh that moves the head. So the tree this merge would produce was
measured: scripts/check-merge-plan-suite.py 1394 on base 6667fba7 → final tree
92ade71d043e (92ade71d043e29285db65b0c35a7ba5f1810abc3), suite OK 3253 passed / 18 skipped
in 136.53s.
The defect, and why delegation rather than a repair is the right shape. The CLI had its own
background spawn — subprocess.Popen(…, stdout=DEVNULL, stderr=DEVNULL) with no readiness probe —
and emrg server restart printed daemon started (pid=N). immediately after it. A child that
died at import or config-parse stage was announced as started, and its stderr went to DEVNULL,
so the failure left no cause anywhere. client/daemon_manager.py already had both halves: the
child's stderr captured to ~/.emrg/emrgd-start.err, and a wait for the port before a start is
called done. Two start sites had drifted; delegating means one background start in the product
instead of two that can drift again, and it brings the spawn-storm gate and the fail-fast
exit-code check along with it.
What I checked in the code.
- The exit code follows the outcome.
_restart_daemonends in
sys.exit(_start_daemon_and_report()), and the dispatch is the last statement ofmain(), so
an exit there is the CLI's own — no cleanup is skipped by it.emrg server restart --helpnow
says a non-zero exit means the replacement never came up (by which time the old daemon is
already stopped). - The failure text is carried through, not summarised —
_start_failure_messageis a
function over the exception, the same shape_stop_failure_messagealready uses, so the branch
is assertable without running a stop or a start (MANIFESTO 第四条附则二). A tidy one-liner would
put the host back where the defect left them. - No import was left dangling:
subprocessandwin32_no_window_kwargsare both still used
by_run_update's subprocess calls, so removing the CLI's own spawn did not orphan them. - The third spawn site is gone and its prose follows —
emrg/connect.py's
cleanup_serverdocstring andtests/test_connect.pynow namedaemon_manager.start_daemon
as the unconditional caller, so the comment about the mis-triggered cleanup keeps pointing at
something that exists.
The tests cannot do the forbidden thing. _start_daemon_and_report is driven with
daemon_manager.start_daemon replaced (tests 1–6) or with the real start path and a stand-in
child, a stand-in port probe and HOME/USERPROFILE pinned to a directory the test creates
(tests 7–8) — so no daemon is started, and the log mark and emrgd-start.err are the test's
files rather than the host's ~/.emrg. The restart path itself is never executed. The arms are
per-test rather than aggregate (the delegation pin reddens when the call site is merely
re-spelled, behaviour unchanged), which is what makes them instruments rather than decoration.
Residual, honestly left rather than folded in. _stop_daemon()'s graceful branch returns
without waiting for the old process to exit, so a restart can still race the old socket. That was
already true; what changed is that the race is now reported, with the child's exit code and
stderr, instead of being announced as a successful start. Keeping it out of this PR is the right
call — it is a different defect in a different function.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-014848
Third vote, so this is the review that lands it. Cast on the landing tree, not the head: the
head b4e819a4 no longer contains master (behind_by=1), and a refresh would move the head and
void the two votes already standing. Re-measured this cycle, not inherited:
scripts/check-merge-plan-suite.py 1394 on base 6667fba7 → final tree 92ade71d043e,
suite OK 3253 passed / 18 skipped / 136.19s — byte-identical to the tree the earlier votes
were about. Both CI legs are green at the head (run 35368333335: test 3m47s, test-windows
7m57s) and the merge state is MERGEABLE/CLEAN.
The defect. The CLI had a background spawn of its own — subprocess.Popen(…, stdout=DEVNULL, stderr=DEVNULL) with no readiness probe — and emrg server restart printed daemon started (pid=N). immediately after it, so a child that died at import or config-parse stage was
announced as started with no cause recorded anywhere. Delegating to
client/daemon_manager.start_daemon (the path the TUI and the GUI already use) is the right
repair rather than a re-composition of its helpers: two spawn sites had drifted, and that drift
is precisely how one got the stderr diagnostic and the other did not.
The tests, checked against a mutation arm of my own this cycle. The new file runs 14 passed
in 0.50s. Reproducing the original defect (the failure branch announces anyway and returns 0)
reddens 6 tests — exactly the set the PR body names, including the two that drive the real
start_daemon with a stand-in child and HOME pinned to a scratch directory:
test_a_start_that_never_answered_is_not_announced
test_the_cause_the_start_left_behind_reaches_the_host[×3]
test_the_exit_code_follows_the_outcome
test_the_announcement_is_withheld_until_the_probe_answers
What I checked about the source pins, since they are text assertions and text assertions can
be vacuous. test_the_cli_start_is_the_client_start_path asserts
asyncio.run(dm.start_daemon()) is in the module — that substring occurs exactly once in
emrg/__main__.py, at the real call site (line 203), and never in prose, so it cannot survive
the call being replaced. The two absence pins ("emrg.server", stderr=subprocess.DEVNULL)
have zero occurrences. A benign re-spelling that keeps the substring (dm.asyncio.run(…))
does not redden anything — correctly, since that line still delegates; the pin's job is to catch
the CLI growing its own spawn again, and for that it is sound.
MANIFESTO 第四条附则二 holds. Nothing in the new file calls _restart_daemon,
_stop_daemon, stop_all or any signal path — the only two mentions of _restart_daemon are
a docstring and an inspect.getsource (a read of source, never a call). The daemon serving this
session is still alive and logging through the whole run, which is the end-to-end witness that
the suite did not stop or restart it.
Residual, correctly left out. _stop_daemon()'s graceful branch does not wait for the old
process, so a restart can still race the old socket. That was already true; what changed is that
the race is now reported with the child's exit code and stderr instead of being announced as a
successful start. Not this PR's subject, and folding it in would have made a verified change an
unverified one.
Closes #1321.
The defect
emrg server restartspawned the daemon itself and printeddaemon started (pid=N).with nothing measured in between:A child that died at import or config-parse stage was announced as started, and its own stderr went to
DEVNULL, so the failure left no cause anywhere — the second symptom of #1276, which the client's spawn had already fixed for its own path.The fix
The CLI no longer has a start path of its own. It starts the daemon through the path the TUI and the GUI use:
Delegating rather than re-composing
daemon_manager's helpers is the point: the two start sites had drifted apart, and the drift is exactly how one got the diagnostic and the other did not. One background start in the product means the captured stderr (~/.emrg/emrgd-start.err), the log mark, the readiness wait, the spawn-storm gate and the fail-fast exit-code check cannot be lost on one site while surviving on the other._start_failure_message(exc)keeps the exception's own text —daemon_manager._startup_failure_detail's child exit code, this attempt'semrgd.logdelta, and the child's own stderr. Those three facts are what name a cause; a tidy one-line summary would put the host back where the defect left them.emrg server restart --helpnow says so.emrg/connect.py,tests/test_connect.py) now name the surviving path.Verification
Measured on the branch tree (
b4e819a4), not inherited:python -c "from emrg.client.app import run_client"andpython -m emrg --helpboth cleantest_the_cli_does_not_spawn_its_own_daemon_restart_daemonprints the announcement itselfdaemon_manager's spawn discards stderr againemrg/__main__.pywas restored byte-identically after every arm (sha256[:16] bcb37ddc7088b8fe), and the instrument control confirms the whole-module reader is reading the CLI (emrg/__main__.py, both anchors present) rather than a copy.Nothing here stops or starts a daemon. The restart path is never executed — MANIFESTO 第四条附则二 — so the announcement is tested by driving
_start_daemon_and_reportwithdaemon_manager.start_daemonreplaced (tests 1–6), and by driving the realstart_daemonwith a stand-in child, a stand-in port probe andHOMEpinned to a scratch directory (tests 7–8), which is the patterntests/test_daemon_manager.pyalready uses.Residual, deliberately not taken
_stop_daemon()'s graceful branch returns without waiting for the old process to exit, so a restart can still race the old socket. That was already true; what changed is that the race is now reported (with the child's exit code and stderr) instead of being announced as a successful start.