这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@anthonyshew
Copy link
Contributor

@anthonyshew anthonyshew commented Nov 10, 2025

Description

nextest was showing how these tests could leak, since proc wouldn't get killed on drop. Now they will.

Testing Instructions

CI

CLOSES TURBO-4958

@anthonyshew anthonyshew requested a review from a team as a code owner November 10, 2025 05:37
@vercel
Copy link
Contributor

vercel bot commented Nov 10, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
examples-basic-web Ready Ready Preview Comment Nov 10, 2025 2:31pm
examples-designsystem-docs Building Building Preview Comment Nov 10, 2025 2:31pm
examples-gatsby-web Ready Ready Preview Comment Nov 10, 2025 2:31pm
examples-kitchensink-blog Ready Ready Preview Comment Nov 10, 2025 2:31pm
examples-nonmonorepo Ready Ready Preview Comment Nov 10, 2025 2:31pm
examples-svelte-web Ready Ready Preview Comment Nov 10, 2025 2:31pm
examples-tailwind-web Ready Ready Preview Comment Nov 10, 2025 2:31pm
examples-vite-web Ready Ready Preview Comment Nov 10, 2025 2:31pm
turbo-site Ready Ready Preview Comment Nov 10, 2025 2:31pm

@anthonyshew anthonyshew changed the title ci: fix leaky test ci: Fix leaky test Nov 10, 2025
Copy link
Contributor

@vercel vercel bot left a comment

Choose a reason for hiding this comment

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

Additional Suggestion:

The handles_kill_dead_server test spawns a process without .kill_on_drop(true), while the similar handles_kill_dead_server_wrong_process test includes this setting to fix the process leak. Both tests should be fixed consistently.

View Details
📝 Patch Details
diff --git a/crates/turborepo-lib/src/daemon/connector.rs b/crates/turborepo-lib/src/daemon/connector.rs
index a418cd41d..88a198432 100644
--- a/crates/turborepo-lib/src/daemon/connector.rs
+++ b/crates/turborepo-lib/src/daemon/connector.rs
@@ -562,6 +562,7 @@ mod test {
             .stderr(Stdio::null())
             .arg("-e")
             .arg("setInterval(() => {}, 1000)")
+            .kill_on_drop(true)
             .spawn()
             .unwrap();
 

Analysis

Process leak in handles_kill_dead_server test due to missing kill_on_drop(true)

What fails: The handles_kill_dead_server test spawns a Node.js child process without setting .kill_on_drop(true), causing the process to remain alive after the test completes while handles_kill_dead_server_wrong_process properly cleans up with .kill_on_drop(true).

How to reproduce:

  1. Run the test: cargo test --lib daemon::connector::handles_kill_dead_server
  2. The spawned Node.js process with setInterval(() => {}, 1000) will not be terminated when the test ends since tokio::process::Child defaults to kill_on_drop=false
  3. Without explicit cleanup, the process persists after test completion

What happens vs expected: Without .kill_on_drop(true), when proc goes out of scope at the end of the test, the underlying Node.js process continues running. The similar test handles_kill_dead_server_wrong_process (line 522) includes .kill_on_drop(true) for proper cleanup. Per Tokio documentation, the default value is false, meaning "spawned processes will not be killed on drop, similar to the behavior of the standard library."

Both tests spawn long-running Node processes for testing daemon process management. Both should ensure these test processes are properly cleaned up when the test scope exits. The kill_on_drop(true) setting ensures SIGKILL is sent when the Child handle is dropped, guaranteeing process termination regardless of whether the process received SIGTERM earlier.

Fix: Added .kill_on_drop(true) to the handles_kill_dead_server test to match the pattern in handles_kill_dead_server_wrong_process and ensure proper process cleanup.

Fix on Vercel

vercel bot and others added 2 commits November 10, 2025 14:21
…ill_on_drop(true)`\, while the similar `handles_kill_dead_server_wrong_process` test includes this setting to fix the process leak\. Both tests should be fixed consistently\.
@anthonyshew anthonyshew changed the title ci: Fix leaky test ci: Fix leaky tests Nov 10, 2025
@anthonyshew anthonyshew merged commit 40b1976 into main Nov 10, 2025
164 of 166 checks passed
@anthonyshew anthonyshew deleted the shew/89bde branch November 10, 2025 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants