Repository navigation
Verify MNIST download integrity before using cached file - #72
sharmis-tp wants to merge 2 commits into
Conversation
CanReader
left a comment
There was a problem hiding this comment.
Thanks for your contribution, I appreciate that. There are some small improvement. Requesting changes for a few things; the first one is the one that matters.
-
A file that is already truncated is still trusted. The PR makes new writes safe, but anyone who hit #15 already has a short file in the cache, and for them nothing changes: target.exists() is true, the download is skipped, and parse_images fails on every run. The issue asks for this case too. Simplest fix: in load_from, if parse_images or parse_labels fails on a cached file, remove that file and download it again once. If it fails again, return the error with the path in it.
-
The tests don’t run the new code. Both pass with the old File::create(&target) line put back: the first checks that nine bytes are not an IDX file, the second that writing x.partial does not create x. To test the rename, the write has to be separable from the network. A small fn write_atomic(target: &Path, bytes: &[u8]) -> Result<()> that creates the partial, writes, syncs, renames, and removes the partial on error can be tested directly: success leaves target and no .partial, and a junk .partial already in the directory does not change the result.
-
sync_all() before the rename. Without it a power loss can keep the rename and lose the data, and because the target did not exist before, ext4 does not flush for you. That leaves a short file the cache check trusts, the same stuck state as before.
-
A per-process temp name. Two processes loading MNIST at the same time write the same .partial, and one renames the other’s half-written bytes. format!("{name}.{}.partial", std::process::id()) is the pattern tests/robustness.rs uses.
I can merge your PR if you fix those. Thank you again
Summary
Closes #15. An interrupted download left a truncated file in the cache;
download_missingsawtarget.exists() == trueand skipped it forever, causingparse_imagesto fail with a size mismatch.Fix: write to
<name>.partialfirst, thenfs::renameto the final path. The rename is atomic on POSIX so only a fully decompressed file is ever visible as the cached copy.Test plan
cargo test --lib data::mnist::tests— 2 new tests passtruncated_file_rejected_by_parser: confirms the parser rejects short datapartial_suffix_not_mistaken_for_final: confirms.partialdoes not satisfy the cache check