Repository navigation
fix(memory): quarantine unreadable memory summaries - #7076
Conversation
github-actions
Bot
added
area:agents
…ch 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 bytedance#7072
cc50270 to
069e9bd
Compare
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed at 069e9bd. Verified independently: the new regression test fails at the merge base (62130ca — zero quarantine files) and passes at this head; backend/tests/test_memory_storage.py is 45/45 and ruff check/format are clean on both changed files. The fix correctly closes the one path that did not honor the quarantine contract documented in this module's docstring — the read-error branch previously returned None without preserving the file, letting the next save rebuild over it (the #7072 data-loss scenario). The rename-based quarantine is self-limiting (the unreadable file is moved aside, so repeated loads do not multiply memory.json.corrupt-* files), and quarantined bytes stay recoverable manually, matching the existing parse-path helper's contract. Also consistent with #6971's tolerate-one-bad-item direction. Two non-blocking notes below.
| # Same contract as the decode/fence path below: quarantine the | ||
| # unreadable file (best-effort) so the next save cannot silently | ||
| # overwrite the user's memory with a fresh manifest. | ||
| self._quarantine_unreadable(path) |
There was a problem hiding this comment.
Suggestion (non-blocking): this branch now quarantines on any OSError, not only bad bytes. Transient or permission read failures (EACCES, EIO/ESTALE on NFS, Windows sharing violations) can hit a file whose bytes are perfectly fine; the rename then moves a healthy summary aside and memory resets (revision reset, empty summary) until someone manually restores the memory.json.corrupt-* file — on POSIX, Path.replace only needs write permission on the directory, so it succeeds even when the read itself was denied. That is still strictly better than the pre-fix behavior (return None and let the next save overwrite the bytes), so nothing to fix here, but if a deployment reports memory "resets" after transient I/O errors, this branch is why; splitting UnicodeError (deterministic decode failure) from OSError (possibly transient) would be the escape hatch.
There was a problem hiding this comment.
Nit: the module docstring (lines 14-19 of this file) still enumerates the quarantine trigger only for the parse path ("neither valid JSON nor a Markdown summary with a usable fenced block"); with this change the decode-error branch triggers quarantine too, so that paragraph could mention the read-error case to stay exact.
github-actions
Bot
added
area:docs
Fixes #7072
Why
MarkdownMemoryStoragereadsmemory.jsonas UTF-8. A UTF-16/BOM or otherwise invalid UTF-8 summary raisesUnicodeErrorbefore parsing; previously that read-error branch returnedNonewithout preserving the original file, allowing the next save to rebuild over it. This is the data-loss scenario described in #7072.What changed
_quarantine_unreadablehelper before returning from the read-error branch.Surface area
MarkdownMemoryStorage)Bug fix verification
backend/tests/test_memory_storage.py::TestMarkdownMemoryStorage::test_non_utf8_summary_is_quarantined_before_rebuild62130caa) with zero quarantined files and passes on this branch. The changed read-error path now invokes the same quarantine helper used for parse failures.Validation
cd backend && .venv/bin/python -m pytest tests/test_memory_storage.py tests/test_memory_storage_markdown.py tests/test_memory_storage_user_isolation.py tests/test_memory_retrieval_index_local.py tests/test_memory_markdown_migration_cli.py -q --no-header— 147 passed, 1 skipped.uv build --package deerflow-harness --wheel— passed.git diff --check— passed.AI assistance
Tool(s) used: GLM coding agent (zai-coding-cn/glm-5.3-flash) for implementation; pi coding agent for independent verification.
How you used it: Drafted under my direction — the agents implemented the fix and regression test and ran the affected test modules, wheel build, lint, and format checks. I reviewed every line of the diff and take responsibility for this change.