Conversation
This shows the currently active builds for a particular Nix store. These are tracked in per-build files in `/nix/var/nix/active-builds`.
WalkthroughAdds active-build tracking and querying across store, daemon, and builders: new ActiveBuild types and serializers, TrackActiveBuildsStore/QueryActiveBuildsStore interfaces, LocalStore on-disk tracking with per-build JSON and process inspection, RemoteStore/daemon protocol support, a new nix ps command, table/terminal utilities, and a small progress-bar width tweak. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "nix ps"
participant Remote as "RemoteStore"
participant Daemon as "Daemon"
participant Local as "LocalStore"
participant FS as "active-builds dir"
CLI->>Remote: require<QueryActiveBuildsStore>()\nqueryActiveBuilds()
Remote->>Daemon: send WorkerProto::Op::QueryActiveBuilds
Daemon->>Local: resolve QueryActiveBuildsStore\nqueryActiveBuilds()
Local->>FS: scan per-build files (lock & read JSON)\ncollect /proc or cgroup info
FS-->>Local: vector<ActiveBuildInfo>
Local-->>Daemon: JSON response
Daemon-->>Remote: forward JSON
Remote-->>CLI: return vector<ActiveBuildInfo>
CLI->>CLI: render table/process trees (uses getWindowWidth())
sequenceDiagram
participant Builder as "DerivationBuilder"
participant Tracker as "TrackActiveBuildsStore"
participant FS as "active-builds dir"
Builder->>Tracker: buildStarted(ActiveBuild)
Tracker->>FS: create & lock per-build file\nwrite JSON metadata
FS-->>Tracker: BuildHandle (holds lock/id)
Tracker-->>Builder: return BuildHandle
Note over Builder: build runs with handle retained
Builder->>Tracker: buildFinished(BuildHandle) or handle destructor
Tracker->>FS: unlock/remove per-build file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 3
🧹 Nitpick comments (7)
src/libutil/include/nix/util/file-system.hh (1)
290-296: AutoDelete move constructor semantics look correct; consider include + noexceptThe move constructor correctly transfers the deletion responsibility by moving
_path, copyingdel/recursive, and clearingx.del, so the moved‑from object will not attempt cleanup. To make this more robust and self‑contained, consider:
- Including
<utility>in this header sostd::moveis not reliant on transitive includes.- Marking the move constructor
noexceptifstd::filesystem::pathmoves are assumed not to throw in your targets, which can help with container optimizations.Please confirm that this header compiles in isolation (e.g., with a TU that includes only
nix/util/file-system.hh) and that no additional noexcept requirements exist for uses ofAutoDeletein standard containers.src/libstore/local-store.cc (1)
150-150: Verify behavior in read-only mode.The
createDirs(activeBuildsDir)is called unconditionally, similar tocreateDirs(linksDir)and others. In read-only mode, this could fail. However, active build tracking would only be meaningful when builds can occur, so this is likely acceptable. Consider whether this should be guarded by!config->readOnlyfor consistency with other conditional directory operations.src/libutil/terminal.cc (1)
182-188: Minor: Use== 0instead of<= 0for unsigned type.Since
widthisunsigned int, the comparison<= 0is equivalent to== 0. Using== 0would be clearer and avoids potential compiler warnings about comparing unsigned values with zero in a less-than context.unsigned int getWindowWidth() { unsigned int width = getWindowSize().second; - if (width <= 0) + if (width == 0) width = std::numeric_limits<unsigned int>::max(); return width; }src/nix/ps.cc (1)
78-78: String concatenation in recursive call creates copies.Each recursive call creates new
std::stringobjects viaprefix + treeNullorprefix + treeLine. For deep process trees, consider usingstd::stringand+=to reduce allocations, though for typical process tree depths this is acceptable.src/libstore/local-store-active-builds.cc (1)
10-20: Potential out-of-bounds access when parsing /proc/PID/stat.The
.at(3)call will throw if/proc/PID/stathas fewer than 4 space-separated tokens. While this is unlikely for valid processes, a race condition could occur if the process exits between reading the cgroup and reading/proc/PID/stat.Consider adding bounds checking:
static ActiveBuildInfo::ProcessInfo getProcessInfo(pid_t pid) { + auto statTokens = tokenizeString<std::vector<std::string>>(readFile(fmt("/proc/%d/stat", pid))); return { .pid = pid, - .parentPid = - string2Int<pid_t>(tokenizeString<std::vector<std::string>>(readFile(fmt("/proc/%d/stat", pid))).at(3)) - .value_or(0), + .parentPid = statTokens.size() > 3 ? string2Int<pid_t>(statTokens[3]).value_or(0) : 0, .argv = tokenizeString<std::vector<std::string>>(readFile(fmt("/proc/%d/cmdline", pid)), std::string("\000", 1)), }; }src/libstore/include/nix/store/active-builds.hh (2)
36-66: Consider adding virtual destructor to TrackActiveBuildsStore.Since
TrackActiveBuildsStorehas virtual methods and is used polymorphically (e.g., viadynamic_castin derivation-builder.cc), it should have a virtual destructor to ensure proper cleanup when deleted through a base pointer.struct TrackActiveBuildsStore { + virtual ~TrackActiveBuildsStore() = default; + struct BuildHandle {
68-73: Consider adding virtual destructor to QueryActiveBuildsStore.For consistency with
TrackActiveBuildsStoreand to support polymorphic usage:struct QueryActiveBuildsStore { + virtual ~QueryActiveBuildsStore() = default; + inline static std::string operationName = "Querying active builds";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/libmain/progress-bar.cc(1 hunks)src/libstore/daemon.cc(2 hunks)src/libstore/include/nix/store/active-builds.hh(1 hunks)src/libstore/include/nix/store/local-store.hh(3 hunks)src/libstore/include/nix/store/meson.build(1 hunks)src/libstore/include/nix/store/remote-store.hh(3 hunks)src/libstore/include/nix/store/worker-protocol.hh(1 hunks)src/libstore/local-store-active-builds.cc(1 hunks)src/libstore/local-store.cc(2 hunks)src/libstore/meson.build(1 hunks)src/libstore/remote-store.cc(1 hunks)src/libstore/unix/build/derivation-builder.cc(6 hunks)src/libstore/unix/build/linux-derivation-builder.cc(1 hunks)src/libutil/include/nix/util/file-system.hh(1 hunks)src/libutil/include/nix/util/terminal.hh(1 hunks)src/libutil/linux/cgroup.cc(1 hunks)src/libutil/linux/include/nix/util/cgroup.hh(2 hunks)src/libutil/terminal.cc(1 hunks)src/nix/meson.build(1 hunks)src/nix/ps.cc(1 hunks)src/nix/ps.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
src/libstore/unix/build/linux-derivation-builder.cc (2)
src/libstore/include/nix/store/active-builds.hh (1)
build(63-63)src/libstore/unix/build/derivation-builder.cc (2)
getActiveBuild(874-884)getActiveBuild(874-874)
src/libutil/terminal.cc (1)
src/libutil/include/nix/util/terminal.hh (1)
getWindowSize(37-37)
src/libstore/remote-store.cc (5)
src/libstore/local-store-active-builds.cc (2)
queryActiveBuilds(23-60)queryActiveBuilds(23-23)src/libstore/include/nix/store/remote-store.hh (2)
conn(170-170)conn(174-174)src/libstore/uds-remote-store.cc (1)
conn(100-100)src/libstore/store-reference.cc (2)
parse(70-171)parse(70-70)src/libstore/realisation.cc (2)
parse(12-22)parse(12-12)
src/nix/ps.cc (3)
src/libstore/include/nix/store/active-builds.hh (1)
build(63-63)src/libstore/include/nix/store/local-store.hh (1)
build(479-479)src/libstore/unix/build/linux-derivation-builder.cc (1)
build(734-739)
src/libmain/progress-bar.cc (2)
src/libutil/include/nix/util/terminal.hh (2)
filterANSIEscapes(21-22)getWindowWidth(42-42)src/libutil/terminal.cc (4)
filterANSIEscapes(75-150)filterANSIEscapes(75-75)getWindowWidth(182-188)getWindowWidth(182-182)
src/libstore/local-store-active-builds.cc (2)
src/libutil/linux/cgroup.cc (2)
getPidsInCgroup(170-187)getPidsInCgroup(170-170)src/libstore/include/nix/store/active-builds.hh (4)
build(63-63)BuildHandle(43-47)BuildHandle(49-54)BuildHandle(56-60)
src/libstore/local-store.cc (2)
src/libutil/include/nix/util/file-system.hh (1)
createDirs(215-215)src/libutil/file-system.cc (2)
createDirs(568-575)createDirs(568-568)
src/libutil/linux/cgroup.cc (2)
src/libutil/posix-source-accessor.cc (4)
pathExists(82-87)pathExists(82-82)readFile(41-80)readFile(41-41)src/libfetchers/filtering-source-accessor.cc (6)
pathExists(26-29)pathExists(26-26)readFile(14-18)readFile(14-14)readFile(20-24)readFile(20-20)
src/libstore/include/nix/store/local-store.hh (1)
src/libstore/include/nix/store/active-builds.hh (2)
build(63-63)handle(65-65)
src/libstore/include/nix/store/active-builds.hh (3)
src/libstore/unix/build/derivation-builder.cc (2)
id(205-208)id(205-205)src/libstore/include/nix/store/local-store.hh (2)
build(479-479)handle(481-481)src/libstore/unix/build/linux-derivation-builder.cc (1)
build(734-739)
🪛 Clang (14.0.6)
src/nix/ps.cc
[error] 1-1: 'nix/cmd/command.hh' file not found
(clang-diagnostic-error)
src/libstore/local-store-active-builds.cc
[error] 1-1: 'nix/store/local-store.hh' file not found
(clang-diagnostic-error)
src/libstore/include/nix/store/active-builds.hh
[error] 3-3: 'nix/store/path.hh' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (30)
src/nix/meson.build (1)
99-99:ps.ccis correctly wired into the CLI buildAdding
ps.cctonix_sourceshere is consistent with the other subcommands and should be sufficient to buildnix psas part of the main binary.src/libmain/progress-bar.cc (1)
473-473: Use ofgetWindowWidth()keeps behavior while centralizing TTY width logicSwitching to
filterANSIEscapes(line, false, getWindowWidth())preserves the previous “use terminal width or fall back to max” behavior, but now relies on a single shared helper, which improves consistency across callers.If you haven’t already, please confirm that
getWindowWidth()mirrors the oldgetWindowSize().second/ fallback behavior in non‑TTY and “width unknown” cases by exercising the progress bar in both TTY and non‑TTY environments.src/libstore/include/nix/store/meson.build (1)
13-13: Publicly exportingactive-builds.hhis appropriateAdding
active-builds.hhto the installed headers list is the right way to expose the active-build tracking API to clients linking againstnixstore.src/libstore/meson.build (1)
341-341:local-store-active-builds.cccorrectly added to libnixstore sourcesIncluding
local-store-active-builds.ccin thesourceslist ensures the active-build tracking implementation is part of thenixstorelibrary and available to callers.src/libstore/include/nix/store/worker-protocol.hh (1)
189-189: NewQueryActiveBuildsopcode looks fine; ensure it’s gated by feature/version negotiationAdding
QueryActiveBuilds = 48toWorkerProto::Opis consistent with the existing numbering and avoids changingPROTOCOL_VERSION, as suggested by the comment above. The only requirement is that clients must only send this opcode when the remote daemon has advertised support (via a feature flag and/or minimum version), otherwise older daemons will reject the request.Please double‑check that:
- The daemon side advertises a feature (or equivalent) for active‑build queries, and
RemoteStore::queryActiveBuilds()(and any other callers) check that feature or version before issuingQueryActiveBuilds.src/libstore/unix/build/linux-derivation-builder.cc (1)
734-739: Active build now correctly carries the cgroup without breaking teardownOverriding
getActiveBuild()to callDerivationBuilderImpl::getActiveBuild()and then setbuild.cgroup = cgroup;cleanly propagates the per-build cgroup intoActiveBuildwhile leaving the membercgroupintact for later use inkillSandbox(). This matches the lifecycle expectations set up inprepareUser()andkillSandbox().Please confirm that
getActiveBuild()is only called afterstartChild()andprepareUser()have run, so that bothpidandcgroupare initialized when the active build is recorded.src/libutil/linux/cgroup.cc (1)
170-187: LGTM!The implementation is consistent with the existing PID parsing pattern in
destroyCgroup(lines 82-95). The early return for non-existent cgroup paths and the use ofstring2Intwith proper error handling for invalid PIDs follows established conventions in this file.src/nix/ps.md (1)
1-24: LGTM!The documentation is clear and the example effectively demonstrates the process tree visualization. The platform-dependent note on line 22 accurately reflects that full command line information relies on Linux-specific
/procreading.src/libstore/local-store.cc (1)
128-128: LGTM!The member initialization follows the established pattern used for other directory paths like
tempRootsDir.src/libstore/remote-store.cc (1)
766-772: LGTM!The implementation follows the established pattern for remote store operations, consistent with other methods like
queryAllValidPaths. The JSON parsing integrates with theActiveBuildInfoserialization defined inactive-builds.hh.src/libutil/include/nix/util/terminal.hh (1)
39-42: LGTM!Clean declaration with appropriate Doxygen documentation that accurately describes the return value behavior.
src/libstore/include/nix/store/remote-store.hh (3)
10-10: LGTM!Necessary include for the new
QueryActiveBuildsStoreinterface andActiveBuildInfotype.
40-43: LGTM!The inheritance from
QueryActiveBuildsStorefollows the established virtual inheritance pattern used forGcStoreandLogStore.
150-151: LGTM!Method declaration correctly overrides the interface from
QueryActiveBuildsStore.src/libstore/include/nix/store/local-store.hh (3)
9-9: LGTM!Necessary include for the new active builds interfaces and types.
129-132: LGTM!The dual inheritance from
TrackActiveBuildsStore(for lifecycle management viabuildStarted/buildFinished) andQueryActiveBuildsStore(for querying viaqueryActiveBuilds) provides a clean separation of concerns while following the established virtual inheritance pattern.
464-481: LGTM!The private state management for active builds is well-designed:
ActiveBuildFileuses RAII (AutoCloseFDfor lock,AutoDeletefor cleanup)Sync<std::unordered_map<...>>provides thread-safe access to the in-memory build tracking- Method declarations correctly override the interface methods from the base classes
src/libstore/unix/build/derivation-builder.cc (5)
81-84: LGTM! Build handle tracking member added correctly.The
std::optional<BuildHandle>is appropriate for tracking the active build state, and the documentation comment clearly explains its purpose.
485-486: Build handle reset on process termination looks correct.The handle is reset after
pid.wait()completes, ensuring the build is marked finished after the child process has fully terminated.
520-521: Appropriate cleanup of build tracking in unprepareBuild.The handle reset occurs after
killSandbox(true)which ensures process cleanup before deregistering the build.
865-868: Build visibility registration implemented correctly.The
dynamic_castcheck safely determines if the store supports active build tracking before registering. Usingemplaceto construct the handle in-place is appropriate.
874-884: ActiveBuild construction looks good, with noted TODOs.The FIXME comments for
clientPidandclientUidare acknowledged in the PR as future work. The remaining fields are correctly populated from the build context.src/libstore/daemon.cc (1)
1018-1025: QueryActiveBuilds operation follows daemon conventions correctly.The implementation properly uses
require<>for store interface access, wraps work withstartWork/stopWork, and serializes the result as JSON consistent with the RemoteStore client implementation.src/nix/ps.cc (2)
29-38: LGTM! Clean command structure and early return for empty builds.The
require<QueryActiveBuildsStore>pattern and early exit with informative message for no active builds follows good UX practices.
64-64: C++23 explicit object parameter is properly supported by the build configuration.The codebase explicitly requires C++23 via
cpp_std=c++23insrc/nix/meson.build(line 6), whereps.ccis compiled (line 99). The syntax[&](this const auto & visit, ...)is valid C++23 and will be compiled with the correct standard.src/libstore/local-store-active-builds.cc (2)
62-87: Build tracking registration looks correct.The use of
static std::atomic<uint64_t>combined withgetpid()in the filename provides sufficient uniqueness. The lock is acquired before writing, and theActiveBuildFilestruct properly manages the file descriptor and auto-deletion.
94-139: JSON serialization implementations are well-structured.The
adl_serializerspecializations correctly handle all fields including theStorePathconversion. UsinggetString()for parsing and.to_string()for serialization maintains consistency.src/libstore/include/nix/store/active-builds.hh (3)
49-54: BuildHandle move constructor doesn't nullify tracker reference concern.The move constructor sets
other.id = 0to prevent double-finish, which is correct. However,BuildHandlelacks a move assignment operator. If move assignment is attempted, the compiler-deleted version will cause a compile error, which is acceptable if intentional.
10-22: ActiveBuild struct is well-designed for the use case.Fields capture all relevant build metadata. Using
std::optionalfor client-related fields and cgroup appropriately handles cases where they're unavailable.
24-34: ActiveBuildInfo properly extends ActiveBuild with process details.The nested
ProcessInfostruct andprocessesvector cleanly separate the static build info from the dynamic process tree data gathered at query time.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/libstore/local-store-active-builds.cc (1)
35-37: Fix the stale file cleanup logic.As noted in the previous review,
AutoDelete(path, false)creates a temporary object that is immediately destroyed without deleting the file. Stale build files will accumulate in the active-builds directory.Apply the fix suggested in the previous review:
auto fd = openLockFile(path, false); if (!fd || lockFile(fd.get(), ltRead, false)) { - AutoDelete(path, false); + std::filesystem::remove(path); continue; }
🧹 Nitpick comments (1)
src/libstore/local-store-active-builds.cc (1)
12-22: Consider safer access for /proc/stat parsing.Line 17 uses
.at(3)which will throwout_of_rangeif the/proc/<pid>/statfile has an unexpected format or fewer than 4 fields. While the caller handles exceptions, using bounds checking or a safer accessor would be more defensive.Apply this diff to add bounds checking:
- .parentPid = - string2Int<pid_t>(tokenizeString<std::vector<std::string>>(readFile(fmt("/proc/%d/stat", pid))).at(3)) - .value_or(0), + .parentPid = [&]() { + auto fields = tokenizeString<std::vector<std::string>>(readFile(fmt("/proc/%d/stat", pid))); + return fields.size() > 3 ? string2Int<pid_t>(fields[3]).value_or(0) : 0; + }(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/libstore/local-store-active-builds.cc(1 hunks)src/nix/ps.cc(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/nix/ps.cc (4)
src/libstore/include/nix/store/worker-protocol.hh (2)
store(123-123)store(124-124)src/libstore/include/nix/store/active-builds.hh (1)
build(63-63)src/libstore/include/nix/store/local-store.hh (1)
build(479-479)src/libstore/unix/build/linux-derivation-builder.cc (1)
build(734-739)
🪛 Clang (14.0.6)
src/libstore/local-store-active-builds.cc
[error] 1-1: 'nix/store/local-store.hh' file not found
(clang-diagnostic-error)
src/nix/ps.cc
[error] 1-1: 'nix/cmd/command.hh' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (7)
src/libstore/local-store-active-builds.cc (1)
81-86: Perfect! I now have complete clarity on the issue. The review comment is based on a misunderstanding of the AutoDelete class semantics. Let me create the rewritten review:
The review comment is based on an incorrect understanding of AutoDelete semantics.
The
falseparameter inAutoDelete(infoFilePath, false)is therecursiveflag (defaulttrue), not a "don't delete" flag. The AutoDelete constructor (insrc/libutil/file-system.cc:603-608) always setsdel = trueregardless of therecursiveparameter. In the destructor (lines 610-624), the file is deleted via eitherdeletePath(_path)(if recursive) orstd::filesystem::remove(_path)(if non-recursive), both controlled by thedelflag being true.The cleanup flow works correctly:
- When
buildFinishederases the map entry, theActiveBuildFileis destroyed, triggering theAutoDeletedestructor, which callsstd::filesystem::remove(infoFilePath).- In
queryActiveBuilds, stale files (those with failed lock acquisition) are also deleted via the temporaryAutoDelete(path, false)at line 36, whose destructor runs immediately.No resource leak exists, and no changes are needed to the code.
Likely an incorrect or invalid review comment.
src/nix/ps.cc (6)
29-38: LGTM!The query logic correctly obtains the active builds interface and handles the empty case appropriately.
85-85: LGTM!Command registration follows the standard Nix pattern.
55-55: Project is configured with C++23 standard; .contains() usage is valid.The project uses Meson as its build system and explicitly configures
cpp_std=c++23in allmeson.buildfiles, includingsrc/nix/meson.build(line 6). Since C++23 is newer than C++20, the.contains()method onstd::setused at line 55 is fully supported and requires no changes.
63-79: C++23 explicit object parameter support is confirmed and correctly configured.The project's build configuration in
src/nix/meson.buildexplicitly specifies'cpp_std=c++23'(line 5 of the Meson project declaration), which enables the C++23 feature used in the lambda on line 63. The deducing-this syntax (this const auto & visit) is valid and properly supported. All utility functions (enumerate,filterANSIEscapes,tokenizeString,concatStringsSep) are available and correctly used in the recursive tree-rendering logic.
44-44: Tree connector constants are properly defined and accessible.The constants
treeLast,treeConn,treeNull, andtreeLineare defined insrc/libutil/include/nix/util/util.hh(lines 177-180) asconstexpr chararrays containing box-drawing characters. They are available in a standard utility header that othersrc/nix/files access using the include pattern#include "nix/util/util.hh".
66-74: All utility functions are properly available and included.The code uses four utility functions that are all correctly declared and accessible:
filterANSIEscapes– Declared innix/util/terminal.hh, which is directly included in ps.cctokenizeString– Declared innix/util/strings.hh, available through precompiled headers (libcmd/pch/precompiled-headers.hh)concatStringsSep– Declared innix/util/strings.hh, available through precompiled headersenumerate– Defined innix/util/util.hh(which includesstrings.hh), available through precompiled headersThe C++ build system uses precompiled headers that automatically include
nix/util/util.hhfor all compilation units in libcmd, ensuring these utility functions are accessible throughout the codebase including ps.cc.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/libstore/local-store-active-builds.cc (1)
35-37: AutoDelete temporary issue already flagged in a previous review.This has been addressed in a past review comment — the
AutoDelete(path, false)temporary is immediately destroyed and has no effect.
🧹 Nitpick comments (1)
src/libstore/local-store-active-builds.cc (1)
50-52: Broad exception swallowing could hide unexpected errors.While
ignoreExceptionExceptInterrupt()is intentional for robustness when reading transient/procdata, consider whether logging at debug level would help diagnose issues during development without affecting production resilience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/libstore/active-builds.cc(1 hunks)src/libstore/local-store-active-builds.cc(1 hunks)src/libstore/meson.build(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libstore/meson.build
🧰 Additional context used
🪛 Clang (14.0.6)
src/libstore/active-builds.cc
[error] 1-1: 'nix/store/active-builds.hh' file not found
(clang-diagnostic-error)
src/libstore/local-store-active-builds.cc
[error] 1-1: 'nix/store/local-store.hh' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (5)
src/libstore/active-builds.cc (3)
1-10: LGTM!The JSON serialization setup is correct. Using
NLOHMANN_DEFINE_TYPE_NON_INTRUSIVEforProcessInfoinside thenixnamespace is the right approach for simple structs.Note: The static analysis error about the missing header is a false positive since
nix/store/active-builds.hhis introduced in this PR.
16-27: LGTM on the ActiveBuild deserialization.Field extraction and StorePath construction from the JSON string are handled correctly.
42-53: LGTM on the ActiveBuildInfo serialization.Correctly reuses the base
ActiveBuildserialization and extends it with theprocessesvector.src/libstore/local-store-active-builds.cc (2)
25-62: Overall approach to queryActiveBuilds looks reasonable.The lock-based detection of active builds (if lockable → stale, otherwise → active) and the Linux-specific process enumeration via cgroups or direct PID are appropriate patterns for this use case.
64-89: The review comment is incorrect — build info files are properly cleaned up.The second parameter to
AutoDeleteisrecursive, not a "deletion enabled" flag. TheAutoDeleteconstructor (line 603 insrc/libutil/file-system.cc) unconditionally setsdel = true, enabling automatic deletion. WhenbuildFinished()erases the entry from theactiveBuildsmap, theAutoDeletedestructor is invoked and callsstd::filesystem::remove(_path)to delete the file. No stale files accumulate.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/libstore/remote-store.cc (1)
766-774: Feature‑gatedQueryActiveBuildslooks correct; consider fallback behavior for unsupported daemonsThe implementation is consistent with other remote operations:
- It uses
getConnection()and the negotiatedconn->featuresset to guard the new op.- The feature check ensures we never send
Op::QueryActiveBuildsto daemons that didn’t advertisefeatureQueryActiveBuilds, covering both pre‑feature and pre‑feature‑negotiation protocol versions.- Using
nlohmann::json::parse(...).get<std::vector<ActiveBuildInfo>>()matches the JSON‑based representation introduced forActiveBuildInfo.One behavioral question: currently, stores that don’t support
featureQueryActiveBuildswill cause this to throw:if (!conn->features.count(WorkerProto::featureQueryActiveBuilds)) throw Error("remote store does not support querying active builds");Depending on how
nix psand other callers handle this, it might be preferable to degrade gracefully (e.g., return an empty vector and optionally log a debug/warn message) instead of failing the whole operation when any configuredRemoteStorelacks this feature. If you want that behavior, something along these lines would work:std::vector<ActiveBuildInfo> RemoteStore::queryActiveBuilds() { auto conn(getConnection()); - if (!conn->features.count(WorkerProto::featureQueryActiveBuilds)) - throw Error("remote store does not support querying active builds"); + if (!conn->features.count(WorkerProto::featureQueryActiveBuilds)) { + debug("remote store '%s' does not support querying active builds", config.getHumanReadableURI()); + return {}; + } conn->to << WorkerProto::Op::QueryActiveBuilds; conn.processStderr(); return nlohmann::json::parse(readString(conn->from)).get<std::vector<ActiveBuildInfo>>(); }If the CLI already expects and handles this
Error, keeping the current behavior is fine; otherwise, a graceful fallback would likely improve UX for mixed or older daemon setups.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/libstore/include/nix/store/worker-protocol.hh(2 hunks)src/libstore/remote-store.cc(1 hunks)src/libstore/worker-protocol-connection.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libstore/include/nix/store/worker-protocol.hh
🧰 Additional context used
🧬 Code graph analysis (1)
src/libstore/remote-store.cc (4)
src/libstore/local-store-active-builds.cc (2)
queryActiveBuilds(25-62)queryActiveBuilds(25-25)src/libstore/include/nix/store/remote-store.hh (2)
conn(170-170)conn(174-174)src/libstore/uds-remote-store.cc (1)
conn(100-100)src/libstore/ssh-store.cc (2)
conn(80-80)conn(181-181)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (2)
src/libstore/worker-protocol-connection.cc (1)
8-8: AdvertiseQueryActiveBuildsin the negotiated feature setInitializing
WorkerProto::allFeatureswithfeatureQueryActiveBuildscorrectly exposes this capability during handshake and lets the daemon negotiate it via the existing feature-exchange logic. This is consistent with howsupportedFeaturesis passed inBasicClientConnection::handshake.src/libstore/remote-store.cc (1)
22-22: New JSON dependency include for active-build queryingIncluding
<nlohmann/json.hpp>here is appropriate given the JSON parsing inqueryActiveBuilds()below. Please just double-check that the build system for this target already links innlohmann_jsonso this compiles in all supported configurations (including non-default ones).
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/libstore/active-builds.cc(1 hunks)src/libstore/include/nix/store/active-builds.hh(1 hunks)src/libstore/unix/build/derivation-builder.cc(6 hunks)src/nix/ps.cc(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/nix/ps.cc (2)
src/libstore/include/nix/store/active-builds.hh (1)
build(65-65)src/libstore/include/nix/store/local-store.hh (1)
build(479-479)
src/libstore/include/nix/store/active-builds.hh (2)
src/libstore/include/nix/store/local-store.hh (2)
build(479-479)handle(481-481)src/libstore/unix/build/linux-derivation-builder.cc (1)
build(734-739)
src/libstore/active-builds.cc (1)
src/libstore/include/nix/store/active-builds.hh (1)
build(65-65)
🪛 Clang (14.0.6)
src/nix/ps.cc
[error] 1-1: 'nix/cmd/command.hh' file not found
(clang-diagnostic-error)
src/libstore/include/nix/store/active-builds.hh
[error] 3-3: 'nix/store/path.hh' file not found
(clang-diagnostic-error)
src/libstore/active-builds.cc
[error] 1-1: 'nix/store/active-builds.hh' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (7)
src/libstore/unix/build/derivation-builder.cc (3)
81-84: LGTM!The optional
BuildHandlemember is well-documented and follows RAII patterns for lifecycle tracking.
865-867: LGTM!The active build tracking initialization correctly uses dynamic casting to detect feature support and RAII to manage the build lifecycle.
874-885: LGTM!The
getActiveBuild()implementation correctly populates the active build metadata. The FIXME comments forclientPidandclientUidare acceptable for this feature introduction.src/nix/ps.cc (2)
29-38: LGTM!The query logic cleanly handles the empty state with appropriate user feedback.
40-62: LGTM!The build summary formatting and process tree reconstruction logic are both well-implemented. The parent-child mapping correctly identifies root processes when their parents aren't in the build's process set.
src/libstore/active-builds.cc (1)
1-57: LGTM!The JSON serialization implementation is clean and follows nlohmann::json best practices. The delegation pattern in
ActiveBuildInfoserialization nicely reuses theActiveBuildserialization logic.src/libstore/include/nix/store/active-builds.hh (1)
10-36: LGTM!The
ActiveBuildandActiveBuildInfotype hierarchy is well-designed, with clear separation between core build metadata and optional process information.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/libstore/include/nix/store/active-builds.hh (1)
63-67: Destructor must not propagate exceptions.This was flagged in a previous review. The
BuildHandledestructor callsbuildFinished(), which could throw. If this destructor is invoked during stack unwinding, a thrown exception will callstd::terminate().src/libutil/linux/include/nix/util/cgroup.hh (1)
43-46: Add#include <set>to make the header self-contained.The
getPidsInCgroupfunction returnsstd::set<pid_t>, but the header does not directly include<set>. While it may compile due to transitive inclusion, the header should be self-contained.src/nix/ps.cc (1)
70-70: C++23 explicit object parameter syntax:constshould come afterauto.Per C++23 P0847 (Deducing this), the correct syntax is
this auto const &, notthis const auto &.src/libstore/local-store-active-builds.cc (1)
13-42: Fix fragile/proc/[pid]/statparsing and adjust CPU time field indicesThe current parsing of
/proc/[pid]/statassumes that splitting on spaces yields the documented fields, which breaks when thecommfield contains spaces or). In that case the parent PID and CPU times will be read from the wrong positions, giving incorrect trees and CPU stats innix ps.You also index
statFields[13]/statFields[14]forutime/stimebased on the full-field layout; once you parse robustly, those indices need to change.Consider something along these lines instead:
- // Read /proc/[pid]/stat for parent PID and CPU times - auto statFields = tokenizeString<std::vector<std::string>>(readFile(fmt("/proc/%d/stat", pid))); - - // Field 3 (index 3) is ppid - if (statFields.size() > 3) - info.parentPid = string2Int<pid_t>(statFields[3]).value_or(0); - - // Fields 13 and 14 (indices 13, 14) are utime and stime in clock ticks - static long clkTck = sysconf(_SC_CLK_TCK); - if (statFields.size() > 14 && clkTck > 0) { - auto utime = string2Int<uint64_t>(statFields[13]); - auto stime = string2Int<uint64_t>(statFields[14]); - - if (utime) { - // Convert clock ticks to microseconds: (ticks / clkTck) * 1000000 - info.cpuUser = std::chrono::microseconds((*utime * 1'000'000) / clkTck); - } - if (stime) { - info.cpuSystem = std::chrono::microseconds((*stime * 1'000'000) / clkTck); - } - } + // Read /proc/[pid]/stat for parent PID and CPU times. + // The comm field is wrapped in parentheses and may contain spaces, so we + // must skip past the final ')' before tokenising. + auto statContent = readFile(fmt("/proc/%d/stat", pid)); + auto rparen = statContent.rfind(')'); + if (rparen != std::string::npos && rparen + 2 < statContent.size()) { + // Drop "<pid> (comm) " and start at the state field. + auto rest = statContent.substr(rparen + 2); + auto fields = tokenizeString<std::vector<std::string>>(rest); + + // fields[0] is state, fields[1] is ppid. + if (fields.size() > 1) + info.parentPid = string2Int<pid_t>(fields[1]).value_or(0); + + // utime and stime are fields 14 and 15 in /proc/[pid]/stat, which are + // indices 11 and 12 in 'fields' after skipping pid and comm. + static long clkTck = sysconf(_SC_CLK_TCK); + if (fields.size() > 12 && clkTck > 0) { + auto utime = string2Int<uint64_t>(fields[11]); + auto stime = string2Int<uint64_t>(fields[12]); + + if (utime) { + // Convert clock ticks to microseconds: (ticks / clkTck) * 1000000 + info.cpuUser = std::chrono::microseconds((*utime * 1'000'000) / clkTck); + } + if (stime) { + info.cpuSystem = std::chrono::microseconds((*stime * 1'000'000) / clkTck); + } + } + }You might also optionally guard against overflow in
(*utime * 1'000'000)by dividing byclkTckfirst for very large tick counts, but that’s a minor robustness concern compared to the parsing/field-indexing bug.
🧹 Nitpick comments (8)
src/libstore/active-builds.cc (3)
21-26: Consider extracting the CPU time deserialization pattern to reduce duplication.The same pattern for deserializing optional CPU times from seconds (float) to microseconds appears twice: in
ProcessInfo::from_json(lines 22-26) andActiveBuildInfo::from_json(lines 85-89). This could be extracted into a helper function.+namespace { +std::optional<std::chrono::microseconds> deserializeCpuTime(const nlohmann::json & j, const char * key) +{ + if (j.contains(key) && !j.at(key).is_null()) + return std::chrono::microseconds(static_cast<int64_t>(j.at(key).get<double>() * 1'000'000)); + return std::nullopt; +} +} // anonymous namespaceThen use as:
info.cpuUser = deserializeCpuTime(j, "cpuUser"); info.cpuSystem = deserializeCpuTime(j, "cpuSystem");Also applies to: 84-89
31-49: CPU time serialization logic is also duplicated.Similarly, the
to_jsonpattern for optional CPU times is duplicated betweenProcessInfo::to_jsonandActiveBuildInfo::to_json. A helper could consolidate this.Also applies to: 94-109
6-8: Empty namespace body is unnecessary.The
nixnamespace block is empty. If no declarations are needed here, consider removing it to reduce noise.-namespace nix { - -} // namespace nix - namespace nlohmann {src/libstore/include/nix/store/active-builds.hh (2)
43-73: Consider adding a virtual destructor toTrackActiveBuildsStore.Since
TrackActiveBuildsStorehas virtual methods and is intended to be used polymorphically (viarequire<QueryActiveBuildsStore>), it should have a virtual destructor to ensure proper cleanup when deleting through a base pointer.struct TrackActiveBuildsStore { + virtual ~TrackActiveBuildsStore() = default; + struct BuildHandle {
75-80: Consider adding a virtual destructor toQueryActiveBuildsStore.Same reasoning as above - this interface is used polymorphically and should have a virtual destructor.
struct QueryActiveBuildsStore { + virtual ~QueryActiveBuildsStore() = default; + inline static std::string operationName = "Querying active builds";src/libutil/linux/include/nix/util/cgroup.hh (1)
24-24: Inconsistent parameter types:std::filesystem::pathvsPath.
getCgroupStatsandgetPidsInCgroupusestd::filesystem::path, whiledestroyCgroupusesPath(a string alias). Consider using a consistent type across all functions for uniformity.Also applies to: 32-32, 46-46
src/nix/ps.cc (1)
94-94: String concatenation in recursive call creates new strings on each level.The expression
prefix + treeNullorprefix + treeLinecreates a newstd::stringon each recursive call. For deeply nested process trees, this could be inefficient. Consider passingstd::stringby value and using move semantics, or using astd::stringthat grows in place.- [&](this const auto & visit, const Processes & processes, std::string_view prefix) -> void { + [&](this auto const & visit, const Processes & processes, std::string prefix) -> void { for (const auto & [n, process] : enumerate(processes)) { bool last = n + 1 == processes.size(); // ... formatting code ... - visit(children[process->pid], last ? prefix + treeNull : prefix + treeLine); + visit(children[process->pid], prefix + (last ? treeNull : treeLine)); } }(rootProcesses, "");This also fixes the C++23 syntax issue.
src/libstore/local-store-active-builds.cc (1)
56-59:AutoDelete(path, false)here is effectful but non-obvious; consider making deletion intent clearer
AutoDelete’s constructor takes arecursiveflag, not a “delete-on-destruction” flag, and default-initialisesdelso that the destructor deletespath. Creating a temporaryAutoDelete(path, false)therefore does deletepathimmediately (non‑recursively) at the end of the full expression, which is what you want for clearing stale entries.That said, this usage is easy to mis-read (as seen in earlier review comments) and looks like a no-op at a glance. For clarity, consider replacing it with an explicit helper such as
deletePath(path);or a small wrapper that self-documents the intent while preserving non-recursive semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/libstore/active-builds.cc(1 hunks)src/libstore/include/nix/store/active-builds.hh(1 hunks)src/libstore/local-store-active-builds.cc(1 hunks)src/libutil/linux/cgroup.cc(3 hunks)src/libutil/linux/include/nix/util/cgroup.hh(3 hunks)src/nix/ps.cc(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/libstore/local-store-active-builds.cc (3)
src/libstore/active-builds.cc (1)
info(81-81)src/libutil/linux/cgroup.cc (4)
getPidsInCgroup(177-194)getPidsInCgroup(177-177)getCgroupStats(52-77)getCgroupStats(52-52)src/libutil/linux/include/nix/util/cgroup.hh (2)
getPidsInCgroup(46-46)getCgroupStats(24-24)
src/libutil/linux/cgroup.cc (1)
src/libutil/linux/include/nix/util/cgroup.hh (1)
getCgroupStats(24-24)
src/libstore/active-builds.cc (1)
src/libstore/include/nix/store/active-builds.hh (1)
build(70-70)
src/libstore/include/nix/store/active-builds.hh (2)
src/libstore/include/nix/store/local-store.hh (2)
build(479-479)handle(481-481)src/libstore/unix/build/linux-derivation-builder.cc (1)
build(734-739)
src/nix/ps.cc (5)
src/libstore/include/nix/store/worker-protocol.hh (2)
store(123-123)store(124-124)src/libstore/include/nix/store/active-builds.hh (1)
build(70-70)src/libstore/include/nix/store/local-store.hh (1)
build(479-479)src/libstore/unix/build/linux-derivation-builder.cc (1)
build(734-739)src/libutil/include/nix/util/args.hh (1)
max(27-109)
🪛 Clang (14.0.6)
src/libstore/local-store-active-builds.cc
[error] 1-1: 'nix/store/local-store.hh' file not found
(clang-diagnostic-error)
src/libstore/active-builds.cc
[error] 1-1: 'nix/store/active-builds.hh' file not found
(clang-diagnostic-error)
src/libstore/include/nix/store/active-builds.hh
[error] 3-3: 'nix/store/path.hh' file not found
(clang-diagnostic-error)
src/nix/ps.cc
[error] 1-1: 'nix/cmd/command.hh' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (9)
src/libstore/include/nix/store/active-builds.hh (1)
56-61: Move constructor should be markednoexceptand use member initializer list correctly.The move constructor looks correct. The
other.id = 0ensures the moved-from handle won't callbuildFinishedon destruction.src/libutil/linux/include/nix/util/cgroup.hh (1)
21-24: Good documentation for the new function.The comment clearly describes the purpose of
getCgroupStats.src/libutil/linux/cgroup.cc (3)
52-77: Implementation correctly handles missing cpu.stat file.The function gracefully returns default-constructed stats when
cpu.statdoesn't exist. The parsing logic is straightforward and handles the expected format.
143-145: Good refactoring to reusegetCgroupStats.The delegation to
getCgroupStatsreduces code duplication and ensures consistent behavior.
177-194: Add explicit check forcgroup.procsfile or document the expected behavior.The function checks
pathExists(cgroup)but not for the specificcgroup.procsfile before callingreadFile. If the cgroup directory exists but the procs file doesn't,readFilewill throw. ThedestroyCgroupfunction (line 86) explicitly validates this withpathExists(procsFile)and throws a descriptive error if missing. For consistency, either add a similar check here or document that a missing procs file is treated as an error.src/nix/ps.cc (3)
29-38: Good use ofrequire<>for interface casting and clear empty state handling.The implementation correctly uses
require<QueryActiveBuildsStore>to obtain the tracker interface and provides user-friendly feedback when no builds are active.
54-66: Process tree reconstruction logic is correct.The algorithm correctly identifies root processes (those whose parent PID is not in the cgroup) and builds a parent-child map for tree rendering.
87-90: NestedconcatStringsSepandtokenizeStringfor whitespace normalization.The approach of tokenizing and rejoining to normalize whitespace is functional but somewhat expensive. This is likely acceptable for the
pscommand's use case.src/libstore/local-store-active-builds.cc (1)
90-120: Active-build file lifecycle and locking look consistentThe
buildStarted/buildFinishedpair cleanly models build lifecycle:
- Per-build file name is unique per process (
getpid()+nextId).- You take a write lock when marking a build active, and
queryActiveBuilds()treats an unlockable file as stale.- The
ActiveBuildFileentry keeps the fd and anAutoDeleteso thatbuildFinished()simply erases the map entry to both unlock and remove the file.Assuming
openLockFile/lockFilebehave as in the rest of the store code, this is a good fit for thenix psuse case.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/libutil/table.cc (1)
18-24: Handle ANSI escape sequences consistently in width and padding calculationsColumn widths are computed from
j->content.size(), but the final line is passed throughfilterANSIEscapes, so cells containing ANSI escapes end up visually narrower than their computed width and columns can misalign. If you switch to using the ANSI‑stripped length for widths, you also need to base padding on the stripped length to avoidsize_tunderflow when the raw string (with escapes) is longer than the visible width.Apply a change along these lines so both the width pass and the padding pass use the visible (ANSI‑stripped) length:
- for (auto & i : table) { - assert(i.size() == nrColumns); - size_t column = 0; - for (auto j = i.begin(); j != i.end(); ++j, ++column) - // TODO: take ANSI escapes into account when calculating width. - widths[column] = std::max(widths[column], j->content.size()); - } + for (auto & i : table) { + assert(i.size() == nrColumns); + size_t column = 0; + for (auto j = i.begin(); j != i.end(); ++j, ++column) { + auto visible = filterANSIEscapes(j->content, true); + widths[column] = std::max(widths[column], visible.size()); + } + } @@ - for (auto j = i.begin(); j != i.end(); ++j, ++column) { - std::string s = j->content; - replace(s.begin(), s.end(), '\n', ' '); - - auto padding = std::string(widths[column] - s.size(), ' '); + for (auto j = i.begin(); j != i.end(); ++j, ++column) { + std::string s = j->content; + replace(s.begin(), s.end(), '\n', ' '); + + auto visible = filterANSIEscapes(s, true); + auto padding = std::string(widths[column] - visible.size(), ' ');This keeps ANSI codes in the rendered content while aligning columns based on the visible character width.
Also applies to: 33-41, 46-47
src/libstore/local-store-active-builds.cc (1)
41-66: Fix/proc/[pid]/statparsing for command names containing)The current regex-based parsing assumes the first
)ends the(comm)field:auto statContent = trim(readFile(statFd.get())); static std::regex statRegex(R"((\d+) \(([^)]*)\) (.*))"); std::smatch match; if (!std::regex_match(statContent, match, statRegex)) throw Error("failed to parse /proc/%d/stat", pid); // Parse the remaining fields after (comm). auto remainingFields = tokenizeString<std::vector<std::string>>(match[3].str());This breaks when
commitself contains a), somatch[3]no longer starts at the true “state ppid …” segment and the field indices (includingppid,utime,stime,cutime,cstime) become misaligned.The
/proc/[pid]/statformat guarantees thatcommis enclosed in parentheses but may contain spaces and parentheses. A robust approach is to locate the last)and tokenize the substring after it:- auto statContent = trim(readFile(statFd.get())); - static std::regex statRegex(R"((\d+) \(([^)]*)\) (.*))"); - std::smatch match; - if (!std::regex_match(statContent, match, statRegex)) - throw Error("failed to parse /proc/%d/stat", pid); - - // Parse the remaining fields after (comm). - auto remainingFields = tokenizeString<std::vector<std::string>>(match[3].str()); + auto statContent = trim(readFile(statFd.get())); + + // Find the last ')' to skip past the comm field, which may contain spaces + // and parentheses. + auto lastParen = statContent.rfind(')'); + if (lastParen == std::string::npos) + throw Error("failed to parse /proc/%d/stat", pid); + + // Parse the remaining fields after the closing ')'. + auto remainingFields = + tokenizeString<std::vector<std::string>>(statContent.substr(lastParen + 1));The existing indices (
remainingFields[1]forppid,[11]..[14]forutime..cstime) stay valid with this change.
🧹 Nitpick comments (1)
src/nix/ps.cc (1)
90-137: Simplify argv formatting and consider deterministic process orderingInside the process-tree rendering, argv is currently rebuilt via an unnecessary join‑split‑join round‑trip:
auto argv = concatStringsSep( " ", tokenizeString<std::vector<std::string>>(concatStringsSep(" ", process->argv)));This:
- Allocates and concatenates the arguments into a single string.
- Immediately re-tokenizes on spaces (losing the original argument boundaries for values containing spaces).
- Then concatenates again just to produce another space‑separated string.
You can produce the same displayed command line more simply and without extra work:
- auto argv = concatStringsSep( - " ", tokenizeString<std::vector<std::string>>(concatStringsSep(" ", process->argv))); + auto argv = concatStringsSep(" ", process->argv);Also,
Processesis astd::set<const ActiveBuildInfo::ProcessInfo *>, so children in the tree are ordered by pointer value rather than PID or start order. If you care about stable, human‑friendly ordering (e.g. bypid), consider using a custom comparator or buildingchildrenasstd::map<pid_t, std::set<const ProcessInfo *, ByPid>>.Also applies to: 125-127
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/libstore/active-builds.cc(1 hunks)src/libstore/local-store-active-builds.cc(1 hunks)src/libutil/include/nix/util/table.hh(1 hunks)src/libutil/table.cc(1 hunks)src/nix/nix-env/nix-env.cc(3 hunks)src/nix/ps.cc(1 hunks)src/nix/ps.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nix/ps.md
🧰 Additional context used
🧬 Code graph analysis (4)
src/libstore/local-store-active-builds.cc (3)
src/libutil/linux/include/nix/util/cgroup.hh (2)
getPidsInCgroup(46-46)getCgroupStats(24-24)src/libutil/linux/cgroup.cc (4)
getPidsInCgroup(177-194)getPidsInCgroup(177-177)getCgroupStats(52-77)getCgroupStats(52-52)src/libstore/include/nix/store/active-builds.hh (4)
build(91-91)BuildHandle(66-70)BuildHandle(72-77)BuildHandle(79-88)
src/nix/ps.cc (4)
src/libstore/include/nix/store/active-builds.hh (1)
build(91-91)src/libstore/include/nix/store/local-store.hh (1)
build(479-479)src/libstore/unix/build/linux-derivation-builder.cc (1)
build(734-739)src/libutil/include/nix/util/table.hh (1)
printTable(25-25)
src/libstore/active-builds.cc (2)
src/libstore/include/nix/store/active-builds.hh (2)
uid(23-23)build(91-91)src/libutil/include/nix/util/serialise.hh (1)
buf(332-341)
src/nix/nix-env/nix-env.cc (2)
src/libutil/table.cc (2)
printTable(11-49)printTable(11-11)src/libutil/include/nix/util/table.hh (1)
printTable(25-25)
🪛 Clang (14.0.6)
src/libstore/local-store-active-builds.cc
[error] 1-1: 'nix/store/local-store.hh' file not found
(clang-diagnostic-error)
src/nix/ps.cc
[error] 1-1: 'nix/cmd/command.hh' file not found
(clang-diagnostic-error)
src/libstore/active-builds.cc
[error] 1-1: 'nix/store/active-builds.hh' file not found
(clang-diagnostic-error)
src/libutil/include/nix/util/table.hh
[error] 3-3: 'nix/util/types.hh' file not found
(clang-diagnostic-error)
src/libutil/table.cc
[error] 1-1: 'nix/util/table.hh' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (5)
src/nix/nix-env/nix-env.cc (1)
19-20: nix-env’s adoption of Table/TableRow and printTable looks correctThe new include, use of
Table/TableRowinopQuery, and the finalprintTable(std::cout, table);call all line up with the new table API. The implicit construction ofTableCellfromstd::stringfor each column keeps behavior equivalent to the previous string-based table output.Also applies to: 1050-1053, 1067-1073, 1241-1243, 1254-1255
src/libutil/include/nix/util/table.hh (1)
9-25: TableCell/Table abstractions and printTable declaration are coherentThe TableCell struct, TableRow/Table aliases, and printTable declaration form a small, consistent API that matches the implementation and current call sites; move-aware constructor and default left alignment are sensible choices.
src/libstore/local-store-active-builds.cc (1)
204-283: Active-build lifecycle, locking, and cgroup/process integration look solidThe
queryActiveBuilds()implementation, together withbuildStarted()/buildFinished(), correctly:
- Uses per-build lock files to distinguish active vs. stale records and cleans up stale ones via
AutoDelete.- Populates
ActiveBuildInfofrom the storedActiveBuildJSON, then augments it with per-process data either from the associated cgroup (including CPU stats) or via descendant PID enumeration.- Manages build‑lifetime state in
activeBuildswith RAII cleanup viaBuildHandleandAutoDelete.This aligns well with the intended
TrackActiveBuildsStore/QueryActiveBuildsStoredesign.src/nix/ps.cc (1)
33-47: End-to-endnix psflow is consistent and well‑structuredThe command cleanly:
- Requires
QueryActiveBuildsStorefrom the store and reuses the shared ActiveBuildInfo JSON representation.- Supports both JSON and table output, including a per‑build CPU summary that prefers cgroup stats when available.
- Presents a clear “No active builds.” path.
This ties together the new active-build tracking infrastructure in a straightforward way.
Also applies to: 59-82
src/libstore/active-builds.cc (1)
12-27: UserInfo lookup and ActiveBuild JSON serializers look correct and consistent
UserInfo::fromUidsensibly resolves a username viagetpwuid_rwhen available and falls back to UID-only, which is appropriate for both process and build metadata.- Duration helpers (
parseDuration/printDuration) provide a consistent JSON representation (seconds as a number) while storingstd::chrono::microsecondsinternally.- The adl_serializers for
UserInfo,ActiveBuildInfo::ProcessInfo,ActiveBuild, andActiveBuildInfoalign with how the rest of the code constructs and consumes these types (e.g. LocalStore’s active-build registry and thenix pscommand).I don’t see correctness issues here; the design looks coherent with the rest of the PR.
Also applies to: 35-51, 53-71, 72-143
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/libutil/table.cc (1)
18-24: Column width miscalculates when cells contain ANSI escape codes.Width is computed with
j->content.size(), which counts ANSI escape sequences, but the final output is passed throughfilterANSIEscapes. This causes misaligned columns for colored or otherwise ANSI-formatted cells. The existing TODO already notes this.A concrete fix is to calculate width from the ANSI-stripped string:
- for (auto j = i.begin(); j != i.end(); ++j, ++column) - // TODO: take ANSI escapes into account when calculating width. - widths[column] = std::max(widths[column], j->content.size()); + for (auto j = i.begin(); j != i.end(); ++j, ++column) { + const auto visible = filterANSIEscapes(j->content, true); + widths[column] = std::max(widths[column], visible.size()); + }This keeps ANSI codes in
j->contentfor display but bases padding on visible width. Please verify this matches the expectedfilterANSIEscapessemantics in your tree.src/libstore/local-store-active-builds.cc (1)
41-66: Make/proc/[pid]/statparsing robust to)incommand avoid regex.On Linux the
commfield in/proc/[pid]/statis enclosed in(...)and may contain both spaces and)characters. The current regexstatic std::regex statRegex(R"((\d+) \(([^)]*)\) (.*))"); ... if (!std::regex_match(statContent, match, statRegex)) throw Error("failed to parse /proc/%d/stat", pid); // ... auto remainingFields = tokenizeString<std::vector<std::string>>(match[3].str());stops
commat the first), so for executables whose names contain)the tail (match[3]) starts in the wrong place and all subsequent fields (including state,ppid, and CPU times) are misaligned. That yields incorrect parent PIDs and bogus CPU accounting for such processes.Suggested fix (also lets you drop
<regex>entirely):- auto statContent = trim(readFile(statFd.get())); - static std::regex statRegex(R"((\d+) \(([^)]*)\) (.*))"); - std::smatch match; - if (!std::regex_match(statContent, match, statRegex)) - throw Error("failed to parse /proc/%d/stat", pid); - - // Parse the remaining fields after (comm). - auto remainingFields = tokenizeString<std::vector<std::string>>(match[3].str()); + auto statContent = trim(readFile(statFd.get())); + + // Find the *last* ')' to skip past comm, which may itself contain ')' + // and spaces: format is "pid (comm) state ppid ...". + auto lastParen = statContent.rfind(')'); + if (lastParen == std::string::npos) + throw Error("failed to parse /proc/%d/stat", pid); + + // Parse the remaining fields after the closing ')'. + auto remainingFields = + tokenizeString<std::vector<std::string>>(statContent.substr(lastParen + 1));The existing indices (
remainingFields[1]forppid,[11..14]for utime/stime/cutime/cstime) remain correct with this change.Please double‑check against
proc(5)to confirm the field ordering and that using the substring after the last)keeps the CPU time indices aligned.
🧹 Nitpick comments (3)
src/libutil/table.cc (1)
11-17: Consider takingTablebyconst &and preinitialising column widths (optional).The function never mutates
table, so the parameter can beconst Table &; andwidthscan be initialised tonrColumnszeroes directly. This slightly tightens the API and makes intent clearer.Example for this file (header would need to match):
-void printTable(std::ostream & out, Table & table, unsigned int width) +void printTable(std::ostream & out, const Table & table, unsigned int width) { - auto nrColumns = table.size() > 0 ? table.front().size() : 0; - - std::vector<size_t> widths; - widths.resize(nrColumns); + auto nrColumns = !table.empty() ? table.front().size() : 0; + + std::vector<size_t> widths(nrColumns, 0);If existing callers rely on passing a non-const
Table, please double-check that const-qualification is safe across the call sites.src/libstore/local-store-active-builds.cc (2)
212-219: Clarify intent when deleting stale active-build files.The use of
AutoDeletewith a temporary here is subtle:if (!fd || lockFile(fd.get(), ltRead, false)) { AutoDelete(path, false); continue; } ... activeBuilds.lock()->emplace( id, ActiveBuildFile{ .fd = std::move(infoFd), .del = AutoDelete(infoFilePath, false), });In the
ifblock this constructs a short‑livedAutoDeletesolely to deletepathimmediately, whereas in the map you correctly keep anAutoDeletealive untilbuildFinished()to clean up the file. The behavior is correct, but the temporary use is non‑obvious to readers familiar with the RAII pattern.Consider replacing the temporary with an explicit delete wrapped in the same error‑swallowing semantics, for readability:
- if (!fd || lockFile(fd.get(), ltRead, false)) { - AutoDelete(path, false); - continue; - } + if (!fd || lockFile(fd.get(), ltRead, false)) { + try { + deletePath(path); + } catch (...) { + ignoreExceptionExceptInterrupt(); + } + continue; + }(or equivalent explicit unlink), and keep the RAII use in
ActiveBuildFileas‑is.Please verify that
deletePath(or your chosen helper) has the same failure‑handling semantics asAutoDeletefor these simple file paths.Also applies to: 271-276
168-201: macOS descendant PID resolution is fine; optional robustness aroundsysctl.The BFS over the parent→children map from
KERN_PROC/KERN_PROC_ALLis straightforward and should produce a complete descendant set forstartPid.If you want to harden this further, consider the usual
sysctlresize loop pattern in case the process list grows between the size probe and the second call (which can return-1/ENOMEM):
- Loop until
sysctl(mib, 4, nullptr, &size, nullptr, 0)and the subsequent data call both succeed withoutENOMEM.- Or, if
ENOMEMpersists, log and fall back to{startPid}as you currently do.This is not critical for correctness—today you just return
{startPid}on failure—but it would make process-tree collection more reliable under heavy churn.Please confirm the current
sysctlbehavior forKERN_PROC_ALLon the macOS versions you target; the recommended retry pattern is documented in Apple’ssysctl/kinfo_procexamples.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/libstore/local-store-active-builds.cc(1 hunks)src/libutil/table.cc(1 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
src/libstore/local-store-active-builds.cc
[error] 1-1: 'nix/store/local-store.hh' file not found
(clang-diagnostic-error)
src/libutil/table.cc
[error] 1-1: 'nix/util/table.hh' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_x86_64-linux / build
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (2)
src/libutil/table.cc (1)
30-31: Use qualifiedstd::replacefor clarity, though ADL resolves this unqualified call correctly.The
replacecall at line 31 is unqualified, but it will still compile: because<algorithm>is included and the iterators belong tostd::string, argument-dependent lookup (ADL) findsstd::replacein thestdnamespace. However, explicit qualification withstd::replaceis clearer and follows best practices.std::string s = j->content; - replace(s.begin(), s.end(), '\n', ' '); + std::replace(s.begin(), s.end(), '\n', ' ');Likely an incorrect or invalid review comment.
src/libstore/local-store-active-builds.cc (1)
95-162: macOSgetProcessInfoimplementation looks correct.The macOS path—using
PROC_PIDTBSDINFOforppid/UID,PROC_PIDTASKINFOplusmach_timebase_info()for CPU times, andKERN_PROCARGS2viasysctlforargv—matches the documented APIs and data formats. The bounds checks on theKERN_PROCARGS2buffer and the loop overargckeep it reasonably safe; I don’t see functional issues here.If you want extra confidence, cross‑check the units of
pti_total_user/pti_total_systemin the current macOS SDK docs and confirm they are still in “absolute time” ticks scaled bymach_timebase_info().
| {formatUser(build.mainUser), | ||
| std::to_string(build.mainPid), | ||
| {"", TableCell::Alignment::Right}, | ||
| fmt("%s" ANSI_ITALIC "(no process info)" ANSI_NORMAL, treeLast)}); |
There was a problem hiding this comment.
For all of these treeLast, treeConn, treeNull, treeLine, can we add a space after this character please?
I'm always frustrated when I go to copy something from e.g. systemd's status output and get something like ├─304273 instead of 304273 because they didn't want to put a space between the tree character and its contents. Let's please not make this same mistake; I don't want to have to manually trim └───bash.
There was a problem hiding this comment.
I gave this a try but it looks kind of ugly IMHO:
nixbld10 606215 145.7s /nix/store/id2gxxr8p530rwac73qwns3rwpa8qprh-determinate-nix-util-3.13.2.drv (wall=8s)
nixbld10 606215 1.4s └── bash -e /nix/store/vj1c3wf9c11a0qs6p3ymfvrnsdgsdcbq-source-stdenv.sh /nix/store/shkw4qm9qcw5sc5n1k5jznc83ny02r39-default-builder.sh
nixbld10 606556 61.2s └── ninja -j24
If we do change this, it should be done for all tree rendering since it's a libutil thing.
BTW in konsole text selection doesn't include the tree characters...
There was a problem hiding this comment.
Mm, okay. I would still like to see it changed (so people are not beholden to their terminal emulator agreeing with the selection boundary characters), but if it'll be larger, then I'll leave it for now.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/nix/ps.cc (1)
125-126: Consider simplifying argv formatting.The current implementation joins
process->argvwith spaces, tokenizes it, then joins again with spaces. Unless there's a specific reason to re-tokenize (such as normalizing whitespace or handling quoted arguments), this could be simplified to:- auto argv = concatStringsSep( - " ", tokenizeString<std::vector<std::string>>(concatStringsSep(" ", process->argv))); + auto argv = concatStringsSep(" ", process->argv);If the re-tokenization serves a purpose (e.g., collapsing multiple spaces), consider adding a comment explaining why.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nix/ps.cc(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/nix/ps.cc (4)
src/libstore/include/nix/store/active-builds.hh (1)
build(91-91)src/libstore/include/nix/store/local-store.hh (1)
build(479-479)src/libstore/unix/build/linux-derivation-builder.cc (1)
build(734-739)src/libutil/include/nix/util/table.hh (1)
printTable(25-25)
🪛 Clang (14.0.6)
src/nix/ps.cc
[error] 1-1: 'nix/cmd/command.hh' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (3)
src/nix/ps.cc (3)
1-57: LGTM!The includes, class structure, and table initialization are well-organized. The
formatUserhelper appropriately handles both named and anonymous users.
59-81: LGTM! CPU time calculation is sound.The logic correctly prioritizes cgroup statistics when available and falls back to summing individual process times. The wall time calculation using
time(nullptr) - build.startTimeis reasonable for this monitoring utility, though it could theoretically be affected by system clock adjustments.
140-146: LGTM! Width calculation correctly handles piped output.The check for both
isTTY()andisatty(STDOUT_FILENO)ensures that output is only truncated when both stderr and stdout are terminals, which correctly addresses the grepping concern raised in previous reviews. Command registration is standard.
cole-h
left a comment
cole-h
left a comment
There was a problem hiding this comment.
(Haven't tested on macOS yet, but these should be my final comments)
| {formatUser(build.mainUser), | ||
| std::to_string(build.mainPid), | ||
| {"", TableCell::Alignment::Right}, | ||
| fmt("%s" ANSI_ITALIC "(no process info)" ANSI_NORMAL, treeLast)}); |
There was a problem hiding this comment.
Mm, okay. I would still like to see it changed (so people are not beholden to their terminal emulator agreeing with the selection boundary characters), but if it'll be larger, then I'll leave it for now.
| .clientPid = std::nullopt, // FIXME | ||
| .clientUid = std::nullopt, // FIXME |
There was a problem hiding this comment.
Somehow we have to get the Unix domain socket peer info from src/nix/unix/daemon.cc to here. I think I implemented something like that for ACL support once... Anyway that's future work.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/libstore/local-store-active-builds.cc (1)
22-69: /proc[pid]/statparsing still breaks for command names containing); prefer parsing after the last)The regex-based split assumes
commnever contains), so lines like1234 (foo)bar) R ...will mis-parse:remainingFields[1]is no longer the parent PID and the CPU time fields shift, corruptingparentPidand timing for such processes. This can affectnix psoutput for oddly named builders.Replace the regex with a simple “find last
)then tokenize the remainder” approach socommmay contain spaces and parentheses safely:@@ - // Read /proc/[pid]/stat for parent PID and CPU times. - // Format: pid (comm) state ppid ... - // Note that the comm field can contain spaces, so use a regex to parse it. - auto statContent = trim(readFile(statFd.get())); - static std::regex statRegex(R"((\d+) \(([^)]*)\) (.*))"); - std::smatch match; - if (!std::regex_match(statContent, match, statRegex)) - throw Error("failed to parse /proc/%d/stat", pid); - - // Parse the remaining fields after (comm). - auto remainingFields = tokenizeString<std::vector<std::string>>(match[3].str()); + // Read /proc/[pid]/stat for parent PID and CPU times. + // Format: pid (comm) state ppid ... + // `comm` can contain spaces and parentheses, so find the *last* ')' and + // parse fields after it. + auto statContent = trim(readFile(statFd.get())); + auto lastParen = statContent.rfind(')'); + if (lastParen == std::string::npos) + throw Error("failed to parse /proc/%d/stat", pid); + + // Parse the remaining fields after the closing ')'. + auto remainingFields = + tokenizeString<std::vector<std::string>>(statContent.substr(lastParen + 1)); @@ - static long clkTck = sysconf(_SC_CLK_TCK); + static long clkTck = sysconf(_SC_CLK_TCK);You can then drop the
<regex>include entirely.
🧹 Nitpick comments (1)
src/libstore/local-store-active-builds.cc (1)
96-159: Consider cachingmach_timebase_inforesultThe proc-based collection of
ppid,uid/user, CPU times, and argv viaKERN_PROCARGS2is correct. The time conversion is also accurate—pti_total_userandpti_total_systemare indeed expressed in mach absolute-time ticks and requiremach_timebase_infoconversion to nanoseconds.If you want a micro-optimization, cache the
mach_timebase_inforesult in a static variable since it's constant per system and unnecessary to query on everygetProcessInfocall:- if (proc_pidinfo(pid, PROC_PIDTASKINFO, 0, &taskInfo, sizeof(taskInfo)) == sizeof(taskInfo)) { - - mach_timebase_info_data_t timebase; - mach_timebase_info(&timebase); - auto nanosecondsPerTick = (double) timebase.numer / (double) timebase.denom; + if (proc_pidinfo(pid, PROC_PIDTASKINFO, 0, &taskInfo, sizeof(taskInfo)) == sizeof(taskInfo)) { + static mach_timebase_info_data_t timebase = [] { + mach_timebase_info_data_t tb; + mach_timebase_info(&tb); + return tb; + }(); + auto nanosecondsPerTick = (double) timebase.numer / (double) timebase.denom;This is purely optional; behavior is already correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/libstore/local-store-active-builds.cc(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/libstore/local-store-active-builds.cc (2)
src/libstore/active-builds.cc (3)
info(130-130)fromUid(12-27)fromUid(12-12)src/libutil/linux/cgroup.cc (4)
getPidsInCgroup(177-194)getPidsInCgroup(177-177)getCgroupStats(52-77)getCgroupStats(52-52)
🪛 Clang (14.0.6)
src/libstore/local-store-active-builds.cc
[error] 1-1: 'nix/store/local-store.hh' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-linux / build
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (4)
src/libstore/local-store-active-builds.cc (4)
74-92: Descendant PID traversal on Linux looks robust and interrupt‑awareThe recursive lambda correctly accumulates descendants via
/proc/[pid]/task/[pid]/children, and the catch block now callsignoreExceptionExceptInterrupt(), so transient races with exiting processes are tolerated without swallowing interrupts.
164-198: macOS descendant PID walk viaKERN_PROC_ALLis correct and avoids recursionBuilding a
parentPid -> {childPid}map from theKERN_PROC_ALLsnapshot and then doing a queue-based BFS fromstartPidgives a complete descendant set, avoids recursion depth issues, and gracefully degrades to{startPid}if eithersysctlcall fails.
201-248: Active-build querying and cleanup logic is robust;AutoDelete(path, false)here is intentionalThe
queryActiveBuilds()loop:
- Treats “lockable” entries as inactive (
!fd || lockFile(..., ltRead, false)), immediately cleaning up stale records viaAutoDelete(path, false); givenAutoDelete’s ctor uses the bool for “recursive” and defaultsdel=true, this temporary correctly deletes the file at end of the statement.- For locked entries, reads and deserializes the JSON, then augments with per-process info (either via cgroup PIDs + cgroup CPU stats, or descendant PIDs), with all of that work wrapped in
try { ... } catch (...) { ignoreExceptionExceptInterrupt(); }to prevent a single bad record or transient/procrace from breaking the whole query.This is a good balance of resilience and cleanup semantics for
nix ps.
250-280: Build lifecycle tracking via per-build files andBuildHandlelooks sound
buildStartedcreates a uniquepid-idfile, acquires a write lock to mark the build active, writes the JSON, and stores the open FD plus anAutoDelete(infoFilePath, false)inactiveBuilds.buildFinishederases the map entry, closing the FD and lettingAutoDeleteremove the file, soqueryActiveBuilds()will only ever see files for builds that are still in progress or crashed.The use of
std::atomic<uint64_t> nextIdensures uniqueness across concurrent starts, and the locking protocol matches the read‑side logic inqueryActiveBuilds().
2 participants
Motivation
nix psshows active builds, e.g.This improves the observability of a Nix system.
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.