diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 5a853e8f..1f526e86 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -221,7 +221,7 @@ def _redis_urls(base: str) -> dict[str, str]: "database each." ) path = parts.path.rstrip("/").lstrip("/") - if path and not path.isdigit(): + if path and not (path.isascii() and path.isdigit()): raise DeployError( f"the Redis URL {base!r} has {path!r} where a database number would go. Pass a URL like " "redis://host:6379 or redis://host:6379/5." @@ -271,6 +271,36 @@ def _refuse_docker_only_options(args) -> None: ) +def _port_number(value: object, source: str) -> int: + """A port from the command line or a hand-edited .env, or a DeployError naming where it came from.""" + text = str(value) + if not (text.isascii() and text.isdigit() and 0 < int(text) < 65536): + raise DeployError(f"{source} is {value!r}, which is not a port number between 1 and 65535.") + return int(text) + + +def _service_names(directory: Path) -> tuple[str, str]: + """This install's service names; an install elsewhere gets its own, so the two cannot collide.""" + return native.service_names(directory, stack.stack_dir(None)) + + +def _refuse_other_docker_stack(context: Context, directory: Path, port: int) -> None: + """A Docker stack elsewhere on this port would answer the health check the native API failed.""" + try: + others = {path for path in context.docker.project_dirs(PROJECT) if path.resolve() != directory.resolve()} + except DeployError: + return # No Docker on this machine to ask, which is a fair reason to run natively. + for other in sorted(others): + other_port = envfile.read(other / ".env").get("DOCSGPT_PORT") or str(stack.DEFAULT_PORT) + if str(other_port) == str(port): + raise DeployError( + f"Docker already runs a DocsGPT stack from {other} on port {port}, which a native " + f"install would try to bind as well: the health check here could answer from that " + f"stack while these services failed to start. Stop it with `docsgpt down --dir " + f"{other}`, or give this install another port with --port." + ) + + def _native_port(env: Mapping[str, str]) -> str: """The port a native install listens on.""" return str(env.get("DOCSGPT_PORT") or stack.DEFAULT_PORT) @@ -304,7 +334,12 @@ def _native_up(args, context: Context, directory: Path) -> int: ) # An install keeps the port it was given: a later `up` with no --port must not move it back to the default. - port = int(args.port or existing.get("DOCSGPT_PORT") or stack.DEFAULT_PORT) + if args.port: + port = _port_number(args.port, "--port") + elif existing.get("DOCSGPT_PORT"): + port = _port_number(existing["DOCSGPT_PORT"], f"DOCSGPT_PORT in {env_path}") + else: + port = stack.DEFAULT_PORT updates: dict[str, Optional[str]] = { "POSTGRES_URI": postgres, "API_URL": f"http://127.0.0.1:{port}", @@ -334,6 +369,8 @@ 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.") + _refuse_other_docker_stack(context, directory, port) + # 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") @@ -341,9 +378,10 @@ def _native_up(args, context: Context, directory: Path) -> int: 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): + names = _service_names(directory) + for unit in native.units_for(directory, launcher, port, directory, names): services.install(unit) - for name in native.SERVICES: + for name in names: print(f"Starting {name} ...") services.start(name) @@ -358,7 +396,7 @@ def _native_up(args, context: Context, directory: Path) -> int: return 1 print(f"\nDocsGPT is running at http://localhost:{port}") - print(f"Services: {', '.join(native.SERVICES)} under {services.name}") + print(f"Services: {', '.join(names)} under {services.name}") print(f"Settings: {env_path}") print("Manage it with: docsgpt status | logs | down | uninstall") return 0 @@ -471,7 +509,7 @@ def down(args, context: Optional[Context] = None) -> int: if _mode(directory) == "native": services = context.service_manager() # The worker goes first: it talks to the API, not the other way round. - for name in reversed(native.SERVICES): + for name in reversed(_service_names(directory)): services.stop(name) return 0 context.docker.compose(directory, *_EVERY_PROFILE, "down") @@ -492,7 +530,7 @@ def status(args, context: Optional[Context] = None) -> int: port = _native_port(env) print(f"DocsGPT {_record(directory).get('version', 'unknown')} in {directory} (native, {services.name})") print(f"Address: {_native_address(env)}") - for name in native.SERVICES: + for name in _service_names(directory): print(f" {name}: {'running' if services.is_running(name) else 'stopped'}") 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`)") @@ -515,7 +553,7 @@ def logs(args, context: Optional[Context] = None) -> int: return 1 if _mode(directory) == "native": logs_dir = directory / "logs" - wanted = args.services or [name.removeprefix("docsgpt-") for name in native.SERVICES] + wanted = args.services or ["api", "worker"] for service in wanted: path = logs_dir / f"{service}.log" print(f"=== {path}") @@ -775,9 +813,9 @@ def uninstall(args, context: Optional[Context] = None) -> int: return 1 if _mode(directory) == "native": services = context.service_manager() - for name in reversed(native.SERVICES): + for name in reversed(_service_names(directory)): services.remove(name) - print(f"Removed the {', '.join(native.SERVICES)} services.") + print(f"Removed the {', '.join(_service_names(directory))} services.") if args.purge: shutil.rmtree(directory) print(f"Removed {directory}. The database and Redis it used are untouched.") diff --git a/docsgpt/deploy/native.py b/docsgpt/deploy/native.py index ab78d0ed..85e02e01 100644 --- a/docsgpt/deploy/native.py +++ b/docsgpt/deploy/native.py @@ -11,6 +11,7 @@ already run (``--postgres-uri``, ``--redis-url``). from __future__ import annotations +import hashlib import os import plistlib import re @@ -26,7 +27,6 @@ from docsgpt.deploy.docker import DeployError API_SERVICE = "docsgpt-api" WORKER_SERVICE = "docsgpt-worker" -SERVICES = (API_SERVICE, WORKER_SERVICE) LABEL_PREFIX = "cloud.docsgpt" @@ -41,6 +41,20 @@ class Unit: log_file: str +def service_names(stack_directory: Path, default_directory: Path) -> tuple[str, str]: + """This install's two service names: the plain pair for the default install, suffixed for others. + + Service managers keep one namespace for the whole user, so two installs in different directories + would write over each other's units. These names show up in launchctl and systemctl output, so + the usual install keeps the readable ones and only a second install carries a digest. + """ + resolved = stack_directory.expanduser().resolve() + if resolved == default_directory.expanduser().resolve(): + return (API_SERVICE, WORKER_SERVICE) + digest = hashlib.sha256(str(resolved).encode("utf-8")).hexdigest()[:8] + return (f"{API_SERVICE}-{digest}", f"{WORKER_SERVICE}-{digest}") + + def label_for(name: str) -> str: """The launchd label for a service name (``docsgpt-api`` -> ``cloud.docsgpt.api``).""" return f"{LABEL_PREFIX}.{name.removeprefix('docsgpt-')}" @@ -84,8 +98,8 @@ def _check_unit_values(unit: Unit) -> None: 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('"', '\\"') + """A unit-file value: quoted, with backslashes, quotes and percent signs escaped for systemd.""" + escaped = value.replace("\\", "\\\\").replace('"', '\\"').replace("%", "%%") return f'"{escaped}"' @@ -95,7 +109,9 @@ def systemd_unit(unit: Unit) -> str: 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) + # systemd expands % specifiers such as %h, so a literal percent has to be doubled everywhere. + command = " ".join(shlex.quote(argument).replace("%", "%%") for argument in unit.arguments) + log_file = unit.log_file.replace("%", "%%") return f"""[Unit] Description=DocsGPT ({unit.name}) After=network-online.target @@ -107,8 +123,8 @@ WorkingDirectory={_systemd_quote(unit.working_directory)} {environment} Restart=always RestartSec=5 -StandardOutput=append:{unit.log_file} -StandardError=append:{unit.log_file} +StandardOutput=append:{log_file} +StandardError=append:{log_file} [Install] WantedBy=default.target @@ -238,22 +254,24 @@ def services_for_platform(platform: str = sys.platform): ) -def units_for(stack_directory: Path, launcher: list[str], port: int, home: Path) -> list[Unit]: +def units_for(stack_directory: Path, launcher: list[str], port: int, home: Path, + names: tuple[str, str]) -> list[Unit]: """The API and worker services for a native install in ``stack_directory``. ``launcher`` is how DocsGPT is started: the ``docsgpt`` command, or an interpreter and ``-m``. """ environment = {"DOCSGPT_HOME": str(home)} + api_name, worker_name = names return [ Unit( - name=API_SERVICE, + name=api_name, arguments=[*launcher, "api", "--host", "127.0.0.1", "--port", str(port)], environment=dict(environment), working_directory=str(stack_directory), log_file=str(stack_directory / "logs" / "api.log"), ), Unit( - name=WORKER_SERVICE, + name=worker_name, arguments=[*launcher, "worker"], environment=dict(environment), working_directory=str(stack_directory), diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index 0e09a382..55b6d3c3 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -4,13 +4,14 @@ import json import plistlib import sys import types +from pathlib import Path import pytest -from docsgpt.deploy import envfile, native +from docsgpt.deploy import envfile, native, stack from docsgpt.deploy.docker import DeployError -from .test_commands import FakePrompter, _context, _run +from .test_commands import FakeDocker, FakePrompter, _context, _run class FakeServices: @@ -44,6 +45,11 @@ class FakeServices: return name in self.running +def _names(directory): + """The two service names an install in ``directory`` gets; a second directory gets its own.""" + return native.service_names(Path(directory), stack.stack_dir(None)) + + def _native_context(services=None, migrated=None, **overrides): context = _context(**overrides) context.services = services or FakeServices() @@ -82,14 +88,15 @@ class TestNativeUp: assert _run(["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/docsgpt"], context) == 0 assert any("migrate" in " ".join(call) for call in migrated), migrated - assert services.started == ["docsgpt-api", "docsgpt-worker"] + assert services.started == list(_names(tmp_path)) def test_the_units_run_this_interpreter_with_the_stack_as_its_home(self, tmp_path): services = FakeServices() assert _run(["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/docsgpt"], _native_context(services)) == 0 - api = services.units["docsgpt-api"] - worker = services.units["docsgpt-worker"] + api_name, worker_name = _names(tmp_path) + api = services.units[api_name] + worker = services.units[worker_name] assert api.arguments[-5:] == ["api", "--host", "127.0.0.1", "--port", "7091"] assert worker.arguments[-1] == "worker" for unit in (api, worker): @@ -130,7 +137,7 @@ class TestNativeUp: env = envfile.read(tmp_path / ".env") assert env["DOCSGPT_PORT"] == "7099" assert env["API_URL"] == "http://127.0.0.1:7099" - assert services.units["docsgpt-api"].arguments[-1] == "7099" + assert services.units[_names(tmp_path)[0]].arguments[-1] == "7099" def test_it_refuses_to_run_beside_a_docker_install(self, tmp_path): """Native services on the same port would orphan the containers from down/status/uninstall.""" @@ -146,14 +153,14 @@ class TestNativeUp: services = FakeServices() argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d"] assert _run(argv, _native_context(services)) == 0 - assert services.units["docsgpt-api"].arguments[:3] == [sys.executable, "-m", "docsgpt"] + assert services.units[_names(tmp_path)[0]].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": + if name == _names(tmp_path)[1]: raise DeployError("systemctl could not start docsgpt-worker") services = FailingStart() @@ -162,7 +169,7 @@ class TestNativeUp: _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"] + assert sorted(services.removed) == sorted(_names(tmp_path)) def test_docker_only_options_are_refused_rather_than_ignored(self, tmp_path): """Asking for network exposure and silently getting loopback is the worst of both.""" @@ -225,6 +232,51 @@ class TestNativeUp: with pytest.raises(DeployError, match="Redis URL"): _run(argv, _native_context()) + def test_a_port_that_is_not_a_number_is_reported(self, tmp_path): + """A hand-edited DOCSGPT_PORT reached int() unchecked and came out as a traceback.""" + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d"] + assert _run([*argv, "--port", "7099"], _native_context()) == 0 + envfile.update(tmp_path / ".env", {"DOCSGPT_PORT": "seven thousand"}) + with pytest.raises(DeployError, match="not a port number"): + _run(["up", "--dir", str(tmp_path), "--yes"], _native_context()) + + def test_a_redis_database_that_int_would_reject_is_reported(self, tmp_path): + """str.isdigit() is true for a superscript two, which int() then refuses.""" + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d", + "--redis-url", "redis://localhost:6379/\u00b2"] + with pytest.raises(DeployError, match="Redis URL"): + _run(argv, _native_context()) + + def test_two_installs_do_not_share_service_names(self, tmp_path): + """A service manager has one namespace per user, so the second install would overwrite the first.""" + installed = {} + for name in ("one", "two"): + directory = tmp_path / name + services = FakeServices() + argv = ["up", "--native", "--dir", str(directory), "--yes", + "--postgres-uri", "postgresql://localhost/d"] + assert _run(argv, _native_context(services)) == 0 + installed[name] = set(services.units) + assert installed["one"].isdisjoint(installed["two"]), installed + + def test_the_default_install_keeps_the_readable_names(self): + """They are what shows up in launchctl and systemctl, so the usual install is not a digest.""" + default = Path("/opt/docsgpt") + assert native.service_names(default, default) == ("docsgpt-api", "docsgpt-worker") + + def test_a_docker_stack_elsewhere_on_the_same_port_is_refused(self, tmp_path): + """Its API would answer the health check while these services quietly failed to bind.""" + other = tmp_path / "docker-install" + other.mkdir() + (other / ".env").write_text("DOCSGPT_PORT=7099\n", encoding="utf-8") + context = _native_context(FakeServices(), docker=FakeDocker(project_dirs=[str(other)])) + directory = tmp_path / "native" + argv = ["up", "--native", "--dir", str(directory), "--yes", "--port", "7099", + "--postgres-uri", "postgresql://localhost/d"] + with pytest.raises(DeployError, match="already runs a DocsGPT stack"): + _run(argv, context) + assert not (directory / "install.json").exists(), "it refuses before recording the install" + 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()) @@ -286,14 +338,14 @@ class TestNativeLifecycle: services = FakeServices() self._installed(tmp_path, services) assert _run(["down", "--dir", str(tmp_path)], _native_context(services)) == 0 - assert services.stopped == ["docsgpt-worker", "docsgpt-api"], "the worker goes first" + assert services.stopped == list(reversed(_names(tmp_path))), "the worker goes first" assert (tmp_path / ".env").is_file() def test_uninstall_removes_the_services(self, tmp_path): services = FakeServices() self._installed(tmp_path, services) assert _run(["uninstall", "--yes", "--dir", str(tmp_path)], _native_context(services)) == 0 - assert sorted(services.removed) == ["docsgpt-api", "docsgpt-worker"] + assert sorted(services.removed) == sorted(_names(tmp_path)) def test_docker_commands_refuse_a_native_install(self, tmp_path, capsys): self._installed(tmp_path, FakeServices()) @@ -404,7 +456,8 @@ class TestSystemd: def test_install_writes_the_unit_and_reloads(self, tmp_path): systemctl = FakeSystemctl() services = self._services(systemctl, tmp_path) - services.install(native.units_for(tmp_path, ["/venv/bin/docsgpt"], 7091, tmp_path)[0]) + services.install(native.units_for(tmp_path, ["/venv/bin/docsgpt"], 7091, tmp_path, + ("docsgpt-api", "docsgpt-worker"))[0]) unit = tmp_path / ".config" / "systemd" / "user" / "docsgpt-api.service" assert "ExecStart=/venv/bin/docsgpt api" in unit.read_text() assert systemctl.verbs == ["daemon-reload"] @@ -420,7 +473,8 @@ class TestSystemd: def test_stop_and_remove(self, tmp_path): systemctl = FakeSystemctl() services = self._services(systemctl, tmp_path) - services.install(native.units_for(tmp_path, ["/venv/bin/docsgpt"], 7091, tmp_path)[0]) + services.install(native.units_for(tmp_path, ["/venv/bin/docsgpt"], 7091, tmp_path, + ("docsgpt-api", "docsgpt-worker"))[0]) unit = tmp_path / ".config" / "systemd" / "user" / "docsgpt-api.service" services.stop("docsgpt-api") assert unit.is_file(), "stopping keeps the unit" @@ -451,7 +505,8 @@ class TestSystemd: 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]) + services.install(native.units_for(tmp_path, ["/venv/bin/docsgpt"], 7091, tmp_path, + ("docsgpt-api", "docsgpt-worker"))[0]) with pytest.raises(DeployError, match="Failed to disable"): services.remove("docsgpt-api") assert (tmp_path / ".config" / "systemd" / "user" / "docsgpt-api.service").is_file() @@ -557,6 +612,21 @@ class TestUnitFiles: with pytest.raises(DeployError, match="control character"): native.systemd_unit(unit) + def test_a_percent_sign_is_doubled_for_systemd(self, tmp_path): + """systemd expands %h and friends, so a literal percent in a path has to be escaped.""" + unit = native.Unit( + name="docsgpt-api", + arguments=["/venv/bin/docsgpt", "api", "--flag", "100%"], + environment={"DOCSGPT_HOME": "/srv/100% full"}, + working_directory="/srv/100% full", + log_file="/srv/100% full/api.log", + ) + body = native.systemd_unit(unit) + assert 'WorkingDirectory="/srv/100%% full"' in body + assert 'Environment="DOCSGPT_HOME=/srv/100%% full"' in body + assert "append:/srv/100%% full/api.log" in body + assert "100%%" in body.split("ExecStart=")[1].split("\n")[0] + 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"]