LocalStore::doAddToStore(): Check NAR size before NAR hash

[proski]

If the file size is wrong, the hash is almost certainly wrong. Checking the size first provides useful information that is not shown otherwise.

Size mismatch indicates issues like truncated files, whereas hash mismatch for the matching file size suggests data corruption. Reporting the correct condition helps troubleshoot the issue.

Summary by CodeRabbit

• Bug Fixes

  • Improved validation order for binary cache integrity checks to detect size mismatches before hash mismatches.

• Tests

  • Added regression test for NAR size mismatch detection in binary cache.
  • Enhanced hash mismatch regression test with improved corruption method.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a65f43ee-541f-4255-b5b6-8f547519cde3

📥 Commits

Reviewing files that changed from the base of the PR and between 21ed367 and da3c761.

📒 Files selected for processing (2)

• src/libstore/local-store.cc
• tests/functional/binary-cache.sh

---

📝 Walkthrough

Walkthrough

In LocalStore::doAddToStore, the post-restoration integrity checks are reordered so that narSize is validated before narHash. A new functional regression test asserts size mismatch detection, and the existing hash mismatch test is reworked to corrupt the NAR stream directly rather than regenerate from an empty dump.

Changes

NAR integrity check ordering and test coverage

Layer / File(s)	Summary
Reorder size/hash checks in LocalStore::doAddToStore src/libstore/local-store.cc	Moves the narSize mismatch check before the narHash mismatch check in the post-restore validation block. Error messages and formatting are unchanged; only the evaluation order differs.
Size mismatch regression test and hash mismatch test rework tests/functional/binary-cache.sh	Adds a new test that swaps in a wrong-size NAR and asserts "size mismatch" appears in the nix-build log. Updates the existing hash mismatch test to corrupt the cached NAR via decompressed-stream editing (foo → Foo) rather than replacing it with an empty dump.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A bunny checked sizes before checking hash,
No more wrong-hash blame when the bytes are a crash.
The stream got corrupted with Foo instead of foo,
And size mismatch now jumps first into view. 🐇
Order matters in the warren, and tests prove it true!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main change: reordering validation checks in LocalStore::doAddToStore to verify NAR size before NAR hash.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[edolstra]

Looks good, thanks! Unfortunately our CI is currently broken for external PRs, so I've recreated this as #512.