Merge feat/install-4-backup into feat/install-5-native

docs/content/Deploying/Docker-Deploying.mdx

@@ -89,6 +89,12 @@ docsgpt backup                      # into <stack>/backups
 docsgpt backup --out /mnt/backups   # somewhere else, e.g. a mounted disk
 ```
 
+While the archive is made, the backend and the worker stop and start again, so
+the database dump and the files in the volumes describe the same moment; Postgres
+itself keeps running. On a small install that pause is seconds, but count on it
+if you run `docsgpt backup` from cron. The archive is written readable only by
+the user who took it.
+
 The archive does **not** include `.env`, because that file holds the install's
 secrets. `docsgpt backup --with-settings` puts it in, for when the archive
 itself is stored somewhere private. Keep `.env` safe separately otherwise: the

docs/content/changelog.mdx

@@ -26,7 +26,8 @@ the same way they do on a Docker one, and never touch the database or Redis. See
 `docsgpt backup` writes a dump of the database and a tar of each data volume into one archive, and
 `docsgpt restore <archive>` puts them back. The settings file is left out unless
 `--with-settings` asks for it, since it holds the install's secrets, and a backup from a newer
-DocsGPT is refused unless you pass `--force`. See
+DocsGPT is refused unless you pass `--force`. The archive is written readable only by its owner,
+and the backend and the worker pause while it is made so the dump and the volume tars match. See
 [Backups](/Deploying/Docker-Deploying#backups).
 
 ## 0.21.0

docsgpt/deploy/backup.py

@@ -13,6 +13,7 @@ from __future__ import annotations
 
 import io
 import json
+import os
 import tarfile
 import time
 from collections.abc import Mapping
@@ -50,7 +51,10 @@ def write_archive(
 ) -> None:
     """Write the backup archive; the manifest is added last so a truncated file has none."""
     path.parent.mkdir(parents=True, exist_ok=True)
-    with tarfile.open(path, "w:gz") as archive:
+    # 0600 from the start: the dump is the install's data, and --with-settings adds its secrets.
+    descriptor = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
+    os.fchmod(descriptor, 0o600)
+    with os.fdopen(descriptor, "wb") as handle, tarfile.open(fileobj=handle, mode="w:gz") as archive:
         archive.add(dump, arcname=DUMP)
         for name, tar_path in sorted(volume_tars.items()):
             archive.add(tar_path, arcname=volume_member(name))
@@ -81,6 +85,34 @@ def read_manifest(path: Path) -> dict:
     return manifest
 
 
+def validate(path: Path, manifest: Mapping[str, object]) -> list[str]:
+    """The volumes to restore, once the archive is known to hold everything it declares.
+
+    Checked before the stack is stopped: a damaged or hand-made archive must fail while DocsGPT is
+    still running, not after ``docker compose down``. Only the volumes a backup is made of are
+    accepted, so a manifest cannot name ``postgres_data`` and have it emptied on the way in.
+    """
+    volumes = manifest.get("volumes")
+    if not isinstance(volumes, list) or not all(isinstance(name, str) for name in volumes):
+        raise DeployError(f"{path} is not a DocsGPT backup: its manifest does not list the volumes it holds")
+    unsupported = sorted(set(volumes) - set(DATA_VOLUMES))
+    if unsupported:
+        raise DeployError(
+            f"{path} names volumes that are not part of a backup: {', '.join(unsupported)}. "
+            f"A DocsGPT backup holds {', '.join(DATA_VOLUMES)}."
+        )
+    required = [DUMP, *(volume_member(name) for name in volumes)]
+    try:
+        with tarfile.open(path, "r:gz") as archive:
+            present = {member.name for member in archive.getmembers() if member.isfile()}
+    except (tarfile.TarError, OSError) as exc:
+        raise DeployError(f"{path} is not a DocsGPT backup: {exc}") from exc
+    missing = [name for name in required if name not in present]
+    if missing:
+        raise DeployError(f"{path} is missing {', '.join(missing)}, so there is nothing to restore from")
+    return list(volumes)
+
+
 def extract(path: Path, destination: Path) -> None:
     """Unpack the archive into ``destination`` (data filter: no paths outside it, no devices)."""
     with tarfile.open(path, "r:gz") as archive:

docsgpt/deploy/commands.py

@@ -527,6 +527,26 @@ def backup(args, context: Optional[Context] = None) -> int:
     target = out_dir / backup_format.archive_name(taken_at)
     image = _stack_image(env)
 
+    print("Pausing the backend and the worker so the database and the files match ...")
+    context.docker.compose(directory, "stop", "backend", "worker")
+    try:
+        _write_backup(args, context, directory, env, target, image, taken_at)
+    finally:
+        print("Starting the backend and the worker again ...")
+        context.docker.compose(directory, "up", "-d", "backend", "worker")
+
+    size = target.stat().st_size / 1_000_000
+    print(f"\nBackup written to {target} ({size:.1f} MB)")
+    if args.with_settings:
+        print("It contains .env, so it holds this install's secrets: keep it somewhere private.")
+    else:
+        print("Settings are not in it; `docsgpt backup --with-settings` includes .env, secrets and all.")
+    print(f"Restore it with: docsgpt restore {target}")
+    return 0
+
+
+def _write_backup(args, context: Context, directory: Path, env, target: Path, image: str, taken_at) -> None:
+    """Dump the database and the data volumes into ``target``, with the writers stopped."""
     with tempfile.TemporaryDirectory() as workspace:
         work = Path(workspace)
         dump = work / backup_format.DUMP
@@ -554,15 +574,6 @@ def backup(args, context: Optional[Context] = None) -> int:
         settings = directory / ".env" if args.with_settings else None
         backup_format.write_archive(target, dump=dump, volume_tars=volume_tars, manifest=manifest, settings=settings)
 
-    size = target.stat().st_size / 1_000_000
-    print(f"\nBackup written to {target} ({size:.1f} MB)")
-    if args.with_settings:
-        print("It contains .env, so it holds this install's secrets: keep it somewhere private.")
-    else:
-        print("Settings are not in it; `docsgpt backup --with-settings` includes .env, secrets and all.")
-    print(f"Restore it with: docsgpt restore {target}")
-    return 0
-
 
 def restore(args, context: Optional[Context] = None) -> int:
     """Put a backup's database and data volumes back over this install."""
@@ -570,6 +581,8 @@ def restore(args, context: Optional[Context] = None) -> int:
     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)
@@ -592,7 +605,7 @@ def restore(args, context: Optional[Context] = None) -> int:
     with tempfile.TemporaryDirectory() as workspace:
         work = Path(workspace)
         backup_format.extract(archive, work)
-        for name in manifest.get("volumes", []):
+        for name in volumes:
             tar_path = work / backup_format.volume_member(name)
             if not tar_path.is_file():
                 raise DeployError(f"{archive} is missing the {name} volume it says it contains")
@@ -608,7 +621,7 @@ def restore(args, context: Optional[Context] = None) -> int:
         with dump.open("r", encoding="utf-8") as handle:
             context.docker.compose(
                 directory, "exec", "-T", "postgres",
-                "psql", "--quiet", "-U", "docsgpt", "-d", "docsgpt",
+                "psql", "--quiet", "--set", "ON_ERROR_STOP=on", "-U", "docsgpt", "-d", "docsgpt",
                 stdin=handle,
             )
 

docsgpt/deploy/docker.py

@@ -119,8 +119,11 @@ class Docker:
         """Replace ``volume``'s contents with the tar at ``source``; the volume is created when missing."""
         with source.open("rb") as handle:
             self._run(
-                ["docker", "run", "--rm", "-i", "-v", f"{volume}:/data", image,
-                 "sh", "-c", "find /data -mindepth 1 -delete && tar xf - -C /data"],
+                ["docker", "run", "--rm", "-i", "-v", f"{volume}:/data", image, "sh", "-c",
+                 # Unpack into the container's own filesystem first: a truncated or corrupt tar must
+                 # fail before the live volume is touched, not halfway through emptying it.
+                 "set -e; rm -rf /stage; mkdir /stage; tar xf - -C /stage; "
+                 "find /data -mindepth 1 -delete; tar cf - -C /stage . | tar xf - -C /data"],
                 stdin=handle,
             )
 

tests/deploy/test_backup.py

@@ -2,6 +2,7 @@
 
 import io
 import json
+import stat
 import tarfile
 
 import pytest
@@ -26,6 +27,22 @@ def _manifest(path):
         return json.loads(archive.extractfile(backup_module.MANIFEST).read())
 
 
+def _rewrite(archive, dest, *, volumes=None, drop=()):
+    """The same archive with other volumes in its manifest, or with members left out."""
+    with tarfile.open(archive, "r:gz") as source, tarfile.open(dest, "w:gz") as out:
+        for member in source.getmembers():
+            if member.name in drop:
+                continue
+            body = source.extractfile(member).read() if member.isfile() else None
+            if member.name == backup_module.MANIFEST and volumes is not None:
+                manifest = json.loads(body.decode())
+                manifest["volumes"] = volumes
+                body = json.dumps(manifest).encode()
+                member.size = len(body)
+            out.addfile(member, io.BytesIO(body) if body is not None else None)
+    return dest
+
+
 def _copy_with_version(archive, version, dest):
     """The same archive, with another DocsGPT version written into its manifest."""
     with tarfile.open(archive, "r:gz") as source, tarfile.open(dest, "w:gz") as out:
@@ -80,6 +97,40 @@ class TestBackup:
         assert dumps, docker.calls
         assert dumps[0][:3] == ["exec", "-T", "postgres"]
 
+    def test_the_archive_is_private_to_whoever_took_it(self, tmp_path):
+        """It can hold .env, and the dump is the install's data either way."""
+        _installed(tmp_path)
+        out = tmp_path / "backups"
+        argv = ["backup", "--dir", str(tmp_path), "--out", str(out), "--with-settings"]
+        assert _run(argv, _context()) == 0
+        archive = next(out.glob("*.tar.gz"))
+        assert stat.S_IMODE(archive.stat().st_mode) == 0o600
+
+    def test_the_writers_are_stopped_while_the_archive_is_made(self, tmp_path):
+        """Ingestion writes files and rows; a dump taken alongside live writes would not match them."""
+        _installed(tmp_path)
+        docker = FakeDocker()
+        assert _run(["backup", "--dir", str(tmp_path), "--out", str(tmp_path / "b")], _context(docker)) == 0
+        joined = [" ".join(args) for _, args in docker.calls]
+        stopped = next(index for index, call in enumerate(joined) if call.startswith("stop backend worker"))
+        dumped = next(index for index, call in enumerate(joined) if "pg_dump" in call)
+        started = next(index for index, call in enumerate(joined) if call.startswith("up -d backend worker"))
+        assert stopped < dumped < started, joined
+
+    def test_the_writers_come_back_even_when_the_dump_fails(self, tmp_path):
+        _installed(tmp_path)
+
+        class FailingDump(FakeDocker):
+            def compose(self, directory, *args, **kwargs):
+                if "pg_dump" in args:
+                    raise DeployError("pg_dump exploded")
+                return super().compose(directory, *args, **kwargs)
+
+        docker = FailingDump()
+        with pytest.raises(DeployError, match="pg_dump exploded"):
+            _run(["backup", "--dir", str(tmp_path), "--out", str(tmp_path / "b")], _context(docker))
+        assert any(" ".join(args).startswith("up -d backend worker") for _, args in docker.calls), docker.calls
+
     def test_without_an_install(self, tmp_path, capsys):
         assert _run(["backup", "--dir", str(tmp_path)], _context()) == 1
         assert "docsgpt up" in capsys.readouterr().err
@@ -122,6 +173,31 @@ class TestRestore:
         with pytest.raises(DeployError, match="does not exist"):
             _run(["restore", str(tmp_path / "nope.tar.gz"), "--dir", str(tmp_path), "--yes"], _context())
 
+    def test_a_manifest_naming_another_volume_is_refused(self, tmp_path):
+        """import_volume empties what it is given, so a hand-made manifest must not name postgres_data."""
+        crafted = _rewrite(self._backup(tmp_path), tmp_path / "crafted.tar.gz", volumes=["postgres_data"])
+        docker = FakeDocker(volumes={"docsgpt_postgres_data"})
+        with pytest.raises(DeployError, match="not part of a backup"):
+            _run(["restore", str(crafted), "--dir", str(tmp_path), "--yes"], _context(docker))
+        assert docker.calls == [], "nothing was stopped"
+        assert docker.volume_ops == [], "and nothing was touched"
+
+    def test_a_damaged_archive_fails_before_the_stack_is_stopped(self, tmp_path):
+        """Discovering a missing member after `compose down` would leave DocsGPT down for nothing."""
+        without_dump = _rewrite(self._backup(tmp_path), tmp_path / "nodump.tar.gz", drop=(backup_module.DUMP,))
+        docker = FakeDocker(volumes={"docsgpt_postgres_data"})
+        with pytest.raises(DeployError, match="nothing to restore"):
+            _run(["restore", str(without_dump), "--dir", str(tmp_path), "--yes"], _context(docker))
+        assert docker.calls == [], "DocsGPT is still running"
+
+    def test_psql_stops_at_the_first_failing_statement(self, tmp_path):
+        """Without it psql runs on after an error and a half-restored database looks like success."""
+        archive = self._backup(tmp_path)
+        docker = FakeDocker(volumes={"docsgpt_postgres_data"})
+        assert _run(["restore", str(archive), "--dir", str(tmp_path), "--yes"], _context(docker)) == 0
+        psql = next(args for _, args in docker.calls if "psql" in args)
+        assert "ON_ERROR_STOP=on" in psql
+
     def test_a_newer_backup_is_refused_without_force(self, tmp_path):
         """Restoring a 0.22 backup into 0.21 would hand an older schema newer data."""
         newer = _copy_with_version(self._backup(tmp_path), "0.22.0", tmp_path / "newer.tar.gz")

tests/deploy/test_docker.py

@@ -129,7 +129,10 @@ class TestVolumes:
         assert "docsgpt_inputs:/data" in args
         assert args[-2] == "-c"
         assert "tar xf - -C /data" in args[-1]
-        assert "find /data -mindepth 1 -delete" in args[-1], "old contents go first"
+        command = args[-1]
+        assert command.index("tar xf - -C /stage") < command.index("find /data -mindepth 1 -delete"), (
+            "the incoming tar is unpacked outside the volume first, so a corrupt one leaves it alone"
+        )
         assert runner.streams[0][1] is not None, "the tar is fed in on stdin"