fix(providers): atomic .env write via tempfile + os.replace (nesquena#1164 cross-process)

nesquena · claude · nesquena · commit 7e5cdfa5d7d7 · 2026-04-27T15:11:45.000-07:00
The contributor PR's within-process fix (_ENV_LOCK on the load→modify→write cycle) is correct, but doesn't close the cross-process race that is the actual root cause of nesquena#1164. The hermes-agent CLI/Telegram bot uses hermes_cli.config.save_env_value, which writes .env atomically via tempfile + os.replace. The WebUI's _write_env_file was using os.open(..., O_TRUNC) — between the truncate and the complete write, a concurrent reader sees a partial or empty file. The agent has a _sanitize_env_lines workaround for the resulting corruption pattern (concatenated KEY=VALUE pairs from interleaved writes / partial reads), which is a strong signal that this race actually occurs in practice. Fix: stage the write through tempfile.mkstemp() in the same directory (same filesystem, so os.replace is atomic on POSIX), fsync before rename, chmod 0600 before rename so readers never see a 0644 window, and clean up the tempfile on any exception. Matches the exact pattern hermes_cli/config.py:save_env_value uses. Within-process safety from the PR's _ENV_LOCK is preserved (the whole sequence still runs inside the with-lock block). Also added a regression test asserting the new invariants: - tempfile staging present - os.replace called - O_TRUNC pattern is gone Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
diff --git a/api/providers.py b/api/providers.py
@@ -153,12 +153,29 @@ def _write_env_file(env_path: Path, updates: dict[str, str | None]) -> None:
         content = "\n".join(output_lines)
         if content:
             content += "\n"
-        # Create at owner-only mode from the first byte (O_CREAT honours the mode
-        # argument subject to umask). A trailing chmod guards pre-existing files.
+        # Atomic write via tempfile + os.replace so cross-process readers
+        # (Telegram bot, CLI) never see a half-truncated file.  The shared
+        # ``~/.hermes/.env`` is also written by ``hermes_cli.config.save_env_value``
+        # using the same atomic pattern; matching it here closes the
+        # cross-process leg of #1164 (within-process is covered by _ENV_LOCK).
         _mode = _stat.S_IRUSR | _stat.S_IWUSR  # 0o600
-        _fd = os.open(str(env_path), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, _mode)
-        with os.fdopen(_fd, "w", encoding="utf-8") as _f:
-            _f.write(content)
+        import tempfile as _tempfile
+        _tmp_fd, _tmp_path = _tempfile.mkstemp(
+            dir=str(env_path.parent), prefix=".env_", suffix=".tmp"
+        )
+        try:
+            with os.fdopen(_tmp_fd, "w", encoding="utf-8") as _f:
+                _f.write(content)
+                _f.flush()
+                os.fsync(_f.fileno())
+            os.chmod(_tmp_path, _mode)  # tighten before rename so readers see 0600
+            os.replace(_tmp_path, env_path)
+        except BaseException:
+            try:
+                os.unlink(_tmp_path)
+            except OSError:
+                pass
+            raise
         try:
             env_path.chmod(_mode)
         except OSError:
diff --git a/tests/test_issue1164_env_file_corruption.py b/tests/test_issue1164_env_file_corruption.py
@@ -152,3 +152,19 @@ def test_providers_write_env_holds_env_lock(self):
                       "_write_env_file must use _ENV_LOCK for concurrency safety")
         self.assertIn("from api.streaming import _ENV_LOCK", source,
                       "_ENV_LOCK must be imported from api.streaming")
+
+    def test_providers_write_env_uses_atomic_rename(self):
+        """providers._write_env_file must write atomically via tempfile +
+        os.replace so cross-process readers (Telegram, CLI) never observe
+        a truncated half-written file (#1164 cross-process leg)."""
+        import inspect
+        from api.providers import _write_env_file
+        source = inspect.getsource(_write_env_file)
+        self.assertIn("tempfile", source,
+                      "_write_env_file must stage writes through a tempfile")
+        self.assertIn("os.replace(", source,
+                      "_write_env_file must atomically rename via os.replace")
+        # The original O_TRUNC pattern must NOT remain — it is the source of
+        # the cross-process race the PR is closing.
+        self.assertNotIn("O_TRUNC", source,
+                         "_write_env_file must not truncate-in-place (#1164)")
