src/libexpr/include/nix/expr/eval-settings.hh (1)

366-380: Good: clear doc for eval-cores and affected commands.

This addresses the prior request to document impact; default and “0 = all cores” semantics are clear.

src/libexpr/include/nix/expr/symbol-table.hh (1)

20-24: Consider adding a cast function for type safety

Based on previous review feedback requesting _cast functions, consider introducing a named cast function for clarity and type safety instead of using reinterpret_cast directly.

     operator std::string_view() const noexcept
     {
         // The actual string is stored directly after the value.
-        return reinterpret_cast<const char *>(this + 1);
+        return castToString();
     }
+
+private:
+    const char * castToString() const noexcept
+    {
+        return reinterpret_cast<const char *>(this + 1);
+    }

src/libexpr/print-ambiguous.cc (1)

81-83: Handle failed values in ambiguous printer — consider de-duplicating the literal.

The new nFailed branch is correct. To prevent drift with Printer::printFailed, centralize the "«failed»" rendering (shared helper or constant).

src/libexpr/include/nix/expr/eval-inline.hh (1)

91-142: Complex control flow with goto needs careful review

While the goto usage here appears to be for performance optimization in a hot path, the complex state machine with multiple exit points makes the code harder to maintain and reason about.

Consider restructuring to avoid goto while maintaining performance:

+    auto processResult = [&](auto p0_val) {
+        if (InternalType(p0_val & 0xff) == tFailed)
+            std::rethrow_exception((std::bit_cast<Failed *>(p1))->ex);
+    };
+
     if (pd == pdThunk) {
         try {
             // ... existing thunk processing logic ...
         } catch (...) {
             // ... existing error handling ...
             throw;
         }
+        return;
     }
-    else if (pd == pdPending || pd == pdAwaited)
+    
+    if (pd == pdPending || pd == pdAwaited) {
         p0_ = waitOnThunk(state, pd == pdAwaited);
+        processResult(p0_);
+        return;
+    }
 
-done:
-    if (InternalType(p0_ & 0xff) == tFailed)
-        std::rethrow_exception((std::bit_cast<Failed *>(p1))->ex);
+    processResult(p0_);

src/libexpr/include/nix/expr/eval.hh (1)

970-975: Counter naming clarification needed.

The name microsecondsWaiting could be clearer. Also ensure these new performance counters are properly initialized.

Consider renaming to microsWaiting or waitTimeMicros for clarity, as suggested in the past review comment.

src/libexpr/include/nix/expr/parallel-eval.hh (1)

76-76: Address the FIXME comment about destructor behavior.

There's an existing comment from cole-h asking about the reason this hasn't been fixed. The FIXME suggests that the destructor should handle cancellation/waiting for futures, which is important for proper cleanup and avoiding resource leaks or undefined behavior when a FutureVector is destroyed with pending work.

Do you want me to generate a solution that properly implements cancellation/waiting in the destructor?

src/libexpr/eval.cc (1)

1182-1184: Potential race condition when copying forced value.

The comment on line 2189 mentions a race where a thunk might be updated by another thread. This same race condition could occur here when copying the value after forcing.

The value might be modified by another thread between forceValue and the assignment. Consider using atomic operations or proper synchronization:

     forceValue(*vExpr, noPos);
-
-    v = *vExpr;
+    // Use memcpy to avoid issues with concurrent value updates
+    memcpy(&v, vExpr, sizeof(Value));

src/libexpr/parallel-eval.cc (2)

257-257: Inconsistent error context message.

The error context differs from the suggested fix in the past review comment.

-    state.forceList(*args[0], pos, "while evaluating the first argument passed to builtins.parallel");
+    state.forceList(*args[0], pos, "while evaluating the first argument passed to builtins.parallel");

The current message is correct and matches the convention used elsewhere in the codebase.

---

270-281: Remove or implement the experimental feature gate.

The FIXME comment at line 271 indicates this should be gated behind an experimental feature, but it's currently always enabled when registered with .experimentalFeature = Xp::ParallelEval.

Either remove the FIXME comment since the feature is already gated, or clarify what additional gating is needed:

-// FIXME: gate this behind an experimental feature.
 static RegisterPrimOp r_parallel({
     .name = "__parallel",

src/libstore/include/nix/store/remote-store.hh (1)

24-26: Default jump to 64 connections: aligns with parallel eval, but consider CPU-aware cap and doc note

• This materially changes behavior (1 → 64). Good for multithreaded eval, but may oversubscribe small machines or CI runners and increase daemon FD/memory pressure.
• Suggest a CPU-aware default with a ceiling to keep behavior sane across hosts, while preserving the ability to override via nix.conf.

Apply this diff within the initializer to pick a dynamic default (cap at 64, reasonable floor), keeping the option overrideable:

-    const Setting<int> maxConnections{
-        this, 64, "max-connections", "Maximum number of concurrent connections to the Nix daemon."};
+    const Setting<int> maxConnections{
+        this,
+        [] {
+            unsigned c = std::thread::hardware_concurrency();
+            int d = c ? static_cast<int>(c) * 2 : 8; // fallback for unknown core count
+            if (d < 4) d = 4;
+            if (d > 64) d = 64;
+            return d;
+        }(),
+        "max-connections",
+        "Maximum number of concurrent connections to the Nix daemon."};

Additional includes required at the top of the file:

#include <thread>

Follow-ups:

• Document the new default in nix.conf docs/manpage and release notes as a behavior change (“per RemoteStore instance”).
• If you see daemon load spikes in CI, consider lowering the cap (e.g., 32) or introducing a per-host override for builders with tight limits.

packaging/dependencies.nix (1)

88-88: Boost: verify Thread’s transitive libs are built
Ensure Boost.Thread’s dependent libraries (System, Chrono, Atomic) are present after building with extraB2Args = ["--with-thread"]. After running

nix build .#boost
fd -HI 'libboost_{thread,system,chrono,atomic}*' $(nix eval --raw .#boost.outPath)

verify that libboost_thread*, libboost_system*, libboost_chrono*, and libboost_atomic* all exist. If any are missing, extend extraB2Args as:

         "--with-thread"
+        "--with-system"
+        "--with-chrono"
+        "--with-atomic"

tests/functional/misc.sh (1)

25-26: Make error-location assertions resilient and quote variables

Avoid brittle exact columns and unquoted expansions.

Apply:

-e
-echo $eval_arg_res | grep "at «string»:1:12:"
-echo $eval_arg_res | grep "infinite recursion encountered"
+printf '%s\n' "$eval_arg_res" | grep -E 'at «string»:1:[0-9]+:'
+printf '%s\n' "$eval_arg_res" | grep -F "infinite recursion encountered"

-e
-echo $eval_stdin_res | grep "at «stdin»:1:12:"
-echo $eval_stdin_res | grep "infinite recursion encountered"
+printf '%s\n' "$eval_stdin_res" | grep -E 'at «stdin»:1:[0-9]+:'
+printf '%s\n' "$eval_stdin_res" | grep -F "infinite recursion encountered"

Also applies to: 29-30

tests/functional/flakes/show.sh (1)

62-62: Harden the assertion against blank lines/colors.

Counting raw lines is brittle if formatting changes. Filter empty lines and force no color.

-[[ $(nix flake show --all-systems --legacy | wc -l) = 1 ]]
+[[ $(nix --color never flake show --all-systems --legacy | sed '/^[[:space:]]*$/d' | wc -l) = 1 ]]

Optional (if jq is available): assert via JSON instead of display formatting.

nix flake show --json --all-systems --legacy | jq -e 'keys==["description"]'

src/libstore/globals.cc (1)

153-162: Consider marking noexcept (if getMaxCPU() is non-throwing).

Minor polish; communicates intent and may enable small optimizations.

src/libutil/include/nix/util/pos-table.hh (1)

54-63: Add missing standard headers explicitly.

This header uses std::map (and std::prev in resolve()) but doesn’t include (and ). Avoid relying on transitive includes.

Add:

• #include
• #include
• #include

src/libutil/include/nix/util/util.hh (1)

221-229: Generalize key type and consider ref-returning variant to avoid copies.

Current getOptional copies mapped_type and only accepts key_type; heavy types pay a copy cost.

Minimal generalization:

-template<class T>
-std::optional<typename T::mapped_type> getOptional(const T & map, const typename T::key_type & key)
+template<class T, typename K>
+std::optional<typename T::mapped_type> getOptional(const T & map, const K & key)
{
    auto i = map.find(key);
    if (i == map.end())
        return std::nullopt;
    return {i->second};
}

Optional: add a zero-copy helper alongside (keeps current API intact):

template<class T, typename K>
std::optional<std::reference_wrapper<const typename T::mapped_type>>
getOptionalRef(const T & map, const K & key)
{
    auto i = map.find(key);
    if (i == map.end()) return std::nullopt;
    return std::cref(i->second);
}

src/libexpr/include/nix/expr/eval-settings.hh (1)

388-392: Consider making eval stack size configurable.

Hard-coding 64 MiB may not fit all platforms/workloads. Expose as a Setting (with this as default) or env knob for tunability.

src/libutil/mounted-source-accessor.cc (1)

62-69: Consider extracting the lock scope for clarity.

While the current implementation is correct, the scoping block on lines 62-69 could be more explicit about its purpose. This is a minor readability suggestion.

Consider adding a comment to clarify the lock scope:

-            {
+            { // Acquire read lock for mounts lookup
                 auto mounts(mounts_.readLock());

src/libutil/experimental-features.cc (1)

318-320: Consider expanding the feature description.

The description for the ParallelEval feature is quite minimal compared to other features. Given this is a significant performance enhancement, users would benefit from more details about what it enables and any potential implications.

Consider providing a more comprehensive description:

         .description = R"(
-            Enable built-in functions for parallel evaluation.
+            Enable built-in functions for parallel evaluation.
+            
+            This feature allows Nix to evaluate expressions in parallel using multiple
+            threads, potentially improving performance for large evaluations. The number
+            of threads can be configured using the `eval-cores` setting.
+            
+            Note: This is an experimental feature and may have stability implications.
         )",

.github/workflows/build.yml (5)

128-134: Matrix entry “eval-cores = 24” is mutually exclusive with lazy-trees settings.

Right now, each job tests either lazy-trees or eval-cores, but never both together. If the intent is to test eval-cores under both lazy-trees true/false, consider a 2D matrix (lazy_trees x eval_cores) and composing NIX_CONFIG from both, or add combined entries like "lazy-trees = true\neval-cores = 24".

Example change:

 strategy:
   matrix:
-    nix_config:
-      - "lazy-trees = true"
-      - "lazy-trees = false"
-      - "eval-cores = 24"
+    lazy_trees: [true, false]
+    eval_cores: [0, 24]
     glob:
       - "[0-d]*"
       - "[e-l]*"
       - "[m]*"
       - "[n-r]*"
       - "[s-z]*"

and in the step env:

-  NIX_CONFIG: ${{ matrix.nix_config }}
+  NIX_CONFIG: |
+    lazy-trees = ${{ matrix.lazy_trees }}
+    eval-cores = ${{ matrix.eval_cores }}

---

156-161: Re-enable dynamic PARALLEL to keep overall CI throughput.

Hard-coding PARALLEL to -P 1 may slow the suite when eval-cores isn’t configured. If that was unintentional, restore the conditional variant.

-          PARALLEL: '-P 1'
+          PARALLEL: ${{ contains(matrix.nix_config, 'eval-cores') && '-P 1' || '-P 50%' }}

---

166-169: Shellcheck: prefer -n over ! -z and avoid assigning in export.

Small bash hygiene fixes; keeps actionlint/shellcheck green.

-          if [ ! -z "${NSC_CACHE_PATH:-}" ]; then
+          if [ -n "${NSC_CACHE_PATH:-}" ]; then
             mkdir -p "${NSC_CACHE_PATH}/nix/xdg-cache"
-            export XDG_CACHE_HOME="${NSC_CACHE_PATH}/nix/xdg-cache"
+            XDG_CACHE_HOME="${NSC_CACHE_PATH}/nix/xdg-cache"
+            export XDG_CACHE_HOME
           fi

---

170-173: Shellcheck: avoid command substitution inside export (SC2155).

Split assignment and export for PATH.

-          export PATH=$(pwd)/new-nix/bin:$PATH
+          PATH="$(pwd)/new-nix/bin:$PATH"
+          export PATH

---

179-183: Optional: stricter bash mode with a controlled retry.

set -euo pipefail can surface early failures; keep the retry by disabling -e just for the flaky block.

-          set -x
+          set -xeuo pipefail
           echo "PARALLEL: $PARALLEL"
           echo "NIX_CONFIG: $NIX_CONFIG"
@@
-          if ! flake-regressions/eval-all.sh; then
+          set +e
+          flake-regressions/eval-all.sh
+          rc=$?
+          set -e
+          if [ $rc -ne 0 ]; then
             echo "Some failed, trying again"
             printf "\n\n\n\n\n\n\n\n"
             NIX_REMOTE=/tmp/nix flake-regressions/eval-all.sh
           fi

src/libexpr/include/nix/expr/counter.hh (2)

15-28: Prefer explicit atomic loads; avoid implicit conversion from std::atomic.

Use inner.load(memory_order_relaxed) for reads to be clear and avoid any ambiguity with deprecated implicit conversions.

-    operator value_type() const noexcept
-    {
-        return inner;
-    }
+    operator value_type() const noexcept { return inner.load(std::memory_order_relaxed); }
@@
-    value_type load() const noexcept
-    {
-        return inner;
-    }
+    value_type load() const noexcept { return inner.load(std::memory_order_relaxed); }

---

59-59: Use standard alignas for portability.

attribute((aligned)) is GCC/Clang-specific. Prefer alignas(64).

-} __attribute__((aligned(64))); // cache line alignment to prevent false sharing
+} alignas(64); // cache line alignment to prevent false sharing

src/libexpr/include/nix/expr/value.hh (5)

440-473: Concurrency doc and protocol look good; minor nit on wording.

Great to see finish() centralizing the p1-before-p0 invariant with release semantics. The comment block is clear and addresses the awaited/pending states.

Two nits:

• “use `acquire semantics” -> remove the stray asterisks for consistency with “release”.
• Consider explicitly mentioning that readers must use acquire loads on p0 (as you already do) before reading p1.

---

524-543: finish(): handle unexpected prior states explicitly (defensive).

If p0_ decodes to pdSingleDWord/pdPairOfPointers/etc., silently doing nothing hides bugs. Logging once (rate-limited) or asserting in debug builds helps catch misuse.

-        if (pd == pdPending)
-            // Nothing to do; no thread is waiting on this thunk.
-            ;
-        else if (pd == pdAwaited)
+        if (pd == pdPending) {
+            // No waiters.
+        } else if (pd == pdAwaited) {
             // Slow path: wake up the threads that are waiting on this
             // thunk.
             notifyWaiters();
-        else if (pd == pdThunk) {
+        } else if (pd == pdThunk) {
             printError("BAD FINISH %x", this);
             unreachable();
+        } else {
+            // Unexpected: finishing over a non-thunk state.
+            debug("finish(): overwrote state %d", (int)pd);
         }

---

574-586: setThunkPayload(): consider release semantics on publishing the thunk header.

Even though this is initial construction, using a release store on p0 (or a fence) future-proofs against any re-ordering surprises on less common architectures or if construction paths evolve.

-        p0.store(static_cast<int>(pdThunk) | firstFieldPayload, std::memory_order_relaxed);
+        p0.store(static_cast<int>(pdThunk) | firstFieldPayload, std::memory_order_release);

---

1280-1323: Accessor methods for thunk/app noted as racy — follow-up to remove or guard.

You’ve marked thunk() and app() as racy. Consider guarding their use under asserts that the value is a finished thunk representation or delete them once call sites are migrated.

---

1028-1033: isFinished(): optional fast-path.

The FIXME notes a possible optimization. A small fast-path check on p0’s primary discriminator (without decoding full type) would avoid a switch in hot paths. Not urgent, but worth a TODO link.

src/libexpr/include/nix/expr/symbol-table.hh (1)

35-35: Centralize cache line alignment into a platform‐configurable constant

Current code hard-codes __attribute__((aligned(64))) in three places:

• src/libexpr/parallel-eval.cc:175
• src/libexpr/include/nix/expr/counter.hh:59
• src/libexpr/include/nix/expr/symbol-table.hh:35

Assuming a 64-byte cache line is common on x86_64 but may be suboptimal on other architectures (e.g. 32-byte on ARM). Define a single CACHE_LINE_SIZE (or use std::hardware_destructive_interference_size) and replace each hard-coded 64 with alignas(CACHE_LINE_SIZE) to make cache‐line alignment configurable per platform.

src/libexpr/symbol-table.cc (3)

8-11: Document MAP_NORESERVE fallback behavior

The fallback definition of MAP_NORESERVE to 0 when undefined could lead to different memory allocation behavior across platforms. Consider adding a comment explaining the implications.

 #ifndef MAP_NORESERVE
+// MAP_NORESERVE is not available on all platforms (e.g., macOS).
+// When set to 0, memory is reserved immediately rather than on-demand.
 #  define MAP_NORESERVE 0
 #endif

---

12-18: Consider fallback for mmap failure on large allocations

The arena requests 1GB of virtual memory (1 << 30 from the header). On systems with limited virtual memory or restrictive overcommit settings, this could fail even with MAP_NORESERVE.

Consider implementing a fallback mechanism or making the arena size configurable:

 static void * allocateLazyMemory(size_t maxSize)
 {
     auto p = mmap(nullptr, maxSize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0);
-    if (p == MAP_FAILED)
+    if (p == MAP_FAILED) {
+        // Try without MAP_NORESERVE as a fallback
+        p = mmap(nullptr, maxSize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+        if (p == MAP_FAILED)
             throw SysError("allocating arena using mmap");
+    }
     return p;
 }

---

45-61: Extract alignment computation into a shared helper

The constructor’s expression ((rawSize + alignment – 1)/alignment)*alignment and the dump method’s (n + (n%8 ? 8-(n%8) : 0)) both perform the same rounding up to SymbolTable::alignment. Factor this into a single helper to avoid drift and ease future changes:

+static size_t alignSize(size_t rawSize, size_t alignment) {
+    return ((rawSize + alignment - 1) / alignment) * alignment;
+}
…
-    auto size = ((rawSize + SymbolTable::alignment - 1) / SymbolTable::alignment) * SymbolTable::alignment;
+    auto size = alignSize(rawSize, SymbolTable::alignment);

src/libexpr/print.cc (2)

515-519: Optional: colorize 'failed' consistently and/or reuse a shared helper.

Match styling with other diagnostic prints (e.g., ANSI_RED) and avoid duplicating the literal across files.

 void printFailed(Value & v)
 {
-    output << "«failed»";
+    if (options.ansiColors) output << ANSI_RED;
+    output << "«failed»";
+    if (options.ansiColors) output << ANSI_NORMAL;
 }

---

636-638: Remove unused local ‘seen’ shadowing the member.

This local variable is unused and shadows the optional member ‘seen’, which is confusing.

-        ValuesSeen seen;
         print(v, 0);

src/libexpr/include/nix/expr/nixexpr.hh (1)

93-93: Switching to Counter for nrExprs: confirm cheap disabled-path.

Assuming Counter guards increments behind an enable flag. If not, consider gating the hot-path increment to keep parser perf stable when counting is disabled.

Apply if Counter exposes an enabled() check:

-    nrExprs++;
+    if (nrExprs.enabled()) nrExprs++;

src/libexpr-tests/value/print.cc (2)

13-18: Pass Value as const reference.

The helper doesn’t mutate v; prefer const Value & to tighten the contract.

-    void test(Value & v, std::string_view expected, A... args)
+    void test(const Value & v, std::string_view expected, A... args)

---

624-633: Avoid mutating the same Bindings backing vAttrs (aliasing hazard).

Reusing the same builder after finish() likely mutates the bindings that vAttrs points to. Build vAttrs2 with a fresh builder to keep tests independent.

-    builder.insert(state.symbols.create("three"), &vThree);
-    Value vAttrs2;
-    vAttrs2.mkAttrs(builder.finish());
+    BindingsBuilder builder3(state, state.allocBindings(10));
+    builder3.insert(state.symbols.create("one"),  &vOne);
+    builder3.insert(state.symbols.create("two"),  &vTwo);
+    builder3.insert(state.symbols.create("three"), &vThree);
+    Value vAttrs2;
+    vAttrs2.mkAttrs(builder3.finish());

src/libexpr/primops.cc (1)

5101-5160: Unconditionally constructing currentTime/currentSystem: confirm pure-eval behavior.

Values are created even in pure-eval but gated from exposure via impureOnly. Please confirm:

• In pure-eval, neither symbol appears in builtins.
• No tests depend on their absence vs presence in the environment snapshot.

If you want to avoid doing time(0) work when impure-only, guard it:

-    v.mkInt(time(0));
+    if (!settings.pureEval && !settings.restrictEval) v.mkInt(time(0));

src/libexpr/eval-gc.cc (3)

36-68: Consider improving error handling in memory parsing

The getFreeMem() function silently falls back to defaults when parsing fails. Consider logging warnings when memory information cannot be obtained or parsed incorrectly.

 static size_t getFreeMem()
 {
     /* On Linux, use the `MemAvailable` or `MemFree` fields from
        /proc/cpuinfo. */
 #  ifdef __linux__
     {
         std::unordered_map<std::string, std::string> fields;
         for (auto & line :
              tokenizeString<std::vector<std::string>>(readFile(std::filesystem::path("/proc/meminfo")), "\n")) {
             auto colon = line.find(':');
             if (colon == line.npos)
                 continue;
             fields.emplace(line.substr(0, colon), trim(line.substr(colon + 1)));
         }
 
         auto i = fields.find("MemAvailable");
         if (i == fields.end())
             i = fields.find("MemFree");
         if (i != fields.end()) {
             auto kb = tokenizeString<std::vector<std::string>>(i->second, " ");
-            if (kb.size() == 2 && kb[1] == "kB")
-                return string2Int<size_t>(kb[0]).value_or(0) * 1024;
+            if (kb.size() == 2 && kb[1] == "kB") {
+                auto mem = string2Int<size_t>(kb[0]);
+                if (mem.has_value())
+                    return mem.value() * 1024;
+                debug("Failed to parse memory value from /proc/meminfo");
+            }
         }
+        debug("Could not find MemAvailable or MemFree in /proc/meminfo");
     }
 #  endif
 
     /* On non-Linux systems, conservatively assume that 25% of memory is free. */
     long pageSize = sysconf(_SC_PAGESIZE);
     long pages = sysconf(_SC_PHYS_PAGES);
     if (pageSize != -1)
         return (pageSize * pages) / 4;
     return 0;
 }

---

119-121: Consider adding assertions for stack pointer validity

After adjusting the stack pointer to osStackLo, consider adding debug assertions to verify the adjusted pointer is valid and within expected bounds.

     if (sp >= osStackHi || sp < osStackLo) // sp is outside the os stack
         sp = osStackLo;
+#ifndef NDEBUG
+    assert(sp >= osStackLo && sp < osStackHi && "Adjusted stack pointer should be within bounds");
+#endif

---

156-173: Consider making heap sizing parameters configurable

The heap sizing logic uses hardcoded values (32MB min, 4GB max, 50% of free memory). Consider making these configurable via environment variables or settings for better flexibility in different deployment scenarios.

Would you like me to help create a configuration system for these heap sizing parameters?

src/nix/search.cc (2)

113-117: Consider explaining the priority logic

The priority assignment logic (checking for "Packages" in the attribute name) could benefit from a comment explaining why packages get lower priority (0 vs 2).

                         work.emplace_back(
                             [cursor2, attrPath2, visit]() { visit(*cursor2, attrPath2, false); },
+                            // Give "Packages" attributes lower priority to process other attributes first
                             std::string_view(state->symbols[attr]).find("Packages") != std::string_view::npos ? 0 : 2);

---

167-175: Consider batching console output for better performance

Writing to the console per-result in parallel threads could cause contention and jumbled output. Consider accumulating results and printing them in batches.

Instead of immediate console output, consider collecting results and printing them after futures.finishAll() or implementing a dedicated output queue with a single writer thread.

src/libexpr/include/nix/expr/eval-inline.hh (1)

106-107: Consider strengthening memory ordering for CAS

The compare_exchange uses acquire ordering for both success and failure cases. Consider if acq_rel for success would provide better guarantees for the subsequent operations on the thunk.

-            if (!p0.compare_exchange_strong(p0_, pdPending, std::memory_order_acquire, std::memory_order_acquire)) {
+            if (!p0.compare_exchange_strong(p0_, pdPending, std::memory_order_acq_rel, std::memory_order_acquire)) {

src/libexpr/value-to-json.cc (2)

44-142: Consider adding bounded parallelism for deeply nested structures.

The recursive traversal correctly handles all value types with parallel spawning. However, for deeply nested structures, unbounded parallelism could lead to excessive memory usage from queued tasks.

Consider adding a depth limit or task queue size limit to prevent resource exhaustion:

-    auto spawn = [&](auto work) {
+    size_t depth = 0;
+    auto spawn = [&](auto work, size_t currentDepth = 0) {
-        if (doParallel) {
+        if (doParallel && currentDepth < 100) {  // Limit parallel depth
             futures.spawn(0, [work{std::move(work)}]() { work(); });
         } else {
             work();
         }
     };

---

96-106: Add error recovery for parallel attribute evaluation.

While error traces are properly added, there's no mechanism to cancel other spawned tasks when one fails. This could lead to unnecessary computation.

Consider implementing early termination when errors occur in parallel tasks to avoid wasted computation on siblings that will ultimately be discarded.

src/nix/flake.cc (1)

519-528: Document and replace magic priority levels in futures.spawn

Add documentation for priority values (0–3) in parallel-eval.hh and replace raw literals (e.g., 1 in flake.cc:519-528) with a descriptive enum or named constants to clarify the task ordering in futures.spawn calls.

src/libexpr/eval.cc (3)

493-495: Document why raw memcpy is necessary here.

The comment mentions that operator = fails on thunks, but since this occurs after value construction, it would be helpful to explain why a simple assignment cannot be used even for non-thunk values.

 Value * EvalState::addConstant(const std::string & name, Value & v, Constant info)
 {
     Value * v2 = allocValue();
-    // Do a raw copy since `operator =` barfs on thunks.
+    // Do a raw copy since `operator =` barfs on thunks. This is safe because
+    // we're copying the entire Value structure including its internal storage.
     memcpy((char *) v2, (char *) &v, sizeof(Value));

---

1540-1540: Thread-local variable should be documented.

The thread-local callDepth variable is introduced without documentation about its purpose or usage pattern.

+// Track recursion depth per thread to detect infinite recursion in parallel evaluation
 thread_local size_t EvalState::callDepth = 0;

---

2813-2815: Consider handling nFailed case in switch statement.

The new nFailed value type is marked as unreachable in forceValue() results, but it would be safer to handle it explicitly.

-    // Cannot be returned by forceValue().
-    case nThunk:
-    case nFailed:
-        unreachable();
+    case nThunk:
+        unreachable(); // Cannot be returned by forceValue()
+    case nFailed:
+        error<AssertionError>("cannot compare failed values").debugThrow();

src/libexpr/parallel-eval.cc (3)

101-126: Potential integer overflow in priority key calculation.

The key calculation (uint64_t(item.second) << 48) could silently truncate if item.second (uint8_t) multiplied by the shift exceeds expected bounds.

-            auto key = (uint64_t(item.second) << 48) | dist(rd);
+            // Priority in upper 16 bits, random value in lower 48 bits
+            auto key = (static_cast<uint64_t>(item.second) << 48) | dist(rd);

---

175-175: Consider making cache line size configurable.

The hard-coded cache line alignment of 64 bytes may not be optimal for all architectures (some use 128 bytes).

-} __attribute__((aligned(64))); // cache line alignment to prevent false sharing
+} __attribute__((aligned(std::hardware_destructive_interference_size))); // cache line alignment to prevent false sharing

Note: std::hardware_destructive_interference_size requires C++17 and may not be available on all compilers. Consider using a compile-time constant that can be configured per platform.

---

221-224: Error message could be more informative.

When detecting infinite recursion with a single eval core, the error message could explain why this happens in parallel evaluation.

-        state.error<InfiniteRecursionError>("infinite recursion encountered")
+        state.error<InfiniteRecursionError>("infinite recursion encountered (parallel evaluation requires multiple cores to resolve circular dependencies)")

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/libexpr/eval-gc.cc (1)

99-106: Restore pthread_attr_get_np fallback; current #error breaks BSD/non-glibc builds

The FreeBSD/non-glibc path is hard-erroring. Implement the pthread_attr_get_np branch with proper error checks.

Apply:

-#    ifdef HAVE_PTHREAD_GETATTR_NP
-    if (pthread_getattr_np(pthread_id, &pattr))
-        throw Error("fixupBoehmStackPointer: pthread_getattr_np failed");
-#    else
-#      error "Need  `pthread_attr_get_np`"
-#    endif
+#    ifdef HAVE_PTHREAD_GETATTR_NP
+    if (pthread_getattr_np(pthread_id, &pattr))
+        throw Error("fixupBoehmStackPointer: pthread_getattr_np failed");
+#    elif defined(HAVE_PTHREAD_ATTR_GET_NP)
+    if (pthread_attr_get_np(pthread_id, &pattr))
+        throw Error("fixupBoehmStackPointer: pthread_attr_get_np failed");
+#    else
+#      error "Need pthread_getattr_np or pthread_attr_get_np"
+#    endif

src/libexpr/include/nix/expr/symbol-table.hh (2)

240-244: Add bounds and alignment checks in operator[] and launder the pointer

Prevent out-of-bounds access and misaligned loads; also use std::launder for safety after placement construction.

     SymbolStr operator[](Symbol s) const
     {
-        assert(s.id);
-        return SymbolStr(*reinterpret_cast<const SymbolValue *>(arena.data + s.id));
+        assert(s.id);
+        // Basic integrity checks
+        assert(s.id < arena.size); // offset within arena
+        assert((s.id % alignment) == 0); // start of an aligned record
+        assert(s.id + sizeof(SymbolValue) <= arena.size); // header fits
+        auto p = std::launder(reinterpret_cast<const SymbolValue *>(arena.data + s.id));
+        return SymbolStr(*p);
     }

---

256-273: Fix dump() record-advance logic to account for header in alignment calculation

Current padding uses only the string length; it should include the Value header to remain aligned. Otherwise, iteration can desynchronize and hit asserts or skip symbols.

     void dump(T callback) const
     {
         std::string_view left{arena.data, arena.size};
         left = left.substr(alignment);
         while (true) {
             if (left.empty())
                 break;
             left = left.substr(sizeof(Value));
             auto p = left.find('\0');
             assert(p != left.npos);
             auto sym = left.substr(0, p);
             callback(sym);
             // skip alignment padding
-            auto n = sym.size() + 1;
-            left = left.substr(n + (n % alignment ? alignment - (n % alignment) : 0));
+            auto n = sizeof(Value) + sym.size() + 1; // header + string + NUL
+            auto alignedSize = ((n + alignment - 1) / alignment) * alignment;
+            left = left.substr(alignedSize - sizeof(Value));
         }
     }

src/libexpr/eval-gc.cc (4)

38-40: Fix comment: /proc/meminfo, not /proc/cpuinfo

The comment references cpuinfo but the code reads meminfo.

Apply:

-    /* On Linux, use the `MemAvailable` or `MemFree` fields from
-       /proc/cpuinfo. */
+    /* On Linux, use the `MemAvailable` or `MemFree` fields from
+       /proc/meminfo. */

---

41-59: Harden Linux meminfo parsing against read failures

readFile("/proc/meminfo") can throw; let’s fall back instead of propagating.

Apply:

-    {
+    try {
         std::unordered_map<std::string, std::string> fields;
         for (auto & line :
              tokenizeString<std::vector<std::string>>(readFile(std::filesystem::path("/proc/meminfo")), "\n")) {
             auto colon = line.find(':');
             if (colon == line.npos)
                 continue;
             fields.emplace(line.substr(0, colon), trim(line.substr(colon + 1)));
         }

         auto i = fields.find("MemAvailable");
         if (i == fields.end())
             i = fields.find("MemFree");
         if (i != fields.end()) {
             auto kb = tokenizeString<std::vector<std::string>>(i->second, " ");
             if (kb.size() == 2 && kb[1] == "kB")
                 return string2Int<size_t>(kb[0]).value_or(0) * 1024;
         }
-    }
+    } catch (const std::exception &) {
+        /* Fall through to generic sysconf-based fallback below. */
+    }

---

151-159: Comment/code mismatch (80% vs 50%)

Comment says 80% but code uses ~50% of free RAM.

Apply one of:

• Update comment to 50%:

-    /* Set the initial heap size to something fairly big (80% of
+    /* Set the initial heap size to something fairly big (50% of
        free RAM, up to a maximum of 4 GiB) so that in most cases

• Or change code to 80% (if intended).

---

164-166: Avoid float math; use integer halving

free * 0.5 promotes to double and back. Use integer division for precision.

Apply:

-        auto free = getFreeMem();
-        size = std::max(size, std::min((size_t) (free * 0.5), maxSize));
+        auto free = getFreeMem();
+        auto halfFree = free / 2;
+        size = std::max(size, std::min(halfFree, maxSize));

src/libexpr/include/nix/expr/symbol-table.hh (7)

27-40: Make arena.data writable without const_casts and keep the pointer itself const

Construction writes through data; using const here forces const_cast elsewhere. Prefer a const pointer to mutable memory.

-    const char * data;
+    char * const data;

Follow-up: drop const_casts at call sites (e.g., SymbolStr::SymbolStr in symbol-table.cc).

---

32-36: Confirm cache-line isolation intent is achieved

aligned(64) on the atomic ensures its start is 64B-aligned, but only if the compiler inserts padding before it. Given preceding fields, this is likely fine, but please verify with static_asserts or sizeof/offsetof in a unit test.

Example check:

• Expect offsetof(ContiguousArena, size) % 64 == 0.
• Optionally add alignas(64) to the struct and reorder members if needed.

---

101-109: Key stores arena by reference; ensure lifetime and thread-safety are clear

Key captures ContiguousArena& and is used inside a concurrent container; document that arena outlives the set and is append-only. If not guaranteed, consider a pointer.

---

139-149: size()/empty() are O(n) due to std::string_view length computation

This is fine if not hot; otherwise consider storing the length next to the string to avoid repeated scans.

---

219-225: Constructor reserves ID 0; also assert arena invariants

Add cheap asserts to catch early misuse.

     SymbolTable()
         : arena(1 << 30)
     {
         // Reserve symbol ID 0 and ensure alignment of the first allocation.
         arena.allocate(alignment);
+        assert((uintptr_t)arena.data % alignment == 0);
+        assert(arena.size == alignment);
     }

---

251-254: Atomic load semantics for totalSize()

If used for metrics only, relaxed is fine and cheaper; otherwise use acquire. Make it explicit.

-    size_t totalSize() const
-    {
-        return arena.size;
-    }
+    size_t totalSize() const
+    {
+        return arena.size.load(std::memory_order_relaxed);
+    }

---

22-24: Consistent access: consider using Value APIs or document the aliasing

Returning a string_view by reinterpreting (this + 1) is correct for your layout but bypasses Value’s accessor; add a comment asserting that mkString stored the same pointer and that the trailing bytes remain immutable.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/libexpr/include/nix/expr/value.hh (4)

440-542: Complex concurrency design requires careful analysis.

The atomic p0 and non-atomic p1 design with the ordering constraint ("p1 must be written before p0") is a sophisticated approach to lock-free value updates. However, this creates several concerns:

1. Race condition potential: The requirement that "p1 must always be updated before p0" (Line 459) must be strictly enforced across all code paths.

2. Memory ordering: The use of memory_order_release for p0 writes and memory_order_acquire for reads is correct, but the implementation must be consistent.

3. Debug output in production: Lines 539-541 contain debug output that may not be appropriate for production builds.

Apply this fix for the debug output:

-        else if (pd == pdThunk) {
-            printError("BAD FINISH %x", this);
-            unreachable();
-        }
+        else if (pd == pdThunk)
+            unreachable();

---

1308-1312: Race condition warning for thunk accessor.

The comment "FIXME: remove this since reading it is racy" correctly identifies that accessing thunk data is unsafe in a multithreaded context. This method should be marked deprecated or removed.

Consider deprecating this method or adding runtime checks:

+    [[deprecated("Reading thunk data is racy in multithreaded evaluation")]]
     ClosureThunk thunk() const noexcept

---

1319-1323: Race condition warning for app accessor.

The comment "FIXME: remove this since reading it is racy" correctly identifies the same threading issue as with the thunk accessor. This should also be addressed.

---

781-791: Testing-only blackhole function should be restricted.

The mkBlackhole() function is marked as "only used for testing" but is in the public interface. This could be accidentally used in production code.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/libexpr/include/nix/expr/value.hh (1)

572-585: Release-store in setThunkPayload is correct.

This now preserves the “p1-before-p0” ordering without spurious wakeups for awaited states. Thanks for addressing the earlier fence concern.

src/libexpr/include/nix/expr/eval.hh (1)

377-381: Good move to SharedSync for caches; consider similar treatment for remaining maps.

lookupPathResolved, primOpCalls, and attrSelects are still marked non-thread-safe. Consider SharedSync or per-thread aggregation with periodic merge to avoid lock contention.

Also applies to: 392-393, 398-398

src/libexpr/include/nix/expr/value.hh (2)

1217-1221: Failed payload: confirm GC ownership and lifetime.

mkFailed() allocates with new on a gc-derived type. Please confirm this is traced by the GC in both Boehm and non-Boehm builds and that any printers/visitors handle nFailed without dereferencing a null ex.

Optionally add a brief docstring to ValueType::nFailed and Value::failed() clarifying semantics.

Also applies to: 1331-1335

---

1044-1049: Expose racy accessors only where safe.

isBlackhole() is fine. The “FIXME: remove this since reading it is racy” for thunk()/app() remains—consider #ifdef UNIT_TESTS or moving to a test-only header to prevent accidental use.

Also applies to: 1304-1309, 1315-1320

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/libstore/include/nix/store/ssh.hh (1)

30-31: Make tmpDir immutable and document lifetime/thread-safety

tmpDir is effectively write-once; marking it const prevents accidental reassignment and clarifies intent. Also, because Connections don’t hold a ref to tmpDir, verify SSHMaster outlives any in-flight ssh child using files under tmpDir.

-    ref<AutoDelete> tmpDir;
+    const ref<AutoDelete> tmpDir;

If you want to make usage clearer and avoid repeated recomputation of paths, consider adding:

• a cached known-hosts file path (e.g., Path knownHostsFile;)
• a cached control socket path (e.g., Path controlSocket;)

src/libstore/ssh.cc (1)

241-242: Use consistent path joining; avoid deref/cast of ref

Dereferencing tmpDir and C-style casting to Path is brittle. Use the same path API you used above.

-    state->socketPath = (Path) *tmpDir + "/ssh.sock";
+    state->socketPath = (tmpDir->path() / "ssh.sock").string();

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.