fix(storage): stop collection removing a fanout directory out from under an upload - #82
Conversation
There was a problem hiding this comment.
Fix is sound: removing directory cleanup from both levels of sweep()/sweep_directory() fully closes the race, since write()/link_or_move() already use create_dir_all (idempotent) rather than assuming the directory doesn't exist. No other code path in this store relies on sweep pruning empty directories, and .content never pruned its own either, so this doesn't introduce a new asymmetry — consistent with the PR description.
Test (an_upload_survives_a_collection_emptying_its_fanout) plausibly reproduces the race (concurrent sweep loop + 100 uploads, LFSX_GC_GRACE=0) and the author confirmed it fails on main with only the sweep.rs change reverted. README update accurately documents the tradeoff (directories are never reclaimed).
Nothing blocking. Nit: the assert message in the new test still describes the old buggy behavior ("collection removes a fanout directory... an upload that arrives in that window did nothing wrong") — reads a bit oddly as an invariant description now that it can't happen, but it's harmless as a failure explanation.
SonarQube — 1 issue(s) introduite(s) par cette PR
1 issue(s) corrigée(s) sur les fichiers touchés. Comparaison entre le projet bac à sable de cette PR et la branche par défaut : SonarQube Community n'analyse pas les PR, ce delta est calculé côté CI. Détail |
Closes #31.
A push creates its fanout directory, and until the staging file lands that directory is empty — so a collection running alongside removed it and the push died on
File::createwith a 500, on a directory made moments earlier for that very push.I wrote the test first and it failed on the first iteration, well before the two hundredth upload, which puts this closer to "a busy server hits it" than the issue's "eventually".
Which side to fix
The issue offered two: retry the create, or keep young directories out of the sweep's reach. I tried the retry first and the test still failed — now inside
link_or_move, because the same window reopens between promoting the staged file into.contentand hard-linking it back into the repository. Recreating the directory and trying again just moved the failure, since nothing here can hold a lock the filesystem would honour and the sweep is free to take the directory again a microsecond later. A retry does not close this race, it narrows it — and a bounded retry loop would have been a test that passes because the loop is longer than the adversary is patient.The mtime variant does not survive contact either: the sweep has just deleted files from the directory, so its mtime is now, and
LFSX_GC_GRACE=0is a supported setting the tests already use.So the fix is to stop competing: collection no longer removes directories. The shared
.contenttree has never pruned its own, so this makes the two sides consistent rather than introducing a new asymmetry. What is left behind is an inode and a block per prefix, reused by the next object that hashes into it — set against a push failing for a reason no operator could act on or diagnose.The diff is eight lines removed.
Test
an_upload_survives_a_collection_emptying_its_fanoutruns a hundred uploads against a collection loop on a four-thread runtime. It fails onmainand passes here, and it is deterministic in both directions: nothing removes directories any more, so there is no window left to lose. Verified by stashing only thesweep.rschange and watching it go red again.Documented in the README under Reclaiming space, since an operator who empties a repository will see the directories stay.