From 26237f2335b6e2b6a1ebe2ff260ffdbd08cb03b4 Mon Sep 17 00:00:00 2001 From: Shay Date: Mon, 25 May 2026 20:18:40 -0700 Subject: [PATCH] fix(engine-state): resolve atomic checkpoint review findings (#285) --- engine_state/__init__.py | 56 ++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/engine_state/__init__.py b/engine_state/__init__.py index 1bcfc826..1758a12c 100644 --- a/engine_state/__init__.py +++ b/engine_state/__init__.py @@ -14,6 +14,7 @@ from __future__ import annotations import json import os +import stat import subprocess import tempfile import warnings @@ -28,46 +29,45 @@ def _atomic_write_text(target: Path, content: str, *, encoding: str = "utf-8") - """ADR-0156 (W-022) — atomic checkpoint write. Write ``content`` to a temp file in the same directory as ``target``, - fsync it, then ``os.replace`` it into place. Same-directory rename is + fsync it, then ``os.replace`` it into place. Same-directory rename is atomic on POSIX (and on Windows since Python 3.3 via ``os.replace``). A SIGINT/SIGKILL between ``write`` and ``replace`` leaves the prior - target file fully intact; an interrupted write leaves an orphan - temp file that is cleaned up on the next ``save_*`` call. + target file fully intact. - ADR-0146 §"File Operations and Invariants" specified this behavior; - pre-W-022 the writers used ``Path.write_text`` directly, which - truncated the target before writing and corrupted the checkpoint - on mid-write process termination. + Existing file mode bits are preserved when replacing a prior checkpoint + file so engine-state readers do not silently lose access after a save. """ target.parent.mkdir(parents=True, exist_ok=True) - # NamedTemporaryFile with delete=False so we can rename out of its handle. - # ``dir=target.parent`` keeps the rename on the same filesystem (atomic). - with tempfile.NamedTemporaryFile( - mode="w", - encoding=encoding, - dir=str(target.parent), - prefix=f".{target.name}.", - suffix=".tmp", - delete=False, - ) as fh: - tmp_path = Path(fh.name) - try: + + existing_mode: int | None = None + if target.exists(): + existing_mode = stat.S_IMODE(target.stat().st_mode) + + tmp_path: Path | None = None + try: + with tempfile.NamedTemporaryFile( + mode="w", + encoding=encoding, + dir=str(target.parent), + prefix=f".{target.name}.", + suffix=".tmp", + delete=False, + ) as fh: + tmp_path = Path(fh.name) fh.write(content) fh.flush() os.fsync(fh.fileno()) - except BaseException: + + if existing_mode is not None and tmp_path is not None: + os.chmod(tmp_path, existing_mode) + + os.replace(tmp_path, target) + except BaseException: + if tmp_path is not None: try: tmp_path.unlink() except FileNotFoundError: pass - raise - try: - os.replace(tmp_path, target) - except BaseException: - try: - tmp_path.unlink() - except FileNotFoundError: - pass raise _SCHEMA_VERSION = 1