Merge pull request #462 from openhop-dev/fix-restart-fix

Fix restart fix

.github/workflows/docker-publish.yml

@@ -3,8 +3,10 @@ name: Publish Docker Image
 on:
   push:
     branches:
-      - main
       - dev
+    tags:
+      - "[0-9]*.[0-9]*.[0-9]*"
+      - "v[0-9]*.[0-9]*.[0-9]*"
   workflow_dispatch:
     inputs:
       image_repository:
@@ -50,6 +52,12 @@ jobs:
           version="$(python -m setuptools_scm)"
           echo "version=${version}" >> "$GITHUB_OUTPUT"
 
+      - name: Validate release channel
+        id: release_channel
+        env:
+          PACKAGE_VERSION: ${{ steps.package_version.outputs.version }}
+        run: bash scripts/docker-release-channel.sh
+
       - name: Log in to Docker Hub
         uses: docker/login-action@v3
         with:
@@ -77,7 +85,7 @@ jobs:
         with:
           images: ${{ steps.image_repo.outputs.image_repository }}
           tags: |
-            type=raw,value=main,enable=${{ github.ref == 'refs/heads/main' }}
+            type=raw,value=main,enable=${{ steps.release_channel.outputs.channel == 'main' }}
             type=raw,value=dev,enable=${{ github.ref == 'refs/heads/dev' }}
             type=raw,value=v${{ steps.package_version.outputs.version }}
             type=sha,format=short
@@ -99,10 +107,10 @@ jobs:
       - name: Notify Home Assistant add-on repository
         if: >-
           github.repository == 'openhop-dev/openhop_repeater' &&
-          (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/dev')
+          (steps.release_channel.outputs.channel == 'main' || steps.release_channel.outputs.channel == 'dev')
         env:
           DISPATCH_TOKEN: ${{ secrets.HA_ADDON_REPO_DISPATCH_TOKEN }}
-          CHANNEL: ${{ github.ref_name }}
+          CHANNEL: ${{ steps.release_channel.outputs.channel }}
           REVISION: ${{ github.sha }}
         run: |
           if [ -z "${DISPATCH_TOKEN}" ]; then

repeater/service_utils.py

@@ -18,6 +18,7 @@ _CONTAINER_RESTART_DELAY_SECONDS = 1.0
 _SH_BIN = shutil.which("sh") or "sh"
 _SYSTEMCTL_BIN = shutil.which("systemctl") or "systemctl"
 _SUDO_BIN = shutil.which("sudo") or "sudo"
+_SUDO_SYSTEMCTL_BIN = "/usr/bin/systemctl"
 
 
 def is_buildroot() -> bool:
@@ -277,9 +278,53 @@ def restart_service() -> Tuple[bool, str]:
             logger.error(f"Buildroot restart failed: {exc}")
             return False, f"Restart failed: {exc}"
 
+    def _try_legacy_restart(reason: str) -> Tuple[bool, str]:
+        """Fallback to the pre-systemd-run direct restart path."""
+        legacy_cmd = [systemctl_bin, "restart", "openhop-repeater"]
+        if os.geteuid() != 0:
+            legacy_cmd = [_SUDO_BIN, "--non-interactive", *legacy_cmd]
+        try:
+            legacy = subprocess.run(
+                legacy_cmd,
+                capture_output=True,
+                text=True,
+                timeout=5,
+            )  # nosec B603
+            if legacy.returncode == 0:
+                logger.info("Service restart via legacy fallback succeeded (%s)", reason)
+                return True, "Service restart initiated (legacy fallback)"
+            if legacy.returncode < 0:
+                # When running inside the service cgroup, a direct restart can
+                # terminate this process before subprocess.run returns. Treat
+                # termination by signal as likely success.
+                logger.warning(
+                    "Legacy restart interrupted by signal (%s) after %s; treating as likely success",
+                    legacy.returncode,
+                    reason,
+                )
+                return (
+                    True,
+                    "Service restart likely initiated (legacy fallback interrupted by signal)",
+                )
+            legacy_err = legacy.stderr.strip() or f"exit status {legacy.returncode}"
+            return False, f"legacy fallback failed: {legacy_err}"
+        except subprocess.TimeoutExpired:
+            logger.warning("Legacy restart timed out after %s", reason)
+            return True, "Service restart initiated (legacy fallback timeout - likely restarting)"
+        except FileNotFoundError:
+            return False, "legacy fallback unavailable: systemctl/sudo not found"
+        except Exception as exc:
+            return False, f"legacy fallback error: {exc}"
+
     # A command launched inside this service shares its cgroup, even with
     # --no-block or start_new_session. PID 1 must own the delayed restart so
     # submission can return before our process group receives SIGTERM.
+    # Non-root restarts are constrained by manage.sh sudoers allowlists,
+    # which pin /usr/bin/systemctl in the permitted command string.
+    systemctl_bin = _SYSTEMCTL_BIN
+    if os.geteuid() != 0 and os.path.exists(_SUDO_SYSTEMCTL_BIN):
+        systemctl_bin = _SUDO_SYSTEMCTL_BIN
+
     command = [
         "/usr/bin/systemd-run",
         "--quiet",
@@ -288,7 +333,7 @@ def restart_service() -> Tuple[bool, str]:
         "--on-active=2s",
         "--timer-property=AccuracySec=100ms",
         "--timer-property=RemainAfterElapse=no",
-        _SYSTEMCTL_BIN,
+        systemctl_bin,
         "restart",
         "openhop-repeater",
     ]
@@ -306,8 +351,11 @@ def restart_service() -> Tuple[bool, str]:
                 "Restart unconfirmed: command interrupted; check service status before retrying",
             )
         error_msg = result.stderr.strip() or f"exit status {result.returncode}"
+        fallback_ok, fallback_msg = _try_legacy_restart("scheduled restart failure")
+        if fallback_ok:
+            return True, fallback_msg
         logger.error("Restart scheduling failed: %s", error_msg)
-        return False, f"Restart failed: {error_msg}"
+        return False, f"Restart failed: {error_msg}; {fallback_msg}"
     except subprocess.TimeoutExpired:
         logger.warning("Restart scheduling timed out; acceptance is unconfirmed")
         return (
@@ -315,10 +363,19 @@ def restart_service() -> Tuple[bool, str]:
             "Restart unconfirmed: submission timeout; check service status before retrying",
         )
     except FileNotFoundError:
-        return False, "Restart scheduler unavailable; install systemd-run and sudo via manage.sh"
+        fallback_ok, fallback_msg = _try_legacy_restart("missing systemd-run scheduler")
+        if fallback_ok:
+            return True, fallback_msg
+        return False, (
+            "Restart scheduler unavailable; install systemd-run and sudo via manage.sh; "
+            f"{fallback_msg}"
+        )
     except Exception as exc:
+        fallback_ok, fallback_msg = _try_legacy_restart("restart scheduler exception")
+        if fallback_ok:
+            return True, fallback_msg
         logger.error("Error scheduling restart: %s", exc)
-        return False, f"Restart command failed: {exc}"
+        return False, f"Restart command failed: {exc}; {fallback_msg}"
 
 
 def _is_cherrypy_engine_running() -> Optional[bool]:

repeater/web/update_endpoints.py

@@ -693,6 +693,10 @@ def _fetch_latest_version(channel: str) -> str:
             raise RuntimeError(
                 f"Could not compare {base_tag}...{channel} on GitHub: {exc}"
             ) from exc
+        # On an exact tag (ahead_by == 0), setuptools-scm reports the tag
+        # itself, not next-patch.dev0.
+        if ahead_by <= 0:
+            return base_tag
         return _next_dev_version(base_tag, ahead_by)
 
     # Static version channel — read the pinned version from pyproject.toml on

scripts/docker-release-channel.sh

@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+# Validate stable publication before Docker login/build. Dev remains push-driven.
+set -euo pipefail
+
+channel=""
+case "${GITHUB_REF:?}" in
+  refs/heads/dev) channel=dev ;;
+  refs/heads/main|refs/tags/*)
+    git fetch origin main
+    git merge-base --is-ancestor HEAD FETCH_HEAD || {
+      printf '%s\n' 'Release commit must belong to main.' >&2
+      exit 1
+    }
+    if [[ "$GITHUB_REF" == refs/tags/* ]]; then
+      tag="${GITHUB_REF#refs/tags/}"
+    else
+      tag="$(git describe --tags --exact-match HEAD)"
+    fi
+    [[ "$tag" =~ ^v?(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$ ]] || {
+      printf '%s\n' 'Main publication requires an exact stable X.Y.Z or vX.Y.Z tag.' >&2
+      exit 1
+    }
+    [[ "$(git rev-parse "refs/tags/${tag}^{commit}")" == "$(git rev-parse HEAD)" ]] || exit 1
+    [[ "${PACKAGE_VERSION:?}" == "${tag#v}" ]] || {
+      printf '%s\n' 'Computed package version does not match the release tag.' >&2
+      exit 1
+    }
+    channel=main
+    ;;
+esac
+printf 'channel=%s\n' "$channel" >> "${GITHUB_OUTPUT:?}"

tests/test_docker_release_channel.py

@@ -0,0 +1,93 @@
+"""Exercise publication guards with real temporary Git history; no registry writes."""
+
+import os
+from pathlib import Path
+import subprocess
+import tempfile
+import unittest
+
+
+SCRIPT = Path(__file__).resolve().parents[1] / "scripts/docker-release-channel.sh"
+
+
+class ReleaseChannelTests(unittest.TestCase):
+    def setUp(self):
+        self.temp = tempfile.TemporaryDirectory()
+        self.addCleanup(self.temp.cleanup)
+        self.root = Path(self.temp.name)
+        self.repo = self.root / "repo"
+        self.repo.mkdir()
+        self.git("init", "-b", "main")
+        self.git("config", "user.name", "Test")
+        self.git("config", "user.email", "test@example.invalid")
+        self.git("commit", "--allow-empty", "-m", "base")
+        self.git("remote", "add", "origin", str(self.repo))
+        self.git("tag", "1.2.3")
+
+    def git(self, *args):
+        return subprocess.run(
+            ["git", *args], cwd=self.repo, check=True, capture_output=True, text=True
+        ).stdout.strip()
+
+    def run_guard(self, ref, version, channel=None):
+        output = self.root / "output"
+        output.write_text("")
+        result = subprocess.run(
+            ["bash", str(SCRIPT)],
+            cwd=self.repo,
+            env={
+                **os.environ,
+                "GITHUB_REF": ref,
+                "PACKAGE_VERSION": version,
+                "GITHUB_OUTPUT": str(output),
+            },
+            capture_output=True,
+            text=True,
+        )
+        if channel is None:
+            self.assertNotEqual(result.returncode, 0, result.stdout)
+            self.assertEqual(output.read_text(), "")
+        else:
+            self.assertEqual(result.returncode, 0, result.stderr)
+            self.assertEqual(output.read_text(), f"channel={channel}\n")
+
+    def test_release_tag(self):
+        self.run_guard("refs/tags/1.2.3", "1.2.3", "main")
+
+    def test_v_release_tag(self):
+        self.git("tag", "v1.2.3")
+        self.run_guard("refs/tags/v1.2.3", "1.2.3", "main")
+
+    def test_manual_tagged_main(self):
+        self.run_guard("refs/heads/main", "1.2.3", "main")
+
+    def test_untagged_main(self):
+        self.git("commit", "--allow-empty", "-m", "untagged")
+        self.run_guard("refs/heads/main", "1.2.4.dev1")
+
+    def test_prerelease_rejected(self):
+        self.git("tag", "1.2.4.dev1")
+        self.run_guard("refs/tags/1.2.4.dev1", "1.2.4.dev1")
+
+    def test_version_mismatch_rejected(self):
+        self.run_guard("refs/tags/1.2.3", "1.2.3.dev348")
+
+    def test_tag_on_non_main_commit_rejected(self):
+        self.git("checkout", "-b", "dev")
+        self.git("commit", "--allow-empty", "-m", "dev only")
+        self.git("tag", "1.2.4")
+        self.run_guard("refs/tags/1.2.4", "1.2.4")
+
+    def test_tag_points_elsewhere_rejected(self):
+        self.git("commit", "--allow-empty", "-m", "new main")
+        self.run_guard("refs/tags/1.2.3", "1.2.3")
+
+    def test_dev_unchanged(self):
+        self.run_guard("refs/heads/dev", "1.2.4.dev7", "dev")
+
+    def test_feature_manual_does_not_publish_main(self):
+        self.run_guard("refs/heads/fix/example", "1.2.4.dev7", "")
+
+
+if __name__ == "__main__":
+    unittest.main()

tests/test_restart_job_submission.py

@@ -19,6 +19,7 @@ def systemd_host(monkeypatch):
 @pytest.mark.parametrize("root", [False, True])
 def test_restart_submits_delayed_external_job(monkeypatch, root):
     monkeypatch.setattr(su.os, "geteuid", lambda: 0 if root else 1000)
+    monkeypatch.setattr(su.os.path, "exists", lambda p: p == su._SUDO_SYSTEMCTL_BIN)
     run = Mock(return_value=subprocess.CompletedProcess([], 0, "", ""))
     monkeypatch.setattr(su.subprocess, "run", run)
     ok, message = su.restart_service()
@@ -30,32 +31,77 @@ def test_restart_submits_delayed_external_job(monkeypatch, root):
     assert "--timer-property=AccuracySec=100ms" in args
     assert "--timer-property=RemainAfterElapse=no" in args
     assert "--collect" in args
-    assert args[-3:] == [su._SYSTEMCTL_BIN, "restart", "openhop-repeater"]
+    expected_systemctl = su._SYSTEMCTL_BIN if root else su._SUDO_SYSTEMCTL_BIN
+    assert args[-3:] == [expected_systemctl, "restart", "openhop-repeater"]
     if not root:
         assert args[:2] == [su._SUDO_BIN, "--non-interactive"]
     else:
         assert args[0] == "/usr/bin/systemd-run"
 
 
-@pytest.mark.parametrize("outcome", ["timeout", "interrupted", "denied"])
+def test_restart_non_root_prefers_sudoers_whitelisted_systemctl_path(monkeypatch):
+    monkeypatch.setattr(su.os, "geteuid", lambda: 1000)
+    monkeypatch.setattr(su.os.path, "exists", lambda p: p == su._SUDO_SYSTEMCTL_BIN)
+    run = Mock(return_value=subprocess.CompletedProcess([], 0, "", ""))
+    monkeypatch.setattr(su.subprocess, "run", run)
+
+    ok, _message = su.restart_service()
+
+    assert ok is True
+    args = run.call_args.args[0]
+    assert args[-3:] == [su._SUDO_SYSTEMCTL_BIN, "restart", "openhop-repeater"]
+
+
+@pytest.mark.parametrize("outcome", ["timeout", "interrupted"])
 def test_failed_submission_does_not_claim_success_or_retry(monkeypatch, outcome):
     run = Mock()
     if outcome == "timeout":
         run.side_effect = subprocess.TimeoutExpired("systemd-run", 5)
     else:
-        run.return_value = subprocess.CompletedProcess(
-            [], -15 if outcome == "interrupted" else 1, "", "Access denied"
-        )
+        run.return_value = subprocess.CompletedProcess([], -15, "", "Access denied")
     monkeypatch.setattr(su.subprocess, "run", run)
     ok, message = su.restart_service()
     assert not ok
-    if outcome != "denied":
-        assert "unconfirmed" in message.lower()
-    else:
-        assert "Access denied" in message
+    assert "unconfirmed" in message.lower()
     run.assert_called_once()
 
 
+def test_non_root_denied_schedule_retries_legacy_and_succeeds(monkeypatch):
+    monkeypatch.setattr(su.os, "geteuid", lambda: 1000)
+    monkeypatch.setattr(su.os.path, "exists", lambda p: p == su._SUDO_SYSTEMCTL_BIN)
+    run = Mock(
+        side_effect=[
+            subprocess.CompletedProcess([], 1, "", "sudo: a password is required"),
+            subprocess.CompletedProcess([], 0, "", ""),
+        ]
+    )
+    monkeypatch.setattr(su.subprocess, "run", run)
+
+    ok, message = su.restart_service()
+
+    assert ok is True
+    assert "legacy fallback" in message
+    assert run.call_count == 2
+
+
+def test_non_root_denied_schedule_legacy_interrupted_is_likely_success(monkeypatch):
+    monkeypatch.setattr(su.os, "geteuid", lambda: 1000)
+    monkeypatch.setattr(su.os.path, "exists", lambda p: p == su._SUDO_SYSTEMCTL_BIN)
+    run = Mock(
+        side_effect=[
+            subprocess.CompletedProcess([], 1, "", "sudo: a password is required"),
+            subprocess.CompletedProcess([], -15, "", ""),
+        ]
+    )
+    monkeypatch.setattr(su.subprocess, "run", run)
+
+    ok, message = su.restart_service()
+
+    assert ok is True
+    assert "likely initiated" in message
+    assert run.call_count == 2
+
+
 def test_installer_authorizes_only_fixed_restart_timer_command():
     script = (Path(__file__).resolve().parents[1] / "manage.sh").read_text()
     rules = [line for line in script.splitlines() if "NOPASSWD:" in line]

tests/test_update_endpoints_unit.py

@@ -337,6 +337,11 @@ def test_fetch_latest_version_dynamic_and_static(monkeypatch):
     dyn = ue._fetch_latest_version("dev")
     assert dyn == "1.0.6.dev3"
 
+    # Exact tag (ahead_by == 0) must resolve to the tag, not next.dev0.
+    monkeypatch.setattr(ue, "_fetch_url", lambda _url, timeout=10: '{"ahead_by": 0}')
+    on_tag = ue._fetch_latest_version("main")
+    assert on_tag == "1.0.5"
+
     # Static branch path parses version from pyproject content.
     monkeypatch.setattr(ue, "_branch_is_dynamic", lambda _ch: False)
     monkeypatch.setattr(