fix(memories): stop losing memory content on an interrupted write (#1969)

* fix(memories): stop losing memory content on an interrupted write

save_memory() and edit_memory() wrote the memory file with a plain
open(path, "w") followed by f.write(content). That truncates the file
before the new content is complete, so a crash, an OOM kill, or a
disk-full error partway through the write can leave the file empty or
holding a stale tail instead of either the old or the new content.

Add write_file_atomic() to util/file_system.py: it writes to a temp
file in the same directory, then os.replaces it into place, with the
same short PermissionError retry save_yaml() already uses for a
transient Windows sharing violation. save_memory() and edit_memory()
now go through it instead of the direct open().

CodeEditor._save_edited_file() is left out of this change: os.replace
makes the destination inherit the temp file's permissions rather than
the original's, and tempfile.mkstemp creates that temp file 0600, so
an executable source file would lose its +x bit on save. Memory files
are always plain text Serena itself creates, so this doesn't apply to
them.

Refs #1958

* fix(memories): preserve file mode in write_file_atomic

mkstemp() creates the temp file with mode 0600 regardless of umask, and
os.replace() carries that mode into the destination. save_memory() and
edit_memory() going through write_file_atomic() silently tightened an
existing memory file's permissions (e.g. 0644 -> 0600) on every save or
edit, dropping group/other read access.

Restore the target's existing mode before the replace, or fall back to
what a plain open(path, "w") would give a brand-new file (0666 masked
by the process umask) when there is nothing to preserve.

Refs #1958

* test(memories): skip POSIX permission-preservation tests on Windows

os.chmod does not set POSIX group/other bits on Windows, so
test_save_memory_preserves_existing_file_permissions and
test_edit_memory_preserves_existing_file_permissions assert a mode
the platform cannot produce. Matches the existing skipif convention
used elsewhere in this suite for POSIX-only filesystem behavior.

Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>

---------

Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>

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

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(

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.

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

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: