Merge branch 'sh-fix-popen-exit-status' into 'master'

Fix Popen not always returning error code

See merge request gitlab-org/gitlab!79564

lib/gitlab/popen.rb

@@ -10,10 +10,19 @@ module Gitlab
     Result = Struct.new(:cmd, :stdout, :stderr, :status, :duration)
 
     # Returns [stdout + stderr, status]
+    # status is either the exit code or the signal that killed the process
     def popen(cmd, path = nil, vars = {}, &block)
       result = popen_with_detail(cmd, path, vars, &block)
 
-      ["#{result.stdout}#{result.stderr}", result.status&.exitstatus]
+      # Process#waitpid returns Process::Status, which holds a 16-bit value.
+      # The higher-order 8 bits hold the exit() code (`exitstatus`).
+      # The lower-order bits holds whether the process was terminated.
+      # If the process didn't exit normally, `exitstatus` will be `nil`,
+      # but we still want a non-zero code, even if the value is
+      # platform-dependent.
+      status = result.status&.exitstatus || result.status.to_i
+
+      ["#{result.stdout}#{result.stderr}", status]
     end
 
     # Returns Result

spec/lib/gitlab/popen_spec.rb

@@ -40,6 +40,17 @@ RSpec.describe Gitlab::Popen do
     it { expect(@output).to include('No such file or directory') }
   end
 
+  context 'non-zero status with a kill' do
+    let(:cmd) { [Gem.ruby, "-e", "thr = Thread.new { sleep 5 }; Process.kill(9, Process.pid); thr.join"] }
+
+    before do
+      @output, @status = @klass.new.popen(cmd)
+    end
+
+    it { expect(@status).to eq(9) }
+    it { expect(@output).to be_empty }
+  end
+
   context 'unsafe string command' do
     it 'raises an error when it gets called with a string argument' do
       expect { @klass.new.popen('ls', path) }.to raise_error(RuntimeError)