Skip to content

fix(config): derive core health URL from [core].port - #122

Closed
Khaostica wants to merge 4 commits into
GeniePod:mainfrom
Khaostica:fix/derive-core-health-url
Closed

fix(config): derive core health URL from [core].port#122
Khaostica wants to merge 4 commits into
GeniePod:mainfrom
Khaostica:fix/derive-core-health-url

Conversation

@Khaostica

Copy link
Copy Markdown
Contributor

Summary

genie-health and the genie-api dashboard Services row sourced the core probe URL from [services.core].url, while genie-ctl and the genie-api TCP proxy (post-#90 / PR #102) derive it from [core].port via Config::core_http_addr(). When an operator changes only [core].port and leaves [services.core].url at its default, the probes hit the wrong port and report core as DOWN even though it is healthy on the configured port.

Fixes #121.

Changes

  • Add Config::core_health_url() in genie-common — single source of truth for the local core health URL, derived from [core].port and [core].bind_host.
  • genie-health::collect_endpoints() now returns resolved (name, url) pairs and uses core_health_url() for the core entry. LLM and the optional services keep sourcing from [services.<svc>].url so their existing override semantics are unchanged.
  • genie-api::dashboard_service_targets() core entry uses core_health_url() for latency_url.
  • Unit tests in all three crates cover the regression: [core].port = 3001 with a stale default [services.core].url on :3000 must derive :3001.

Behavior is unchanged when the default config is used (port = 3000, default URL).

Real Behavior Proof

  • I have built and run the affected code locally (or noted why I could not).
  • I have verified the change end-to-end on Jetson hardware OR explained the equivalent verification path I used.

What I ran

Windows dev box, no Jetson available, no cross-compile toolchain installed:

  • cargo fmt --all -- --check — clean
  • cargo clippy -p genie-common --all-targets -- -D warnings — clean
  • cargo test -p genie-common38 passed, 0 failed (5 new tests covering core_health_url(): default port, custom port, custom bind_host, listen-all → loopback mapping, and the stale [services.core].url override case).

genie-health and genie-api use Unix-only deps (std::os::unix, tokio signal handlers) and cannot compile on Windows — pre-existing constraint, not introduced by this PR. New regression tests added in each (core_endpoint_url_tracks_configured_core_port in genie-health; dashboard_core_target_uses_derived_health_url in genie-api) will exercise on the Linux CI job.

What I observed

cargo test -p genie-common excerpt:

running 38 tests
test config::tests::core_health_url_uses_default_port ... ok
test config::tests::core_health_url_tracks_custom_core_port ... ok
test config::tests::core_health_url_maps_listen_all_to_loopback ... ok
test config::tests::core_health_url_honors_custom_bind_host ... ok
test config::tests::core_health_url_ignores_stale_services_core_url ... ok
...
test result: ok. 38 passed; 0 failed

Reviewer with Jetson access: please apply the repro from the issue body ([core] port = 3001, leave [services.core].url at the :3000 default) and confirm genie-health logs and the dashboard Services row both report core healthy on the new port.

Test plan

  1. Apply the issue's repro config: override [core] port = 3001, leave default [services.core].url.
  2. Start the stack with GENIEPOD_CONFIG=... for genie-core, genie-health, genie-api.
  3. genie-ctl health and genie-health logs should agree — core healthy on :3001.
  4. Open the dashboard Services panel — core row should be healthy.
  5. Restore default config (port = 3000, default URL) and confirm no regression.

Notes for reviewers

  • [services.core].url is intentionally left in the config schema for backward compatibility. The issue body suggested logging a divergence warning at Config::load(); deferred from this PR to keep scope tight on the bug fix — happy to follow up if you'd like it.
  • LLM and the optional services (Home Assistant, Nextcloud, Jellyfin) still read from [services.<svc>].url — only core is special-cased because only core has a dedicated [core] config block as the listen-side source of truth.

genie-health and the genie-api dashboard Services row both sourced the
core probe URL from [services.core].url, while genie-ctl and the genie-api
TCP proxy (post-GeniePod#90 / GeniePod#102) derive it from [core].port via
Config::core_http_addr(). When an operator changed only [core].port and
left [services.core].url at its default, the probes hit the wrong port
and showed core as DOWN even though it was healthy on the new port.

- Add Config::core_health_url() in genie-common that returns
  "http://{core_http_addr()}/api/health". Single source of truth shared
  by every local core health probe.
- genie-health: collect_endpoints() now returns (name, resolved_url) and
  uses core_health_url() for the core entry. LLM and optional services
  keep reading [services.<svc>].url so their existing override semantics
  are preserved.
- genie-api: dashboard_service_targets() core entry uses core_health_url()
  for latency_url so the Services row tracks the configured port.
- Tests: genie-common covers default/custom port, custom bind_host, and
  the listen-all-mapped-to-loopback case. genie-health and genie-api
  each have a regression test asserting the core URL follows [core].port
  when [services.core].url is left stale, plus a sibling test confirming
  LLM still sources from [services.llm].url.

Fixes GeniePod#121.
@ai-hpc

ai-hpc commented May 22, 2026

Copy link
Copy Markdown
Contributor

Not merging this yet. After refreshing the branch against current main and approving the fork checks, cargo test --workspace --locked --all-targets failed in GitHub Actions.

Failure:

checker::tests::llm_endpoint_url_still_sources_from_services_config panicked at crates/genie-health/src/checker.rs:370:50:
called `Result::unwrap()` on an `Err` value: database is locked
Error code 5: The database file is locked

This looks like the new genie-health tests can contend on the same SQLite path when the test binary runs tests in parallel. Please isolate the health monitor data directory per test, or restructure the endpoint-collection assertions so they do not open a shared runtime DB. Cross-compile and clippy are green, but I cannot merge with a failing workspace test job.

@ai-hpc ai-hpc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes because the refreshed branch has a failing required test job: cargo test --workspace --locked --all-targets fails in checker::tests::llm_endpoint_url_still_sources_from_services_config with database is locked. Please isolate the new health-check tests from shared SQLite state or avoid opening a shared runtime DB in these endpoint assertions, then refresh against current main.

The three checker unit tests each constructed a HealthMonitor via
HealthMonitor::new(test_config()) to reach collect_endpoints, which opens
the SQLite log DB at config.data_dir. Under the parallel cargo test
runner all three would race for the write lock on the shared
/tmp/geniepod-health-test/health.db path, producing
"database is locked" failures in CI (workspace test job).

collect_endpoints only reads from &Config — it never touches self.db or
self.failure_counts — so make it a module-level free function taking
&Config. check_all calls it as collect_endpoints(&self.config); the three
tests now call collect_endpoints(&test_config()) (or a mutated config)
directly and never open the DB.

Pure refactor: behavior of HealthMonitor is unchanged.
@Khaostica
Khaostica requested a review from ai-hpc May 22, 2026 16:03
@ai-hpc

ai-hpc commented May 23, 2026

Copy link
Copy Markdown
Contributor

Reviewed and closed: this PR is conflicting with current main and already has requested changes, so it is not safe to merge as-is. Please reopen or resubmit a rebased branch if you want to continue it; thanks @Khaostica.

@ai-hpc

ai-hpc commented May 23, 2026

Copy link
Copy Markdown
Contributor

Reviewed and superseded: #121 was valid, but this branch is conflicting and maintainer edits are disabled, so #122 could not be merged directly. Ported and merged the valid fix in #158 at 99650b1; thanks @Khaostica.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[services.core].url drifts from [core].port — health monitor and dashboard probe wrong port

2 participants