Merge pull request #6061 from cre4ture/fix/flaky_timeout_kill_subprocess

sylvestre · web-flow · commit f89cfe2a5eea · 2024-03-13T11:13:09.000+01:00
Fix/flaky timeout kill subprocess
diff --git a/src/uu/timeout/src/timeout.rs b/src/uu/timeout/src/timeout.rs
@@ -202,14 +202,15 @@ fn send_signal(process: &mut Child, signal: usize, foreground: bool) {
     // NOTE: GNU timeout doesn't check for errors of signal.
     // The subprocess might have exited just after the timeout.
     // Sending a signal now would return "No such process", but we should still try to kill the children.
-    _ = process.send_signal(signal);
-    if !foreground {
-        _ = process.send_signal_group(signal);
-        let kill_signal = signal_by_name_or_value("KILL").unwrap();
-        let continued_signal = signal_by_name_or_value("CONT").unwrap();
-        if signal != kill_signal && signal != continued_signal {
-            _ = process.send_signal(continued_signal);
-            _ = process.send_signal_group(continued_signal);
+    match foreground {
+        true => _ = process.send_signal(signal),
+        false => {
+            _ = process.send_signal_group(signal);
+            let kill_signal = signal_by_name_or_value("KILL").unwrap();
+            let continued_signal = signal_by_name_or_value("CONT").unwrap();
+            if signal != kill_signal && signal != continued_signal {
+                _ = process.send_signal_group(continued_signal);
+            }
         }
     }
 }
@@ -342,8 +343,15 @@ fn timeout(
             send_signal(process, signal, foreground);
             match kill_after {
                 None => {
+                    let status = process.wait()?;
                     if preserve_status {
-                        Err(ExitStatus::SignalSent(signal).into())
+                        if let Some(ec) = status.code() {
+                            Err(ec.into())
+                        } else if let Some(sc) = status.signal() {
+                            Err(ExitStatus::SignalSent(sc.try_into().unwrap()).into())
+                        } else {
+                            Err(ExitStatus::CommandTimedOut.into())
+                        }
                     } else {
                         Err(ExitStatus::CommandTimedOut.into())
                     }
diff --git a/tests/by-util/test_timeout.rs b/tests/by-util/test_timeout.rs
@@ -93,6 +93,18 @@ fn test_preserve_status() {
         .no_stdout();
 }
 
+#[test]
+fn test_preserve_status_even_when_send_signal() {
+    // When sending CONT signal, process doesn't get killed or stopped.
+    // So, expected result is success and code 0.
+    new_ucmd!()
+        .args(&["-s", "CONT", "--preserve-status", ".1", "sleep", "5"])
+        .succeeds()
+        .code_is(0)
+        .no_stderr()
+        .no_stdout();
+}
+
 #[test]
 fn test_dont_overflow() {
     new_ucmd!()
@@ -151,10 +163,10 @@ fn test_kill_subprocess() {
             "10",
             "sh",
             "-c",
-            "sh -c \"trap 'echo xyz' TERM; sleep 30\"",
+            "trap 'echo inside_trap' TERM; sleep 30",
         ])
         .fails()
         .code_is(124)
-        .stdout_contains("xyz")
+        .stdout_contains("inside_trap")
         .stderr_contains("Terminated");
 }
