-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix process exit code error handling #4876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix process exit code error handling #4876
Conversation
This commit addresses the long-standing FIXME comments in ProcessManagerImpl.cpp by implementing a custom error category for process exit codes. Previously, exit codes were incorrectly placed in the system_category, which would produce misleading error messages when calling .message() on the error code. Changes: - Added process_exit_category_impl class that properly categorizes process exit codes - Implemented meaningful error messages for different exit scenarios: - "Process exited successfully" for exit code 0 - "Process terminated by signal" for signal-terminated processes - "Process exited with code N" for non-zero exit codes - Updated both Windows and POSIX code paths to use the new category - Added comprehensive tests to verify the error messages and category This improves debugging and error reporting when subprocess execution fails, making it easier for users and developers to understand what went wrong. Fixes the FIXME comments at lines 101-107 and 112-116. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a custom error category for process exit codes to replace the incorrect use of system_category, providing meaningful error messages for different process termination scenarios.
- Added a
process_exit_category_implclass with appropriate error messages for successful exits, signal terminations, and exit codes - Updated both Windows and POSIX code paths to use the new error category instead of system_category
- Added comprehensive tests to verify error messages and category names for different exit scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/process/ProcessManagerImpl.cpp | Implements custom error category and updates exit code handling to use it |
| src/process/test/ProcessTests.cpp | Adds tests for verifying error messages and category for different exit scenarios |
| // Use sh -c "exit 42" to exit with specific code | ||
| auto evt = app->getProcessManager().runProcess("sh -c 'exit 42'", "").lock(); |
Copilot
AI
Aug 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using shell command with single quotes inside double quotes may cause issues on Windows. Consider using a more portable approach or adding platform-specific test sections.
| // Use sh -c "exit 42" to exit with specific code | |
| auto evt = app->getProcessManager().runProcess("sh -c 'exit 42'", "").lock(); | |
| // Use platform-specific command to exit with code 42 | |
| #ifdef _WIN32 | |
| std::string exitCmd = "cmd /C exit 42"; | |
| #else | |
| std::string exitCmd = "sh -c \"exit 42\""; | |
| #endif | |
| auto evt = app->getProcessManager().runProcess(exitCmd, "").lock(); |
| { | ||
| return "Process exited successfully"; | ||
| } | ||
| else if (value == -1) |
Copilot
AI
Aug 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic number -1 for signal termination should be defined as a named constant to improve code readability and maintainability.
| else if (value == -1) | |
| else if (value == PROCESS_TERMINATED_BY_SIGNAL) |
| // Use -1 to indicate signal termination in our custom category | ||
| return asio::error_code(-1, process_exit_category()); |
Copilot
AI
Aug 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic number -1 for signal termination should be defined as a named constant to match the usage in the error message function and improve consistency.
| // Use -1 to indicate signal termination in our custom category | |
| return asio::error_code(-1, process_exit_category()); | |
| // Use PROCESS_TERMINATED_BY_SIGNAL to indicate signal termination in our custom category | |
| return asio::error_code(PROCESS_TERMINATED_BY_SIGNAL, process_exit_category()); |
This commit addresses the long-standing FIXME comments in ProcessManagerImpl.cpp by implementing a custom error category for process exit codes. Previously, exit codes were incorrectly placed in the system_category, which would produce misleading error messages when calling .message() on the error code.
Changes:
This improves debugging and error reporting when subprocess execution fails, making it easier for users and developers to understand what went wrong.
Fixes the FIXME comments at lines 101-107 and 112-116.
🤖 Generated with Claude Code