Serialize memory writes and gate vault-mcp on sync health - #134
Merged
Conversation
The section-scoped read modes added in #130 (outline, heading, heading_level, alongside the existing properties_only) were never reflected in the prose/table docs, so readers couldn't discover that large notes can be read cheaply by outline or single section. - README Tools table: expand the vault_read_note row to surface the full-body / properties / outline / section modes - ARCHITECTURE.md MCP Tools table: list the optional params in the Input column (matching how vault_patch_note is shown) and add an explanatory sentence after the table Docs-only; tool count and behavior unchanged. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> https://claude.ai/code/session_01R4TfpeScf76Mhn2ZAdfanf
Two follow-ups to the memory-write-safety hardening: - Add a per-file in-memory mutex (a per-path promise chain) around updateMemory/deleteMemory so concurrent read-modify-write cycles on the same memory file can't interleave and drop an entry (lost update). vault-mcp is a single Node process, so an in-process lock is sufficient and avoids syncing .lock artifacts into the vault. - Switch the vault-mcp -> obsidian-sync depends_on from condition: service_started to condition: service_healthy in all three remote compose files, so the memory bootstrap waits for sync's healthcheck (and its start_period) instead of racing the first sync on a fresh volume. Document the ordering and its caveat in ARCHITECTURE.md. Adds concurrency tests covering same-section appends, parallel writes to different files, and a racing update+delete.
📝 WalkthroughWalkthroughIntroduces per-file promise-chain locking ( ChangesMemory store write serialization
Docker Compose startup ordering and docs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…mmitted artifacts" This reverts commit 4290a03.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vault-mcp/vault-operations/__tests__/memory-store.test.ts`:
- Around line 998-1000: The test name "writes concurrent updates to different
files in parallel" claims to verify parallel execution behavior, but the actual
assertions do not prove that different-file writes execute concurrently or avoid
serialization—the test would pass even with a global lock. Either add
deterministic instrumentation (such as timing measurements or synchronization
points) that proves different-file writes overlap in time while same-file writes
serialize, or rename the test to accurately describe what it currently asserts,
such as "persists concurrent updates to different files without interference."
Ensure the test name matches the actual behavior being verified per coding
guidelines.
- Around line 987-995: Replace the loose substring matching approach in the test
that uses expect(section).toContain() calls combined with a bulletCount check
with exact assertions that verify the full expected array of bullet-line values.
Instead of checking for substring presence and counting bullets separately,
assert the complete array of formatted bullet lines that should exist in the
section, which will catch duplicates, malformed lines, or unexpected extra
bullets while still proving no lost updates.
In `@src/vault-mcp/vault-operations/memory-store.ts`:
- Around line 52-55: The fileWriteLocks map retains entries indefinitely after
write operations complete, causing memory to accumulate in long-running servers
that create many different file paths. After setting thisWrite in the
fileWriteLocks map, attach cleanup logic to the promise using .finally() (or
similar) to delete the map entry when the promise settles, but only if thisWrite
is still the current tail of fileWriteLocks for that filePath. This ensures
stale promises are removed while maintaining correct synchronization for
concurrent writes to the same file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eb69ac84-c80e-449a-b8f3-d206229136e2
📒 Files selected for processing (7)
ARCHITECTURE.mdREADME.mdcli/templates/remote/docker-compose.ymldeploy/remote/docker-compose.ymldocker-compose.ymlsrc/vault-mcp/vault-operations/__tests__/memory-store.test.tssrc/vault-mcp/vault-operations/memory-store.ts
Comment on lines
+987
to
+995
| // All five new entries plus the one original entry survive. | ||
| for (const entry of entries) { | ||
| expect(section).toContain(`- **2026-06-14**: ${entry}`) | ||
| } | ||
| expect(section).toContain("Single-purpose files") | ||
| const bulletCount = section | ||
| .split("\n") | ||
| .filter((line) => line.startsWith("- **")).length | ||
| expect(bulletCount).toBe(entries.length + 1) |
There was a problem hiding this comment.
Use exact assertions for the final section contents.
The expected bullet lines are known here, so assert the full bullet-line array instead of substring presence plus a count. That makes these tests fail on duplicates, malformed lines, or unexpected extra bullets while still proving no lost updates. As per coding guidelines, “Assert the full value, not just substrings or loose matchers” and “Use exact assertions.”
Also applies to: 1040-1041, 1079-1085
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vault-mcp/vault-operations/__tests__/memory-store.test.ts` around lines
987 - 995, Replace the loose substring matching approach in the test that uses
expect(section).toContain() calls combined with a bulletCount check with exact
assertions that verify the full expected array of bullet-line values. Instead of
checking for substring presence and counting bullets separately, assert the
complete array of formatted bullet lines that should exist in the section, which
will catch duplicates, malformed lines, or unexpected extra bullets while still
proving no lost updates.
Source: Coding guidelines
Comment on lines
+998
to
+1000
| it("writes concurrent updates to different files in parallel", async () => { | ||
| // Different files key on different lock paths, so they must not serialize | ||
| // into each other or deadlock — both writes complete and persist. |
There was a problem hiding this comment.
Make the test name match what the assertions prove.
This test would still pass if withFileLock used one global lock, so it does not verify “in parallel” or “must not serialize.” Either add deterministic instrumentation that proves same-file writes are serialized while different-file writes overlap, or rename this to the behavior currently asserted, e.g. “persists concurrent updates to different files without interference.” As per coding guidelines, “Every test must actually verify the behavior it claims to test” and “Test names match what they assert.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vault-mcp/vault-operations/__tests__/memory-store.test.ts` around lines
998 - 1000, The test name "writes concurrent updates to different files in
parallel" claims to verify parallel execution behavior, but the actual
assertions do not prove that different-file writes execute concurrently or avoid
serialization—the test would pass even with a global lock. Either add
deterministic instrumentation (such as timing measurements or synchronization
points) that proves different-file writes overlap in time while same-file writes
serialize, or rename the test to accurately describe what it currently asserts,
such as "persists concurrent updates to different files without interference."
Ensure the test name matches the actual behavior being verified per coding
guidelines.
Source: Coding guidelines
Comment on lines
+52
to
+55
| const previousWrite = fileWriteLocks.get(filePath) ?? Promise.resolve() | ||
| const thisWrite = previousWrite.then(operation, operation) | ||
| fileWriteLocks.set(filePath, thisWrite) | ||
| return thisWrite |
There was a problem hiding this comment.
Clean up settled file-lock tails.
fileWriteLocks retains every filePath forever after the write settles. Since updateMemory can auto-create arbitrary memory files, a long-running server can accumulate stale promises for one-off paths; delete the map entry when the settled promise is still the current tail.
Suggested cleanup
const withFileLock = <T>(
filePath: string,
operation: () => Promise<T>,
): Promise<T> => {
const previousWrite = fileWriteLocks.get(filePath) ?? Promise.resolve()
const thisWrite = previousWrite.then(operation, operation)
fileWriteLocks.set(filePath, thisWrite)
+ const cleanup = (): void => {
+ if (fileWriteLocks.get(filePath) === thisWrite) {
+ fileWriteLocks.delete(filePath)
+ }
+ }
+ void thisWrite.then(cleanup, cleanup)
return thisWrite
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const previousWrite = fileWriteLocks.get(filePath) ?? Promise.resolve() | |
| const thisWrite = previousWrite.then(operation, operation) | |
| fileWriteLocks.set(filePath, thisWrite) | |
| return thisWrite | |
| const withFileLock = <T>( | |
| filePath: string, | |
| operation: () => Promise<T>, | |
| ): Promise<T> => { | |
| const previousWrite = fileWriteLocks.get(filePath) ?? Promise.resolve() | |
| const thisWrite = previousWrite.then(operation, operation) | |
| fileWriteLocks.set(filePath, thisWrite) | |
| const cleanup = (): void => { | |
| if (fileWriteLocks.get(filePath) === thisWrite) { | |
| fileWriteLocks.delete(filePath) | |
| } | |
| } | |
| void thisWrite.then(cleanup, cleanup) | |
| return thisWrite | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vault-mcp/vault-operations/memory-store.ts` around lines 52 - 55, The
fileWriteLocks map retains entries indefinitely after write operations complete,
causing memory to accumulate in long-running servers that create many different
file paths. After setting thisWrite in the fileWriteLocks map, attach cleanup
logic to the promise using .finally() (or similar) to delete the map entry when
the promise settles, but only if thisWrite is still the current tail of
fileWriteLocks for that filePath. This ensures stale promises are removed while
maintaining correct synchronization for concurrent writes to the same file.
…p-setup-4v23q9 # Conflicts: # src/vault-mcp/vault-operations/memory-store.ts
This was referenced Jul 2, 2026
13 tasks
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
2 participants
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.
Summary
Two independent follow-ups to the memory-write-safety hardening (
atomicWriteFile+ the shrink guard). Both make theAbout Me/memory layer more robust: one closes an in-process race that could silently drop entries, the other stops the server from bootstrapping memory templates before the vault has synced on a fresh deploy.1. Per-file mutex for memory writes
The problem
updateMemoryanddeleteMemoryperform a read-modify-write: read the file, parse it, splice in (or remove) one dated bullet, write it back. vault-mcp is a single Node process serving concurrent MCP request handlers on one event loop, and eachawaitis a yield point. So two calls touching the same file can interleave:The file ends with
[x, b]and entryais silently lost (a classic lost update). It's not parallelism — it's interleaving — but the effect is data loss.The fix
A small in-memory per-file mutex that serializes the read-modify-write cycle. It's built as a promise chain, not a held boolean:
Map<string, Promise>stores, per file path, the tail of that file's operation queue ("the last op queued for this key").await, so two calls racing in the same tick still serialize correctly..thenhandlers (fulfilled and rejected) run the same work, so a prior failure can't skip a turn or leave a rejected promise poisoning the rest of the queue. Each op's own error still surfaces to its own caller.The mechanism and its subtleties are documented in a walkthrough comment on
withFileLockinsrc/vault-mcp/vault-operations/memory-store.ts.Why not a lockfile (e.g.
proper-lockfile)?A cross-process file lock would only coordinate between processes that both acquire it. The only other writer to the vault is the
obsync binary, which honors no advisory lock, and running multiple vault-mcp instances isn't part of the architecture. So a lockfile would add a dependency and sync.lockartifacts into the vault for protection it can't actually deliver. The in-process mutex covers the real, always-present case (concurrent handlers in one process) with no new dependency and no on-disk artifacts.2.
condition: service_healthystartup gateThe problem
The
vault-mcp → obsidian-syncdependency usedcondition: service_started, which only waits for obsidian-sync's container to launch. On a fresh volume, vault-mcp could then run its memory bootstrap (which writes theAbout Me/skeleton templates) before the real files have been pulled down by sync — risking the skeleton overwriting real content.The fix
Switch the dependency to
condition: service_healthyin all three remote compose files (docker-compose.yml,deploy/remote/docker-compose.yml,cli/templates/remote/docker-compose.yml). obsidian-sync already defines a healthcheck, so nothing new is added — vault-mcp now waits until that healthcheck passes (which includes itsstart_periodgrace window) before starting.Honest caveat (also documented in ARCHITECTURE.md)
The healthcheck verifies the
ob syncprocess is running and/vaultexists — it does not prove the initial sync has completed (there's noobcapability for that). The practical win is thestart_period: it gives sync runway to land files before the bootstrap runs. Combined with the shrink guard, that's enough to prevent the fresh-volume overwrite. Local-only compose files have no obsidian-sync service and are unchanged.Tests
src/vault-mcp/vault-operations/__tests__/memory-store.test.tsgains aconcurrent memory writessuite:Verified the new tests fail without the mutex (the lost-update symptom — fewer entries than fired) and pass with it; the different-files test passes either way, as expected. Full suite green: 662 passing. Lint, build, and prettier clean.
Note
This branch also includes an earlier docs-only commit (
vault_read_notesection-scoped read modes), unrelated to the hardening above and with no behavior change.Summary by CodeRabbit
Bug Fixes
Documentation