fix: Convey wrapped process's exit code in metrics wrapper (openedx-unsupported#768)

timmc-edx · web-flow · commit 170e553b95f2 · 2021-06-04T13:45:38.000Z
diff --git a/scripts/send-metrics.py b/scripts/send-metrics.py
@@ -199,6 +199,8 @@ def read_git_state():
 def run_wrapped(make_target, config):
     """
     Runs specified make shell target and collects performance data.
+
+    Return exit code of process (negative for signals).
     """
     # Do as much as possible inside try blocks
     do_collect = True
@@ -219,16 +221,16 @@ def run_wrapped(make_target, config):
               file=sys.stderr)
 
     completed_process = run_target(make_target)
+    subprocess_exit_code = completed_process.returncode
 
     # Do as much as possible inside try blocks
     try:
         # Skip metrics reporting if setup failed
         if not do_collect:
-            return
+            return subprocess_exit_code
 
         signal(SIGINT, SIG_DFL)  # stop trapping SIGINT (if haven't already)
         end_time = datetime.now(timezone.utc)
-        exit_code = completed_process.returncode
         time_diff_millis = (end_time - start_time).microseconds // 1000
         # Must be compatible with our Segment schema, and must not be
         # expanded to include additional attributes without an
@@ -244,7 +246,7 @@ def run_wrapped(make_target, config):
             # https://docs.python.org/3.8/library/subprocess.html#subprocess.CompletedProcess
             #
             # If a make subprocess exits non-zero, make exits with code 2.
-            'exit_status': exit_code,
+            'exit_status': subprocess_exit_code,
             **read_git_state()
         }
         send_metrics_to_segment('devstack.command.run', event_properties, config)
@@ -258,16 +260,24 @@ def run_wrapped(make_target, config):
               "(This should not have affected the outcome of your make command.)",
               file=sys.stderr)
 
+    return subprocess_exit_code
+
 
 def run_target(make_target):
-    """Just run make on the given target."""
+    """
+    Just run make on the given target.
+
+    Return exit code of process (negative for signals).
+    """
     return subprocess.run(["make", "impl-%s" % make_target], check=False)
 
 
 def do_wrap(make_target):
     """
     Run the given make target, and collect and report data if and only if
     the user has consented to data collection.
+
+    Return exit code to exit with (signals become 128 + signal value).
     """
     try:
         consent_state = check_consent()
@@ -277,9 +287,9 @@ def do_wrap(make_target):
         consent_state = {}
 
     if consent_state.get('ok_to_collect'):
-        run_wrapped(make_target, consent_state.get('config'))
+        subprocess_exit_code = run_wrapped(make_target, consent_state.get('config'))
     else:
-        run_target(make_target)
+        subprocess_exit_code = run_target(make_target).returncode
         if consent_state.get('print_invitation'):
             print(
                 "Would you like to assist devstack development by sending "
@@ -288,6 +298,12 @@ def do_wrap(make_target):
                 file=sys.stderr
             )
 
+    if subprocess_exit_code < 0:
+        # Translate to shell convention.
+        return 128 + -subprocess_exit_code
+    else:
+        return subprocess_exit_code
+
 
 def do_opt_in():
     """
@@ -435,7 +451,8 @@ def main(args):
         if len(action_args) != 1:
             print("send-metrics wrap takes one argument", file=sys.stderr)
             sys.exit(1)
-        do_wrap(action_args[0])
+        conveyed_exit_code = do_wrap(action_args[0])
+        sys.exit(conveyed_exit_code)
     elif action == 'opt-in':
         if len(action_args) != 0:
             print("send-metrics opt-in takes zero arguments", file=sys.stderr)
diff --git a/tests/metrics.py b/tests/metrics.py
@@ -233,6 +233,11 @@ def test_no_arbitrary_target_instrumented():
         assert 'Send metrics info:' not in p.before.decode()
         assert invitation not in p.before.decode()
 
+        # Also confirm that the exit code is conveyed properly
+        p.close()
+        assert p.exitstatus == 2  # make's generic error code
+        assert p.signalstatus is None
+
 
 def test_metrics():
     """
@@ -294,3 +299,35 @@ def test_handle_ctrl_c():
 
         # Exit status is negative of signal's value (SIGINT = 2)
         assert data['properties']['exit_status'] == -2
+
+        # Exit signal here actually comes from make, so this doesn't
+        # really test the wrapper's own exit code. This assertion
+        # really just serves as documentation of behavior.
+        p.close()
+        assert p.exitstatus == None
+        assert p.signalstatus is 2
+
+
+def test_signal_conversion():
+    """
+    This is like test_handle_ctrl_c, except calling the wrapper
+    directly to avoid Make's conversion of all exit codes to 2.
+    """
+    with environment_as({'collection_enabled': True}):
+        do_opt_in()
+        p = pexpect.spawn('scripts/send-metrics.py wrap dev.pull', timeout=60)
+        # Make sure wrapped command has started before we interrupt,
+        # otherwise signal handler won't even have been registered
+        # yet.
+        p.expect(r'Are you sure you want to run this command')
+        p.send(b'\x03')  # send Ctrl-C to process group
+        # Confirm that the process is actually catching the signal, as
+        # proven by it printing some things before ending.
+        p.expect(r'Send metrics info:.*"exit_status": ?-2')
+        p.expect(EOF)
+
+        # This time we're calling the script directly, so we see the
+        # script exiting with code 130 (128 + SIGINT).
+        p.close()
+        assert p.exitstatus == 130
+        assert p.signalstatus is None
