Enforce process ownership: guarantee process termination across all execution paths

[Copilot]

When a pipe delegate threw an exception, the underlying process was left running in the background. In the worst case (process blocked writing to a full pipe buffer with no reader), this became a permanent deadlock with no way out short of user cancellation.

The new convention: CliWrap takes full ownership of the process it spawns and guarantees it is terminated before any execution method returns or throws, regardless of exit path.

Changes

• Command.Execution.cs

  • Use Task.WhenAny(waitTask, pipingTask) to detect early pipe failure, then immediately Kill() the process before awaiting it — prevents the deadlock where the process is blocked on a full pipe buffer with no reader
  • Add try/finally with process.Kill() as a belt-and-suspenders guarantee covering any exit path not already handled by the inline kill

• PullEventStreamCommandExtensions.cs

  • Replace abandonCts (which let the process keep running in the background when the consumer broke out) with killCts wired as the forceful cancellation token into ExecuteAsync
  • Breaking out of await foreach now cancels killCts, killing the process before ListenAsync returns

• Readme.md

  • Add "Process ownership" section documenting the guarantee and the ListenAsync break behavior

Example: previously broken, now correct

// Pipe throws → process was orphaned, now it's killed
var task = Cli.Wrap("long-running-process")
    .WithStandardOutputPipe(PipeTarget.Create((_, _) => throw new Exception("oops")))
    .ExecuteAsync();

await task; // throws, and process is now guaranteed dead

// Breaking out of ListenAsync → process was orphaned, now it's killed
await foreach (var e in cmd.ListenAsync())
    break; // process is killed before returning

[Tyrrrz]

This is a breaking change, but it only affects very specific edge cases where all the following conditions were true:

• Something threw an unhandled exception within ExecuteAsync(...) (for example, a user-provided pipe)
• The process continued executing despite its stdin/stdout/stderr streams closing
• The process continued using system resources and/or running indefinitely

In most cases, in the event of std streams closing, the process would just exit anyway.

Also, theoretically the streams may not close in a rare case where the Dispose(...) methods never get called -- i.e., when the execution doesn't reach await pipingTask:

CliWrap/CliWrap/Command.Execution.cs

Lines 250 to 260 in 234ae44

	// Wait until the process exits normally or gets killed.
	// The timeout is started after the execution is forcefully canceled and ensures
	// that we don't wait forever in case the attempt to kill the process failed.
	await process.WaitUntilExitAsync(waitTimeoutCts.Token).ConfigureAwait(false);
	// Send the cancellation signal to the stdin pipe since the process has exited
	// and won't need it anymore. This should prevent it from hanging in some edge cases.
	await stdInCts.CancelAsync();
	// Wait until piping is done and propagate exceptions
	await pipingTask.ConfigureAwait(false);

Additionally, the pull event stream execution model currently employs the behavior of detaching from the process when the iterator is abandoned (i.e., reached break or an unhandled exception within await foreach in user code). This behavior existed before and it was recently formalized in code in #338 -- however, it's not documented. To make things consistent, an abandoned iterator should be treated as a cancellation request and should result in a killed process.

The ultimate goal of this PR is to normalize the behavior under a simple and predictable expectation: when ExecuteAsync(), ExecuteBufferedAsync(), ListenAsync() finish, their respective spawned processes should have exited by then (either normally or by terminating gracefully or forcefully).

The open question remains about the Observe() model, as the observable can similarly be abandoned, but there isn't a clear way to tell if it was (correct me if I'm wrong). My inclination is to leave things as is there for now. Long term, we may make that model obsolete, as it can be rather easily derived from ListenAsync() by users and I doubt many people use it anyway.

@copilot please share your thoughts

[Copilot]

This is a breaking change, but it only affects very specific edge cases where all the following conditions were true:

• Something threw an unhandled exception within ExecuteAsync(...) (for exampl...

Agree with normalizing everything under "when ExecuteAsync() / ExecuteBufferedAsync() / ListenAsync() return or throw, the process has exited." A few thoughts on the specific points:

On streams not closing / Dispose(...) never running. This is exactly why the PR doesn't rely on stream closure anymore. The inline Task.WhenAny(waitTask, pipingTask) + Kill() only handles the deadlock (process blocked on a full pipe with no reader), while the try/finally kill is the real guarantee — it fires on every exit path, including the ones that never reach await pipingTask and therefore never dispose the std streams. So termination no longer depends on the process reacting to its streams closing; we terminate it explicitly regardless.

On Observe() — I think there actually is a clear signal. The subscribe callback in Observable.CreateSynchronized(...) returns an IDisposable, and Rx invokes it whenever the subscription is disposed — which is precisely the "abandoned" case (.Take(1), .FirstAsync(), an explicit subscription.Dispose(), or a downstream operator tearing down). Today we return Disposable.Null, so that teardown is a no-op and the process detaches — the direct analog of the abandonCts behavior I just removed from ListenAsync. To make Observe() consistent, we'd:

• create a killCts linked to forcefulCancellationToken,
• pass killCts.Token into ExecuteAsync(...),
• return a disposable that cancels killCts instead of Disposable.Null.

One caveat: that disposable also runs on normal completion, because OnCompleted()/OnError() cause the subscription to be disposed too. That's harmless here — by then the process has already exited, so cancelling killCts → Kill() is a no-op. So the convention holds for Observe() as well without special-casing.

So I'd gently push back on "there isn't a clear way to tell": there is, and it's symmetric with the ListenAsync fix. That said, I'm fine leaving Observe() out of this PR if you'd prefer to keep the diff focused (or if it's headed for obsoletion anyway) — just say the word and I'll wire up the killCts for it in a follow-up commit.

[codecov]

Codecov Report

❌ Patch coverage is 94.44444% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.45%. Comparing base (234ae44) to head (22a6489).

Files with missing lines	Patch %	Lines
CliWrap/Command.Execution.cs	90.00%	2 Missing and 1 partial ⚠️
...ap/EventStream/PushEventStreamCommandExtensions.cs	94.73%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            prime     #339      +/-   ##
==========================================
- Coverage   92.46%   92.45%   -0.02%     
==========================================
  Files          26       26              
  Lines        1155     1166      +11     
  Branches       68       69       +1     
==========================================
+ Hits         1068     1078      +10     
+ Misses         66       65       -1     
- Partials       21       23       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Tyrrrz]

@copilot

[Tyrrrz]

On Observe() — I think there actually is a clear signal. The subscribe callback in Observable.CreateSynchronized(...) returns an IDisposable, and Rx invokes it whenever the subscription is disposed — which is precisely the "abandoned" case (.Take(1), .FirstAsync(), an explicit subscription.Dispose(), or a downstream operator tearing down). Today we return Disposable.Null, so that teardown is a no-op and the process detaches — the direct analog of the abandonCts behavior I just removed from ListenAsync. To make Observe() consistent, we'd:

• create a killCts linked to forcefulCancellationToken,
• pass killCts.Token into ExecuteAsync(...),
• return a disposable that cancels killCts instead of Disposable.Null.

One caveat: that disposable also runs on normal completion, because OnCompleted()/OnError() cause the subscription to be disposed too. That's harmless here — by then the process has already exited, so cancelling killCts → Kill() is a no-op. So the convention holds for Observe() as well without special-casing.

So I'd gently push back on "there isn't a clear way to tell": there is, and it's symmetric with the ListenAsync fix. That said, I'm fine leaving Observe() out of this PR if you'd prefer to keep the diff focused (or if it's headed for obsoletion anyway) — just say the word and I'll wire up the killCts for it in a follow-up commit.

Hmm, but what makes that disposable get disposed? What if I just run cmd.Observe() and do nothing with it? Will it just get processed by the finalizer?

In any case, if the fix is simple and symmetrical, we should apply it to the Observe() model as well.

@copilot

[Copilot]

@Tyrrrz I've opened a new pull request, #340, to work on those changes. Once the pull request is ready, I'll request review from you.

[Tyrrrz]

@copilot why do you require two pull requests? You already own this one

[Copilot]

@copilot why do you require two pull requests? You already own this one

You're right — no need for a second PR. I've made the Observe() changes directly in this PR.

On Observe() — I think there actually is a clear signal. The subscribe callback in Observable.CreateSynchronized(...) returns an IDisposable, and Rx invokes it whenever the subscription ...

Applied the symmetric fix to Observe() here in this PR.

On your question: Observe() returns a cold observable, so the subscribe callback (which spawns the process) only runs when someone actually subscribes. If you call cmd.Observe() and do nothing with it, nothing happens — no process is spawned, so there's nothing to clean up and the finalizer never comes into play. The disposable is created on subscription and disposed when the subscription ends: a downstream operator tearing it down (Take, FirstAsync, ...), an explicit subscription.Dispose(), or normal OnCompleted/OnError. Only if you subscribe and never dispose nor let it complete (e.g. an endless process with a subscriber that stays attached) does the process keep running — the direct analog of never breaking out of an await foreach. So disposal is driven by subscription lifetime, not GC.

The fix mirrors ListenAsync: a forcefulCancellationOrAbandonCts linked to forcefulCancellationToken is passed into ExecuteAsync, and the returned disposable cancels it. Abandoning the observable now kills the process; on normal completion it's a no-op since the process has already exited. I also remap the surfaced cancellation to the user's original token so the existing Observe() cancellation tests keep asserting their own token. While here I also made ListenAsync's finally await full termination so the "process has exited by the time it returns" guarantee is actually met (it was previously fire-and-forget, which made the break-out assertion racy under load).

[Tyrrrz]

@copilot