fix: give each native install its own services, and guard the port

From the outside-diff findings on #2800:

- Service names are derived from the install directory. A service manager has
  one namespace per user, so two installs in different --dir directories wrote
  over each other's units and down, status and uninstall acted on whichever was
  written last. The default install keeps the readable names; another directory
  gets a digest suffix.
- `up --native` refuses when a Docker stack in another directory publishes the
  same port: its API would answer the health check while these services failed
  to bind. The check degrades quietly when Docker is absent, which is exactly
  the machine a native install targets.
- A Redis database path is required to be ASCII digits: str.isdigit() is true
  for characters int() then refuses.
- DOCSGPT_PORT from a hand-edited .env is validated before conversion, and the
  error names where the bad value came from.
- Percent signs are doubled in systemd values, arguments and log paths, since
  systemd expands specifiers in all of them.

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.")

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),

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