fix(parity): contain fork-bombing tests in the node-suite harness#6762
Open
TheHypnoo wants to merge 5 commits into
Open
fix(parity): contain fork-bombing tests in the node-suite harness#6762TheHypnoo wants to merge 5 commits into
TheHypnoo wants to merge 5 commits into
Conversation
Two cluster/setup tests (validation-exec-args, validation-serialization- inspect) re-exec themselves recursively under Perry. The harness's 10s timeout only kills the direct child, so orphaned workers kept forking until the macOS per-user process cap saturated and killed the suite itself mid-run (twice), leaving ~1500 orphans behind. Containment, two layers: - Run each perry test binary under its own fork budget (current user procs + 50 via a /bin/sh ulimit wrapper): legit multi-process tests fit easily, a fork bomb stalls at ~50 orphans instead of starving the harness out of fork(). - Reap orphans after every test with pkill scoped to this run's PARITY_TMP dir, so concurrent suite runs are unaffected. A global ulimit around the whole suite does not work: the harness inherits it, the bomb pins the process count at the cap, and the cleanup pkill itself can no longer fork.
📝 WalkthroughWalkthroughThe Perry parity test runner applies a dynamic per-user process limit before each execution and forcefully cleans up orphaned processes associated with the run’s temporary directory afterward. ChangesParity process hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🤖 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 `@run_parity_tests.sh`:
- Around line 760-761: Update the process-limit setup immediately before the
exec command to validate both the process-count calculation and the ulimit -u
application. Capture or check each operation’s status, abort with a nonzero
status if either fails, and only execute Perry after the limit has been
successfully established.
- Around line 760-761: Replace the per-process ulimit-based budget around the
Perry execution command with coordinated resource isolation: use a cgroup or
container PID limit scoped to this run, or serialize runs through a shared
budget mechanism. Ensure concurrent module runs and unrelated processes cannot
consume the run’s reserved process capacity, while preserving the existing exec
invocation and environment setup.
- Line 767: Replace the raw-path pkill invocation in the parity test cleanup
with targeted termination of the tracked test process group or PIDs. If
retaining pkill, escape the temporary path for literal matching and verify the
selector before sending SIGKILL, ensuring unrelated processes cannot be
terminated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…kill pattern CodeRabbit review of #6762: - If ps can't measure the current process count, skip the cap entirely (fail open, commented why) instead of computing ulimit -u 50 from a zero count — a bogus low cap would silently break every legitimate multi-process test. The cap is containment, not a correctness gate; the post-test reap still runs either way. - pkill -f treats its pattern as an ERE, so escape the tmp-dir path's metacharacters (the mktemp component contains a dot) before matching. Validated: node-suite path module runs end-to-end through the updated wrapper (97.8%, same 2 known mismatches as the 2026-07-22 baseline, 0 crashes).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@run_parity_tests.sh`:
- Around line 765-780: Update the process-counting logic before the ulimit call
in the run_with_timeout block to count all current user tasks/threads rather
than only process leaders returned by ps -u -o pid=. Use a Linux-compatible
task-level count, preserve the existing validation and failure handling, and
continue setting the limit relative to that count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+765
to
+780
| run_with_timeout 10 /bin/sh -c ' | ||
| proc_list=$(ps -u "$(id -u)" -o pid= 2>/dev/null) || { | ||
| echo "failed to measure the current user process count" >&2 | ||
| exit 125 | ||
| } | ||
| nproc_now=$(printf "%s\n" "$proc_list" | awk "NF { count++ } END { print count + 0 }") || { | ||
| echo "failed to count the current user processes" >&2 | ||
| exit 125 | ||
| } | ||
| if ! [ "$nproc_now" -gt 0 ] 2>/dev/null; then | ||
| echo "invalid current user process count: $nproc_now" >&2 | ||
| exit 125 | ||
| fi | ||
| if ! ulimit -u $(( nproc_now + 50 )) 2>/dev/null; then | ||
| echo "failed to apply the per-test process limit" >&2 | ||
| exit 125 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
uid=$(id -u)
leaders=$(ps -u "$uid" -o pid= | awk 'NF { count++ } END { print count + 0 }')
tasks=$(ps -u "$uid" -L -o lwp= | awk 'NF { count++ } END { print count + 0 }')
printf 'process leaders: %s\ntasks: %s\n' "$leaders" "$tasks"
test "$tasks" -ge "$leaders"Repository: PerryTS/perry
Length of output: 179
Count tasks, not process leaders, before applying the process limit.
On Linux, RLIMIT_NPROC is thread-based, but ps -u ... -o pid= only counts process leaders. For a multithreaded user, this can set ulimit -u below current task usage, causing the next fork or thread creation to fail immediately.
Proposed direction
- proc_list=$(ps -u "$(id -u)" -o pid= 2>&1) || {
+ proc_list=$(ps -u "$(id -u)" -L -o lwp= 2>&1) || {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| run_with_timeout 10 /bin/sh -c ' | |
| proc_list=$(ps -u "$(id -u)" -o pid= 2>/dev/null) || { | |
| echo "failed to measure the current user process count" >&2 | |
| exit 125 | |
| } | |
| nproc_now=$(printf "%s\n" "$proc_list" | awk "NF { count++ } END { print count + 0 }") || { | |
| echo "failed to count the current user processes" >&2 | |
| exit 125 | |
| } | |
| if ! [ "$nproc_now" -gt 0 ] 2>/dev/null; then | |
| echo "invalid current user process count: $nproc_now" >&2 | |
| exit 125 | |
| fi | |
| if ! ulimit -u $(( nproc_now + 50 )) 2>/dev/null; then | |
| echo "failed to apply the per-test process limit" >&2 | |
| exit 125 | |
| run_with_timeout 10 /bin/sh -c ' | |
| proc_list=$(ps -u "$(id -u)" -L -o lwp= 2>/dev/null) || { | |
| echo "failed to measure the current user process count" >&2 | |
| exit 125 | |
| } | |
| nproc_now=$(printf "%s\n" "$proc_list" | awk "NF { count++ } END { print count + 0 }") || { | |
| echo "failed to count the current user processes" >&2 | |
| exit 125 | |
| } | |
| if ! [ "$nproc_now" -gt 0 ] 2>/dev/null; then | |
| echo "invalid current user process count: $nproc_now" >&2 | |
| exit 125 | |
| fi | |
| if ! ulimit -u $(( nproc_now + 50 )) 2>/dev/null; then | |
| echo "failed to apply the per-test process limit" >&2 | |
| exit 125 |
🤖 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 `@run_parity_tests.sh` around lines 765 - 780, Update the process-counting
logic before the ulimit call in the run_with_timeout block to count all current
user tasks/threads rather than only process leaders returned by ps -u -o pid=.
Use a Linux-compatible task-level count, preserve the existing validation and
failure handling, and continue setting the limit relative to that count.
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
1 participant
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.
Problem
Running the full node-suite parity run locally (
./run_parity_tests.sh --suite node-suite) kills the host machine. Two cluster tests (cluster/setup/validation-exec-argsandcluster/setup/validation-serialization-inspect) fork-bomb under Perry: each forked worker re-execs itself and forks again. The harness's 10s timeout only kills the direct child, so the orphaned workers keep replicating until the macOS per-user process cap saturates — at which point the harness itself can no longerfork()and the suite dies mid-run. Reproduced twice: both runs died at the same spot withfork: Resource temporarily unavailable, leaving ~1,500 orphaned processes behind.Fix (two layers, both in
run_parity_tests.sh)/bin/sh -c 'ulimit -u <current user procs + 50>; exec …'. Legitimate multi-process tests (cluster/child_process spawn 2–4 workers) fit easily; a fork bomb stalls at ~50 orphans instead of starving the harness.pkill -9 -f "$PARITY_TMP/"reaps anything still running from this run's tmp dir. Scoped to the per-runmktemp -dpath, so concurrent suite runs are unaffected.Note: a global
ulimit -uaround the whole suite does not work — the harness inherits the limit, the bomb pins the process count at the cap, and the cleanuppkillitself can no longer fork. (Tried; failed in practice.)Validation
Full node-suite run (3,873 tests, Node 26.5.0) completes end-to-end with this fix: 3,815 run, 3,019 pass (79.1%), report generated. The two bombing tests are correctly classified as CRASH (timeout) and no orphan processes remain after the run (verified with
ps). First 21 tests all pass under the wrapper, confirming it doesn't perturb normal execution.The underlying Perry bug (cluster.fork re-exec loop) is a separate issue — this PR only makes the harness survive it.
Summary by CodeRabbit