t1398.1: Add process resource guard to pulse-wrapper.sh#2881
t1398.1: Add process resource guard to pulse-wrapper.sh#2881alex-solovyev merged 1 commit intomainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue of runaway processes consuming excessive system resources, which previously led to kernel panics. By introducing a robust process resource guard, the system can now automatically identify and terminate processes that exceed configurable memory or runtime limits. This enhancement significantly improves the stability and reliability of the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR updates the header documentation in pulse-wrapper.sh to reflect the planned process resource guard functionality (t1398.1), repositioning the documented order of responsibilities to include the new capability that kills child processes exceeding RSS or runtime limits. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a resource guard to pulse-wrapper.sh to prevent runaway processes by monitoring RSS and runtime, which is a great addition for system stability. However, a critical security vulnerability has been identified: a logic error in how the current process is excluded from monitoring, which can lead to a security bypass, and a log injection vulnerability where unsanitized process command lines are written to the log file. Additionally, the review suggests improving the performance and robustness of the process-parsing loops by using more idiomatic Bash constructs and adhering to project guidelines on error handling.
.agents/scripts/pulse-wrapper.sh
Outdated
| killed=$((killed + 1)) | ||
| total_freed_mb=$((total_freed_mb + mb)) | ||
| fi | ||
| done < <(ps -axo pid,rss,command 2>/dev/null | grep -E 'shellcheck|\.opencode|node.*opencode' | grep -v grep | grep -v "$$" || true) |
There was a problem hiding this comment.
A critical security bypass vulnerability exists due to the use of grep -v "$$" to exclude the current process. This method performs a substring match, which can unintentionally exclude other processes whose PID, RSS, or command line contains the script's PID as a substring. This allows runaway processes to escape termination, potentially leading to a system-wide denial-of-service. The suggested fix removes this problematic grep filter.
Additionally, the current method of parsing ps output by reading line-by-line is inefficient. A more performant approach would be to use read to parse fields directly within the while loop. While the suggested code retains 2>/dev/null for ps output, it's generally recommended to avoid suppressing ps errors to ensure potential system or command errors are visible for debugging.
| done < <(ps -axo pid,rss,command 2>/dev/null | grep -E 'shellcheck|\.opencode|node.*opencode' | grep -v grep | grep -v "$$" || true) | |
| done < <(ps -axo pid,rss,command 2>/dev/null | grep -E 'shellcheck|\.opencode|node.*opencode' | grep -v grep || true) |
References
- Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.
.agents/scripts/pulse-wrapper.sh
Outdated
| killed=$((killed + 1)) | ||
| total_freed_mb=$((total_freed_mb + mb)) | ||
| fi | ||
| done < <(ps -axo pid,etime,command 2>/dev/null | grep 'shellcheck' | grep -v grep | grep -v "$$" || true) |
There was a problem hiding this comment.
Similar to the previous instance, a critical security bypass vulnerability exists here due to the use of grep -v "$$" to exclude the current process. This substring matching can unintentionally exclude other processes, allowing runaway processes to escape termination and potentially leading to a system-wide denial-of-service. The suggested fix removes this problematic grep filter.
Additionally, this loop can be made more efficient by using read to parse the ps output directly instead of using echo, awk, and cut in subshells for each line. While the suggested code retains 2>/dev/null for ps output, it's generally recommended to avoid suppressing ps errors to ensure potential system or command errors are visible for debugging.
| done < <(ps -axo pid,etime,command 2>/dev/null | grep 'shellcheck' | grep -v grep | grep -v "$$" || true) | |
| done < <(ps -axo pid,etime,command 2>/dev/null | grep 'shellcheck' | grep -v grep || true) |
References
- Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.
.agents/scripts/pulse-wrapper.sh
Outdated
| if [[ "$rss" -gt "$RESOURCE_GUARD_RSS_LIMIT_KB" ]]; then | ||
| local mb=$((rss / 1024)) | ||
| local limit_mb=$((RESOURCE_GUARD_RSS_LIMIT_KB / 1024)) | ||
| echo "[pulse-wrapper] Resource guard: killing PID $pid (RSS ${mb}MB > ${limit_mb}MB limit): ${cmd:0:120}" >>"$LOGFILE" |
There was a problem hiding this comment.
The script logs the command line of processes it terminates to $LOGFILE. The command line (cmd) is extracted directly from the ps output and appended to the log without sanitization. An attacker who can execute processes on the system can craft a process with a malicious command line (e.g., containing newlines or fake log prefixes) to inject arbitrary entries into the log file. This can be used to mislead administrators, spoof system messages, or obfuscate malicious activity.
.agents/scripts/pulse-wrapper.sh
Outdated
| rss_kb=$(ps -p "$pid" -o rss= 2>/dev/null | tr -d ' ') || rss_kb=0 | ||
| [[ "$rss_kb" =~ ^[0-9]+$ ]] || rss_kb=0 | ||
| local mb=$((rss_kb / 1024)) | ||
| echo "[pulse-wrapper] Resource guard: killing shellcheck PID $pid (runtime ${age_seconds}s > ${RESOURCE_GUARD_RUNTIME_LIMIT}s limit, RSS ${mb}MB): ${cmd:0:120}" >>"$LOGFILE" |
There was a problem hiding this comment.
The script logs the command line of processes it terminates to $LOGFILE. The command line (cmd) is extracted directly from the ps output and appended to the log without sanitization. An attacker who can execute processes on the system can craft a process with a malicious command line (e.g., containing newlines or fake log prefixes) to inject arbitrary entries into the log file. This can be used to mislead administrators, spoof system messages, or obfuscate malicious activity.
|
This PR has merge conflicts after recent merges to main. Dispatching a worker to rebase and resolve conflicts. |
alex-solovyev
force-pushed
the
bugfix/t1398.1-process-resource-guard
branch
from
6e4de7b to
844cffb
Compare
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Thu Mar 5 03:57:15 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
Add cleanup_runaway_processes() that kills child processes exceeding configurable RSS limit (2GB default) or runtime limit (10min for shellcheck). Prevents single processes from consuming all RAM — root cause of the March 3 kernel panic where shellcheck with --external-sources reached 5.7GB RSS. Two-pass design: - Pass 1: RSS limit on all framework processes (opencode, shellcheck, node) - Pass 2: Runtime limit specifically on shellcheck (catches exponential expansion before RSS ceiling) Extends existing _kill_tree/_force_kill_tree pattern. Configurable via RESOURCE_GUARD_RSS_LIMIT_KB and RESOURCE_GUARD_RUNTIME_LIMIT env vars. Closes #2874
alex-solovyev
force-pushed
the
bugfix/t1398.1-process-resource-guard
branch
from
844cffb to
545701e
Compare
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Thu Mar 5 04:30:04 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…1402) Add _sanitize_log_field() helper that strips control characters (ASCII 0x00-0x1F and 0x7F) from untrusted strings before writing to log files. Applied to cmd_base in guard_child_processes() where process names from ps output are logged during termination. Addresses the medium-severity log injection findings from Gemini review on PR #2881. The two critical findings (grep -v $$ self-exclusion bypass) were already fixed by prior PRs that rewrote the process guard to use awk descendant-tree walking. Closes #2892
…1402) Add _sanitize_log_field() helper that strips control characters (ASCII 0x00-0x1F and 0x7F) from untrusted strings before writing to log files. Applied to cmd_base in guard_child_processes() where process names from ps output are logged during termination. Addresses the medium-severity log injection findings from Gemini review on PR #2881. The two critical findings (grep -v $$ self-exclusion bypass) were already fixed by prior PRs that rewrote the process guard to use awk descendant-tree walking. Closes #2892
…1402) (#2908) Add _sanitize_log_field() helper that strips control characters (ASCII 0x00-0x1F and 0x7F) from untrusted strings before writing to log files. Applied to cmd_base in guard_child_processes() where process names from ps output are logged during termination. Addresses the medium-severity log injection findings from Gemini review on PR #2881. The two critical findings (grep -v $$ self-exclusion bypass) were already fixed by prior PRs that rewrote the process guard to use awk descendant-tree walking. Closes #2892
2 participants
Summary
cleanup_runaway_processes()to pulse-wrapper.sh that kills child processes exceeding configurable RSS limit (2GB default) or runtime limit (10min for shellcheck)--external-sourcesreached 5.7GB RSSDesign
Configuration (env var overrides with validated defaults):
RESOURCE_GUARD_RSS_LIMIT_KB— default 2097152 (2GB). Any framework process exceeding this RSS is killed.RESOURCE_GUARD_RUNTIME_LIMIT— default 600 (10min). ShellCheck processes exceeding this runtime are killed.Integration: Runs in
main()aftercleanup_orphansand beforecleanup_worktrees, so it catches runaway processes from previous cycles before starting new work.Safety:
shellcheck,.opencode,node.*opencode) — never touches unrelated user processes$$)_kill_tree→_force_kill_treeescalation pattern (SIGTERM first, SIGKILL if still alive after 1s)_validate_intwith minimum of 1 (prevents divide-by-zero and command injection)Verification
bash -n: syntax OKCloses #2874
Summary by CodeRabbit