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

Fix restart fix

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]:

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]