fix(core): serialise record_installation, format the credential-store 500 (#1085)

codeframe/core/atomic_io.py

@@ -24,11 +24,21 @@
 """
 
 import json
+import logging
 import os
 import tempfile
+import threading
+from contextlib import contextmanager
 from pathlib import Path
 from typing import Any, Union
 
+try:  # optional, exactly as credentials.py treats it
+    from filelock import FileLock
+except ImportError:  # pragma: no cover - exercised by the no-filelock path
+    FileLock = None  # type: ignore[assignment]
+
+logger = logging.getLogger(__name__)
+
 __all__ = [
     "atomic_write_bytes",
     "atomic_write_text",
@@ -148,3 +158,57 @@ def atomic_write_text(
 def atomic_write_json(path: Union[str, Path], payload: Any, mode: int | None = None) -> None:
     """Durably replace ``path`` with ``payload`` serialized as indented JSON."""
     atomic_write_text(path, json.dumps(payload, indent=2), mode=mode)
+
+
+# ---------------------------------------------------------------------------
+# Serialising a read-modify-write
+# ---------------------------------------------------------------------------
+#
+# An atomic write stops a crash from truncating the file. It does NOT stop two
+# writers from losing each other's work: both read the same base, both write a
+# whole new file, and the second one wins (#1085). Anything doing
+# read-modify-write on a shared file needs this as well as atomic_write_*.
+#
+# `filelock` is optional. Without it this degrades to thread-only
+# serialisation, which is honest for a single-process CLI and logged once so
+# the weaker guarantee is not silent.
+
+_THREAD_LOCKS: dict[Path, threading.Lock] = {}
+_FILELOCK_WARNED = False
+
+
+def read_modify_write_lock(lock_path: Union[str, Path]):
+    """Serialise a read-modify-write keyed on ``lock_path``.
+
+    Returns a context manager holding a process-wide thread lock and, when
+    ``filelock`` is installed, a cross-process file lock on the same path.
+
+    ``lock_path`` is the lock file itself, not the data file — callers pass
+    something like ``dir / ".history.lock"``.
+    """
+    return _ReadModifyWriteLock(Path(lock_path))
+
+
+@contextmanager
+def _ReadModifyWriteLock(lock_path: Path):  # noqa: N802 - reads as a class at call sites
+    global _FILELOCK_WARNED
+
+    lock_path.parent.mkdir(parents=True, exist_ok=True)
+    thread_lock = _THREAD_LOCKS.setdefault(lock_path, threading.Lock())
+
+    file_lock = None
+    if FileLock is not None:
+        file_lock = FileLock(str(lock_path))
+    elif not _FILELOCK_WARNED:
+        _FILELOCK_WARNED = True
+        logger.warning(
+            "filelock is not installed — read-modify-write is serialised within "
+            "this process only. Concurrent processes can still lose an entry."
+        )
+
+    with thread_lock:
+        if file_lock is None:
+            yield
+        else:
+            with file_lock:
+                yield

codeframe/core/installer.py

@@ -27,7 +27,7 @@
 from pathlib import Path
 from typing import Optional
 
-from codeframe.core.atomic_io import atomic_write_json
+from codeframe.core.atomic_io import atomic_write_json, read_modify_write_lock
 
 logger = logging.getLogger(__name__)
 
@@ -614,32 +614,38 @@ def record_installation(self, result: InstallResult) -> None:
         """
         self.history_dir.mkdir(parents=True, exist_ok=True)
 
-        # Load existing history. The file is on disk and can be anything —
-        # hand-edited, truncated, or written by an older version — and this runs
-        # AFTER a successful install, so a bad shape here must not turn a working
-        # install into a crash (#954). Anything that is not the expected
-        # dict-of-dicts is treated as "no usable history" rather than indexed
-        # into (a list or a string raised TypeError).
-        installations: dict = {}
-        if self.history_file.exists():
-            try:
-                with open(self.history_file, encoding="utf-8") as f:
-                    loaded = json.load(f)
-            except (json.JSONDecodeError, OSError, UnicodeDecodeError):
-                loaded = None
-            if isinstance(loaded, dict) and isinstance(loaded.get("installations"), dict):
-                installations = loaded["installations"]
-
-        installations[result.tool_name] = {
-            "status": result.status.value,
-            "installed_at": datetime.now(UTC).isoformat(),
-            "command": result.command_used,
-            "message": result.message,
-        }
-
-        # Atomic: an in-place rewrite truncated the history first, so a crash
-        # mid-write lost every previously recorded install (#954).
-        atomic_write_json(self.history_file, {"installations": installations})
+        # Serialised, not just atomic (#1085). atomic_write_json stops a crash
+        # truncating the file; it does not stop two concurrent installs reading
+        # the same base and the second write dropping the first tool. Matters
+        # once `cf env install` runs tools in parallel.
+        with read_modify_write_lock(self.history_dir / ".history.lock"):
+
+            # Load existing history. The file is on disk and can be anything —
+            # hand-edited, truncated, or written by an older version — and this runs
+            # AFTER a successful install, so a bad shape here must not turn a working
+            # install into a crash (#954). Anything that is not the expected
+            # dict-of-dicts is treated as "no usable history" rather than indexed
+            # into (a list or a string raised TypeError).
+            installations: dict = {}
+            if self.history_file.exists():
+                try:
+                    with open(self.history_file, encoding="utf-8") as f:
+                        loaded = json.load(f)
+                except (json.JSONDecodeError, OSError, UnicodeDecodeError):
+                    loaded = None
+                if isinstance(loaded, dict) and isinstance(loaded.get("installations"), dict):
+                    installations = loaded["installations"]
+
+            installations[result.tool_name] = {
+                "status": result.status.value,
+                "installed_at": datetime.now(UTC).isoformat(),
+                "command": result.command_used,
+                "message": result.message,
+            }
+
+            # Atomic: an in-place rewrite truncated the history first, so a crash
+            # mid-write lost every previously recorded install (#954).
+            atomic_write_json(self.history_file, {"installations": installations})
 
     def get_installation_history(self) -> dict:
         """Get the installation history.

codeframe/ui/routers/github_integrations_v2.py

@@ -27,7 +27,11 @@
 
 from codeframe.auth.api_keys import SCOPE_ADMIN
 from codeframe.auth.dependencies import require_auth, require_scope
-from codeframe.core.credentials import CredentialManager, CredentialProvider
+from codeframe.core.credentials import (
+    CredentialManager,
+    CredentialProvider,
+    CredentialStoreUnreadableError,
+)
 from codeframe.core.github_connect_service import (
     GitHubConnectError,
     InsufficientScopeError,
@@ -53,7 +57,7 @@
 from codeframe.core.workspace import Workspace
 from codeframe.lib.rate_limiter import rate_limit_ai, rate_limit_standard
 from codeframe.ui.dependencies import get_v2_workspace, resolve_github_pat
-from codeframe.ui.response_models import ErrorCodes, api_error
+from codeframe.ui.response_models import ErrorCodes, api_error, internal_error
 
 logger = logging.getLogger(__name__)
 
@@ -67,7 +71,27 @@ def get_credential_manager(auth: dict = Depends(require_auth)) -> CredentialMana
     store. Overridden in tests to point at an isolated temp directory.
     Runs the machine-wide migration — use only on write paths.
     """
-    return CredentialManager(user_id=auth.get("user_id"), migrate=True)
+    # CredentialManager's constructor runs the machine-wide migration, which
+    # can raise CredentialStoreUnreadableError since #954. Raised from a
+    # DEPENDENCY it bypasses each route's own try/except, so the client got a
+    # bare 500 instead of the formatted error every other path produces (#1085).
+    # The exception's message carries the recovery text the CLI already prints.
+    try:
+        return CredentialManager(user_id=auth.get("user_id"), migrate=True)
+    except CredentialStoreUnreadableError as e:
+        # internal_error, NOT str(e) (#934): the exception message embeds the
+        # absolute store path — /home/<operator>/.codeframe/users/<id>/... —
+        # so rendering it would hand an authenticated tenant the operator's
+        # home directory and the per-tenant storage layout. The full message
+        # goes to the operator's log under the correlation id; the client gets
+        # the recovery step, which is the part that is actually actionable and
+        # contains no path.
+        body = internal_error(e, operation="read the credential store", logger=logger)
+        body["detail"] += (
+            " The credential store could not be read; re-enter your keys with "
+            "`cf auth setup`."
+        )
+        raise HTTPException(status_code=500, detail=body)
 
 
 def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> CredentialManager:
@@ -76,7 +100,27 @@ def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> Crede
     Used on GET endpoints so that a plain status check cannot trigger a
     credential write into a new tenant's store (#790).
     """
-    return CredentialManager(user_id=auth.get("user_id"), migrate=False)
+    # CredentialManager's constructor runs the machine-wide migration, which
+    # can raise CredentialStoreUnreadableError since #954. Raised from a
+    # DEPENDENCY it bypasses each route's own try/except, so the client got a
+    # bare 500 instead of the formatted error every other path produces (#1085).
+    # The exception's message carries the recovery text the CLI already prints.
+    try:
+        return CredentialManager(user_id=auth.get("user_id"), migrate=False)
+    except CredentialStoreUnreadableError as e:
+        # internal_error, NOT str(e) (#934): the exception message embeds the
+        # absolute store path — /home/<operator>/.codeframe/users/<id>/... —
+        # so rendering it would hand an authenticated tenant the operator's
+        # home directory and the per-tenant storage layout. The full message
+        # goes to the operator's log under the correlation id; the client gets
+        # the recovery step, which is the part that is actually actionable and
+        # contains no path.
+        body = internal_error(e, operation="read the credential store", logger=logger)
+        body["detail"] += (
+            " The credential store could not be read; re-enter your keys with "
+            "`cf auth setup`."
+        )
+        raise HTTPException(status_code=500, detail=body)
 
 
 class ConnectRequest(BaseModel):

codeframe/ui/routers/settings_v2.py

@@ -40,6 +40,7 @@
     save_environment_config,
 )
 from codeframe.core.credentials import (
+    CredentialStoreUnreadableError,
     CredentialManager,
     CredentialProvider,
     CredentialSource,
@@ -72,7 +73,7 @@
     VerifyKeyRequest,
     VerifyKeyResponse,
 )
-from codeframe.ui.response_models import ErrorCodes, api_error
+from codeframe.ui.response_models import ErrorCodes, api_error, internal_error
 from codeframe.core.notifications_config import redact_webhook_url
 
 logger = logging.getLogger(__name__)
@@ -87,7 +88,27 @@ def get_credential_manager(auth: dict = Depends(require_auth)) -> CredentialMana
     store. Overridden in tests to point at an isolated temp directory.
     Runs the machine-wide migration — use only on write (admin-scoped) paths.
     """
-    return CredentialManager(user_id=auth.get("user_id"), migrate=True)
+    # CredentialManager's constructor runs the machine-wide migration, which
+    # can raise CredentialStoreUnreadableError since #954. Raised from a
+    # DEPENDENCY it bypasses each route's own try/except, so the client got a
+    # bare 500 instead of the formatted error every other path produces (#1085).
+    # The exception's message carries the recovery text the CLI already prints.
+    try:
+        return CredentialManager(user_id=auth.get("user_id"), migrate=True)
+    except CredentialStoreUnreadableError as e:
+        # internal_error, NOT str(e) (#934): the exception message embeds the
+        # absolute store path — /home/<operator>/.codeframe/users/<id>/... —
+        # so rendering it would hand an authenticated tenant the operator's
+        # home directory and the per-tenant storage layout. The full message
+        # goes to the operator's log under the correlation id; the client gets
+        # the recovery step, which is the part that is actually actionable and
+        # contains no path.
+        body = internal_error(e, operation="read the credential store", logger=logger)
+        body["detail"] += (
+            " The credential store could not be read; re-enter your keys with "
+            "`cf auth setup`."
+        )
+        raise HTTPException(status_code=500, detail=body)
 
 
 def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> CredentialManager:
@@ -96,7 +117,27 @@ def get_credential_manager_readonly(auth: dict = Depends(require_auth)) -> Crede
     Used on GET endpoints so that a plain status check cannot trigger a
     credential write into a new tenant's store (#790).
     """
-    return CredentialManager(user_id=auth.get("user_id"), migrate=False)
+    # CredentialManager's constructor runs the machine-wide migration, which
+    # can raise CredentialStoreUnreadableError since #954. Raised from a
+    # DEPENDENCY it bypasses each route's own try/except, so the client got a
+    # bare 500 instead of the formatted error every other path produces (#1085).
+    # The exception's message carries the recovery text the CLI already prints.
+    try:
+        return CredentialManager(user_id=auth.get("user_id"), migrate=False)
+    except CredentialStoreUnreadableError as e:
+        # internal_error, NOT str(e) (#934): the exception message embeds the
+        # absolute store path — /home/<operator>/.codeframe/users/<id>/... —
+        # so rendering it would hand an authenticated tenant the operator's
+        # home directory and the per-tenant storage layout. The full message
+        # goes to the operator's log under the correlation id; the client gets
+        # the recovery step, which is the part that is actually actionable and
+        # contains no path.
+        body = internal_error(e, operation="read the credential store", logger=logger)
+        body["detail"] += (
+            " The credential store could not be read; re-enter your keys with "
+            "`cf auth setup`."
+        )
+        raise HTTPException(status_code=500, detail=body)
 
 
 def _config_to_response(config: EnvironmentConfig) -> AgentSettingsResponse: