Skip to content

fix(memory): re-read and merge on save_to_disk to prevent concurrent data loss - #96195

Open
michielhdoteth wants to merge 1 commit into
NousResearch:mainfrom
michielhdoteth:main
Open

fix(memory): re-read and merge on save_to_disk to prevent concurrent data loss#96195
michielhdoteth wants to merge 1 commit into
NousResearch:mainfrom
michielhdoteth:main

Conversation

@michielhdoteth

@michielhdoteth michielhdoteth commented Aug 27, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes silent MEMORY.md data loss when two Hermes instances (or a Hermes instance + any external writer — daemon, cron, script) share one profile and write concurrently. save_to_disk() now re-reads the file under the already-held lock from #1726 and merges on-disk entries with the session's pending entries before writing. Last-writer-wins on content is eliminated; no entry from any writer is silently dropped.

Closes #85858.

Related Issue

Fixes #85858

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✅ Tests (adding or improving test coverage)

Changes Made

  • tools/memory_tool.py:387save_to_disk() now re-reads the file under the already-held _file_lock and merges on-disk entries with the session's pending entries before writing. New _merge_disk_with_pending() method implements the merge (union-where-session-wins, deduplicated). +58 lines.
  • tests/tools/test_memory_concurrent_85858.py — new file. 6 tests reproducing the concurrent-snapshot-staleness bug and verifying the fix:
  • tests/tools/test_memory_tool.py — updated test_mutations_read_the_file_exactly_once docstring/assertions to distinguish the drift-detection single-read invariant (still fixed) from the new intentional merge-read in save_to_disk. -1/+19 lines.

Total: 3 files, +290/-6 lines, 47 tests pass, 0 regressions.

How to Test

  1. Clone the PR branch: gh pr checkout 96195
  2. Run the full test suite: pytest tests/ -q — all tests pass
  3. Run the new concurrent tests specifically: pytest tests/tools/test_memory_concurrent_85858.py -v — 6/6 pass
  4. Reproduction of the original bug (before fix): the test test_concurrent_adds_both_survive encodes the exact reproduction from MEMORY.md lost under concurrent Hermes instances sharing one profile (frozen-snapshot + full-file rewrite = last-writer-wins) #85858 — session A adds X, session B adds Y, session A saves from stale snapshot. Before the fix this test fails (Y is lost); after the fix it passes (both X and Y survive).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(memory):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • N/A — no tool behavior change that needs doc update; the memory tool's contract ("Mid-session writes update files on disk immediately (durable)") is now actually true under concurrency, which the docstring at tools/memory_tool.py:11-14 already claims.
  • N/A — no new config keys, no CLI changes
  • N/A — CONTRIBUTING.md and AGENTS.md unchanged
  • N/A — cross-platform: fix is Windows + Unix compatible (uses the existing _file_lock from fix(memory): concurrent writes silently drop entries — add file locking #1726 and stdlib dict.fromkeys)
  • N/A — tool description/schema unchanged

Screenshots / Logs

N/A — behavior change, not UI. Test output demonstrates the fix:

tests/tools/test_memory_concurrent_85858.py::TestConcurrentSnapshotStaleness::test_concurrent_adds_both_survive PASSED
tests/tools/test_memory_concurrent_85858.py::TestConcurrentSnapshotStaleness::test_concurrent_add_replace_both_survive PASSED
tests/tools/test_memory_concurrent_85858.py::TestConcurrentSnapshotStaleness::test_concurrent_adds_no_duplicate PASSED
tests/tools/test_memory_concurrent_85858.py::TestExternalWriterPreservation::test_external_append_preserved PASSED
tests/tools/test_memory_concurrent_85858.py::TestExternalWriterPreservation::test_external_single_entry_preserved PASSED
tests/tools/test_memory_concurrent_85858.py::TestExternalWriterPreservation::test_multiple_external_entries_preserved PASSED

…data loss

save_to_disk() previously wrote self._entries_for(target) without
re-reading the file, so a session's stale in-memory snapshot could
overwrite entries from concurrent writers (other sessions, daemons,
cron jobs, scripts) — issue NousResearch#85858.

The fix re-reads the file under the already-held lock and merges
on-disk entries with pending entries before writing. This preserves
writes from external writers that don't use the memory tool's lock
(the primary gap) and adds defense-in-depth for concurrent sessions
(beyond PR NousResearch#1726's file locking).

Merge strategy: union-where-session-wins — all pending entries
preserved, on-disk entries not in pending appended, no duplicates.
Conflicts (same entry modified by both) result in both versions
present — safe, no data loss.

Closes NousResearch#85858

Co-Authored-By: Hermes Agent <noreply@nousresearch.com>
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists tool/memory Memory tool and memory providers area/memory Memory subsystem: store, providers, sync, background reviews sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/memory Memory subsystem: store, providers, sync, background reviews P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MEMORY.md lost under concurrent Hermes instances sharing one profile (frozen-snapshot + full-file rewrite = last-writer-wins)

2 participants