fix(parity): contain fork-bombing tests in the node-suite harness#6762
Open
TheHypnoo wants to merge 3 commits into
Open
fix(parity): contain fork-bombing tests in the node-suite harness#6762TheHypnoo wants to merge 3 commits into
TheHypnoo wants to merge 3 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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Perry parity test runner applies a dynamic per-user process limit before 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 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
Comment on lines
+760
to
+761
| ulimit -u $(( $(ps -u "$(id -u)" -o pid= | wc -l) + 50 )) 2>/dev/null | ||
| exec "$@"' _ env PERRY_STUB_DIAG=off "${parity_env[@]}" "$perry_binary" "${test_argv[@]}" > "$perry_tmp" 2>&1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail closed when the process limit cannot be established.
Failures from ps, the arithmetic probe, and ulimit -u are ignored, so Perry still launches without protection if the limit cannot be applied. Validate the process count and ulimit result, then abort before exec when either fails.
🤖 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 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.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
The process budget is shared across all runs for this user.
ulimit -u limits the entire UID, not this Perry process tree. Concurrent module runs or unrelated jobs can consume the same +50 headroom and cause legitimate forks to fail, so the stated concurrent-run isolation is not achieved. Use a cgroup/container PID limit or serialize and coordinate the budget.
🤖 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 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.
…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).
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