fix(settings-ui): unify restart flow — save commits, button restarts

Every save endpoint (Tools, Server Settings, Backups) now goes through
the same restart UX: persist the change, surface a "Saved — restart
required" toast and the cross-tab restart banner, and let the user
pick the moment to actually fire the supervisor restart via the
global Restart Add-on button. No save handler auto-restarts the addon
anymore. The button auto-reloads the page 10 seconds after the click.

Why
- Adjusting a Server Setting toggle still produced a "Restart failed"
  alert in addon mode. Root cause: the previous fix moved each save's
  supervisor self-restart into a background task, but the user-visible
  outcome was unchanged — every toggle change immediately fired a
  supervisor restart, and if the user toggled multiple settings in
  quick succession, a later POST would race the in-flight restart
  (supervisor kills the addon → ingress returns 5xx → JS shows
  "Restart failed").
- Saving auto-backup settings on the Backups tab jumped straight to
  "addon is restarting" with no preview, no second-chance confirm,
  and no way to bundle multiple edits before paying the restart cost.
- Clicking the explicit Restart button on the Tools tab left the page
  stuck on "Restart initiated — reload page in ~30s" forever; the
  user had to manually F5.

Server changes
- ``_save_feature_flags`` addon branch: drop
  ``_schedule_supervisor_self_restart`` call. Return
  ``{success, applied, mode, restart_required: True}`` instead of
  ``{..., restarting: True}``.
- ``_save_backup_config`` addon branch: same — no auto-restart.
- ``_save_backup_config`` file branch: rename ``restarting: False``
  → ``restart_required: False`` for naming consistency across all
  save endpoints.
- ``_save_feature_flags`` file/default branch: now also returns
  ``restart_required: True`` (matches the FEATURE_META copy on every
  flag, which says "Requires restart to take effect"). The UI
  surfaces the banner; the restart button stays hidden in non-addon
  mode since there is no supervisor to drive it.

JS changes
- ``saveFeatureFlag``: drops the "add-on is restarting…" branch.
  Every successful save shows ``Saved — restart required`` and
  surfaces the cross-tab banner.
- ``saveBackupConfig``: branches on ``data.restart_required`` instead
  of ``data.restarting``. Re-enables the Save button so the user can
  bundle further edits before clicking Restart.
- ``restartAddon``: auto-reloads the page after
  ``RESTART_RELOAD_DELAY_S = 10`` seconds. 4xx responses (genuine
  config error, e.g. SUPERVISOR_TOKEN missing) suppress the reload
  and surface the error inline; 5xx-from-ingress and network drops
  fall through to the reload because both mean the supervisor killed
  our upstream and the restart is in flight.
- ``ORIGIN_INFO_NOTE.addon`` and ``BACKUP_ORIGIN_LABELS.addon``:
  "save will restart the add-on" → "restart required after save".

Test changes
- ``test_addon_save_merges_and_schedules_restart`` renamed to
  ``test_addon_save_merges_without_restart_returns_restart_required``
  for both the backup-config and feature-flags variants. New
  assertions: ``schedule_mock.assert_not_called()``,
  ``body["restart_required"] is True``,
  ``"restarting" not in body``.
- ``test_standalone_writes_file_and_invalidates_cache``: assert the
  new field name.

Verified: ruff check, ruff format, mypy src/, 85 passing in
test_settings_ui.py (1 pre-existing Windows skip).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: kingpanther13

src/ha_mcp/settings_ui.py

@@ -921,28 +921,41 @@ def apply_tool_visibility(
   }
 }
 
+const RESTART_RELOAD_DELAY_S = 10;
+
 async function restartAddon() {
   const btn = document.getElementById('restartBtn');
-  if (!confirm('Restart the add-on now? The web UI will become unreachable for ~30 seconds.')) return;
+  if (!confirm('Restart the add-on now? The page will reload automatically once the add-on is back online.')) return;
   btn.disabled = true;
-  btn.textContent = 'Restarting...';
+  btn.textContent = 'Restarting…';
+  // Only suppress the auto-reload on a genuine config error (4xx like
+  // SUPERVISOR_TOKEN missing). Anything else — 200, 5xx from ingress
+  // when supervisor killed our upstream mid-response, or a thrown
+  // network error from the same kill — means the restart is in flight
+  // and we should reload after the addon comes back.
+  let configError = false;
   try {
     const resp = await fetch('./api/settings/restart', {method: 'POST'});
-    if (resp.ok) {
-      btn.textContent = 'Restart initiated — reload page in ~30s';
-    } else {
+    if (!resp.ok && resp.status < 500) {
+      configError = true;
       let msg = 'Restart failed';
       try {
         const err = await resp.json();
-        if (err.error && err.error.message) msg = 'Failed: ' + err.error.message;
+        if (err && err.error && err.error.message) {
+          msg = 'Failed: ' + err.error.message;
+        }
       } catch (_e) {}
       btn.textContent = msg;
       btn.disabled = false;
       alert(msg);
     }
   } catch (_e) {
-    // Connection lost mid-request is actually expected — the addon is restarting
-    btn.textContent = 'Restart initiated (connection dropped)';
+    // Connection lost mid-request — restart in flight, fall through.
+  }
+  if (!configError) {
+    btn.textContent =
+      'Restarting… page will reload in ' + RESTART_RELOAD_DELAY_S + 's';
+    setTimeout(() => window.location.reload(), RESTART_RELOAD_DELAY_S * 1000);
   }
 }
 
@@ -1204,7 +1217,7 @@ def apply_tool_visibility(
 };
 
 const BACKUP_ORIGIN_LABELS = {
-  addon: 'Synced to Supervisor — save will restart the add-on.',
+  addon: 'Synced to Supervisor — restart required after save.',
   env: null,  // banner generated dynamically with the env var name
   file: 'Persisted locally; takes effect immediately.',
   default: 'Using default; first save creates a local override file.',
@@ -1300,14 +1313,18 @@ def apply_tool_visibility(
       statusEl.textContent = msg;
       return;
     }
-    if (data.restarting) {
-      statusEl.textContent = 'Saved — addon is restarting. Reload in ~30s.';
-      // Surface the cross-tab restart banner so the user has a clear
-      // reload-when-ready signal regardless of which tab they're on.
+    btn.disabled = false;
+    if (data.restart_required) {
+      // Unified restart flow — save persists but does NOT auto-restart.
+      // Surface the cross-tab restart-required banner; user picks the
+      // moment via the global Restart Add-on button. Refresh the form
+      // so origins update (default → addon/file etc.) but skip the
+      // backup-list reload until after the actual restart.
+      statusEl.textContent = 'Saved — restart required';
       document.getElementById('restartNotice').classList.add('show');
+      loadBackupConfig();
     } else {
       statusEl.textContent = 'Saved.';
-      btn.disabled = false;
       // Refresh display so origins update (default → file, etc.).
       loadBackupConfig();
       loadBackups();
@@ -1497,7 +1514,7 @@ def apply_tool_visibility(
 };
 
 const ORIGIN_INFO_NOTE = {
-  addon: 'Synced to the add-on Configuration tab — save will restart the add-on.',
+  addon: 'Synced to the add-on Configuration tab — restart required after save.',
 };
 
 async function loadFeatureFlags() {
@@ -1624,19 +1641,15 @@ def apply_tool_visibility(
     updateStatus(msg);
     return;
   }
-  if (data && data.restarting) {
-    // Addon mode: the server has already POSTed the merged options to
-    // Supervisor and scheduled a self-restart in a background task —
-    // the user just needs to wait. Surface the restart-required banner
-    // so it is visible regardless of which tab they are on (the banner
-    // lives above the tabs).
-    updateStatus('Saved — add-on is restarting. Reload in ~30s.', true);
+  // Unified restart flow — save persists the change but does NOT fire
+  // the addon restart. The user picks when to restart by clicking the
+  // global Restart Add-on button in the cross-tab restart-required
+  // banner. Same UX as the Tools tab. In standalone modes the restart
+  // button is hidden (no supervisor to drive it) but the banner still
+  // surfaces "restart required" as guidance.
+  updateStatus('Saved — restart required', true);
+  if (data && data.restart_required) {
     document.getElementById('restartNotice').classList.add('show');
-  } else {
-    // Standalone modes: the cache is reset in-process. Most flags still
-    // require an MCP-host restart to take effect (the docstring on each
-    // flag says so) but the addon itself is not the host — just toast.
-    updateStatus('Saved — restart required', true);
   }
 }
 
@@ -2390,13 +2403,19 @@ async def _save_feature_flags(request: Request) -> JSONResponse:
                     create_error_response(code, err.message),
                     status_code=err.status_code,
                 )
-            _schedule_supervisor_self_restart(server.settings.verify_ssl)
+            # Unified restart flow: every save that requires an addon
+            # restart (Tools, Server Settings, Backups) returns
+            # ``restart_required=True`` and lets the user pick when to
+            # fire the actual restart via the global Restart Add-on
+            # button. Don't auto-restart here — racing the response
+            # against the supervisor kill caused the "Restart failed"
+            # alert reported by the user.
             return JSONResponse(
                 {
                     "success": True,
                     "applied": new_overrides,
                     "mode": "addon",
-                    "restarting": True,
+                    "restart_required": True,
                 }
             )
 
@@ -2486,8 +2505,13 @@ async def _save_feature_flags(request: Request) -> JSONResponse:
         # required for many flags — the UI surfaces that — but the
         # cached singleton must not return the stale pre-write
         # values to subsequent /api/settings/features GETs).
+        # ``restart_required=True`` because every flag in
+        # FEATURE_META carries "Requires restart to take effect" in its
+        # help text — file-mode saves persist the value but the live
+        # process keeps the old reads until the MCP host is restarted
+        # (Claude Desktop relaunch, Docker container restart, etc.).
         _reset_global_settings()
-        return JSONResponse({"success": True})
+        return JSONResponse({"success": True, "mode": "file", "restart_required": True})
 
     # ---- Auto-backup routes (#1288) ----
 
@@ -2817,19 +2841,15 @@ async def _save_backup_config(request: Request) -> JSONResponse:
                     create_error_response(code, sup_err.message),
                     status_code=sup_err.status_code,
                 )
-            # Background restart so this JSON response can flush through HA
-            # ingress before supervisor kills the addon. The old code POSTed
-            # synchronously and the kill-mid-response would convert into a
-            # 5xx Bad Gateway at the proxy, which the JS rendered as
-            # "Save failed" / "Restart failed" — see _restart_addon for the
-            # same fix.
-            _schedule_supervisor_self_restart(server.settings.verify_ssl)
+            # Unified restart flow — see _save_feature_flags for the
+            # rationale. Don't auto-restart from a save handler; the
+            # global Restart Add-on button is the single restart path.
             return JSONResponse(
                 {
                     "success": True,
                     "applied": clean,
                     "mode": "addon",
-                    "restarting": True,
+                    "restart_required": True,
                 }
             )
 
@@ -2871,13 +2891,18 @@ async def _save_backup_config(request: Request) -> JSONResponse:
                 status_code=500,
             )
         # Drop the cached Settings so the next read sees the merged value.
+        # File-mode auto-backup settings take effect immediately on the
+        # next ``get_global_settings()`` read — no restart needed, hence
+        # ``restart_required=False``. The JS uses the same field name on
+        # every save endpoint to decide whether to surface the
+        # restart-required banner.
         _reset_global_settings()
         return JSONResponse(
             {
                 "success": True,
                 "applied": clean,
                 "mode": "file",
-                "restarting": False,
+                "restart_required": False,
             }
         )
 

tests/src/unit/test_settings_ui.py

@@ -1094,7 +1094,13 @@ async def test_standalone_writes_file_and_invalidates_cache(
         assert resp.status_code == 200
         body = json.loads(resp.body)
         assert body["mode"] == "file"
-        assert body["restarting"] is False
+        # File-mode auto-backup save applies live via the cache reset;
+        # no addon restart needed, hence restart_required=False. The
+        # field name was renamed from "restarting" → "restart_required"
+        # as part of the unified restart flow (Tools / Server Settings
+        # / Backups all use the same field).
+        assert body["restart_required"] is False
+        assert "restarting" not in body  # legacy field name must be gone
         on_disk = json.loads(override_path.read_text())
         assert on_disk["enable_auto_backup"] is True
         assert on_disk["auto_backup_throttle_minutes"] == 9
@@ -1526,7 +1532,20 @@ def decorator(fn):
         return captured["post"]
 
     @pytest.mark.asyncio
-    async def test_addon_save_merges_and_schedules_restart(self, monkeypatch):
+    async def test_addon_save_merges_without_restart_returns_restart_required(
+        self, monkeypatch
+    ):
+        """Unified restart flow: every save endpoint (Tools, Server
+        Settings, Backups) commits the change but **does not** fire the
+        addon restart. The user picks when to restart via the global
+        Restart Add-on button. The save response carries
+        ``restart_required=True`` so the cross-tab banner appears.
+
+        Pins the contract — a regression that re-introduces an
+        auto-restart from inside the save handler races the supervisor
+        kill against the JSON response flush and surfaces a spurious
+        "Restart failed" alert / "addon is restarting" message.
+        """
         post_handler = self._capture_post_handler(monkeypatch)
 
         merge_mock = AsyncMock(return_value=(True, None))
@@ -1547,12 +1566,13 @@ async def test_addon_save_merges_and_schedules_restart(self, monkeypatch):
         assert resp.status_code == 200
         body = json.loads(resp.body)
         assert body["mode"] == "addon"
-        assert body["restarting"] is True
+        assert body["restart_required"] is True
+        assert "restarting" not in body  # legacy field name must be gone
         merge_mock.assert_awaited_once_with(
             True,
             {"enable_auto_backup": True, "auto_backup_throttle_minutes": 5},
         )
-        schedule_mock.assert_called_once_with(True)
+        schedule_mock.assert_not_called()
 
     @pytest.mark.asyncio
     async def test_addon_save_surfaces_validation_error_with_supervisor_status(
@@ -1670,7 +1690,12 @@ def decorator(fn):
         return captured["post"]
 
     @pytest.mark.asyncio
-    async def test_addon_save_merges_and_schedules_restart(self, monkeypatch):
+    async def test_addon_save_merges_without_restart_returns_restart_required(
+        self, monkeypatch
+    ):
+        """Mirror of TestSaveBackupConfigAddonMode's counterpart — see
+        that test for the unified-restart-flow rationale.
+        """
         post_handler = self._capture_post_handler(monkeypatch)
 
         merge_mock = AsyncMock(return_value=(True, None))
@@ -1689,9 +1714,10 @@ async def test_addon_save_merges_and_schedules_restart(self, monkeypatch):
         assert resp.status_code == 200
         body = json.loads(resp.body)
         assert body["mode"] == "addon"
-        assert body["restarting"] is True
+        assert body["restart_required"] is True
+        assert "restarting" not in body
         merge_mock.assert_awaited_once_with(True, {"enable_yaml_config_editing": True})
-        schedule_mock.assert_called_once_with(True)
+        schedule_mock.assert_not_called()
 
     @pytest.mark.asyncio
     async def test_addon_save_surfaces_validation_error_with_supervisor_status(