Skip to content

Enforce process ownership: guarantee process termination across all execution paths - #340

Closed
Tyrrrz with Copilot wants to merge 2 commits into
copilot/fix-background-execution-on-exceptionfrom
copilot/enforce-process-ownership
Closed

Enforce process ownership: guarantee process termination across all execution paths#340
Tyrrrz with Copilot wants to merge 2 commits into
copilot/fix-background-execution-on-exceptionfrom
copilot/enforce-process-ownership

Conversation

Copilot AI commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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
  • PushEventStreamCommandExtensions.cs

    • Replace Disposable.Null teardown with a killCts-backed disposable, symmetrical with the ListenAsync fix
    • Disposing the Observe() subscription (e.g. via .FirstAsync(), .Take(n), or explicit Dispose()) now cancels killCts, killing the underlying process
  • 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

// Abandoning Observe() subscription → process was orphaned, now it's killed
await cmd.Observe().FirstAsync(); // process is killed after the subscription is disposed

Copilot AI changed the title [WIP] Enforce process ownership: guarantee process termination across all execution paths Enforce process ownership: guarantee process termination across all execution paths Jul 26, 2026
Copilot AI requested a review from Tyrrrz July 26, 2026 14:16
@Tyrrrz Tyrrrz closed this Jul 26, 2026
An error occurred while trying to automatically change base from copilot/fix-background-execution-on-exception to prime July 26, 2026 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants