[337] Add logic to run child processes at lower priority than parent#142
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds an exported Command field 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@system/exec_cmd_runner.go`:
- Around line 57-59: RunComplexCommandAsync currently ignores
Command.SpawnWithLowerPriority; update RunComplexCommandAsync to mirror
RunComplexCommand by checking cmd.SpawnWithLowerPriority immediately after
starting the process (after invoking Start()) and calling
r.lowerProcessPriority(cmd.Name, process.cmd.Process.Pid) (same nolint:errcheck
handling if present). Ensure you reference the same symbols:
RunComplexCommandAsync, Command.SpawnWithLowerPriority, lowerProcessPriority,
and the started process at process.cmd.Process.Pid so async execution respects
the lower-priority flag.
- Around line 31-40: The Windows branch currently hardcodes
processpriority.BelowNormal which can be >= parent when the parent is Idle;
instead derive the child's priority class from the parent's current class and
clamp at Idle so the child is strictly lower. In the Windows branch (where
runtime.GOOS == "windows"), read the parent's priority class (use the variable
or API you already obtain for the parent), compute the next-lower Windows class
(e.g., map parent -> next lower: High->AboveNormal, AboveNormal->Normal,
Normal->BelowNormal, BelowNormal->Idle, Idle->Idle), clamp to
IDLE_PRIORITY_CLASS, then call processpriority.Set(processPid, computedClass)
rather than using the hardcoded processpriority.BelowNormal; keep logging via
r.logger.Debug and preserve SpawnWithLowerPriority semantics.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38e47492-53c3-41a7-8a93-3fcd67d0d9b3
⛔ Files ignored due to path filters (33)
go.sumis excluded by!**/*.sumvendor/github.com/hekmon/processpriority/.gitignoreis excluded by!vendor/**vendor/github.com/hekmon/processpriority/LICENSEis excluded by!vendor/**vendor/github.com/hekmon/processpriority/README.mdis excluded by!vendor/**vendor/github.com/hekmon/processpriority/priority.gois excluded by!vendor/**vendor/github.com/hekmon/processpriority/priority_unix.gois excluded by!vendor/**vendor/github.com/hekmon/processpriority/priority_win.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/aliases.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/dll_windows.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/env_windows.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/eventlog.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/exec_windows.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/memory_windows.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/mkerrors.bashis excluded by!vendor/**vendor/golang.org/x/sys/windows/mkknownfolderids.bashis excluded by!vendor/**vendor/golang.org/x/sys/windows/mksyscall.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/race.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/race0.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/security_windows.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/service.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/setupapi_windows.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/str.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/syscall.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/syscall_windows.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/types_windows.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/types_windows_386.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/types_windows_amd64.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/types_windows_arm.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/types_windows_arm64.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/zerrors_windows.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/zknownfolderids_windows.gois excluded by!vendor/**vendor/golang.org/x/sys/windows/zsyscall_windows.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (4)
go.modsystem/cmd_runner_interface.gosystem/exec_cmd_runner.gosystem/exec_cmd_runner_test.go
github-project-automation
Bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in mechanism in bosh-utils/system to spawn child processes at a lower scheduling priority than the parent process, to prevent child scripts from starving the agent of CPU time.
Changes:
- Adds
SpawnWithLowerPrioritytosystem.Commandto request reduced scheduling priority for spawned processes. - Implements priority lowering in
execCmdRunnerusinggithub.com/hekmon/processpriority(nice adjustment on Unix; priority class adjustment on Windows). - Vendors new dependencies and adds integration tests validating the effective priority of spawned processes on Unix and Windows.
Reviewed changes
Copilot reviewed 4 out of 37 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Adds github.com/hekmon/processpriority dependency. |
| go.sum | Adds checksums for github.com/hekmon/processpriority. |
| system/cmd_runner_interface.go | Extends Command with SpawnWithLowerPriority. |
| system/exec_cmd_runner.go | Applies post-Start() priority lowering when requested. |
| system/exec_cmd_runner_test.go | Adds OS-specific integration tests asserting lowered child priority. |
| vendor/modules.txt | Updates vendoring metadata to include new dependency and Windows sys package. |
| vendor/github.com/hekmon/processpriority/README.md | Vendors processpriority documentation. |
| vendor/github.com/hekmon/processpriority/priority.go | Vendors processpriority cross-platform API. |
| vendor/github.com/hekmon/processpriority/priority_unix.go | Vendors Unix nice-based priority implementation. |
| vendor/github.com/hekmon/processpriority/priority_win.go | Vendors Windows priority-class implementation. |
| vendor/github.com/hekmon/processpriority/LICENSE | Vendors processpriority license. |
| vendor/github.com/hekmon/processpriority/.gitignore | Vendors processpriority gitignore. |
| vendor/golang.org/x/sys/windows/aliases.go | Vendors x/sys/windows support code (required by processpriority on Windows). |
| vendor/golang.org/x/sys/windows/dll_windows.go | Vendors x/sys/windows support code. |
| vendor/golang.org/x/sys/windows/env_windows.go | Vendors x/sys/windows support code. |
| vendor/golang.org/x/sys/windows/eventlog.go | Vendors x/sys/windows support code. |
| vendor/golang.org/x/sys/windows/exec_windows.go | Vendors x/sys/windows support code. |
| vendor/golang.org/x/sys/windows/memory_windows.go | Vendors x/sys/windows support code. |
| vendor/golang.org/x/sys/windows/mkerrors.bash | Vendors x/sys/windows generation script. |
| vendor/golang.org/x/sys/windows/mkknownfolderids.bash | Vendors x/sys/windows generation script. |
| vendor/golang.org/x/sys/windows/mksyscall.go | Vendors x/sys/windows generation driver. |
| vendor/golang.org/x/sys/windows/race.go | Vendors x/sys/windows race build helper. |
| vendor/golang.org/x/sys/windows/race0.go | Vendors x/sys/windows non-race build helper. |
| vendor/golang.org/x/sys/windows/security_windows.go | Vendors x/sys/windows security APIs. |
| vendor/golang.org/x/sys/windows/service.go | Vendors x/sys/windows service APIs. |
| vendor/golang.org/x/sys/windows/setupapi_windows.go | Vendors x/sys/windows setupapi bindings. |
| vendor/golang.org/x/sys/windows/str.go | Vendors x/sys/windows helper code. |
| vendor/golang.org/x/sys/windows/syscall.go | Vendors x/sys/windows syscall helpers. |
| vendor/golang.org/x/sys/windows/types_windows_386.go | Vendors x/sys/windows types for 386. |
| vendor/golang.org/x/sys/windows/types_windows_amd64.go | Vendors x/sys/windows types for amd64. |
| vendor/golang.org/x/sys/windows/types_windows_arm.go | Vendors x/sys/windows types for arm. |
| vendor/golang.org/x/sys/windows/types_windows_arm64.go | Vendors x/sys/windows types for arm64. |
| vendor/golang.org/x/sys/windows/zknownfolderids_windows.go | Vendors generated known-folder IDs for Windows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Idle priority is too aggressive and can cause process starvation since it only runs when the system is idle. BelowNormal provides a closer behavioral match to the Linux nice +5 implementation.
Pass the command name as a logTag parameter instead of using os.Args[0]. This ensures logs from LowerProcessPriority use the same tag as the calling RunComplexCommand.
The receiver type (execCmdRunner) is unexported, so there is no need to export LowerProcessPriority. Also remove duplicate error logging from the caller since the function already logs errors.
Log the process pid in all debug/error messages to aid troubleshooting. Use Error level (not Debug) when getting the parent priority fails since it is an operational failure.
Match the implementation's clamping logic in the test. If the parent process runs with nice >= 15, rawParentPrio+5 exceeds 19 and the test would fail even though the implementation is correct.
Since priority is set after Start(), a short-lived script may read its nice value before the parent has applied the change. Add a brief sleep in test scripts to ensure the priority is set before querying it. Also fix typo in comments.
The async path is used for drain scripts, errands, and cert manager operations - the primary use cases that benefit from lower priority execution.
Document the exact behavior on each platform and clarify that the child may run at the same priority if the parent is already at the minimum.
neddp
force-pushed
the
337-nice-child-scripts-v2
branch
from
ba45651 to
244989b
Compare
Member
Author
|
Had to force-push to remove the inception@anynines.com email from some of the commits. This was causing the EasyCLA to fail. |
Verify that RunComplexCommandAsync also applies lower priority to spawned processes on both Linux and Windows.
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
Draft
3 tasks
rkoster
requested changes
Jun 4, 2026
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@system/exec_cmd_runner_test.go`:
- Around line 190-211: The test is executing the temp script file directly which
fails on mounts with noexec; modify the test to invoke the script via the shell
instead of executing the file: keep creating/writing/chmod the temp script, but
construct the Command passed to runner.RunComplexCommand with Name set to "bash"
(or the system shell) and pass the temp file path as the first argument (e.g.,
Args containing tmpFile.Name()), preserving SpawnWithLowerPriority on the
Command; update any expectations that reference the command invocation if
needed.
In `@system/process_priority_windows.go`:
- Around line 15-22: The code currently defines processAllAccess with a broad
0xffff mask; replace it with the minimal Windows rights used for priority
operations: use the narrower constants PROCESS_QUERY_INFORMATION for
GetPriorityClass and PROCESS_SET_INFORMATION for SetPriorityClass (or define a
combined mask PROCESS_QUERY_INFORMATION|PROCESS_SET_INFORMATION if a single mask
is needed). Update the constant(s) (e.g., replace processAllAccess) and any
callers that pass it to OpenProcess so they request only the required rights
instead of full processAllAccess.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54506463-2ec4-4823-b5c4-acf0b161afe4
⛔ Files ignored due to path filters (1)
vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (6)
go.modsystem/cmd_runner_interface.gosystem/exec_cmd_runner.gosystem/exec_cmd_runner_test.gosystem/process_priority_unix.gosystem/process_priority_windows.go
Use PROCESS_QUERY_INFORMATION for GetPriorityClass and PROCESS_SET_INFORMATION for SetPriorityClass instead of PROCESS_ALL_ACCESS.
Avoids failures on systems where /tmp is mounted noexec. Also removes the now-unnecessary os.Chmod calls.
rkoster
approved these changes
Jun 4, 2026
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Done
in Foundational Infrastructure Working Group
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR continues the work from #133 (by @ionphractal) to add support for spawning child processes with a lower scheduling priority than the parent process.
Motivation
Related to cloudfoundry/bosh-agent#337. When the BOSH agent spawns child scripts (drain, pre-start, etc.), they can starve the agent itself of CPU time. By running child processes at a lower priority (nice +5 on Linux, BelowNormal on Windows), the agent remains responsive even under heavy child workload.
Changes
SpawnWithLowerPriorityfield tosystem.CommandstructlowerProcessPriorityinexecCmdRunnerthat:min(parent_nice + 5, 19)BelowNormalStart()in bothRunComplexCommandandRunComplexCommandAsyncsystem/process_priority_unix.goandsystem/process_priority_windows.go(inspired bygithub.com/hekmon/processpriority, MIT, Copyright 2024 Edouard Hur) to avoid the external dependencyOpenProcessuses minimum required access rights (PROCESS_QUERY_INFORMATIONfor get,PROCESS_SET_INFORMATIONfor set) following the principle of least privilegebash <script>instead of executing the temp file directly, to work correctly on systems withnoexec/tmpmountsOriginal author
Commits authored by Michael Both (@ionphractal)
Changes compared to original PR #133
The following changes address review feedback and fix issues found during testing:
os.Args[0]so logs match the calling RunComplexCommandgithub.com/hekmon/processprioritydependency and inlined the relevant logic into platform-specific files to eliminate supply chain riskPROCESS_QUERY_INFORMATION/PROCESS_SET_INFORMATIONinstead ofPROCESS_ALL_ACCESSnoexec/tmpmountsbosh-agent changes: cloudfoundry/bosh-agent/pull/438