Skip to content

Commit dceab17

Browse files
kingpanther13claudecoderabbitai[bot]
authored
fix: replace the stdio settings sidecar on every startup and keep its URL stable (#2134)
* fix: replace the stdio settings sidecar on every startup instead of reusing it Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qhw1SVHGJZw2Pj9ifdJgN * feat: persist the sidecar port and secret so the settings URL survives replacement Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qhw1SVHGJZw2Pj9ifdJgN * test: address CodeRabbit round 1 (dedupe uvicorn stub, pin-free settings stub, exit-wait timeout coverage) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qhw1SVHGJZw2Pj9ifdJgN * fix: address Codex round 1 (ownership-guarded cleanup, proxy bypass, malformed-url guard, legacy ui.state seed, doc accuracy) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qhw1SVHGJZw2Pj9ifdJgN * fix: pid-based ownership + inter-process lock for serving-file cleanup (CodeRabbit round 2) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qhw1SVHGJZw2Pj9ifdJgN * fix: address opus review round (bind-first port acquisition, truthful shutdown errors, hardened edges, doc accuracy, test gaps) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qhw1SVHGJZw2Pj9ifdJgN * fix: address Codex round 2 (sentinel request ownership, winner-only cache dump, listen-before-publish) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qhw1SVHGJZw2Pj9ifdJgN * fix: sentinel-free retire mode so a user's Stop always sticks (CodeRabbit round 3) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qhw1SVHGJZw2Pj9ifdJgN * 📝 Add docstrings to `issue-2131-sidecar-respawn` Docstrings generation was requested by @kingpanther13. The following files were modified: * `src/ha_mcp/__main__.py` * `src/ha_mcp/stdio_settings_sidecar.py` * `tests/src/unit/test_sidecar_pin_port.py` * `tests/src/unit/test_stdio_settings_sidecar.py` These files were kept as they were: * `src/ha_mcp/settings_ui/__init__.py` These file types are not supported: * `src/ha_mcp/settings_ui/AGENTS.md` * `src/ha_mcp/settings_ui/locales/en.json` * `src/ha_mcp/settings_ui/settings.html` * revert: drop the auto-generated docstring commit (lint-broken, replaced reviewed docstrings) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qhw1SVHGJZw2Pj9ifdJgN * fix: address Patch76 review (dump-before-retire ordering, ADVANCED_FIELD_META copy, publish-wait warning tests, retire response text) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qhw1SVHGJZw2Pj9ifdJgN * fix: atomic tool-metadata dump + stale maybe_spawn docstring (Patch76 round 2) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qhw1SVHGJZw2Pj9ifdJgN * fix: address Patch76 round 3 (true atomicity assertion, pre-POST pid capture, mkstemp docstring) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011qhw1SVHGJZw2Pj9ifdJgN --------- Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 parent 6e6b7d0 commit dceab17

12 files changed

Lines changed: 2142 additions & 359 deletions

File tree

src/ha_mcp/__main__.py

Lines changed: 36 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -901,27 +901,27 @@ def _maybe_spawn_settings_sidecar() -> None:
901901
is async; this happens before the main stdio loop so there's no
902902
nested-loop conflict with ``_run_entrypoint``'s own ``asyncio.run``.
903903
904-
Performance: the dump constructs the full FastMCP server, which is
905-
heavy. Skip it (and the server build) when there's nothing to spawn
906-
for — sidecar disabled or already alive. Warm restarts that already
907-
have a sidecar pay zero cold-start tax from this path.
904+
Performance: the dump constructs the full FastMCP server via the
905+
cached ``_get_server()`` singleton the stdio session builds anyway,
906+
so this only front-loads that cost. The dump runs as maybe_spawn's
907+
``prepare`` hook — winner-only, inside the spawn lock — because the
908+
replacement sidecar must read a cache dumped by the SAME parent that
909+
spawned it: a spawn-lock loser with a different environment could
910+
otherwise overwrite the winner's cache (issue #2131 review), and the
911+
sidecar reloads that shared file per request.
908912
"""
909913
from ha_mcp.settings_ui import (
910914
_get_tool_metadata,
911915
dump_tool_metadata_cache,
912916
)
913917
from ha_mcp.stdio_settings_sidecar import (
914-
_existing_sidecar_alive,
915918
_is_disabled,
916919
maybe_spawn,
917920
)
918921

919-
# Cheap gates first; skip the heavy metadata dump when the sidecar
920-
# would be a no-op anyway. Any condition that makes maybe_spawn()
921-
# short-circuit also makes the dump pointless (the running sidecar
922-
# already has a cache from a prior parent startup; a disabled
923-
# sidecar never reads one).
924-
if _is_disabled() or _existing_sidecar_alive():
922+
# Disabled is the only dump-skipping gate; maybe_spawn() logs the
923+
# skip reason (a disabled sidecar never reads the cache).
924+
if _is_disabled():
925925
try:
926926
maybe_spawn()
927927
except Exception as e:
@@ -932,32 +932,35 @@ def _maybe_spawn_settings_sidecar() -> None:
932932
)
933933
return
934934

935-
try:
936-
metadata = asyncio.run(_get_tool_metadata(_get_server()))
937-
dumped = dump_tool_metadata_cache(metadata)
938-
# Log a deliberate one-liner so users debugging an empty
939-
# settings page can see whether the parent's dump succeeded
940-
# by grepping the stdio process output (which Claude Desktop
941-
# surfaces in its MCP server log panel).
942-
logger.info(
943-
"Tool metadata cache: %d tools dumped, write %s",
944-
len(metadata),
945-
"succeeded" if dumped else "FAILED",
946-
)
947-
except Exception as e:
948-
# Cache dump is best-effort — the sidecar falls back to an empty
949-
# tools list rather than blocking stdio startup. Include the
950-
# exception class in the warning so ops can distinguish
935+
def _dump_metadata_cache() -> None:
936+
# One-off asyncio.run is safe here: this runs before the main
937+
# stdio loop, so there's no nested-loop conflict with
938+
# _run_entrypoint's own asyncio.run. Best-effort — the sidecar
939+
# falls back to an empty tools list rather than blocking stdio
940+
# startup. The exception class in the warning distinguishes
951941
# server-init failures (Pydantic ValidationError) from cache I/O
952942
# (OSError) from event-loop issues (RuntimeError).
953-
logger.warning(
954-
"Failed to dump tool metadata cache (%s)",
955-
type(e).__name__,
956-
exc_info=True,
957-
)
943+
try:
944+
metadata = asyncio.run(_get_tool_metadata(_get_server()))
945+
dumped = dump_tool_metadata_cache(metadata)
946+
# Deliberate one-liner so users debugging an empty settings
947+
# page can see whether the parent's dump succeeded by
948+
# grepping the stdio process output (which Claude Desktop
949+
# surfaces in its MCP server log panel).
950+
logger.info(
951+
"Tool metadata cache: %d tools dumped, write %s",
952+
len(metadata),
953+
"succeeded" if dumped else "FAILED",
954+
)
955+
except Exception as e:
956+
logger.warning(
957+
"Failed to dump tool metadata cache (%s)",
958+
type(e).__name__,
959+
exc_info=True,
960+
)
958961

959962
try:
960-
maybe_spawn()
963+
maybe_spawn(prepare=_dump_metadata_cache)
961964
except Exception as e:
962965
# Spawn failures already log inside maybe_spawn(); the bare
963966
# except here is a defense-in-depth guard for any unexpected

src/ha_mcp/config.py

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -102,11 +102,12 @@ class Settings(BaseSettings):
102102
# WebSocket configuration (essential for async operations)
103103
enable_websocket: bool = Field(True, alias="ENABLE_WEBSOCKET")
104104

105-
# Settings UI sidecar (stdio mode only, #1587). 0 = pick a free
106-
# ephemeral port at every spawn (default); 1024-65535 pins the sidecar
107-
# to a fixed port so the settings URL/origin stays stable across
108-
# restarts (bookmarks, browser localStorage). Read by run_main() in
109-
# stdio_settings_sidecar.py.
105+
# Settings UI sidecar (stdio mode only, #1587). 0 (default) = pick a
106+
# free ephemeral port on the first spawn and reuse it afterwards
107+
# (persisted in ui.state, #2131) so the settings URL/origin stays
108+
# stable across restarts; 1024-65535 pins a preferred fixed port
109+
# instead (best-effort: falls back to an ephemeral one if taken).
110+
# Read by run_main() in stdio_settings_sidecar.py.
110111
sidecar_pin_port: int = Field(0, alias="HA_MCP_SIDECAR_PORT")
111112

112113
# Development/Debug configuration
@@ -859,9 +860,9 @@ class AdvancedField(NamedTuple):
859860
AdvancedField("environment", "ENVIRONMENT", str, "diagnostics", True),
860861
AdvancedField("log_level", "LOG_LEVEL", str, "diagnostics", True),
861862
AdvancedField("debug", "DEBUG", bool, "diagnostics", True),
862-
# Settings UI sidecar (stdio-only). Pin the sidecar's port so the
863-
# settings URL/origin is stable across restarts; 0 = ephemeral
864-
# (default). #1587.
863+
# Settings UI sidecar (stdio-only). 0 (default) = first spawn picks a
864+
# free port and later spawns reuse it via ui.state (#2131); a value
865+
# pins a preferred fixed port instead (best-effort, #1587).
865866
AdvancedField("sidecar_pin_port", "HA_MCP_SIDECAR_PORT", int, "sidecar", True),
866867
# NOTE: ``auto_backup_dir`` and ``auto_backup_calendar_lookahead_days``
867868
# are NOT in this tuple. They are in ``BACKUP_OVERRIDE_FIELDS`` (defined

src/ha_mcp/settings_ui/AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ Any reachable HA instance works for `HOMEASSISTANT_URL` / `HOMEASSISTANT_TOKEN`.
4646
- **Relative imports are double-dot.** This is a subpackage of `ha_mcp`, so siblings are `from ..config import ...`, `from ..tools.X import ...`, etc. A single dot resolves inside `settings_ui/` and will `ModuleNotFoundError`.
4747
- **Python ↔ JS sentinel sync.** `__init__.py` substitutes `__HA_MCP_DEFAULT_PINNED__` and `__HA_MCP_MANDATORY__` into `settings.js` at load. The presence of both sentinels is asserted at import — a rename in one place that isn't mirrored in the other fails fast (not silently).
4848
- **Anti-FOUC parity with the docs site (JS logic only, NOT visual design).** The theme/accessibility resolver core in `settings.js` (PREFS / PRESETS / apply functions / custom-color layering) must stay logically identical to `site/src/layouts/Layout.astro`. Enforced by `tests/src/unit/test_anti_fouc_parity.py`. Mirror any change in both, or that test fails. This is about *how a saved theme preference is applied before paint*, not about what the page looks like — the visual target is HA (see "Design goal").
49-
- **Theme prefs persist server-side** (`theme_prefs.json`) because the stdio sidecar respawns on a random port (a fresh origin with empty `localStorage`). The page seeds only *missing* `localStorage` keys from the server payload; the browser's own latest choice wins.
49+
- **Theme prefs persist server-side** (`theme_prefs.json`) because the stdio sidecar's port — the browser origin — can change (stable by default since #2131, but fresh on first spawn, a lost `ui.state`, a pin change, or a taken remembered port), and a new origin starts with empty `localStorage`. The page seeds only *missing* `localStorage` keys from the server payload; the browser's own latest choice wins.
5050
- **Locale selection order is explicit cookie → Home Assistant `ha_lang` hint → `Accept-Language` → English.** Keep the selector cookie name synchronized with `_i18n.py`, and preserve English fallback for incomplete community translations.
5151
- **Callouts use the `ha-alert` style.** Notice bars (`.ha-alert`, `.readonly-notice`, `.pin-notice`, `.restart-notice`) replicate HA's `ha-alert`: full type-tint background, rounded, leading mdi icon (`--icon-info` / `--icon-warning` masks), icon in an absolute left gutter so text flows full-width. Use this for new callouts, not a left-border bar.
5252

src/ha_mcp/settings_ui/__init__.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -626,7 +626,9 @@ def register_settings_routes(
626626
("/api/settings/features", ["GET"], "get_feature_flags"),
627627
("/api/settings/features", ["POST"], "save_feature_flags"),
628628
# Theme / accessibility prefs (#1574 review) — server-side copy so
629-
# they survive the stdio sidecar's per-spawn origin change
629+
# they survive a stdio sidecar origin change (stable by default
630+
# since #2131, but fresh on first spawn / lost ui.state / pin
631+
# change / taken remembered port)
630632
("/api/settings/theme", ["GET"], "get_theme_prefs"),
631633
("/api/settings/theme", ["POST"], "save_theme_prefs"),
632634
# Advanced settings endpoints

src/ha_mcp/settings_ui/_persistence.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,11 @@ def dump_tool_metadata_cache(metadata: list[dict[str, Any]]) -> bool:
6767
"""
6868
path = _get_tool_metadata_cache_path()
6969
try:
70-
path.write_text(json.dumps(metadata))
70+
# Atomic (tmp-then-rename): since the dump moved ahead of the
71+
# sidecar retire it overlaps the OLD still-serving sidecar on
72+
# every startup, and a truncate-write would hand a concurrent
73+
# /api/tools request an empty tool list from the torn file.
74+
_atomic_write_json(path, metadata)
7175
except OSError:
7276
logger.warning("Failed to dump tool metadata cache to %s", path, exc_info=True)
7377
return False
@@ -203,10 +207,13 @@ def effective_tool_config(settings: Settings | None = None) -> dict[str, Any]:
203207
return {**cfg, "tools": tools}
204208

205209

206-
def _atomic_write_json(path: Path, payload: dict[str, Any]) -> None:
210+
def _atomic_write_json(
211+
path: Path, payload: dict[str, Any] | list[dict[str, Any]]
212+
) -> None:
207213
"""Write ``payload`` to ``path`` atomically.
208214
209-
Writes to ``<path>.tmp`` first and ``os.replace``s into place so a
215+
Writes to a unique same-directory temp file (``mkstemp``, since
216+
#1993) first and ``os.replace``s into place so a
210217
crash or out-of-space mid-write cannot leave a partial/empty file —
211218
callers that read the file back (``load_tool_config`` /
212219
``_load_backup_settings_override``) would otherwise treat a

src/ha_mcp/settings_ui/_theme.py

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,15 @@
11
"""Theme / accessibility preference persistence for the settings UI.
22
33
The browser keeps these in localStorage for synchronous pre-paint reads,
4-
but localStorage is origin-scoped and the stdio settings sidecar binds a
5-
random free port per spawn — every session is a fresh origin that starts
6-
empty. The server-side copy here survives that: POSTs land in
7-
``theme_prefs.json`` next to the other settings files, and the page
8-
handler seeds them back into the served HTML (``server-prefs`` head
9-
script) so a fresh origin paints with the user's saved choices.
4+
but localStorage is origin-scoped and the stdio settings sidecar's port
5+
can change — historically on every spawn, now (#2131) only on the first
6+
spawn, a lost ``ui.state``, a pin change, or a remembered port another
7+
process took — and each port change is a fresh origin that starts
8+
empty. The
9+
server-side copy here survives that: POSTs land in ``theme_prefs.json``
10+
next to the other settings files, and the page handler seeds them back
11+
into the served HTML (``server-prefs`` head script) so a fresh origin
12+
paints with the user's saved choices.
1013
1114
Kept as a leaf module (no imports from the settings_ui package) so the
1215
handler families and ``__init__`` can depend on it without cycles.

src/ha_mcp/settings_ui/locales/en.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@
6363
"policies.pending.empty": "No pending approvals.",
6464
"policies.rules.title": "Gated tools",
6565
"policies.rules.empty": "No tools currently security-gated. Enable per-tool gating from the <a href=\"#\" data-panel-link=\"tools\">Tools</a> tab.",
66-
"accessibility.intro": "These settings apply immediately and are saved in this browser and on the server, so they survive restarts in every mode (including stdio, where the settings page moves to a fresh port each session). The docs site offers the same controls in its navigation bar; it has no server and saves per browser.",
66+
"accessibility.intro": "These settings apply immediately and are saved in this browser and on the server, so they survive restarts in every mode (including stdio, where the settings page can occasionally move to a new port). The docs site offers the same controls in its navigation bar; it has no server and saves per browser.",
6767
"accessibility.storage_blocked": "Your browser is blocking site storage; choices apply for this session only.",
6868
"accessibility.theme.title": "Theme",
6969
"accessibility.theme.help": "One-click color schemes. Auto, the default, follows your OS preference and flips live when it changes.",
@@ -420,7 +420,7 @@
420420
"advanced.extra_yaml_write_keys.label": "Extra YAML write keys",
421421
"advanced.extra_yaml_write_keys.help": "Comma-separated top-level keys ha_config_set_yaml may write in addition to the built-in ones, for YAML-first integrations on this install (e.g. alert2). Keys that redefine Home Assistant's own trust boundary can never be added and are ignored. Requires custom component 1.2.4 or newer.",
422422
"advanced.sidecar_pin_port.label": "Settings UI sidecar port",
423-
"advanced.sidecar_pin_port.help": "0 chooses a free port each restart; 1024–65535 pins a preferred port. Restart required.",
423+
"advanced.sidecar_pin_port.help": "0 picks a free port on first start and keeps it for later restarts; 1024–65535 pins a preferred port (falls back to a free one if taken). Restart required.",
424424
"advanced.enable_dev_mode.label": "Developer mode",
425425
"advanced.enable_dev_mode.help": "⚠ Registers hidden tools that can change settings and replace the running server. Development and testing only. Restart required.",
426426
"addon.backup_hint.name": "Full-HA snapshot suggestion level (LLM-facing)",

src/ha_mcp/settings_ui/settings.html

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,14 @@
1515
system sans-serif. -->
1616
<script data-purpose="server-prefs" data-prefs="__HA_MCP_THEME_PREFS__">
1717
// #1574 review: theme/accessibility prefs also persist server-side
18-
// (theme_prefs.json) because stdio respawns its settings sidecar on a
19-
// random port — a fresh origin whose localStorage starts empty. The page
20-
// handler substitutes the data-prefs attribute at request time; this
21-
// seeds only MISSING localStorage keys (the browser's own latest choice
22-
// wins) before the anti-FOUC resolver below reads them. The docs site is
23-
// a static build and stays localStorage-only.
18+
// (theme_prefs.json) because the stdio sidecar's port — stable by
19+
// default since #2131, but fresh on first spawn, a lost ui.state, a
20+
// pin change, or a taken remembered port — is the browser origin, and
21+
// a new origin's localStorage starts empty. The page handler
22+
// substitutes the data-prefs attribute at request time; this seeds
23+
// only MISSING localStorage keys (the browser's own latest choice
24+
// wins) before the anti-FOUC resolver below reads them. The docs site
25+
// is a static build and stays localStorage-only.
2426
(function () {
2527
var el = document.currentScript;
2628
var raw = el ? el.getAttribute("data-prefs") : "";
@@ -366,10 +368,10 @@ <h3 style="font-size:1rem;margin-bottom:8px" data-i18n="policies.rules.title">Ga
366368
<div class="panel" id="panel-accessibility" role="tabpanel" aria-labelledby="tab-accessibility" tabindex="0">
367369
<p class="tool-desc" style="margin-bottom:16px" data-i18n="accessibility.intro">
368370
These settings apply immediately and are saved in this browser and on the
369-
server, so they survive restarts in every mode (including stdio, where the
370-
settings page moves to a fresh port each session). The docs site offers
371-
the same controls in its navigation bar; it has no server and saves per
372-
browser.
371+
server, so they survive restarts in every mode (including stdio, where
372+
the settings page can occasionally move to a new port). The docs site
373+
offers the same controls in its navigation bar; it has no server and
374+
saves per browser.
373375
</p>
374376
<p class="a11y-contrast-warning" id="a11y-storage-note" hidden data-i18n="accessibility.storage_blocked">
375377
Your browser is blocking site storage; choices apply for this session

src/ha_mcp/settings_ui/settings.js

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3534,7 +3534,7 @@ const ADVANCED_FIELD_META = {
35343534
code_mode_max_invocations: { label: "Code-mode max invocations", help: "API/tool-call cap per sandbox run. Restart required." },
35353535
code_mode_saved_tools_path:{ label: "Saved-tools path", help: "JSON file where ha_manage_custom_tool persists saved tools across restarts. Restart required." },
35363536
extra_yaml_write_keys: { label: "Extra YAML write keys", help: "Comma-separated top-level keys ha_config_set_yaml may write in addition to the built-in ones, for YAML-first integrations on this install (e.g. alert2). Keys that redefine Home Assistant's own trust boundary can never be added and are ignored. Requires custom component 1.2.4 or newer." },
3537-
sidecar_pin_port: { label: "Settings UI sidecar port", help: "0 = a new free port each restart (default); set 1024–65535 to pin a fixed port so the settings URL stays stable across restarts. Falls back to a free port if the pinned one is busy. Restart required." },
3537+
sidecar_pin_port: { label: "Settings UI sidecar port", help: "0 picks a free port on first start and keeps it for later restarts; 1024–65535 pins a preferred port (falls back to a free one if taken). Restart required." },
35383538
enable_dev_mode: { label: "Developer mode", help: "⚠ DANGER: registers hidden developer tools (ha_dev_manage_server, ha_dev_manage_settings) that let AI agents change server settings and replace the running server version (e.g. install a PR build). For development and testing only. Restart required." },
35393539
};
35403540

@@ -4020,8 +4020,10 @@ loadFsCustomPaths();
40204020

40214021
// #1574 review: localStorage is the synchronous store the anti-FOUC
40224022
// script reads at paint time, but it is origin-scoped and the stdio
4023-
// sidecar binds a fresh random port (= fresh empty origin) per session.
4024-
// This hook therefore (a) mirrors every change to the server
4023+
// sidecar's port (= the origin) can still change: stable by default
4024+
// since #2131, yet fresh on first spawn, a lost ui.state, a pin
4025+
// change, or a taken remembered port. This hook therefore (a) mirrors
4026+
// every change to the server
40254027
// (./api/settings/theme -> theme_prefs.json), which seeds the next
40264028
// fresh origin via the server-prefs head script, and (b) surfaces a
40274029
// blocked localStorage (private mode) once instead of silently losing

0 commit comments

Comments
 (0)