Track written-file identity (device + inode) for reuse - #131
Merged
Conversation
Collaborator
Author
Independent code review trailRan the independent Opus reviewer in a loop. Each round surfaced real issues, so it went to four rounds; all substantive findings are resolved. Round 1
Round 2
Round 3
Round 4
Skipped / not actioned
Verification after the final round: |
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.
What & why
Closes #124.
writeMarkdownreuses a record's already-written file across autoSync passes (so a record whose server-side settle failed and is re-fetched as still-pending doesn't drop a fresh<slug>-2.md,<slug>-3.mdduplicate every pass). Reuse eligibility previously checked only existence + type of the tracked path. That left one data-loss edge underautoDelete: if a record's own file was deleted and a different regular file was dropped at the same path between passes, reuse settled — and underautoDelete, deleted server-side — the record against a file that is not its content. The record's content was then gone from the server and never persisted locally.This tracks each written file's identity (
deviceId+inode) alongside the path and content hash. On a later pass, reuse is refused when the file at the tracked path is no longer the one we wrote, so the record is written fresh (suffix) / clobbers underoverwrite/ stays safely pending underskipinstead of being settled against an unrelated file.Key decisions
deviceId+inode, stored asbigint. This is the OS's own file identity (what hardlink detection andfind -samefileuse).deviceIddisambiguates inode numbers that are only unique within one filesystem (a vault on an external/network mount).bigint(via lstat'sbigint: true) preserves 64-bit inode/device ids (Btrfs, Windows file indexes, some network FSes) that would otherwise lose their low bits rounding through a JS double and false-match a different file.statx(containers), and ctime also moves on edits — so matching on it would reintroduce the per-pass duplicate bug on those hosts.inodeis stable across in-place edits on every platform.Behaviour by strategy when a foreign file is found at the tracked path
suffix: reuse refused → record written to a fresh suffixed file (its content persisted), then re-tracked and reused on later passes.overwrite: reuse refused, but the fresh overwrite still lands on the base path and replaces the foreign file with the record's own content (the strategy the user opted into) — no data loss.skip: reuse refused andskipwon't clobber, so the write is a no-op and the record is left safely pending on the server (skip's normal occupied-slot contract). It recovers once the foreign file is gone.Tests
Unit tests in
tests/libs/markdown.test.tscover: a different file (new inode) dropped at the path is refused and the record recovers onto a fresh file; a matching inode on a different device is refused; an in-place vault edit (same device+inode, moved mtime) is still reused with the edit preserved; identity unchanged reuses without a duplicate; the on-disk identity is stored; a transient post-write stat failure tracks without identity, reuses via the fallback, and re-arms the guard; andskip/overwritebehaviour under a foreign file. The reuse mock now models per-path device+inode identity.Viewable
CLI behaviour (no UI): exercised via the
writeMarkdownreuse path used bysync/get.Verification
npm run lint,npm run typecheck,npm run build(tsc + tsc-alias), andnpm run test:ci(707 tests) all pass.Follow-up suggestions
Capture written-file identity from the created fd— Replace the write path'swriteFileSync+ stat-after-write withopenSync(path, 'wx')+fstatSync(fd)+writeSync(fd)so the identity comes from the exact inode created, closing the narrow stat-after-write replacement race this PR only narrows. (suggested: P4, effort: M, evidence: src/libs/markdown.ts writers + readRegularFileIdentity)