Repository navigation
fix(import): skip existing content-addressed .dir objects (0.19.1) - #176
Merged
Merged
Conversation
Closes #175. dt import failed with EACCES writing a .dir into a shared registry when another member had written that object first. Shared registries are setgid and sticky, so one member cannot overwrite another's file. The symptom read as intermittent: sections you had imported yourself worked, sections a colleague imported first failed. Cache objects are content-addressed -- the filename is the md5 of the content -- so an object that already exists is by definition the one being written. There is never a reason to rewrite it. The bug was one function. The other four .dir writers in this codebase (import_data.py:235, :366, :430 and update.py:367) already guard with `if not dest_file.exists()`; create_dir_file alone wrote unconditionally. Two independent halves, which is deliberate: - Skip the write when the object exists. - Treat a failed write as success when the object exists afterwards, for the race where concurrent --csv imports populate one registry and another writer lands between the check and the write. Either alone fixes the reported failure. I discovered that by accident: a first mutation test removed only the pre-check and the reproduction still passed, because the post-write recovery covers the permission case on its own. Both halves reverted, it fails as expected. Beyond the issue: when skipping, the existing object's size is compared against what would have been written and a mismatch warns. A truncated .dir keeps its filename, so it still looks like the right object while decoding to a short file list, surfacing much later as a fetch that silently misses files. Two syscalls, and it warns rather than fails so it cannot break a working import. This diverges from the other four sites, which skip silently. 8 tests, 4 of which fail against the unfixed original, including one that reproduces #175 with real chmod 0444 on the object and 0555 on its directory. I confirmed separately that a 0444 file does raise PermissionError for its owner on this filesystem rather than assuming it. Not done here: retiring the import_data.sh dvc import fallback the issue mentions -- that lives in swarbricklab/images. Co-Authored-By: Claude <noreply@anthropic.com>
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.
Closes #175.
The bug was one function
Your diagnosis was exactly right, and the codebase already had the pattern to copy — the other four
.dirwriters guard correctly and onlycreate_dir_filedidn't:import_data.py:235,:366,:430if not dest_file.exists(): … write✅update.py:367import_data.py:594Cache objects are content-addressed, so an object that already exists is the one being written. It now skips.
Two halves, independently sufficient
--csvimports populate one registry and another writer lands between the check and the write.I found out these are independent by accident, and it's worth recording. My first mutation test removed only the pre-check, and the reproduction test still passed — because the
except OSErrorrecovery handles the permission case on its own. Reverting both halves, it fails properly. So the belt and the braces each do the job; keeping both covers the concurrent case the--csvbulk form makes likely.One addition beyond the issue
When skipping, the existing object's size is compared against what would have been written, and a mismatch warns.
A truncated
.dirkeeps its filename, so it still looks like the correct object while decoding to a short file list — surfacing much later as a fetch that silently misses files. Two syscalls, and it warns rather than fails, so it cannot break an import that currently works.This deliberately diverges from the other four sites, which skip silently. Happy to drop it if you'd rather they stayed consistent.
Verification
8 new tests. 4 fail against the unfixed original, including
test_read_only_object_no_longer_fails, which reproduces #175 with realchmod 0444on the object and0555on its directory rather than a mockedPermissionError.I also confirmed independently that a
0444file genuinely raisesPermissionErrorfor its owner on this filesystem, rather than assuming owner-bypass semantics — that assumption is the sort that quietly makes a reproduction test vacuous.Full suite: 1997 passed, 1 skipped.
Also
docs/import.mdgains a troubleshooting entry. "Worked for my sections, failed for my colleague's" reads as intermittent rather than as a permissions problem, and that's worth naming.import_data.shdvc importfallback the issue mentions — that lives inswarbricklab/images, not this repo. Worth a follow-up there once this ships.🤖 Generated with Claude Code