Fix: Enforce memory limit on macOS and enable C/C++ tests#170
Fix: Enforce memory limit on macOS and enable C/C++ tests#170google-labs-jules[bot] wants to merge 1 commit into
Conversation
The `process_control` crate does not enforce memory limits on macOS, causing tests to be skipped on this platform. This commit introduces a macOS-specific implementation using the `rlimit` crate. A `pre_exec` hook is used to set the `RLIMIT_AS` resource limit on the child process, which is the correct way to enforce memory constraints on Unix-like systems. The signal handling logic is also updated to correctly interpret `SIGKILL` (signal 9) as a "Memory Limit Exceeded" error on macOS. Additionally, this commit: - Enables the previously disabled C and C++ memory limit tests on macOS. - Introduces test isolation by using unique directories for test runs, preventing interference between parallel tests.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This PR implements platform-specific memory limit enforcement for macOS by using the rlimit crate to set resource limits via a pre_exec hook, replacing the non-functional process_control approach. The signal handling was updated to correctly interpret SIGKILL as a memory limit exceeded error on macOS.
Changes:
- Added
rlimitdependency and implemented macOS-specific memory limit enforcement usingRLIMIT_AS - Updated signal handling to treat
SIGKILL(signal 9) as MLE on macOS - Enabled previously disabled C/C++ memory limit tests on macOS with isolated test folders
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Added rlimit dependency for macOS memory limit enforcement |
| src/runner/cmd.rs | Implemented macOS-specific memory limit using pre_exec hook and updated signal handling |
| tests/util/test_constants.rs | Added missing stdlib.h include to C MLE test code |
| tests/util/test_command_handler.rs | Added folder parameter to stress test functions for test isolation |
| tests/cli/stress_subcommand/mod.rs | Added c_mle module declaration |
| tests/cli/stress_subcommand/cpp_mle.rs | Removed macOS skip condition and added unique folder names for test isolation |
| tests/cli/stress_subcommand/codes.rs | Added C stress test generator and target code |
| tests/cli/stress_subcommand/c_mle.rs | New test file for C memory limit exceeded scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let cases: usize = 3; | ||
|
|
||
| let mut cmd = Command::new(BINARY); | ||
| execute_command_stress_with_timeout(&mut cmd, TARGET_FILE_C, GEN_FILE_C, cases, 5000usize, folder); |
There was a problem hiding this comment.
The timeout value 5000usize is duplicated across multiple test functions. Consider extracting this as a constant (e.g., const MLE_TEST_TIMEOUT: usize = 5000;) to improve maintainability and make it easier to adjust the timeout for all MLE tests consistently.
| let cases: usize = 3; | ||
|
|
||
| let mut cmd = Command::new(BINARY); | ||
| execute_command_stress_with_timeout(&mut cmd, TARGET_FILE_C, GEN_FILE_C, cases, 5000usize, folder); |
There was a problem hiding this comment.
The timeout value 5000usize is duplicated across multiple test functions. Consider extracting this as a constant (e.g., const MLE_TEST_TIMEOUT: usize = 5000;) to improve maintainability and make it easier to adjust the timeout for all MLE tests consistently.
This pull request fixes an issue where memory limit enforcement was not working on macOS. The
process_controlcrate does not support this feature on macOS, so a platform-specific solution was implemented using therlimitcrate to set theRLIMIT_ASresource limit in apre_exechook.The signal handling has also been updated to correctly interpret
SIGKILL(signal 9) as a "Memory Limit Exceeded" error on macOS.With this fix, the C and C++ memory limit tests are now enabled and passing on macOS.
To verify the changes, you can run the following commands:
cargo test --test mod cli::stress_subcommand::c_mlecargo test --test mod cli::stress_subcommand::cpp_mlePR created automatically by Jules for task 16864974441326449042 started by @LuchoBazz