Preserve provenance while copying paths to the daemon - #399
Conversation
📝 WalkthroughWalkthroughAdds a negotiated feature flag for versioned multi-add, removes per-connection Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Daemon
participant Store
participant RemoteWorker as Worker
Client->>Daemon: AddMultipleToStore (count + items stream)
Daemon->>Daemon: read uint64 count
loop for each item
Daemon->>Daemon: deserialize ValidPathInfo (version gated)
Daemon->>Store: addToStore(ValidPathInfo, repair, checkSigs)
alt remote store path (worker protocol)
Store->>RemoteWorker: send AddToStore frames (use negotiated protoVersion if feature present else legacy 1.16)
RemoteWorker-->>Store: ack / result
else local store path
Store-->>Daemon: result
end
end
Daemon-->>Client: aggregate results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (18)
src/libexpr/include/nix/expr/counter.hh-14-14 (1)
14-14:⚠️ Potential issue | 🟠 MajorAdd
#include <new>forstd::hardware_destructive_interference_size.The code uses
std::hardware_destructive_interference_size(a C++17 feature from the<new>header) without explicitly including it. While this may compile with current toolchains due to transitive includes from<atomic>, the C++ standard does not guarantee this. Adding the explicit include ensures portability across different STL implementations and toolchains.Suggested fix
`#include` <atomic> `#include` <cstdint> +#include <new>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/include/nix/expr/counter.hh` at line 14, Add an explicit include of the header that defines std::hardware_destructive_interference_size by adding `#include` <new> at the top of the file so the struct declaration using alignas(std::hardware_destructive_interference_size) Counter is well-defined and portable across toolchains; update the includes in the file containing the Counter struct to include <new>.src/libstore/builtins/buildenv.cc-30-31 (1)
30-31:⚠️ Potential issue | 🟠 MajorUse
generic_string()for suffix checks that depend on forward-slash separators.Lines 51–52 convert
std::filesystem::pathtostd::stringusing.string(), which returns native path separators. On Windows, this produces backslash-separated paths. However, lines 67–69 then check these paths against hard-coded forward-slash suffixes ("/nix-support","/log","/manifest.nix", etc.). This causes the suffix checks to fail on Windows, allowing excluded directories to leak into the generated profile.Replace
.string()with.generic_string()on lines 51–52, or use path component comparisons (e.g.,path.filename()) for the suffix matching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/builtins/buildenv.cc` around lines 30 - 31, In createLinks(State & state, const std::filesystem::path & srcDir, const std::filesystem::path & dstDir, int priority) the code converts paths to std::string using .string() then performs suffix checks against hard-coded forward-slash literals (e.g., "/nix-support", "/log", "/manifest.nix"), which breaks on Windows; fix by using .generic_string() instead of .string() for the variables used in those suffix comparisons or, preferably, perform component-based comparisons (e.g., compare path.filename() or iterate path elements) so the exclusion checks (the code that tests for those suffixes) work correctly across platforms.src/libmain/unix/stack.cc-75-76 (1)
75-76:⚠️ Potential issue | 🟠 MajorAvoid
strlen()in the SIGSEGV handler path.Line 76 calls
strlen()beforewrite(). According to POSIX standards (POSIX.1-2001 through POSIX.1-2024),strlen()is not an async-signal-safe function and cannot be safely used in signal handlers. This can cause undefined behavior ifstrlen()reenters libc while the handler is executing in a critical context. Replace with a compile-time length:Proposed fix
void defaultStackOverflowHandler(siginfo_t * info, void * ctx) { - char msg[] = "error: stack overflow (possible infinite recursion)\n"; - [[gnu::unused]] auto res = ::write(2, msg, strlen(msg)); + static constexpr char msg[] = "error: stack overflow (possible infinite recursion)\n"; + [[gnu::unused]] auto res = ::write(2, msg, sizeof(msg) - 1); _exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libmain/unix/stack.cc` around lines 75 - 76, The signal-handler uses strlen() when calling write which is not async-signal-safe; change the write invocation to pass a compile-time constant length instead of calling strlen — e.g. use sizeof(msg)-1 (or the literal byte length) when calling ::write for the msg array referenced in this file (the char msg[] and the subsequent ::write(2, msg, ... ) call in src/libmain/unix/stack.cc) so the handler no longer invokes non-async-signal-safe functions.src/libfetchers/include/nix/fetchers/fetchers.hh-89-89 (1)
89-89:⚠️ Potential issue | 🟠 MajorConfirm: This is an ABI break for downstream implementations.
The headers in
src/libfetchers/include/nix/fetchers/are publicly exported (install_headersatsrc/libfetchers/meson.build:83), so changingInput::isRelative()and theInputScheme::isRelative(const Input &)virtual method affects any out-of-tree implementations.In-tree implementations are updated:
PathInputScheme::isRelative()insrc/libfetchers/path.cc:117correctly returnsstd::optional<std::filesystem::path>. Versioning is handled dynamically vianix_soversiontied to the project version. However, no explicit release notes documenting this breaking change were found in thedoc/manual/source/release-notes/directory. If this is part of an upcoming release, ensure the breaking change is explicitly documented in the version's release notes.The same concern applies to the companion change in
src/libfetchers/include/nix/fetchers/fetch-to-store.hh:38.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libfetchers/include/nix/fetchers/fetchers.hh` at line 89, You changed the public ABI by altering the return type of Input::isRelative() and the virtual InputScheme::isRelative(const Input&) (with in-tree override PathInputScheme::isRelative()), so either revert the signature change or explicitly document this breaking change in the release notes and bump the SONAME/versioning metadata; update the release-notes for the upcoming version to state that Input::isRelative() and InputScheme::isRelative(const Input&) now return std::optional<std::filesystem::path> (and list affected downstream implications), and ensure project soversion/packaging is adjusted to reflect the ABI break.src/libstore-tests/data/worker-substitution/single/substituter.json-13-26 (1)
13-26:⚠️ Potential issue | 🟠 MajorThis golden snapshot still can't catch provenance loss.
The path info only records CA/NAR metadata. A worker/daemon copy that strips provenance will still match this fixture, so the regression from
#369can pass unnoticed. Please add a source path with non-empty provenance here and assert it survives in the paired destination snapshot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore-tests/data/worker-substitution/single/substituter.json` around lines 13 - 26, The golden snapshot only records CA/NAR metadata (fields like "info", "ca", "narHash", "narSize", "storeDir", "version") so it won't catch provenance being stripped; add a source path entry in this JSON with a non-empty provenance object (e.g., a "source" or path entry under the same top-level object that includes a "provenance" field containing identifiable metadata such as origin, builder, or signed-by entries) and update the paired destination snapshot test to assert that this provenance survives the round-trip (i.e., ensure the destination snapshot contains the same provenance field/value for that path).src/libfetchers/path.cc-20-23 (1)
20-23:⚠️ Potential issue | 🟠 MajorUse
.generic_string()to preserve path serialization across platforms.Line 22 stores platform-native separators via
.string(), buttoURL()at line 98 reconstructs the URL path by splitting on/. On Windows, this breakspath:input round-tripping and invalidates cache identity.Fix
- input.attrs.insert_or_assign("path", urlPathToPath(url.path).string()); + input.attrs.insert_or_assign("path", urlPathToPath(url.path).generic_string());Additionally, the parametrized
InputFromURLTestinsrc/libflake-tests/flakeref.cclacks anypath:input test cases in its round-trip coverage, despite having round-trip assertions for other schemes. Add a test case to verifypath:round-tripping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libfetchers/path.cc` around lines 20 - 23, Replace the platform-native path serialization call using urlPathToPath(...).string() with urlPathToPath(...).generic_string() so Input.attrs stores a POSIX-style path and toURL() splitting on '/' will round-trip correctly (update the usage in the Input construction where Input input{} and input.attrs.insert_or_assign("path", ...) are set). Also add a path: case to the parametrized InputFromURLTest (the test named InputFromURLTest in flakeref.cc) that constructs a URL with scheme "path" and asserts it round-trips through Input->toURL() to catch regressions.src/libexpr/include/nix/expr/value.hh-1277-1301 (1)
1277-1301:⚠️ Potential issue | 🟠 MajorKeep the invalid-discriminator guard in
Value::type().Lines 1279-1301 replace the switch with a dense table, but
getInternalType()can still synthesize sparse or invalidInternalTypevalues from bad tag bits. Those now either read past the table or silently default tonThunkinstead of tripping an unreachable path, which weakens the hardening added elsewhere in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/include/nix/expr/value.hh` around lines 1277 - 1301, The dense lookup table in Value::type() removes the previous invalid-discriminator guard, which allows getInternalType() to produce out-of-range or sparse InternalType values that index past the table or silently yield nThunk; restore a bounds check and hard-fail path: before returning table[getInternalType()] in Value::type(), validate the index produced by getInternalType() is within the table size and corresponds to a valid InternalType (or re-use the earlier switch/unreachable pattern), and on invalid values call the same unreachable/error-handling used elsewhere in this file so bad tag bits still trigger a hard failure rather than silent fallback.src/libstore/build/build-log.cc-11-17 (1)
11-17:⚠️ Potential issue | 🟠 MajorCRLF line endings are currently dropped as empty lines.
Line 14 zeroes
currentLogLinePoson\r, so"foo\r\n"reaches Line 17 as an empty line. Any builder that emits CRLF will lose its log content and push blank lines intologTail; this needs to distinguish a standalone carriage return from the\r\nnewline sequence, including across chunk boundaries.Also applies to: 31-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/build/build-log.cc` around lines 11 - 17, BuildLog::operator() is treating '\r' as an immediate line-end (setting currentLogLinePos = 0) which causes CRLF sequences to be interpreted as an extra empty line; change the logic to detect a pending carriage-return across chunks: introduce a pendingCR flag (persisted in BuildLog) that is set when seeing '\r' instead of resetting currentLogLinePos, and on seeing '\n' check pendingCR to consume the '\n' as the continuation of the CRLF pair (call flushLine() once), while treating a standalone '\r' (when followed by a non-'\n' or at chunk boundary without a subsequent '\n') as a real newline; apply the same fix to the other handler section (lines 31-45) and ensure pendingCR is cleared appropriately after handling.doc/manual/source/protocols/json/schema/build-trace-entry-v2.yaml-33-36 (1)
33-36:⚠️ Potential issue | 🟠 MajorInvalid JSON Schema
additionalPropertiesstructure.In JSON Schema draft-04,
additionalPropertiesmust be either a boolean or a schema that applies to all additional properties—not a mapping of specific property names. The current structure won't validatedependentRealisationsas intended.If the goal is to allow
dependentRealisationsas a deprecated optional field while disallowing other additional properties, movedependentRealisationsintopropertiesand setadditionalProperties: false.🔧 Proposed fix
properties: id: {} outPath: {} signatures: {} -additionalProperties: - dependentRealisations: - description: deprecated field - type: object + dependentRealisations: + description: deprecated field + type: object +additionalProperties: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/manual/source/protocols/json/schema/build-trace-entry-v2.yaml` around lines 33 - 36, The schema incorrectly uses additionalProperties as a mapping with a key dependentRealisations; change the schema to declare dependentRealisations as an explicit optional property under properties (e.g., add dependentRealisations with description "deprecated field" and type: object inside the properties block) and then set additionalProperties: false so that other unknown properties are disallowed; ensure the dependentRealisations entry remains marked deprecated in its description and remove the misplaced mapping under additionalProperties.src/libcmd/unix/unix-socket-server.cc-67-68 (1)
67-68:⚠️ Potential issue | 🟠 MajorReplace
assertwith proper error handling forLISTEN_FDSparsing.If
LISTEN_FDScontains a non-numeric value,string2Intreturnsstd::nullopt. Usingasserthere is problematic: in release builds (withNDEBUG), the assertion may be compiled out, leading to undefined behavior when dereferencing*count. In debug builds, it aborts without a clear error message.🔧 Proposed fix
auto count = string2Int<unsigned int>(*listenFds); - assert(count); + if (!count) + throw Error("invalid LISTEN_FDS value: '%s'", *listenFds); for (unsigned int i = 0; i < count; ++i) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libcmd/unix/unix-socket-server.cc` around lines 67 - 68, Replace the unsafe assert on the optional returned by string2Int when parsing LISTEN_FDS: check whether auto count = string2Int<unsigned int>(*listenFds) has a value before dereferencing, and handle the error path by logging a clear error and returning/throwing (e.g., processLogger.error or throw std::runtime_error) instead of using assert; update the code around the LISTEN_FDS parsing in unix-socket-server.cc (the count variable and its use) to fail gracefully with a descriptive message when string2Int returns std::nullopt.src/libstore-tests/worker-substitution.cc-179-180 (1)
179-180:⚠️ Potential issue | 🟠 MajorRestore the previous experimental-feature set with RAII.
These tests overwrite the global
extra-experimental-featuressetting and then hard-reset it to"". If anASSERT_*aborts early, or if the suite started with other experimental features already enabled, later tests inherit the wrong configuration. Please capture the previous value and restore it with a scope guard instead of hardcoding the reset.Also applies to: 272-273, 283-284, 446-447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore-tests/worker-substitution.cc` around lines 179 - 180, The test mutates the global experimentalFeatureSettings by calling experimentalFeatureSettings.set("extra-experimental-features", "...") and then resets it to "", which can leak state on early aborts; capture the current value into a local string and use an RAII scope guard (e.g., a small local struct with a destructor or existing ScopeExit utility) that restores experimentalFeatureSettings.set("extra-experimental-features", previous) when it goes out of scope; replace the hard-reset calls at the shown location and the other instances referenced (around lines ~272-273, ~283-284, ~446-447) to ensure the original value is always restored even on ASSERT_* failures.src/libexpr/primops.cc-1882-1889 (1)
1882-1889:⚠️ Potential issue | 🟠 MajorRead-only mode now returns a
drvPaththe store cannot actually resolve.This branch skips
writeDerivation(...), but the result still exposesdrvPathwithDrvDeepcontext later in the function. Line 1760 already notes thatDrvDeepresolution does not work in read-only mode, and those paths still call back intocomputeFSClosure()/readDerivation()on the store. That means nested derivations or anything that later realizesdrv.drvPathcan now fail underreadOnlyMode.Either keep materializing
.drvfiles when their path escapes evaluation, or add a read-only derivation cache that those readers consult before bypassing the write.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr/primops.cc` around lines 1882 - 1889, The current read-only branch returns a drvPath computed via computeStorePath when settings.readOnlyMode is true, but that produces DrvDeep paths that later call computeFSClosure()/readDerivation() and will fail; update the logic around drvPath creation (the assignment using computeStorePath and state.store->writeDerivation) so that when readOnlyMode is set but the derivation will be used/resolved with DrvDeep semantics you still materialize the .drv (call state.store->writeDerivation with state.asyncPathWriter, state.repair, provenance) instead of computeStorePath, or alternatively introduce a read-only derivation cache that computeFSClosure()/readDerivation() consult before failing; adjust the branch around settings.readOnlyMode, computeStorePath, and writeDerivation to ensure no DrvDeep resolution points to an unmaterialized path (refer to drvPath, DrvDeep, computeFSClosure, readDerivation, settings.readOnlyMode, computeStorePath, writeDerivation, state.asyncPathWriter, state.repair, provenance).src/libstore/http-binary-cache-store.cc-28-29 (1)
28-29:⚠️ Potential issue | 🟠 MajorValidate the actual URI scheme here, not the global allowed-scheme set.
This condition now skips authority validation for all URLs whenever
_NIX_FORCE_HTTP=1makes"file"appear inuriSchemes(). That lets malformedhttp/httpsstore URLs through.Suggested patch
- if (!uriSchemes().contains("file") && (!cacheUri.authority || cacheUri.authority->host.empty())) + if (cacheUri.scheme != "file" && (!cacheUri.authority || cacheUri.authority->host.empty())) throw UsageError("`%s` Store requires a non-empty authority in Store URL", cacheUri.scheme);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/http-binary-cache-store.cc` around lines 28 - 29, The check is incorrectly using uriSchemes() to decide whether to require an authority, so when "file" is present (e.g. via _NIX_FORCE_HTTP) it skips validation for all stores; change the condition to validate based on the specific URI scheme in the incoming cacheUri (e.g., check cacheUri.scheme == "file" or != "file") rather than uriSchemes(), and throw the UsageError with the same message when cacheUri.scheme requires an authority but cacheUri.authority is missing or host is empty; update the condition around the existing UsageError call that references cacheUri.scheme/cacheUri.authority accordingly.src/libfetchers/git-utils.cc-643-650 (1)
643-650:⚠️ Potential issue | 🟠 MajorUse OS-string literals for all fixed git arguments.
The initializer at line 643 mixes raw narrow string literals with platform-native
OsStringtypes. On Windows whereOsStringis wide-character based, this causes type mismatches in the initializer list. Wrap all fixed flags withOS_STR()to ensure consistent platform-native width, matching the pattern already used in the subsequentpush_backcalls.Suggested patch
- OsStrings gitArgs{"-C", dir.native(), "--git-dir", ".", "fetch", "--progress", "--force"}; + OsStrings gitArgs{ + OS_STR("-C"), + dir.native(), + OS_STR("--git-dir"), + OS_STR("."), + OS_STR("fetch"), + OS_STR("--progress"), + OS_STR("--force"), + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libfetchers/git-utils.cc` around lines 643 - 650, The gitArgs initializer mixes narrow string literals with OsStrings causing platform mismatch; change the initializer for gitArgs in function creating the fetch command so every fixed argument uses OS_STR(...) (e.g., replace "-C", "--git-dir", ".", "fetch", "--progress", "--force" with OS_STR("-C"), OS_STR("--git-dir"), OS_STR("."), OS_STR("fetch"), OS_STR("--progress"), OS_STR("--force")) so all entries in gitArgs are platform-native OsString instances and remain consistent with later push_back calls that use OS_STR() and string_to_os_string(dir.native())/string_to_os_string(refspec).src/libstore/build/goal.cc-24-55 (1)
24-55:⚠️ Potential issue | 🟠 MajorQueue
ChildEOFevents instead of storing just one.Hook builds now watch multiple child descriptors. A single
childEOFslot plusassert(!childEOF)means two EOFs before the coroutine drains the queue will either trip the assert or lose one close notification. This needs the same FIFO treatment aschildOutputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/build/goal.cc` around lines 24 - 55, The code stores only a single ChildEOF in childEOF and asserts on a second, which can drop or crash when multiple EOFs arrive; change childEOF from an optional to a FIFO (e.g., a std::deque or std::queue) and treat it the same way as childOutputs: remove the assert in pushChildEvent(ChildEOF) and push the event onto the EOF queue, update hasChildEvent() to check the EOF queue, and update popChildEvent() to return/consume from the EOF queue (after childOutputs) before falling back to the existing childTimeout handling; also ensure pushChildEvent(TimedOut) still flushes both output and EOF queues when a timeout occurs.src/libstore/build/derivation-building-goal.cc-816-820 (1)
816-820:⚠️ Potential issue | 🟠 MajorRead derivation provenance from the build store too.
When
worker.evalStore != worker.store, copied/imported derivations can exist only inworker.store. Looking only inworker.evalStoreturnsdrvProvenanceintonullptr, so a local build drops the provenance chain instead of propagating it to the new outputs.Possible fix
- if (auto info = worker.evalStore.maybeQueryPathInfo(drvPath)) - provenance = info->provenance; + for (auto * drvStore : {&worker.evalStore, &worker.store}) { + if (auto info = drvStore->maybeQueryPathInfo(drvPath)) { + provenance = info->provenance; + break; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/build/derivation-building-goal.cc` around lines 816 - 820, The code only queries worker.evalStore for derivation provenance (via maybeQueryPathInfo(drvPath)) which returns nullptr when evalStore != worker.store and the derivation lives in worker.store; update the logic in build/derivation-building-goal.cc so after trying worker.evalStore you also try worker.store (when different) and set the local provenance variable accordingly (e.g., try auto info = worker.evalStore.maybeQueryPathInfo(drvPath); if not found and worker.store != worker.evalStore then info = worker.store.maybeQueryPathInfo(drvPath); provenance = info ? info->provenance : nullptr) to ensure drvProvenance/provenance is populated from the build store as well.src/libstore/aws-creds.cc-273-280 (1)
273-280:⚠️ Potential issue | 🟠 MajorKeep ECS enabled when shared TLS setup fails.
createECSProvider()already accepts a nullable TLS context, and bothcreateECSProviderandcreateSTSWebIdentityProviderimplementations show they handle null by passingnullptrto the AWS CRT library. AWS documentation confirms thatAWS_CONTAINER_CREDENTIALS_RELATIVE_URI(the standard ECS flow) uses HTTP to the local agent (no TLS needed), whileAWS_CONTAINER_CREDENTIALS_FULL_URI(EKS) can optionally use HTTPS. STS WebIdentity, however, always requires HTTPS/TLS.Gating ECS on
if (tlsContext)therefore makes the warning message inaccurate and disables valid HTTP-based container credential flows whenever shared TLS context creation fails. The warning incorrectly claims "ECS container authentication will be unavailable" when only the HTTPS variant would be affected.Suggested fix
tlsContext = std::make_shared<Aws::Crt::Io::TlsContext>(tlsCtxOptions, Aws::Crt::Io::TlsMode::CLIENT, allocator); if (!tlsContext || !*tlsContext) { warn( - "failed to create TLS context for AWS credential providers; SSO, STS WebIdentity, and ECS container authentication will be unavailable"); + "failed to create TLS context for AWS credential providers; SSO and STS WebIdentity authentication will be unavailable"); tlsContext = nullptr; } @@ - if (tlsContext) { - addProviderToChain("STS WebIdentity", [&]() { - return createSTSWebIdentityProvider(profile, bootstrap, tlsContext.get(), allocator); - }); - ecsAdded = - addProviderToChain("ECS", [&]() { return createECSProvider(bootstrap, tlsContext.get(), allocator); }); - } else { - debug( - "Skipped AWS STS WebIdentity and ECS Credential Providers for profile '%s': TLS context unavailable", - profileDisplayName); - } + if (tlsContext) { + addProviderToChain("STS WebIdentity", [&]() { + return createSTSWebIdentityProvider(profile, bootstrap, tlsContext.get(), allocator); + }); + } else { + debug( + "Skipped AWS STS WebIdentity Credential Provider for profile '%s': TLS context unavailable", + profileDisplayName); + } + + ecsAdded = + addProviderToChain("ECS", [&]() { return createECSProvider(bootstrap, tlsContext.get(), allocator); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/aws-creds.cc` around lines 273 - 280, The shared TLS context creation warning and gating incorrectly disables ECS credentials; change the logic so createECSProvider() is still invoked when tlsContext is null (pass nullptr) while only SSO and STS WebIdentity behavior that truly requires TLS is conditioned on tlsContext, and update the warn(...) text to say TLS creation failed and that SSO and STS WebIdentity (and any HTTPS-based ECS flow) will be unavailable rather than claiming ECS container authentication is entirely unavailable; reference tlsContext, createECSProvider(), and createSTSWebIdentityProvider() when making the change.src/libstore/aws-creds.cc-382-392 (1)
382-392:⚠️ Potential issue | 🟠 MajorAdd
AWS_EC2_METADATA_DISABLEDcheck before adding the IMDS provider.The aws-c-auth default credentials chain checks
AWS_EC2_METADATA_DISABLEDat provider construction time and skips adding the IMDS provider entirely if set totrue. This custom chain bypasses that check by unconditionally adding IMDS whenever ECS was not added, so explicit IMDS opt-outs will still trigger metadata traffic. (docs.aws.amazon.com)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/aws-creds.cc` around lines 382 - 392, Before adding the IMDS provider in the block that calls addProviderToChain with CredentialsProviderImdsConfig/CreateCredentialsProviderImds, check the AWS_EC2_METADATA_DISABLED environment variable (e.g., getenv) and treat values like "true" or "1" (case-insensitive) as disabling IMDS; if disabled, skip registering the IMDS provider and emit a debug/info message similar to the existing ECS-skip message referencing profileDisplayName and that AWS_EC2_METADATA_DISABLED is set. Ensure the check is performed only when ecsAdded is false so behavior remains unchanged when ECS is active.
🟡 Minor comments (18)
packaging/wasmtime.nix-6-12 (1)
6-12:⚠️ Potential issue | 🟡 MinorClarify Rust version requirement for wasmtime 40.0.2.
This change removes the explicit
rust_1_89pin in favor of a genericrustinput. However, wasmtime 40.0.2 specifiesrust-version = "1.89.0"in its Cargo.toml. Removing the version pin without a mechanism to enforce this requirement risks build failures if a caller provides an incompatible Rust version.Either restore the explicit version pin or add a comment documenting that callers must provide Rust 1.89.0 (or compatible).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packaging/wasmtime.nix` around lines 6 - 12, The change replaces the explicit rust_1_89 pin with a generic rust input, but wasmtime 40.0.2 requires Rust 1.89.0 (Cargo.toml rust-version = "1.89.0"); either restore the explicit rust_1_89 input or document the requirement so callers provide a Rust 1.89-compatible toolchain. Modify the packaging/wasmtime.nix invocation that uses rust.packages.stable.rustPlatform.buildRustPackage: either reintroduce the rust_1_89 input and use it in place of rust, or add a clear comment above the rust argument and validate (or assert) the provided rust matches 1.89 semantics to prevent accidental mismatches.src/libstore/builtins/buildenv.cc-54-57 (1)
54-57:⚠️ Potential issue | 🟡 MinorWarn on the broken source path.
Line 54 probes
srcFile, but Line 56 logsdstFile. That points debugging at the profile destination, which does not exist yet, instead of the dangling store entry that was actually skipped.Suggested fix
- warn("skipping dangling symlink '%s'", dstFile); + warn("skipping dangling symlink '%s'", srcFile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/builtins/buildenv.cc` around lines 54 - 57, The warning logs the wrong path when maybeStat(srcFile.c_str()) fails: change the warn call in the block that checks srcStOpt (the code using maybeStat, srcFile, dstFile and warn in buildenv.cc) so it reports the broken source path (use srcFile / srcFile.c_str()) instead of dstFile, preserving the existing message format and continuing behavior.doc/manual/source/language/syntax.md-289-308 (1)
289-308:⚠️ Potential issue | 🟡 MinorDocumentation of legacy syntax is helpful, but the evaluation result formatting is inconsistent.
The example shows
This evaluates to "baz".but for consistency with other examples in this document (e.g., line 252:This evaluates to '123'., line 287:This evaluates to '"foobar"'.), the string value should be shown in a way that distinguishes it as a string literal.Consider either quoting it consistently:
-This evaluates to "baz". +This evaluates to `"baz"`.Or using a code block like other examples in the document.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/manual/source/language/syntax.md` around lines 289 - 308, The example sentence uses inconsistent quoting: change the line "This evaluates to "baz"." to match the document's other examples by formatting the result as a literal string (e.g., This evaluates to '"baz"'.) for the example block that contains the let { foo = bar; bar = "baz"; body = foo; } snippet so the string value is shown consistently as a string literal.src/libstore/build-result.cc-24-28 (1)
24-28:⚠️ Potential issue | 🟡 MinorTypo: "permenant" should be "permanent".
📝 Suggested fix
case BuildResult::Failure::PermanentFailure: - // Also considered a permenant failure, it seems + // Also considered a permanent failure, it seems case BuildResult::Failure::InputRejected:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/build-result.cc` around lines 24 - 28, Fix the typo in the comment above the switch cases for BuildResult::Failure::PermanentFailure and BuildResult::Failure::InputRejected: change "permenant" to "permanent" so the comment reads correctly and matches the variable permanentFailure referenced in this block.src/libstore/include/nix/store/build/drv-output-substitution-goal.hh-23-24 (1)
23-24:⚠️ Potential issue | 🟡 MinorTypo in TODO comment: "BuidlTraceEntryGoal" should be "BuildTraceEntryGoal".
📝 Suggested fix
- * `@todo` rename this `BuidlTraceEntryGoal`, which will make sense + * `@todo` rename this `BuildTraceEntryGoal`, which will make sense🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/include/nix/store/build/drv-output-substitution-goal.hh` around lines 23 - 24, Fix the typo in the TODO comment inside drv-output-substitution-goal.hh by changing "BuidlTraceEntryGoal" to "BuildTraceEntryGoal" (the comment also references `Realisation`/`BuildTraceEntry` context—only the typo in the TODO needs correction). Update the single-line TODO so it reads "rename this `BuildTraceEntryGoal`, which will make sense especially once `Realisation` is renamed to `BuildTraceEntry`."src/libflake-c/nix_api_flake_internal.hh-16-16 (1)
16-16:⚠️ Potential issue | 🟡 MinorAdd a direct
<filesystem>include forstd::filesystem::path.Line 16 uses
std::filesystem::pathbut this header does not include<filesystem>directly, relying instead on transitive includes which is fragile.Proposed fix
`#pragma` once +#include <filesystem> `#include` <optional>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libflake-c/nix_api_flake_internal.hh` at line 16, The header uses std::filesystem::path via the member baseDirectory but doesn't include <filesystem>, relying on transitive includes; add a direct `#include` <filesystem> at the top of nix_api_flake_internal.hh so the declaration std::optional<std::filesystem::path> baseDirectory is valid and not fragile.src/libstore/builtins/fetchurl.cc-69-70 (1)
69-70:⚠️ Potential issue | 🟡 MinorRemoved error handling for
chmodfailure.The previous implementation threw
SysErroronchmodfailure. The new implementation silently ignores errors. While the derivation would likely fail later if executability is critical, consider at minimum logging a warning on failure for debuggability.🛡️ Proposed fix to add warning on chmod failure
auto executable = ctx.drv.env.find("executable"); if (executable != ctx.drv.env.end() && executable->second == "1") { - chmod(storePath, 0755); + if (chmod(storePath, 0755) != 0) + warn("chmod failed for '%s': %s", storePath, strerror(errno)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/builtins/fetchurl.cc` around lines 69 - 70, The chmod call on storePath currently ignores failures; restore error handling by checking its return value and, on failure, log a warning that includes the failing path and system error details (errno/strerror) so it's debuggable (e.g., after chmod(storePath, 0755) check for -1 and call the module's logger or perror with storePath and strerror(errno)); optionally preserve previous behavior of throwing SysError in contexts where executability is critical, but at minimum emit a clear warning referencing storePath and chmod.src/libflake-tests/nix_api_flake.cc-380-385 (1)
380-385:⚠️ Potential issue | 🟡 Minor
overrideFlakeReferencestill leaks on the Step 6 success path.
overrideFlakeReferenceallocated at Line 353 is never released in this cleanup block. If leak checking is enabled, this test will still report a leak.♻️ Proposed cleanup
ASSERT_EQ("Claire", helloStr); nix_locked_flake_free(lockedFlake); + nix_flake_reference_free(overrideFlakeReference); nix_flake_reference_parse_flags_free(parseFlags); nix_flake_lock_flags_free(lockFlags); nix_flake_reference_free(flakeReference);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libflake-tests/nix_api_flake.cc` around lines 380 - 385, The cleanup block is missing a free for the overrideFlakeReference allocated in overrideFlakeReference (Step 6 success path); add a call to nix_flake_reference_free(overrideFlakeReference) alongside the existing frees (nix_locked_flake_free, nix_flake_reference_parse_flags_free, nix_flake_lock_flags_free, nix_flake_reference_free(flakeReference), nix_state_free, nix_flake_settings_free) so overrideFlakeReference is released on the success path to prevent the leak.doc/manual/source/protocols/json/build-trace-entry.md-20-20 (1)
20-20:⚠️ Potential issue | 🟡 MinorInconsistent version reference in comment.
The text says "Build Trace Entry v1" but the link points to
build-trace-entry-v2.json. The text should be updated to match the schema version.📝 Proposed fix
-[JSON Schema for Build Trace Entry v1](schema/build-trace-entry-v2.json) +[JSON Schema for Build Trace Entry v2](schema/build-trace-entry-v2.json)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/manual/source/protocols/json/build-trace-entry.md` at line 20, Update the inconsistent version label: replace the text "Build Trace Entry v1" with "Build Trace Entry v2" (or change the linked filename to build-trace-entry-v1.json if v1 is intended) so the display label and the link target (schema/build-trace-entry-v2.json) match; look for the literal "Build Trace Entry v1" in build-trace-entry.md and adjust it to the correct version string to keep text and link consistent.src/libstore-tests/nix_api_store.cc-874-875 (1)
874-875:⚠️ Potential issue | 🟡 MinorFinish the test-data lookup migration.
Line 966 still uses
getenv("_NIX_TEST_UNIT_DATA"), sonix_store_build_pathsremains environment-dependent even after introducinggetUnitTestData(). That leaves one test with the same harness fragility this change is fixing elsewhere.♻️ Minimal follow-up
- std::filesystem::path unitTestData{getenv("_NIX_TEST_UNIT_DATA")}; + std::filesystem::path unitTestData = nix::getUnitTestData();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore-tests/nix_api_store.cc` around lines 874 - 875, The test still reads test data via getenv("_NIX_TEST_UNIT_DATA") in nix_store_build_paths, leaving the harness environment-dependent; replace that getenv usage with the new nix::getUnitTestData() call and use its returned std::filesystem::path to construct the test-data file paths (similar to how unitTestData / filename is used elsewhere), updating nix_store_build_paths to derive paths from getUnitTestData() instead of getenv.src/libexpr-tests/get-drvs-bench.cc-16-32 (1)
16-32:⚠️ Potential issue | 🟡 MinorMember
evalSettingsat line 18 is shadowed by the initializer list.The
evalSettingsmember is initialized at line 18 usingreadOnlyMode, but then immediately overwritten by the initializer list lambda (lines 26-30). The member initialization at line 18 is dead code.🔧 Proposed fix to remove redundant initialization
struct GetDerivationsEnv { ref<Store> store = openStore("dummy://"); fetchers::Settings fetchSettings{}; - bool readOnlyMode = true; - EvalSettings evalSettings{readOnlyMode}; + static constexpr bool readOnlyMode = true; + EvalSettings evalSettings; std::shared_ptr<EvalState> statePtr; EvalState & state; Bindings * autoArgs = nullptr; Value attrsValue; explicit GetDerivationsEnv(size_t attrCount) - : evalSettings([&]() { - EvalSettings settings{readOnlyMode}; + : evalSettings([&]() -> EvalSettings { + EvalSettings settings{GetDerivationsEnv::readOnlyMode}; settings.nixPath = {}; return settings; }())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libexpr-tests/get-drvs-bench.cc` around lines 16 - 32, The member evalSettings is redundantly initialized at declaration and then overwritten in GetDerivationsEnv's ctor initializer list; remove the in-class initialization "EvalSettings evalSettings{readOnlyMode};" and instead initialize evalSettings only in the constructor initializer (the lambda currently used) so the ctor initializes evalSettings, then statePtr (std::make_shared<EvalState>(... , evalSettings, ...)) and state reference remain consistent; update declarations to just "EvalSettings evalSettings;" and leave the constructor initializer that builds the desired settings.doc/manual/source/release-notes/rl-2.34.md-11-13 (1)
11-13:⚠️ Potential issue | 🟡 MinorAdd language identifiers to these fenced code blocks.
markdownlintis already flagging MD040 here. Please tag these blocks (shell,console,ini,text, etc.) so the release notes lint cleanly.Also applies to: 18-20, 44-50, 57-63, 81-86, 89-94, 102-115, 150-152, 166-169, 233-235, 391-394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/manual/source/release-notes/rl-2.34.md` around lines 11 - 13, The fenced code blocks in the release notes lack language identifiers (e.g. the block containing "curl -sSfL https://artifacts.nixos.org/nix-installer | sh -s -- install" should be tagged, e.g. ```sh or ```bash); update that block and all other blocks referenced in the comment to include appropriate language tags (shell/console/ini/text as appropriate) so markdownlint MD040 is satisfied—search for the identical fenced snippets and add the language identifier directly after the opening triple backticks.src/libstore-c/nix_api_store.cc-334-339 (1)
334-339:⚠️ Potential issue | 🟡 MinorMinor typo in comment.
Line 335: "suceed" should be "succeed".
📝 Typo fix
- /* Quite dubious that users would want this to silently suceed + /* Quite dubious that users would want this to silently succeed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore-c/nix_api_store.cc` around lines 334 - 339, Fix the typo in the comment above the derivation write logic: change "suceed" to "succeed" in the comment that precedes the conditional assignment involving nix::settings.readOnlyMode, nix::computeStorePath(*store->ptr, derivation->drv) and store->ptr->writeDerivation(derivation->drv, nix::NoRepair).doc/manual/source/protocols/json/schema/build-trace-entry-v2.yaml-15-15 (1)
15-15:⚠️ Potential issue | 🟡 MinorTypo: "Verision" should be "Version".
📝 Proposed fix
- Verision history: + Version history:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/manual/source/protocols/json/schema/build-trace-entry-v2.yaml` at line 15, Fix the typo in the YAML header by changing the string "Verision history:" to "Version history:" in the schema (locate the header line that currently reads "Verision history:" and correct the spelling). Ensure only the single word "Verision" is replaced with "Version" and preserve the rest of the line and formatting.src/libstore/build/worker.cc-28-31 (1)
28-31:⚠️ Potential issue | 🟡 MinorUse the worker’s captured settings here, not the mutable global singleton.
Workernow stores asettingssnapshot, but these paths still consultnix::settings.getWorkerSettings(). That lets an already-constructed worker change substitution behavior or emit a different-j0diagnostic if global settings are mutated later. Line 377 has the same pattern forbuilders.♻️ Suggested change
- , getSubstituters{[] { - return nix::settings.getWorkerSettings().useSubstitutes ? getDefaultSubstituters() : std::list<ref<Store>>{}; - }} + , getSubstituters{[this] { + return settings.useSubstitutes ? getDefaultSubstituters() : std::list<ref<Store>>{}; + }} ... - if (Machine::parseConfig({nix::settings.thisSystem}, nix::settings.getWorkerSettings().builders).empty()) + if (Machine::parseConfig({nix::settings.thisSystem}, settings.builders).empty())Also applies to: 377-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/build/worker.cc` around lines 28 - 31, The constructor currently calls nix::settings.getWorkerSettings() when building getSubstituters and builders, which ignores the Worker instance's captured settings; change those initializers to use the instance member settings (e.g. use settings.useSubstitutes and settings.builders or equivalent fields/methods) and call getDefaultSubstituters() only when settings.useSubstitutes is true, and likewise construct builders from settings rather than nix::settings.getWorkerSettings(); update both the getSubstituters initializer and the builders initialization (the members named getSubstituters and builders and the captured member settings) to reference the captured settings snapshot.src/libstore/gc.cc-311-323 (1)
311-323:⚠️ Potential issue | 🟡 MinorThe roots-daemon path censors runtime roots unconditionally.
requestRuntimeRoots()inserts{censored}beforefindRuntimeRoots()has a chance to look at thecensorflag, so callers that explicitly request uncensored roots lose the original labels wheneveruseRootsDaemonis enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/gc.cc` around lines 311 - 323, requestRuntimeRoots currently unconditionally inserts the sentinel symbol censored into roots, which destroys original labels for callers that want uncensored names; modify requestRuntimeRoots (and its call sites if needed) to respect the censor flag by inserting the actual parsed label (e.g. config.parseStorePath(line) value) when censoring is disabled and only use the censored sentinel when censoring is enabled (or when a config.censor or explicit bool censor argument is true). Locate requestRuntimeRoots, the use of censored, and any callers that rely on uncensored output (e.g. findRuntimeRoots/useRootsDaemon) and propagate a censor parameter or consult LocalStoreConfig so the function conditionally inserts censored vs the real label.src/libstore/include/nix/store/build-result.hh-183-189 (1)
183-189:⚠️ Potential issue | 🟡 MinorThrow a copy instead of mutating the stored failure.
withExitStatus()mutates theFailureinsideBuildResult, so a one-off exit code can leak into later rethrows or serialization of the same result. This helper should only decorate the thrown copy.Suggested patch
void tryThrowBuildError(std::optional<unsigned int> exitStatus = std::nullopt) { if (auto * failure = tryGetFailure()) { + auto toThrow = *failure; if (exitStatus) - failure->withExitStatus(*exitStatus); - throw *failure; + toThrow.withExitStatus(*exitStatus); + throw toThrow; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/include/nix/store/build-result.hh` around lines 183 - 189, tryThrowBuildError currently mutates the stored Failure via failure->withExitStatus(...), which can leak the one-off exit code; instead, copy the Failure returned by tryGetFailure() into a local variable (e.g. Failure copy = *failure), call copy.withExitStatus(*exitStatus) when present, and then throw the copy so the original Failure inside BuildResult remains unmodified; reference tryThrowBuildError, tryGetFailure, withExitStatus, Failure and BuildResult when making this change.src/libstore-test-support/include/nix/store/tests/https-store.hh-68-70 (1)
68-70:⚠️ Potential issue | 🟡 MinorAvoid a fixed TCP port in the fixture.
Hard-coding
8443makes this suite race with local services and parallel test workers. Please allocate an ephemeral port inSetUp()and thread it intomakeConfig()/serverArgs()instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore-test-support/include/nix/store/tests/https-store.hh` around lines 68 - 70, The fixture currently hard-codes uint16_t port = 8443 which causes port collisions; change the member `port` to be uninitialized (or 0) and in the test fixture's `SetUp()` bind a temporary socket to port 0 to obtain an ephemeral assigned port, store that value into `port`, and then pass that dynamic port into `makeConfig()` and `serverArgs()` (and any code that currently reads the literal 8443) so the server and client use the allocated ephemeral port rather than a fixed one; ensure `serverPid`, `localCacheStore`, and teardown still work with the new `port` member.
This was used in only one place (the daemon implementation) and it hard-coded a protocol version, so let's get rid of it.
This makes `nix copy` from ssh-ng to the daemon preserve provenance, because in combination with the previous commits, RemoteStore::addToStoreMultiple() now sends provenance if the daemon supports it. Fixes #369.
1b8c48b to
4934dbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libstore/remote-store.cc`:
- Around line 472-477: The version comparison should use the numeric component
only to avoid unordered three-way comparisons: replace the condition that
compares getConnection()->protoVersion with WorkerProto::Version{.number={1,32}}
so it compares getConnection()->protoVersion.number against the target numeric
version (e.g. WorkerProto::Version{.number={1,32}}.number or the equivalent
VersionNumber literal). Update the if in remote-store.cc (the block that
currently calls Store::addMultipleToStore and returns) to use
getConnection()->protoVersion.number <
WorkerProto::Version{.number={1,32}}.number so older daemons like
1.31+provenance are correctly detected as less-than.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dea712ad-e234-442b-8f52-7b46ac3084e2
📒 Files selected for processing (8)
src/libstore/daemon.ccsrc/libstore/include/nix/store/remote-store.hhsrc/libstore/include/nix/store/store-api.hhsrc/libstore/include/nix/store/worker-protocol-connection.hhsrc/libstore/include/nix/store/worker-protocol.hhsrc/libstore/remote-store.ccsrc/libstore/store-api.ccsrc/libstore/worker-protocol.cc
💤 Files with no reviewable changes (4)
- src/libstore/include/nix/store/remote-store.hh
- src/libstore/include/nix/store/worker-protocol-connection.hh
- src/libstore/include/nix/store/store-api.hh
- src/libstore/store-api.cc
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/libstore/remote-store.cc (1)
472-477: Double connection acquisition is inefficient.
getConnection()is called at line 472 for the version check (temporary handle, released immediately), then called again at line 477. Other fallback patterns in this file acquire once and release before fallback (e.g.,queryMissinguses scope +goto,buildPathsWithResultsusesstd::optional+reset()).♻️ Suggested refactor
- if (getConnection()->protoVersion.number < WorkerProto::Version::Number{1, 32}) { + auto conn(getConnection()); + + if (conn->protoVersion.number < WorkerProto::Version::Number{1, 32}) { + // Release connection before fallback to prevent deadlock + conn.handle.markBad(); Store::addMultipleToStore(std::move(pathsToCopy), act, repair, checkSigs); return; } - - auto conn(getConnection());Alternatively, follow the
buildPathsWithResultspattern usingstd::optional<ConnectionHandle>andreset()before fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/remote-store.cc` around lines 472 - 477, Currently getConnection() is called twice (once for the version check and again to create conn), which is inefficient; change the logic to acquire the connection exactly once into a local handle (e.g., use std::optional<ConnectionHandle> or the existing conn variable), inspect conn->protoVersion.number to decide the fallback, and if you must call Store::addMultipleToStore(...) release the connection first by resetting the optional or letting the handle go out of scope before invoking the fallback; update the code around getConnection(), conn, and the Store::addMultipleToStore call to follow the buildPathsWithResults pattern (optional + reset()) so the connection is not held during the fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/libstore/remote-store.cc`:
- Around line 472-477: Currently getConnection() is called twice (once for the
version check and again to create conn), which is inefficient; change the logic
to acquire the connection exactly once into a local handle (e.g., use
std::optional<ConnectionHandle> or the existing conn variable), inspect
conn->protoVersion.number to decide the fallback, and if you must call
Store::addMultipleToStore(...) release the connection first by resetting the
optional or letting the handle go out of scope before invoking the fallback;
update the code around getConnection(), conn, and the Store::addMultipleToStore
call to follow the buildPathsWithResults pattern (optional + reset()) so the
connection is not held during the fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f06d0c87-d85c-4c84-b067-e0692cac0a11
📒 Files selected for processing (1)
src/libstore/remote-store.cc
This branch was previously deployed
2 participants
Motivation
Fixes #369.
Depends on #386.
Context
Summary by CodeRabbit
New Features
Refactor