-
Notifications
You must be signed in to change notification settings - Fork 9.4k
feat(memory): add file locking to support multi-instance #3286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
xz-dev
wants to merge
11
commits into
modelcontextprotocol:main
Choose a base branch
from
xz-dev:add-file-locking
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+993
−48
Conversation
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
53850a2 to
8c72c32
Compare
- Add proper-lockfile dependency for cross-process file locking - Wrap all write operations (create/delete entities/relations/observations) in withLock() to ensure atomic read-modify-write - Add chaos test validating lock consistency under 10k concurrent writes
…ile locking Spawns 5 worker processes each writing 2000 entities to verify that proper-lockfile correctly prevents data loss across separate processes. This simulates the real-world scenario where multiple MCP clients spawn separate mcp-server-memory instances via stdio.
- Increase stale timeout to 60s (safe for NFS acregmax) - Add 10s update heartbeat for lock freshness - Refactor lockOptions to use spread for easier customization - Update test to match new lockOptions structure
- Move worker logic into multi-process-lock.test.ts to make the test self-contained - Remove separate worker script - Add tsx to devDependencies to support running the worker process - Fix vitest import issue in worker process
Replace manual temp file + rename implementation with write-file-atomic package, which provides more robust atomic write handling including: - Automatic concurrent write serialization - Better temp file naming with murmur hash - Automatic cleanup on errors - Worker thread support Disabled fsync for better performance.
- Add missing @types/write-file-atomic to fix TypeScript build error - Refactor withLock to use try/finally for clearer control flow - Fix incorrect comment about proper-lockfile requirements
…ck vs business errors
- Use rmdir instead of unlink for lock directory cleanup - Update error message assertion to match new format
8c72c32 to
ffd7c53
Compare
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
This PR implements proper cross-process file locking for the memory server using
proper-lockfileto ensure atomic read-modify-write operations across multiple MCP server instances.Key changes:
proper-lockfiledependency for cross-process file lockingcreateEntities,deleteEntities,createRelations,deleteRelations,addObservations,deleteObservations) inwithLock()to ensure atomic read-modify-writewrite-file-atomicfor more robust atomic file writesWhy PR #3060's in-memory lock solution is insufficient
PR #3060 attempts to fix Issue #2579's race condition using an in-memory Promise-based lock. While this works for concurrent operations within a single process, it fundamentally misses the real-world deployment model:
MCP servers using stdio transport are spawned as separate processes per client.
PR #3060 acknowledges this limitation:
This multi-client scenario is the default use case for many users, not an edge case.
Server Details
Motivation and Context
The memory server experiences file corruption when multiple MCP server instances (spawned by different AI clients) attempt concurrent writes to the same
memory.jsonfile.This is critical because:
How Has This Been Tested?
1. Single-process concurrent writes (10,000 operations)
knowledge-graph.test.tscreateEntitiescalls within a single process2. Multi-process concurrent writes (5 processes × 2,000 = 10,000 operations)
multi-process-lock.test.tsmcp-server-memoryprocesses via stdio transportAdditional testing:
Breaking Changes
No breaking changes:
Types of changes
Checklist
Additional context
Related Issues and PRs
mcp-server-memoryFailures due to Race Condition and Environment Misconfiguration #2579 - Race condition causing "Unexpected non-whitespace character after JSON" errorsWhy
proper-lockfile?proper-lockfile(this PR)proper-lockfileprovides the best balance of correctness and simplicity - true cross-process safety without external dependencies or storage migration.