diff --git a/first_time_install.sh b/first_time_install.sh index 7d313d5d..53bbb2e5 100755 --- a/first_time_install.sh +++ b/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: - json.dump(config, f, indent=4) - f.write("\n") +# Written beside the original and swapped in whole: the display service's +# config watcher may be running and must never read a half-written file. +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 diff --git a/scripts/utils/auto_update_verify.py b/scripts/utils/auto_update_verify.py index 245166eb..048721a2 100644 --- a/scripts/utils/auto_update_verify.py +++ b/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') - self.restart('ledmatrix-web') + ok = self.restart('ledmatrix') and ok + 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) + still = (self.wait_healthy(display) if self.restart_services(display) + else 'restarting the services failed') if still: self._finish(pending, 'rollback_failed', reason, f'still unhealthy after rolling back: {still}' diff --git a/src/auto_update_setup.py b/src/auto_update_setup.py index bd814c8e..6ebae2fb 100644 --- a/src/auto_update_setup.py +++ b/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: diff --git a/systemd/ledmatrix-update-verify.service b/systemd/ledmatrix-update-verify.service index dba4b47b..5b4408d7 100644 --- a/systemd/ledmatrix-update-verify.service +++ b/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 -ExecStart=/usr/bin/python3 __PROJECT_ROOT_DIR__/data/auto_update_verifier.py __PROJECT_ROOT_DIR__ +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__" TimeoutStartSec=30min StandardOutput=syslog StandardError=syslog diff --git a/test/test_auto_update_setup.py b/test/test_auto_update_setup.py index 866c2ee7..745ff455 100644 --- a/test/test_auto_update_setup.py +++ b/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): - root = tmp_path / 'LEDMatrix' +def project(tmp_path, user='hdpi', workdir=None, name='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 diff --git a/test/test_auto_update_verify.py b/test/test_auto_update_verify.py index 58560081..8a6c06c2 100644 --- a/test/test_auto_update_verify.py +++ b/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'__PROJECT_ROOT_DIR__/{au.VERIFIER_COPY_REL.as_posix()} __PROJECT_ROOT_DIR__' in service + 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 au.PENDING_REL.name == av.PENDING_NAME assert au.PATH_UNIT == aus.PATH_UNIT and au.SETUP_RESULT_REL == aus.RESULT_REL diff --git a/web_interface/blueprints/api_v3/system.py b/web_interface/blueprints/api_v3/system.py index 0346ab21..a0eb3370 100644 --- a/web_interface/blueprints/api_v3/system.py +++ b/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', - 'details': describe_exception(e)}), 500 + return jsonify({'status': 'error', + 'message': 'Could not read automatic update status; see logs for details'}), 500 @api_v3.route('/system/auto-update/dismiss', methods=['POST'])