diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d062e28..e1c5ab69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,13 @@ Status of the `main` branch. Changes prior to the next official version change w project list in `serena_config.yml` - Fix: `read_only` restriction in project definition was not applied to base tool set when in single-project context (#1938) +* Memories: + - Fix: `save_memory`/`edit_memory` wrote directly to the memory file with `open(path, "w")`, which + truncates it before the new content is written; a crash, OOM kill, or full disk partway through + the write could destroy the previous, valid content instead of just losing the update. Both now + write through a temp-file-plus-`os.replace` helper, matching the approach `save_yaml()` already + uses for settings files (#1958) + * JetBrains: - Fix: Concurrent Serena sessions activating different projects at the same time with `jetbrains_launch_command` set would each independently launch the IDE, racing each other for diff --git a/src/serena/memories/memory_manager.py b/src/serena/memories/memory_manager.py index daa03725..30c518a9 100644 --- a/src/serena/memories/memory_manager.py +++ b/src/serena/memories/memory_manager.py @@ -10,6 +10,7 @@ from serena.config.serena_config import ( SerenaPaths, ) from serena.constants import SERENA_FILE_ENCODING +from serena.util.file_system import write_file_atomic from serena.util.text_utils import ContentReplacer from .memory_reference_analysis import ( @@ -215,8 +216,7 @@ class MemoryManager: self._check_not_ignored(name) self._check_write_access(name, is_tool_context) memory_file_path = self.get_memory_file_path(name) - with open(memory_file_path, "w", encoding=self._encoding) as f: - f.write(content) + write_file_atomic(str(memory_file_path), content, encoding=self._encoding) return f"Memory {name} written." class MemoriesList: @@ -399,8 +399,7 @@ class MemoryManager: original_content = f.read() replacer = ContentReplacer(mode=mode, allow_multiple_occurrences=allow_multiple_occurrences, regex_multiline=regex_multiline) updated_content = replacer.replace(original_content, needle, repl) - with open(memory_file_path, "w", encoding=self._encoding) as f: - f.write(updated_content) + write_file_atomic(str(memory_file_path), updated_content, encoding=self._encoding) return f"Memory {name} edited successfully." def validate_referential_integrity( diff --git a/src/serena/util/file_system.py b/src/serena/util/file_system.py index fcde0e09..fd7a0cc5 100644 --- a/src/serena/util/file_system.py +++ b/src/serena/util/file_system.py @@ -1,6 +1,9 @@ import logging import os import re +import stat +import tempfile +import time from collections.abc import Callable, Iterator from dataclasses import dataclass, field from pathlib import Path @@ -12,6 +15,70 @@ from sensai.util.logging import LogTime log = logging.getLogger(__name__) + +def write_file_atomic(path: str, content: str, *, encoding: str, newline: str | None = None) -> None: + """ + Write ``content`` to ``path`` atomically: the content is written to a temporary file in the + same directory first, then swapped into place with ``os.replace``. A plain + ``open(path, "w")`` is not atomic: it truncates the file before the new content is complete, + so a crash, an out-of-memory kill, or a disk-full error partway through the write leaves + ``path`` holding neither the old content nor the new one. + + :param path: the path to write to + :param content: the text content to write + :param encoding: the encoding to use for the write + :param newline: passed through to the underlying ``open()`` call to control newline translation + """ + target_dir = os.path.dirname(path) or "." + try: + existing_mode: int | None = stat.S_IMODE(os.stat(path).st_mode) + except FileNotFoundError: + existing_mode = None + fd, tmp_path = tempfile.mkstemp(dir=target_dir, prefix=os.path.basename(path) + ".", suffix=".tmp") + try: + with os.fdopen(fd, "w", encoding=encoding, newline=newline) as f: + f.write(content) + # mkstemp creates the temp file with mode 0600 regardless of umask, which would silently + # tighten an existing file's permissions (e.g. 0644 -> 0600) on replace. Restore the + # original mode, or fall back to what a plain open(path, "w") would have produced for a + # new file (0666 masked by the process umask). + os.chmod(tmp_path, existing_mode if existing_mode is not None else _new_file_mode()) + _replace_with_retry(tmp_path, path) + except BaseException: + try: + os.unlink(tmp_path) + except OSError: + pass + raise + + +def _new_file_mode() -> int: + """The mode a plain ``open(path, "w")`` would give a brand-new file: 0o666 masked by the + process umask. Reading the umask requires setting it, so the previous value is restored + immediately after. + """ + current_umask = os.umask(0o022) + os.umask(current_umask) + return 0o666 & ~current_umask + + +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`` if another process momentarily holds ``dst`` open + (e.g. a second Serena process reading the same memory or source file). A brief bounded retry + rides out that contention; the temp file is still complete, so this never falls 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) + + # Characters meaningful to pathspec's gitignore grammar: glob wildcards, bracket expressions, # the escape character itself, and '!'/'#' which change a whole pattern's meaning when they # are its first character. Backslash-escaping them makes a literal name safe to interpolate. diff --git a/test/serena/test_memories_manager.py b/test/serena/test_memories_manager.py index 158ab379..46c1b47b 100644 --- a/test/serena/test_memories_manager.py +++ b/test/serena/test_memories_manager.py @@ -5,6 +5,8 @@ and :meth:`_prepare_name`). """ import os +import stat +import sys import pytest @@ -16,6 +18,7 @@ from serena.memories.memory_reference_analysis import ( find_stale_reference_candidates, ) from serena.project import MemoryManager +from serena.util import file_system @pytest.fixture @@ -269,6 +272,83 @@ def _write(manager: MemoryManager, name: str, content: str) -> None: manager.save_memory(name, content, is_tool_context=False) +class TestSaveAndEditMemoryAreAtomic: + """Regression for issue #1958: ``save_memory``/``edit_memory`` used a plain + ``open(path, "w")``, which truncates the file before the new content is written. An + interrupted write (crash, OOM kill, disk full) then loses the previous content entirely. + """ + + @staticmethod + def _install_crashing_fdopen(monkeypatch) -> None: + real_fdopen = os.fdopen + + def crashing_fdopen(fd, *args, **kwargs): + f = real_fdopen(fd, *args, **kwargs) + real_write = f.write + + def crashing_write(data): + real_write(data[: len(data) // 4]) + f.flush() + raise RuntimeError("simulated crash mid-write") + + f.write = crashing_write + return f + + monkeypatch.setattr(file_system.os, "fdopen", crashing_fdopen) + + def test_save_memory_preserves_prior_content_on_interrupted_write(self, fs_manager: MemoryManager, monkeypatch) -> None: + _write(fs_manager, "notes", "original notes" * 20) + self._install_crashing_fdopen(monkeypatch) + + with pytest.raises(RuntimeError, match="simulated crash mid-write"): + fs_manager.save_memory("notes", "new notes that never fully arrive" * 20, is_tool_context=False) + + assert fs_manager.load_memory("notes") == "original notes" * 20 + + def test_edit_memory_preserves_prior_content_on_interrupted_write(self, fs_manager: MemoryManager, monkeypatch) -> None: + _write(fs_manager, "notes", "the original needle is here" * 20) + self._install_crashing_fdopen(monkeypatch) + + with pytest.raises(RuntimeError, match="simulated crash mid-write"): + fs_manager.edit_memory( + "notes", + needle="original needle", + repl="replacement needle", + mode="literal", + allow_multiple_occurrences=True, + is_tool_context=False, + ) + + assert fs_manager.load_memory("notes") == "the original needle is here" * 20 + + @pytest.mark.skipif(sys.platform == "win32", reason="os.chmod does not set POSIX group/other bits on Windows") + def test_save_memory_preserves_existing_file_permissions(self, fs_manager: MemoryManager) -> None: + _write(fs_manager, "notes", "original notes") + path = fs_manager.get_memory_file_path("notes") + os.chmod(path, 0o644) + + fs_manager.save_memory("notes", "updated notes", is_tool_context=False) + + assert stat.S_IMODE(os.stat(path).st_mode) == 0o644 + + @pytest.mark.skipif(sys.platform == "win32", reason="os.chmod does not set POSIX group/other bits on Windows") + def test_edit_memory_preserves_existing_file_permissions(self, fs_manager: MemoryManager) -> None: + _write(fs_manager, "notes", "the original needle is here") + path = fs_manager.get_memory_file_path("notes") + os.chmod(path, 0o644) + + fs_manager.edit_memory( + "notes", + needle="original needle", + repl="replacement needle", + mode="literal", + allow_multiple_occurrences=True, + is_tool_context=False, + ) + + assert stat.S_IMODE(os.stat(path).st_mode) == 0o644 + + class TestListMemoriesFollowsSymlinks: """Regression: memories reachable only through a directory symlink (e.g. a monorepo whose ``.serena/memories`` symlinks each submodule's memory folder, making them addressable as diff --git a/test/serena/util/test_file_system.py b/test/serena/util/test_file_system.py index 1e1272b9..6e366f3b 100644 --- a/test/serena/util/test_file_system.py +++ b/test/serena/util/test_file_system.py @@ -7,7 +7,97 @@ from pathlib import Path import pytest from pathspec import PathSpec -from serena.util.file_system import GitignoreParser, GitignoreSpec, _escape_gitignore_path_component, match_path +from serena.util import file_system +from serena.util.file_system import GitignoreParser, GitignoreSpec, _escape_gitignore_path_component, match_path, write_file_atomic + + +class TestWriteFileAtomic: + """Regression tests for issue #1958: a plain ``open(path, "w")`` truncates the file before + the new content is complete, so a crash, OOM kill, or disk-full error partway through the + write loses the previous content. ``write_file_atomic`` must never expose that intermediate + state. + """ + + def test_writes_new_file(self, tmp_path): + target = tmp_path / "notes.md" + write_file_atomic(str(target), "hello", encoding="utf-8") + assert target.read_text(encoding="utf-8") == "hello" + + def test_overwrites_existing_file(self, tmp_path): + target = tmp_path / "notes.md" + target.write_text("old", encoding="utf-8") + write_file_atomic(str(target), "new", encoding="utf-8") + assert target.read_text(encoding="utf-8") == "new" + + def test_no_leftover_temp_file_after_success(self, tmp_path): + target = tmp_path / "notes.md" + write_file_atomic(str(target), "hello", encoding="utf-8") + assert list(tmp_path.iterdir()) == [target] + + def test_respects_newline_argument(self, tmp_path): + target = tmp_path / "notes.md" + write_file_atomic(str(target), "a\nb\n", encoding="utf-8", newline="\r\n") + assert target.read_bytes() == b"a\r\nb\r\n" + + def test_preserves_original_content_when_write_is_interrupted(self, tmp_path, monkeypatch): + """The core invariant: an interrupted write (process killed / OOM / disk full while the + temp file is being written) must leave the target file exactly as it was, never + truncated or half-overwritten. + """ + target = tmp_path / "notes.md" + original = "original content that must survive" * 20 + target.write_text(original, encoding="utf-8") + + real_fdopen = os.fdopen + + def crashing_fdopen(fd, *args, **kwargs): + f = real_fdopen(fd, *args, **kwargs) + real_write = f.write + + def crashing_write(data): + # write a truncated prefix to the temp file, flush it to disk, then blow up, + # simulating a crash after the OS has seen some but not all of the new content. + real_write(data[: len(data) // 4]) + f.flush() + raise RuntimeError("simulated crash mid-write") + + f.write = crashing_write + return f + + monkeypatch.setattr(file_system.os, "fdopen", crashing_fdopen) + + with pytest.raises(RuntimeError, match="simulated crash mid-write"): + write_file_atomic(str(target), "brand new content that never fully arrives" * 20, encoding="utf-8") + + assert target.read_text(encoding="utf-8") == original + # the partially-written temp file must be cleaned up, not left behind + assert list(tmp_path.iterdir()) == [target] + + def test_replace_with_retry_survives_transient_permission_error(self, tmp_path, monkeypatch): + """Mirrors ``util/yaml.py``'s ``_replace_with_retry``: on Windows, ``os.replace`` can + fail with a transient ``PermissionError`` while another process momentarily holds the + destination open. The write must not be treated as failed while the temp file is still + complete and a retry can still succeed. + """ + target = tmp_path / "notes.md" + target.write_text("old", encoding="utf-8") + + real_replace = os.replace + calls = {"n": 0} + + def flaky_replace(src, dst): + calls["n"] += 1 + if calls["n"] < 3: + raise PermissionError("simulated transient sharing violation") + return real_replace(src, dst) + + monkeypatch.setattr(file_system.os, "replace", flaky_replace) + monkeypatch.setattr(file_system.time, "sleep", lambda _seconds: None) + + write_file_atomic(str(target), "new", encoding="utf-8") + + assert calls["n"] == 3 + assert target.read_text(encoding="utf-8") == "new" class TestGitignoreParser: