fix(engine-state): resolve atomic checkpoint review findings (#285)

This commit is contained in:
Shay 2026-05-25 20:18:40 -07:00 committed by GitHub
parent fbff161a2e
commit 26237f2335
No known key found for this signature in database
GPG key ID: B5690EEEBB952194

View file

@ -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