Hot reload the models config block when the config file changes - #2377
Hot reload the models config block when the config file changes#2377Devin-Holland wants to merge 7 commits into
Conversation
An orchestrator can now rotate a credential or re-point a baseUrl by rewriting the models block of harperdb-config.yaml; each worker watches the file (the same RootConfigWatcher pattern the logger uses) and reprojects its registry with no restart. Changed entries are rebuilt through the same factories boot uses and swapped in atomically; removed entries stop serving; programmatic registrations that overrode a config entry keep their documented precedence; fallback routing is rebuilt with the block. If HARPER_DEFAULT_CONFIG / HARPER_CONFIG / HARPER_SET_CONFIG also defines models, the file alone is not authoritative for the block and hot reload stays off — which doubles as the compatibility gate: an orchestrator still injecting models through HARPER_SET_CONFIG keeps today's restart behavior, one that writes the file instead gets live reload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces hot reloading for the models configuration block, enabling credential rotation and endpoint updates without requiring a process restart. It adds atomic conditional replacement and removal of backends using AsyncLocalStorage to capture registrations during construction, alongside a file watcher to apply updates. A review comment correctly identifies a high-severity issue where the isBoot flag can be lost during coalesced applies if a reload is queued immediately after a boot, and provides a robust suggestion to make the flag sticky within the coalescing window.
A watcher event landing while a re-bootstrap sat queued overwrote the pending apply wholesale, silently demoting boot to reload semantics and losing its occupant-overwrite contract (found by review). Merging the flag instead would launder the raw watcher block through boot semantics, past reload validation and the missing-key no-op. Each lane coalesces latest-wins on its own; boot drains first, the newest reload then refines it under its own rules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
The gate test only exercised HARPER_SET_CONFIG; a typo in either other layer name would silently un-gate that layer. Parametrized over all three, mutation-verified (misspelling the untested names fails two tests). Found by CI review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An onSnapshotObserved test seam marks the moment a watcher snapshot is observed, so the test can arm the settle timer, boot before it fires, and assert the pre-boot content is discarded — converting the disclosed review-covered guard into a mutation-verified one. Suggested by CI review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kriszyp
left a comment
There was a problem hiding this comment.
This looks good, and I approve of moving forward with this. However, I will note that this PR inherently creates an interesting situation: Currently it is targeting 5.3, and I think that is the right target for a PR of this size that is adding functionality. But that means you will still need to employ (thread) restarts on 5.2 servers to adopt backend config changes.
🤖 Reviewed with Codex
…tcher Three findings from review. Boot publishes each entry as it is built, in config order, so a later module factory observes earlier entries (a wrapper resolves its base) exactly as before this feature; reload keeps the staged atomic publish but runs factories for built-ins only, since module factories may compose across entries and staged construction cannot honor that — changing one keeps restart semantics. A helper whose name a config entry claims is suppressed rather than forgotten, and restored the moment the claiming entry is removed, so the live registry matches a restart with the final config. Models rides the isolate-shared RootConfigWatcher instead of opening a second native watcher per worker; logging shares the same instance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Re the 5.2 note in Kris's review: agreed, and the host-manager follow-up is scoped exactly that way — HM only switches from HARPER_SET_CONFIG injection to file writes for instances whose core version advertises this capability (5.3+); 5.2 instances keep today's compose-recreate path. Mixed fleets need no negotiation because the env-layer gate in this PR keeps hot reload off wherever HM still injects models through HARPER_SET_CONFIG. — Claude (devain) |
kriszyp
left a comment
There was a problem hiding this comment.
Looks like one fix, but not a blocker. Sounds like we are in agreement on target.
🤖 Reviewed with Codex
A module-backed primary removed or renamed out of the config block is retained and keeps serving (restart-managed removal), but clearFallbackGroups() wiped its routing and neither rebuild loop could restore it — the slot is absent from both desiredKeys and presentKeys. Failover was silently dropped while the primary kept answering. Track retained module keys and restore their recorded fallback group after the clear. Co-Authored-By: Claude Opus <noreply@anthropic.com>
The restart-only guard checked only the incoming backend, so rewriting a module-backed entry to a built-in passed staging and publishEntry, live-replacing the custom backend and retiring its helpers with none of the disposal a restart performs — contradicting the InstalledSlot invariant that module-backed entries require a restart to add, change, OR remove. Extend the guard to also refuse when the currently-installed slot is module-backed, keeping the module and its fallback routing until a restart. Co-Authored-By: Claude Opus <noreply@anthropic.com>
| }, debounceMs); | ||
| pendingSettle.unref?.(); | ||
| }; | ||
| modelsConfigWatcher.on('ready', applyFromFile); |
There was a problem hiding this comment.
Suggestion (non-blocking): this subscribes with a raw .on('ready', applyFromFile) rather than gating on the ready promise, the way harper_logger.ts's updateLogSettings() does (await rootConfig.ready before attaching 'change', at utility/logging/harper_logger.ts:171-175).
RootConfigWatcher#config (config/RootConfigWatcher.ts) flips from undefined exactly once per instance; only that first handleChange() fires 'ready', every later one fires 'change'. modelsConfigWatcher here is usually the shared singleton, and logging is typically the first consumer to construct it. If the shared instance's one-time 'ready' already fired (e.g. logger init resolving await rootConfig.ready before component loading reaches this line) before this .on('ready', ...) attaches, the event is gone for good — EventEmitter doesn't replay past emissions to a late listener — so this code never sees it.
Impact is narrow, not a correctness break: bootstrapModels(config) already applies the real initial snapshot independently of the watcher, and any write after the missed one still emits 'change' (which this code does catch), so it self-heals on the next config write. The only loss is a rewrite landing in the gap between Harper's boot-time config read and the shared watcher's first scan completing, with no further writes before the next restart.
Might be worth mirroring the ready-promise gating, or synchronously checking modelsConfigWatcher.config right after obtaining the shared instance and calling applyFromFile immediately if it's already populated, so a second consumer can't miss the one-time signal.
Closes #2344.
The
models:block ofharperdb-config.yamlnow hot-reloads when the file changes: an orchestrator (or a self-hosted operator) can rotate a credential, re-point abaseUrl, or add/remove an entry by rewriting the file, and every worker reprojects its registry with no restart. For Fabric that removes the container recreate — and the GTM drain — that a models change costs today (central-manager#753).Boot publishes each entry as it is built, in config order — module factories keep their order-dependent composition (a
cachedwrapper still resolves its earlierbase), exactly as before this PR. A reload stages changed entries off to the side through the same factories (constructBackenddiverts the factory's install — including its helper registrations — via an AsyncLocalStorage capture scope), then one synchronous publish tick swaps them in conditionally, removes entries absent from the block (helpers cascade; a helper whose name a config entry claimed is suppressed, and restored the moment that entry is removed), and rebuilds fallback routing — so a request observes either the previous projection or the new one, never a partial rebuild. Reload runs factories for built-ins only: module-backed entries are restart-managed in both directions (add, change, and remove are refused with a warning), because a module factory may compose across entries and staged construction cannot honor that ordering — review decision with Kris. Applies are serialized; boot and reload coalesce latest-wins in separate lanes (a boot supersedes reloads observed before it — including a pending debounce snapshot, whose invalidation-on-boot guard is mutation-verified through anonSnapshotObservedtest seam — while a reload arriving after still refines under reload rules), and a reload snapshot is validated against the same JoimodelsSchemaboot enforces (hoisted out ofconfigValidator,allowUnknownmirrored) — a snapshot boot would reject keeps the prior projection wholesale. One isolate-sharedRootConfigWatcherdrives it (getSharedRootConfigWatcher()), consumed by both logging and models — no second native watcher per worker.Removal semantics are deliberate and partial-write-hardened.
models: {}(or a shrunken block) removes built-in-backed entries (module-backed ones are restart-managed, above);models: null(a baremodels:key) is rejected exactly as boot's validator rejects it, so a reload can never accept a file the next restart would refuse; a snapshot with nomodelskey is a no-op, because a non-atomic in-place rewrite can be observed as a valid YAML prefix that hasn't reached the block yet, and adopting that would tear down every backend. The watcher additionally applies only the last event in a 150ms quiet window. The orchestrator contract is still an atomictmp+renamewrite.For the human reviewer
The compatibility gate is the design's hinge. If
HARPER_SET_CONFIG/HARPER_CONFIG/HARPER_DEFAULT_CONFIGalso namesmodels, hot reload stays off — the watcher sees the raw file, env layers compose over it at boot, so the file alone is not authoritative for the block. This is deliberately also the migration story: a host-manager still injecting models throughHARPER_SET_CONFIGkeeps today's restart semantics untouched; one that writes the file instead (the follow-up HM change) gets live reload. Mixed fleets need no version negotiation.Boot and reload deliberately differ in one respect. Boot overwrites whatever occupies a slot (the documented
bootstrapModelscontract); a reload swaps only against the instance the config projection itself installed, so a programmaticregisterBackendthat shadows a config entry keeps its documented precedence and a reload can never clobber it. Removal is equally guarded (removeIfCurrent).Change detection is per-entry raw-JSON equality. An unrelated config edit (logging level, ports) re-runs no factories; a key-reorder inside an entry would spuriously rebuild it — harmless for built-ins (pure constructors), and module factories are only re-invoked when their own entry text changes.
Carried, not fixed — two ledger items for your judgment. (a) Helper (
extras) ownership is publish-order and history dependent — inherent to conditional-swap semantics; the concrete failure modes found in review are fixed and tested, the abstract property stands. (b)bootstrapModelsno longer rejects at boot on a wholesale failure (it logs at error and continues) — this matches the file's documented per-entry design, and the only reachable systemic throw is a non-YAML poisoned block, but it is a behavior change frommain.Review history, for the record. Seven implementation review rounds (codex/gemini/harper-domain via the pre-push CLI, plus two standalone Cursor-Grok legs after the CLI's per-branch Cursor cap was spent on the abandoned design). They produced 5 majors, ~20 minors, and one Grok blocker (partial-write adoption), all fixed with mutation-verified tests or declined with a stated contract rationale.
Planning history, for the record. Eight planning-review rounds on earlier designs (secrets-store credential rotation) never cleared the framing gate; the owner then clarified the actual goal — replace
HARPER_SET_CONFIGinjection with file writes that hot-reload — and that design was abandoned. What survived from it are the registry primitives (constructBackend/replaceIfCurrent/removeIfCurrent), which are exactly the staged-apply mechanics this needs.Docs: user-facing behavior (the models block hot-reloads; env-layer pinning). Companion documentation PR to follow together with the host-manager change, since the operator guidance (write via tmp+rename, move models out of
HARPER_SET_CONFIG) is about the orchestrator side.Verification
Live, on a running instance —
harper dev5.2.5 from this branch,threads_count=2, isolated scratch HOME/rootPath; a component with the documented shape (import { models } from 'harper', aResource.get()callingmodels.embed) and a stub provider recording everyAuthorizationheader Harper sends:models:block to the live config file →/Embed/went 500ModelBackendNotFoundError→ 200 in ~1s, stub saw the configured keyapiKey(plain write) → requests carry the new bearer in ~1srename(how an orchestrator should write) → ~1sModelBackendNotFoundErrorin ~1sThat live run predates the final review rounds; behaviors added after it (reload validation,
models: nullsemantics, whole-key-absence no-op, the 150ms settle window, helper lifecycle) are covered by the unit suite below, and the file-watch path itself is exercised end-to-end by a real-chokidar test. Happy to re-run the live smoke on request before merge.Unit —
unitTests/resources/models/modelsConfigReload.test.js(28 tests): rewritten credential reaches a realembed()throughresolveEmbedding(fetch-stubbed); unchanged entries keep their instance; removed entries stop serving while a present-but-malformed entry retains its last applied backend; application overrides survive reload and removal; a failed rebuild keeps the previous backend and its fallback routing; fallback routing updates with the block; rapid applies coalesce latest-wins (asserted with no settling re-apply, so a dropped queued update fails it); a poisoned block neither rejects into the watcher's fire-and-forget call nor blocks the next apply; the env-pin gate stays off underHARPER_SET_CONFIG; and a real-file chokidar end-to-end (temp config,getConfigFilePathpatched without sinon, the sharedwaitForhelper).unitTests/resources/models/backendRegistry.test.jscovers the capture scope (concurrent constructions stay separate; an unrelated registration mid-capture installs normally; capture survives a throwing factory) and both conditional primitives.Mutation checks (against
dist, which the tests actually load) — unconditional swap, disabled removal pass, disabled change-diff, disabled env gate, malformed-treated-as-absent, drain-catch removal, and dropped-queued-update each fail their targeted tests; baseline restored green.Boot parity — the full models suite (431 tests, including the pre-existing
bootstrap.test.jsboot contract: overwrite semantics, fallback clear-on-rebootstrap, module specifier resolution) passes unmodified.Suites that could not run locally:
test:unit:components/test:unit:configfail at load in this checkout on pre-existing ESM-only dependency issues (chai,@harperfast/skills) — verified pre-existing by running untouched files;package-lock.jsonis unmodified. CI runs them.Complexity: moderate
Review-Coverage: authored=claude; ran=codex; blocked=gemini(auth); declined=cursor-grok,cursor-composer,domain; rounds=20 @ 40052f3
Human-Review-Need: 3 @ 40052f3