Fix process crash when testing Stderr

[ericstj]

Fix #1448

@stephentoub

[stephentoub]

I don't understand this change or the comment on it. If HasExited return false, how does this prevent ErrorDataReceived from firing after Dispose?

[ericstj]

Here's what Copilot says:

The guard is there because WaitForExit() (no-arg) blocks indefinitely if the process hasn't exited.

Consider the scenarios at that point in the code:

Scenario	HasExited	What happens
Process exited on its own	true	Call WaitForExit() to flush events ✓
KillTree killed it	true	Call WaitForExit() to flush events ✓
KillTree called but process didn't die (edge case)	false	Skip — avoids hanging forever
Process never started (processStarted=false)	throws → true	Caught by the outer try

Without the guard, an unkillable process (or a race between kill and exit check) would cause WaitForExit() to hang the calling thread indefinitely. The tradeoff is that in that rare edge case, some ErrorDataReceived events may not be fully dispatched before Dispose() — but that's better than a deadlock.

HasExited returns true when the process has exited, but the no-arg WaitForExit() does additional work beyond waiting for exit — it drains the async stdout/stderr output readers.

Here are the docs that confirm that indeed you must call WaitForExit() to flush redirected events.
https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.process.waitforexit?view=net-10.0#system-diagnostics-process-waitforexit(system-int32)

Seems we could have a better name such an API - like Flush() or something.

[stephentoub]

I do not understand that response. My question wasn't about whether there needs to be a guard on the HasExited call... there already was a guard prior to this PR. This PR is just changing that guard from:

if (!processRunning && HasExited(process))

to

if (HasExited(process))

and the value of processRunning is not impacted at all based on whether KillTree was successful or not.

Moreover, the comment that was added suggests that "we must call [WaitForExit] [...] otherwise ErrorDataReceived handlers may fire after Dispose()" but there's nothing here preventing that race condition when we don't call WaitForExit because the process hasn't yet exited by the time we get here. So the logic is lost on me.

What am I missing?