From c8a012d4d2db27142196a9a7b290687323979b5b Mon Sep 17 00:00:00 2001
From: Ashvin <76151462+ashvinctrl@users.noreply.github.com>
Date: Thu, 6 Aug 2026 14:03:50 +0530
Subject: [PATCH] fix(memory): don't let an unreadable store get overwritten
 with an empty one (#5831)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix(memory): don't let an unreadable store get overwritten with an empty one

load_all() answered a failed read the same way it answered an empty store:
with []. Every mutation path is a read-modify-write (load the whole file,
change it, save it back), so a failed read became

    load_all() -> []  ->  [].append(new)  ->  save([new])

and save() is atomic, so the replacement stuck.

The case that actually destroys data is a store that is READABLE but not
parseable - a truncated file, or one holding {} instead of []. Nothing
obstructs the write, so adding a memory returns HTTP 200 and every memory
already stored is gone. Verified end-to-end against a running instance: on the
current code a truncated memory.json plus one add leaves the file holding only
the new entry. Truncation is reachable - core/database.py rewrites memory.json
during migration with a plain open(.., "w") + json.dump, which is not atomic.

A live exclusive lock is not the dangerous case: it blocks the read and the
os.replace alike, so the save fails too and the store survives. That path
currently 500s and loses nothing.

_read_entries() now returns [] only when the file genuinely does not exist and
raises MemoryStoreUnreadable for every other failure, including a store that
parses but is not a JSON array. load_all() keeps the old lenient behaviour so
display, search and context injection still degrade quietly instead of
breaking chat. The read-modify-write callers switch to load_all_for_update(),
which propagates the error: the memory routes turn it into a 503 and change
nothing, backup import refuses rather than saving only the incoming rows, and
auto-extraction and the audit merge skip the write. The audit merge mattered
most - it rebuilds the whole file from one owner's slice plus everyone else's
rows, so an empty read there dropped every other tenant's memories.

The corrupt-JSON path still gets its one shot at the legacy memory.txt
migration before raising, so that recovery is unchanged.

The two updated fakes gained load_all_for_update because the real class has it;
MagicMock would otherwise hand the import path a Mock instead of the seeded list.

Fixes #5673

* fix(memory): fail closed on the remaining read-modify-write add paths

The strict loader landed with the routes, the backup import and the extractor
converted, but three read-modify-write sinks still called load_all(), which
degrades an unreadable store to []. Two of them are the paths users actually
reach, so the data loss in #5673 stayed reproducible:

- src/ai_interaction.py do_manage_memory, action "add" — reached from ordinary
  chat via src/tool_execution.py:793 -> dispatch_ai_tool. "Remember that I
  prefer X" against an unreadable store wrote a one-entry file over it and
  reported success.
- mcp_servers/memory_server.py, action "add" — the same shape through
  _scope_entries(), registered as a built-in in src/builtin_mcp.py.
- src/memory_provider.py NativeMemoryProvider.remember and .delete — wired
  into app state in src/app_initializer.py but not consumed outside tests yet,
  converted here so the pattern is uniform before it goes live.

The MCP server takes _scope_entries(for_update=True) so list keeps the lenient
read. The edit and delete branches on both tool paths were already fail-closed
by accident — an empty view matches nothing and returns before the save — so
they are left alone.

The three new tests drive the real entry points rather than replaying the
shape, and use a truncated store, which is the case that reads back fine so
nothing stops the save. Each asserts memory.json is byte-identical afterwards;
all three fail on the previous commit with the store overwritten.
---
 mcp_servers/memory_server.py                  |  26 +-
 routes/backup_routes.py                       |  11 +-
 routes/memory/memory_routes.py                |  26 +-
 services/memory/__init__.py                   |   3 +-
 services/memory/memory.py                     |  14 +-
 services/memory/memory_extractor.py           |  23 +-
 src/ai_interaction.py                         |  11 +-
 src/memory.py                                 |  90 ++++++-
 src/memory_provider.py                        |  11 +-
 tests/test_backup_import_cross_user_dedup.py  |   3 +
 ...st_memory_extractor_vector_cross_tenant.py |   6 +
 tests/test_memory_store_unreadable_no_wipe.py | 255 ++++++++++++++++++
 12 files changed, 451 insertions(+), 28 deletions(-)
 create mode 100644 tests/test_memory_store_unreadable_no_wipe.py

diff --git a/mcp_servers/memory_server.py b/mcp_servers/memory_server.py
index fafbcfc2b..fd574fd1f 100644
--- a/mcp_servers/memory_server.py
+++ b/mcp_servers/memory_server.py
@@ -17,6 +17,8 @@ from mcp.types import Tool, TextContent
 
 sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
 
+from src.memory import MemoryStoreUnreadable
+
 server = Server("memory")
 
 # Late-initialized managers (set during first tool call)
@@ -29,6 +31,10 @@ _OWNER_SCOPE_ERROR = (
     "Error: Memory MCP owner is not configured for an owner-scoped memory store. "
     "Set ODYSSEUS_MCP_MEMORY_OWNER for this server or use the owner-aware native memory tool."
 )
+_UNREADABLE_STORE_ERROR = (
+    "Error: Memory store is temporarily unreadable — nothing was saved. "
+    "Repair or restore memory.json, then retry."
+)
 
 
 def _configured_owner() -> str | None:
@@ -51,9 +57,21 @@ def _owner_scoped_store(entries: list[dict]) -> bool:
     return any(_entry_owner(entry) for entry in entries if isinstance(entry, dict))
 
 
-def _scope_entries() -> tuple[str | None, list[dict], list[dict], str | None]:
-    """Return configured owner, all entries, visible entries, and optional error."""
-    entries = _memory_manager.load_all()
+def _scope_entries(for_update: bool = False) -> tuple[str | None, list[dict], list[dict], str | None]:
+    """Return configured owner, all entries, visible entries, and optional error.
+
+    ``for_update=True`` is for read-modify-write callers. They save the ``all
+    entries`` list back, so an unreadable store must be reported as an error
+    instead of degrading to ``[]`` — otherwise the save writes their one new
+    entry over the whole store (issue #5673).
+    """
+    if for_update:
+        try:
+            entries = _memory_manager.load_all_for_update()
+        except MemoryStoreUnreadable as e:
+            return None, [], [], f"{_UNREADABLE_STORE_ERROR} ({e})"
+    else:
+        entries = _memory_manager.load_all()
     owner = _configured_owner()
     if owner is None and _owner_scoped_store(entries):
         return None, entries, [], _OWNER_SCOPE_ERROR
@@ -161,7 +179,7 @@ async def call_tool(name: str, arguments: dict) -> list[TextContent]:
         category = arguments.get("category", "fact")
         if not text:
             return _text_result("Error: Memory text cannot be empty")
-        owner, memories, _visible, scope_error = _scope_entries()
+        owner, memories, _visible, scope_error = _scope_entries(for_update=True)
         if scope_error:
             return _text_result(scope_error)
         entry = _memory_manager.add_entry(text, source="ai_agent", category=category, owner=owner)
diff --git a/routes/backup_routes.py b/routes/backup_routes.py
index 313369370..4ecf4f165 100644
--- a/routes/backup_routes.py
+++ b/routes/backup_routes.py
@@ -6,6 +6,7 @@ from datetime import datetime
 
 from fastapi import APIRouter, HTTPException, Request, Response
 from core.middleware import require_admin
+from services.memory import MemoryStoreUnreadable
 from src.auth_helpers import get_current_user
 from src.settings import load_settings, save_settings, load_features, save_features
 
@@ -76,7 +77,15 @@ def setup_backup_routes(memory_manager, preset_manager, skills_manager) -> APIRo
 
         # ── Memories ──
         if "memories" in body and isinstance(body["memories"], list):
-            existing = memory_manager.load_all()
+            # Strict load: importing on top of an unreadable store would write
+            # only the incoming rows back and drop everything already saved.
+            try:
+                existing = memory_manager.load_all_for_update()
+            except MemoryStoreUnreadable as e:
+                logger.error("Refusing to import memories: %s", e)
+                raise HTTPException(
+                    503, "Memory store is temporarily unreadable — nothing was imported."
+                )
             # Dedup against THIS user's own memories only. Using every tenant's
             # rows (load_all) meant a memory whose text matched any other
             # user's was silently skipped, so the importing user lost their own
diff --git a/routes/memory/memory_routes.py b/routes/memory/memory_routes.py
index d290046ec..c4232bec4 100644
--- a/routes/memory/memory_routes.py
+++ b/routes/memory/memory_routes.py
@@ -21,7 +21,7 @@ def _strip_list_prefix(text: str) -> str:
         return text
     return _LIST_PREFIX_RE.sub("", text, count=1).strip()
 
-from services.memory import MemoryManager
+from services.memory import MemoryManager, MemoryStoreUnreadable
 from core.session_manager import SessionManager
 from src.request_models import MemoryAddRequest
 from core.database import SessionLocal
@@ -35,6 +35,22 @@ from src.upload_limits import read_upload_limited, MEMORY_IMPORT_MAX_BYTES
 logger = logging.getLogger(__name__)
 
 
+def _load_for_update(memory_manager) -> List[Dict[str, Any]]:
+    """Load the whole store for a read-modify-write cycle.
+
+    A transient read failure must not look like an empty store: the caller
+    would append to ``[]`` and save that back, atomically destroying every
+    existing memory (issue #5673). Surface it as a 503 and change nothing.
+    """
+    try:
+        return memory_manager.load_all_for_update()
+    except MemoryStoreUnreadable as e:
+        logger.error("Refusing to rewrite the memory store: %s", e)
+        raise HTTPException(
+            503, "Memory store is temporarily unreadable — no changes were made."
+        )
+
+
 def setup_memory_routes(memory_manager: MemoryManager, session_manager: SessionManager, memory_vector=None):
     """Set up memory-related routes."""
     router = APIRouter(prefix="/api/memory", tags=["memory"])
@@ -116,7 +132,7 @@ def setup_memory_routes(memory_manager: MemoryManager, session_manager: SessionM
         new_entry = memory_manager.add_entry(text, memory_data.source, memory_data.category, owner=user)
         if memory_data.session_id:
             new_entry["session_id"] = memory_data.session_id
-        all_mem = memory_manager.load_all()
+        all_mem = _load_for_update(memory_manager)
         all_mem.append(new_entry)
         memory_manager.save(all_mem)
         # Sync vector index
@@ -487,7 +503,7 @@ def setup_memory_routes(memory_manager: MemoryManager, session_manager: SessionM
     def pin_memory(request: Request, memory_id: str, pinned: bool = Form(True)):
         """Pin or unpin a memory. Pinned memories are always included in context."""
         user = _owner(request)
-        all_mem = memory_manager.load_all()
+        all_mem = _load_for_update(memory_manager)
         for i, memory in enumerate(all_mem):
             if memory["id"] == memory_id:
                 _verify_memory_owner(memory, user)
@@ -512,7 +528,7 @@ def setup_memory_routes(memory_manager: MemoryManager, session_manager: SessionM
     def update_memory(request: Request, memory_id: str, text: str = Form(...), category: str = Form(None)):
         """Update an existing memory item with new text and optional category."""
         user = _owner(request)
-        all_mem = memory_manager.load_all()
+        all_mem = _load_for_update(memory_manager)
         for i, memory in enumerate(all_mem):
             if memory["id"] == memory_id:
                 _verify_memory_owner(memory, user)
@@ -534,7 +550,7 @@ def setup_memory_routes(memory_manager: MemoryManager, session_manager: SessionM
     def delete_memory(request: Request, memory_id: str):
         """Delete a memory item by its ID."""
         user = _owner(request)
-        all_mem = memory_manager.load_all()
+        all_mem = _load_for_update(memory_manager)
 
         # Find and verify ownership before deleting
         target = next((m for m in all_mem if m["id"] == memory_id), None)
diff --git a/services/memory/__init__.py b/services/memory/__init__.py
index 53fc80bd8..31fa1d5fa 100644
--- a/services/memory/__init__.py
+++ b/services/memory/__init__.py
@@ -2,7 +2,7 @@
 """Memory service — persistent memory storage and retrieval."""
 
 from .service import MemoryService, Memory, MemorySearchResult
-from .memory import MemoryManager
+from .memory import MemoryManager, MemoryStoreUnreadable
 from .memory_vector import MemoryVectorStore
 
 __all__ = [
@@ -10,5 +10,6 @@ __all__ = [
     "Memory",
     "MemorySearchResult",
     "MemoryManager",
+    "MemoryStoreUnreadable",
     "MemoryVectorStore",
 ]
diff --git a/services/memory/memory.py b/services/memory/memory.py
index 031c13ac4..b9aaaa2a8 100644
--- a/services/memory/memory.py
+++ b/services/memory/memory.py
@@ -5,6 +5,16 @@ application runtime instantiates ``src.memory.MemoryManager``, so keeping a
 parallel implementation here risks silent drift between import paths.
 """
 
-from src.memory import MemoryManager, get_text_similarity, tokenize
+from src.memory import (
+    MemoryManager,
+    MemoryStoreUnreadable,
+    get_text_similarity,
+    tokenize,
+)
 
-__all__ = ["MemoryManager", "get_text_similarity", "tokenize"]
+__all__ = [
+    "MemoryManager",
+    "MemoryStoreUnreadable",
+    "get_text_similarity",
+    "tokenize",
+]
diff --git a/services/memory/memory_extractor.py b/services/memory/memory_extractor.py
index e5f609250..11539263b 100644
--- a/services/memory/memory_extractor.py
+++ b/services/memory/memory_extractor.py
@@ -17,6 +17,8 @@ import os
 import re
 from typing import Optional
 
+from src.memory import MemoryStoreUnreadable
+
 logger = logging.getLogger(__name__)
 
 
@@ -387,7 +389,13 @@ async def extract_and_store(
         # Get owner from session
         _owner = getattr(session, 'owner', None)
 
-        existing = memory_manager.load_all()
+        # Strict load: this is a read-modify-write. Degrading to [] here would
+        # save only the newly extracted facts and drop the entire store.
+        try:
+            existing = memory_manager.load_all_for_update()
+        except MemoryStoreUnreadable as e:
+            logger.error("Skipping auto memory extraction, store unreadable: %s", e)
+            return
         added = 0
 
         for fact in facts:
@@ -626,7 +634,18 @@ async def audit_memories(
 
         # Merge audited entries back with other users' entries
         if owner:
-            all_entries = memory_manager.load_all()
+            # Strict load: the merge below reconstructs the whole file. If this
+            # degraded to [] we would save only this owner's audited slice and
+            # destroy every other tenant's memories.
+            try:
+                all_entries = memory_manager.load_all_for_update()
+            except MemoryStoreUnreadable as e:
+                logger.error("Aborting memory audit save, store unreadable: %s", e)
+                return {
+                    "before": before_count,
+                    "after": before_count,
+                    "error": "store_unreadable",
+                }
             audited_ids = {e["id"] for e in final_entries}
             other_entries = [e for e in all_entries if e.get("owner") != owner and (e.get("owner") is not None)]
             # Also keep legacy entries that weren't part of this audit
diff --git a/src/ai_interaction.py b/src/ai_interaction.py
index 9ee97368f..e777ca32a 100644
--- a/src/ai_interaction.py
+++ b/src/ai_interaction.py
@@ -22,6 +22,7 @@ import time
 from typing import Any, Awaitable, Callable, Dict, Optional, Tuple
 
 from src.constants import GENERATED_IMAGES_DIR
+from src.memory import MemoryStoreUnreadable
 
 logger = logging.getLogger(__name__)
 
@@ -384,7 +385,15 @@ async def do_manage_memory(content: str, session_id: Optional[str] = None, owner
             return {"error": "Memory text cannot be empty"}
 
         entry = _memory_manager.add_entry(text, source="ai_agent", category=category, owner=owner)
-        memories = _memory_manager.load_all()
+        # Strict load: this is a read-modify-write, and it is the path an
+        # ordinary "remember that I prefer X" takes. Degrading to [] here would
+        # save just this one entry over a store we only failed to read,
+        # atomically destroying every memory in it (issue #5673).
+        try:
+            memories = _memory_manager.load_all_for_update()
+        except MemoryStoreUnreadable as e:
+            logger.error("Refusing to add memory, store unreadable: %s", e)
+            return {"error": "Memory store is temporarily unreadable — nothing was saved."}
         memories.append(entry)
         _memory_manager.save(memories)
 
diff --git a/src/memory.py b/src/memory.py
index 1d8cdbc1e..92efbf5b2 100644
--- a/src/memory.py
+++ b/src/memory.py
@@ -10,6 +10,18 @@ from datetime import datetime
 
 logger = logging.getLogger(__name__)
 
+
+class MemoryStoreUnreadable(RuntimeError):
+    """memory.json exists on disk but could not be read or parsed.
+
+    "The contents are unknown" is categorically different from "there are no
+    memories". A read-modify-write caller that conflates the two appends to an
+    empty view and then persists it, destroying the whole store — the writes
+    are atomic, so the loss is durable. Raised by
+    :meth:`MemoryManager.load_all_for_update` so those callers fail closed.
+    """
+
+
 def tokenize(text: str) -> List[str]:
     """Simple tokenizer that splits on whitespace and removes punctuation."""
     return [word.strip('.,!?";') for word in text.split()]
@@ -110,21 +122,69 @@ class MemoryManager:
             with open(self.memory_file, 'w', encoding='utf-8') as f:
                 json.dump([], f, ensure_ascii=False, indent=2)
     
-    def load_all(self) -> List[Dict]:
-        """Load all memory entries from JSON file (unfiltered)."""
+    def _read_entries(self) -> List[Dict]:
+        """Parse the store, or raise :class:`MemoryStoreUnreadable`.
+
+        Returns ``[]`` only when the file genuinely does not exist. Every other
+        failure mode raises, so callers can tell "no memories" apart from
+        "couldn't read the memories".
+        """
         if not os.path.exists(self.memory_file):
             return []
 
         try:
             with open(self.memory_file, "r", encoding="utf-8") as f:
                 data = json.load(f)
-                if isinstance(data, list):
-                    return self._validate_entries(data)
-        except (json.JSONDecodeError, PermissionError) as e:
-            logger.error("Error loading memory.json: %s", e)
-            return self._migrate_from_legacy()
+        except OSError as e:
+            # PermissionError is an OSError (a scanner holding the file, a
+            # permissions problem, bad media).
+            raise MemoryStoreUnreadable(
+                f"cannot read {self.memory_file}: {e}"
+            ) from e
+        except json.JSONDecodeError as e:
+            # This is the branch that actually destroyed stores: the file reads
+            # back fine, so nothing stops the save that follows. A truncated
+            # memory.json is reachable because core/database.py rewrites it with
+            # a plain open(..,"w") + json.dump during migration.
+            #
+            # Preserved behaviour: a corrupt store still gets one shot at the
+            # pre-JSON memory.txt migration. Only raise when that finds nothing,
+            # so we never report "empty" for a store we simply failed to parse.
+            legacy = self._migrate_from_legacy()
+            if legacy:
+                return legacy
+            raise MemoryStoreUnreadable(
+                f"{self.memory_file} is not valid JSON: {e}"
+            ) from e
 
-        return []
+        if not isinstance(data, list):
+            raise MemoryStoreUnreadable(
+                f"{self.memory_file} is not a JSON array (got {type(data).__name__})"
+            )
+        return self._validate_entries(data)
+
+    def load_all(self) -> List[Dict]:
+        """Load all memory entries from JSON file (unfiltered).
+
+        Lenient by design: this feeds display, search, and context-injection
+        paths, so an unreadable store degrades to an empty list rather than
+        breaking chat. Never build a value from this that you intend to save
+        back — use :meth:`load_all_for_update` for that.
+        """
+        try:
+            return self._read_entries()
+        except MemoryStoreUnreadable as e:
+            logger.error("Error loading memory.json: %s", e)
+            return []
+
+    def load_all_for_update(self) -> List[Dict]:
+        """Load for a read-modify-write cycle.
+
+        Propagates :class:`MemoryStoreUnreadable` instead of degrading to ``[]``
+        so a caller can never append to an empty view and persist it over a
+        store that was only temporarily unreadable (issue #5673).
+        """
+        return self._read_entries()
 
     def load(self, owner: str = None) -> List[Dict]:
         """Load memory entries, optionally filtered by owner."""
@@ -135,7 +195,12 @@ class MemoryManager:
 
     def claim_ownerless(self, owner: str):
         """Assign all ownerless memory entries to the given owner."""
-        entries = self.load_all()
+        try:
+            entries = self.load_all_for_update()
+        except MemoryStoreUnreadable as e:
+            # Skip the sweep rather than rewrite the store from an unknown view.
+            logger.error("Skipping ownerless claim, memory store unreadable: %s", e)
+            return
         changed = False
         claimed = 0
         for entry in entries:
@@ -235,7 +300,12 @@ class MemoryManager:
         if not ids:
             return
         id_set = set(ids)
-        entries = self.load_all()
+        try:
+            entries = self.load_all_for_update()
+        except MemoryStoreUnreadable as e:
+            # Best-effort counter; never worth rewriting the store blind.
+            logger.error("Skipping uses bump, memory store unreadable: %s", e)
+            return
         changed = False
         for e in entries:
             if e.get("id") in id_set:
diff --git a/src/memory_provider.py b/src/memory_provider.py
index 925c59192..8974a6e84 100644
--- a/src/memory_provider.py
+++ b/src/memory_provider.py
@@ -157,7 +157,11 @@ class NativeMemoryProvider(MemoryProvider):
         if metadata:
             entry["metadata"] = dict(metadata)
 
-        memories = self.memory_manager.load_all()
+        # Strict load: read-modify-write. `load_all` degrades an unreadable
+        # store to [], which would save this single entry over everything
+        # already stored (issue #5673). The provider API has no error channel,
+        # so MemoryStoreUnreadable propagates to the caller.
+        memories = self.memory_manager.load_all_for_update()
         memories.append(entry)
         self.memory_manager.save(memories)
 
@@ -223,7 +227,10 @@ class NativeMemoryProvider(MemoryProvider):
         ]
 
     async def delete(self, memory_id: str, *, owner: Optional[str] = None) -> bool:
-        memories = self.memory_manager.load_all()
+        # Strict load for the same reason: `remaining` is derived from this
+        # list and saved back, so it must never be built from a store we
+        # failed to read.
+        memories = self.memory_manager.load_all_for_update()
         remaining = []
         deleted_id = None
 
diff --git a/tests/test_backup_import_cross_user_dedup.py b/tests/test_backup_import_cross_user_dedup.py
index 2df5936ef..135be78ee 100644
--- a/tests/test_backup_import_cross_user_dedup.py
+++ b/tests/test_backup_import_cross_user_dedup.py
@@ -27,6 +27,9 @@ def _setup(monkeypatch, store, user="alice"):
 
     mem = MagicMock()
     mem.load_all.return_value = list(store)
+    # import_data reads through the strict loader so a store it cannot read is
+    # never overwritten (#5673); the double has to offer the same entry point.
+    mem.load_all_for_update.return_value = list(store)
     saved = {}
     mem.save.side_effect = lambda entries: saved.__setitem__("entries", entries)
 
diff --git a/tests/test_memory_extractor_vector_cross_tenant.py b/tests/test_memory_extractor_vector_cross_tenant.py
index 49702c17f..06ca31667 100644
--- a/tests/test_memory_extractor_vector_cross_tenant.py
+++ b/tests/test_memory_extractor_vector_cross_tenant.py
@@ -67,6 +67,12 @@ class FakeMemoryManager:
     def load_all(self):
         return list(self.rows)
 
+    def load_all_for_update(self):
+        # Mirrors the real MemoryManager: extraction is a read-modify-write and
+        # goes through the strict loader (#5673). A healthy store behaves the
+        # same as load_all.
+        return list(self.rows)
+
     def load(self, owner=None):
         return [r for r in self.rows if r.get("owner") == owner]
 
diff --git a/tests/test_memory_store_unreadable_no_wipe.py b/tests/test_memory_store_unreadable_no_wipe.py
new file mode 100644
index 000000000..4b9076065
--- /dev/null
+++ b/tests/test_memory_store_unreadable_no_wipe.py
@@ -0,0 +1,255 @@
+"""A memory store that cannot be READ must never be overwritten (issue #5673).
+
+`MemoryManager.save` is atomic, and the add/import/extract paths are all
+read-modify-write: load the whole store, append, save it back. `load_all`
+used to answer a *failed read* with `[]` — indistinguishable from "no
+memories" — so a failed read turned into
+
+    load_all() -> []  ->  [].append(new)  ->  save([new])
+
+which atomically replaced the entire store with one entry.
+
+The trigger that actually bites is a store that is **readable but not
+parseable** — a truncated file, or one holding `{}` instead of `[]`. Nothing
+obstructs the write, so the request succeeds with HTTP 200 and every existing
+memory is destroyed silently. Truncation is reachable: `core/database.py`
+rewrites memory.json during migration with a plain `open(..., "w")` +
+`json.dump`, which is not atomic.
+
+A live exclusive lock is NOT the dangerous case: it blocks the read and the
+`os.replace` alike, so the save fails too and the store survives (verified
+end-to-end — clean dev returns 500 there and loses nothing).
+
+`load_all_for_update` is the strict loader those callers now use: it raises
+`MemoryStoreUnreadable` rather than reporting an empty store.
+"""
+
+import asyncio
+import builtins
+import json
+import os
+
+import pytest
+
+from src.memory import MemoryManager, MemoryStoreUnreadable
+
+_SEED = [
+    {"id": "m1", "text": "user prefers dark mode", "owner": "alice"},
+    {"id": "m2", "text": "user lives in Berlin", "owner": "alice"},
+    {"id": "m3", "text": "bob's cat is called Mila", "owner": "bob"},
+]
+
+
+def _seeded(tmp_path):
+    m = MemoryManager(str(tmp_path))
+    m.save([dict(e) for e in _SEED])
+    return m
+
+
+def _break_reads_of(monkeypatch, target, exc):
+    """Make open() raise `exc` for `target` only, leaving every other path alone."""
+    real_open = builtins.open
+
+    def fake_open(file, mode="r", *args, **kwargs):
+        if os.path.abspath(str(file)) == os.path.abspath(target) and "r" in mode:
+            raise exc
+        return real_open(file, mode, *args, **kwargs)
+
+    monkeypatch.setattr(builtins, "open", fake_open)
+
+
+# ── the strict loader signals, rather than reporting "empty" ──────────────
+
+def test_strict_load_raises_on_permission_error(tmp_path, monkeypatch):
+    m = _seeded(tmp_path)
+    _break_reads_of(monkeypatch, m.memory_file, PermissionError(13, "locked"))
+    with pytest.raises(MemoryStoreUnreadable):
+        m.load_all_for_update()
+
+
+def test_strict_load_raises_on_corrupt_json(tmp_path):
+    m = _seeded(tmp_path)
+    with open(m.memory_file, "w", encoding="utf-8") as f:
+        f.write('[{"id": "m1", "text": "truncated mid-writ')
+    with pytest.raises(MemoryStoreUnreadable):
+        m.load_all_for_update()
+
+
+def test_strict_load_raises_when_store_is_not_a_list(tmp_path):
+    # A file holding `{}` or `null` is not an empty store, it is a broken one.
+    m = _seeded(tmp_path)
+    with open(m.memory_file, "w", encoding="utf-8") as f:
+        json.dump({}, f)
+    with pytest.raises(MemoryStoreUnreadable):
+        m.load_all_for_update()
+
+
+def test_strict_load_returns_entries_when_healthy(tmp_path):
+    m = _seeded(tmp_path)
+    assert {e["id"] for e in m.load_all_for_update()} == {"m1", "m2", "m3"}
+
+
+def test_strict_load_returns_empty_when_file_genuinely_absent(tmp_path):
+    m = _seeded(tmp_path)
+    os.remove(m.memory_file)
+    # Absent is the one case that legitimately means "no memories yet".
+    assert m.load_all_for_update() == []
+
+
+# ── read paths stay lenient, so an unreadable store can't break chat ──────
+
+def test_read_path_still_degrades_to_empty(tmp_path, monkeypatch):
+    m = _seeded(tmp_path)
+    _break_reads_of(monkeypatch, m.memory_file, PermissionError(13, "locked"))
+    # Context injection / search must not raise; they just see nothing.
+    assert m.load_all() == []
+    assert m.load(owner="alice") == []
+
+
+# ── the actual #5673 regression: the store survives ───────────────────────
+
+def test_add_cycle_under_transient_read_error_does_not_wipe(tmp_path, monkeypatch):
+    """Mirrors routes/memory/memory_routes.py api_add_memory exactly."""
+    m = _seeded(tmp_path)
+    new_entry = m.add_entry("a brand new fact", owner="alice")
+
+    with monkeypatch.context() as mp:
+        _break_reads_of(mp, m.memory_file, PermissionError(13, "locked"))
+        with pytest.raises(MemoryStoreUnreadable):
+            all_mem = m.load_all_for_update()
+            all_mem.append(new_entry)
+            m.save(all_mem)
+
+    # Reads work again; every original memory is still there and the file was
+    # never replaced by the single new entry.
+    assert {e["id"] for e in m.load_all()} == {"m1", "m2", "m3"}
+
+
+def test_audit_merge_cannot_drop_other_tenants(tmp_path, monkeypatch):
+    """The audit path rebuilds the whole file from load_all + one owner's slice.
+
+    Reading [] there would save only the audited owner's entries and destroy
+    every other tenant's memories, so it has to fail closed too.
+    """
+    m = _seeded(tmp_path)
+    alice_slice = [e for e in _SEED if e["owner"] == "alice"]
+
+    with monkeypatch.context() as mp:
+        _break_reads_of(mp, m.memory_file, PermissionError(13, "locked"))
+        with pytest.raises(MemoryStoreUnreadable):
+            all_entries = m.load_all_for_update()
+            others = [e for e in all_entries if e.get("owner") != "alice"]
+            m.save(alice_slice + others)
+
+    assert any(e["id"] == "m3" for e in m.load_all()), "bob's memory was destroyed"
+
+
+def test_uses_bump_skips_write_when_unreadable(tmp_path, monkeypatch):
+    m = _seeded(tmp_path)
+    with monkeypatch.context() as mp:
+        _break_reads_of(mp, m.memory_file, PermissionError(13, "locked"))
+        m.increment_uses(["m1"])  # must not raise, must not write
+    assert {e["id"] for e in m.load_all()} == {"m1", "m2", "m3"}
+
+
+def test_claim_ownerless_skips_write_when_unreadable(tmp_path, monkeypatch):
+    m = _seeded(tmp_path)
+    with monkeypatch.context() as mp:
+        _break_reads_of(mp, m.memory_file, PermissionError(13, "locked"))
+        m.claim_ownerless("alice")
+    assert {e["id"] for e in m.load_all()} == {"m1", "m2", "m3"}
+
+
+# ── the add sinks users actually reach ────────────────────────────────────
+#
+# The tests above replay the read-modify-write shape. These drive the real
+# entry points end to end, because those are what #5673 reports: "remember
+# that I prefer X" in ordinary chat (src/ai_interaction.py do_manage_memory,
+# routed from src/tool_execution.py) and the built-in memory MCP server
+# (mcp_servers/memory_server.py, registered in src/builtin_mcp.py).
+#
+# They use a truncated store rather than a read error on purpose: it reads
+# fine, so nothing stops the save, which is the case that silently destroyed
+# stores. The assertion is that the file is left byte-identical — still broken,
+# but still holding the user's memories, so it can be repaired by hand.
+
+
+def _truncated_store(tmp_path):
+    """Seed a store that reads back fine but no longer parses."""
+    m = _seeded(tmp_path)
+    good = json.dumps([dict(e) for e in _SEED], indent=2)
+    with open(m.memory_file, "w", encoding="utf-8") as f:
+        f.write(good[:good.rindex("]")])  # drop the closing bracket only
+    with open(m.memory_file, "rb") as f:
+        return m, f.read()
+
+
+def _on_disk(manager) -> bytes:
+    with open(manager.memory_file, "rb") as f:
+        return f.read()
+
+
+def test_agent_memory_add_does_not_overwrite_unreadable_store(tmp_path, monkeypatch):
+    """src/ai_interaction.py do_manage_memory, action "add"."""
+    from src import ai_interaction
+
+    manager, before = _truncated_store(tmp_path)
+    monkeypatch.setattr(ai_interaction, "_memory_manager", manager)
+    monkeypatch.setattr(ai_interaction, "_memory_vector", None)
+
+    result = asyncio.run(ai_interaction.do_manage_memory("add\nuser prefers tabs"))
+
+    assert _on_disk(manager) == before, "the unreadable store was overwritten"
+    assert b"m3" in _on_disk(manager)
+    assert "error" in result, "the add reported success over an unreadable store"
+
+
+def test_mcp_memory_add_does_not_overwrite_unreadable_store(tmp_path, monkeypatch):
+    """mcp_servers/memory_server.py, action "add"."""
+    import mcp_servers.memory_server as memory_server
+
+    manager, before = _truncated_store(tmp_path)
+    monkeypatch.setattr(memory_server, "_memory_manager", manager)
+    monkeypatch.setattr(memory_server, "_memory_vector", None)
+    monkeypatch.setattr(memory_server, "_initialized", True)
+    for key in memory_server._OWNER_ENV_KEYS:
+        monkeypatch.delenv(key, raising=False)
+
+    result = asyncio.run(memory_server.call_tool(
+        "manage_memory", {"action": "add", "text": "user prefers tabs"}
+    ))
+
+    assert _on_disk(manager) == before, "the unreadable store was overwritten"
+    assert b"m3" in _on_disk(manager)
+    assert result[0].text.startswith("Error:")
+
+
+def test_native_provider_remember_does_not_overwrite_unreadable_store(tmp_path):
+    """src/memory_provider.py NativeMemoryProvider.remember.
+
+    Registered into app state in src/app_initializer.py but not yet consumed
+    outside tests, so this is the pattern held in place before it goes live.
+    """
+    from src.memory_provider import NativeMemoryProvider
+
+    manager, before = _truncated_store(tmp_path)
+    provider = NativeMemoryProvider(manager)
+
+    with pytest.raises(MemoryStoreUnreadable):
+        asyncio.run(provider.remember("user prefers tabs", owner="alice"))
+
+    assert _on_disk(manager) == before
+
+
+# ── the legacy memory.txt migration is preserved ──────────────────────────
+
+def test_corrupt_store_still_migrates_from_legacy_txt(tmp_path):
+    m = _seeded(tmp_path)
+    with open(m.memory_file, "w", encoding="utf-8") as f:
+        f.write("{ not json")
+    legacy = os.path.join(str(tmp_path), "memory.txt")
+    with open(legacy, "w", encoding="utf-8") as f:
+        f.write("recovered fact one\nrecovered fact two\n")
+
+    entries = m.load_all_for_update()
+    assert [e["text"] for e in entries] == ["recovered fact one", "recovered fact two"]