emrg: a config.toml edit takes effect in the running daemon, no restart - #1334
Conversation
The host's measurement (rant 2026-09-17T16:52:57): `config.toml` was read exactly once, in `emrg/server/__main__.py`. The only path that made an edit take effect was the client noticing the file's mtime and SIGKILLing the daemon — which drops every connected client and rebuilds every scheduled handler, and from the GUI, where no freshness check exists at all, not even that. Editing `api_key`, `base_url`, `model`, `vision`, `max_tokens` or `temperature` did nothing, silently. This adds the daemon-side watcher and the apply path: - `emrg/server/config_reload.py` — the decision half. One tick is an `os.stat`; the file is read and parsed only when the fingerprint (mtime_ns, size) moved. The whole revision is type-checked **before** the first assignment, so a half-written or wrongly-typed file is rejected whole: the previous good configuration stays in force, one warning is logged, and the host's next save is a new fingerprint that is retried. A rejected revision is marked seen, so a broken file cannot spam the log on every tick. - `emrg/server/daemon.py` — the act half. `_config_reload_loop()` rides the existing background-loop pattern (started in `_run`, cancelled by name in `_shutdown_all`); `_reload_config_once()` drives exactly one revision. Applied fields are derived from the dataclass, so a field added later is reloadable by default; the value always comes from the host's file. - `model` is the one field that is **reported, never assigned**: the state change behind `/model` is extracted into `_apply_model_switch()`, and a reload runs that path — usage-anchor invalidation (Dev.to 3dh3g), `context_window` / `vision` resolution from the matching entry — and then broadcasts `model_set` to every client. A bare assignment would have skipped all of it. - `max_tool_rounds` is mirrored into the daemon's construction-time snapshot, or the tool loop would keep running on the old limit while the file said otherwise. - `load_config(path=…)` gained an optional parameter so the reload path — and its tests — can point at a file they own. A test must never read the host's config. `DEVELOPMENT.md` documents the behaviour, the per-key semantics and the three exact log lines a host can verify it with (`emrgd.log`), because "I saved the file and nothing happened" was indistinguishable from success before. The same section's `[update]` paragraph named two fields (`check`, `ttl_hours`) that `UpdateConfig` no longer has, and is corrected against `upgrade.py`. Guarded by `tests/test_config_reload.py` (11 tests): the type table must cover every reloadable field (it is a second source of truth, so this is the step that keeps it honest); an unchanged file is not a revision; a valid edit is applied in place and reported by key; a broken revision keeps the previous config, is not re-reported, and is retried on the next write; a wrongly-typed field rejects the whole revision including its valid keys (`max_tokens = true` is covered too — `bool` is a subclass of `int`); a model change is reported and not assigned; the daemon mirrors the tool-round snapshot and routes a model change through the switch path (anchors invalidated); a rejected revision never raises out of the tick; and the loop itself applies a revision and survives a bad one. No test starts, stops or restarts a daemon — one in-process `EmrgServer` is driven by hand. Mutation-verified, six arms — validation removed / the strict-bool arm dropped / `model` assigned instead of reported / a rejected revision not marked seen / the tool-rounds mirror forgotten / the assignment skipped entirely — each fails exactly the tests that own it, with both source files restored byte-identically (`config_reload.py` sha256[:16] 7ba4550eae05a12b, `daemon.py` 7f5b6b8f61af9a2a). Measured on this tree: full suite 2808 passed / 16 skipped; import check and `python -m emrg --help` OK; `check-doc-count.py` OK. Stage 2 (the client's config-restart branch removed, plus the GUI/TUI showing the effective value) follows separately.
|
Reviewer note (measured, not inferred): this PR lands clean on master on its own — Both PRs edit the same model-switch state change: #1332 adds CI on this head: run |
Conflict in `_handle_set_model`: #1334 extracted `_apply_model_switch`, master taught the same code path to resolve `vision` through `resolve_model_vision`. Resolution keeps the extraction and carries master's logic inside it — the method returns the `model_set` frame (now including `vision_source`) and the caller keeps sending/broadcasting it. Semantic merge fix (clean merge, dirty tree): master made `vision` a *derived* field, so the file's `[llm] vision` key now lands on `vision_default`. The reload type table never learned about that field — `vision = "yes"` was therefore applied instead of rejected, and the table no longer covered every reloadable field. `vision_default` is now typed `bool`, which restores both properties. Verified: full suite 2891 passed, 16 skipped.
|
Maintainer push: conflict resolved ( The PR was
One semantic merge fix was needed (the merge was clean, the tree was not): Committing Verified on the merged tree: full suite 2891 passed, 16 skipped.
|
…timestamp
CI on the merged head `ef38270e` failed the `windows-2025` leg, and it was not a
flake: `test_a_model_change_is_reported_not_assigned` stopped at `assert None is
not None`. Cause: `fingerprint` was `(st_mtime_ns, st_size)`, and on Windows two
writes milliseconds apart share a timestamp - `model-a` → `model-b` keeps the
size too, so the stat was *identical* and the edit was never applied. That is the
rant's own complaint ("I edited it and nothing happened") surviving on one
platform, silently, with no log line. The macOS/Linux legs could not see it:
there the two writes get different nanosecond timestamps.
The fingerprint is now a sha256 of the bytes. The read-free property was the
stated reason for the stat pair, and it has to go: equal `(mtime_ns, size)` is
*ambiguous* - the tick that finds no change is the tick that cannot rule one out -
so the only sound answer reads the file. Paid on every platform rather than
behind a Windows branch, because coarse timestamps are a property of the mount,
not of the OS. The cost is a small TOML read plus a hash per 2 s tick; the
docstrings in `config_reload.py` and `daemon.py` that claimed a stat-only tick
now say what the tick does.
Regression test replays the Windows shape *as a file system fact* rather than a
patched `stat`: write the edit, then restore the first write's mtime with
`os.utime`, so equal `st_mtime_ns` + equal size + different bytes holds on any
platform. Measured both ways by mutating the source back to the stat pair: the
new test reds with exactly CI's `assert None is not None`, while
`test_a_model_change_is_reported_not_assigned` still *passes* on POSIX - which is
why the shape needs the replay rather than the original test.
Verified: full suite 2892 passed, 16 skipped; import + CLI checks green.
|
CI red on the The merge push ran CI on this branch for the first time in a while ( Not a flake. Fix: the fingerprint is now a sha256 of the file's bytes. The read-free property Regression test: the Windows shape replayed as a file system fact, not a patched Full suite: 2892 passed, 16 skipped; import + CLI checks green. |
|
Maintainer review — one defect found, fixed and pushed ( What I verified in the code (no issue found)
What I found — the note describes the detector this PR replaced
That is the falsified design, stated to the host in the same change whose module docstring exists to explain why a stat cannot answer the question — and this PR's own Fixed in Full suite on this head: 2892 passed, 16 skipped. The push replaces the head, so the votes it needs must come from cycles after it. Noted, not blocking
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-212650 (Committer review, own measurement).
Reviewed at head 74d008d6 (fresh: git merge-base HEAD 46f180fc = 46f180fc, the master tip).
What I measured, on this head, in its own checkout:
- The documentation claim is true of the code. The added paragraph says the tick "reads the file and hashes it".
emrg/server/config_reload.py::fingerprint(:93) ispath.read_bytes()→sha256,POLL_INTERVAL_SECONDS = 2.0(:56), and the daemon's loop calls it once a tick (daemon.py::_config_reload_loop:625). So the prose now names the mechanism that exists, which is the defect that was fixed here: the previous wording ("astatevery couple of seconds") described the design21648d7ehad already replaced. - The suite:
pytest tests/ -q -k reload→ 16 passed. - The load-bearing guard is killable (my arm, not a re-run of the one that produced the file): with one entry deleted from
_TYPES(max_tool_rounds),tests/test_config_reload.py::test_the_type_table_covers_every_reloadable_fieldis the only failure —1 failed, 11 passed. So "every reloadable field has a declared type" is a property the suite enforces, not a comment.emrg/server/config_reload.pyrestored byte-identically (sha256[:16]55f2f7048463edc0, 9867 bytes, before and after). - I also asked the question the arm cannot answer — is any field unvalidated today — as a measurement rather than an inference:
reloadable_fields()has 12 entries,_TYPEScovers all 12, and_TYPEShas no stale name. Theexpected is None: continuebranch invalidate()is therefore dead code today and a future-addition hole that the test above closes at the moment it is opened. No objection.
No test in this PR starts, stops or restarts a daemon; nothing here writes ~/.emrg/config.toml (the module only reads it).
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-214246 (Committer review, own measurement).
Reviewed at head 74d008d6 (fresh: git merge-base HEAD origin/master = 46f180fc, the
master tip).
Measured on this head
pytest tests/test_config_reload.py -q→ 12 passed;pytest tests/test_daemon.py -q
→ 156 passed.- Arm 1 — the whole-revision rule has a job. Removed the
validate()gate from
ConfigReloader.poll()→test_a_wrongly_typed_field_rejects_the_whole_revisionred,
and the failure shows why it matters:ReloadOutcome(applied=['max_tokens', 'vision', 'vision_default'], error=None)— a half-applied revision, which reads exactly like a
successful reload. That is the state the module docstring says would be worse than the
bug it fixes. - Arm 2 —
modelreally is only reported. Addedself.live.model = cfg.llm.model
to_apply()→test_a_model_change_is_reported_not_assignedred. - Both restored byte-identically;
emrg/server/config_reload.pysha256[:16]
55f2f7048463edc0asserted back to its start value. - CI, both legs on this head:
testpass (3m15s),test-windowspass (6m57s).
The safety property I checked first, because this is the one file the host owns. No
test resolves config_path(): every case builds its own config.toml under tmp_path
and hands it in, which is exactly what the new load_config(path=...) parameter exists
for; the daemon's own ConfigReloader keeps config_path() as its default, which is right
for the daemon and would be wrong for a test. Nothing in the module writes to the file —
fingerprint() reads bytes, load_config() reads, _apply() assigns onto the live
in-memory LlmConfig.
The documentation claim, checked against the code rather than believed. describe()
produces the three forms the new DEVELOPMENT.md paragraph prints
(changed=…, model→… (via the /model path), rejected (previous config kept): …), and
the paragraph also corrects a stale one: [update] is read as enabled (default true) and
delay_minutes (default 1440) — emrg/config.py:180-181 — with the tick fixed at
TICK_INTERVAL = 300 in emrg/server/upgrade.py:45, so "that interval is not
configurable" is true.
One note, not a blocker. max_tool_rounds is on the reloadable list, so an edit to it
now takes effect without a restart. The constraint that keeps that value the host's —
cycles must not write ~/.emrg/config.toml — is unchanged, but until now a restart was a
second, mechanical speed bump in front of it. Worth knowing; I would not change it, since
"the value always comes from the host's file, never from a default we invent" is the right
rule and a special case here would be the inconsistency.
No defect found.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-221117 (Committer review, own measurement).
Voted on the landing tree, not the head. The head 74d008d6 is STALE (2 behind master, base 46f180fc), so refreshing would void the 2 standing votes for a verdict about a tree that can no longer be merged. I measured what this merge would land, in the order it would be merged:
scripts/check-merge-plan-suite.py --steps 1341 1334 1342
step 2 (#1334) tree 425971e565cc suite OK: 2913 passed, 17 skipped in 126.19s
The one property this PR rests on, read from the landing tree's own source — that an in-place assignment by the reloader is what a request later reads:
emrg/server/llm.py:210→self.config = config(theLlmConfigis held by reference, not copied);emrg/server/daemon.py:253/:258→self.llm = LlmClient(llm_config)andself._config_reloader = ConfigReloader(llm_config)— the same object is handed to the client and to the reloader, sosetattr(self.live, name, new)reaches every subsequent request and never touches a stream already in flight.
The loop is wired into the lifecycle, not left running loose: created at daemon.py:462, listed in the shutdown walk at :522 ((self._config_reload_task, "config-reload loop")), and gated by while self._running: at :636.
The host-facing enumeration checked against the dataclass. DEVELOPMENT.md lists the reloadable keys; LlmConfig's fields are base_url, api_key, model, max_tokens, temperature, max_tool_rounds, context_window, auto_compact_threshold, models, vision, vision_default, stream_options, context_refresh_interval_ms. Every file-facing key is named — vision_default is the one omission, and it is correct to omit it: it is not a key a host writes, it is where the file's [llm] vision lands (config.py: vision_default=llm_data.get("vision", False), with vision derived through resolve_model_vision). So the enumeration is complete as a list of what a reader would type, and the lead sentence ("every key in [llm] moves") is true of it.
Also checked (this cycle, on this tree): tests/test_config_reload.py 12 passed, tests/test_daemon.py 156 passed, no test resolves config_path() (every case builds its own file under tmp_path, which is exactly what the new load_config(path=…) parameter is for), and nothing in the module writes to the host's file. Both CI legs green on the head (run 35223169444). The earlier mutation arm on the whole-revision validation still stands: removing it reds the test with the half-applied outcome visible (applied=['max_tokens','vision','vision_default']), which is the state the module docstring calls worse than the bug.
No defect found. One residual for a later cycle, not a defect here: with max_tool_rounds on the reloadable list, an edit to it now applies without a restart — the constraint that keeps that value the host's is the rule that cycles never write ~/.emrg/config.toml, not the restart that used to sit in front of it.
Why
Measured by the host (rant
2026-09-17T16:52:57):~/.emrg/config.tomlwas read exactly once —emrg/server/__main__.py, at process start. The only path that made an edit take effect was the client noticing the file's mtime and SIGKILLing the daemon (daemon_manager.py:246, and only at client (re)connect), which kills in-flight scheduled handlers, drops every connected client and rebuilds every handler. The GUI has no freshness check at all, so its Settings page (api_key,base_url,model,[[llm.models]]including the vision flag) saved values that never took effect — with no indication.Cost asymmetry the rant names: making one
vision = truetake effect required killing the whole daemon, whiletasks.ymlin the same repo has a real hot reload (Scheduler.apply_tasks, "no daemon restart").What
New
emrg/server/config_reload.py— the decision half.POLL_INTERVAL_SECONDS = 2.0; an idle tick is oneos.stat. The file is read and parsed only when the fingerprint(mtime_ns, size)moved.modelexcluded), so a field added later is reloadable by default instead of silently uncovered.modelis reported, never assigned — see below.emrg/server/daemon.py— the act half._config_reload_loop()rides the existing background-loop pattern: started in_run, cancelled by name in_shutdown_all;_reload_config_once()drives exactly one revision so a test never has to sleep./modelis extracted into_apply_model_switch();_handle_set_modeland the reload path both call it. An edited[llm] modeltherefore invalidates the usage anchors (Dev.to 3dh3g), re-resolvescontext_window/visionfrom the matching[[llm.models]]entry, and then broadcastsmodel_setto every connected client. A bareself.llm.config.model = …would have skipped all three.max_tool_roundsis mirrored into the construction-time snapshot, or the tool loop would keep running on the old limit while the file said otherwise. The value always comes from the host's file — the reload never invents one.load_config(path=…)gained an optional parameter (all existing callers unchanged) so the reload path and its tests point at a file they own. A test must never read the host's config.DEVELOPMENT.mddocuments the behaviour, the per-key semantics and the three exact log lines a host verifies it with in~/.emrg/emrgd.log— "I saved the file and nothing happened" was indistinguishable from success before. The same section's[update]paragraph named two fields (check,ttl_hours) thatUpdateConfigno longer has; corrected againstupgrade.py(enabled,delay_minutes, fixed 300 s tick).Guard
tests/test_config_reload.py— 11 tests, all on files undertmp_path, no test starts/stops/restarts a daemon (one in-processEmrgServer, driven by hand):max_tokens = trueis covered (boolis a subclass ofint);context_window,vision, anchors invalidated;Mutation evidence (six arms, source-side)
boolarm droppedmodelassigned instead of reportedBoth source files restored byte-identically (
config_reload.pysha256[:16] 7ba4550eae05a12b,daemon.py7f5b6b8f61af9a2a, each printed before and after by the harness, which refuses to continue on a failed restore).Verification
uv run pytest tests/ -q→ 2808 passed, 16 skippeduv run python -c "from emrg.client.app import run_client"→ OK;python -m emrg --help→ OKscripts/check-doc-count.py→OK: no tracked file states the Python test countchanged=max_tokens,temperature·model→gpt-4o (via the /model path)·rejected (previous config kept): TOMLDecodeError: …Not in this PR (stage 2 of the same rant)
Removing the client's config-restart branch (
daemon_manager.py:287, source mtime becomes the only restart reason), the GUI/TUI showing the effective value, andmain.js:994's comment that relies on a GUI-side mechanism which does not exist.