feat(memory): harden writes against partial-write and clobber data loss - #128
Conversation
In-app defense-in-depth for the memory-layer data-loss class (the June 13 About Me clobber), so the memory layer is safe even without Obsidian Sync version history. - Atomic writes: new atomicWriteFile (stage to temp + rename) replaces every note/memory writeFile in vault-filesystem, vault-patcher, and memory-store. The target is never truncated, so a concurrent reader (obsidian-sync) sees old or new content, never a 0-byte/partial write. - Large-shrink guard on memory writes: refuse an update/delete that would drop an existing file below 50% of its size (above a 200-byte floor that sits just past the empty templates), failing loud instead of silently truncating. - Size logging: before/after byte counts on every write for observability. - Graceful SIGTERM: drain in-flight requests via server.close() with a 10s force-exit fallback instead of a hard process.exit(0). - Surface the guard error in the vault_update_memory / vault_delete_memory tool descriptions (Errors: sections). 634 tests (15 new), build + lint clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 50 minutes and 43 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an ChangesAtomic writes, shrink guard, and byte-count logging
Graceful SIGTERM shutdown handler
Sequence Diagram(s)sequenceDiagram
participant Caller as Write caller
participant atomicWriteFile
participant guardAgainstShrink
participant fs as Node fs
Caller->>guardAgainstShrink: beforeBytes, afterBytes
alt afterBytes < 50% of beforeBytes AND beforeBytes > SHRINK_FLOOR_BYTES
guardAgainstShrink-->>Caller: throw "refusing memory write"
else safe
guardAgainstShrink-->>Caller: ok
Caller->>atomicWriteFile: filePath, content
atomicWriteFile->>fs: writeFile(tmpPath)
atomicWriteFile->>fs: rename(tmpPath, filePath)
atomicWriteFile-->>Caller: void
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/vault-mcp/server.ts (1)
38-43: ⚡ Quick winAdd an explicit outer return type to the exported shutdown factory.
createShutdownHandleris exported, but its outer return type is inferred. Please annotate it explicitly for API clarity and to align with repository rules.Suggested change
export const createShutdownHandler = ( httpServer: { close: (callback: () => void) => void }, forceExitMs = 10_000, - ) => + ): (() => void) => (): void => {As per coding guidelines, "Provide explicit return types on exported functions and modules."
🤖 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/server.ts` around lines 38 - 43, The exported function `createShutdownHandler` is a higher-order function (factory pattern) that lacks an explicit return type annotation. Add an explicit return type annotation to `createShutdownHandler` to declare what it returns. Since the function returns another function that takes no parameters and returns void, annotate the return type as `() => void` on the outer function signature to provide API clarity and meet the repository's coding guidelines requiring explicit return types on exported functions.Source: Coding guidelines
🤖 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/vault-filesystem.ts`:
- Around line 45-48: The `rm()` call on line 47 in the catch block can throw an
error that overwrites the original `writeFile` or `rename` error, defeating the
purpose of best-effort cleanup. Wrap the `await rm(tmpPath, { force: true })`
call in a nested try-catch block so that if the cleanup fails, the error is
silently caught and the original `err` is still thrown at the end of the catch
block, preserving the root cause of the write failure.
---
Nitpick comments:
In `@src/vault-mcp/server.ts`:
- Around line 38-43: The exported function `createShutdownHandler` is a
higher-order function (factory pattern) that lacks an explicit return type
annotation. Add an explicit return type annotation to `createShutdownHandler` to
declare what it returns. Since the function returns another function that takes
no parameters and returns void, annotate the return type as `() => void` on the
outer function signature to provide API clarity and meet the repository's coding
guidelines requiring explicit return types on exported functions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ed7f9c1-84c9-4f29-be4f-7d4fc005f06c
📒 Files selected for processing (9)
src/vault-mcp/__tests__/server.test.tssrc/vault-mcp/__tests__/tool-definitions.test.tssrc/vault-mcp/server.tssrc/vault-mcp/tool-definitions.tssrc/vault-mcp/vault-operations/__tests__/memory-store.test.tssrc/vault-mcp/vault-operations/__tests__/vault-filesystem.test.tssrc/vault-mcp/vault-operations/memory-store.tssrc/vault-mcp/vault-operations/vault-filesystem.tssrc/vault-mcp/vault-operations/vault-patcher.ts
…tdown factory Address review feedback on PR #128: - atomicWriteFile: wrap the best-effort temp rm() in its own try/catch so a cleanup failure can't overwrite the original write/rename error. - createShutdownHandler: add the explicit outer return type required for exported functions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Why
In-app defense-in-depth for the memory-layer data-loss class — partial/truncated writes and silent catastrophic shrinks — so the
About Me/memory layer is safe even for adopters without Obsidian Sync version history (which is the only recovery path today).What
Four safety fixes plus a tool-description update:
atomicWriteFilehelper (stage to a unique temp file, thenrenameover the target) replaces every note/memorywriteFileinvault-filesystem,vault-patcher, andmemory-store.renameis atomic on the same filesystem, so the target is never truncated — a concurrent reader (the obsidian-sync container) sees either the old or the new content, never a 0-byte or partial write. Bonus:*.tmpstaging files are ignored by the file watcher (.md-only), and the rename hands the watcher a guaranteed-complete file.updateMemory/deleteMemoryrefuse a write that would drop an existing file below 50% of its size, above a 200-byte floor (which sits just past the empty memory templates, so files with no real content aren't guarded). Fails loud instead of silently truncating.write_note/patch/replaceare intentionally left unguarded — their shrinks (full overwrite, empty-new_textdeletion) are legitimate.beforeBytes/afterByteson every write log line for observability.server.close()with a 10s force-exit fallback, instead of a hardprocess.exit(0)that could interrupt a write mid-flight.vault_update_memory/vault_delete_memorygainErrors:sections documenting the guard refusal and how to recover.Testing
npm test— 634 passing (15 new: atomic-write success/cleanup, shrink-guard throw/allow/floor, before/after size logging, SIGTERM drain + force-exit, tool-description errors). Guards mutation-checked (verified each test fails when the guard is neutered).npm run build:serverandnpm run lintclean.Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Chores