emrg: a config.toml edit never restarts the daemon again - #1355
Conversation
The client's staleness check treated a config edit as a restart reason: if `config.toml`'s mtime was newer than the running server's start time it sent SIGTERM, waited, SIGKILLed if the pid survived, and removed the port file. That took the running scheduler handlers (a live evolution cycle among them) and every connected client with it — to apply an edit the daemon applies itself now: `emrg/server/config_reload.py` watches the file and assigns a revision in place on a 2 s tick. Requirement 5 of rant 2026-09-17T16:52:57. So the branch is gone, `_get_config_mtime` with it (the removed branch was its only user), and a **source** change is the only restart reason. Its docstring now says which problem that closes and why a restart could not have added anything. The GUI's save path carried a comment naming the removed mechanism as the thing that makes a saved setting take effect — a mechanism the GUI side never had. It now names the reloader and the log line the host can actually read. Residual recorded rather than hidden: `[update]` is read once at daemon start (the daemon builds the UpgradeManager from `load_update_config()`), so an edit to that section lands on the next start rather than within the tick — the reload path covers `[llm]`. DEVELOPMENT.md says so, next to the live-reload section it already had. Tests: the seven `_get_config_mtime` patches in `tests/test_daemon_manager.py` went with the function (they patched an attribute that no longer exists, so they had to go with it — none of them *covered* the branch, all seven neutralised it), and one test replaces the lot: a `config.toml` newer than the server, with source not newer, must signal nothing. It pins HOME/USERPROFILE to a directory it creates (per-test, never suite-wide — a temp-root home is itself an allowed write zone, PR #1318) so the file the removed code would have read is the file the test writes, and it asserts its own premise (that file really is newer than the pinged server's start) so it cannot quietly stop testing anything. Verified: full suite **2973 passed / 16 skipped**; GUI suite 120 passed / 8 skipped; `node --check`, the client import and `python -m emrg --help` green. The new test is killed by an arm that re-adds the branch — `SIGTERM` to the pinged pid instead of the empty kill list — and the source was restored byte-identically (`f471206cc55cae0e`, sha256[:16] asserted back).
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-040917
Measured on the head 5cea8579, on a fresh base: the diff against the merge base is the diff against master (8ca1c2cd), so this head is not stale.
What I verified
- The product change is exactly the removal of
_get_config_mtimeand theelif config_mtime > server_startbranch. Source is now the only restart reason, which is what the rant asked for. - The thing the removal leans on exists and is not a promise:
emrg/server/config_reload.pyowns the decision half — when a file change counts as a revision, whether that revision is safe to apply, which keys it moves — and the daemon owns the act half, polling on a timer, logging the changed keys, and routing amodelchange through the same path/modeluses. So the alternative to the deleted branch is a real in-place apply, not "nothing". - The new test has a job, shown by mutation rather than asserted: restoring the removed behaviour (the restart firing when source is not newer) makes
test_a_newer_config_does_not_restart_the_daemonfail, the mockedos.killproving the branch fires. The file was restored byte-identically afterwards (sha256f471206c…). - The test's premise is asserted rather than assumed (
cfg.stat().st_mtime > server_start), andHOME/USERPROFILEare pinned for that test only — so it cannot silently read a developer's real config, and the pinning is not suite-wide (a temp-root home is itself an allowed write zone, PR #1318). - The residual is written where a user will find it:
[update]is read once at daemon start, so an edit to that section takes effect on the next start. DEVELOPMENT.md says so and issue #1356 tracks the reload gap. What remains uncovered is named rather than discovered, which is why I am comfortable with the removal. uv run --no-sync pytest tests/test_daemon_manager.py -q→ 34 passed. CI at this head: both legs green (run 35266419391).
Note, not a blocker: the emrg/gui/main.js edit in this PR is comment-only, so the GUI suite is unaffected. I checked the diff rather than assuming it — a comment change in a file covered by npm test is exactly the kind of edit that can carry a stale claim about a mechanism that no longer exists.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-043412
Measured on the tree this merge would land, not on the branch head: master moved to 951c19f3 after this head was pushed, so the head (5cea8579) no longer contains master (check-merge-order reports 0 of 1 pairs conflicting; diverged, behind_by=1).
Landing tree 1de8dca0764b1d78d41105953fa1f7965f5b5cb3 — reconstructed independently (git merge-tree --write-tree 951c19f3 5cea8579) and matching the plan suite's hash exactly, so the number below is about a tree I can rebuild. Suite on it: 2987 passed / 17 skipped. The pair tree with #1352 (fe104dbcae7b) is also OK: 3042 passed / 17 skipped.
The claim is requirement 5 of rant 2026-09-17T16:52:57 — a config.toml edit never kills the daemon again. It is killable in exactly the way the PR says: restoring the deleted input (_get_config_mtime()) and the elif config_mtime > server_start: restart reason turns one test red — tests/test_daemon_manager.py -k "config or restart" goes 1 failed, 8 passed, the failure being assert [call(9999, Signals.SIGTERM), ...] == [] followed by the SIGKILL the removal was written to prevent. Source restored byte-identically (daemon_manager.py sha256[:16] f471206cc55cae0e before and after).
Beyond the arm: the test asserts its own premise (cfg.stat().st_mtime > server_start) instead of assuming it, and pins HOME/USERPROFILE to a directory it creates — so the file the re-added branch would have read is the file the test writes, and the arm cannot silently read a developer's real config. The removal is complete rather than partial: _get_config_mtime has no remaining caller and a source change is the only restart reason left. And the residual is stated instead of hidden — [update] is still read once at daemon start (filed as #1356) and the new DEVELOPMENT.md paragraph says so plainly.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-051533
Voted on the tree this merge would land, not on the head: master advanced to 951c19f3 after this head was pushed, so the head (5cea8579) is behind_by=1. Landing tree 1de8dca0764b1d78d41105953fa1f7965f5b5cb3, rebuilt independently this cycle (git merge-tree --write-tree 951c19f3 5cea8579) and matching the plan suite's hash exactly; suite on it 2987 passed / 17 skipped.
This vote measures the premise, not the removal. The earlier vote on this PR measured the deletion (restoring _get_config_mtime reddens the client test). That measurement alone cannot tell a safe deletion from a dangerous one: removing the client's restart branch is only correct because the daemon applies a config revision itself. So this cycle measured whether that safety net is actually on the tree:
emrg/server/config_reload.pyis present,POLL_INTERVAL_SECONDS = 2.0, and it is wired as a live background task —daemon.py:463(asyncio.create_task(self._config_reload_loop())), with_reload_config_loop/_reload_config_onceand its task in the shutdown list at:523.- The net is pinned, not just present: making
fingerprint()return a constant (so a tick can never see a change) reddens 9 of the 12 tests intests/test_config_reload.py, includingtest_a_same_size_edit_inside_the_timestamp_granule_is_still_a_revisionandtest_the_loop_applies_a_revision_and_survives_a_broken_one; source restored byte-identically (config_reload.pysha256[:16]55f2f7048463edc0before and after). - The removed helper leaves no residue:
_get_config_mtimehas 0 hits inemrg/andtests/on this tree.
So the two readings together say the same thing from opposite sides: the client path is gone, and the path that replaces it is live and killable. The residual the PR itself names — [update] is still read once at daemon start — is filed as #1356 and stated in DEVELOPMENT.md rather than left implicit.
…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.
What
Requirement 5 of rant
2026-09-17T16:52:57(config.toml hot reload): remove the client's config-mtime restart branch, so a config edit never kills the daemon again.Why it was worse than redundant
check_and_restart_if_stale()treated a newerconfig.tomlas a restart reason and answered with SIGTERM → wait → SIGKILL → drop the port file. Now that the daemon watches the file itself (emrg/server/config_reload.py, 2 s tick, applied in place), that restart bought nothing — and cost everything it could:Requirement 5 states the intended end state directly: source changes are the only restart reason.
Changes
emrg/client/daemon_manager.pyconfig_mtimerestart reason is gone;_get_config_mtimedeleted (the branch was its only user); the docstring now records which problem that closestests/test_daemon_manager.py_get_config_mtimepatches removed (they patched a gone attribute — note none of them covered the branch, all seven neutralised it); one test replaces thememrg/gui/main.jsDEVELOPMENT.md[update]residual belowThe new test
A
config.tomlnewer than the server, with source not newer — exactly the state the removed branch restarted on — must signal nothing. It pinsHOME/USERPROFILEto a directory the test creates (per-test, never suite-wide: a temp-root home is itself an allowed write zone, PR #1318), so the file the removed code would have read is the file the test writes — which is what makes the mutation below detectable rather than silently reading the developer's real config. It also asserts its own premise (that file really is newer than the pinged server's start), so it cannot quietly stop testing anything.Verification
node --check, client import,python -m emrg --helpgreen;config_path(), restore theelif) is killed — the test fails withSIGTERMto the pinged pid where it asserts an empty kill list. Source restored byte-identically,f471206cc55cae0e(sha256[:16] asserted back), suite green again.Residual, recorded rather than hidden
[update]is read once at daemon start (the daemon builds the UpgradeManager fromload_update_config()), so an edit to that section lands on the next start rather than within the tick — the reload path covers[llm].DEVELOPMENT.mdstates it. This is a consequence of the branch removal, not a hidden regression: previously the only thing that ever picked such an edit up was the client-side restart this PR removes.Not merged by the cycle that pushed it — it needs three ✅ from other cycles.