diff --git a/README.md b/README.md index 56e1ff65f6..418b5ef25f 100644 --- a/README.md +++ b/README.md @@ -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-` 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-` 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. diff --git a/backend/packages/harness/deerflow/agents/memory/AGENTS.md b/backend/packages/harness/deerflow/agents/memory/AGENTS.md index 6873f114e3..2d3395717f 100644 --- a/backend/packages/harness/deerflow/agents/memory/AGENTS.md +++ b/backend/packages/harness/deerflow/agents/memory/AGENTS.md @@ -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. diff --git a/backend/packages/harness/deerflow/agents/memory/backends/deermem/deermem/core/markdown_storage.py b/backend/packages/harness/deerflow/agents/memory/backends/deermem/deermem/core/markdown_storage.py index 07ff73810b..2454d72de0 100644 --- a/backend/packages/harness/deerflow/agents/memory/backends/deermem/deermem/core/markdown_storage.py +++ b/backend/packages/harness/deerflow/agents/memory/backends/deermem/deermem/core/markdown_storage.py @@ -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-`` 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: diff --git a/backend/tests/test_memory_storage.py b/backend/tests/test_memory_storage.py index 73d1dd7d20..571aaee42e 100644 --- a/backend/tests/test_memory_storage.py +++ b/backend/tests/test_memory_storage.py @@ -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."""