mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
fix(memory): quarantine unreadable memory summaries (#7076)
* fix(memory): quarantine unreadable memory.json in the read-error branch too MarkdownMemoryStorage._load_memory_file quarantined summaries that failed to decode as JSON/Markdown, but the earlier except (OSError, UnicodeError) branch (unreadable bytes, e.g. a UTF-16 BOM) still returned None without quarantining. The leftover file was then silently overwritten by the next save, destroying the user's memories with no backup. Call the existing best-effort self._quarantine_unreadable(path) there as well, matching the documented quarantine contract, and add a regression test asserting the original bytes survive a subsequent load+save. Fixes #7072 * fix(memory): preserve summaries on transient read failures --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
This commit is contained in:
parent
68d4e5527c
commit
2174c1d81e
@ -2913,8 +2913,10 @@ and agents. Failed extraction attempts are retryable execution errors, not safet
|
||||
For DeerMem, `memory.backend_config.storage_class: markdown` opts into tolerant
|
||||
summary reads while keeping JSON writes and the existing UI. A hand-edited
|
||||
`memory.json` can contain its JSON object inside a fenced `memory-json` block;
|
||||
embedded backticks and later fenced notes are supported. Unparseable summary
|
||||
text is moved to `memory.json.corrupt-<timestamp>` for recovery before rebuilding.
|
||||
embedded backticks and later fenced notes are supported. Invalid UTF-8 bytes and
|
||||
unparseable summary text are moved to `memory.json.corrupt-<timestamp>` for recovery
|
||||
before rebuilding. File-access and I/O read errors preserve the original file and
|
||||
fail the storage operation; retry after access recovers.
|
||||
`storage_path` must point to the data root directory, not an existing JSON file.
|
||||
Hand-edited DeerMem summaries and fact Markdown files may have a leading UTF-8
|
||||
BOM; reads accept it, while writes remain BOM-free UTF-8.
|
||||
|
||||
@ -151,7 +151,9 @@ fence is accepted from Markdown. Decode the value before checking its closing
|
||||
fence so embedded backticks and later fenced notes stay lossless. Parser and
|
||||
storage regressions belong in `tests/test_memory_storage.py`; configure a data
|
||||
root directory, not the existing manifest file. Legacy v1 reads still migrate
|
||||
and advance the revision. Unparseable summary text is quarantined before rebuild.
|
||||
and advance the revision. Invalid UTF-8 bytes and unparseable summary text are
|
||||
quarantined before rebuild. Summary read `OSError`s raise `MemoryStorageCorruption`
|
||||
and preserve the file; `save()` returns false until access recovers.
|
||||
Summary and canonical fact reads strip a leading UTF-8 BOM; writes use UTF-8.
|
||||
Fact BOM regressions use public save/reload and preserve content and scope.
|
||||
|
||||
|
||||
@ -11,21 +11,23 @@ backward compatibility, but its loader is *tolerant*:
|
||||
* a corrupt or partially written summary no longer crashes the agent;
|
||||
* a Markdown summary is accepted only through its fenced ```` ```memory-json ````
|
||||
block, which is parsed losslessly;
|
||||
* when the on-disk file is unreadable (neither valid JSON nor a Markdown
|
||||
summary with a usable fenced block), it is *quarantined* as
|
||||
* invalid UTF-8 bytes, or text that is neither valid JSON nor a Markdown
|
||||
summary with a usable fenced block, trigger best-effort quarantine as
|
||||
``memory.json.corrupt-<timestamp>`` before the loader returns ``None``.
|
||||
Quarantining keeps the content recoverable: returning ``None`` alone would
|
||||
make the next :meth:`save` rebuild the manifest from scratch (revision
|
||||
reset, no journal backup) and silently erase the unreadable state.
|
||||
reset, no journal backup) and silently erase the unreadable state;
|
||||
* file-access and I/O read failures raise ``MemoryStorageCorruption``, matching
|
||||
the default storage error contract. The original file stays in place, and
|
||||
reads and writes can be retried after access recovers.
|
||||
|
||||
Writes still persist JSON (the write path is untouched). Hand-edited
|
||||
Markdown files are therefore a *read-time* convenience: the next write
|
||||
rewrites ``memory.json`` as JSON, so the Markdown rendering is temporary
|
||||
until a Markdown write path lands.
|
||||
|
||||
This is intentionally a small, additive change scoped to the load path only:
|
||||
the JSON UI and all other backends are untouched. Enabling it cannot break
|
||||
existing deployments.
|
||||
The tolerant decoding policy is scoped to this opt-in loader. Summary I/O
|
||||
failures use the same storage error contract as the default backend.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
@ -36,13 +38,13 @@ from pathlib import Path
|
||||
from typing import Any
|
||||
|
||||
from .markdown_format import _parse_markdown_memory
|
||||
from .storage import FileMemoryStorage, logger
|
||||
from .storage import FileMemoryStorage, MemoryStorageCorruption, logger
|
||||
|
||||
|
||||
class MarkdownMemoryStorage(FileMemoryStorage):
|
||||
"""File-backed storage whose summary loader tolerates corrupt/Markdown.
|
||||
|
||||
Fully opt-in. Enable via ``memory.storage_class: markdown`` (or the full
|
||||
Fully opt-in. Enable via ``memory.backend_config.storage_class: markdown`` (or the full
|
||||
import path ``deerflow.agents.memory.backends.deermem.deermem.core.
|
||||
markdown_storage.MarkdownMemoryStorage``). The default JSON summary
|
||||
format is unchanged, so the existing JSON UI and all other backends keep
|
||||
@ -56,9 +58,17 @@ class MarkdownMemoryStorage(FileMemoryStorage):
|
||||
# utf-8-sig, matching the base loader: a BOM does not make the
|
||||
# summary unreadable, so it must not trigger the quarantine path.
|
||||
raw = path.read_text(encoding="utf-8-sig")
|
||||
except (OSError, UnicodeError) as exc:
|
||||
logger.warning("Cannot read memory summary %s: %s", path, exc)
|
||||
except UnicodeError as exc:
|
||||
logger.warning("Cannot decode memory summary %s as UTF-8: %s", path, exc)
|
||||
# Invalid text has the same quarantine contract as an unparseable
|
||||
# summary below: preserve its bytes before allowing a rebuild.
|
||||
self._quarantine_unreadable(path)
|
||||
return None
|
||||
except OSError as exc:
|
||||
# A failed read does not establish that the bytes are corrupt.
|
||||
# Match the default storage error contract, preserving the file
|
||||
# and preventing a write from rebuilding over a transient failure.
|
||||
raise MemoryStorageCorruption(f"Failed to read memory summary {path}: {exc}") from exc
|
||||
|
||||
parsed: dict[str, Any] | None = None
|
||||
try:
|
||||
|
||||
@ -1,7 +1,9 @@
|
||||
"""Tests for memory storage providers (DI: FileMemoryStorage(config) / create_storage)."""
|
||||
|
||||
import errno
|
||||
import json
|
||||
import threading
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
@ -13,6 +15,7 @@ from deerflow.agents.memory.backends.deermem.deermem.core.paths import validate_
|
||||
from deerflow.agents.memory.backends.deermem.deermem.core.storage import (
|
||||
FileMemoryStorage,
|
||||
MemoryStorage,
|
||||
MemoryStorageCorruption,
|
||||
create_empty_memory,
|
||||
create_storage,
|
||||
normalize_memory_data,
|
||||
@ -392,6 +395,23 @@ class TestMarkdownMemoryStorage:
|
||||
assert len(quarantined) == 1
|
||||
assert quarantined[0].read_text(encoding="utf-8") == truncated
|
||||
|
||||
@pytest.mark.parametrize("encoding", ["utf-16", "invalid-utf8"])
|
||||
def test_non_utf8_summary_is_quarantined_before_rebuild(self, tmp_path, encoding):
|
||||
"""Invalid UTF-8 bytes stay recoverable across repeated reads and a save."""
|
||||
memory_file = tmp_path / "memory.json"
|
||||
payload = json.dumps(create_empty_memory(), ensure_ascii=False)
|
||||
original = payload.encode("utf-16") if encoding == "utf-16" else payload.encode("utf-8") + b"\xff"
|
||||
memory_file.write_bytes(original)
|
||||
storage = self._markdown_storage_at(memory_file)
|
||||
loaded = storage.load()
|
||||
assert isinstance(loaded, dict)
|
||||
assert isinstance(storage.reload(), dict)
|
||||
assert storage.save(create_empty_memory()) is True
|
||||
assert isinstance(storage.load(), dict)
|
||||
quarantined = list(tmp_path.glob("memory.json.corrupt-*"))
|
||||
assert len(quarantined) == 1, "a non-UTF-8 summary must be quarantined, not silently rebuilt over"
|
||||
assert quarantined[0].read_bytes() == original
|
||||
|
||||
def test_fenced_block_containing_backticks_parses_losslessly(self):
|
||||
"""Remembered code snippets must not terminate the JSON block."""
|
||||
manifest = create_empty_memory()
|
||||
@ -432,6 +452,59 @@ class TestMarkdownMemoryStorage:
|
||||
assert mf._parse_markdown_memory(body) is None
|
||||
|
||||
|
||||
class TestMarkdownMemoryReadFailures:
|
||||
@pytest.mark.parametrize("error_number", [errno.EACCES, errno.EIO, getattr(errno, "ESTALE", errno.EIO), errno.EBUSY])
|
||||
@pytest.mark.parametrize("operation", ["load", "reload", "save", "apply_changes"])
|
||||
@pytest.mark.parametrize("cached", [False, True])
|
||||
def test_read_error_preserves_summary_and_retries_after_recovery(self, tmp_path, monkeypatch, error_number, operation, cached):
|
||||
memory_file = tmp_path / "memory.json"
|
||||
manifest = create_empty_memory()
|
||||
manifest["version"] = "2.0"
|
||||
manifest.pop("facts")
|
||||
manifest["revision"] = 41
|
||||
manifest["user"]["workContext"]["summary"] = "private saved summary"
|
||||
original = json.dumps(manifest, ensure_ascii=False).encode("utf-8")
|
||||
memory_file.write_bytes(original)
|
||||
storage = MarkdownMemoryStorage(DeerMemConfig(storage_path=str(tmp_path), retrieval_adapter=""))
|
||||
try:
|
||||
if cached:
|
||||
assert storage.load()["revision"] == 41
|
||||
replacement = create_empty_memory()
|
||||
replacement["user"]["workContext"]["summary"] = "replacement summary"
|
||||
read_text = Path.read_text
|
||||
|
||||
def fail_summary_read(path, *args, **kwargs):
|
||||
if path == memory_file:
|
||||
raise OSError(error_number, "synthetic summary read failure")
|
||||
return read_text(path, *args, **kwargs)
|
||||
|
||||
with monkeypatch.context() as patcher:
|
||||
patcher.setattr(Path, "read_text", fail_summary_read)
|
||||
if operation == "save":
|
||||
assert storage.save(replacement) is False
|
||||
else:
|
||||
with pytest.raises(MemoryStorageCorruption, match="Failed to read memory summary") as raised:
|
||||
if operation == "apply_changes":
|
||||
storage.apply_changes({"summaries": {"user": replacement["user"]}})
|
||||
else:
|
||||
getattr(storage, operation)()
|
||||
assert isinstance(raised.value.__cause__, OSError)
|
||||
assert raised.value.__cause__.errno == error_number
|
||||
|
||||
assert memory_file.read_bytes() == original
|
||||
assert list(tmp_path.glob("memory.json.corrupt-*")) == []
|
||||
assert not (tmp_path / ".memory.journal.json").exists()
|
||||
recovered = storage.load()
|
||||
assert recovered["revision"] == 41
|
||||
assert recovered["user"]["workContext"]["summary"] == "private saved summary"
|
||||
assert storage.save(replacement, expected_revision=41) is True
|
||||
saved = storage.reload()
|
||||
assert saved["revision"] == 42
|
||||
assert saved["user"]["workContext"]["summary"] == "replacement summary"
|
||||
finally:
|
||||
storage.close()
|
||||
|
||||
|
||||
class TestFactMarkdownBom:
|
||||
"""Hand-edited canonical facts must survive the complete storage read path."""
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user