[PR] fix(config): make a hot reload change exactly what it says it changes [skywalking-horizon-ui]

["wu-sheng (via GitHub)" <gi...@apache.org>]

wu-sheng opened a new pull request, #124:
URL: https://github.com/apache/skywalking-horizon-ui/pull/124

   ## Why
   
   `horizon.yaml` is re-read on change and the new values are supposed to take effect without a restart. Three things did not behave that way, and the page describing the behaviour was wrong about several more.
   
   The worst is silent. A reload that finds no file replaces the running configuration with schema **defaults**. At startup that is right — bootstrap validation then tells the operator no users are configured. On a reload it is catastrophic and says nothing, because defaults are a *valid* config: every user, OAP URL, role and signing key is swapped for one and the reload is reported as successful.
   
   ## What
   
   **A vanished file no longer yields defaults.** The reload path refuses a missing file and keeps serving what it has, which is what the documentation always promised.
   
   **A reload that repoints OAP reaches MQE immediately.** Metrics Inspect resolved its target through a 60-second cache that was not keyed by the config it came from, so it kept querying the previous OAP for up to a minute after every other call had moved. It now keys on the OAP settings it resolved from — the shape the capability and time-window caches already use.
   
   **Two code comments asserted the opposite of the code.** The `performance` block claimed its values are clamped to a ceiling; `.max()` *rejects*, so an above-ceiling number exits at boot or bounces the whole reload rather than being pulled down — and `topologyMaxNodes` / `topologyMaxEdges` have no ceiling at all. The template-source flag claimed it is set at boot *and on reload*; it is set only at boot.
   
   **The hot-reload documentation matches the code.** Capability probes were listed as needing a restart, when they are cached for five minutes and keyed by the query URL, so an OAP upgrade is picked up on its own and repointing OAP re-probes at once. `server.staticDir` does need a restart and was missing. `docs/operate/browser-source-maps.md` led a note with "Restart-only knobs" and then said those same three knobs apply live. `debugLog`, `security.trustedLinkDomains`, and a caveat about enabling source maps in a live file were absent from the applied-live list.
   
   **Three config blocks appear in the reference.** `security`, `mcp.name` and `oauth.clientMetadataHosts` are all real and two are documented elsewhere, but none had a row in the section table or the top-level shape — so a reader consulting the reference to learn what is configurable never found them.
   
   **Bundled dashboard docs are catalogued the way the sidebar is.** Forty-six pages sat in one flat alphabetical list, so ActiveMQ preceded Airflow and the six database dashboards were scattered. They are now grouped by the `group` the layer templates already carry, plus Overviews for the two cross-layer dashboards and Application for the general-service and browser ones. The grouping is read from the templates, so the docs cannot drift from what the sidebar shows.
   
   ## Validation
   
   `pnpm -r run lint`, `pnpm -r run type-check`, both app builds, `pnpm license:check` (0 invalid) and **2119 unit tests** across the three workspaces are green.
   
   The reload lifecycle had no tests; it has four now, driving the real watcher against a real file rather than a mocked fs — a valid edit applies and notifies listeners, a malformed one is rejected with the previous config still serving, a vanished file never yields defaults, and a closed source stops watching. `loadConfig` takes an optional write-settle threshold so a case costs milliseconds instead of chokidar's default two seconds, the same seam `SessionStore` exposes for its reaper interval.
   
   Two honest limits. The missing-file case asserts the invariant but could not be made to fail against the unfixed code: only `change` is subscribed, so a deleted file never reaches the reload path — the guard closes a narrow race (a symlink swap mid-stabilisation) that is reachable in principle and not reproducible on demand. The test says so in place, rather than claiming coverage it does not provide.
   
   And the MQE cache change was **not** validated against a live OAP. It is a cache-key change on the Inspect path; the behaviour to confirm is that repointing `oap.*` in a mounted config makes the next MQE execution hit the new target rather than the old one.
   
   An e2e case for hot reload is a reasonable follow-up and is not in this PR: the e2e fixture drives Horizon entirely through `HORIZON_*` environment variables, so proving a *file* reload needs a compose overlay that bind-mounts a literal `horizon.yaml` — fixture work worth doing deliberately rather than shipping unrun.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(config): make a hot reload change exactly what it says it changes [skywalking-horizon-ui]

["wu-sheng (via GitHub)" <gi...@apache.org>]

wu-sheng merged PR #124:
URL: https://github.com/apache/skywalking-horizon-ui/pull/124


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org