mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-08-08 06:28:37 -04:00
Ashvin
c8a012d4d2
* 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.
64 lines
2.5 KiB
Python
64 lines
2.5 KiB
Python
"""Backup import must dedup memories against the importing user only.
|
|
|
|
import_data deduped incoming memories against memory_manager.load_all()
|
|
(every tenant\'s rows), so a memory whose text matched ANY other user\'s
|
|
memory was silently skipped - the importing user lost their own data. The
|
|
dedup must be scoped to the caller\'s own memories. The full multi-tenant
|
|
store is still saved back.
|
|
"""
|
|
import asyncio
|
|
from types import SimpleNamespace
|
|
from unittest.mock import MagicMock
|
|
|
|
import routes.backup_routes as br
|
|
|
|
|
|
class _Req:
|
|
def __init__(self, body):
|
|
self._body = body
|
|
|
|
async def json(self):
|
|
return self._body
|
|
|
|
|
|
def _setup(monkeypatch, store, user="alice"):
|
|
monkeypatch.setattr(br, "require_admin", lambda request: None)
|
|
monkeypatch.setattr(br, "get_current_user", lambda request: user)
|
|
|
|
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)
|
|
|
|
skills = MagicMock()
|
|
skills.load_all.return_value = []
|
|
router = br.setup_backup_routes(mem, MagicMock(), skills)
|
|
endpoint = None
|
|
for r in router.routes:
|
|
if r.path == "/api/import" and "POST" in getattr(r, "methods", set()):
|
|
endpoint = r.endpoint
|
|
assert endpoint is not None
|
|
return endpoint, saved
|
|
|
|
|
|
def test_user_can_import_memory_matching_another_users_text(monkeypatch):
|
|
# bob already has "buy milk"; alice imports her own "Buy Milk".
|
|
endpoint, saved = _setup(monkeypatch, [{"text": "buy milk", "owner": "bob"}])
|
|
body = {"memories": [{"text": "Buy Milk"}]}
|
|
asyncio.run(endpoint(_Req(body)))
|
|
texts_by_owner = {(e.get("owner"), e.get("text")) for e in saved["entries"]}
|
|
assert ("alice", "Buy Milk") in texts_by_owner # not dropped as a "duplicate"
|
|
assert ("bob", "buy milk") in texts_by_owner # other tenant preserved
|
|
|
|
|
|
def test_users_own_duplicate_is_still_skipped(monkeypatch):
|
|
endpoint, saved = _setup(monkeypatch, [{"text": "buy milk", "owner": "alice"}])
|
|
body = {"memories": [{"text": "Buy Milk"}]}
|
|
asyncio.run(endpoint(_Req(body)))
|
|
alice_milk = [e for e in saved["entries"]
|
|
if e.get("owner") == "alice" and e.get("text", "").lower() == "buy milk"]
|
|
assert len(alice_milk) == 1 # the real duplicate is still deduped
|