Improve output capture for short-lived processes in System module #533
Conversation
Updates the system method to reliably capture all output from processes that exit quickly, before all their output has been read. The previous implementation could miss output if the process exited between IO checks. Key changes: - Use IO.select with timeout to efficiently wait for data - Non-blocking process reaping with Process::WNOHANG - Loop continues until both pipes are closed AND process is reaped - Final flush of any trailing partial UTF-8 characters Also fixes RuboCop warnings: - Use safe navigation operator for readers check - Remove EOFError from rescue (IOError is the parent class) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
donk-shopify
left a comment
donk-shopify
left a comment
There was a problem hiding this comment.
I've left a few timing related questions based on my reading of IO.select, readpartial.
| break if ios.empty? && process_reaped | ||
|
|
||
| readers, = IO.select(ios, [], [], 1) | ||
| next if readers.nil? # If IO.select times out we iterate again so we can check if the process has exited |
There was a problem hiding this comment.
Redo this comment in context with the new implementation so that readers understand why the readers.each loop needs to be attempted.
| rescue IOError | ||
| # Pipe closed - this is expected when process exits | ||
| io.close | ||
| ios.delete(io) |
There was a problem hiding this comment.
After the process has exited, the size of this array controls the loop exit. If IO.select continues to timeout, this code won't be reached. It looks like there's a possibility of:
- IO.select timeout
- Skip the readers loop
- Again determine that
process_reapedshould be true - Repeat
I guess this depends on what IO.select does when process_reaped is true. It seems like it might raise EOFError or Errno::EPIPE. Perhaps just handling those would be a good way to exit the loop.
I'm not convinced that this could happen. If it did occur, then the next if early loop control would've handled it. Perhaps unintentionally.
Since this condition is logically possible, the new implementation should also handle it. Preferably explicitly.
| end | ||
|
|
||
| # Ensure we've fully reaped the child (if WNOHANG never caught it) | ||
| Process.wait(pid) unless process_reaped |
There was a problem hiding this comment.
The loop isn't exited until process_reaped is true.
If the loop somehow did exit with a live process that needs waiting on, there's the potential for data to appear on the pipes. Outside the loop, nothing is calling readpartial. The lingering data from the loop should be in previous_trailing but new data on the pipes (assuming the process is live) seems to be lost.
2 participants
Updates the system method to reliably capture all output from processes that exit quickly, before all their output has been read. The previous implementation could miss output if the process exited between IO checks.
Key changes:
Also fixes RuboCop warnings:
🤖 Generated with Claude Code