Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe CatalystCX module and its public API were removed; platform-specific process spawning, async pipe reading, command validation, and signal utilities were refactored in headers (Windows: SearchPathW, simplified CreateProcess and async ReadFile usage; Unix: inlined waitpid and path checks). CMake EXIT_FAIL_EC changed 127→255. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Command
participant OS as OS (Win/Unix)
participant Child
participant Reader as AsyncPipeReader
Caller->>Command: Spawn()
Note right of Command: Windows: executable resolved via SearchPathW
Command->>OS: CreateProcess / posix_spawn (simplified env handling)
OS-->>Command: process handle / pid
Command-->>Caller: Child
Caller->>Child: Wait(optional timeout)
par Read stdout/stderr
Child->>Reader: ReadPipes(stdout_handle, stderr_handle)
loop until both streams closed
Reader->>OS: ReadFile / read
alt data available
OS-->>Reader: bytes
Reader-->>Reader: append buffer
else EOF or error
Reader-->>Reader: mark finished
end
end
Reader-->>Child: {stdout, stderr}
and Process termination
Child->>OS: WaitForSingleObject / waitpid
OS-->>Child: exit code / timeout
end
Child-->>Caller: CommandResult (stdout, stderr, status)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
CatalystCX.hpp (2)
85-101: Child is copyable but owns OS handles ⇒ double-close/use-after-close.
Returning std::optional by value copies Child (move is suppressed by user-declared dtor), so the temporary and the local both own the same handles. This is a correctness/timebomb bug.Apply this diff to make Child move-only and safe (Windows + POSIX):
class Child { public: +#if 1 + Child(const Child&) = delete; + Child& operator=(const Child&) = delete; + Child(Child&& other) noexcept +#ifdef _WIN32 + : ProcessId(other.ProcessId), + ProcessHandle(other.ProcessHandle), + ThreadHandle(other.ThreadHandle), + StdoutHandle(other.StdoutHandle), + StderrHandle(other.StderrHandle), + PipesClosed(other.PipesClosed) { + other.ProcessId = 0; + other.ProcessHandle = INVALID_HANDLE_VALUE; + other.ThreadHandle = INVALID_HANDLE_VALUE; + other.StdoutHandle = INVALID_HANDLE_VALUE; + other.StderrHandle = INVALID_HANDLE_VALUE; + other.PipesClosed = true; + } + Child& operator=(Child&& other) noexcept { + if (this != &other) { + if (ProcessHandle != INVALID_HANDLE_VALUE) CloseHandle(ProcessHandle); + if (ThreadHandle != INVALID_HANDLE_VALUE) CloseHandle(ThreadHandle); + if (!PipesClosed) { + if (StdoutHandle != INVALID_HANDLE_VALUE) CloseHandle(StdoutHandle); + if (StderrHandle != INVALID_HANDLE_VALUE) CloseHandle(StderrHandle); + } + ProcessId = other.ProcessId; + ProcessHandle = other.ProcessHandle; + ThreadHandle = other.ThreadHandle; + StdoutHandle = other.StdoutHandle; + StderrHandle = other.StderrHandle; + PipesClosed = other.PipesClosed; + other.ProcessId = 0; + other.ProcessHandle = INVALID_HANDLE_VALUE; + other.ThreadHandle = INVALID_HANDLE_VALUE; + other.StdoutHandle = INVALID_HANDLE_VALUE; + other.StderrHandle = INVALID_HANDLE_VALUE; + other.PipesClosed = true; + } + return *this; + } +#else + : ProcessId(other.ProcessId), + StdoutFd(other.StdoutFd), + StderrFd(other.StderrFd) { + other.ProcessId = 0; + other.StdoutFd = -1; + other.StderrFd = -1; + } + Child& operator=(Child&& other) noexcept { + if (this != &other) { + if (StdoutFd >= 0) ::close(StdoutFd); + if (StderrFd >= 0) ::close(StderrFd); + ProcessId = other.ProcessId; + StdoutFd = other.StdoutFd; + StderrFd = other.StderrFd; + other.ProcessId = 0; + other.StdoutFd = -1; + other.StderrFd = -1; + } + return *this; + } +#endif +#endif private: - pid_t ProcessId; + pid_t ProcessId; #ifdef _WIN32 HANDLE ProcessHandle; HANDLE ThreadHandle; HANDLE StdoutHandle; HANDLE StderrHandle; mutable bool PipesClosed; #else - int StdoutFd; - int StderrFd; + mutable int StdoutFd; + mutable int StderrFd; #endifAdditionally, on POSIX add a small dtor to avoid FD leaks when Wait() isn’t called:
#else Child(const pid_t pid, const int stdout_fd, const int stderr_fd) : ProcessId(pid), StdoutFd(stdout_fd), StderrFd(stderr_fd) {} + ~Child() { + if (StdoutFd >= 0) ::close(StdoutFd); + if (StderrFd >= 0) ::close(StderrFd); + } #endifAnd in POSIX Wait(), set FDs to -1 after closing (they’re mutable):
close(StdoutFd); close(StderrFd); + StdoutFd = -1; + StderrFd = -1;Also applies to: 116-127
433-446: POSIX: double-wait bug (waitpid reaps child, then wait4).
You call waitpid(WNOHANG) and, on success, call wait4 which can fail with ECHILD. Use wait4(WNOHANG) in the polling loop and avoid a second reap.Apply this diff:
- while (std::chrono::steady_clock::now() < timeout_time) { - const int wait_result = waitpid(ProcessId, &status, WNOHANG); - if (wait_result == ProcessId) { - wait4(ProcessId, &status, 0, &usage); // Get resource usage - break; // Process finished - } + while (std::chrono::steady_clock::now() < timeout_time) { + const pid_t wait_result = wait4(ProcessId, &status, WNOHANG, &usage); + if (wait_result == ProcessId) { + break; // Process finished (status+usage populated) + }And in the timeout block keep the existing wait4 after Kill().
Also applies to: 451-460
module/CatalystCX.cppm (3)
278-395: Windows Spawn/handles: OK, but still at risk due to Child copyability.
Fix Child move-only semantics here too (same as header), otherwise handles can be double-closed/invalidated.Apply the same move-only changes to the class in this module (declarations at Lines 90-121; definitions mirror header changes).
508-546: POSIX: double-wait bug here too.
Same issue as header: swap to wait4(WNOHANG) in the polling loop.Apply the same wait4-loop diff as proposed for the header.
595-709: POSIX Spawn FD lifecycle: add dtor and mark FDs mutable.
Without a dtor, dropping Child without Wait() leaks FDs. Also set FDs to -1 in Wait().Mirror the header changes: add ~Child() closing FDs; mark StdoutFd/StderrFd mutable; set to -1 after closing in Wait().
🧹 Nitpick comments (2)
CatalystCX.hpp (1)
668-675: POSIX ReadFromPipe returns true on -1 (EAGAIN).
This makes the loop think data was read when none was. Return bytes_read > 0.Apply this diff:
- return bytes_read != 0; + return bytes_read > 0;module/CatalystCX.cppm (1)
712-752: POSIX ReadFromPipe should return bytes_read > 0.
Current logic treats -1 as success.Apply this diff:
-bool AsyncPipeReader::ReadFromPipe(PipeData& pipe_data, std::array<char, 8192>& buffer) { +bool AsyncPipeReader::ReadFromPipe(PipeData& pipe_data, std::array<char, 8192>& buffer) { const ssize_t bytes_read = read(pipe_data.Fd, buffer.data(), buffer.size()); if (bytes_read > 0) { pipe_data.Buffer.append(buffer.data(), bytes_read); return true; } - return bytes_read != 0; + return false; }Also applies to: 753-760
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CatalystCX.hpp(4 hunks)module/CatalystCX.cppm(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Performance Benchmarks
- GitHub Check: Static Analysis
🔇 Additional comments (2)
CatalystCX.hpp (1)
28-28: Good addition: public header needs .
Keeps SignalInfo declarations portable.module/CatalystCX.cppm (1)
233-276: Inline Child::Wait (Windows): OK assuming pipe reader is fixed.
Once ReadPipes is made concurrent, this looks fine.Please re-run a stress test that writes >8KB to both stdout and stderr concurrently to validate no deadlocks.
1 participant
Summary by CodeRabbit
Breaking Changes
Refactor
Build
Reliability
Performance