Skip to content

Improve output capture for short-lived processes in System module #533

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.

Sign up for GitHub

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

Open
GoodForOneFare wants to merge 1 commit into
base: main
Choose a base branch
from
+21 −6
Open

Improve output capture for short-lived processes in System module #533

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions lib/cli/kit/system.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,23 +157,38 @@ def system(cmd, *args, sudo: false, env: ENV.to_h, stdin: nil, **kwargs, &block)
}
end

process_reaped = false
previous_trailing = Hash.new('')
loop do
break if Process.wait(pid, Process::WNOHANG)
ios = [err_r, out_r]

ios = [err_r, out_r].reject(&:closed?)
next if ios.empty?
loop do
# Break only when child exited AND there are no more open pipes
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
Copy link
Contributor

@donk-shopify donk-shopify Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redo this comment in context with the new implementation so that readers understand why the readers.each loop needs to be attempted.


readers.each do |io|
readers&.each do |io|
data, trailing = split_partial_characters(io.readpartial(4096))
handlers[io].call(previous_trailing[io] + data)
previous_trailing[io] = trailing
rescue IOError
# Pipe closed - this is expected when process exits
io.close
ios.delete(io)
Copy link
Contributor

@donk-shopify donk-shopify Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. IO.select timeout
  2. Skip the readers loop
  3. Again determine that process_reaped should be true
  4. 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

# Non-blocking reap of child; only do it once
process_reaped ||= !Process.wait(pid, Process::WNOHANG).nil?
end

# Ensure we've fully reaped the child (if WNOHANG never caught it)
Process.wait(pid) unless process_reaped
Copy link
Contributor

@donk-shopify donk-shopify Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Flush any remaining trailing fragments so nothing is lost
previous_trailing.each do |io, trailing|
next if trailing.empty?

handlers[io].call(trailing)
end

$CHILD_STATUS
Expand Down
Loading