From f23a32d9c5a3e27939a45038d2653ff4878fc2ab Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 16 Sep 2026 21:51:43 +0100 Subject: [PATCH 01/17] feat: docsgpt up --native Run DocsGPT without Docker: the API and the worker each become a service on the machine itself, a launchd agent on macOS and a systemd user unit on Linux, pointed at a PostgreSQL and a Redis that already run. `docsgpt up --native --postgres-uri ... --redis-url ...` writes the same .env a Docker install uses, applies the migrations and starts both services. status, logs, down and uninstall work on a native install the same way they do on a Docker one, and never touch the database or Redis: they were the user's to begin with. One Redis URL covers the broker, the result backend and the cache on three consecutive databases, starting at the one the URL names, so a Redis that already holds something else can be shared. Windows has neither service manager, so native mode refuses it and says what to do instead. --- .../config/vocabularies/DocsGPT/accept.txt | 6 +- docs/content/Deploying/Pip-Install.mdx | 29 ++ docs/content/changelog.mdx | 8 + docsgpt/cli.py | 4 + docsgpt/deploy/commands.py | 180 ++++++++++- docsgpt/deploy/native.py | 218 ++++++++++++++ tests/deploy/test_native.py | 284 ++++++++++++++++++ 7 files changed, 722 insertions(+), 7 deletions(-) create mode 100644 docsgpt/deploy/native.py create mode 100644 tests/deploy/test_native.py diff --git a/.github/styles/config/vocabularies/DocsGPT/accept.txt b/.github/styles/config/vocabularies/DocsGPT/accept.txt index 7605df49..48520858 100644 --- a/.github/styles/config/vocabularies/DocsGPT/accept.txt +++ b/.github/styles/config/vocabularies/DocsGPT/accept.txt @@ -3,9 +3,9 @@ Anthropic's api APIs Atlassian -automations autoescaping Autoescaping +automations backfill backfills bool @@ -21,9 +21,9 @@ diarization Docling docsgpt docstrings +enqueues Entra env -enqueues EOL ESLint feedbacks @@ -35,6 +35,7 @@ hardcoding Idempotency JSONPath kubectl +launchd Lightsail llama_cpp llm @@ -70,6 +71,7 @@ SGLang Shareability Signup Supabase +systemd UIs uncomment URl diff --git a/docs/content/Deploying/Pip-Install.mdx b/docs/content/Deploying/Pip-Install.mdx index 1b946687..04574490 100644 --- a/docs/content/Deploying/Pip-Install.mdx +++ b/docs/content/Deploying/Pip-Install.mdx @@ -99,6 +99,35 @@ Other commands: - `docsgpt verify-offline`: check that a prepared install starts with networking off. - `docsgpt reembed`: re-embed every index after changing `EMBEDDINGS_NAME` (see [Upgrading](/upgrading)). +## Run it as services, without Docker + +`docsgpt up --native` runs the API and the worker as services on the machine itself: launchd agents on macOS, systemd user units on Linux. It does not start PostgreSQL or Redis; point it at ones you already run. + +```bash +docsgpt up --native \ + --postgres-uri postgresql://docsgpt:@localhost:5432/docsgpt \ + --redis-url redis://localhost:6379 +``` + +Without a terminal both flags are required; with one, it asks for them and for the model provider. It writes the same `.env` a Docker install uses (minus the image settings), generates `INTERNAL_KEY` and `JWT_SECRET_KEY` on the first run, applies the migrations, then starts `docsgpt-api` and `docsgpt-worker` and waits for the API to answer. + +One Redis URL covers all three uses: the Celery broker, its result backend and the cache go on databases 0, 1 and 2 of it. Name a database in the URL and the three start there instead, so `redis://localhost:6379/5` puts them on 5, 6 and 7 — that is how you share a Redis that already holds something else. + +The same commands manage it: + +| Command | In native mode | +| --- | --- | +| `docsgpt status` | Which services run, the address, and whether the API answers | +| `docsgpt logs [api\|worker]` | The service log files under `/logs` | +| `docsgpt down` | Stops both services; settings stay | +| `docsgpt uninstall [--purge]` | Removes the services; `--purge` also deletes the stack directory | + +`uninstall` never touches the database or Redis: they were yours to begin with. + + + Windows has neither launchd nor systemd, so native mode is macOS and Linux only. On Windows, run DocsGPT on Docker with `docsgpt up`, or start `docsgpt api` and `docsgpt worker` yourself. + + ## Upgrade ```bash diff --git a/docs/content/changelog.mdx b/docs/content/changelog.mdx index 2c5f9501..107685e7 100644 --- a/docs/content/changelog.mdx +++ b/docs/content/changelog.mdx @@ -13,6 +13,14 @@ request, and [Upgrading](/upgrading) covers the steps an existing deployment has ## Unreleased +### Run DocsGPT without Docker + +`docsgpt up --native` runs the API and the worker as services on the machine itself, launchd on +macOS and systemd user units on Linux, against a PostgreSQL and Redis you already have +(`--postgres-uri`, `--redis-url`). `status`, `logs`, `down` and `uninstall` work on such an install +the same way they do on a Docker one, and never touch the database or Redis. See +[Run it as services, without Docker](/Deploying/Pip-Install#run-it-as-services-without-docker). + ### Back up and restore an install `docsgpt backup` writes a dump of the database and a tar of each data volume into one archive, and diff --git a/docsgpt/cli.py b/docsgpt/cli.py index e5a5af2a..1ca2a187 100644 --- a/docsgpt/cli.py +++ b/docsgpt/cli.py @@ -206,6 +206,10 @@ def _add_deploy_commands(commands) -> None: help="go back to the default image") up.set_defaults(docling=None) up.add_argument("--image-tag", help="image tag to run instead of this package's version, e.g. develop") + up.add_argument("--native", action="store_true", + help="run the API and worker as services on this machine instead of on Docker") + up.add_argument("--postgres-uri", help="native mode: the PostgreSQL DocsGPT should use") + up.add_argument("--redis-url", help="native mode: the Redis for the queue and the cache (default: localhost:6379)") up.add_argument("-y", "--yes", action="store_true", help="ask nothing: use the flags, then the defaults") up.add_argument("--reconfigure", action="store_true", help="ask the setup questions again") up.add_argument("--adopt", action="store_true", help="take over a DocsGPT stack started from another folder") diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index fb9c5e25..26c287fc 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -6,6 +6,7 @@ import getpass import json import os import re +import secrets import shutil import subprocess import sys @@ -18,7 +19,7 @@ from pathlib import Path from typing import Any, Optional from docsgpt.deploy import backup as backup_format -from docsgpt.deploy import envfile, stack +from docsgpt.deploy import envfile, native, stack from docsgpt.deploy.docker import DeployError, Docker, lan_ip, wait_healthy PROJECT = "docsgpt" @@ -78,9 +79,10 @@ def detect_installer() -> str: return "pip" -def _run_command(args: list[str]) -> int: +def _run_command(args: list[str], env: Optional[Mapping[str, str]] = None) -> int: + """Run ``args``; ``env`` adds to this process's environment rather than replacing it.""" try: - return subprocess.call(args) + return subprocess.call(args, env={**os.environ, **env} if env else None) except FileNotFoundError as exc: raise DeployError(f"{args[0]} is not on PATH") from exc @@ -106,8 +108,9 @@ class Context: wait: Callable[[str, float], bool] = wait_healthy open_browser: Callable[[str], Any] = webbrowser.open installer: Callable[[], str] = detect_installer - run: Callable[[list[str]], int] = _run_command + run: Callable[..., int] = _run_command exec_up: Callable[[list[str]], int] = _exec_command + services: Any = None @classmethod def default(cls, args) -> "Context": @@ -116,10 +119,33 @@ class Context: interactive = sys.stdin.isatty() and not getattr(args, "yes", False) return cls(docker=Docker(), prompter=Prompter(), interactive=interactive, version=__version__) + def service_manager(self): + """The launchd or systemd wrapper, made on first use so Docker installs never touch it.""" + if self.services is None: + self.services = native.services_for_platform() + return self.services + + +def _record(directory: Path) -> dict: + """What ``install.json`` says about this install (empty when there is none).""" + path = directory / stack.RECORD_FILE + if not path.is_file(): + return {} + try: + return json.loads(path.read_text(encoding="utf-8")) + except ValueError: + return {} + + +def _mode(directory: Path) -> str: + """``native`` or ``docker``, from the install record.""" + return "native" if _record(directory).get("mode") == "native" else "docker" + def _installed(directory: Path) -> Optional[dict[str, str]]: """The stack's settings, or None (with a message) when there is no install in ``directory``.""" - if not (directory / stack.COMPOSE_FILE).is_file(): + installed = (directory / stack.COMPOSE_FILE).is_file() or _mode(directory) == "native" + if not installed: print(f"No DocsGPT install in {directory}. Run `docsgpt up` first, or pass --dir.", file=sys.stderr) return None return envfile.read(directory / ".env") @@ -174,10 +200,110 @@ def _choose_provider(args, context: Context, existing: Mapping[str, str], ask: b raise DeployError(str(exc)) from exc +def _redis_urls(base: str) -> dict[str, str]: + """Celery's broker and result backend, and the cache, on three consecutive databases of one Redis. + + They start at database 0, or at the one the URL names: ``redis://host:6379/5`` puts them on 5, 6 + and 7, which is how one Redis is shared with something that already uses the first databases. + """ + trimmed = base.rstrip("/") + first = 0 + head, _, tail = trimmed.rpartition("/") + if tail.isdigit(): + trimmed, first = head, int(tail) + return { + "CELERY_BROKER_URL": f"{trimmed}/{first}", + "CELERY_RESULT_BACKEND": f"{trimmed}/{first + 1}", + "CACHE_REDIS_URL": f"{trimmed}/{first + 2}", + } + + +def _native_up(args, context: Context, directory: Path) -> int: + """Run the API and the worker as services on this machine, against an existing Postgres and Redis.""" + services = context.service_manager() + env_path = directory / ".env" + existing = envfile.read(env_path) + record = _record(directory) + configured = bool(record) + + postgres = args.postgres_uri or existing.get("POSTGRES_URI") + redis = args.redis_url or "" + ask = context.interactive and (not configured or args.reconfigure) + if not postgres and ask: + postgres = context.prompter.text("PostgreSQL URL (postgresql://user:password@host:5432/docsgpt)") + if not redis and ask: + redis = context.prompter.text("Redis URL", default="redis://localhost:6379") + if not postgres: + raise DeployError( + "native mode needs a database: pass --postgres-uri postgresql://user:password@host:5432/docsgpt " + "(Redis defaults to redis://localhost:6379)." + ) + + # 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) + updates: dict[str, Optional[str]] = { + "POSTGRES_URI": postgres, + "API_URL": f"http://127.0.0.1:{port}", + "DOCSGPT_PORT": str(port), + } + if redis or "CELERY_BROKER_URL" not in existing: + updates.update(_redis_urls(redis or "redis://localhost:6379")) + for key in ("INTERNAL_KEY", "JWT_SECRET_KEY"): + if not existing.get(key): + updates[key] = secrets.token_hex(32) + if "VITE_API_STREAMING" not in existing: + updates["VITE_API_STREAMING"] = "true" + provider = _choose_provider(args, context, existing, ask) + if provider: + updates.update(provider) + elif "LLM_PROVIDER" not in existing: + updates.update(stack.provider_settings("docsgpt")) + + directory.mkdir(parents=True, exist_ok=True) + (directory / "logs").mkdir(exist_ok=True) + envfile.update(env_path, updates) + + executable = shutil.which("docsgpt") or sys.argv[0] + print("Applying database migrations ...") + # The child reads the stack's settings, not a .env in whatever directory this was run from. + stack_env = {"DOCSGPT_HOME": str(directory), "DOCSGPT_ENV_FILE": str(env_path)} + if context.run([executable, "migrate"], stack_env) != 0: + raise DeployError("`docsgpt migrate` failed; check the database URL and that the server is reachable.") + + for unit in native.units_for(directory, executable, 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): + print( + f"DocsGPT did not answer at {health} within {args.timeout} seconds. " + "See what happened with `docsgpt logs`.", + file=sys.stderr, + ) + return 1 + + print(f"\nDocsGPT is running at http://localhost:{port}") + print(f"Services: {', '.join(native.SERVICES)} under {services.name}") + print(f"Settings: {env_path}") + print("Manage it with: docsgpt status | logs | down | uninstall") + return 0 + + def up(args, context: Optional[Context] = None) -> int: """Install or update the stack in its directory and start it.""" context = context or Context.default(args) directory = stack.stack_dir(args.dir) + if getattr(args, "native", False) or _mode(directory) == "native": + return _native_up(args, context, directory) env_path = directory / ".env" record_path = directory / stack.RECORD_FILE existing = envfile.read(env_path) @@ -269,6 +395,12 @@ def down(args, context: Optional[Context] = None) -> int: directory = stack.stack_dir(args.dir) if _installed(directory) is None: return 1 + 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): + services.stop(name) + return 0 context.docker.compose(directory, *_EVERY_PROFILE, "down") return 0 @@ -280,6 +412,16 @@ def status(args, context: Optional[Context] = None) -> int: env = _installed(directory) if env is None: return 1 + if _mode(directory) == "native": + services = context.service_manager() + print(f"DocsGPT {_record(directory).get('version', 'unknown')} in {directory} (native, {services.name})") + print(f"Address: {stack.url(env, context.lan_ip())}") + for name in native.SERVICES: + print(f" {name}: {'running' if services.is_running(name) else 'stopped'}") + healthy = context.wait(stack.health_url(env), 0) + print("API: answering" if healthy else "API: not answering (see `docsgpt logs`)") + return 0 if healthy else 1 + tag = env.get("DOCSGPT_IMAGE_TAG", "unknown") + env.get("DOCSGPT_IMAGE_VARIANT", "") print(f"DocsGPT {tag} in {directory}") print(f"Address: {stack.url(env, context.lan_ip())} ({stack.exposure(env)})") @@ -295,6 +437,20 @@ def logs(args, context: Optional[Context] = None) -> int: directory = stack.stack_dir(args.dir) if _installed(directory) is None: return 1 + if _mode(directory) == "native": + logs_dir = directory / "logs" + wanted = args.services or [name.removeprefix("docsgpt-") for name in native.SERVICES] + for service in wanted: + path = logs_dir / f"{service}.log" + print(f"=== {path}") + if path.is_file(): + lines = path.read_text(encoding="utf-8", errors="replace").splitlines() + print("\n".join(lines[-args.tail:] if args.tail else lines)) + else: + print("(nothing logged yet)") + if args.follow: + print("Following is not supported in native mode; use `tail -f` on the files above.", file=sys.stderr) + return 0 options = (["--follow"] if args.follow else []) + (["--tail", str(args.tail)] if args.tail else []) return context.docker.compose(directory, "logs", *options, *args.services, check=False).returncode @@ -499,6 +655,20 @@ def uninstall(args, context: Optional[Context] = None) -> int: if not context.prompter.confirm(f"Remove the DocsGPT {what} in {directory}?", default=False): print("Nothing removed.") return 1 + if _mode(directory) == "native": + services = context.service_manager() + for name in reversed(native.SERVICES): + services.remove(name) + print(f"Removed the {', '.join(native.SERVICES)} services.") + if args.purge: + shutil.rmtree(directory) + print(f"Removed {directory}. The database and Redis it used are untouched.") + else: + (directory / stack.RECORD_FILE).unlink(missing_ok=True) + print(f"Settings stay in {directory / '.env'}; the database and Redis are untouched.") + print(f"To remove the docsgpt command too: {_uninstall_hint(context.installer())}") + return 0 + context.docker.compose(directory, *_EVERY_PROFILE, "down", "--remove-orphans", *(["-v"] if args.purge else [])) if args.purge: shutil.rmtree(directory) diff --git a/docsgpt/deploy/native.py b/docsgpt/deploy/native.py new file mode 100644 index 00000000..0ef0cf19 --- /dev/null +++ b/docsgpt/deploy/native.py @@ -0,0 +1,218 @@ +"""Running DocsGPT without Docker, supervised by the system's own service manager. + +The API and the worker each become one service: a launchd agent on macOS, a +systemd user unit on Linux. Both run the ``docsgpt`` command of this +installation with the stack directory as their data home, so a native install +keeps its settings in the same ``.env`` a Docker install would. + +Postgres and Redis are not started here; a native install points at ones that +already run (``--postgres-uri``, ``--redis-url``). +""" + +from __future__ import annotations + +import os +import plistlib +import shlex +import subprocess +import sys +import time +from dataclasses import dataclass +from pathlib import Path +from typing import Optional + +from docsgpt.deploy.docker import DeployError + +API_SERVICE = "docsgpt-api" +WORKER_SERVICE = "docsgpt-worker" +SERVICES = (API_SERVICE, WORKER_SERVICE) +LABEL_PREFIX = "cloud.docsgpt" + + +@dataclass +class Unit: + """One supervised process, in the terms every service manager needs.""" + + name: str + arguments: list[str] + environment: dict[str, str] + working_directory: str + log_file: str + + +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-')}" + + +def launchd_plist(unit: Unit, label: Optional[str] = None) -> str: + """The launchd agent for ``unit``: kept alive, with its output in the stack's log file.""" + body = { + "Label": label or label_for(unit.name), + "ProgramArguments": list(unit.arguments), + "EnvironmentVariables": dict(unit.environment), + "WorkingDirectory": unit.working_directory, + "StandardOutPath": unit.log_file, + "StandardErrorPath": unit.log_file, + "KeepAlive": True, + "RunAtLoad": True, + "ProcessType": "Background", + } + return plistlib.dumps(body).decode("utf-8") + + +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())) + command = " ".join(shlex.quote(argument) for argument in unit.arguments) + return f"""[Unit] +Description=DocsGPT ({unit.name}) +After=network-online.target + +[Service] +Type=simple +ExecStart={command} +WorkingDirectory={unit.working_directory} +{environment} +Restart=always +RestartSec=5 +StandardOutput=append:{unit.log_file} +StandardError=append:{unit.log_file} + +[Install] +WantedBy=default.target +""" + + +class LaunchdServices: + """launchd user agents in ~/Library/LaunchAgents (macOS).""" + + name = "launchd" + poll_interval = 0.2 + unload_timeout = 15.0 + + def __init__(self, runner=subprocess.run, home: Optional[Path] = None) -> None: + self._run = runner + self.directory = (home or Path.home()) / "Library" / "LaunchAgents" + + def _plist(self, name: str) -> Path: + return self.directory / f"{label_for(name)}.plist" + + def _target(self, name: str) -> str: + return f"gui/{os.getuid()}/{label_for(name)}" + + def _print(self, name: str): + return self._run(["launchctl", "print", self._target(name)], capture_output=True, text=True, check=False) + + def _bootout(self, name: str) -> None: + """Unload the job and wait for it to go. + + ``bootout`` returns before launchd has finished unloading, and bootstrapping a label that is + still on its way out fails with "Bootstrap failed: 5: Input/output error" — which is what a + restart used to hit. Waiting for the label to stop resolving makes stop and start ordered. + """ + self._run(["launchctl", "bootout", self._target(name)], capture_output=True, text=True, check=False) + deadline = time.monotonic() + self.unload_timeout + while self._print(name).returncode == 0: + if time.monotonic() >= deadline: + raise DeployError(f"{label_for(name)} is still loaded after {self.unload_timeout:.0f}s") + time.sleep(self.poll_interval) + + def install(self, unit: Unit) -> None: + self.directory.mkdir(parents=True, exist_ok=True) + self._plist(unit.name).write_text(launchd_plist(unit), encoding="utf-8") + + def start(self, name: str) -> None: + # bootout first: a reinstall must pick up the new plist rather than the loaded one. + self._bootout(name) + result = self._run( + ["launchctl", "bootstrap", f"gui/{os.getuid()}", str(self._plist(name))], + capture_output=True, text=True, check=False, + ) + if result.returncode != 0: + raise DeployError(f"launchctl could not start {name}: {(result.stderr or '').strip()}") + + def stop(self, name: str) -> None: + self._bootout(name) + + def remove(self, name: str) -> None: + self.stop(name) + self._plist(name).unlink(missing_ok=True) + + def is_running(self, name: str) -> bool: + """A loaded job is not a running one: a crashed service still answers ``launchctl print``.""" + result = self._print(name) + return result.returncode == 0 and "state = running" in (result.stdout or "") + + +class SystemdServices: + """systemd user units in ~/.config/systemd/user (Linux).""" + + name = "systemd" + + def __init__(self, runner=subprocess.run, home: Optional[Path] = None) -> None: + self._run = runner + base = os.environ.get("XDG_CONFIG_HOME") + root = Path(base) if base else (home or Path.home()) / ".config" + self.directory = root / "systemd" / "user" + + def _unit_file(self, name: str) -> Path: + return self.directory / f"{name}.service" + + def _systemctl(self, *args: str, check: bool = False): + result = self._run(["systemctl", "--user", *args], capture_output=True, text=True, check=False) + if check and result.returncode != 0: + raise DeployError(f"systemctl --user {' '.join(args)} failed: {(result.stderr or '').strip()}") + return result + + def install(self, unit: Unit) -> None: + self.directory.mkdir(parents=True, exist_ok=True) + self._unit_file(unit.name).write_text(systemd_unit(unit), encoding="utf-8") + self._systemctl("daemon-reload") + + def start(self, name: str) -> None: + self._systemctl("enable", "--now", f"{name}.service", check=True) + + def stop(self, name: str) -> None: + self._systemctl("stop", f"{name}.service") + + def remove(self, name: str) -> None: + self._systemctl("disable", "--now", f"{name}.service") + self._unit_file(name).unlink(missing_ok=True) + self._systemctl("daemon-reload") + + def is_running(self, name: str) -> bool: + return self._systemctl("is-active", "--quiet", f"{name}.service").returncode == 0 + + +def services_for_platform(platform: str = sys.platform): + """The service manager for this machine, or a DeployError saying what to do instead.""" + if platform == "darwin": + return LaunchdServices() + if platform.startswith("linux"): + return SystemdServices() + raise DeployError( + "native mode supervises services with launchd or systemd, which Windows does not have. " + "Run DocsGPT on Docker with `docsgpt up`, or start `docsgpt api` and `docsgpt worker` yourself." + ) + + +def units_for(stack_directory: Path, executable: str, port: int, home: Path) -> list[Unit]: + """The API and worker services for a native install in ``stack_directory``.""" + environment = {"DOCSGPT_HOME": str(home)} + return [ + Unit( + name=API_SERVICE, + arguments=[executable, "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, + arguments=[executable, "worker"], + environment=dict(environment), + working_directory=str(stack_directory), + log_file=str(stack_directory / "logs" / "worker.log"), + ), + ] diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py new file mode 100644 index 00000000..63646d98 --- /dev/null +++ b/tests/deploy/test_native.py @@ -0,0 +1,284 @@ +"""`docsgpt up --native`: run the server without Docker, supervised by the system's service manager.""" + +import json +import plistlib +import types + +import pytest + +from docsgpt.deploy import envfile, native +from docsgpt.deploy.docker import DeployError + +from .test_commands import FakePrompter, _context, _run + + +class FakeServices: + """A stand-in for launchd/systemd: records what would be installed and started.""" + + name = "fake" + + def __init__(self, running=()): + self.units = {} + self.started = [] + self.stopped = [] + self.removed = [] + self.running = set(running) + + def install(self, unit): + self.units[unit.name] = unit + + def start(self, name): + self.started.append(name) + self.running.add(name) + + def stop(self, name): + self.stopped.append(name) + self.running.discard(name) + + def remove(self, name): + self.removed.append(name) + self.running.discard(name) + + def is_running(self, name): + return name in self.running + + +def _native_context(services=None, migrated=None, **overrides): + context = _context(**overrides) + context.services = services or FakeServices() + context.run = lambda args, env=None: (migrated.append(args) if migrated is not None else None) or 0 + return context + + +class TestNativeUp: + def test_writes_settings_for_the_given_postgres_and_redis(self, tmp_path): + context = _native_context() + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", + "--postgres-uri", "postgresql://docsgpt:pw@localhost:5432/docsgpt", + "--redis-url", "redis://localhost:6379"] + assert _run(argv, context) == 0 + + env = envfile.read(tmp_path / ".env") + assert env["POSTGRES_URI"] == "postgresql://docsgpt:pw@localhost:5432/docsgpt" + assert env["CELERY_BROKER_URL"] == "redis://localhost:6379/0" + assert env["CELERY_RESULT_BACKEND"] == "redis://localhost:6379/1" + assert env["CACHE_REDIS_URL"] == "redis://localhost:6379/2" + assert env["INTERNAL_KEY"], "the worker needs it to hand indexes to the API" + assert env["API_URL"] == "http://127.0.0.1:7091" + assert "DOCSGPT_IMAGE_TAG" not in env, "nothing here runs an image" + + def test_records_the_mode_so_the_other_commands_can_tell(self, tmp_path): + assert _run(["up", "--native", "--dir", str(tmp_path), "--yes", + "--postgres-uri", "postgresql://localhost/docsgpt"], _native_context()) == 0 + record = json.loads((tmp_path / "install.json").read_text()) + assert record["mode"] == "native" + assert record["version"] == "0.21.0" + + def test_migrates_before_starting_the_services(self, tmp_path): + migrated = [] + services = FakeServices() + context = _native_context(services=services, migrated=migrated) + 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"] + + 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"] + assert api.arguments[1:] == ["api", "--host", "127.0.0.1", "--port", "7091"] + assert worker.arguments[1:] == ["worker"] + for unit in (api, worker): + assert unit.environment["DOCSGPT_HOME"] == str(tmp_path) + assert unit.working_directory == str(tmp_path) + assert unit.log_file.endswith(".log") + + def test_the_migration_targets_the_stack_not_the_working_directory(self, tmp_path): + """`docsgpt up --native` run from a checkout must not migrate the checkout's database.""" + calls = [] + context = _native_context() + context.run = lambda args, env=None: calls.append((args, env)) or 0 + assert _run(["up", "--native", "--dir", str(tmp_path), "--yes", + "--postgres-uri", "postgresql://localhost/docsgpt"], context) == 0 + arguments, environment = calls[0] + assert arguments[1] == "migrate" + assert environment["DOCSGPT_HOME"] == str(tmp_path) + assert environment["DOCSGPT_ENV_FILE"] == str(tmp_path / ".env") + + def test_a_redis_database_in_the_url_moves_the_three_up(self, tmp_path): + """Sharing a Redis: `/5` puts the broker, the backend and the cache on 5, 6 and 7.""" + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", + "--postgres-uri", "postgresql://localhost/docsgpt", "--redis-url", "redis://localhost:6379/5"] + assert _run(argv, _native_context()) == 0 + env = envfile.read(tmp_path / ".env") + assert env["CELERY_BROKER_URL"] == "redis://localhost:6379/5" + assert env["CELERY_RESULT_BACKEND"] == "redis://localhost:6379/6" + assert env["CACHE_REDIS_URL"] == "redis://localhost:6379/7" + + def test_a_later_up_keeps_the_port_the_install_was_given(self, tmp_path): + """`docsgpt up` with no --port must not move an install from its port back to 7091.""" + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--port", "7099", + "--postgres-uri", "postgresql://localhost/docsgpt"] + assert _run(argv, _native_context()) == 0 + + services = FakeServices() + assert _run(["up", "--dir", str(tmp_path), "--yes"], _native_context(services)) == 0 + 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" + + 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()) + + def test_asks_for_the_urls_when_there_is_a_terminal(self, tmp_path): + prompter = FakePrompter(["postgresql://localhost/docsgpt", "redis://localhost:6379", "docsgpt"]) + context = _native_context(interactive=True, prompter=prompter) + assert _run(["up", "--native", "--dir", str(tmp_path)], context) == 0 + env = envfile.read(tmp_path / ".env") + assert env["POSTGRES_URI"] == "postgresql://localhost/docsgpt" + assert env["CELERY_BROKER_URL"] == "redis://localhost:6379/0" + assert env["LLM_PROVIDER"] == "docsgpt" + assert prompter.questions[0].startswith("PostgreSQL URL") + + def test_an_unhealthy_start_points_at_the_logs(self, tmp_path, capsys): + context = _native_context(healthy=False) + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/docsgpt"] + assert _run(argv, context) == 1 + assert "docsgpt logs" in capsys.readouterr().err + + +class TestNativeLifecycle: + def _installed(self, tmp_path, services): + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/docsgpt"] + assert _run(argv, _native_context(services)) == 0 + + def test_status_reports_the_native_services(self, tmp_path, capsys): + services = FakeServices() + self._installed(tmp_path, services) + assert _run(["status", "--dir", str(tmp_path)], _native_context(services)) == 0 + out = capsys.readouterr().out + assert "native" in out.lower() + assert "docsgpt-api" in out + + def test_down_stops_them_and_leaves_the_settings(self, tmp_path): + 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 (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"] + + def test_docker_commands_refuse_a_native_install(self, tmp_path, capsys): + self._installed(tmp_path, FakeServices()) + assert _run(["logs", "--dir", str(tmp_path)], _native_context()) == 0, "logs work in both modes" + + +class FakeLaunchctl: + """launchctl, with a bootout that takes `unload_polls` polls to take effect.""" + + def __init__(self, unload_polls=1, state="running"): + self.calls = [] + self.loaded = True + self.unload_polls = unload_polls + self.state = state + + def __call__(self, args, capture_output=False, text=False, check=False): + self.calls.append(args) + verb = args[1] + if verb == "bootout": + self.pending = self.unload_polls + return types.SimpleNamespace(returncode=0, stdout="", stderr="") + if verb == "print": + if not self.loaded: + return types.SimpleNamespace(returncode=113, stdout="", stderr="Could not find service") + if getattr(self, "pending", 0) > 0: + self.pending -= 1 + if self.pending == 0: + self.loaded = False + return types.SimpleNamespace(returncode=0, stdout="\tstate = running\n", stderr="") + return types.SimpleNamespace(returncode=0, stdout=f"\tstate = {self.state}\n", stderr="") + if verb == "bootstrap": + if self.loaded: + return types.SimpleNamespace(returncode=5, stdout="", stderr="Bootstrap failed: 5: Input/output error") + self.loaded = True + return types.SimpleNamespace(returncode=0, stdout="", stderr="") + return types.SimpleNamespace(returncode=0, stdout="", stderr="") + + +class TestLaunchd: + def _services(self, launchctl, tmp_path): + services = native.LaunchdServices(runner=launchctl, home=tmp_path) + services.poll_interval = 0 + return services + + def test_start_waits_for_the_old_job_to_unload_before_bootstrapping(self, tmp_path): + """launchctl bootout returns early; bootstrapping too soon fails with 'Bootstrap failed: 5'.""" + launchctl = FakeLaunchctl(unload_polls=3) + services = self._services(launchctl, tmp_path) + services.directory.mkdir(parents=True, exist_ok=True) + services.start("docsgpt-api") + verbs = [call[1] for call in launchctl.calls] + assert verbs[0] == "bootout" + assert verbs[-1] == "bootstrap" + assert verbs.count("print") >= 1, "it polled until the label stopped resolving" + assert verbs.index("bootstrap") > max(index for index, verb in enumerate(verbs) if verb == "print") + + def test_stop_returns_only_once_the_job_is_gone(self, tmp_path): + launchctl = FakeLaunchctl(unload_polls=2) + self._services(launchctl, tmp_path).stop("docsgpt-api") + assert not launchctl.loaded + + def test_a_job_that_will_not_unload_is_reported(self, tmp_path): + launchctl = FakeLaunchctl(unload_polls=10_000) + services = self._services(launchctl, tmp_path) + services.unload_timeout = 0 + with pytest.raises(DeployError, match="still loaded"): + services.stop("docsgpt-api") + + def test_a_loaded_but_not_running_job_is_not_running(self, tmp_path): + """`launchctl print` answers for a crashed service too, so status must read its state.""" + services = self._services(FakeLaunchctl(state="not running"), tmp_path) + assert services.is_running("docsgpt-api") is False + assert self._services(FakeLaunchctl(state="running"), tmp_path).is_running("docsgpt-api") is True + + +class TestUnitFiles: + def _unit(self, tmp_path): + return native.Unit( + name="docsgpt-api", + arguments=["/opt/venv/bin/docsgpt", "api", "--port", "7091"], + environment={"DOCSGPT_HOME": str(tmp_path)}, + working_directory=str(tmp_path), + log_file=str(tmp_path / "api.log"), + ) + + def test_launchd_plist_is_valid_and_keeps_the_service_alive(self, tmp_path): + body = native.launchd_plist(self._unit(tmp_path), label="cloud.docsgpt.api") + parsed = plistlib.loads(body.encode()) + assert parsed["Label"] == "cloud.docsgpt.api" + assert parsed["ProgramArguments"][1] == "api" + assert parsed["KeepAlive"] is True + assert parsed["EnvironmentVariables"]["DOCSGPT_HOME"] == str(tmp_path) + assert parsed["StandardErrorPath"].endswith("api.log") + + def test_systemd_unit_restarts_and_carries_the_environment(self, tmp_path): + body = native.systemd_unit(self._unit(tmp_path)) + assert "Restart=always" in body + assert f'Environment="DOCSGPT_HOME={tmp_path}"' in body + assert "ExecStart=/opt/venv/bin/docsgpt api --port 7091" in body + assert "WantedBy=default.target" 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"] + assert "ExecStart='/opt/my venv/bin/docsgpt' api" in native.systemd_unit(unit) From f9f52e99f0f170c670735e65c570f75f9e7b2c31 Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 16 Sep 2026 22:35:14 +0100 Subject: [PATCH 02/17] fix: make a native install take effect on systemd and stay out of Docker's way From review of #2800: - systemd `enable --now` starts nothing when the unit is already active, so a second `up --native` kept the old ExecStart and left the API on its previous port. start enables and then restarts, as the launchd path already did by booting the job out first. - An explicit `home` now wins over XDG_CONFIG_HOME, which is what callers pass it for. - The ExecStart program must be a real executable: when `docsgpt` is not on PATH, sys.argv[0] is accepted only if it can be run, and otherwise the failure is raised before any unit is written. - `up --native` over a directory holding a Docker install now refuses and says how to proceed, instead of starting native services beside containers that down, status and uninstall would no longer see. - Docs: without a terminal only --postgres-uri is required, and the Windows fallback names `docsgpt beat`, which the worker cannot embed there. SystemdServices was the least covered part of the module and cannot be run on this machine, so it now has tests for install, start, stop, remove, is_running and a failing systemctl. --- docs/content/Deploying/Pip-Install.mdx | 4 +- docsgpt/deploy/commands.py | 20 +++++- docsgpt/deploy/native.py | 13 +++- tests/deploy/test_native.py | 99 ++++++++++++++++++++++++++ 4 files changed, 130 insertions(+), 6 deletions(-) diff --git a/docs/content/Deploying/Pip-Install.mdx b/docs/content/Deploying/Pip-Install.mdx index 04574490..64ca8898 100644 --- a/docs/content/Deploying/Pip-Install.mdx +++ b/docs/content/Deploying/Pip-Install.mdx @@ -109,7 +109,7 @@ docsgpt up --native \ --redis-url redis://localhost:6379 ``` -Without a terminal both flags are required; with one, it asks for them and for the model provider. It writes the same `.env` a Docker install uses (minus the image settings), generates `INTERNAL_KEY` and `JWT_SECRET_KEY` on the first run, applies the migrations, then starts `docsgpt-api` and `docsgpt-worker` and waits for the API to answer. +Without a terminal only `--postgres-uri` is required, since Redis defaults to `redis://localhost:6379`; with one, it asks for both and for the model provider. It writes the same `.env` a Docker install uses (minus the image settings), generates `INTERNAL_KEY` and `JWT_SECRET_KEY` on the first run, applies the migrations, then starts `docsgpt-api` and `docsgpt-worker` and waits for the API to answer. One Redis URL covers all three uses: the Celery broker, its result backend and the cache go on databases 0, 1 and 2 of it. Name a database in the URL and the three start there instead, so `redis://localhost:6379/5` puts them on 5, 6 and 7 — that is how you share a Redis that already holds something else. @@ -125,7 +125,7 @@ The same commands manage it: `uninstall` never touches the database or Redis: they were yours to begin with. - Windows has neither launchd nor systemd, so native mode is macOS and Linux only. On Windows, run DocsGPT on Docker with `docsgpt up`, or start `docsgpt api` and `docsgpt worker` yourself. + Windows has neither launchd nor systemd, so native mode is macOS and Linux only. On Windows, run DocsGPT on Docker with `docsgpt up`, or start `docsgpt api`, `docsgpt worker` and `docsgpt beat` yourself — the worker cannot run the scheduler in-process there, so `docsgpt beat` has to run alongside it for scheduled tasks to fire. ## Upgrade diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 9bd2db78..24504a7d 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -218,6 +218,17 @@ def _redis_urls(base: str) -> dict[str, str]: } +def _own_executable() -> str: + """The docsgpt command to put in the service files, or a DeployError naming what to install.""" + candidate = Path(sys.argv[0]).resolve() + if candidate.is_file() and os.access(candidate, os.X_OK): + return str(candidate) + raise DeployError( + "the docsgpt command is not on PATH, so the services would have nothing to run. " + "Install it with `uv tool install docsgpt` (or `pip install docsgpt`) and run this again." + ) + + def _native_up(args, context: Context, directory: Path) -> int: """Run the API and the worker as services on this machine, against an existing Postgres and Redis.""" services = context.service_manager() @@ -263,7 +274,7 @@ def _native_up(args, context: Context, directory: Path) -> int: (directory / "logs").mkdir(exist_ok=True) envfile.update(env_path, updates) - executable = shutil.which("docsgpt") or sys.argv[0] + executable = shutil.which("docsgpt") or _own_executable() print("Applying database migrations ...") # The child reads the stack's settings, not a .env in whatever directory this was run from. stack_env = {"DOCSGPT_HOME": str(directory), "DOCSGPT_ENV_FILE": str(env_path)} @@ -303,6 +314,13 @@ def up(args, context: Optional[Context] = None) -> int: context = context or Context.default(args) directory = stack.stack_dir(args.dir) if getattr(args, "native", False) or _mode(directory) == "native": + if _mode(directory) != "native" and (directory / stack.COMPOSE_FILE).is_file(): + raise DeployError( + f"{directory} holds a Docker install. Native services would run beside its containers " + "on the same port, and down, status and uninstall would stop seeing them. Stop it " + "first with `docsgpt down` (and `docsgpt uninstall` to remove it), or pass --dir to " + "put the native install somewhere else." + ) return _native_up(args, context, directory) env_path = directory / ".env" record_path = directory / stack.RECORD_FILE diff --git a/docsgpt/deploy/native.py b/docsgpt/deploy/native.py index 0ef0cf19..2c6376f2 100644 --- a/docsgpt/deploy/native.py +++ b/docsgpt/deploy/native.py @@ -152,8 +152,12 @@ class SystemdServices: def __init__(self, runner=subprocess.run, home: Optional[Path] = None) -> None: self._run = runner - base = os.environ.get("XDG_CONFIG_HOME") - root = Path(base) if base else (home or Path.home()) / ".config" + # An explicit home wins: it is what a caller passes to redirect the unit directory. + if home is not None: + root = home / ".config" + else: + base = os.environ.get("XDG_CONFIG_HOME") + root = Path(base) if base else Path.home() / ".config" self.directory = root / "systemd" / "user" def _unit_file(self, name: str) -> Path: @@ -171,7 +175,10 @@ class SystemdServices: self._systemctl("daemon-reload") def start(self, name: str) -> None: - self._systemctl("enable", "--now", f"{name}.service", check=True) + # restart, not `enable --now`: --now starts nothing when the unit is already active, so a + # reinstalled unit would keep running with the ExecStart and environment it started with. + self._systemctl("enable", f"{name}.service", check=True) + self._systemctl("restart", f"{name}.service", check=True) def stop(self, name: str) -> None: self._systemctl("stop", f"{name}.service") diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index 63646d98..630e0097 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -2,6 +2,7 @@ import json import plistlib +import sys import types import pytest @@ -131,6 +132,21 @@ class TestNativeUp: assert env["API_URL"] == "http://127.0.0.1:7099" assert services.units["docsgpt-api"].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.""" + assert _run(["up", "--yes", "--dir", str(tmp_path)], _context()) == 0 + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d"] + with pytest.raises(DeployError, match="holds a Docker install"): + _run(argv, _native_context()) + + def test_services_are_not_written_without_a_docsgpt_command(self, tmp_path, monkeypatch): + """A unit pointing at something unexecutable would fail to exec with only a health check to show it.""" + monkeypatch.setattr("docsgpt.deploy.commands.shutil.which", lambda name: None) + monkeypatch.setattr(sys, "argv", [str(tmp_path / "not-a-program")]) + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d"] + with pytest.raises(DeployError, match="not on PATH"): + _run(argv, _native_context()) + 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()) @@ -252,6 +268,89 @@ class TestLaunchd: assert self._services(FakeLaunchctl(state="running"), tmp_path).is_running("docsgpt-api") is True +class FakeSystemctl: + """systemctl, recording what it was asked to do.""" + + def __init__(self, active=True): + self.calls = [] + self.active = active + + def __call__(self, args, capture_output=False, text=False, check=False): + self.calls.append(args) + code = 0 if (args[2] != "is-active" or self.active) else 3 + return types.SimpleNamespace(returncode=code, stdout="", stderr="") + + @property + def verbs(self): + return [call[2] for call in self.calls] + + +class TestSystemd: + def _services(self, systemctl, tmp_path): + return native.SystemdServices(runner=systemctl, home=tmp_path) + + 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]) + unit = tmp_path / ".config" / "systemd" / "user" / "docsgpt-api.service" + assert "ExecStart=/venv/bin/docsgpt api" in unit.read_text() + assert systemctl.verbs == ["daemon-reload"] + + def test_start_restarts_so_a_rewritten_unit_takes_effect(self, tmp_path): + """`enable --now` starts nothing when the unit is already active: it would keep the old ExecStart.""" + systemctl = FakeSystemctl() + self._services(systemctl, tmp_path).start("docsgpt-api") + assert "restart" in systemctl.verbs, systemctl.verbs + assert systemctl.verbs.index("enable") < systemctl.verbs.index("restart") + assert "--now" not in [argument for call in systemctl.calls for argument in call] + + 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]) + unit = tmp_path / ".config" / "systemd" / "user" / "docsgpt-api.service" + services.stop("docsgpt-api") + assert unit.is_file(), "stopping keeps the unit" + services.remove("docsgpt-api") + assert not unit.exists() + assert systemctl.verbs[-1] == "daemon-reload", "systemd is told the unit is gone" + + def test_is_running_asks_systemd(self, tmp_path): + 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_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")) + services = native.SystemdServices(runner=FakeSystemctl(), home=tmp_path) + assert services.directory == tmp_path / ".config" / "systemd" / "user" + + def test_xdg_config_home_is_used_when_no_home_is_given(self, tmp_path, monkeypatch): + monkeypatch.setenv("XDG_CONFIG_HOME", str(tmp_path / "xdg")) + services = native.SystemdServices(runner=FakeSystemctl()) + assert services.directory == tmp_path / "xdg" / "systemd" / "user" + + def test_a_failing_systemctl_is_reported(self, tmp_path): + class Failing(FakeSystemctl): + def __call__(self, args, **kwargs): + self.calls.append(args) + return types.SimpleNamespace(returncode=1, stdout="", stderr="Failed to enable unit") + + with pytest.raises(DeployError, match="Failed to enable unit"): + self._services(Failing(), tmp_path).start("docsgpt-api") + + +class TestPlatformChoice: + def test_each_platform_gets_its_service_manager(self): + assert isinstance(native.services_for_platform("darwin"), native.LaunchdServices) + assert isinstance(native.services_for_platform("linux"), native.SystemdServices) + + def test_windows_says_what_to_do_instead(self): + with pytest.raises(DeployError, match="Windows"): + native.services_for_platform("win32") + + class TestUnitFiles: def _unit(self, tmp_path): return native.Unit( From a96734da07f6792cffc0c7312b89b94a9e8e9c3f Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 16 Sep 2026 22:45:54 +0100 Subject: [PATCH 03/17] fix: run the module when the docsgpt command is not on PATH Refusing to write the service units when `docsgpt` is not on PATH was wrong. A package installed in a virtualenv is runnable whether or not its console script is on PATH, and CI runs pytest as `python -m pytest`, where argv[0] is a module file: the refusal failed thirteen native tests there. The launcher now prefers the command on PATH, resolved to an absolute path since PATH can hold relative entries, then an argv[0] that can be executed, and otherwise this interpreter with `-m docsgpt`, which works wherever the package is importable. `python -m docsgpt` became an entrypoint of its own and has a test that runs it. --- docsgpt/__main__.py | 8 ++++++++ docsgpt/deploy/commands.py | 25 ++++++++++++++++++------- docsgpt/deploy/native.py | 11 +++++++---- tests/deploy/test_native.py | 19 ++++++++++--------- tests/test_cli.py | 10 ++++++++++ 5 files changed, 53 insertions(+), 20 deletions(-) create mode 100644 docsgpt/__main__.py diff --git a/docsgpt/__main__.py b/docsgpt/__main__.py new file mode 100644 index 00000000..397614de --- /dev/null +++ b/docsgpt/__main__.py @@ -0,0 +1,8 @@ +"""``python -m docsgpt`` runs what the ``docsgpt`` command runs.""" + +import sys + +from docsgpt.cli import main + +if __name__ == "__main__": + sys.exit(main()) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 24504a7d..0988044d 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -218,13 +218,24 @@ def _redis_urls(base: str) -> dict[str, str]: } -def _own_executable() -> str: - """The docsgpt command to put in the service files, or a DeployError naming what to install.""" +def _native_launcher() -> list[str]: + """How the service files start DocsGPT: the command on PATH, or this interpreter and the module. + + A unit has to name something that can be executed, and ``sys.argv[0]`` often cannot be: under + ``python -m docsgpt``, or pytest, it is a module file. Falling back to the running interpreter + works wherever the package is importable, which it must be to have got here. + """ + found = shutil.which("docsgpt") + if found: + # Absolute: PATH can hold relative entries, and a unit file needs a program it can exec. + return [str(Path(found).resolve())] candidate = Path(sys.argv[0]).resolve() if candidate.is_file() and os.access(candidate, os.X_OK): - return str(candidate) + return [str(candidate)] + if sys.executable: + return [sys.executable, "-m", "docsgpt"] raise DeployError( - "the docsgpt command is not on PATH, so the services would have nothing to run. " + "could not work out how to start docsgpt for the services. " "Install it with `uv tool install docsgpt` (or `pip install docsgpt`) and run this again." ) @@ -274,14 +285,14 @@ def _native_up(args, context: Context, directory: Path) -> int: (directory / "logs").mkdir(exist_ok=True) envfile.update(env_path, updates) - executable = shutil.which("docsgpt") or _own_executable() + launcher = _native_launcher() print("Applying database migrations ...") # The child reads the stack's settings, not a .env in whatever directory this was run from. stack_env = {"DOCSGPT_HOME": str(directory), "DOCSGPT_ENV_FILE": str(env_path)} - if context.run([executable, "migrate"], stack_env) != 0: + if context.run([*launcher, "migrate"], stack_env) != 0: raise DeployError("`docsgpt migrate` failed; check the database URL and that the server is reachable.") - for unit in native.units_for(directory, executable, port, directory): + for unit in native.units_for(directory, launcher, port, directory): services.install(unit) for name in native.SERVICES: print(f"Starting {name} ...") diff --git a/docsgpt/deploy/native.py b/docsgpt/deploy/native.py index 2c6376f2..8191a33f 100644 --- a/docsgpt/deploy/native.py +++ b/docsgpt/deploy/native.py @@ -204,20 +204,23 @@ def services_for_platform(platform: str = sys.platform): ) -def units_for(stack_directory: Path, executable: str, port: int, home: Path) -> list[Unit]: - """The API and worker services for a native install in ``stack_directory``.""" +def units_for(stack_directory: Path, launcher: list[str], port: int, home: Path) -> 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)} return [ Unit( name=API_SERVICE, - arguments=[executable, "api", "--host", "127.0.0.1", "--port", str(port)], + 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, - arguments=[executable, "worker"], + arguments=[*launcher, "worker"], environment=dict(environment), working_directory=str(stack_directory), log_file=str(stack_directory / "logs" / "worker.log"), diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index 630e0097..cc4b13d3 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -90,8 +90,8 @@ class TestNativeUp: "--postgres-uri", "postgresql://localhost/docsgpt"], _native_context(services)) == 0 api = services.units["docsgpt-api"] worker = services.units["docsgpt-worker"] - assert api.arguments[1:] == ["api", "--host", "127.0.0.1", "--port", "7091"] - assert worker.arguments[1:] == ["worker"] + assert api.arguments[-5:] == ["api", "--host", "127.0.0.1", "--port", "7091"] + assert worker.arguments[-1] == "worker" for unit in (api, worker): assert unit.environment["DOCSGPT_HOME"] == str(tmp_path) assert unit.working_directory == str(tmp_path) @@ -105,7 +105,7 @@ class TestNativeUp: assert _run(["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/docsgpt"], context) == 0 arguments, environment = calls[0] - assert arguments[1] == "migrate" + assert arguments[-1] == "migrate", "the launcher can be an interpreter and -m docsgpt" assert environment["DOCSGPT_HOME"] == str(tmp_path) assert environment["DOCSGPT_ENV_FILE"] == str(tmp_path / ".env") @@ -139,13 +139,14 @@ class TestNativeUp: with pytest.raises(DeployError, match="holds a Docker install"): _run(argv, _native_context()) - def test_services_are_not_written_without_a_docsgpt_command(self, tmp_path, monkeypatch): - """A unit pointing at something unexecutable would fail to exec with only a health check to show it.""" + def test_without_the_command_on_path_the_services_run_the_module(self, tmp_path, monkeypatch): + """A unit must name something executable; argv[0] is a module file under `python -m`.""" monkeypatch.setattr("docsgpt.deploy.commands.shutil.which", lambda name: None) monkeypatch.setattr(sys, "argv", [str(tmp_path / "not-a-program")]) + services = FakeServices() argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d"] - with pytest.raises(DeployError, match="not on PATH"): - _run(argv, _native_context()) + assert _run(argv, _native_context(services)) == 0 + assert services.units["docsgpt-api"].arguments[:3] == [sys.executable, "-m", "docsgpt"] def test_a_database_url_is_required(self, tmp_path): with pytest.raises(DeployError, match="--postgres-uri"): @@ -292,7 +293,7 @@ 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)[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"] @@ -308,7 +309,7 @@ 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)[0]) unit = tmp_path / ".config" / "systemd" / "user" / "docsgpt-api.service" services.stop("docsgpt-api") assert unit.is_file(), "stopping keeps the unit" diff --git a/tests/test_cli.py b/tests/test_cli.py index 452f5598..dcad465e 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -34,6 +34,16 @@ class TestTopLevel: subprocess.run([sys.executable, "-c", code], cwd=Path(__file__).resolve().parents[1], check=True) +class TestModuleEntrypoint: + def test_python_m_docsgpt_runs_the_cli(self): + """`python -m docsgpt` is what a native service falls back to when the script is not on PATH.""" + result = subprocess.run( + [sys.executable, "-m", "docsgpt", "--version"], + cwd=Path(__file__).resolve().parents[1], capture_output=True, text=True, check=True, + ) + assert result.stdout.strip() == f"docsgpt {__version__}" + + class TestHome: @staticmethod def _installed(monkeypatch, tmp_path): From dbbed28f88e17d6eefdb77bcac1335aad099afe9 Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 16 Sep 2026 22:55:09 +0100 Subject: [PATCH 04/17] fix: upgrade re-execs a docsgpt it can actually find `upgrade` exec'd the bare name `docsgpt`, so after `python -m docsgpt upgrade` in a virtualenv without the console script on PATH, os.execv failed with a traceback. It now uses the same launcher the service units get, which is why that helper is no longer named for native mode. --- docsgpt/deploy/commands.py | 9 +++++---- tests/deploy/test_commands.py | 13 ++++++++++++- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 0988044d..f718e2e0 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -218,8 +218,8 @@ def _redis_urls(base: str) -> dict[str, str]: } -def _native_launcher() -> list[str]: - """How the service files start DocsGPT: the command on PATH, or this interpreter and the module. +def _docsgpt_launcher() -> list[str]: + """How to start DocsGPT from another process: the command on PATH, or this interpreter and the module. A unit has to name something that can be executed, and ``sys.argv[0]`` often cannot be: under ``python -m docsgpt``, or pytest, it is a module file. Falling back to the running interpreter @@ -285,7 +285,7 @@ def _native_up(args, context: Context, directory: Path) -> int: (directory / "logs").mkdir(exist_ok=True) envfile.update(env_path, updates) - launcher = _native_launcher() + launcher = _docsgpt_launcher() print("Applying database migrations ...") # The child reads the stack's settings, not a .env in whatever directory this was run from. stack_env = {"DOCSGPT_HOME": str(directory), "DOCSGPT_ENV_FILE": str(env_path)} @@ -692,7 +692,8 @@ def upgrade(args, context: Optional[Context] = None) -> int: if installer == "uv": if context.run(["uv", "tool", "install", "--force", spec]) != 0: raise DeployError(f"uv could not install {spec}") - return context.exec_up(["docsgpt", "up", "--dir", str(stack.stack_dir(args.dir))]) + # The same launcher the service units get: a bare name is not always on PATH to exec. + return context.exec_up([*_docsgpt_launcher(), "up", "--dir", str(stack.stack_dir(args.dir))]) command = f"pipx install --force {spec}" if installer == "pipx" else f"pip install -U {spec}" print(f"Upgrade the package with `{command}`, then run `docsgpt up` to move the stack to it.", file=sys.stderr) return 1 diff --git a/tests/deploy/test_commands.py b/tests/deploy/test_commands.py index b73fedb2..9e9e684b 100644 --- a/tests/deploy/test_commands.py +++ b/tests/deploy/test_commands.py @@ -334,7 +334,8 @@ class TestUpgrade: ) assert _run(["upgrade", "--dir", str(tmp_path), "--version", "0.22.0"], context) == 0 assert self_calls[0] == ["uv", "tool", "install", "--force", "docsgpt==0.22.0"] - assert self_calls[1] == ["exec", "docsgpt", "up", "--dir", str(tmp_path)] + assert self_calls[1][0] == "exec" + assert self_calls[1][-3:] == ["up", "--dir", str(tmp_path)], "the launcher can be an interpreter and -m" def test_latest_when_no_version_is_given(self, tmp_path): self_calls = [] @@ -343,6 +344,16 @@ class TestUpgrade: assert _run(["upgrade", "--dir", str(tmp_path)], context) == 0 assert self_calls[0] == ["uv", "tool", "install", "--force", "docsgpt"] + def test_without_the_command_on_path_it_re_execs_the_module(self, tmp_path, monkeypatch): + """After `python -m docsgpt upgrade` there may be no docsgpt on PATH for execv to find.""" + monkeypatch.setattr("docsgpt.deploy.commands.shutil.which", lambda name: None) + monkeypatch.setattr(sys, "argv", [str(tmp_path / "not-a-program")]) + self_calls = [] + context = _context(installer=lambda: "uv", run=lambda args: 0, + exec_up=lambda argv: self_calls.append(argv) or 0) + assert _run(["upgrade", "--dir", str(tmp_path)], context) == 0 + assert self_calls[0][:3] == [sys.executable, "-m", "docsgpt"] + def test_a_pip_install_is_told_what_to_run(self, tmp_path, capsys): context = _context(installer=lambda: "pip", run=lambda args: pytest.fail("must not run")) assert _run(["upgrade", "--dir", str(tmp_path)], context) == 1 From 4d9f1d47a98ad9852a4dd23bbd203043ace3bbcc Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 16 Sep 2026 22:58:52 +0100 Subject: [PATCH 05/17] 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:, 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 | 20 +++++++---- docsgpt/deploy/native.py | 23 ++++++++---- tests/deploy/test_native.py | 70 +++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 13 deletions(-) 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"] From 61f06fcce8732ad69874ce5c34bbbde4361cce1c Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 16 Sep 2026 23:00:33 +0100 Subject: [PATCH 06/17] fix: docsgpt open points at the address a native install answers on `open` built its address from stack.url, which honours DOCSGPT_BIND, while the native units always bind 127.0.0.1: with a LAN bind it handed the browser an address nothing was listening on. status had the same mismatch and was fixed with it; both now go through one helper so they cannot drift apart again. --- docsgpt/deploy/commands.py | 22 +++++++++++++++++----- tests/deploy/test_native.py | 12 ++++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 45b9c461..aad99560 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -240,6 +240,16 @@ def _docsgpt_launcher() -> list[str]: ) +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) + + +def _native_address(env: Mapping[str, str]) -> str: + """Where a native install answers: its units bind 127.0.0.1, whatever DOCSGPT_BIND says.""" + return f"http://localhost:{_native_port(env)}" + + def _native_up(args, context: Context, directory: Path) -> int: """Run the API and the worker as services on this machine, against an existing Postgres and Redis.""" services = context.service_manager() @@ -445,12 +455,11 @@ 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 + # 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 + port = _native_port(env) print(f"DocsGPT {_record(directory).get('version', 'unknown')} in {directory} (native, {services.name})") - print(f"Address: http://localhost:{port}") + print(f"Address: {_native_address(env)}") for name in native.SERVICES: print(f" {name}: {'running' if services.is_running(name) else 'stopped'}") healthy = context.wait(f"http://127.0.0.1:{port}/api/health", 0) @@ -510,7 +519,10 @@ def open_ui(args, context: Optional[Context] = None) -> int: env = _installed(directory) if env is None: return 1 - address = stack.url(env, context.lan_ip()) + # A native install answers on loopback only, so stack.url would hand the browser a LAN address + # or a domain that nothing behind this command is serving. + native_install = _mode(directory) == "native" + address = _native_address(env) if native_install else stack.url(env, context.lan_ip()) print(address) context.open_browser(address) return 0 diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index 54cb681d..39a8938c 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -209,6 +209,18 @@ class TestNativeLifecycle: assert _run(["status", "--dir", str(tmp_path)], context) == 0 assert checked == ["http://127.0.0.1:7091/api/health"] + def test_open_hands_the_browser_the_address_that_answers(self, tmp_path, capsys): + """stack.url would offer a LAN address, but the native units listen on loopback only.""" + services = FakeServices() + self._installed(tmp_path, services) + envfile.update(tmp_path / ".env", {"DOCSGPT_BIND": "192.168.1.50"}) + opened = [] + context = _native_context(services) + context.open_browser = lambda url: opened.append(url) + assert _run(["open", "--dir", str(tmp_path)], context) == 0 + assert opened == ["http://localhost:7091"] + assert "192.168.1.50" not in capsys.readouterr().out + def test_down_stops_them_and_leaves_the_settings(self, tmp_path): services = FakeServices() self._installed(tmp_path, services) From 501baf8aae6988d369800a6a559f882a963db93c Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 16 Sep 2026 23:01:30 +0100 Subject: [PATCH 07/17] fix: say why backup and restore do not apply to a native install Both accepted a native install and then drove `docker compose` in a directory with no compose file, so the user got "no configuration file provided" rather than an explanation. They now refuse with what to do instead, and restore refuses before it reads the archive or stops anything. --- docsgpt/deploy/commands.py | 10 +++++++++- tests/deploy/test_native.py | 11 +++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index aad99560..6353925a 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -568,6 +568,10 @@ def backup(args, context: Optional[Context] = None) -> int: env = _installed(directory) if env is None: return 1 + if _mode(directory) == "native": + raise DeployError( + f"{directory} is a native install: its database and its files are not in Docker volumes, so there is nothing here to archive. Back up the PostgreSQL that POSTGRES_URI points at with pg_dump, and copy the indexes, inputs and vectors folders from the data home." + ) out_dir = Path(args.out).expanduser() if args.out else directory / "backups" taken_at = datetime.now(timezone.utc) @@ -657,13 +661,17 @@ def _restore_data(context: Context, directory: Path, archive: Path, volumes: lis def restore(args, context: Optional[Context] = None) -> int: """Put a backup's database and data volumes back over this install.""" context = context or Context.default(args) + directory = stack.stack_dir(args.dir) + if _mode(directory) == "native": + raise DeployError( + f"{directory} is a native install: its database and its files are not in Docker volumes, so there is nothing here to archive. Back up the PostgreSQL that POSTGRES_URI points at with pg_dump, and copy the indexes, inputs and vectors folders from the data home. `docsgpt restore` puts back what `docsgpt backup` wrote for a Docker install." + ) archive = Path(args.archive).expanduser() manifest = backup_format.read_manifest(archive) backup_format.check_version(manifest, context.version, args.force) # Everything the archive declares is checked here, while DocsGPT is still up. volumes = backup_format.validate(archive, manifest) - directory = stack.stack_dir(args.dir) env = _installed(directory) if env is None: return 1 diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index 39a8938c..2b360803 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -238,6 +238,17 @@ class TestNativeLifecycle: self._installed(tmp_path, FakeServices()) assert _run(["logs", "--dir", str(tmp_path)], _native_context()) == 0, "logs work in both modes" + def test_backup_says_why_there_is_nothing_to_archive(self, tmp_path): + """Its data is not in Docker volumes, so compose would fail with no configuration file.""" + self._installed(tmp_path, FakeServices()) + with pytest.raises(DeployError, match="native install"): + _run(["backup", "--dir", str(tmp_path)], _native_context()) + + def test_restore_refuses_before_it_touches_anything(self, tmp_path): + self._installed(tmp_path, FakeServices()) + with pytest.raises(DeployError, match="native install"): + _run(["restore", str(tmp_path / "any.tar.gz"), "--dir", str(tmp_path), "--yes"], _native_context()) + class FakeLaunchctl: """launchctl, with a bootout that takes `unload_polls` polls to take effect.""" From 6cfd0948bc5b5604d3c76b9f188404e48e65605a Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 16 Sep 2026 23:03:03 +0100 Subject: [PATCH 08/17] fix: refuse the Docker-only options in native mode instead of ignoring them `up --native` took --expose, --domain and --docling and did nothing with them. Asking for network exposure and silently getting a loopback-only install, or asking for docling and getting an install without it, is worse than being told. Each now says what to do instead: a reverse proxy or the Docker stack for exposure, and the docling extra for the parser engine. --expose local and --no-docling already describe native mode, so they stay silent. --- docsgpt/deploy/commands.py | 17 +++++++++++++++++ tests/deploy/test_native.py | 16 ++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 6353925a..1495bb5b 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -240,6 +240,22 @@ def _docsgpt_launcher() -> list[str]: ) +def _refuse_docker_only_options(args) -> None: + """Options that only mean something to the Docker stack, refused rather than quietly ignored.""" + if getattr(args, "domain", None) or getattr(args, "expose", None) in ("network", "domain"): + raise DeployError( + "a native install serves on 127.0.0.1 only, so --domain, --expose network and " + "--expose domain have nothing to act on here. Put a reverse proxy in front of it, or run " + "the Docker stack with `docsgpt up --expose ...`, which brings its own Caddy for a domain." + ) + if getattr(args, "docling", None): + raise DeployError( + "--docling selects a Docker image variant, which a native install does not use. Install the " + 'parser engine into this environment instead, with `uv tool install "docsgpt[docling]"` or ' + '`pip install "docsgpt[docling]"`, then run `docsgpt up --native` again.' + ) + + 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) @@ -252,6 +268,7 @@ def _native_address(env: Mapping[str, str]) -> str: def _native_up(args, context: Context, directory: Path) -> int: """Run the API and the worker as services on this machine, against an existing Postgres and Redis.""" + _refuse_docker_only_options(args) services = context.service_manager() env_path = directory / ".env" existing = envfile.read(env_path) diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index 2b360803..db3b1dfb 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -164,6 +164,22 @@ class TestNativeUp: assert _run(["uninstall", "--yes", "--dir", str(tmp_path)], _native_context(services)) == 0 assert sorted(services.removed) == ["docsgpt-api", "docsgpt-worker"] + 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.""" + base = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d"] + with pytest.raises(DeployError, match="127.0.0.1 only"): + _run([*base, "--expose", "network"], _native_context()) + with pytest.raises(DeployError, match="127.0.0.1 only"): + _run([*base, "--domain", "docs.example.com"], _native_context()) + with pytest.raises(DeployError, match=r"docsgpt\[docling\]"): + _run([*base, "--docling"], _native_context()) + assert not (tmp_path / ".env").exists(), "it refuses before writing anything" + + def test_the_options_that_describe_what_native_already_does_are_kept(self, tmp_path): + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d", + "--expose", "local", "--no-docling"] + assert _run(argv, _native_context()) == 0 + 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()) From 76d81caaa468dd79de8bd20eda097379dc6bded4 Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 16 Sep 2026 23:10:53 +0100 Subject: [PATCH 09/17] fix: refuse control characters in the values of a service file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Quoting cannot carry a newline into a unit file or a plist: the line ends and whatever follows becomes another directive. Every value bound for a service file — the working directory, the log path, environment names and values, and the command arguments — is checked before any of it is rendered, on both launchd and systemd. --- docsgpt/deploy/native.py | 23 +++++++++++++++++++++++ tests/deploy/test_native.py | 25 +++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/docsgpt/deploy/native.py b/docsgpt/deploy/native.py index 876e912a..ab78d0ed 100644 --- a/docsgpt/deploy/native.py +++ b/docsgpt/deploy/native.py @@ -13,6 +13,7 @@ from __future__ import annotations import os import plistlib +import re import shlex import subprocess import sys @@ -47,6 +48,7 @@ def label_for(name: str) -> str: def launchd_plist(unit: Unit, label: Optional[str] = None) -> str: """The launchd agent for ``unit``: kept alive, with its output in the stack's log file.""" + _check_unit_values(unit) body = { "Label": label or label_for(unit.name), "ProgramArguments": list(unit.arguments), @@ -61,6 +63,26 @@ def launchd_plist(unit: Unit, label: Optional[str] = None) -> str: return plistlib.dumps(body).decode("utf-8") +_CONTROL_CHARACTERS = re.compile(r"[\x00-\x1f\x7f]") + + +def _reject_control_characters(what: str, value: str) -> None: + """Service files are line-based, so a newline in a value adds a directive instead of text.""" + if _CONTROL_CHARACTERS.search(value): + raise DeployError(f"{what} contains a control character, which a service file cannot carry: {value!r}") + + +def _check_unit_values(unit: Unit) -> None: + """Everything bound for a service file, checked before any of it is rendered.""" + _reject_control_characters("the working directory", unit.working_directory) + _reject_control_characters("the log file path", unit.log_file) + for key, value in unit.environment.items(): + _reject_control_characters("an environment name", key) + _reject_control_characters(f"the environment value for {key}", value) + for argument in unit.arguments: + _reject_control_characters("a command argument", argument) + + 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('"', '\\"') @@ -69,6 +91,7 @@ def _systemd_quote(value: str) -> str: def systemd_unit(unit: Unit) -> str: """The systemd user unit for ``unit``; values are quoted, so a path with spaces survives.""" + _check_unit_values(unit) environment = "\n".join( f"Environment={_systemd_quote(f'{key}={value}')}" for key, value in sorted(unit.environment.items()) ) diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index db3b1dfb..31dc6729 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -487,6 +487,31 @@ class TestUnitFiles: assert 'WorkingDirectory="/srv/my \\"odd\\" dir"' in body assert 'Environment="DOCSGPT_HOME=/srv/my \\"odd\\" dir"' in body + def test_a_newline_in_a_path_is_refused_rather_than_quoted(self, tmp_path): + """A service file is line-based: quoting cannot hold a newline, it would add a directive.""" + unit = native.Unit( + name="docsgpt-api", + arguments=["/venv/bin/docsgpt", "api"], + environment={"DOCSGPT_HOME": str(tmp_path)}, + working_directory="/srv/x\nExecStart=/bin/sh -c evil", + log_file="/srv/api.log", + ) + with pytest.raises(DeployError, match="control character"): + native.systemd_unit(unit) + with pytest.raises(DeployError, match="control character"): + native.launchd_plist(unit) + + def test_a_newline_in_the_environment_is_refused(self, tmp_path): + unit = native.Unit( + name="docsgpt-api", + arguments=["/venv/bin/docsgpt", "api"], + environment={"DOCSGPT_HOME": "/srv/x\nEnvironment=EVIL=1"}, + working_directory=str(tmp_path), + log_file="/srv/api.log", + ) + with pytest.raises(DeployError, match="control character"): + native.systemd_unit(unit) + 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"] From 65ee2f42019b7dd496d8cd6b9ac4f955e582525c Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 16 Sep 2026 23:22:07 +0100 Subject: [PATCH 10/17] fix: keep the whole Redis URL when handing out its databases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three URLs were built by string surgery, so anything after the database number was mangled rather than kept: rediss://host:6380/0?ssl_cert_reqs=required came out as .../0?ssl_cert_reqs=required/0, and a URL carrying a query but no database had /0 appended after the query. TLS and managed Redis endpoints usually carry exactly those parameters. The URL is split properly now, the three databases go in the path, and scheme, credentials, host, query and fragment are preserved. A URL that cannot be numbered this way — one that is not redis:// or rediss://, or that has something other than a number where the database goes — is refused with a message instead of being turned into something that merely looks like a URL. --- docsgpt/deploy/commands.py | 27 +++++++++++++++++++-------- tests/deploy/test_native.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 1495bb5b..8d00f7bb 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -17,6 +17,7 @@ from dataclasses import dataclass from datetime import datetime, timezone from pathlib import Path from typing import Any, Optional +from urllib.parse import urlsplit, urlunsplit from docsgpt.deploy import backup as backup_format from docsgpt.deploy import envfile, native, stack @@ -205,16 +206,26 @@ def _redis_urls(base: str) -> dict[str, str]: They start at database 0, or at the one the URL names: ``redis://host:6379/5`` puts them on 5, 6 and 7, which is how one Redis is shared with something that already uses the first databases. + Everything else in the URL is kept — TLS, credentials, and query parameters such as the + ``ssl_cert_reqs`` that a rediss:// endpoint usually needs. """ - trimmed = base.rstrip("/") - first = 0 - head, _, tail = trimmed.rpartition("/") - if tail.isdigit(): - trimmed, first = head, int(tail) + parts = urlsplit(base) + if parts.scheme not in ("redis", "rediss"): + raise DeployError( + f"the Redis URL {base!r} should start with redis:// or rediss://, with any options as " + "query parameters, so the broker, the result backend and the cache can be given a " + "database each." + ) + path = parts.path.rstrip("/").lstrip("/") + if path and not 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." + ) + first = int(path) if path else 0 return { - "CELERY_BROKER_URL": f"{trimmed}/{first}", - "CELERY_RESULT_BACKEND": f"{trimmed}/{first + 1}", - "CACHE_REDIS_URL": f"{trimmed}/{first + 2}", + key: urlunsplit((parts.scheme, parts.netloc, f"/{first + offset}", parts.query, parts.fragment)) + for key, offset in (("CELERY_BROKER_URL", 0), ("CELERY_RESULT_BACKEND", 1), ("CACHE_REDIS_URL", 2)) } diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index 31dc6729..cd7092ca 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -180,6 +180,35 @@ class TestNativeUp: "--expose", "local", "--no-docling"] assert _run(argv, _native_context()) == 0 + def test_a_redis_url_keeps_its_credentials_tls_and_query(self, tmp_path): + """A rediss:// endpoint usually needs ssl_cert_reqs, and losing it breaks every connection.""" + url = "rediss://user:pw@redis.example.com:6380/3?ssl_cert_reqs=required" + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", + "--postgres-uri", "postgresql://localhost/d", "--redis-url", url] + assert _run(argv, _native_context()) == 0 + env = envfile.read(tmp_path / ".env") + host = "rediss://user:pw@redis.example.com:6380" + assert env["CELERY_BROKER_URL"] == f"{host}/3?ssl_cert_reqs=required" + assert env["CELERY_RESULT_BACKEND"] == f"{host}/4?ssl_cert_reqs=required" + assert env["CACHE_REDIS_URL"] == f"{host}/5?ssl_cert_reqs=required" + + def test_a_redis_url_with_a_query_and_no_database(self, tmp_path): + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d", + "--redis-url", "redis://localhost:6379?health_check_interval=30"] + assert _run(argv, _native_context()) == 0 + env = envfile.read(tmp_path / ".env") + assert env["CELERY_BROKER_URL"] == "redis://localhost:6379/0?health_check_interval=30" + assert env["CACHE_REDIS_URL"] == "redis://localhost:6379/2?health_check_interval=30" + + @pytest.mark.parametrize("url", ["localhost:6379", "redis+socket:///var/run/redis.sock", + "redis://localhost:6379/queue"]) + def test_a_redis_url_that_cannot_be_numbered_is_refused(self, tmp_path, url): + """Silently turning it into something that looks like a URL is the worse failure.""" + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", + "--postgres-uri", "postgresql://localhost/d", "--redis-url", url] + with pytest.raises(DeployError, match="Redis URL"): + _run(argv, _native_context()) + 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()) From c8ab1cbab26f882d63a3a5a5d085e4cd3c5353d4 Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 16 Sep 2026 23:30:47 +0100 Subject: [PATCH 11/17] fix: report a malformed Redis URL instead of raising from urlsplit urlsplit raises ValueError on input such as redis://[::1 , which nothing converted, so a typo left native setup with a traceback rather than the message every other unusable URL gets. --- docsgpt/deploy/commands.py | 6 +++++- tests/deploy/test_native.py | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 8d00f7bb..5a853e8f 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -209,7 +209,11 @@ def _redis_urls(base: str) -> dict[str, str]: Everything else in the URL is kept — TLS, credentials, and query parameters such as the ``ssl_cert_reqs`` that a rediss:// endpoint usually needs. """ - parts = urlsplit(base) + try: + parts = urlsplit(base) + except ValueError as exc: + # urlsplit raises on things like redis://[::1 ; that is a typo, not a crash. + raise DeployError(f"the Redis URL {base!r} could not be read: {exc}") from exc if parts.scheme not in ("redis", "rediss"): raise DeployError( f"the Redis URL {base!r} should start with redis:// or rediss://, with any options as " diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index cd7092ca..0e09a382 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -200,6 +200,22 @@ class TestNativeUp: assert env["CELERY_BROKER_URL"] == "redis://localhost:6379/0?health_check_interval=30" assert env["CACHE_REDIS_URL"] == "redis://localhost:6379/2?health_check_interval=30" + def test_a_malformed_redis_url_is_reported_not_raised(self, tmp_path): + """urlsplit raises ValueError on an unterminated IPv6 bracket; a typo is not a traceback.""" + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", + "--postgres-uri", "postgresql://localhost/d", "--redis-url", "redis://[::1"] + with pytest.raises(DeployError, match="could not be read"): + _run(argv, _native_context()) + + def test_an_ipv6_redis_url_still_works(self, tmp_path): + """Refusing the malformed form must not cost the valid one.""" + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d", + "--redis-url", "redis://[::1]:6379/2"] + assert _run(argv, _native_context()) == 0 + env = envfile.read(tmp_path / ".env") + assert env["CELERY_BROKER_URL"] == "redis://[::1]:6379/2" + assert env["CACHE_REDIS_URL"] == "redis://[::1]:6379/4" + @pytest.mark.parametrize("url", ["localhost:6379", "redis+socket:///var/run/redis.sock", "redis://localhost:6379/queue"]) def test_a_redis_url_that_cannot_be_numbered_is_refused(self, tmp_path, url): From 4976b3104cbd9c2f832e6a909d076e697dcdc71a Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 16 Sep 2026 23:51:20 +0100 Subject: [PATCH 12/17] 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 | 58 ++++++++++++++++++---- docsgpt/deploy/native.py | 36 ++++++++++---- tests/deploy/test_native.py | 98 +++++++++++++++++++++++++++++++------ 3 files changed, 159 insertions(+), 33 deletions(-) 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"] From 217310e201c71cfbbc0004bffa050277a8645eaa Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 00:03:06 +0100 Subject: [PATCH 13/17] fix: run the native preflights before anything is written MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Docker-stack check ran after logs/ was created, .env written and the migrations applied, so a conflict left a migrated database and partial files behind with no install.json — exactly the directory that down and uninstall then refuse. It runs immediately after the port is resolved now. Alongside it, `up --native` refuses a port it cannot bind. Neither service manager confirms that the API bound, and /api/health carries no installation identity, so a second install on the same port would have been answered by the first and reported success while its own API was dead. An install re-running on its own port is the exception, since its services are what hold it. --- docsgpt/deploy/commands.py | 36 +++++++++++++++++++++++++++++++++--- tests/deploy/test_native.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 1f526e86..5ba2c4e3 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -8,6 +8,7 @@ import os import re import secrets import shutil +import socket import subprocess import sys import tempfile @@ -284,6 +285,33 @@ def _service_names(directory: Path) -> tuple[str, str]: return native.service_names(directory, stack.stack_dir(None)) +def _port_is_free(port: int) -> bool: + """Whether the loopback port can still be bound.""" + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as probe: + try: + probe.bind(("127.0.0.1", port)) + except OSError: + return False + return True + + +def _refuse_busy_port(directory: Path, port: int, services, names: tuple[str, str]) -> None: + """Whatever holds the port would answer the health check while these services failed to bind. + + An install re-running on its own port is the exception: its services are holding it, and + starting them again replaces them. + """ + if _port_is_free(port): + return + if _record(directory).get("mode") == "native" and any(services.is_running(name) for name in names): + return + raise DeployError( + f"port {port} is already in use by something other than this install. The API would fail to " + f"bind it while the health check answered from whatever holds it, so the install would look " + f"healthy and be dead. Free the port, or give this install another one with --port." + ) + + 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: @@ -340,6 +368,11 @@ def _native_up(args, context: Context, directory: Path) -> int: port = _port_number(existing["DOCSGPT_PORT"], f"DOCSGPT_PORT in {env_path}") else: port = stack.DEFAULT_PORT + + names = _service_names(directory) + _refuse_other_docker_stack(context, directory, port) + _refuse_busy_port(directory, port, services, names) + updates: dict[str, Optional[str]] = { "POSTGRES_URI": postgres, "API_URL": f"http://127.0.0.1:{port}", @@ -369,8 +402,6 @@ 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") @@ -378,7 +409,6 @@ 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") - names = _service_names(directory) for unit in native.units_for(directory, launcher, port, directory, names): services.install(unit) for name in names: diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index 55b6d3c3..ce66744f 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -2,6 +2,7 @@ import json import plistlib +import socket import sys import types from pathlib import Path @@ -273,9 +274,37 @@ class TestNativeUp: directory = tmp_path / "native" argv = ["up", "--native", "--dir", str(directory), "--yes", "--port", "7099", "--postgres-uri", "postgresql://localhost/d"] + migrated = [] + context.run = lambda args, env=None: migrated.append(args) or 0 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" + assert not (directory / ".env").exists(), "and before writing any settings" + assert not (directory / "logs").exists() + assert migrated == [], "and before touching the database" + + def test_a_port_something_else_holds_is_refused(self, tmp_path): + """Nothing confirms the API bound its port, and a generic health check answers from the holder.""" + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as held: + held.bind(("127.0.0.1", 0)) + held.listen(1) + port = held.getsockname()[1] + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--port", str(port), + "--postgres-uri", "postgresql://localhost/d"] + with pytest.raises(DeployError, match="already in use"): + _run(argv, _native_context()) + assert not (tmp_path / ".env").exists(), "it refuses before writing anything" + + def test_an_install_may_keep_the_port_its_own_services_hold(self, tmp_path): + """Re-running an install must not trip over the services it is about to replace.""" + services = FakeServices() + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d"] + assert _run(argv, _native_context(services)) == 0 + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as held: + held.bind(("127.0.0.1", 0)) + held.listen(1) + envfile.update(tmp_path / ".env", {"DOCSGPT_PORT": str(held.getsockname()[1])}) + assert _run(["up", "--dir", str(tmp_path), "--yes"], _native_context(services)) == 0 def test_a_database_url_is_required(self, tmp_path): with pytest.raises(DeployError, match="--postgres-uri"): From 0d153d3c0de23e06963f1f575cdcc2d54d83e8d5 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 00:12:40 +0100 Subject: [PATCH 14/17] fix: tie the busy-port exemption to the port the install is recorded on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The exemption asked whether any service of the install was running, so moving an install onto a different port that something else held would pass the check and then fail to bind, with the health poll answered by whatever owned that port — the false success the check exists to prevent. install.json carries the API port now, and a busy port is allowed only when it is that port and the API service is running. --- docsgpt/deploy/commands.py | 23 ++++++++++++++++------- tests/deploy/test_native.py | 25 +++++++++++++++++++++---- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 5ba2c4e3..c03ccecb 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -298,17 +298,26 @@ def _port_is_free(port: int) -> bool: def _refuse_busy_port(directory: Path, port: int, services, names: tuple[str, str]) -> None: """Whatever holds the port would answer the health check while these services failed to bind. - An install re-running on its own port is the exception: its services are holding it, and - starting them again replaces them. + The one exception is this install's own API holding the port it is recorded on. Ownership is not + inferred from the install having some service running: asking for a different port that something + else holds is how a move to a new port would look, and the API would fail to bind it. """ if _port_is_free(port): return - if _record(directory).get("mode") == "native" and any(services.is_running(name) for name in names): + record = _record(directory) + api_service = names[0] + owns_the_port = ( + record.get("mode") == "native" + and str(record.get("port") or "") == str(port) + and services.is_running(api_service) + ) + if owns_the_port: return raise DeployError( - f"port {port} is already in use by something other than this install. The API would fail to " - f"bind it while the health check answered from whatever holds it, so the install would look " - f"healthy and be dead. Free the port, or give this install another one with --port." + f"port {port} is already in use by something other than this install's API. It would fail to " + f"bind while the health check answered from whatever holds the port, so the install would " + f"look healthy and be dead. Free the port, stop this install first with `docsgpt down` if it " + f"is the one holding it on another port, or choose another port with --port." ) @@ -406,7 +415,7 @@ def _native_up(args, context: Context, directory: Path) -> int: # 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) + record.update(version=context.version, mode="native", port=port, 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): diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index ce66744f..d5a41696 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -295,16 +295,33 @@ class TestNativeUp: _run(argv, _native_context()) assert not (tmp_path / ".env").exists(), "it refuses before writing anything" - def test_an_install_may_keep_the_port_its_own_services_hold(self, tmp_path): - """Re-running an install must not trip over the services it is about to replace.""" + def test_an_install_may_keep_the_port_its_own_api_is_recorded_on(self, tmp_path): + """Re-running an install must not trip over the API it is about to replace.""" + services = FakeServices() + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as probe: + probe.bind(("127.0.0.1", 0)) + port = probe.getsockname()[1] + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--port", str(port), + "--postgres-uri", "postgresql://localhost/d"] + assert _run(argv, _native_context(services)) == 0 + assert json.loads((tmp_path / "install.json").read_text())["port"] == port + + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as held: + held.bind(("127.0.0.1", port)) + held.listen(1) + assert _run(["up", "--dir", str(tmp_path), "--yes"], _native_context(services)) == 0 + + def test_moving_an_install_onto_a_busy_port_is_refused(self, tmp_path): + """Ownership is of one port, not of any port while some service of the install runs.""" services = FakeServices() argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d"] assert _run(argv, _native_context(services)) == 0 with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as held: held.bind(("127.0.0.1", 0)) held.listen(1) - envfile.update(tmp_path / ".env", {"DOCSGPT_PORT": str(held.getsockname()[1])}) - assert _run(["up", "--dir", str(tmp_path), "--yes"], _native_context(services)) == 0 + elsewhere = held.getsockname()[1] + with pytest.raises(DeployError, match="already in use"): + _run(["up", "--dir", str(tmp_path), "--yes", "--port", str(elsewhere)], _native_context(services)) def test_a_database_url_is_required(self, tmp_path): with pytest.raises(DeployError, match="--postgres-uri"): From 147bb352eced358c07c8e5167a23faa13c7cf976 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 00:23:24 +0100 Subject: [PATCH 15/17] fix: report a Redis database number too long for int() to read The ASCII-digit check accepts any length, but since 3.11 Python refuses to convert a digit string past its conversion limit, so a long one raised ValueError straight through the CLI instead of the message every other unusable URL gets. --- docsgpt/deploy/commands.py | 10 +++++++++- tests/deploy/test_native.py | 7 +++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index c03ccecb..19babb5a 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -227,7 +227,15 @@ def _redis_urls(base: str) -> dict[str, str]: 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." ) - first = int(path) if path else 0 + try: + first = int(path) if path else 0 + except ValueError as exc: + # Python refuses to convert a digit string past its conversion limit, and that is a typo + # rather than a crash. + raise DeployError( + f"the Redis URL {base!r} has a database number too long to read. Pass a URL like " + "redis://host:6379 or redis://host:6379/5." + ) from exc return { key: urlunsplit((parts.scheme, parts.netloc, f"/{first + offset}", parts.query, parts.fragment)) for key, offset in (("CELERY_BROKER_URL", 0), ("CELERY_RESULT_BACKEND", 1), ("CACHE_REDIS_URL", 2)) diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index d5a41696..33a50568 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -224,6 +224,13 @@ class TestNativeUp: assert env["CELERY_BROKER_URL"] == "redis://[::1]:6379/2" assert env["CACHE_REDIS_URL"] == "redis://[::1]:6379/4" + def test_a_redis_database_number_too_long_to_convert_is_reported(self, tmp_path): + """Since 3.11 int() refuses a digit string past its limit, which the digit check let through.""" + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d", + "--redis-url", "redis://localhost:6379/" + "1" * 5000] + with pytest.raises(DeployError, match="too long to read"): + _run(argv, _native_context()) + @pytest.mark.parametrize("url", ["localhost:6379", "redis+socket:///var/run/redis.sock", "redis://localhost:6379/queue"]) def test_a_redis_url_that_cannot_be_numbered_is_refused(self, tmp_path, url): From 51b2fa38de42dba88cccb762dc8da60a0043a56f Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 00:34:42 +0100 Subject: [PATCH 16/17] fix: refuse a Redis URL whose port cannot be used urlsplit accepts an authority such as localhost:notaport or localhost:65536 and urlunsplit rebuilds it verbatim; only parts.port raises, and nothing read it. The unusable value reached .env, where the worker picked it up and failed to start its broker, while `up` reported success because it waits only on the API health endpoint. --- docsgpt/deploy/commands.py | 7 +++++++ tests/deploy/test_native.py | 9 +++++++++ 2 files changed, 16 insertions(+) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 19babb5a..db114639 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -215,6 +215,13 @@ def _redis_urls(base: str) -> dict[str, str]: except ValueError as exc: # urlsplit raises on things like redis://[::1 ; that is a typo, not a crash. raise DeployError(f"the Redis URL {base!r} could not be read: {exc}") from exc + try: + _ = parts.port # a non-numeric or out-of-range port raises here, not when the URL is split + except ValueError as exc: + raise DeployError( + f"the Redis URL {base!r} has an unusable port: {exc}. Pass a URL like " + "redis://host:6379 or redis://host:6379/5." + ) from exc if parts.scheme not in ("redis", "rediss"): raise DeployError( f"the Redis URL {base!r} should start with redis:// or rediss://, with any options as " diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index 33a50568..a9335068 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -231,6 +231,15 @@ class TestNativeUp: with pytest.raises(DeployError, match="too long to read"): _run(argv, _native_context()) + @pytest.mark.parametrize("url", ["redis://localhost:notaport/0", "redis://localhost:65536/0"]) + def test_a_redis_url_with_an_unusable_port_is_refused(self, tmp_path, url): + """urlsplit accepts it and rebuilds it verbatim; only .port notices, and the worker dies later.""" + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", + "--postgres-uri", "postgresql://localhost/d", "--redis-url", url] + with pytest.raises(DeployError, match="unusable port"): + _run(argv, _native_context()) + assert not (tmp_path / ".env").exists(), "it refuses before writing settings the worker would read" + @pytest.mark.parametrize("url", ["localhost:6379", "redis+socket:///var/run/redis.sock", "redis://localhost:6379/queue"]) def test_a_redis_url_that_cannot_be_numbered_is_refused(self, tmp_path, url): From 06f233925b3e8dd078d212d5238c71269042d0eb Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 17 Sep 2026 00:44:19 +0100 Subject: [PATCH 17/17] fix: refuse a Redis URL on port zero parts.port returns 0 rather than raising, since 0 is inside the range it checks, so the URL reached .env and the worker and cache had nothing to connect to. --- docsgpt/deploy/commands.py | 8 +++++++- tests/deploy/test_native.py | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index db114639..0d95655f 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -216,12 +216,18 @@ def _redis_urls(base: str) -> dict[str, str]: # urlsplit raises on things like redis://[::1 ; that is a typo, not a crash. raise DeployError(f"the Redis URL {base!r} could not be read: {exc}") from exc try: - _ = parts.port # a non-numeric or out-of-range port raises here, not when the URL is split + port = parts.port # a non-numeric or out-of-range port raises here, not when the URL is split except ValueError as exc: raise DeployError( f"the Redis URL {base!r} has an unusable port: {exc}. Pass a URL like " "redis://host:6379 or redis://host:6379/5." ) from exc + if port == 0: + # urlsplit is happy with it, since 0 is inside the range, but nothing can connect to it. + raise DeployError( + f"the Redis URL {base!r} has an unusable port: 0. Pass a URL like " + "redis://host:6379 or redis://host:6379/5." + ) if parts.scheme not in ("redis", "rediss"): raise DeployError( f"the Redis URL {base!r} should start with redis:// or rediss://, with any options as " diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index a9335068..3c6341f7 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -231,7 +231,8 @@ class TestNativeUp: with pytest.raises(DeployError, match="too long to read"): _run(argv, _native_context()) - @pytest.mark.parametrize("url", ["redis://localhost:notaport/0", "redis://localhost:65536/0"]) + @pytest.mark.parametrize("url", ["redis://localhost:notaport/0", "redis://localhost:65536/0", + "redis://localhost:0/0"]) def test_a_redis_url_with_an_unusable_port_is_refused(self, tmp_path, url): """urlsplit accepts it and rebuilds it verbatim; only .port notices, and the worker dies later.""" argv = ["up", "--native", "--dir", str(tmp_path), "--yes",