BinaryCacheStore: Drop draining of input source

[edolstra]

Motivation

In 602a1f8 we decided it's now the responsibility of the caller to drain the source (using EnsureRead) if that's important. Usually it isn't, so we don't want to read a NAR we don't need.

Small cleanup taken from NixOS#15957.

Context

Summary by CodeRabbit

• Bug Fixes
  • Optimized binary cache store operations to skip unnecessary validation steps when paths are already verified and repair is not required, improving performance and resource management.

[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: e10be8ec-38cc-453f-9f4f-3ca28fe22862

📥 Commits

Reviewing files that changed from the base of the PR and between ff145f2 and 9634784.

📒 Files selected for processing (1)

• src/libstore/binary-cache-store.cc

---

📝 Walkthrough

Walkthrough

Two related functions in BinaryCacheStore are optimized to avoid unnecessary source draining when store paths are already valid and repair is not requested. addToStore returns immediately instead of draining before returning; addMultipleToStore's UploadNar node skips the upload entirely, allowing the source to be cleaned up by scope exit.

Changes

Source drain optimization on valid paths

Layer / File(s)	Summary
Early return and conditional optimization for valid, non-repair paths src/libstore/binary-cache-store.cc	addToStore returns immediately on the fast-path without draining narSource; addMultipleToStore's UploadNar inverts the upload conditional to skip uploading when the path is valid and repair is not requested, letting scope exit clean up the source naturally.

🎯 2 (Simple) | ⏱️ ~8 minutes

🐰 Two functions learn to skip the drain,
When paths are valid and repair's in vain,
Now source cleanup happens all on its own,
No more unnecessary chores to be sown! 🌟

🚥 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 captures the main change: removing automatic draining of input source in BinaryCacheStore, which aligns with the code modifications and PR objectives.
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 docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch no-drain

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Infer (1.2.0)

src/libstore/binary-cache-store.cc

src/libstore/binary-cache-store.cc:1:10: fatal error: 'nix/util/archive.hh' file not found
1 | #include "nix/util/archive.hh"
| ^~~~~~~~~~~~~~~~~~~~~
1 error generated.
Error: the following clang command did not run successfully:
/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/bin/clang-18
@/tmp/coderabbit-infer/9634784c7bfffdae9b505f20e8811981d37cbbf7-5fe694ec6369f3e6/tmp/clang_command_.tmp.7e03a6.txt
++Contents of '/tmp/coderabbit-infer/9634784c7bfffdae9b505f20e8811981d37cbbf7-5fe694ec6369f3e6/tmp/clang_command_.tmp.7e03a6.txt':
"-cc1" "-load"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../../facebook-clang-plugins/libtooling/build/FacebookClangPlugin.dylib"
"-add-plugin" "BiniouASTExporter" "-plugin-arg-BiniouASTExporter" "-"
"-plugin-arg-BiniouASTExporter" "PREPEND_CURRENT_DIR=1"
"-plugin-arg-BiniouASTExporter" "MAX_STRING_SIZE=65535" "-cc1" "-triple"
"x86_64-unknown-linux-gnu" "-emit-obj" "-

... [truncated 1108 characters] ...

/lib/clang/18/include"
"-internal-isystem" "/usr/local/include" "-internal-isystem"
"/usr/lib/gcc/x86_64-linux-gnu/12/../../../../x86_64-linux-gnu/include"
"-internal-externc-isystem" "/usr/include/x86_64-linux-gnu"
"-internal-externc-isystem" "/include" "-internal-externc-isystem"
"/usr/include" "-Wno-ignored-optimization-argument" "-Wno-everything"
"-fdeprecated-macro" "-ferror-limit" "19" "-fgnuc-version=4.2.1"
"-fskip-odr-check-in-gmf" "-fcxx-exceptions" "-fexceptions"
"-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o"
"/tmp/coderabbit-infer/5fe694ec6369f3e6/file.o" "-x" "c++"
"src/libstore/binary-cache-store.cc" "-O0" "-fno-builtin" "-include"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../lib/clang_wrappers/global_defines.h"
"-Wno-everything"

---

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

[github-actions]

🚀 Deployed on https://6a1ea09677eea814ba71ca7c--determinate-nix.netlify.app