Skip to content

fix(memory): write the knowledge graph atomically - #4642

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

fix(memory): write the knowledge graph atomically#4642
teddiesloco wants to merge 1 commit into
modelcontextprotocol:mainfrom
teddiesloco:fix/memory-atomic-save

Conversation

@teddiesloco

Copy link
Copy Markdown

Description

saveGraph() in src/memory/index.ts wrote the knowledge graph directly to the memory file:

await fs.writeFile(this.memoryFilePath, lines.join("\n"));

fs.writeFile opens the target with 'w', which truncates it before any new bytes are written. If the process is interrupted between truncation and completion — SIGKILL, container or host stop, OOM kill, power loss — the memory file is left empty or half-written.

Since that file is the only persistence layer for the graph, the window is small but the loss is total and unrecoverable.

This PR writes to a temporary file in the same directory and then renames it over the target. rename(2) is atomic on POSIX filesystems: a reader sees either the complete old file or the complete new one, never an intermediate state.

Two implementation details worth calling out:

  • The temp file is created in the same directory as the target, so the rename stays on one filesystem. A temp file in os.tmpdir() would fail with EXDEV whenever the memory file lives on a different mount — a common setup when the memory file is on a mounted volume.
  • On failure the temp file is unlinked, so repeated failures cannot accumulate strays next to the memory file.

Fixes #4614

Server Details

  • Server: memory
  • Changes to: internal persistence only — no tool, resource, or prompt signatures change

Motivation and Context

The memory server exists to accumulate knowledge across sessions, so durability is the one property users depend on most. The reporter of #4614 hit this with a backup script that stops a Distrobox/Podman container: if the server happens to be mid-write when the container stops, the accumulated graph can be lost.

I reproduced the underlying filesystem behaviour directly to confirm the mechanism rather than assuming it:

before : "ENTITY-A\nENTITY-B\nENTITY-C\n"
after   : "PARTIAL"

Opening the live file with 'w' and dying before the write completes destroys the committed data.

How Has This Been Tested?

Added src/memory/__tests__/atomic-save.test.ts with five tests:

  1. the live memory file is never opened for truncating writes
  2. a committed graph survives a write that fails midway
  3. no temporary files remain after a successful write
  4. the temporary file is cleaned up when the rename fails
  5. graph contents still round-trip correctly across reloads

Tests 1 and 4 fail against the previous implementation and pass after the fix, so they pin the regression rather than merely describing it.

Test Files  4 passed (4)
     Tests  55 passed (55)      # 50 existing + 5 new

npx tsc --noEmit is clean.

I also verified the built output end-to-end: created entities, forced rename to throw to simulate a crash at commit time, and confirmed the previously committed entities were still intact with no temp files left behind.

Breaking Changes

None. No configuration or client changes are required, and the on-disk format is unchanged.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly — not applicable, no user-facing behaviour or configuration changes
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options — not applicable, no new options

Additional context

I kept the change scoped to saveGraph(). Two adjacent hardening steps were deliberately left out to keep this reviewable and focused:

  • fsync before rename. rename is atomic with respect to ordering, but without an fsync on the temp file the new contents may still be in the page cache during a power loss. Adding it costs a sync on every graph mutation, which is a real write-throughput tradeoff — worth a separate discussion.
  • Concurrent writer serialisation. The temp filename includes the pid and a timestamp so two processes cannot collide on the same temp path, but concurrent saveGraph() calls still race on last-writer-wins. That is pre-existing behaviour and orthogonal to this fix.

Happy to fold either in if maintainers prefer.

`saveGraph()` wrote the graph straight to the memory file:

    await fs.writeFile(this.memoryFilePath, lines.join("\n"));

`fs.writeFile` opens the target with `'w'`, which truncates it before any
new bytes are written. If the process is interrupted between truncation
and completion — SIGKILL, container or host stop, OOM kill, power loss —
the memory file is left empty or half-written.

That file is the only persistence layer for the knowledge graph, so the
window is small but the loss is total and unrecoverable.

The fix writes to a temporary file in the same directory and renames it
over the target. `rename(2)` is atomic on POSIX filesystems: a reader
sees either the complete old file or the complete new one, never an
intermediate state. Keeping the temp file in the same directory ensures
the rename stays on one filesystem, since cross-device renames fail with
EXDEV. On failure the temp file is removed so no strays accumulate.

Tests added in `__tests__/atomic-save.test.ts`:

- the live memory file is never opened for truncating writes
- a committed graph survives a write that fails midway
- no temporary files remain after a successful write
- the temporary file is cleaned up when the rename fails
- graph contents still round-trip correctly across reloads

The first and fourth fail against the previous implementation.

Full suite: 55 passed (50 existing, 5 new). `tsc --noEmit` clean.

Fixes modelcontextprotocol#4614
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

1 participant