Skip to content

Commit aab7b73

Browse files
LukeButtersclaude
LukeButters
and
claude
committed
Unblock WaitForExit when a grandchild inherits redirected pipes
SilentProcessRunner.ExecuteCommand could hang indefinitely on cancellation when the launched process spawned a grandchild that inherited the redirected stdout/stderr handles and outlived the immediate child (observed in a real dump: PowerShell -> AutoReport spawning a child and exiting). Process.Kill(true) walks descendants by ParentProcessId — once the intermediate parent dies, the grandchild is no longer reachable from the walk, so the pipes stay open and the parameterless WaitForExit() blocks forever waiting on the async readers to drain. On cancellation, after the kill, close the Process's redirected streams so the in-flight ReadAsync calls throw and the AsyncStreamReader signals EOF, letting WaitForExit return. Gated on asyncReadsStarted so the early-cancel path (before BeginOutputReadLine) is unaffected. Adds a Windows integration test that reproduces the hang via a PowerShell -> cmd -> ping chain where cmd exits after `start /b` returns, breaking the PPID chain and bypassing Kill(true)'s descendant walk. The test guards against a true infinite hang by wrapping the ExecuteCommand call in Task.Wait(30s). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent fe379de commit aab7b73

2 files changed

Lines changed: 227 additions & 1 deletion

File tree

‎source/Octopus.Tentacle.Core/Util/CommandLine/SilentProcessRunner.cs‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,17 +128,30 @@ void WriteData(Action<string> action, ManualResetEventSlim resetEvent, DataRecei
128128
process.Start();
129129

130130
var running = true;
131+
var asyncReadsStarted = false;
131132

132133
using (cancel.Register(() =>
133134
{
134-
if (running) DoOurBestToCleanUp(process, error);
135+
if (!running) return;
136+
DoOurBestToCleanUp(process, error);
137+
// Close our end of the redirected stdout/stderr pipes
138+
// so the parameterless Process.WaitForExit() — which
139+
// waits for the async readers to drain — can return
140+
// even when a grandchild process inherited the
141+
// write-ends and outlives Kill. Without this, the
142+
// active ReadAsync calls never see EOF and the call
143+
// hangs forever. Only valid once BeginOutputReadLine
144+
// has wired up the AsyncStreamReaders.
145+
if (Volatile.Read(ref asyncReadsStarted))
146+
try { process.Close(); } catch (Exception ex) { error($"Failed to close process resources: {ex.Message}"); }
135147
}))
136148
{
137149
if (cancel.IsCancellationRequested)
138150
DoOurBestToCleanUp(process, error);
139151

140152
process.BeginOutputReadLine();
141153
process.BeginErrorReadLine();
154+
Volatile.Write(ref asyncReadsStarted, true);
142155

143156
process.WaitForExit();
144157

‎source/Octopus.Tentacle.Tests.Integration/Util/SilentProcessRunnerFixture.cs‎

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
using System;
2+
using System.Diagnostics;
3+
using System.IO;
24
using System.Text;
35
using System.Threading;
6+
using System.Threading.Tasks;
47
using FluentAssertions;
58
using NUnit.Framework;
69
using Octopus.Tentacle.Tests.Integration.Support;
@@ -118,6 +121,216 @@ public void CancellationToken_ShouldForceKillTheProcess()
118121
}
119122
}
120123

124+
[Test]
125+
[WindowsTest]
126+
[Retry(3)]
127+
public void CancellationToken_WhenGrandchildHoldsRedirectedPipes_ShouldNotHang()
128+
{
129+
// Reproduces a dump scenario: a child process spawns a grandchild that
130+
// inherits the redirected stdout/stderr handles AND is detached from the
131+
// .NET-managed job (CREATE_BREAKAWAY_FROM_JOB). When cancellation fires:
132+
// - Hitman.Kill terminates the immediate child.
133+
// - The grandchild survives and keeps the write-end of the pipes open.
134+
// - Process.WaitForExit() blocks forever waiting for the readers to EOF.
135+
// The fix is to close StandardOutput/StandardError after Kill so the
136+
// readers see EOF on our end and WaitForExit returns.
137+
138+
var grandchildPidFile = Path.Combine(Path.GetTempPath(), $"silent-runner-grandchild-{Guid.NewGuid():N}.pid");
139+
140+
var diagnosticsFile = grandchildPidFile + ".log";
141+
142+
// Reproduces the dump pattern (PowerShell -> AutoReport -> AutoReport-child):
143+
// 1. PowerShell uses Win32 CreateProcessW with bInheritHandles=true and
144+
// STARTF_USESTDHANDLES wired to the std handles GetStdHandle returns,
145+
// which were themselves inherited from SilentProcessRunner's redirected
146+
// pipes. That spawns cmd.exe with those same pipes.
147+
// 2. cmd.exe runs `start /b ping ...` which spawns ping inheriting cmd's
148+
// handles, then immediately exits.
149+
// 3. PowerShell waits for cmd to exit, looks up ping's PID via WMI
150+
// (filtered by ParentProcessId=cmd-PID), writes it for cleanup, exits.
151+
//
152+
// Result on the WaitForExit thread: PowerShell is dead, cmd is dead, ping
153+
// is alive holding the write-ends of the pipes. Process.Kill(true) walks
154+
// descendants by ParentProcessId and finds NOTHING (cmd's PID is gone, so
155+
// ping no longer appears as a descendant). Without the fix that closes
156+
// StandardOutput/StandardError after Kill, WaitForExit() hangs forever.
157+
var psScript = @"
158+
$ErrorActionPreference = 'Stop'
159+
$pidFile = 'PIDFILE_PLACEHOLDER'
160+
$logFile = 'LOGFILE_PLACEHOLDER'
161+
function Log($m) { try { Add-Content -Path $logFile -Value $m } catch {} }
162+
try {
163+
Log 'script start'
164+
$src = @'
165+
using System;
166+
using System.Runtime.InteropServices;
167+
public static class N {
168+
[StructLayout(LayoutKind.Sequential)]
169+
public struct STARTUPINFO {
170+
public int cb;
171+
public IntPtr lpReserved;
172+
public IntPtr lpDesktop;
173+
public IntPtr lpTitle;
174+
public int dwX, dwY, dwXSize, dwYSize, dwXCountChars, dwYCountChars;
175+
public int dwFillAttribute, dwFlags;
176+
public short wShowWindow, cbReserved2;
177+
public IntPtr lpReserved2;
178+
public IntPtr hStdInput, hStdOutput, hStdError;
179+
}
180+
[StructLayout(LayoutKind.Sequential)]
181+
public struct PROCESS_INFORMATION { public IntPtr hProcess, hThread; public int dwProcessId, dwThreadId; }
182+
[DllImport(""kernel32.dll"", SetLastError=true, CharSet=CharSet.Unicode)]
183+
public static extern bool CreateProcessW(
184+
IntPtr lpApplicationName,
185+
System.Text.StringBuilder lpCommandLine,
186+
IntPtr lpProcessAttributes,
187+
IntPtr lpThreadAttributes,
188+
[MarshalAs(UnmanagedType.Bool)] bool bInheritHandles,
189+
uint dwCreationFlags,
190+
IntPtr lpEnvironment,
191+
IntPtr lpCurrentDirectory,
192+
ref STARTUPINFO lpStartupInfo,
193+
out PROCESS_INFORMATION lpProcessInformation);
194+
[DllImport(""kernel32.dll"", SetLastError=true)] public static extern IntPtr GetStdHandle(int n);
195+
[DllImport(""kernel32.dll"", SetLastError=true)] public static extern bool CloseHandle(IntPtr h);
196+
[DllImport(""kernel32.dll"", SetLastError=true)] public static extern bool SetHandleInformation(IntPtr h, uint mask, uint flags);
197+
}
198+
'@
199+
Add-Type -TypeDefinition $src -Language CSharp
200+
Log 'add-type done'
201+
# Make our std handles inheritable (HANDLE_FLAG_INHERIT = 0x1).
202+
$hIn = [N]::GetStdHandle(-10)
203+
$hOut = [N]::GetStdHandle(-11)
204+
$hErr = [N]::GetStdHandle(-12)
205+
[void][N]::SetHandleInformation($hIn, 0x1, 0x1)
206+
[void][N]::SetHandleInformation($hOut, 0x1, 0x1)
207+
[void][N]::SetHandleInformation($hErr, 0x1, 0x1)
208+
$si = New-Object N+STARTUPINFO
209+
$si.cb = [System.Runtime.InteropServices.Marshal]::SizeOf($si)
210+
$si.dwFlags = 0x100 # STARTF_USESTDHANDLES
211+
$si.hStdInput = $hIn
212+
$si.hStdOutput = $hOut
213+
$si.hStdError = $hErr
214+
$pi = New-Object N+PROCESS_INFORMATION
215+
$pingPath = (Join-Path $env:WINDIR 'System32\PING.EXE')
216+
$cmdLine = New-Object System.Text.StringBuilder 1024
217+
# Run cmd.exe with: start /b PING -n 60001 127.0.0.1 -- cmd exits immediately
218+
# after `start` returns, leaving ping orphaned but with PowerShell's std handles.
219+
# -n 60001 is a magic-number tag so cleanup can identify our ping if PID lookup fails.
220+
[void]$cmdLine.Append('cmd.exe /c start /b """" ""'); [void]$cmdLine.Append($pingPath); [void]$cmdLine.Append('"" -n 60001 127.0.0.1')
221+
Log ""cmd: $($cmdLine.ToString())""
222+
# 0x08000000 = CREATE_NO_WINDOW
223+
$ok = [N]::CreateProcessW([IntPtr]::Zero, $cmdLine, [IntPtr]::Zero, [IntPtr]::Zero, $true, 0x08000000, [IntPtr]::Zero, [IntPtr]::Zero, [ref]$si, [ref]$pi)
224+
$err = [System.Runtime.InteropServices.Marshal]::GetLastWin32Error()
225+
Log ""ok=$ok err=$err cmdPid=$($pi.dwProcessId)""
226+
if (-not $ok) { throw ""CreateProcess failed with $err"" }
227+
$cmdPid = $pi.dwProcessId
228+
[void][N]::CloseHandle($pi.hProcess)
229+
[void][N]::CloseHandle($pi.hThread)
230+
# Wait for cmd to exit so we know ping has been spawned and cmd is gone (this is
231+
# what breaks the PPID chain and makes Process.Kill(true) miss the grandchild).
232+
$deadline = (Get-Date).AddSeconds(15)
233+
while (Get-Process -Id $cmdPid -ErrorAction SilentlyContinue) {
234+
if ((Get-Date) -gt $deadline) { throw 'cmd.exe did not exit within 15s' }
235+
Start-Sleep -Milliseconds 50
236+
}
237+
Log 'cmd exited'
238+
# Find our ping (orphaned, PPID = dead cmdPid). If WMI lookup fails, fall back
239+
# to a name+arguments match on the magic ping count.
240+
$pingPid = $null
241+
$deadline = (Get-Date).AddSeconds(10)
242+
while ($null -eq $pingPid -and (Get-Date) -lt $deadline) {
243+
$p = Get-CimInstance Win32_Process -Filter ""ParentProcessId=$cmdPid AND Name='PING.EXE'"" -ErrorAction SilentlyContinue | Select-Object -First 1
244+
if ($p) { $pingPid = $p.ProcessId; break }
245+
$p2 = Get-CimInstance Win32_Process -Filter ""Name='PING.EXE'"" -ErrorAction SilentlyContinue | Where-Object { $_.CommandLine -like '*60001*' } | Select-Object -First 1
246+
if ($p2) { $pingPid = $p2.ProcessId; break }
247+
Start-Sleep -Milliseconds 100
248+
}
249+
if ($null -eq $pingPid) { throw 'failed to locate ping grandchild' }
250+
Log ""pingPid=$pingPid""
251+
Set-Content -Path $pidFile -Value $pingPid
252+
Log 'pid written'
253+
exit 0
254+
} catch {
255+
Log (""error: $_"")
256+
exit 1
257+
}
258+
";
259+
psScript = psScript
260+
.Replace("PIDFILE_PLACEHOLDER", grandchildPidFile)
261+
.Replace("LOGFILE_PLACEHOLDER", diagnosticsFile);
262+
var encoded = Convert.ToBase64String(Encoding.Unicode.GetBytes(psScript));
263+
264+
try
265+
{
266+
using (var cts = new CancellationTokenSource())
267+
{
268+
var task = Task.Run(() => Execute(
269+
"powershell.exe",
270+
$"-NoProfile -NonInteractive -EncodedCommand {encoded}",
271+
"",
272+
out _,
273+
out _,
274+
out _,
275+
cts.Token));
276+
277+
// Wait for the grandchild to actually be spawned before cancelling,
278+
// so the test always exercises the grandchild-pipe scenario rather
279+
// than racing PowerShell's Add-Type compile against the cancel.
280+
WaitForGrandchildSpawn(grandchildPidFile, diagnosticsFile, TimeSpan.FromSeconds(30));
281+
282+
var sw = Stopwatch.StartNew();
283+
cts.Cancel();
284+
285+
var completed = task.Wait(TimeSpan.FromSeconds(30));
286+
sw.Stop();
287+
288+
completed.Should().BeTrue(
289+
$"ExecuteCommand should return shortly after cancellation even when a grandchild " +
290+
$"holds the redirected pipes. Without proactively closing the redirected streams " +
291+
$"after Kill, Process.WaitForExit() blocks indefinitely. Elapsed since cancel: {sw.Elapsed.TotalSeconds:F1}s");
292+
}
293+
}
294+
finally
295+
{
296+
TryKillGrandchildAndDeletePidFile(grandchildPidFile);
297+
try { if (File.Exists(diagnosticsFile)) File.Delete(diagnosticsFile); } catch { /* ignore */ }
298+
}
299+
}
300+
301+
static void WaitForGrandchildSpawn(string pidFile, string diagnosticsFile, TimeSpan timeout)
302+
{
303+
var deadline = DateTime.UtcNow + timeout;
304+
while (DateTime.UtcNow < deadline)
305+
{
306+
if (File.Exists(pidFile) && int.TryParse(SafelyReadAllText(pidFile).Trim(), out var pid) && pid > 0)
307+
return;
308+
Thread.Sleep(100);
309+
}
310+
var diag = File.Exists(diagnosticsFile) ? SafelyReadAllText(diagnosticsFile) : "(no diagnostics file)";
311+
throw new TimeoutException(
312+
$"Test setup failed: the PowerShell child never wrote a grandchild PID to '{pidFile}'. " +
313+
$"This means the grandchild was not spawned, so the hang scenario is not being exercised. PowerShell diagnostics:\n{diag}");
314+
}
315+
316+
static string SafelyReadAllText(string path)
317+
{
318+
try { return File.ReadAllText(path); } catch { return string.Empty; }
319+
}
320+
321+
static void TryKillGrandchildAndDeletePidFile(string pidFile)
322+
{
323+
try
324+
{
325+
if (File.Exists(pidFile) && int.TryParse(SafelyReadAllText(pidFile).Trim(), out var pid))
326+
{
327+
try { Process.GetProcessById(pid).Kill(); } catch { /* already gone */ }
328+
}
329+
}
330+
catch { /* ignore */ }
331+
try { if (File.Exists(pidFile)) File.Delete(pidFile); } catch { /* ignore */ }
332+
}
333+
121334
[Test]
122335
public void EchoHello_ShouldWriteToStdOut()
123336
{

0 commit comments

Comments
 (0)