From b174a2cf729f61a8d44c279bab52cfa0d7670fd6 Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Sun, 14 Jun 2026 23:58:07 +0200 Subject: [PATCH] util/yaml: make save_yaml atomic (temp + os.replace, Windows retry) A plain truncate-and-write (open(path,"w")) is not atomic: a concurrent writer (a second Serena process updating the auto-managed registered-projects list) or an interrupted write can leave the file half-overwritten -- a shorter value over a longer one leaves a stale tail, corrupting the YAML so every later load fails. This is what wedged native benchmark runs (serena MCP crashed at startup with a ruamel ScannerError -> 0 tools registered -> agent silently degraded to Bash). Write to a temp file in the same dir, then os.replace onto the target (atomic; last-writer-wins, never a corrupt interleave). Retry os.replace on Windows PermissionError (WinError 5/32 sharing violation under contention). Adds test/serena/util/test_yaml.py covering the shorter-over-longer corruption signature plus a concurrent-writers stress. Co-Authored-By: Claude Opus 4.8 --- src/serena/util/yaml.py | 42 ++++++++++++++++-- test/serena/util/test_yaml.py | 80 +++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 3 deletions(-) create mode 100644 test/serena/util/test_yaml.py diff --git a/src/serena/util/yaml.py b/src/serena/util/yaml.py index ca63eee7..bc16f431 100644 --- a/src/serena/util/yaml.py +++ b/src/serena/util/yaml.py @@ -1,5 +1,7 @@ import logging import os +import tempfile +import time from collections.abc import Sequence from enum import Enum from typing import Any @@ -199,9 +201,43 @@ def normalise_yaml_comments(commented_map: CommentedMap, comment_normalisation: def save_yaml(path: str, data: dict | CommentedMap, preserve_comments: bool = True) -> None: yaml = _create_yaml(preserve_comments) - os.makedirs(os.path.dirname(path), exist_ok=True) - with open(path, "w", encoding=SERENA_FILE_ENCODING) as f: - yaml.dump(data, f) + target_dir = os.path.dirname(path) + os.makedirs(target_dir, exist_ok=True) + # Atomic write: dump to a temp file in the SAME directory, then os.replace onto the target. + # A plain truncate-and-write (open(path, "w")) is NOT atomic: a concurrent writer (e.g. a second + # Serena process updating the auto-managed registered-projects list) or an interrupted write can + # leave the file half-overwritten — writing a shorter value over a longer one leaves a stale tail, + # which corrupts the YAML so it no longer parses and every later load fails. temp + os.replace makes + # each write all-or-nothing (last-writer-wins, never a corrupt interleave). + fd, tmp = tempfile.mkstemp(dir=target_dir, prefix=os.path.basename(path) + ".", suffix=".tmp") + try: + with os.fdopen(fd, "w", encoding=SERENA_FILE_ENCODING) as f: + yaml.dump(data, f) + _replace_with_retry(tmp, path) + except BaseException: + try: + os.unlink(tmp) + except OSError: + pass + raise + + +def _replace_with_retry(src: str, dst: str, *, attempts: int = 10, delay_s: float = 0.05) -> None: + """``os.replace(src, dst)`` with a short retry on a Windows sharing violation. + + On Windows the atomic rename fails with ``PermissionError`` (WinError 5/32) if another process + momentarily holds ``dst`` open — e.g. a second Serena process reading or replacing the same + config. A brief bounded retry rides out that contention; the temp file is still complete, so we + never fall back to a non-atomic write. + """ + for attempt in range(attempts): + try: + os.replace(src, dst) + return + except PermissionError: + if attempt == attempts - 1: + raise + time.sleep(delay_s) def yaml_comment_entry_is_empty(comment_entry: Any) -> bool: diff --git a/test/serena/util/test_yaml.py b/test/serena/util/test_yaml.py new file mode 100644 index 00000000..a64125ba --- /dev/null +++ b/test/serena/util/test_yaml.py @@ -0,0 +1,80 @@ +"""Tests for serena.util.yaml — focused on the atomic-write guarantee of ``save_yaml``. + +Regression: ``~/.serena/serena_config.yml`` was corrupted in the field when a non-atomic +truncate-and-write saved a SHORTER value over a longer existing file (or two Serena processes +saved concurrently), leaving a stale tail (e.g. a dangling ``ena`` line) that made the YAML +unparseable, which wedged every later load. ``save_yaml`` now writes to a temp file and +``os.replace``s it onto the target, so each write is all-or-nothing. +""" + +import os +import threading + +from ruamel.yaml import YAML + +from serena.util.yaml import load_yaml, save_yaml + + +def _read_text(path: str) -> str: + with open(path, encoding="utf-8") as f: + return f.read() + + +def test_save_yaml_roundtrips(tmp_path): + path = str(tmp_path / "config.yml") + save_yaml(path, {"projects": ["a", "b", "c"], "scalar": 1}) + loaded = load_yaml(path) + assert list(loaded["projects"]) == ["a", "b", "c"] + assert loaded["scalar"] == 1 + + +def test_shorter_write_over_longer_leaves_no_stale_tail(tmp_path): + """The exact field corruption: a long file overwritten by a much shorter one. + + A plain ``open(path, "w")`` truncates first, so this would pass even when buggy; the point + of the assertion is that the final file parses and contains EXACTLY the new content — no + leftover bytes from the longer previous version. + """ + path = str(tmp_path / "config.yml") + long_projects = [f"C:/Users/misch/Projects/some/really/long/path/number/{i}/serena" for i in range(50)] + save_yaml(path, {"projects": long_projects}) + + save_yaml(path, {"projects": ["C:/Users/misch/Projects/oraios/serena"]}) + + loaded = load_yaml(path) + assert list(loaded["projects"]) == ["C:/Users/misch/Projects/oraios/serena"] + # no stale tail from the longer write survived + assert "number/49" not in _read_text(path) + # and it is still valid YAML on a fresh strict parse + with open(path, encoding="utf-8") as f: + YAML().load(f) + + +def test_no_tmp_files_left_behind(tmp_path): + path = str(tmp_path / "config.yml") + save_yaml(path, {"projects": ["a"]}) + leftovers = [p for p in os.listdir(tmp_path) if p != "config.yml"] + assert leftovers == [], f"temp files not cleaned up: {leftovers}" + + +def test_concurrent_saves_never_corrupt(tmp_path): + """Many concurrent writers must yield a file that always parses (last-writer-wins is fine; + a corrupt interleave is not). + """ + path = str(tmp_path / "config.yml") + save_yaml(path, {"projects": ["seed"]}) + + def writer(n: int) -> None: + for _ in range(20): + save_yaml(path, {"projects": [f"p{n}-{k}" for k in range(n + 1)]}) + + threads = [threading.Thread(target=writer, args=(n,)) for n in range(6)] + for t in threads: + t.start() + for t in threads: + t.join() + + # whatever won, the file must be complete + parseable (no corruption) + loaded = load_yaml(path) + assert "projects" in loaded + assert all(isinstance(p, str) for p in loaded["projects"])