Skip to content

fix(memory): write graph atomically in saveGraph to prevent file corruption - #4696

Open
AbhiPra24 wants to merge 1 commit into
modelcontextprotocol:mainfrom
AbhiPra24:fix/memory-atomic-save-graph
Open

fix(memory): write graph atomically in saveGraph to prevent file corruption#4696
AbhiPra24 wants to merge 1 commit into
modelcontextprotocol:mainfrom
AbhiPra24:fix/memory-atomic-save-graph

Conversation

@AbhiPra24

Copy link
Copy Markdown

Description

Fixes #4614.

In KnowledgeGraphManager.saveGraph() (src/memory/index.ts), the knowledge graph was written directly in-place to this.memoryFilePath via fs.writeFile. If the process was terminated mid-write (SIGKILL, container stop, OOM kill, power loss), the memory file could be truncated or left corrupt without recovery.

This PR makes saving atomic by:

  1. Writing the serialized graph content to a uniquely named temporary file (${this.memoryFilePath}.${Date.now()}-${random}.tmp) in the same directory.
  2. Atomically renaming the temporary file over the target this.memoryFilePath using fs.rename().
  3. Cleaning up the temporary file if an error occurs before renaming.

Changes

  • Update KnowledgeGraphManager.saveGraph in src/memory/index.ts to write to temp file then rename.
  • Add test in src/memory/__tests__/knowledge-graph.test.ts verifying atomic persistence and no leftover temp files.

Verification

  • Ran npm test and npm run build in src/memory. All 51 tests pass.

@Selven81

Copy link
Copy Markdown

Thanks — this is the most tightly scoped of the five PRs on #4614, and
splitting the relation validation out into its own change is what makes both
easier to review.

I also followed the naming convention shared by all four open PRs here: the
temp file is not hidden behind a leading dot. If a save dies badly and leaves
an orphan, it should show up in a plain ls — and the leftover-temp-file checks
written in these PRs scan with readdir by prefix, so a hidden name would make
them blind.

Summary of what I took from each PR, and how I validated the merged version:
#4614 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

saveGraph() in src/memory uses non-atomic fs.writeFile — risk of corrupted memory file on interruption

2 participants