fix(memory): write the knowledge graph atomically - #4642
Open
teddiesloco wants to merge 1 commit into
Open
Conversation
`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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
saveGraph()insrc/memory/index.tswrote the knowledge graph directly to the memory file:fs.writeFileopens 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:
os.tmpdir()would fail withEXDEVwhenever the memory file lives on a different mount — a common setup when the memory file is on a mounted volume.Fixes #4614
Server Details
memoryMotivation 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:
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.tswith five tests:Tests 1 and 4 fail against the previous implementation and pass after the fix, so they pin the regression rather than merely describing it.
npx tsc --noEmitis clean.I also verified the built output end-to-end: created entities, forced
renameto 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
Checklist
Additional context
I kept the change scoped to
saveGraph(). Two adjacent hardening steps were deliberately left out to keep this reviewable and focused:renameis atomic with respect to ordering, but without anfsyncon 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.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.