fix: make note writes atomic and serialized - #174
Conversation
write_note and patch_note read-modify-write a note and write it back with a plain writeFile. Two gaps remain after bitbonsai#98 (which fixed the r+ length bug): - Not crash-safe: writeFile truncates then writes, so a crash or a stalled network-backed mount (rclone/cloud) mid-write leaves a truncated file. - Not concurrency-safe: two overlapping write_note/patch_note calls to the same path interleave and lose each others changes. Add atomicWrite (write a temp sibling, then rename over the target - atomic on the same filesystem) and withPathLock (serialize mutations per absolute path). writeNote and patchNote run their read-modify-write under the lock; all in-place note writes (writeNote, patchNote, updateFrontmatter, manageTags) go through atomicWrite. Adds src/atomic-write.test.ts (concurrent-append integrity, temp-file cleanup).
bitbonsai
left a comment
There was a problem hiding this comment.
Hey @ErycM, thanks for splitting this out. It is much easier to review on its own. The concurrent append case works, and all 247 tests, the build, and the audit pass locally.
I found three blockers around preserving existing file behavior:
- Writing through an in-vault file symlink replaces the symlink itself instead of updating its target.
shortcut.mdstops being a symlink while the real note remains unchanged. Existing in-vault symlink support needs to keep working. update_frontmatterandmanage_tagsstill read before taking any lock, then write directly withatomicWrite. They can race withwrite_noteand lose either the metadata update or appended content. Please run every mutating read-modify-write path through the same per-target lock without nesting locks.- Replacing the inode resets file permissions. In my test a
0600note became0644. Please preserve the existing target mode when creating the temporary file.
Canonicalizing an existing target before choosing the lock key and temporary-file location should also make symlink aliases share the same lock. Please add regressions for the symlink, mixed-operation race, and permission cases.
Since this changes the published runtime as a bug fix, please bump the package to 0.12.6 and rebuild dist after the changes.
|
Quick update on the version note in my review: |
Split out of #162 as requested — the atomic/serialized write piece only, rebased on current
main(9748d7e). Independent of #173; no shared files.Why
write_noteandpatch_noteread a note, transform it, and write it back with a plainwriteFile. Two gaps remain after #98 (which fixed ther+length bug):writeFiletruncates and then writes. A crash — or a stalled network-backed mount — inside that window leaves the note truncated, and the truncated version is what the next read returns.write_note/patch_notecalls against the same path interleave: both read the same base content, and the second write drops whatever the first added.Context for the first one: I run mcpvault against a cloud-mounted vault (rclone FUSE) where I/O can stall for tens of seconds. To be precise about what I actually debugged there — the corruption I hit was in the mount's own read cache, not caused by mcpvault — but it is the same class of failure the write path is currently exposed to, and truncate-then-write is what turns a stalled mount into a lost note.
What
atomicWrite— write a temp sibling, thenrenameover the target (atomic on the same filesystem). A reader sees either the old file or the new one, never a partial one; a crash leaves the original intact.withPathLock— serialize mutations per absolute path via an in-process promise chain, closing the read-modify-write race.writeNoteandpatchNoterun their read-modify-write under the lock; all in-place note writes (writeNote,patchNote,updateFrontmatter,manageTags) go throughatomicWrite.updateFrontmatter/manageTagsintentionally useatomicWritewithout taking the lock, since they delegate towriteNote, which takes it — locking at both levels would deadlock the chain.Tests
src/atomic-write.test.ts:main, passes here);Full suite on this branch: 246 passed, 1 failed. The failure is
src/shutdown.test.ts > stdio server exits on SIGTERM, which also fails on pristinemainon this machine — a 500 ms boot race on a slow box, not introduced here. Same note as in #173.Commits
fix: make note writes atomic and serializedchore: rebuild dist— kept as its own commit, matching 5a293c6.No
website/content changed, and no README change either.