fix(atomic): the WinError 5 race had four more copies, including the lock store (2.118.2)

CHANGELOG.md

@@ -4,6 +4,47 @@ All notable changes to Code Context Control (C3) are documented here.
 The format is loosely based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [2.118.2] - 2026-09-05
+
+### Fixed — the WinError 5 race had four more copies, including the lock store
+
+2.118.1 fixed the publish step in the shell job store: a shared `<name>.tmp`
+plus a single `os.replace`, which on Windows can publish a half-written file
+or raise `PermissionError: [WinError 5] Access is denied`. That shape had been
+copied into four other small JSON stores, and fixing one copy of a bug is not
+fixing the bug. All five now go through one helper, `services/atomic_json.py`:
+
+- `services/agent_locks.py` — the lock store, and the most exposed of the
+  four: it is written from a threaded server, `locks.json.tmp` is a name every
+  writer on the box picks, and a lost write drops live leases. That is exactly
+  what its loader refuses to do when it quarantines a corrupt file rather than
+  resetting it to empty.
+- `services/access_guard._write_scope_config` — the CLI's `config.json` writer.
+- `cli/hub_server.py` — the hub's config route, writing the same file.
+- `oracle/services/mobile_api.py` — the mobile override-config writer, ditto.
+  The last three write one file from three processes, one of them threaded.
+
+None had a reported failure. That is a statement about how often they are
+written, not about whether the race is real — the job store's version was
+also unreported until it started failing CI.
+
+### Changed — durable config is now flushed before it is published
+
+`write_json_atomic` fsyncs the temp before the replace, so a crash between the
+two cannot leave a zero-length `config.json`. The Access Guard reads a
+truncated config as `<corrupt-config>` and fails **every** path closed, which
+wedges the session that would otherwise fix it — worth a flush that these
+files take a few times a session. The shell job store opts out (`fsync=False`):
+its records are ephemeral spill state that a crash invalidates anyway.
+
+Serialisation is unchanged at all five sites — indent, `ensure_ascii` and the
+trailing newline are per-caller arguments, so no file on disk reformats.
+
+`cli/_hook_utils._atomic_write_json` still keeps its own copy: a PreToolUse
+hook is a separate short-lived single-threaded process that imports nothing
+from `services`, so pid-only is genuinely enough there. That one is deliberate
+and is the only one left.
+
 ## [2.118.1] - 2026-09-04
 
 ### Fixed — the job store could crash a supervisor on Windows with WinError 5

cli/c3.py

@@ -92,7 +92,7 @@
 # Config
 CONFIG_DIR = ".c3"
 CONFIG_FILE = ".c3/config.json"
-__version__ = "2.118.1"
+__version__ = "2.118.2"
 
 
 def _compress_file_cli(compressor, path, mode="smart", **kw):

cli/hub_server.py

@@ -32,6 +32,7 @@
 from core.ide import PROFILES, detect_ide, get_profile, load_ide_config, normalize_ide_name
 from services import access_guard
 from services.activity_log import ActivityLog
+from services.atomic_json import write_json_atomic
 from services.project_manager import ProjectManager
 from services.tool_classifier import CATEGORIES
 
@@ -3078,9 +3079,10 @@ def api_projects_config_put():
         else:
             merged[key] = val
     cfg[section] = merged
-    tmp = cfg_file.with_name("config.json.tmp")
-    tmp.write_text(json.dumps(cfg, indent=2), encoding="utf-8")
-    os.replace(tmp, cfg_file)
+    # Not `config.json.tmp`: the CLI (services.access_guard) and the mobile
+    # API write this same file, and the hub itself is threaded — one shared
+    # temp name is a name they all pick at once. See services.atomic_json.
+    write_json_atomic(cfg_file, cfg, trailing_newline=False)
     try:
         ActivityLog(str(resolved)).log("hub_config_write", {
             "section": section, "keys": sorted(values.keys()), "source": "hub"})

oracle/services/mobile_api.py

@@ -56,6 +56,7 @@
 from oracle.services import api_auth, discovery_audit
 from oracle.services.api_auth import extract_bearer
 from oracle.services.tool_registry import _c3_version
+from services.atomic_json import write_json_atomic
 
 # 2: /info reports EFFECTIVE capabilities (filtered by the config switches)
 # rather than a static list, so a client can gate its UI instead of probing.
@@ -2090,10 +2091,11 @@ def _write_override_section(project: Path, section: dict) -> dict:
         else:
             block[key] = value
     cfg["override"] = block
-    tmp = cfg_file.with_name(f"{cfg_file.name}.tmp")
-    tmp.write_text(json.dumps(cfg, indent=2, ensure_ascii=False),
-                   encoding="utf-8")
-    tmp.replace(cfg_file)
+    # Shared with the CLI and the hub, and served from a threaded API, so the
+    # publish goes through the one race-safe writer rather than a temp name
+    # every writer of this file would pick. See services.atomic_json.
+    write_json_atomic(cfg_file, cfg, ensure_ascii=False,
+                      trailing_newline=False)
     return block
 
 

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "code-context-control"
-version = "2.118.1"
+version = "2.118.2"
 description = "Local MCP code-intelligence for AI coding tools: surgical search/read/edit, agent-config version history, path-level access + masking guards, and a multi-project hub."
 readme = "README.md"
 requires-python = ">=3.10"

services/access_guard.py

@@ -20,6 +20,8 @@
 from dataclasses import dataclass
 from pathlib import Path
 
+from services.atomic_json import write_json_atomic
+
 # ── Rule model ──────────────────────────────────────────────────────────────
 
 _KIND_DENY = "deny"
@@ -1473,11 +1475,17 @@ def _load_config_for_write(cfg: Path) -> tuple:
 
 
 def _write_scope_config(cfg: Path, data: dict) -> None:
-    """Atomic same-directory replace; privileged internal write."""
-    cfg.parent.mkdir(parents=True, exist_ok=True)
-    tmp = cfg.with_name(cfg.name + ".tmp")
-    tmp.write_text(json.dumps(data, indent=2) + "\n", encoding="utf-8")
-    os.replace(tmp, cfg)
+    """Atomic same-directory replace; privileged internal write.
+
+    The CLI, the hub and the mobile API all reach this for the same scope
+    file, so the publish is via ``services.atomic_json`` — a shared
+    ``config.json.tmp`` is a name they would collide on. The fsync matters
+    more here than anywhere else in the codebase: a truncated ``config.json``
+    comes back from ``load_all`` in its ``corrupt`` list, every path then
+    fails closed with ``<corrupt-config>``, and the session can no longer run
+    the command that would fix it.
+    """
+    write_json_atomic(cfg, data)
 
 
 def seed_default_global_rules() -> dict:

services/agent_locks.py

@@ -41,6 +41,7 @@
 import uuid
 from pathlib import Path
 
+from services.atomic_json import write_json_atomic
 from services.task_store import _FileLock
 
 SCHEMA_VERSION = 1
@@ -219,10 +220,15 @@ def _load(self) -> dict:
         return doc
 
     def _save(self, doc: dict) -> None:
-        self.data_dir.mkdir(parents=True, exist_ok=True)
-        tmp = self.state_file.with_name(self.state_file.name + ".tmp")
-        tmp.write_text(json.dumps(doc, indent=2) + "\n", encoding="utf-8")
-        os.replace(tmp, self.state_file)
+        # The `_FileLock` guard serialises the read-modify-write, but only
+        # among callers that take it — and it is advisory, held per process.
+        # The publish itself has to be safe on its own: a shared
+        # `locks.json.tmp` is one name every writer on the box picks, and two
+        # of them landing together either publishes a half-written lease file
+        # or raises WinError 5 (see services.atomic_json). Losing this write
+        # means losing live leases, which is exactly what `_load` refuses to
+        # do when it quarantines a corrupt file instead of resetting it.
+        write_json_atomic(self.state_file, doc)
 
     def _guard(self):
         return _FileLock(self.lock_file)

services/atomic_json.py

@@ -0,0 +1,118 @@
+"""One durable same-directory replace for the small JSON stores C3 writes.
+
+Why this module exists
+----------------------
+C3 keeps a handful of tiny JSON files that more than one writer touches: the
+agent lock store, the per-scope ``config.json`` written by the CLI, the hub
+and the mobile API, and the shell job store. Every one of them had grown the
+same three-line publish step by copy — ``<name>.tmp`` then a single
+``os.replace`` — and on Windows that shape is a live bug rather than a style
+nit:
+
+1. **A shared temp name races on the TEMP, not on the target.**
+   ``os.replace`` is atomic on the destination; nothing at all protects
+   ``<name>.tmp``. Two writers of one path both create it, so one publishes
+   a file the other is still writing, or the replace raises
+   ``PermissionError: [WinError 5] Access is denied`` because the loser still
+   holds a handle to it. Observed for real in the shell job store, where it
+   crashed a supervisor and reported "failed before running" for a command
+   that was fine (fixed there in 2.118.1, PR #157). The other copies had the
+   identical shape and no demonstrated failure only because they are written
+   far less often — the lock store especially is a coin that had not landed
+   on its edge yet.
+2. **A pid alone is not a unique suffix.** Threads share a pid, and the hub,
+   the Oracle and the lock store are all written from threaded servers. Six
+   threads are enough to drive a ``.tmp<pid>`` implementation into both
+   ``PermissionError`` and ``FileNotFoundError`` within a second, so the
+   suffix carries per-WRITE randomness as well.
+3. **One attempt turns a transient sharing violation into a lost write.** An
+   AV scanner or the Search indexer holding the target open for a few
+   milliseconds is ordinary on Windows and clears on its own; a single
+   ``os.replace`` turns it into an exception at the call site.
+
+``cli/_hook_utils._atomic_write_json`` deliberately does NOT import this. A
+PreToolUse hook is a separate, short-lived, single-threaded process that
+imports nothing from ``services`` on purpose, so pid-only is genuinely
+enough there. That one duplication stays.
+"""
+
+from __future__ import annotations
+
+import json
+import os
+import secrets
+import time
+from pathlib import Path
+
+# Four attempts across ~0.14s of backoff. Sized for a scanner or indexer
+# holding a handle, not for a lock a peer holds for seconds — a failure that
+# survives this is real and is re-raised.
+REPLACE_ATTEMPTS = 4
+REPLACE_BACKOFF_S = 0.02
+
+
+def atomic_tmp_path(path: Path) -> Path:
+    """``<name>.tmp<pid>-<random>`` beside ``path`` — unique per write."""
+    return path.with_name(f"{path.name}.tmp{os.getpid()}-{secrets.token_hex(4)}")
+
+
+def write_text_atomic(path, text: str, *, encoding: str = "utf-8",
+                      fsync: bool = True) -> None:
+    """Publish ``text`` at ``path``: unique temp, then a retried ``os.replace``.
+
+    ``fsync`` flushes the temp's bytes before the replace, so a crash or power
+    loss between the two cannot publish a zero-length or half-written file.
+    Durable state wants that: a truncated ``config.json`` makes the Access
+    Guard read ``<corrupt-config>`` and fail closed on every subsequent call,
+    which wedges the whole session. Callers whose file is ephemeral spill
+    state pass ``fsync=False`` rather than pay a disk flush per update.
+
+    Newline translation matches ``Path.write_text`` (the default), so this is
+    a drop-in for the ``write_text`` + ``os.replace`` pairs it replaces and
+    does not churn line endings in files already on disk.
+    """
+    path = Path(path)
+    path.parent.mkdir(parents=True, exist_ok=True)
+    tmp = atomic_tmp_path(path)
+    try:
+        with open(tmp, "w", encoding=encoding) as fh:
+            fh.write(text)
+            if fsync:
+                fh.flush()
+                os.fsync(fh.fileno())
+        last_exc: OSError | None = None
+        for attempt in range(REPLACE_ATTEMPTS):
+            try:
+                os.replace(tmp, path)
+                return
+            except PermissionError as exc:
+                last_exc = exc
+                if attempt < REPLACE_ATTEMPTS - 1:
+                    time.sleep(REPLACE_BACKOFF_S * (2 ** attempt))
+        if last_exc is not None:
+            raise last_exc
+    finally:
+        # A successful replace already consumed tmp, so this only fires on the
+        # failure paths — otherwise abandoned temps accumulate next to the
+        # file, and for `.c3/` that means they get indexed and committed.
+        try:
+            if tmp.exists():
+                tmp.unlink()
+        except OSError:
+            pass
+
+
+def write_json_atomic(path, data, *, indent: int = 2,
+                      ensure_ascii: bool = True,
+                      trailing_newline: bool = True,
+                      fsync: bool = True) -> None:
+    """``write_text_atomic`` over ``json.dumps``.
+
+    The serialisation knobs exist so each caller keeps the exact bytes it
+    wrote before this module: the point of the change is the publish step,
+    not a reformat of five JSON files that other tools and diffs already read.
+    """
+    text = json.dumps(data, indent=indent, ensure_ascii=ensure_ascii)
+    if trailing_newline:
+        text += "\n"
+    write_text_atomic(path, text, fsync=fsync)

services/shell_output.py

@@ -75,6 +75,8 @@
 from datetime import datetime, timedelta, timezone
 from pathlib import Path
 
+from services.atomic_json import write_json_atomic
+
 __all__ = [
     "STORE_ROOT", "RETENTION_DAYS", "RETENTION_BYTES", "HEAD_BYTES", "TAIL_BYTES",
     "TEXT_MAX_BYTES", "DEFAULT_READ_BYTES", "LINE_CLIP_CHARS",
@@ -960,64 +962,22 @@ def _load_meta(path: Path) -> OutputMeta | None:
         return None
 
 
-#: os.replace retries for when Windows briefly holds a handle on the target.
-#: Same construction as ``cli/_hook_utils._atomic_write_json``; the hook layer
-#: keeps its own copy on purpose (a PreToolUse subprocess imports nothing from
-#: services), so the two are duplicated deliberately rather than by accident.
-_REPLACE_ATTEMPTS = 4
-_REPLACE_BACKOFF_S = 0.02
-
-
 def _write_json_atomic(path: Path, data: dict) -> None:
-    """Write JSON durably: unique temp file in the same directory + os.replace.
-
-    Two bugs this fixes, both observed on Windows CI (2026-09-04, job store):
-
-    1. **A shared temp name.** ``<name>.tmp`` is the same string for every
-       writer, so two processes writing the SAME job file — the parent that
-       creates the record and the supervisor that immediately saves status —
-       raced on the temp itself. One would be mid-write while the other tried
-       to publish it, and ``os.replace`` raised
-       ``PermissionError: [WinError 5] Access is denied``. The suffix is a pid
-       AND a random token, not a pid alone: this store is written from a
-       threaded server, and threads share a pid, so ``.tmp<pid>`` collides
-       exactly as the shared name did. With a per-WRITE temp the race is only
-       on the target, where ``os.replace`` is genuinely atomic.
-    2. **No retry.** Even with unique temps, another process holding a read
-       handle on the target (an AV scanner, the Search indexer, a concurrent
-       reader) makes ``os.replace`` raise transiently. A single attempt turned
-       that into a crashed supervisor — the job never ran, and the caller got
-       "failed before running" for a command that was fine.
-
-    A partial write can still be published without fsync, but this store is
-    ephemeral spill state rather than durable config, so the cost of an fsync
-    per status update is not worth paying here.
+    """Publish a job record durably — see ``services.atomic_json``.
+
+    This was the first copy of the unique-temp + retried-``os.replace`` shape
+    (2.118.1, PR #157, after a shared ``<name>.tmp`` crashed supervisors on
+    Windows CI with WinError 5 and reported "failed before running" for
+    commands that were fine). The implementation now lives in one place so the
+    lock store and the three ``config.json`` writers share it instead of
+    carrying four more copies of a bug.
+
+    ``fsync=False`` is the one thing kept local: a job record is ephemeral
+    spill state that a crash invalidates anyway, so a disk flush per status
+    update buys nothing. The durable stores take the default.
     """
-    path.parent.mkdir(parents=True, exist_ok=True)
-    tmp = path.with_name(
-        f"{path.name}.tmp{os.getpid()}-{secrets.token_hex(4)}")
-    try:
-        tmp.write_text(json.dumps(data, ensure_ascii=False, indent=1),
-                       encoding="utf-8")
-        last_exc: OSError | None = None
-        for attempt in range(_REPLACE_ATTEMPTS):
-            try:
-                os.replace(tmp, path)
-                return
-            except PermissionError as exc:
-                last_exc = exc
-                if attempt < _REPLACE_ATTEMPTS - 1:
-                    time.sleep(_REPLACE_BACKOFF_S * (2 ** attempt))
-        if last_exc is not None:
-            raise last_exc
-    finally:
-        # A successful replace already consumed tmp, so this only fires on the
-        # failure paths — otherwise abandoned temps accumulate in the store.
-        try:
-            if tmp.exists():
-                tmp.unlink()
-        except OSError:
-            pass
+    write_json_atomic(path, data, indent=1, ensure_ascii=False,
+                      trailing_newline=False, fsync=False)
 
 
 def _move(src: Path, dst: Path) -> None: