From 22fe4f7a5913bbb56a98acd55124fee04976cab6 Mon Sep 17 00:00:00 2001 From: David Liedle Date: Sat, 9 Aug 2025 14:49:41 -0600 Subject: [PATCH] Fix process exit code error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/process/ProcessManagerImpl.cpp | 53 +++++++++++++++------ src/process/test/ProcessTests.cpp | 74 ++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 15 deletions(-) diff --git a/src/process/ProcessManagerImpl.cpp b/src/process/ProcessManagerImpl.cpp index 70b4b66245..720fc3598c 100644 --- a/src/process/ProcessManagerImpl.cpp +++ b/src/process/ProcessManagerImpl.cpp @@ -57,6 +57,38 @@ enum ProcessLifecycle namespace stellar { +// Custom error category for process exit codes +class process_exit_category_impl : public std::error_category +{ + public: + virtual const char* name() const noexcept override + { + return "process_exit"; + } + + virtual std::string message(int value) const override + { + if (value == 0) + { + return "Process exited successfully"; + } + else if (value == -1) + { + return "Process terminated by signal"; + } + else + { + return fmt::format("Process exited with code {}", value); + } + } +}; + +const std::error_category& process_exit_category() +{ + static process_exit_category_impl instance; + return instance; +} + static const asio::error_code ABORT_ERROR_CODE(asio::error::operation_aborted, asio::system_category()); @@ -66,7 +98,7 @@ mapExitStatusToErrorCode(std::string const& cmdLine, int pid, int status) // On windows, an exit status is just an exit status. On unix it's // got some flags incorporated into it. #ifdef _WIN32 - return asio::error_code(status, asio::system_category()); + return asio::error_code(status, process_exit_category()); #else if (WIFEXITED(status)) { @@ -98,23 +130,14 @@ mapExitStatusToErrorCode(std::string const& cmdLine, int pid, int status) CLOG_WARNING(Process, ""); } #endif - // FIXME: this doesn't _quite_ do the right thing; it conveys - // the exit status back to the caller but it puts it in "system - // category" which on POSIX means if you call .message() on it - // you'll get perror(value()), which is not correct. Errno has - // nothing to do with process exit values. We could make a new - // error_category to tighten this up, but it's a bunch of work - // just to convey the meaningless string "exited" to the user. - return asio::error_code(WEXITSTATUS(status), asio::system_category()); + // Use our custom process_exit_category for proper error messages + return asio::error_code(WEXITSTATUS(status), process_exit_category()); } else { - // FIXME: for now we also collapse all non-WIFEXITED exits on - // posix into a single "exit 1" error_code. This is enough - // for most callers; we can enrich it if anyone really wants - // to differentiate various signals that might have killed - // the child. - return asio::error_code(1, asio::system_category()); + // Process was terminated by a signal rather than normal exit + // Use -1 to indicate signal termination in our custom category + return asio::error_code(-1, process_exit_category()); } #endif } diff --git a/src/process/test/ProcessTests.cpp b/src/process/test/ProcessTests.cpp index aaf169ae64..6184e22acd 100644 --- a/src/process/test/ProcessTests.cpp +++ b/src/process/test/ProcessTests.cpp @@ -75,6 +75,80 @@ TEST_CASE("subprocess fails", "[process]") REQUIRE(failed); } +TEST_CASE("subprocess error messages", "[process]") +{ + VirtualClock clock; + Config const& cfg = getTestConfig(); + Application::pointer app = createTestApplication(clock, cfg); + + SECTION("successful exit") + { + bool exited = false; + asio::error_code capturedEc; + auto evt = app->getProcessManager().runProcess("true", "").lock(); + REQUIRE(evt); + evt->async_wait([&](asio::error_code ec) { + capturedEc = ec; + exited = true; + }); + + while (!exited && !clock.getIOContext().stopped()) + { + clock.crank(true); + } + + // Process should exit with code 0 + REQUIRE(capturedEc.value() == 0); + REQUIRE(capturedEc.message() == "Process exited successfully"); + REQUIRE(std::string(capturedEc.category().name()) == "process_exit"); + } + + SECTION("failed exit") + { + bool exited = false; + asio::error_code capturedEc; + auto evt = app->getProcessManager().runProcess("false", "").lock(); + REQUIRE(evt); + evt->async_wait([&](asio::error_code ec) { + capturedEc = ec; + exited = true; + }); + + while (!exited && !clock.getIOContext().stopped()) + { + clock.crank(true); + } + + // Process should exit with non-zero code + REQUIRE(capturedEc.value() != 0); + REQUIRE(capturedEc.message().find("Process exited with code") != std::string::npos); + REQUIRE(std::string(capturedEc.category().name()) == "process_exit"); + } + + SECTION("custom exit code") + { + bool exited = false; + asio::error_code capturedEc; + // Use sh -c "exit 42" to exit with specific code + auto evt = app->getProcessManager().runProcess("sh -c 'exit 42'", "").lock(); + REQUIRE(evt); + evt->async_wait([&](asio::error_code ec) { + capturedEc = ec; + exited = true; + }); + + while (!exited && !clock.getIOContext().stopped()) + { + clock.crank(true); + } + + // Process should exit with code 42 + REQUIRE(capturedEc.value() == 42); + REQUIRE(capturedEc.message() == "Process exited with code 42"); + REQUIRE(std::string(capturedEc.category().name()) == "process_exit"); + } +} + TEST_CASE("subprocess redirect to new file", "[process]") { VirtualClock clock;