From a380fe7a14b297cd1a37749df74b8392d9b48da2 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Thu, 15 Feb 2024 10:51:56 -0800 Subject: [PATCH 1/3] chore: add failing test for orphan process --- crates/turborepo-lib/src/process/child.rs | 32 +++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/crates/turborepo-lib/src/process/child.rs b/crates/turborepo-lib/src/process/child.rs index 9c92c62571036..c335a42d085c4 100644 --- a/crates/turborepo-lib/src/process/child.rs +++ b/crates/turborepo-lib/src/process/child.rs @@ -1170,6 +1170,38 @@ mod test { assert_matches!(exit, Some(ChildExit::Killed)); } + #[cfg(unix)] + #[tokio::test] + async fn test_orphan_process() { + let mut cmd = Command::new("sh"); + cmd.args(["-c", "echo hello; sleep 120; echo done"]); + let mut child = Child::spawn(cmd, ShutdownStyle::Kill, false).unwrap(); + + tokio::time::sleep(STARTUP_DELAY).await; + + let child_pid = child.pid().unwrap() as i32; + // We don't kill the process group to simulate what an external program might do + unsafe { + libc::kill(child_pid, libc::SIGKILL); + } + + let exit = child.wait().await; + assert_matches!(exit, Some(ChildExit::KilledExternal)); + + let mut output = Vec::new(); + match tokio::time::timeout( + Duration::from_millis(500), + child.wait_with_piped_outputs(&mut output), + ) + .await + { + Ok(exit_status) => { + assert_matches!(exit_status, Ok(Some(ChildExit::KilledExternal))); + } + Err(_) => panic!("expected wait_with_piped_outputs to exit after it was killed"), + } + } + #[test_case(false)] #[test_case(TEST_PTY)] #[tokio::test] From b6eb1572253d8d44baece66638a7061e6ae085fe Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Thu, 15 Feb 2024 11:19:17 -0800 Subject: [PATCH 2/3] fix: make sure multiple waits return the actual exit status --- crates/turborepo-lib/src/process/child.rs | 18 +++++++++++++++++- crates/turborepo-lib/src/process/mod.rs | 2 +- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/crates/turborepo-lib/src/process/child.rs b/crates/turborepo-lib/src/process/child.rs index c335a42d085c4..4312c4bcc9c2a 100644 --- a/crates/turborepo-lib/src/process/child.rs +++ b/crates/turborepo-lib/src/process/child.rs @@ -500,7 +500,8 @@ impl Child { /// Wait for the `Child` to exit, returning the exit code. pub async fn wait(&mut self) -> Option { - self.exit_channel.changed().await.ok()?; + // If sending end of exit channel closed, then return last value in the channel + self.exit_channel.changed().await.ok(); *self.exit_channel.borrow() } @@ -827,6 +828,21 @@ mod test { assert_matches!(&*state, ChildState::Exited(ChildExit::Killed)); } + #[test_case(false)] + #[test_case(TEST_PTY)] + #[tokio::test] + async fn test_wait(use_pty: bool) { + let script = find_script_dir().join_component("hello_world.js"); + let mut cmd = Command::new("node"); + cmd.args([script.as_std_path()]); + let mut child = Child::spawn(cmd, ShutdownStyle::Kill, use_pty).unwrap(); + + let exit1 = child.wait().await; + let exit2 = child.wait().await; + assert_matches!(exit1, Some(ChildExit::Finished(Some(0)))); + assert_matches!(exit2, Some(ChildExit::Finished(Some(0)))); + } + #[test_case(false)] #[test_case(TEST_PTY)] #[tokio::test] diff --git a/crates/turborepo-lib/src/process/mod.rs b/crates/turborepo-lib/src/process/mod.rs index c7a8b510a8d15..9708519596be6 100644 --- a/crates/turborepo-lib/src/process/mod.rs +++ b/crates/turborepo-lib/src/process/mod.rs @@ -264,7 +264,7 @@ mod test { assert_eq!(kill_code, Some(ChildExit::Finished(Some(0)))); let code = child.wait().await; - assert_eq!(code, None); + assert_eq!(code, Some(ChildExit::Finished(Some(0)))); } #[tokio::test] From 14be6607039047d2ac67633482cc144d77db4643 Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Thu, 15 Feb 2024 11:24:02 -0800 Subject: [PATCH 3/3] fix: stop reading stdout/stderr if child exited with non-zero --- crates/turborepo-lib/src/process/child.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/turborepo-lib/src/process/child.rs b/crates/turborepo-lib/src/process/child.rs index 4312c4bcc9c2a..12c8e2d351bda 100644 --- a/crates/turborepo-lib/src/process/child.rs +++ b/crates/turborepo-lib/src/process/child.rs @@ -672,6 +672,7 @@ impl Child { let mut stdout_buffer = Vec::new(); let mut stderr_buffer = Vec::new(); + let mut is_exited = false; loop { tokio::select! { Some(result) = next_line(&mut stdout_lines, &mut stdout_buffer) => { @@ -686,6 +687,14 @@ impl Child { stdout_pipe.write_all(&stderr_buffer)?; stderr_buffer.clear(); } + status = self.wait(), if !is_exited => { + is_exited = true; + // We don't abort in the cases of a zero exit code as we could be + // caching this task and should read all the logs it produces. + if status != Some(ChildExit::Finished(Some(0))) { + return Ok(status); + } + } else => { // In the case that both futures read a complete line // the future not chosen in the select will return None if it's at EOF