fix: make a native install recoverable, honest and safe to quote

From the outside-diff findings on #2800:

- install.json is written before the services are installed and started. A
  service that fails to start used to leave units behind in a directory that
  status, down and uninstall no longer recognised as a native install, so
  nothing could clean them up.
- systemd stop and removal propagate failures: `down` reporting success while
  the unit still runs, or `uninstall` dropping the unit file and the record
  while systemd still runs the service, is worse than an error. Removing a unit
  that is already gone stays harmless.
- WorkingDirectory and each Environment value are quoted and escaped for
  systemd. `--dir` takes a free-form path, and one with a space in it is not
  hypothetical: this checkout lives in one.
- Native status checks and prints http://localhost:<port>, which is what the
  units bind. With a LAN DOCSGPT_BIND it used to poll an address nothing
  listened on and call a healthy install dead.

docsgpt/deploy/commands.py

@@ -292,17 +292,19 @@ def _native_up(args, context: Context, directory: Path) -> int:
     if context.run([*launcher, "migrate"], stack_env) != 0:
         raise DeployError("`docsgpt migrate` failed; check the database URL and that the server is reachable.")
 
+    # The record goes in before the services: if one fails to start, this is still a native install
+    # that status, down and uninstall can see and clean up, rather than orphaned units.
+    now = datetime.now(timezone.utc).isoformat(timespec="seconds")
+    record = record or {"installed_at": now}
+    record.update(version=context.version, mode="native", updated_at=now)
+    (directory / stack.RECORD_FILE).write_text(json.dumps(record, indent=2) + "\n", encoding="utf-8")
+
     for unit in native.units_for(directory, launcher, port, directory):
         services.install(unit)
     for name in native.SERVICES:
         print(f"Starting {name} ...")
         services.start(name)
 
-    now = datetime.now(timezone.utc).isoformat(timespec="seconds")
-    record = record or {"installed_at": now}
-    record.update(version=context.version, mode="native", updated_at=now)
-    (directory / stack.RECORD_FILE).write_text(json.dumps(record, indent=2) + "\n", encoding="utf-8")
-
     health = f"http://127.0.0.1:{port}/api/health"
     print("Waiting for DocsGPT to answer ...")
     if not context.wait(health, args.timeout):
@@ -443,11 +445,15 @@ def status(args, context: Optional[Context] = None) -> int:
         return 1
     if _mode(directory) == "native":
         services = context.service_manager()
+        # The units bind 127.0.0.1 whatever DOCSGPT_BIND says, so that is what gets printed and checked:
+        # against a LAN bind, stack.url and stack.health_url would advertise and poll an address
+        # nothing listens on, and status would call a healthy install dead.
+        port = env.get("DOCSGPT_PORT") or stack.DEFAULT_PORT
         print(f"DocsGPT {_record(directory).get('version', 'unknown')} in {directory} (native, {services.name})")
-        print(f"Address: {stack.url(env, context.lan_ip())}")
+        print(f"Address: http://localhost:{port}")
         for name in native.SERVICES:
             print(f"  {name}: {'running' if services.is_running(name) else 'stopped'}")
-        healthy = context.wait(stack.health_url(env), 0)
+        healthy = context.wait(f"http://127.0.0.1:{port}/api/health", 0)
         print("API: answering" if healthy else "API: not answering (see `docsgpt logs`)")
         return 0 if healthy else 1
 

docsgpt/deploy/native.py

@@ -61,9 +61,17 @@ def launchd_plist(unit: Unit, label: Optional[str] = None) -> str:
     return plistlib.dumps(body).decode("utf-8")
 
 
+def _systemd_quote(value: str) -> str:
+    """A unit-file value, double-quoted with backslashes and quotes escaped, as systemd reads them."""
+    escaped = value.replace("\\", "\\\\").replace('"', '\\"')
+    return f'"{escaped}"'
+
+
 def systemd_unit(unit: Unit) -> str:
-    """The systemd user unit for ``unit``; arguments are quoted, so a path with spaces survives."""
-    environment = "\n".join(f'Environment="{key}={value}"' for key, value in sorted(unit.environment.items()))
+    """The systemd user unit for ``unit``; values are quoted, so a path with spaces survives."""
+    environment = "\n".join(
+        f"Environment={_systemd_quote(f'{key}={value}')}" for key, value in sorted(unit.environment.items())
+    )
     command = " ".join(shlex.quote(argument) for argument in unit.arguments)
     return f"""[Unit]
 Description=DocsGPT ({unit.name})
@@ -72,7 +80,7 @@ After=network-online.target
 [Service]
 Type=simple
 ExecStart={command}
-WorkingDirectory={unit.working_directory}
+WorkingDirectory={_systemd_quote(unit.working_directory)}
 {environment}
 Restart=always
 RestartSec=5
@@ -181,12 +189,15 @@ class SystemdServices:
         self._systemctl("restart", f"{name}.service", check=True)
 
     def stop(self, name: str) -> None:
-        self._systemctl("stop", f"{name}.service")
+        self._systemctl("stop", f"{name}.service", check=True)
 
     def remove(self, name: str) -> None:
-        self._systemctl("disable", "--now", f"{name}.service")
+        # Only disable a unit systemd still knows about, so removing twice stays harmless; a real
+        # failure has to surface before the unit file and the install record are thrown away.
+        if self._unit_file(name).is_file():
+            self._systemctl("disable", "--now", f"{name}.service", check=True)
         self._unit_file(name).unlink(missing_ok=True)
-        self._systemctl("daemon-reload")
+        self._systemctl("daemon-reload", check=True)
 
     def is_running(self, name: str) -> bool:
         return self._systemctl("is-active", "--quiet", f"{name}.service").returncode == 0

tests/deploy/test_native.py

@@ -148,6 +148,22 @@ class TestNativeUp:
         assert _run(argv, _native_context(services)) == 0
         assert services.units["docsgpt-api"].arguments[:3] == [sys.executable, "-m", "docsgpt"]
 
+    def test_a_failed_start_leaves_an_install_the_other_commands_can_clean_up(self, tmp_path):
+        """Without the record, half-started units are orphaned: status, down and uninstall refuse the dir."""
+        class FailingStart(FakeServices):
+            def start(self, name):
+                super().start(name)
+                if name == "docsgpt-worker":
+                    raise DeployError("systemctl could not start docsgpt-worker")
+
+        services = FailingStart()
+        argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d"]
+        with pytest.raises(DeployError, match="could not start"):
+            _run(argv, _native_context(services))
+        assert json.loads((tmp_path / "install.json").read_text())["mode"] == "native"
+        assert _run(["uninstall", "--yes", "--dir", str(tmp_path)], _native_context(services)) == 0
+        assert sorted(services.removed) == ["docsgpt-api", "docsgpt-worker"]
+
     def test_a_database_url_is_required(self, tmp_path):
         with pytest.raises(DeployError, match="--postgres-uri"):
             _run(["up", "--native", "--dir", str(tmp_path), "--yes"], _native_context())
@@ -182,6 +198,17 @@ class TestNativeLifecycle:
         assert "native" in out.lower()
         assert "docsgpt-api" in out
 
+    def test_status_checks_the_address_the_services_actually_listen_on(self, tmp_path):
+        """A LAN DOCSGPT_BIND would have status poll an address the native units never bind."""
+        services = FakeServices()
+        self._installed(tmp_path, services)
+        envfile.update(tmp_path / ".env", {"DOCSGPT_BIND": "192.168.1.50"})
+        checked = []
+        context = _native_context(services)
+        context.wait = lambda url, timeout: checked.append(url) or True
+        assert _run(["status", "--dir", str(tmp_path)], context) == 0
+        assert checked == ["http://127.0.0.1:7091/api/health"]
+
     def test_down_stops_them_and_leaves_the_settings(self, tmp_path):
         services = FakeServices()
         self._installed(tmp_path, services)
@@ -321,6 +348,35 @@ class TestSystemd:
         assert self._services(FakeSystemctl(active=True), tmp_path).is_running("docsgpt-api") is True
         assert self._services(FakeSystemctl(active=False), tmp_path).is_running("docsgpt-api") is False
 
+    def test_a_stop_that_fails_is_not_reported_as_success(self, tmp_path):
+        """`down` saying it stopped while the service still runs is worse than an error."""
+        class Failing(FakeSystemctl):
+            def __call__(self, args, **kwargs):
+                self.calls.append(args)
+                return types.SimpleNamespace(returncode=1, stdout="", stderr="Failed to stop docsgpt-api")
+
+        with pytest.raises(DeployError, match="Failed to stop"):
+            self._services(Failing(), tmp_path).stop("docsgpt-api")
+
+    def test_a_removal_that_fails_keeps_the_unit_file(self, tmp_path):
+        """uninstall must not drop the unit file and the record while systemd still runs the service."""
+        class Failing(FakeSystemctl):
+            def __call__(self, args, **kwargs):
+                self.calls.append(args)
+                code = 1 if args[2] == "disable" else 0
+                return types.SimpleNamespace(returncode=code, stdout="", stderr="Failed to disable")
+
+        services = self._services(Failing(), tmp_path)
+        services.install(native.units_for(tmp_path, ["/venv/bin/docsgpt"], 7091, tmp_path)[0])
+        with pytest.raises(DeployError, match="Failed to disable"):
+            services.remove("docsgpt-api")
+        assert (tmp_path / ".config" / "systemd" / "user" / "docsgpt-api.service").is_file()
+
+    def test_removing_a_unit_that_is_already_gone_is_harmless(self, tmp_path):
+        systemctl = FakeSystemctl()
+        self._services(systemctl, tmp_path).remove("docsgpt-api")
+        assert "disable" not in systemctl.verbs, "nothing to disable when the unit file is gone"
+
     def test_an_explicit_home_wins_over_xdg_config_home(self, tmp_path, monkeypatch):
         """home is how a caller redirects the unit directory; the environment must not override it."""
         monkeypatch.setenv("XDG_CONFIG_HOME", str(tmp_path / "xdg"))
@@ -378,6 +434,20 @@ class TestUnitFiles:
         assert "ExecStart=/opt/venv/bin/docsgpt api --port 7091" in body
         assert "WantedBy=default.target" in body
 
+    def test_a_directory_with_spaces_and_quotes_survives_the_systemd_unit(self, tmp_path):
+        """--dir is free-form, and this repo itself lives under a path with a space in it."""
+        odd = '/srv/my "odd" dir'
+        unit = native.Unit(
+            name="docsgpt-api",
+            arguments=["/venv/bin/docsgpt", "api"],
+            environment={"DOCSGPT_HOME": odd},
+            working_directory=odd,
+            log_file="/srv/api.log",
+        )
+        body = native.systemd_unit(unit)
+        assert 'WorkingDirectory="/srv/my \\"odd\\" dir"' in body
+        assert 'Environment="DOCSGPT_HOME=/srv/my \\"odd\\" dir"' in body
+
     def test_an_argument_with_spaces_survives_the_systemd_unit(self, tmp_path):
         unit = self._unit(tmp_path)
         unit.arguments = ["/opt/my venv/bin/docsgpt", "api"]