feat(storage): store identical objects once across repositories - #75
Conversation
There was a problem hiding this comment.
Solid design — hard links keep the on-disk format unchanged, isolation is preserved (nice test for it), and the retain accounting is careful about not over-claiming freed bytes. One accounting gap and two narrow races worth a look.
Blocking: LocalStore::usage()/usage()'s walk() (server/src/storage/sweep.rs) still sums every per-repository hard link at full size, and skips .content entirely (dot-prefixed dirs are filtered out). Before this PR that sum equalled real disk usage; now it's the pre-dedup total, since a shared object is counted once per repository that links it. That total feeds the /metrics store_bytes gauge (routes.rs::scrape) and the dashboard, so the exact capacity metric this feature is meant to move will show no improvement from dedup — and will keep inflating as more repos link the same content. Worth reworking to count unique inodes (dev+ino) once, or to sum .content plus any per-repo file that isn't hard-linked to it (the "no hard link support" copy-fallback path in link_or_move really does duplicate bytes, so a naive "just walk .content" undercounts that case).
Nit: sweep_directory's "free content only if not referenced elsewhere" check (sweep.rs:118-136) has a benign race under concurrent retain calls on two repos dropping the last two references to the same object around the same time: both can observe the other's link already gone and each add the content's size to its own report, double-reporting the same freed bytes across two separate SweepReports (no double-free, no leak — just an inflated total across two API responses). Low priority given retain is presumably not usually run concurrently across repos, but worth a comment if intentionally accepted.
Nit: link_or_move (server/src/storage/mod.rs) checks content exists, then later hard-links it into the new repo. If a concurrent retain on another repo drops the last other reference and deletes that same content file in between, the hard-link (and the copy fallback, since the source is now gone too) will fail and the push errors out — a spurious upload failure rather than data loss. Rare timing window, but a retry-on-NotFound (re-run the write-or-link from scratch) would close it.
|
All three land, and the blocking one was right about more than it said. The capacity gauge. Correct, and it undercut the whole feature: the number meant to show deduplication working would have grown with every project that linked the same pack. The review also called the trap I was walking into — "just walk So on unix it counts each inode once, which is exact in every case including the fallback copy, since two real copies are two inodes. Off unix, where The per-repository page still reports logical size, deliberately: "this project uses 3 GiB of assets" is the right answer on a repository page even when some of it is shared. The double-counted free. Fixed rather than accepted: the bytes are counted only when The spurious upload failure. Also fixed, by not throwing away what we already have: the staged file is kept until the link succeeds, so if a concurrent collection removes the content in between, the staged copy goes back into place and the link is retried. No re-transfer, no failed push. 11 suites green, e2e green. |
There was a problem hiding this comment.
Follow-up to my previous review (commit 60c3b09). All three findings from that pass hold up as fixed in 29a58f3:
- Fixed (was Blocking):
measure()now walks viawalk_unique, deduping by(dev, ino)on unix, solfsx_store_bytes/lfsx_objects_storedreport the disk once instead of the pre-dedup sum across repositories. Confirmed by the newthe_capacity_gauge_reports_the_disk_not_the_sum_of_projectstest. - Fixed (was Nit, double-reporting race):
sweep_directorynow usesfs::remove_file(&content).is_ok()itself as the race gate — only the caller that actually deletes the shared content counts its bytes, closing the concurrent-retaindouble count. - Fixed (was Nit, spurious push failure):
link_or_movenow retries onNotFoundby re-staging the content and re-linking, closing the race where a concurrentretaincollects the content between the existence check and the hard link.
Nit (new, not blocking): the shared scan() still skips any dot-prefixed entry, so .content itself is never walked by walk_unique. That's harmless for the normal hard-link case (the object's inode is still reached through a repo-side link), but on the copy fallback (no hard-link support, or the root spans a device boundary) the .content original has no repo-side twin sharing its inode — it becomes a distinct file invisible to measure(). The comment above walk_unique says the fallback "really does duplicate them and really should be counted twice," but as written it's counted zero times for the original and once for the copy, i.e. undercounted. Narrow case (needs a filesystem without hard-link support, or a multi-filesystem storage root) — worth a follow-up if that configuration is expected to occur.
No other issues in the diff. Approving.
Closes #9.
The same asset pushed to two repositories was stored twice. For a studio where several projects share a Synty or Quixel pack, that is most of the disk.
The bytes now live once under
.content/, and each repository holds a hard link to them. The filesystem keeps the reference count, which is the part that makes this cheap rather than clever.Why hard links rather than a reference table
The obvious design — one content store plus per-repository reference sets — changes the on-disk format, and this repository already runs somewhere holding 3.4 GB of real assets. That means a migration, a flag day, and a window where a half-migrated store serves nothing.
Hard links get the same result with no format change: objects written before this are ordinary files with a single link and keep working untouched, new writes share, and nothing needs migrating. The reference count is maintained by the kernel rather than by code I would have had to keep correct.
Where hard links are unavailable — a filesystem without them, or a device boundary — the write falls back to a full copy. The disk pays, the client never notices.
Isolation was the tension in the issue, and it goes away
Sharing the bytes does not share them over the API. Every route resolves through the repository's own path, so a repository cannot read, list, or learn the existence of an object it never pushed — not even by guessing a digest, because the guess is checked against its tree. The issue's "guessing SHA-256 is not a practical attack" reasoning turned out not to be needed: there is nothing to guess at.
There is a test for exactly that, since it is the property most likely to be broken by a later change.
Retention had to change or it would lie
Dropping a repository's link frees nothing while another repository holds one. So
retainnow removes the shared content only when the reference it dropped was the last, and counts bytes as freed only then. Without that the report would have claimed gigabytes while freeing nothing — worse than not reporting at all, because an operator would act on it.Two tests pin both halves: collecting one project reports zero bytes and leaves the other readable, and collecting the last one reports the real size and empties
.content.Note for backups
rsync -Hortar, not a plain copy — otherwise every shared object expands back into a separate file on the far side. Added to the README.