diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index f718e2e0..45b9c461 100644 --- a/docsgpt/deploy/commands.py +++ b/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 diff --git a/docsgpt/deploy/native.py b/docsgpt/deploy/native.py index 8191a33f..876e912a 100644 --- a/docsgpt/deploy/native.py +++ b/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 diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index cc4b13d3..54cb681d 100644 --- a/tests/deploy/test_native.py +++ b/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"]