Skip to content

Verify MNIST download integrity before using cached file - #72

Open
sharmis-tp wants to merge 2 commits into
CanReader:masterfrom
sharmis-tp:fix/mnist-partial-download-15
Open

sharmis-tp wants to merge 2 commits into
CanReader:masterfrom
sharmis-tp:fix/mnist-partial-download-15

Conversation

@sharmis-tp

Copy link
Copy Markdown

Summary

Closes #15. An interrupted download left a truncated file in the cache; download_missing saw target.exists() == true and skipped it forever, causing parse_images to fail with a size mismatch.

Fix: write to <name>.partial first, then fs::rename to 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 pass
    • truncated_file_rejected_by_parser: confirms the parser rejects short data
    • partial_suffix_not_mistaken_for_final: confirms .partial does not satisfy the cache check

@sharmis-tp
sharmis-tp requested a review from CanReader as a code owner November 7, 2026 14:08

@CanReader CanReader left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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.

  2. 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.

  3. 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.

  4. 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A partial MNIST download gets cached forever

2 participants