fix(auto-update): address review feedback

- Health check: a failed restart command no longer lets the check run
  against the still-running old process; it counts as a failure (and after a
  rollback, as a failed rollback). An unreadable restart count is never
  treated as stable, since a crash loop looks healthy between attempts.
- Installer writes the auto_update setting to a temp file and swaps it in,
  keeping mode and owner, so a running config watcher never reads a
  truncated config.json.
- Verify unit quotes its command-line paths (install folders with spaces);
  setup refuses folder names systemd would reinterpret (%, quotes,
  backslashes, control characters) and says so on the General tab.
- The auto-update status route no longer returns exception text.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

first_time_install.sh

@@ -815,16 +815,31 @@ if [ -z "$AUTO_UPDATE" ] && [ "$ASSUME_YES" != "1" ] && [ -t 0 ]; then
 fi
 if [ "$AUTO_UPDATE" = "1" ] || [ "$AUTO_UPDATE" = "0" ]; then
     if python3 - "$PROJECT_ROOT_DIR/config/config.json" "$AUTO_UPDATE" <<'PY'
-import json, sys
+import json, os, sys, tempfile
 path, enabled = sys.argv[1], sys.argv[2] == "1"
 with open(path, encoding="utf-8") as f:
     config = json.load(f)
 if not isinstance(config.get("auto_update"), dict):
     config["auto_update"] = {}
 config["auto_update"]["enabled"] = enabled
-with open(path, "w", encoding="utf-8") as f:
+# Written beside the original and swapped in whole: the display service's
-    json.dump(config, f, indent=4)
+# config watcher may be running and must never read a half-written file.
-    f.write("\n")
+original = os.stat(path)
+fd, tmp = tempfile.mkstemp(dir=os.path.dirname(os.path.abspath(path)), prefix=".config.")
+try:
+    with os.fdopen(fd, "w", encoding="utf-8") as f:
+        json.dump(config, f, indent=4)
+        f.write("\n")
+        f.flush()
+        os.fsync(f.fileno())
+    os.chmod(tmp, original.st_mode & 0o777)
+    if hasattr(os, "chown"):
+        os.chown(tmp, original.st_uid, original.st_gid)
+    os.replace(tmp, path)
+except BaseException:
+    if os.path.exists(tmp):
+        os.unlink(tmp)
+    raise
 PY
     then
         if [ "$AUTO_UPDATE" = "1" ]; then echo "✓ Weekly automatic updates enabled"; else echo "✓ Weekly automatic updates off"; fi

scripts/utils/auto_update_verify.py

@@ -128,21 +128,27 @@ class Verifier:
         return result.returncode == 0
 
     def restart_services(self, display):
+        """Restart what should be running. False if any restart command failed."""
+        ok = True
         # A display the user had stopped stays stopped.
         if display:
-            self.restart('ledmatrix')
+            ok = self.restart('ledmatrix') and ok
-        self.restart('ledmatrix-web')
+        return self.restart('ledmatrix-web') and ok
 
     def wait_healthy(self, display):
         """None once the services are up and stay up, else what went wrong."""
         deadline = self.clock() + HEALTH_TIMEOUT_SECONDS + STABLE_SECONDS
         healthy_since = baseline = None
         web = disp = False
+        count_known = True
         while self.clock() < deadline:
             web = self.web_responds()
             disp = self.service_active('ledmatrix') if display else True
             restarts = self.restart_count('ledmatrix') if display else None
-            if web and disp and (healthy_since is None or restarts == baseline):
+            # Without a restart count a crash loop looks healthy between
+            # attempts, so an unreadable count never counts as stable.
+            count_known = not display or restarts is not None
+            if web and disp and count_known and (healthy_since is None or restarts == baseline):
                 if healthy_since is None:
                     healthy_since, baseline = self.clock(), restarts
                 elif self.clock() - healthy_since >= STABLE_SECONDS:
@@ -155,6 +161,8 @@ class Verifier:
             problems.append('the web interface did not respond')
         if not disp:
             problems.append('the display service did not stay running')
+        if web and disp and not count_known:
+            problems.append("the display service's restart count could not be read")
         return '; '.join(problems) or 'the display service kept restarting'
 
     # -- rollback ---------------------------------------------------------
@@ -216,8 +224,11 @@ class Verifier:
         if dependency_failures:
             # Never restart onto code whose packages did not install.
             reason = 'installing its dependencies failed (' + ', '.join(dependency_failures) + ')'
+        elif not self.restart_services(display):
+            # The old process may still be answering; checking it would pass
+            # an update that never started.
+            reason = 'restarting the services failed'
         else:
-            self.restart_services(display)
             reason = self.wait_healthy(display)
             if reason is None:
                 self._finish(pending, 'success')
@@ -229,8 +240,8 @@ class Verifier:
         if not ok:
             self._finish(pending, 'rollback_failed', reason, detail)
             return 1
-        self.restart_services(display)
+        still = (self.wait_healthy(display) if self.restart_services(display)
-        still = self.wait_healthy(display)
+                 else 'restarting the services failed')
         if still:
             self._finish(pending, 'rollback_failed', reason,
                          f'still unhealthy after rolling back: {still}'

src/auto_update_setup.py

@@ -133,6 +133,16 @@ class UpdateHelperSetup:
         if not workdir or Path(workdir).resolve() != self.project_root.resolve():
             raise SetupError(f'The web interface service runs from {workdir or "an unknown folder"}, '
                              f'not {self.project_root}.')
+        # Spaces are fine -- the templates quote every command-line path --
+        # but systemd expands % specifiers, and a quote, backslash or line
+        # break would be reinterpreted in a unit file. (On Windows, where the
+        # tests also run, a backslash is the path separator, not a name.)
+        root_text = str(self.project_root)
+        unsafe = set('%"') | ({'\\'} if os.sep == '/' else set())
+        if any(ch in unsafe or ord(ch) < 32 for ch in root_text):
+            raise SetupError(f'LEDMatrix is installed in {root_text!r}, a folder name systemd cannot use '
+                             'in a unit file. Move it to a path without %, quotes, backslashes or '
+                             'control characters.')
 
         rendered = {}
         for name in UNITS:

systemd/ledmatrix-update-verify.service

@@ -13,6 +13,7 @@
 # interface creates data/auto_update_verify.request. It runs as the web
 # interface's own user, and ExecStart points at the copy the updater makes
 # before pulling, so the update being checked cannot replace its checker.
+# Command-line paths are quoted so an install folder with spaces still works.
 
 [Unit]
 Description=LED Matrix automatic update health check and rollback
@@ -22,8 +23,8 @@ Type=oneshot
 User=__USER__
 WorkingDirectory=__PROJECT_ROOT_DIR__
 # Consume the request first, so the path unit cannot start this again.
-ExecStartPre=/bin/rm -f __PROJECT_ROOT_DIR__/data/auto_update_verify.request
+ExecStartPre=/bin/rm -f "__PROJECT_ROOT_DIR__/data/auto_update_verify.request"
-ExecStart=/usr/bin/python3 __PROJECT_ROOT_DIR__/data/auto_update_verifier.py __PROJECT_ROOT_DIR__
+ExecStart=/usr/bin/python3 "__PROJECT_ROOT_DIR__/data/auto_update_verifier.py" "__PROJECT_ROOT_DIR__"
 TimeoutStartSec=30min
 StandardOutput=syslog
 StandardError=syslog

test/test_auto_update_setup.py

@@ -43,8 +43,8 @@ class FakeSystemctl:
         return subprocess.CompletedProcess(args, 0, stdout='', stderr='')
 
 
-def project(tmp_path, user='hdpi', workdir=None):
+def project(tmp_path, user='hdpi', workdir=None, name='LEDMatrix'):
-    root = tmp_path / 'LEDMatrix'
+    root = tmp_path / name
     (root / 'systemd').mkdir(parents=True)
     for name in aus.UNITS:
         shutil.copy(ROOT / 'systemd' / name, root / 'systemd' / name)
@@ -83,7 +83,7 @@ def test_installs_both_units_for_the_web_user(tmp_path):
     assert out['status'] == 'installed' and result(root)['status'] == 'installed'
     service = (etc / aus.SERVICE_UNIT).read_text()
     assert 'User=hdpi' in service
-    assert f'ExecStart=/usr/bin/python3 {root}/data/auto_update_verifier.py {root}' in service
+    assert f'ExecStart=/usr/bin/python3 "{root}/data/auto_update_verifier.py" "{root}"' in service
     assert f'PathExists={root}/data/auto_update_verify.request' in (etc / aus.PATH_UNIT).read_text()
     assert '__' not in service
     assert ['daemon-reload'] in systemctl.calls
@@ -169,6 +169,38 @@ def test_not_a_systemd_host_is_left_alone(tmp_path):
     assert systemctl.calls == []
 
 
+def test_a_folder_with_spaces_is_quoted_in_the_commands(tmp_path):
+    root, etc = project(tmp_path, name='LED Matrix')
+    assert setup(root, etc, FakeSystemctl()).ensure(ON)['status'] == 'installed'
+    service = (etc / aus.SERVICE_UNIT).read_text()
+    assert f'ExecStartPre=/bin/rm -f "{root}/data/auto_update_verify.request"' in service
+    assert f'ExecStart=/usr/bin/python3 "{root}/data/auto_update_verifier.py" "{root}"' in service
+
+
+def test_a_folder_name_systemd_would_reinterpret_is_refused(tmp_path):
+    root, etc = project(tmp_path, name='LED%Matrix')
+    systemctl = FakeSystemctl()
+    out = setup(root, etc, systemctl).ensure(ON)
+    assert out['status'] == 'failed' and 'systemd cannot use' in out['message']
+    assert not (etc / aus.SERVICE_UNIT).exists() and systemctl.calls == []
+
+
+def test_installer_sets_the_toggle_without_a_half_written_config(tmp_path):
+    """first_time_install.sh may run while the display service watches
+    config.json; the file must be replaced whole, never truncated in place."""
+    import re
+    text = (ROOT / 'first_time_install.sh').read_text(encoding='utf-8').replace('\r\n', '\n')
+    script = next(s for s in re.findall(r"<<'PY'\n(.*?)\nPY\n", text, re.S) if 'auto_update' in s)
+    assert 'os.replace(' in script
+    config = tmp_path / 'config.json'
+    config.write_text(json.dumps({'timezone': 'UTC', 'auto_update': {'enabled': False}}))
+    run = subprocess.run([sys.executable, '-', str(config), '1'], input=script, text=True,
+                         capture_output=True)
+    assert run.returncode == 0, run.stderr
+    assert json.loads(config.read_text()) == {'timezone': 'UTC', 'auto_update': {'enabled': True}}
+    assert [p.name for p in tmp_path.iterdir()] == ['config.json'], "a temp file was left behind"
+
+
 def test_the_result_is_kept_when_it_cannot_be_given_to_the_web_user(tmp_path, monkeypatch):
     """Caught by CI, which runs as a non-root Linux user: chown needs root, and
     a failed chown used to discard the result, so the General tab never

test/test_auto_update_verify.py

@@ -62,8 +62,10 @@ class FakeHost:
     ("display_down", "web_down", "crash_loop") or on any commit ("always").
     """
 
-    def __init__(self, repo, bad_head, failure=None, pip_ok=True):
+    def __init__(self, repo, bad_head, failure=None, pip_ok=True, restart_failures=0, count_readable=True):
         self.repo, self.bad_head, self.failure, self.pip_ok = repo, bad_head, failure, pip_ok
+        self.restart_failures = restart_failures  # how many restart commands fail, first to last
+        self.count_readable = count_readable
         self.running_head = None  # not restarted yet: still the old, healthy code
         self.restarts = []        # (unit, commit it was restarted onto)
         self.pip_installs = []
@@ -81,6 +83,8 @@ class FakeHost:
         if args[:2] == ['systemctl', 'is-active']:
             return done(args, 'failed\n' if self.broken('display_down') else 'active\n')
         if args[:2] == ['systemctl', 'show']:
+            if not self.count_readable:
+                return done(args, rc=1)
             if self.broken('crash_loop'):
                 self.nrestarts += 1
             return done(args, f'{self.nrestarts}\n')
@@ -88,6 +92,9 @@ class FakeHost:
             self.pip_installs.append(args[-1])
             return done(args, rc=0 if self.pip_ok else 1)
         if args[:4] == ['sudo', '-n', 'systemctl', 'restart']:
+            if self.restart_failures:
+                self.restart_failures -= 1
+                return done(args, rc=1)  # the old process keeps running
             self.running_head = git(self.repo, 'rev-parse', 'HEAD')
             self.restarts.append((args[4], self.running_head))
             return done(args)
@@ -104,13 +111,14 @@ class FakeHost:
                            web_responds=self.web_responds, log=lambda msg: None)
 
 
-def check(tmp_path, failure=None, new_requirements=False, pip_ok=True, **pending):
+def check(tmp_path, failure=None, new_requirements=False, pip_ok=True, restart_failures=0,
+          count_readable=True, **pending):
     repo, old, new = updated_repo(tmp_path, new_requirements)
     fields = {'status': 'pending', 'old_head': old, 'new_head': new,
               'display_was_active': True, 'dependency_failures': []}
     fields.update(pending)
     av.write_pending(av.pending_path(repo), fields)
-    host = FakeHost(repo, new, failure, pip_ok)
+    host = FakeHost(repo, new, failure, pip_ok, restart_failures, count_readable)
     code = host.verifier().verify()
     result = av.read_pending(av.pending_path(repo))
     return code, result, host, git(repo, 'rev-parse', 'HEAD'), old, new
@@ -169,6 +177,28 @@ def test_still_broken_after_rolling_back_is_rollback_failed(tmp_path):
     assert 'still unhealthy after rolling back' in result['detail']
 
 
+def test_a_failed_restart_is_not_mistaken_for_a_healthy_update(tmp_path):
+    """When the restart command itself fails the old process keeps answering,
+    so checking it would pass an update that never started."""
+    code, result, host, head, old, new = check(tmp_path, restart_failures=1)
+    assert result['status'] == 'rolled_back'
+    assert result['reason'] == 'restarting the services failed'
+    assert head == old
+
+
+def test_restarts_that_keep_failing_after_rollback_are_rollback_failed(tmp_path):
+    code, result, host, head, old, new = check(tmp_path, restart_failures=100)
+    assert code == 1 and result['status'] == 'rollback_failed'
+    assert 'restarting the services failed' in result['detail']
+
+
+def test_an_unreadable_restart_count_is_never_called_stable(tmp_path):
+    """With no restart count a crash loop looks healthy between attempts."""
+    code, result, host, head, old, new = check(tmp_path, count_readable=False)
+    assert result['status'] != 'success'
+    assert 'restart count could not be read' in result['reason']
+
+
 def test_nothing_pending_does_nothing(tmp_path):
     repo, _, _ = updated_repo(tmp_path)
     host = FakeHost(repo, None)
@@ -201,8 +231,8 @@ def test_units_installers_and_updater_agree():
     request = f'__PROJECT_ROOT_DIR__/{au.REQUEST_REL.as_posix()}'
     assert f'PathExists={request}' in path
     assert f'Unit={aus.SERVICE_UNIT}' in path
-    assert f'ExecStartPre=/bin/rm -f {request}' in service, "the request must be consumed or the path unit re-fires"
+    assert f'ExecStartPre=/bin/rm -f "{request}"' in service, "the request must be consumed or the path unit re-fires"
-    assert f'__PROJECT_ROOT_DIR__/{au.VERIFIER_COPY_REL.as_posix()} __PROJECT_ROOT_DIR__' in service
+    assert f'"__PROJECT_ROOT_DIR__/{au.VERIFIER_COPY_REL.as_posix()}" "__PROJECT_ROOT_DIR__"' in service
     assert au.PENDING_REL.name == av.PENDING_NAME
     assert au.PATH_UNIT == aus.PATH_UNIT and au.SETUP_RESULT_REL == aus.RESULT_REL
 

web_interface/blueprints/api_v3/system.py

@@ -129,10 +129,11 @@ def get_auto_update_status():
     try:
         config = api_v3.config_manager.load_config() if api_v3.config_manager else {}
         return jsonify({'status': 'success', 'data': auto_update.describe_status(config)})
-    except Exception as e:
+    except Exception:
+        # Details stay in the log: exception text can carry file paths.
         logger.error("get_auto_update_status failed", exc_info=True)
-        return jsonify({'status': 'error', 'message': 'Could not read automatic update status',
+        return jsonify({'status': 'error',
-                        'details': describe_exception(e)}), 500
+                        'message': 'Could not read automatic update status; see logs for details'}), 500
 
 
 @api_v3.route('/system/auto-update/dismiss', methods=['POST'])