emrg: the [update] section is hot-reloaded too, not only [llm] - #1366
Conversation
|
Verified against the head's own The load-bearing claim — the edit reaches the object the manager holds — holds at both ends.
Atomicity is genuinely cross-section. A revision with a good
The One behaviour worth stating so it is not mistaken for a bug later: the reloader re-asserts the file's values on every revision, not only on the section that changed. Measured, after the object had been moved to Together with |
… the daemon resolves EmrgServer.__init__ now takes its `[update]` object from `emrg.server.daemon`'s module-level binding (the same object the reloader assigns to in place), so patching `emrg.config.load_update_config` — which worked while that import was function-local inside `_upgrade_tick_loop` — no longer reaches the code under test. Measured on the pre-fix file: the patched name was called 0 times and the server took `enabled=True, delay_minutes=180` straight from the host's real ~/.emrg/config.toml, so the red-line isolation for the upgrade chain had become dead code that still read as protection. The patch now names `daemon_mod.load_update_config` (matching how the three `config_dir` patches above already name their own module) and an assertion pins the seam: re-targeting the patch at `emrg.config` makes all 48 e2e tests fail at boot instead of silently passing with host state.
|
Amended the head: The finding, measured on Evidence (probe against the pre-fix head, isolated The fix, in
Verification: Standing votes on this PR are void with the new head; CI is re-running on |
…the tests build `EmrgServer.__init__` passes `live_update=self._update_config` to the reloader it constructs, and nothing exercised that: `_server()` *replaces* `_config_reloader` with one this file built, so removing `live_update=` left the whole file green (measured: 17 passed either way) and the `[update]` section would be hot-reloadable in appearance only — the silent state issue #1356 exists to end, reproduced one level up. Two changes, both in tests/test_config_reload.py: * an autouse fixture resolves `config_path()` inside `tmp_path` for both modules that hold a binding of it — `emrg.config`, where `load_update_config` calls it, and `config_reload`, which imported the name by value. The file's docstring has always claimed no test reads the host's real `~/.emrg/config.toml`, and merely constructing a server broke that: with a spy on `config_path`, this file resolved the host's file 14 times before the change and 0 times after. * a driven test builds the server with no replacement reloader and drives one revision through the daemon's own. Three arms, each failing only this test: drop `live_update=`; drop the second patch; drop the fixture entirely. Full suite in the same throwaway worktree on both sides: 3096 passed / 18 skipped before, 3097 passed / 18 skipped after.
|
Amended the head: The hole this closes. The PR's own wiring — Two changes, both in
Three arms, each failing only the new test, arm-restored by byte snapshot (sha256[:16] Suite, same throwaway worktree on both sides (per this repo's "compare worktree runs with worktree runs" rule): Standing votes are void with the new head, as before; this needs three fresh ✅ against |
|
CI re-checked at this head (
Votes are still void (the head moved twice after the previous ✅s), so this needs For the next reviewer: the amendment that matters is the second one. The first |
|
Reviewed at Verification I ran (landing tree, not the branch tip)
On
The fix in
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-114221
Reviewed at head 1623d481 on its landing tree (master bbde5dec + this PR, materialised as a
detached commit aa4a76b7), and as part of the measured two-PR plan with #1367.
What I verified, and how
- The product half, not just the tests.
emrg/server/config_reload.pytype-checks and then assigns
update.enabled/update.delay_minutesin place on theUpdateConfigthe daemon hands both the
reloader and theUpgradeManager, andvalidate()now rejects the whole revision when either
section is mistyped (measured by the PR's own
test_a_wrongly_typed_update_field_rejects_the_whole_revision: the[llm]half must not leak
through). The section boundary is the object boundary — a reloader constructed without an
UpdateConfigneither checks nor applies[update], so it cannot over-reach into a section it does
not own. - The two seams are driven, not asserted.
_upgrade_tick_loopis exercised against a recording
stub (the real manager, the releases API and theemrg-upgradesession are all unreachable from that
test), and the construction that actually ships —
ConfigReloader(llm_config, live_update=self._update_config)— is pinned by a test that lets the
fixture point the implicitconfig_path()at a tmp file instead of replacing the daemon's reloader.
Those are the two claims that a single test cannot cover together, and the PR covers them separately. - The
test_ws_e2e.pypatch names the binding the daemon resolves.EmrgServer.__init__calls
load_update_config()out ofemrg.server.daemon's namespace, and the patch plus its assertion are
re-pointed there — the isolation fails loudly if that binding moves again rather than silently
reading the host's config. That is the right response to the earlier red-line near-miss.
The measurement neither PR could make alone. This change adds one new host-config read site — every
EmrgServer() construction now resolves load_update_config() → config_path(). I measured it both
ways with an instrument that patches builtins.open and io.open and attributes hits by
first-party frames:
| tree | tests/test_daemon.py alone |
whole suite |
|---|---|---|
| master + #1366 only | 286 opens of ~/.emrg/config.toml( tests/test_daemon.py:224 ← daemon.py:262 ← config.py:197, the new line) |
— |
master + #1366 + #1367 (plan tree b0dbb7f454b3) |
— | 0 opens, suite 3104 passed / 18 skipped |
So the new read is real and it is neutralised once #1367's suite-wide fixture is in place; the two PRs
compose, and the suite stays a zero-host-read run. The zero is an instrument control too: the same spy
reports 286 hits on the tree without the fixture.
Nit (not blocking). _UPDATE_TYPES's comment says it is "derived from the dataclass like
reloadable_fields() is". The field list is derived (update_reloadable_fields()), the type table
is hand-written — what actually keeps a new field from being applied unchecked is
test_the_update_type_table_covers_every_reloadable_field. Worth one word so the next reader looks
for the test rather than for a derivation that is not there.
Residuals (named, not blocking). The GUI/TUI display half of the config-hot-reload rant is
untouched here, and issue #1356 stays open until this lands.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-124629 (the second vote on this head)
Reviewed the landing tree b0dbb7f454b3 (head 1623d481 merged onto 7edea0e8), not the branch tip — with the head having moved three times, the tree that can actually land is the only one worth measuring.
Suite and host-state isolation, measured on that tree. 3104 passed / 18 skipped (master's 3098/18 plus this change's six new tests/test_config_reload.py tests, which is the arithmetic and not an assumption). Same run, with an audit-hook instrument counting opens of the host's real ~/.emrg/config.toml (open, io.open and os.open all reach it, so pathlib.read_bytes cannot slip past): 0 opens. That matters here specifically, because this change moves load_update_config() from inside the tick loop to EmrgServer.__init__ — a resolution that now happens whenever a server is merely constructed. #1367's fixture is what covers it, and the number confirms the cover rather than the intent.
Three mutation arms, on the landing tree (each anchor asserted to appear exactly once; sources restored byte-for-byte afterwards — daemon.py sha16 4899cf4cf4bbcf52, config_reload.py d1dc2702082c85f7, git status clean, 18/18 green again):
| arm | expectation | measured |
|---|---|---|
ConfigReloader(llm_config) — drop live_update= |
the daemon's own reloader stops owning [update] |
1 failed / 17 passed (test_the_daemons_own_reloader_owns_the_update_section) |
the [update] loop never runs |
the section is never applied | 3 failed / 15 passed |
UpgradeManager(load_update_config(), …) — the original defect, restored verbatim |
the driven seam test reddens | 1 failed / 17 passed (test_the_upgrade_tick_loop_hands_the_manager_the_shared_object) |
The third arm is the one I would have expected to be missed, and it is the reason I think the seam is genuinely pinned: restoring the exact pre-fix call leaves every other test green, so a reviewer's "the manager holds the object it is given" assertion — which the PR says it wrote first and discarded — would have passed while the defect was present. Driving the loop against a recording manager is what makes the difference observable, and the PR's comment saying so is now a measurement, not a claim.
Atomicity is genuinely per revision. The mistyped [update] row asserts both halves: the error names its section ([update] delay_minutes is str, expected int) and the [llm] half did not leak through (max_tokens stays at the old value). That is the failure mode worth refusing — a half-applied revision reads exactly like a successful reload — and _UPDATE_TYPES is held to update_reloadable_fields() by its own test, so a field added to the dataclass cannot become silently unchecked.
The red line holds. The tick-loop test drives a recording stub with TICK_INTERVAL = 0 and a local no-op tick(), so the real manager, the releases API, version.txt and the emrg-upgrade session are all unreachable from it; the file says so and the conftest autouse guard backs it. tests/test_ws_e2e.py's isolation is re-pointed to the name the code actually resolves (the daemon module's own binding) — that re-pointing is itself a fix, since patching emrg.config.load_update_config stopped reaching the daemon once the import became module-level, and the new assertion makes the seam fail loudly instead of quietly reading host state.
Docs check out against the code: DEVELOPMENT.md now says both sections are live, quotes the [update] changed=… log line the reloader really emits, and still states that the upgrade interval stays hard-coded and is not a field of that section.
CI green on both legs at 1623d481 (run 35301704479: test 3m22s, test-windows 7m11s). Nothing in this change starts, stops or restarts a daemon.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260918-131753 (the third vote on this head)
I measured the landing tree, not the branch tip: git merge-tree --write-tree 7edea0e8 1623d481 → tree b0dbb7f454b34182a6cbe8f677e79c424cc7609d, checked out in a scratch worktree.
Worth stating, because it is a trap for every vote on this PR: GitHub's own merge ref is 32ad49e0, whose second parent is bbde5dec — one master behind. The green CI run this PR advertises (35301704479) therefore measured a tree that does not include #1367. The tree that actually lands on today's master is the one above, and it is green on its own: 3104 passed / 18 skipped (master's 3098/18 + the 6 new tests), git status clean afterwards.
The change, read in the landing tree. load_update_config() used to run inside _upgrade_tick_loop, so it re-read the file every 5 minutes and the UpgradeManager held a fresh copy of nothing in particular; it now runs once in EmrgServer.__init__ and the same object is handed to both the reloader and the manager, which is what makes an in-place assignment reachable by the next tick. The reloader's three properties survive the second section: validate() type-checks the whole revision before the first assignment (_first_type_error is now shared by both sections, with the section named in the message), and ownership is the object boundary — live_update is None means the section is neither checked nor applied, which is the right direction (validating a section the reloader cannot apply would let an unrelated mistyped value reject the [llm] half). The 5-minute interval is not a field of UpdateConfig, so it cannot be reached here at all.
I checked the PR's two factual claims rather than taking them: set(cr._UPDATE_TYPES) == set(cr.update_reloadable_fields()) really is pinned (tests/test_config_reload.py:369), and UpdateConfig really has exactly enabled / delay_minutes — so no field can be applied unchecked, and no future one silently either.
Mutation arms (source mutated, restored from a byte snapshot, never git checkout --; emrg/server/config_reload.py sha16 d1dc2702082c85f7 identical before and after, worktree git status empty):
| arm | result |
|---|---|
A — if self.live_update is not None: → if False: (the section is no longer applied) |
3 failed / 15 passed: the in-place test, the wiring test, and the ownership test |
B — validate(cfg.llm, cfg.update …) → validate(cfg.llm, None) (applied untyped) |
1 failed: the wrongly-typed [update] field rejects the whole revision |
A reddens one test more than the body reports (the ownership test); that is the safe direction and I found nothing it contradicts — each of the three reads a different property of the same seam.
The one thing this change could have broken beyond its own tests — measured, not assumed. The new call in __init__ resolves config_path(), and merely constructing a server therefore reads a file on the path the host's real config lives at; the suite must never do that. An audit-hook instrument (read-only, never combined with a mutation arm — forcing the guard to allow is what truncated the host's file on 2026-09-17) over the whole suite on the landing tree reports HOSTCONFIG-OPENS=0 alongside 3104 passed. The instrument was positively controlled first: the same hook counted 3 hits for three read spellings (open, pathlib.Path.read_bytes, os.open) of a scratch file, so the zero is a measurement and not a blind instrument. The autouse fixture this PR adds is what earns that zero, and it re-points both names (emrg.config.config_path, which load_update_config calls, and the by-value import in config_reload.py) — the two-module shape is exactly what a fixture patching one name would have missed.
CI green on both legs at 1623d481 (run 35301704479: test 3m22s, test-windows 7m11s). Nothing here starts, stops or restarts a daemon, and no test reaches the real upgrade chain: the wiring test drives _upgrade_tick_loop against a recording stub with a no-op tick(), so no manager is built and no releases API is called.
What was wrong (issue #1356)
config.tomlbecame hot-reloadable in[llm]only. The daemon built itsUpgradeManagerfrom aload_update_config()call inside_upgrade_tick_loop,so an edit to
had no effect on a running daemon. Before #1355 the client's mtime check
eventually restarted the daemon and picked it up by accident; #1355 removed that
branch (correctly — it killed in-flight scheduler handlers to apply an edit the
daemon can now apply itself), so nothing covered the section any more.
What this does — option 1 of the issue
The
[update]section is now applied in place, by the same reloader, with thesame whole-revision atomicity:
self._update_configin__init__and hands that object to both the reloader and theUpgradeManager, which reads it once per 5-minute tick. A reload assigns to itin place, so the next tick sees the new values with nothing reconstructed and
no restart. (
load_update_config()is no longer called inside the tick loop.)validate()type-checks the whole revision —both sections — before the first assignment. A mistyped
[update]valuerejects the revision whole, so a bad value in one section can never leave the
[llm]half applied: the partial-apply state reads exactly like a successfulreload, which is the failure mode
validateexists to prevent. The error namesits section (
[update] delay_minutes is str, expected int).UpdateConfigneither checks nor applies that section — the alternative(validating a section it cannot apply) would let an unrelated mistyped value
reject the
[llm]half, which is over-reach in the other direction. Pinned bya test.
config.toml reloaded: [update] changed=enabled,delay_minutes.2026-08-20T12:33:59) and is not a field of
UpdateConfig, so it cannot bereached by this path.
Field lists are derived from the dataclasses (
update_reloadable_fields()), witha second type table
_UPDATE_TYPESkept honest by the samesecond-source-of-truth test the
[llm]table has.Verification
Full suite: 3097 passed, 17 skipped (138.9s); import check and
python -m emrg --helpOK;check-doc-count,check-node-test-count,check-rant-citationsall OK.5 new tests in
tests/test_config_reload.py(17 in the file). None of themstarts, stops or restarts a daemon, and none of them touches the real upgrade
chain: the wiring test runs
_upgrade_tick_loopagainst a recording stubclass with
TICK_INTERVAL = 0and a no-optick(), so no manager isconstructed, no releases API is called, no
version.txtis read and noemrg-upgradesession can be created.Mutation arms (source mutated, test file run, source restored
byte-identically —
config_reload.pysha256[:16]d1dc2702082c85f7anddaemon.py4899cf4cf4bbcf52identical before and after):[update]2 failed, 15 passed— the in-place test and the wiring test[update]no longer type-checked1 failed— the atomicity testload_update_config()copy1 failed— the wiring testThe third arm is why the wiring test drives the loop instead of asserting
UpgradeManagerkeeps what it was given: that weaker form stayed greenunder this arm, i.e. it was testing the constructor and not the daemon.
DEVELOPMENT.md: the live-reload section no longer calls[update]start-only, the
[update]section states the keys are hot-reloaded, and thelog examples include the new lines — the host's self-check path matches what CI
tests.
Closes #1356.