fix: atomic writes for cache and config files

changelog.d/20260515_140000_mattis.dalleau_atomic_cache_writes.md

@@ -0,0 +1,11 @@
+### Fixed
+
+- Cache and config writes (`ai_discovery.json`, `update_check.yaml`,
+  `auth_config.yaml`, the secret-scan cache, the docker-image ID cache and the
+  plugin manifest) now use an atomic write-then-rename pattern with a
+  per-process temp file. Previously these were truncate-in-place writes
+  (or used a fixed `.tmp` filename), which would corrupt cache contents when
+  several ggshield processes ran concurrently — including the `secret scan
+ai-hook` codepath invoked from AI-agent hooks. A torn write on a cache
+  file another process has memory-mapped surfaces as `SIGBUS` and crashes
+  the reader, which is what triggered this fix.

ggshield/core/cache.py

@@ -1,12 +1,12 @@
 import json
-import os
 from pathlib import Path
 from typing import Any, Dict, List
 
 from ggshield.core import ui
 from ggshield.core.constants import CACHE_PATH
 from ggshield.core.errors import UnexpectedError
 from ggshield.core.types import IgnoredMatch
+from ggshield.utils.files import atomic_write_text
 from ggshield.utils.git_shell import gitignore, is_gitignored
 
 
@@ -61,20 +61,16 @@ def save(self) -> None:
             # if there are no found secrets, don't modify the cache file
             return
         try:
-            fd = os.open(
-                str(self.cache_path), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600
+            text = json.dumps(self.to_dict())
+        except Exception as e:
+            raise UnexpectedError(
+                f"Failed to save cache in {self.cache_path}:\n{str(e)}"
             )
-            f = os.fdopen(fd, "w")
+        try:
+            atomic_write_text(self.cache_path, text, mode=0o600)
         except OSError:
             # Hotfix: for the time being we skip cache handling if permission denied
             return
-        with f:
-            try:
-                json.dump(self.to_dict(), f)
-            except Exception as e:
-                raise UnexpectedError(
-                    f"Failed to save cache in {self.cache_path}:\n{str(e)}"
-                )
 
     def purge(self) -> None:
         self.last_found_secrets = []

ggshield/core/config/utils.py

@@ -12,6 +12,7 @@
 )
 from ggshield.core.dirs import get_config_dir, get_project_root_dir, get_user_home_dir
 from ggshield.core.errors import UnexpectedError
+from ggshield.utils.files import atomic_write_text
 from ggshield.utils.git_shell import GitExecutableNotFound
 
 
@@ -63,18 +64,11 @@ def save_yaml_dict(
     data: Dict[str, Any], path: Union[str, Path], restricted: bool = False
 ) -> None:
     p = Path(path)
-    p.parent.mkdir(parents=True, exist_ok=True)
-    with p.open("w") as f:
-        try:
-            if restricted:
-                # Restrict file permissions: read and write for owner only (600)
-                p.chmod(0o600)
-
-            stream = yaml.dump(data, indent=2, default_flow_style=False)
-            f.write(stream)
-
-        except Exception as e:
-            raise UnexpectedError(f"Failed to save config to {path}:\n{str(e)}") from e
+    try:
+        stream = yaml.dump(data, indent=2, default_flow_style=False)
+        atomic_write_text(p, stream, mode=0o600 if restricted else 0o644)
+    except Exception as e:
+        raise UnexpectedError(f"Failed to save config to {path}:\n{str(e)}") from e
 
 
 def get_auth_config_filepath() -> Path:

ggshield/core/plugin/downloader.py

@@ -30,6 +30,7 @@
 )
 from ggshield.core.plugin.trust import PluginTrustStore
 from ggshield.core.plugin.wheel_utils import WheelError, extract_wheel_metadata
+from ggshield.utils.files import atomic_write_text
 
 
 logger = logging.getLogger(__name__)
@@ -732,13 +733,7 @@ def _write_manifest(
             manifest["signature"] = sig_data
 
         manifest_path = plugin_dir / "manifest.json"
-        tmp_path = manifest_path.with_suffix(".json.tmp")
-        try:
-            tmp_path.write_text(json.dumps(manifest, indent=2))
-            tmp_path.replace(manifest_path)
-        finally:
-            if tmp_path.exists():
-                tmp_path.unlink()
+        atomic_write_text(manifest_path, json.dumps(manifest, indent=2))
 
     def _sync_trust_record(
         self,

ggshield/core/scan/id_cache.py

@@ -3,6 +3,8 @@
 from pathlib import Path
 from typing import Set
 
+from ggshield.utils.files import atomic_write_text
+
 
 logger = logging.getLogger(__name__)
 
@@ -34,11 +36,7 @@ def _load(self) -> None:
         self._ids = set(ids)
 
     def _save(self) -> None:
-        text = json.dumps(list(self._ids))
         try:
-            self.cache_path.parent.mkdir(parents=True, exist_ok=True)
-            tmp = self.cache_path.with_suffix(".tmp")
-            tmp.write_text(text)
-            tmp.replace(self.cache_path)
+            atomic_write_text(self.cache_path, json.dumps(list(self._ids)))
         except Exception as exc:
             logger.warning("Failed to save cache to %s: %s", self.cache_path, exc)

ggshield/utils/files.py

@@ -1,3 +1,5 @@
+import os
+import time
 from enum import Enum, auto
 from pathlib import Path, PurePath, PurePosixPath
 from typing import List, Pattern, Set, Tuple, Union
@@ -145,3 +147,98 @@ def url_for_path(path: PurePath) -> str:
     else:
         # This happens for Windows paths: `path_str` is something like "c:/foo/bar"
         return f"file:///{path_str}"
+
+
+def _open_new_sibling(path: Path, mode: int) -> Tuple[int, Path]:
+    """Create a unique sibling of *path* with ``O_CREAT | O_EXCL`` and return
+    its open fd and path. Used as the staging file for atomic writes — it is
+    the new version of *path*, not a tempfile."""
+    while True:
+        suffix = os.urandom(8).hex()
+        new_path = path.with_name(f".{path.name}.{suffix}.new")
+        try:
+            fd = os.open(
+                new_path,
+                os.O_WRONLY | os.O_CREAT | os.O_EXCL,
+                mode,
+            )
+            return fd, new_path
+        except FileExistsError:
+            # Astronomically unlikely with 64 bits of entropy; retry rather
+            # than swallow.
+            continue
+
+
+def _replace_with_retry(src: Path, dst: Path) -> None:
+    """``os.replace`` with Windows-friendly retries.
+
+    On POSIX, ``rename`` succeeds even when other processes hold the
+    destination open. On Windows, the rename fails with ``PermissionError``
+    while any handle to the destination is open. Under concurrent writers,
+    those handles are brief, so a short retry-with-backoff converges. POSIX
+    almost always succeeds on the first try, so the loop has no overhead
+    there.
+    """
+    delay = 0.005
+    attempts = 10
+    for attempt in range(attempts):
+        try:
+            os.replace(src, dst)
+            return
+        except PermissionError:
+            if attempt == attempts - 1:
+                raise
+            time.sleep(delay)
+            delay = min(delay * 2, 0.1)
+
+
+def atomic_write_text(
+    path: Path,
+    text: str,
+    *,
+    mode: int = 0o644,
+    encoding: str = "utf-8",
+) -> None:
+    """Atomically write *text* to *path*.
+
+    Writes a unique sibling file then ``os.replace()``s it onto *path*. POSIX
+    guarantees the rename leaves *path* pointing at either the previous
+    content or the new content, never a partial write — which prevents
+    readers (especially native code that mmaps) from observing a torn file
+    during concurrent writes.
+    """
+    path = Path(path)
+    path.parent.mkdir(parents=True, exist_ok=True)
+    fd, new_path = _open_new_sibling(path, mode)
+    try:
+        # O_CREAT mode is umask-masked; force the requested mode exactly.
+        # Path-based chmod (not fchmod) so this works under pyfakefs in tests.
+        os.chmod(new_path, mode)
+        with os.fdopen(fd, "w", encoding=encoding) as f:
+            f.write(text)
+        _replace_with_retry(new_path, path)
+    except BaseException:
+        try:
+            os.unlink(new_path)
+        except OSError:
+            pass
+        raise
+
+
+def atomic_write_bytes(path: Path, data: bytes, *, mode: int = 0o644) -> None:
+    """Atomically write *data* to *path*. See :func:`atomic_write_text`."""
+    path = Path(path)
+    path.parent.mkdir(parents=True, exist_ok=True)
+    fd, new_path = _open_new_sibling(path, mode)
+    try:
+        # See atomic_write_text: path-based chmod for pyfakefs / Windows compat.
+        os.chmod(new_path, mode)
+        with os.fdopen(fd, "wb") as f:
+            f.write(data)
+        _replace_with_retry(new_path, path)
+    except BaseException:
+        try:
+            os.unlink(new_path)
+        except OSError:
+            pass
+        raise

ggshield/verticals/ai/cache.py

@@ -5,6 +5,7 @@
 from pygitguardian.models import AIDiscovery
 
 from ggshield.core.dirs import get_cache_dir
+from ggshield.utils.files import atomic_write_text
 
 from .models import MCPConfiguration, MCPServer, Scope
 
@@ -18,8 +19,7 @@ def save_discovery_cache(config: AIDiscovery) -> None:
     """
     cache_path = get_cache_dir() / AI_DISCOVERY_CACHE_FILENAME
     try:
-        cache_path.parent.mkdir(parents=True, exist_ok=True)
-        cache_path.write_text(json.dumps(config.to_dict(), indent=4))
+        atomic_write_text(cache_path, json.dumps(config.to_dict(), indent=4))
     except OSError:
         pass
 

ggshield/verticals/ai/user.py

@@ -19,6 +19,7 @@
 from pygitguardian.models import UserInfo
 
 from ggshield.core.dirs import get_user_home_dir
+from ggshield.utils.files import atomic_write_text
 
 
 logger = logging.getLogger(__name__)
@@ -195,8 +196,7 @@ def _get_machine_id() -> str:
     # Store it so that satori can use it.
     new_id = str(uuid.uuid4())
     try:
-        path.parent.mkdir(parents=True, exist_ok=True)
-        path.write_text(new_id + "\n")
+        atomic_write_text(path, new_id + "\n")
     except OSError:
         pass
 

tests/unit/core/plugin/test_downloader.py

@@ -1642,13 +1642,17 @@ def test_manifest_omits_signature_when_none(self, tmp_path: Path) -> None:
         assert "signature" not in manifest
 
     def test_tmp_file_is_cleaned_up_on_rename_failure(self, tmp_path: Path) -> None:
-        """If replace() fails, the .tmp file is removed and no manifest is left."""
+        """If the atomic rename fails, the temp file is removed and no
+        manifest is left behind."""
         with patch(
             "ggshield.core.plugin.downloader.get_plugins_dir", return_value=tmp_path
         ):
             downloader = PluginDownloader()
 
-        with patch("pathlib.Path.replace", side_effect=OSError("cross-device rename")):
+        with patch(
+            "ggshield.utils.files.os.replace",
+            side_effect=OSError("cross-device rename"),
+        ):
             with pytest.raises(OSError):
                 downloader._write_manifest(
                     plugin_dir=tmp_path,
@@ -1660,7 +1664,9 @@ def test_tmp_file_is_cleaned_up_on_rename_failure(self, tmp_path: Path) -> None:
                 )
 
         assert not (tmp_path / "manifest.json").exists()
-        assert not (tmp_path / "manifest.json.tmp").exists()
+        # No leftover temp file (atomic_write_text uses a unique
+        # tempfile.mkstemp name and cleans it up on failure).
+        assert list(tmp_path.iterdir()) == []
 
 
 def _create_artifact_zip(content: bytes, filename: str) -> bytes: