fix(memory): add a cross-process file lock so separate server processes do not overwrite each other - #4744
Open
daichiyasunami-vottia wants to merge 1 commit into
Conversation
…es do not overwrite each other modelcontextprotocol#4555 serialises mutations within one process. The server is stdio-only, so each client is its own process with its own queue: two editor windows or two git worktrees pointed at one MEMORY_FILE_PATH still overwrite each other's load->mutate->save. Measured on d73f99e with 20 writes split across two processes: exactly 10 survive, every time - each process serialises its own half correctly and the last one to write the file wins. Wrap each mutation in a sidecar lock (MEMORY_FILE_PATH.lock) created with O_EXCL, which is atomic on POSIX and Windows and needs no dependency. A lock older than 30s is treated as left by a crashed process and reclaimed. The in-process queue stays, so one process never contends with itself. Refs modelcontextprotocol#1819, modelcontextprotocol#3286
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Stale-lock reclamation can admit multiple holders and reintroduce lost updates.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds cross-process locking to prevent concurrent memory-server instances from overwriting shared graph data.
Changes:
- Wraps mutations with a sidecar file lock.
- Adds contention, cleanup, and stale-lock tests.
File summaries
| File | Description |
|---|---|
src/memory/index.ts |
Implements cross-process mutation locking. |
src/memory/__tests__/cross-process-lock.test.ts |
Tests lock behavior across manager instances. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+137
to
+140
| const stat = await fs.stat(this.lockFilePath); | ||
| if (Date.now() - stat.mtimeMs > KnowledgeGraphManager.LOCK_STALE_MS) { | ||
| await fs.unlink(this.lockFilePath).catch(() => {}); | ||
| continue; |
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.
Refs #1819, #3286. Builds on #4555.
#4555 serialises mutations within one process, and it fully fixes that case. The server is stdio-only, so every client is its own process with its own queue: two editor windows, or two git worktrees pointed at one
MEMORY_FILE_PATH, still overwrite each other's load→mutate→save.Measured on
d73f99e(this branch's base), 20create_entitiescalls:Each process serialises its own half correctly and the last one to write the file wins. Every call returns success.
Change (one method, no new dependency): wrap each mutation in a sidecar lock
MEMORY_FILE_PATH.lockcreated withfs.open(..., 'wx')— O_EXCL is atomic on POSIX and Windows. Retry with a short backoff; a lock older than 30 s is treated as left by a crashed process and reclaimed. The in-process queue is kept, so a process never contends with itself. With this branch themultirow is 20/20.Relation to #3286: same goal. That PR is now
CONFLICTINGagainst main after #4555 and addsproper-lockfile(+1007 lines). This is the minimal version on top of the current queue; happy to close in favour of a rebased #3286 if that is preferred.Tests:
__tests__/cross-process-lock.test.ts— two instances on one file (a faithful stand-in for two processes, since the queue is per instance) keep 20/20; the lock file is removed after each mutation; a stale lock is reclaimed. Full suite: 90 passed.