Port build hook timeout commits from upstream - #583
Conversation
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds a configurable build-hook termination timeout. Hooks receive SIGTERM before SIGKILL. ChangesBuild hook termination
Filesystem test environment
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WorkerSettings
participant HookInstance
participant Pid
participant BuildHookProcess
WorkerSettings->>HookInstance: provide buildHookKillTimeout
HookInstance->>Pid: set kill signal to SIGTERM
HookInstance->>Pid: setKillTimeout(timeout)
HookInstance->>Pid: kill(SIGTERM)
Pid->>BuildHookProcess: send SIGTERM
Pid->>Pid: start timeout thread
Pid->>BuildHookProcess: send SIGKILL after timeout
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lisanna-dettwyler
changed the title
22a0b7c to
f58683d
Compare
This gives custom build hooks the chance to perform any needed cleanup, which is impossible when killed with SIGKILL. After 500ms, if the hook still hasn't exited, it will be forcibly killed with SIGKILL. Resolves NixOS#14760 Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@gmail.com>
Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@saronic.com>
f58683d to
d7cf5ab
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libutil/unix/processes.cc (1)
83-115: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMake the timeout worker independent of mutable
Pidstate and always join it.
wait()setspidto-1. If the timeout worker runs before Line 112 setskilled, Line 94 can sendSIGKILLto-1or to PID1, instead of the build hook.If
wait()throws, Lines 111-113 do not run. The joinablekillThreadthen remains in the object and can causestd::terminate.Capture immutable target values before starting the worker. Use a scope guard that cancels and joins the worker on both normal and exceptional exits. Coordinate completion before
wait()clears the target PID.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libutil/unix/processes.cc` around lines 83 - 115, The timeout worker lambda captures pid by reference, but wait() modifies pid to -1, causing the worker to send SIGKILL to an incorrect target if it executes after wait(). Additionally, if wait() throws an exception, the killThread cleanup code at lines 111-113 never runs, leaving an unjoinable thread that causes std::terminate. Capture an immutable copy of the target PID before creating the killThread lambda so the worker uses the stable value, and implement a scope guard that ensures killThread is always joined on both normal and exceptional exits before wait() is called, preserving state consistency and eliminating the exception-safety issue.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libstore/include/nix/store/worker-settings.hh`:
- Around line 137-141: The buildHookKillTimeout setting currently accepts zero,
which causes Pid::kill() to skip the SIGKILL timeout enforcement and allows a
hook ignoring SIGTERM to block shutdown indefinitely. Add a validator to the
buildHookKillTimeout Setting that rejects zero values, ensuring only positive
timeout values are accepted and the documented SIGKILL behavior is preserved.
In `@src/libutil-tests/file-system-at.cc`:
- Line 16: Update the readLinkAt.works test so the GTEST_SKIP call is
conditional on detecting EC2 container chroot stores, while preserving the
existing _WIN32 skip and allowing assertions to run on other platforms.
In `@src/libutil/include/nix/util/processes.hh`:
- Around line 39-40: Update Pid::Pid(Pid &&) to preserve killTimeout and move
killThread from the source object, alongside the existing moved fields. Update
Pid::swap to exchange both killTimeout and killThread so move assignment retains
the complete timeout state.
---
Outside diff comments:
In `@src/libutil/unix/processes.cc`:
- Around line 83-115: The timeout worker lambda captures pid by reference, but
wait() modifies pid to -1, causing the worker to send SIGKILL to an incorrect
target if it executes after wait(). Additionally, if wait() throws an exception,
the killThread cleanup code at lines 111-113 never runs, leaving an unjoinable
thread that causes std::terminate. Capture an immutable copy of the target PID
before creating the killThread lambda so the worker uses the stable value, and
implement a scope guard that ensures killThread is always joined on both normal
and exceptional exits before wait() is called, preserving state consistency and
eliminating the exception-safety issue.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a8784a0d-0e01-430d-98bb-c0f716962803
📒 Files selected for processing (8)
src/libstore/build/derivation-building-goal.ccsrc/libstore/include/nix/store/worker-settings.hhsrc/libstore/unix/build/hook-instance.ccsrc/libstore/unix/include/nix/store/build/hook-instance.hhsrc/libutil-tests/file-system-at.ccsrc/libutil/include/nix/util/processes.hhsrc/libutil/unix/processes.ccsrc/nix/build-remote/build-remote.cc
| Setting<uint32_t> buildHookKillTimeout{ | ||
| this, | ||
| 500, | ||
| "build-hook-kill-timeout", | ||
| "How long to wait in milliseconds for build hooks to exit on interrupt before sending SIGKILL."}; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject a zero build-hook kill timeout.
This setting accepts 0. Pid::kill() then skips the SIGKILL timeout worker, so a hook that ignores SIGTERM can block shutdown indefinitely. This conflicts with the documented SIGKILL behavior.
Reject zero during setting validation, or explicitly document that zero disables forced termination.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/libstore/include/nix/store/worker-settings.hh` around lines 137 - 141,
The buildHookKillTimeout setting currently accepts zero, which causes
Pid::kill() to skip the SIGKILL timeout enforcement and allows a hook ignoring
SIGTERM to block shutdown indefinitely. Add a validator to the
buildHookKillTimeout Setting that rejects zero values, ensuring only positive
timeout values are accepted and the documented SIGKILL behavior is preserved.
There was a problem hiding this comment.
@lisanna-dettwyler This seems like a relevant comment. The ability to disable the timeout seems useful, but it should be documented.
|
|
||
| TEST(readLinkAt, works) | ||
| { | ||
| GTEST_SKIP() << "Broken on EC2 container chroot stores"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'EC2|ec2|chroot|CHROOT|store.*chroot|chroot.*store|GTEST_SKIP' src maintainersRepository: DeterminateSystems/nix-src
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- file-system-at.cc ---\n'
cat -n src/libutil-tests/file-system-at.cc | sed -n '1,90p'
printf '\n--- build/test config references to file-system-at target/list ---\n'
rg -n 'file-system-at|fileSystemAt|readLinkAt|GTEST_SKIP|WIN32|Windows' meson.build meson_options.txt src/libutil-tests.cmake src/libutil-tests/CMakeLists.txt . 2>/dev/null | head -n 120Repository: DeterminateSystems/nix-src
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file-system-at.cc ---'
cat -n src/libutil-tests/file-system-at.cc | sed -n '1,90p'
echo
echo '--- build/test config references ---'
rg -n -C 3 'file-system-at|fileSystemAt|readLinkAt|GTEST_SKIP|WIN32|Windows' meson.build meson_options.txt src/libutil-tests.cmake src/libutil-tests/CMakeLists.txt 2>/dev/null || trueRepository: DeterminateSystems/nix-src
Length of output: 4589
Scope the skip to EC2 container chroot stores.
GTEST_SKIP() at the start of TEST(readLinkAt, works) runs before all assertions and makes the #ifdef _WIN32 skip unreachable. Wrap the skip in a check for EC2 container chroot stores before removing readLinkAt coverage on other platforms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/libutil-tests/file-system-at.cc` at line 16, Update the readLinkAt.works
test so the GTEST_SKIP call is conditional on detecting EC2 container chroot
stores, while preserving the existing _WIN32 skip and allowing assertions to run
on other platforms.
| std::chrono::milliseconds killTimeout; | ||
| std::thread killThread; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve timeout state when moving Pid.
Pid::Pid(Pid &&) copies only pid, separatePG, and killSignal. A moved Pid loses killTimeout. The shown swap also omits killTimeout and killThread, so move assignment loses them too.
Initialize killTimeout and move killThread in the move constructor. Swap both fields in swap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/libutil/include/nix/util/processes.hh` around lines 39 - 40, Update
Pid::Pid(Pid &&) to preserve killTimeout and move killThread from the source
object, alongside the existing moved fields. Update Pid::swap to exchange both
killTimeout and killThread so move assignment retains the complete timeout
state.
There was a problem hiding this comment.
This is probably worth fixing even if it doesn't affect this PR.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
processes_cc="$(fd -a '^processes\.cc$' src/libutil | head -n1)"
if [ -z "$processes_cc" ]; then
echo "src/libutil unix processes implementation was not found." >&2
exit 1
fi
ast-grep outline "$processes_cc" --items all --match 'Pid|kill|wait|setKillTimeout' --view expanded
rg -n -C 12 --glob 'processes.cc' \
'Pid::Pid\(Pid &&|Pid::operator=\(Pid &&|void Pid::setKillTimeout|int Pid::kill|int Pid::wait|killThread|killTimeout' \
src/libutil
ast-grep outline src/libutil/include/nix/util/processes.hh --match 'Pid' --view expandedLength of output: 12393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Pid destructor and adjacent lifecycle code ---'
sed -n '35,175p' src/libutil/unix/processes.cc
printf '%s\n' '--- Direct Pid moves and move assignments ---'
rg -n -C 5 --glob '*.{cc,hh}' \
'(?:std::move\([^)]*\)|return)\s*.*\bPid\b|\bPid\s+\w+\s*=\s*std::move|\boperator=\(std::move' \
srcLength of output: 4244
@edolstra, verified.
The minimal safe fix is:
- Copy
killTimeoutinPid::Pid(Pid &&). - Swap
killTimeoutinswap(Pid &, Pid &).
Do not move or swap killThread. Its lambda accesses the source Pid state and the stack-local killed flag. Pid::kill() joins it on the normal path.
You are interacting with an AI system.
Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@saronic.com>
d7cf5ab to
89b7008
Compare
|
Tacked on a unit test disable because this is the branch we're working off of for our slurm cluster, and that test fails when built on the slurm cluster because of a bind mounted rootfs. |
|
Can you rebase this on the current main branch? |
2 participants
Motivation
Ports recent build-hook kill timeout changes from upstream, needed for our custom slurm build hook
Context
NixOS#14760
NixOS#16230
Summary by CodeRabbit
New Features
build-hook-kill-timeoutsetting, defaulting to 500 milliseconds.Bug Fixes