test: GPU-free regression test for env/SSH memory-limit isolation (#2125) - #272
test: GPU-free regression test for env/SSH memory-limit isolation (#2125)#272mohitt31 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mohitt31 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @mohitt31! It looks like this is your first PR to Project-HAMi/HAMi-core 🎉 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis change adds a GPU-free environment-isolation regression test. It checks shared-region memory-limit behavior across configured, environment-stripped, and poisoned-region processes. The test uses a focused non-CUDA build and runs under CTest with a 30-second timeout. ChangesEnvironment isolation test
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/CMakeLists.txt (1)
57-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRaise the CTest timeout above the test's own worst-case budget.
test_env_isolationruns four probes, and each probe allowsTEST_TIMEOUT_MSof 5000 ms. The worst case reaches 20 s plus process overhead, which matches or exceeds this 20 s CTest timeout. CTest then kills the test instead of printing the mismatch report. Use a value above the internal budget.♻️ Proposed timeout adjustment
-set_tests_properties(env_isolation PROPERTIES TIMEOUT 20) +set_tests_properties(env_isolation PROPERTIES TIMEOUT 30)🤖 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 `@test/CMakeLists.txt` around lines 57 - 59, Increase the CTest TIMEOUT for env_isolation above the test_env_isolation internal 20-second worst-case budget, allowing additional process overhead so the test can finish and report failures instead of being killed.test/test_env_isolation.c (1)
65-82: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTerminate and reap the child when the bounded wait expires.
On timeout,
wait_child_boundedreturns -1 and leaves the child running and unreaped. The child holds the shared region and the cache file, so a later probe in the same run can observe state from the stuck child. SendSIGKILLand reap before you return the error.♻️ Proposed cleanup on timeout
if (waited < 0 || now_ms() >= deadline) { + kill(child, SIGKILL); + while (waitpid(child, &status, 0) < 0 && errno == EINTR) { + } return -1; }Add the required header:
`#include` <errno.h> +#include <signal.h>🤖 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 `@test/test_env_isolation.c` around lines 65 - 82, Update wait_child_bounded so timeout or waitpid failure terminates the still-running child with SIGKILL, then reaps it with waitpid before returning -1; add the required signal header and preserve the existing successful-child behavior.
🤖 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.
Nitpick comments:
In `@test/CMakeLists.txt`:
- Around line 57-59: Increase the CTest TIMEOUT for env_isolation above the
test_env_isolation internal 20-second worst-case budget, allowing additional
process overhead so the test can finish and report failures instead of being
killed.
In `@test/test_env_isolation.c`:
- Around line 65-82: Update wait_child_bounded so timeout or waitpid failure
terminates the still-running child with SIGKILL, then reaps it with waitpid
before returning -1; add the required signal header and preserve the existing
successful-child behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2779adb7-06d4-457b-ae38-f7fccfde1cd2
📒 Files selected for processing (2)
test/CMakeLists.txttest/test_env_isolation.c
… (#2125) Adds test/test_env_isolation.c, a hardware-free characterization test for the memory-limit shared-region source of truth described in Project-HAMi/HAMi#2125. It links the production shared-region implementation directly and calls no CUDA/NVML entry point, mirroring test_postinit_owner_death.c, so it runs in CI without a GPU. The test pins get_current_device_memory_limit(0) -- the value oom_check() gates every cudaMalloc on -- across the process-startup shapes from the issue: [A] configured process, fresh region -> enforced [C] env-stripped process, first writer -> unlimited (leak) [D] configured workload joining a region an -> unlimited (leak) env-less process seeded with 0 first Assertions encode CURRENT behavior, so the test is green on today's code and documents the boundary. The two leak assertions are tagged [FIX FLIPS THIS]; whichever fix the maintainers choose flips them from 0 to the configured limit, turning this into the issue's acceptance test. This is intentionally a fix-agnostic CI test, not a fix. Signed-off-by: mohitt31 <mohitprajapati3112@gmail.com>
427a6db to
ae52f8a
Compare
|
Thanks for the review. Addressed both nitpicks in the latest push:
Rebuilt and re-ran locally — all four assertions still pass. |
|
This is being closed because it does not comply with the contribution guidelines. |
2 participants
Description
Adds a hardware-free CI characterization test for the memory-limit isolation gap in Project-HAMi/HAMi#2125.
test/test_env_isolation.clinks the production shared-region implementation directly and calls no CUDA/NVML entry point, so it runs in CI without a GPU — the same GPU-free linking pattern as the existingtest/test_postinit_owner_death.c(--gc-sectionsdrops the GPU-facing paths).It pins
get_current_device_memory_limit(0)— the valueoom_check()gates everycudaMallocon — across the process-startup shapes from the issue:Notes
[FIX FLIPS THIS]; whichever fix maintainers choose flips them from0to the configured limit, turning this into the issue's acceptance test.Repro harness (also GPU-free): https://gist.github.com/mohitt31/a6a825f429ccce3edbaafe50425fa37c
Testing
Built and run locally against the production shared-region source; all four assertions pass (
env-isolation characterization tests passed, exit 0).Summary by CodeRabbit
New Features
Tests