Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens stress-process cleanup by isolating stress runs into their own process group and ensuring the process tree is terminated on exit/signals.
Changes:
- Start stress subprocesses in a new session/process group (
os.setsid) to improve kill reliability. - Extend shutdown handling (SIGTERM +
atexit) to ensure stress processes are cleaned up when the app exits. - Rework
kill_child_processesto attempt graceful termination (process group SIGTERM → per-proc terminate → SIGKILL) and add corresponding tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
s_tui/s_tui.py |
Adds SIGTERM handling, atexit cleanup, and launches stress with process-group isolation. |
s_tui/helper_functions.py |
Reworks stress process-tree termination logic to be more robust and time-bounded. |
tests/test_stress_controller.py |
Adds test ensuring stress start uses preexec_fn=os.setsid. |
tests/test_helper_functions.py |
Adds tests for new process-group-first termination behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| import psutil | ||
| import pytest | ||
| from unittest.mock import MagicMock, patch, PropertyMock |
There was a problem hiding this comment.
PropertyMock is imported but not used in this test module. Please remove the unused import to keep the test file clean and avoid linting failures in stricter environments.
| from unittest.mock import MagicMock, patch, PropertyMock | |
| from unittest.mock import MagicMock, patch |
| stress_proc = subprocess.Popen( | ||
| stress_cmd, stdout=dev_null, stderr=dev_null | ||
| stress_cmd, | ||
| stdout=dev_null, | ||
| stderr=dev_null, | ||
| preexec_fn=os.setsid, | ||
| ) | ||
| self.set_stress_process(psutil.Process(stress_proc.pid)) | ||
| except OSError: |
There was a problem hiding this comment.
subprocess.Popen(..., preexec_fn=os.setsid) can raise ValueError on platforms where preexec_fn isn't supported (and potentially subprocess.SubprocessError if the preexec_fn fails). Right now only OSError is caught, so start_stress may crash instead of logging and continuing. Consider catching ValueError as well (or on Python 3 using start_new_session=True instead of preexec_fn).
No description provided.