Kill the process tree whenever a cancelled wait ends the call [patch]

RunCommand.Test/RunCommandTests.cs

@@ -471,6 +471,91 @@
 		}
 	}
 
+	[TestMethod]
+	public async Task ExecuteAsyncShouldKillTheProcessTreeEveryTimeItIsCancelled()
+	{
+		if (!RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
+		{
+			Assert.Inconclusive("Reads a descendant's state from /proc. The kill this covers is in platform independent code, so the Linux leg covers it.");
+		}
+
+		// A token runs its callbacks newest first, and the wait registers its own after the kill's.
+		// So the wait can observe cancellation, end the call, and dispose the kill's registration
+		// before that callback has run, and the call returns with its whole tree still running. The
+		// wait only wins that race some of the time, which is why this repeats. The shape is a
+		// caller's own deadline linked into its token, which cancels on a timer thread.
+		for (int attempt = 0; attempt < 40; attempt++)
+		{
+			TaskCompletionSource<int> descendant = new(TaskCreationOptions.RunContinuationsAsynchronously);
+			OutputHandler handler = new(
+				chunk =>
+				{
+					if (int.TryParse(chunk.Trim(), out int id))
+					{
+						descendant.TrySetResult(id);
+					}
+				},
+				_ => { });
+
+			using CancellationTokenSource timeout = new(TimeSpan.FromMilliseconds(200));
+			using CancellationTokenSource linked = CancellationTokenSource.CreateLinkedTokenSource(CancellationToken.None, timeout.Token);
+
+			Task<int> execution = RunCommand.ExecuteAsync(
+				"sh",
+				["-c", "sleep 30 & echo $!; wait"],
+				handler,
+				new CommandOptions { StandardInput = StandardInputMode.Closed },
+				linked.Token);
+
+			int descendantId = await descendant.Task.WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false);
+
+			await Assert.ThrowsAsync<OperationCanceledException>(() => execution).ConfigureAwait(false);
+
+			Assert.IsTrue(
+				await HasExitedAsync(descendantId).ConfigureAwait(false),
+				$"Attempt {attempt}: descendant {descendantId} was still running after its call was cancelled.");
+		}
+	}
+
+	/// <summary>
+	/// Waits up to five seconds for a process to exit.
+	/// </summary>
+	/// <remarks>
+	/// Reads <c>/proc</c> rather than asking <see cref="Process"/>, because the process is not a child
+	/// of this one: once its parent is killed it is reparented, and it can sit as a zombie until its
+	/// new parent reaps it. A zombie has exited, so it counts as gone.
+	/// </remarks>
+	private static async Task<bool> HasExitedAsync(int processId)
+	{
+		for (int check = 0; check < 50; check++)
+		{
+			string status;
+
+			try
+			{
+				status = await File.ReadAllTextAsync($"/proc/{processId}/stat").ConfigureAwait(false);
+			}
+			catch (IOException)
+			{
+				return true;
+			}
+			catch (UnauthorizedAccessException)
+			{
+				return true;
+			}
+
+			// The state follows the parenthesised command name, which may itself contain spaces.
+			if (status[(status.LastIndexOf(')') + 2)..].StartsWith('Z'))
+			{
+				return true;
+			}
+
+			await Task.Delay(100).ConfigureAwait(false);
+		}
+
+		return false;
+	}
+
 	[TestMethod]
 	public async Task ExecuteAsyncShouldReturnWhenCancelledWhileADetachedDescendantHoldsTheOutputPipe()
 	{

RunCommand/RunCommand.cs

@@ -23,7 +23,7 @@
 	/// </summary>
 	/// <param name="command">The command to execute.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command) =>
 		ExecuteAsync(command).Result;
@@ -34,7 +34,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="outputHandler">The handler for processing command output.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command, OutputHandler outputHandler) =>
 		ExecuteAsync(command, outputHandler).Result;
@@ -45,7 +45,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="elevation">The privilege level under which to run the command.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command, Elevation elevation) =>
 		ExecuteAsync(command, elevation).Result;
@@ -61,7 +61,7 @@
 	/// </param>
 	/// <param name="elevation">The privilege level under which to run the command.</param>
 	/// <returns>The exit code of the executed process.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static int Execute(string command, OutputHandler outputHandler, Elevation elevation) =>
 		ExecuteAsync(command, outputHandler, elevation).Result;
@@ -108,7 +108,7 @@
 	/// </summary>
 	/// <param name="command">The command to execute.</param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command)
 		=> await ExecuteAsync(command, new OutputHandler()).ConfigureAwait(false);
@@ -119,7 +119,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="outputHandler">The handler for processing command output.</param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, OutputHandler outputHandler)
 		=> await ExecuteAsync(command, outputHandler, Elevation.Default).ConfigureAwait(false);
@@ -130,7 +130,7 @@
 	/// <param name="command">The command to execute.</param>
 	/// <param name="elevation">The privilege level under which to run the command.</param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, Elevation elevation)
 		=> await ExecuteAsync(command, new(), elevation).ConfigureAwait(false);
@@ -146,7 +146,7 @@
 	/// </param>
 	/// <param name="elevation">The privilege level under which to run the command.</param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, OutputHandler outputHandler, Elevation elevation)
 		=> await ExecuteAsync(command, outputHandler, elevation, CancellationToken.None).ConfigureAwait(false);
@@ -159,7 +159,7 @@
 	/// A token that, when cancelled, terminates the running process and its children.
 	/// </param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, CancellationToken cancellationToken)
 		=> await ExecuteAsync(command, new OutputHandler(), cancellationToken).ConfigureAwait(false);
@@ -174,7 +174,7 @@
 	/// A token that, when cancelled, terminates the running process and its children.
 	/// </param>
 	/// <returns>A task representing the asynchronous operation with the process exit code.</returns>
 	[Obsolete("A command string is split on its first space, which cannot handle an executable path "
 		+ "containing spaces. Use the overload taking a file name and an argument list instead.")]
 	public static async Task<int> ExecuteAsync(string command, OutputHandler outputHandler, CancellationToken cancellationToken)
 		=> await ExecuteAsync(command, outputHandler, Elevation.Default, cancellationToken).ConfigureAwait(false);
@@ -475,14 +475,27 @@
 		// would leave the child running unsupervised.
 		using CancellationTokenRegistration registration = cancellationToken.Register(() => TryKill(process));
 
-		if (useElevation)
+		try
 		{
-			await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false);
+			if (useElevation)
+			{
+				await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false);
+			}
+			else
+			{
+				using AsyncProcessStreamReader outputReader = new(process, outputHandler);
+				await Task.WhenAll(outputReader.Start(cancellationToken), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false);
+			}
 		}
-		else
+		catch (OperationCanceledException)
 		{
-			using AsyncProcessStreamReader outputReader = new(process, outputHandler);
-			await Task.WhenAll(outputReader.Start(cancellationToken), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false);
+			// The registration alone does not guarantee the kill. A token runs its callbacks newest
+			// first, and the wait registered its own after this one, so the wait can end this call
+			// and dispose the registration before the kill callback has run. That leaves the whole
+			// tree running unsupervised. Killing here as well makes it certain; a second kill of a
+			// process that has already gone is a no-op.
+			TryKill(process);
+			throw;
 		}
 
 		// Cancellation reaches the wait two ways at once: the registration above kills the process,