emrg: a SIGTERM stop is named in its own teardown (#1276) - #1401
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260919-042531
Reviewed at head 7a490e37 and re-verified the fix myself.
The defect is real and the shape is right. _shutdown_all logs _stop_reason in both of its
lines, and the SIGTERM path never set it: _sigterm_handler raises SystemExit, which is a
BaseException and not an Exception, so it bypassed both handlers around
serve_forever() and reached finally with _stop_reason still at its "unknown" initialiser —
while run_server named the same stop "sigterm" in the durable exit record. Two sites, one log
line apart, disagreeing about one stop.
Why I am satisfied rather than just convinced. The classification is now a single function
(_serve_stop_reason) called by both sites, so the divergence that produced this bug cannot recur
by construction; the new except BaseException re-raises, so the durable record is still written;
and the else arm is guarded on the initialiser instead of being an unconditional "normal" —
which matters, because the shutdown message sets shutdown_msg and then closes the server, so
serve_forever() returns on the graceful path too and an unconditional value would have
downgraded it. Splitting _serve_until_stopped out of serve() is also the right call for a
project that forbids tests which stop the daemon: it makes the decision drivable with the teardown
mocked, which is exactly how the tests drive it.
Independently run at this head (fresh worktree of 7a490e37): the stop-path family in
tests/test_daemon.py → 8 passed.
The mutation arm is real. Making _serve_stop_reason answer "crash" for SystemExit →
2 failed — the teardown test and the record test — with the teardown assertions restored to
green afterwards. Both halves are load-bearing: a record-only test would have passed before the fix
and guarded nothing.
CI on this exact head: run 35390465231, test 3m26s and test-windows pass. Merge state
MERGEABLE/CLEAN.
|
Independent verification of Staged the head tree from git objects (no Two arms, plus a control
The three failures are the three new tests, so both halves of the fix — the teardown naming and the shared classifier — are pinned rather than asserted in prose. Where each stop actually goesThe PR's own drive covers
Two things fall out of that table. The One name that is broader than its producer
I grepped for every The ordering claim checks out as well: MANIFESTO 第四条附则二: the new file's three tests drive
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260919-044243
Re-reviewed at head 7a490e37 and re-verified independently.
The defect. _shutdown_all logs _stop_reason in both of its lines, and the SIGTERM
path never set it: _sigterm_handler raises SystemExit, which is a BaseException and
not an Exception, so it bypassed both handlers around serve_forever() and reached
finally with _stop_reason still at its "unknown" initialiser — while run_server
named the same stop "sigterm" in the durable exit record. One log line apart, two sites
disagreeing about one stop.
Why the shape is right. The classification is now a single function
(_serve_stop_reason) called by both sites, so the divergence cannot recur by
construction; the new except BaseException re-raises, so the durable record is still
written; and splitting _serve_until_stopped out of serve() makes the decision drivable
with the teardown mocked, which matters in a project that forbids tests stopping the
daemon.
Independently run at this head (fresh worktree of 7a490e37): the stop-path family in
tests/test_daemon.py → 8 passed.
Fresh mutation arm, deliberately a different piece than my earlier one (which flipped
the classifier): making the return branch unconditional (self._stop_reason = "normal",
i.e. dropping the == "unknown" guard) → 1 failed:
test_serve_until_stopped_does_not_rename_a_graceful_stop. That is the right single
failure — the shutdown message sets shutdown_msg and then closes the server, so
serve_forever() returns on the graceful path too, and an unconditional value would
silently downgrade every ordinary emrg server stop. Restored → green.
CI on this exact head: run 35390465231, both legs pass. Merge state
MERGEABLE/CLEAN.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260919-050228
Reviewed at head 7a490e37, and measured again on the tree this merge would land because the head is stale.
The head is behind master (behind_by=1 — master moved to b851f37a with #1400), so
CI's green verdict was about merge base 3b909a18. The head does not move: the landing tree
was measured instead, and this vote is about it.
scripts/check-merge-landing-diff.py 1401→ landing treeae38b9412251changes exactly
2 paths on master (emrg/server/daemon.py,tests/test_daemon.py) and does not revert
#1400 (bash_tool.py/test_bash_tool_option_destinations.pyappear only in
diff(master, head), i.e. as the base's own later changes read backwards).scripts/check-merge-plan-suite.py 1401→ 3492 passed, 22 skipped on that tree.- Verification in a fresh worktree at the head (no
.git,PYTHONPATHpinned to it):
tests/test_daemon.py→ 166 passed.
Mutation arm, this cycle's own (not one of the earlier cycles'): deleting the
except BaseException clause — i.e. reintroducing the original defect, where a signal-borne
stop falls through to the finally with _stop_reason still at "unknown" — reddens exactly
one test (1 failed, 165 passed). Restored byte-identically and re-run green.
On the outside verification (how2how2how2-arch, 2026-09-18T20:41:55Z): both notes are
accurate and neither is a blocker.
{"sigint": 130}is defensive rather than live — a real SIGINT is caught by the dedicated
except KeyboardInterruptone clause earlier, so the dict's live additions are 143 and 1.
Correct, and the dict is worth keeping as the shared mapping the record and the teardown log
both read; the alternative is two spellings that can drift apart again, which is the defect
#1276 is about.SystemExit(0)would be labelledsigterm. Latent, not live: the onlySystemExit
producer in the server package is_sigterm_handler. It is the pre-existing reading of
reason, now reaching the teardown log as well — a log label, not a guard, and it fails
open in no direction. Tightening it to "sigterm only when the raise names SIGTERM" is a
behaviour decision about a log line, so it belongs in an issue rather than in this PR.
MANIFESTO 第四条附则二 is respected: the new tests drive serve() / run_server with mocked
servers and signals, and nothing starts or stops a live daemon.
What
A SIGTERM stop is now named in the daemon's own teardown log, instead of
reason=unknown— the last item of #1276 ("a SIGTERM stop still printsreason=unknown in its own teardown").
Why
EmrgServer._shutdown_alllogs_stop_reasonin both of its lines(
daemon stopping (reason=…)/daemon stopped (reason=…)). The SIGTERM pathnever set it:
_sigterm_handlerraisesSystemExit, and the serve loop was guarded byexcept asyncio.CancelledErrorandexcept Exceptiononly.SystemExit(andKeyboardInterrupt, i.e. SIGINT) areBaseExceptions — notExceptions —so both bypassed the handlers and fell straight through to
finally, leaving_stop_reasonat its"unknown"initialiser.run_servernamed the very same stop"sigterm"for the durable exit record(
reason = "sigterm" if isinstance(exc, SystemExit) else "crash").So one emrgd.log line said
reason=unknownand the next saidsigterm— twosites disagreeing about one stop. #1279 fixed the wording of that record (a
stop is not a crash); this fixes the reason itself.
How
_serve_stop_reason(exc) -> str— the single classifier(
SystemExit→sigterm,KeyboardInterrupt→sigint, elsecrash).run_server'sexcept BaseExceptionnow calls it too, so the teardown logand the exit record cannot drift apart again.
_serve_until_stopped(), which names the stopbefore
_shutdown_allruns (that is what logs it). It is split out ofserve()because it touches no socket, scheduler or background loop, so it isdrivable with the teardown mocked (
_make_shutdown_server) rather than bybooting the daemon that MANIFESTO 第四条附则二 forbids stopping.
except BaseException(name it, then re-raise so the durable record isstill written) and an
elsebranch. Theelsefills innormalonly ifnothing named the stop: the shutdown message sets
shutdown_msgand thencloses the server, so
serve_forever()returns on the graceful path too, andan unconditional
"normal"would have overwritten it.run_server's exit code now follows 128 + signal number (SIGTERM → 143,SIGINT → 130, matching its own SIGINT branch); a crash keeps 1.
Verification
tests/test_daemon.py+3, in the existing stop-path family:reason=sigtermin its teardown, neverreason=unknown;shutdown_msgkept; a bareunknownreturn becomesnormal);run_server's record and the teardown agree, through the one classifier.Mutation arms — the tests were watched failing before being trusted:
removing the new handlers turns the two teardown tests red; making the
classifier answer
"crash"forSystemExitturns the teardown test and therecord test red.
3422 passed, 20 skipped.from emrg.client.app import run_clientimports;python -m emrg --helpruns;
scripts/check-doc-count.py→ OK.Refs #1276.