Coordinate Copilot CLI stderr pump cleanup

[xoofx]

Hello,

Discovered this bug when embedding the .NET SDK in an application. The fire-and-forget task had an exception (the CLI process already exited) and I had a TaskSchedule.UnobservedTaskException that was reacting to this.

Summary

Fixes unsafe stderr pumping in the .NET CopilotClient by making the stderr reader an owned part of the connection lifecycle.

Previously, the CLI stderr reader was started as a fire-and-forget task. That meant cleanup could dispose the underlying Process while the background task was still reading from StandardError, creating a race during shutdown or failed startup.

Problem

CopilotClient starts a child Copilot CLI process and continuously reads its stderr so that CLI diagnostics can be logged and included in connection failure messages.

Before this change, that stderr pump had a few lifecycle issues:

• it was started with _ = Task.Run(...), so it was never stored or observed
• it used the startup cancellation token as a long-lived pump token
• it checked Process.HasExited, which can throw or race if another path disposes the process
• cleanup could kill/dispose the process without coordinating with the stderr task
• exceptions from the pump could go unobserved

This can happen during normal client shutdown, force-stop, startup failures, failed protocol negotiation, or any path where the CLI process exits while cleanup is running.

Fix

This PR introduces an owned ProcessStderrPump and attaches it to the connection state.

The pump now:

• has its own cancellation token
• reads stderr until EOF or cancellation
• captures stderr output for error reporting
• logs CLI stderr as before
• handles expected shutdown exceptions when cancellation is requested

Connection cleanup now coordinates shutdown by:

1. cancelling the stderr pump
2. killing the child process if needed
3. awaiting the pump completion
4. disposing the pump
5. disposing the process

Startup failure cleanup also cancels and observes the pump before disposing the process.

Purpose

The purpose of this fix is to make Copilot CLI process cleanup deterministic and safe. The stderr reader should not outlive the connection/process it depends on, and process disposal should not race against a background task that is still reading from the process streams.

Verification

• dotnet build dotnet/src/GitHub.Copilot.SDK.csproj -c Release --no-restore -v minimal
• dotnet test dotnet/test/GitHub.Copilot.SDK.Test.csproj -c Release --no-restore -v minimal --filter Should_Report_Error_With_Stderr_When_CLI_Fails_To_Start

[Copilot]

Pull request overview

This PR makes the .NET SDK’s Copilot CLI stderr reader an owned part of the connection lifecycle to avoid fire-and-forget task races during shutdown/startup failures, and to prevent unobserved task exceptions.

Changes:

• Replaces the previous background stderr read loop with an owned ProcessStderrPump that has explicit cancellation, completion tracking, and buffered stderr capture.
• Coordinates connection cleanup to cancel/await/dispose the stderr pump before disposing the CLI process.
• Updates CLI startup to create the pump and to cancel/observe it during startup-failure cleanup.

Comments suppressed due to low confidence (1)

dotnet/src/Client.cs:230

• If connection startup fails after spawning the CLI (e.g., ConnectToServerAsync throws, protocol negotiation fails, or ConfigureSessionFsAsync throws), the returned Process/StderrPump are never cleaned up because StartCoreAsync doesn't wrap those subsequent awaits in a try/catch/finally. This can leave an orphaned copilot process and a running stderr pump, and later StopAsync/DisposeAsync won't clean up because _connectionTask is faulted. Consider capturing cliProcess/stderrPump in locals and, on any exception after StartCliServerAsync returns, cancel the pump, attempt to kill the process, await pump completion (with a bounded timeout), then dispose both before rethrowing (and ideally reset _connectionTask).

                var (cliProcess, portOrNull, stderrPump) = await StartCliServerAsync(_options, _logger, ct);
                _actualPort = portOrNull;
                result = ConnectToServerAsync(cliProcess, portOrNull is null ? null : "localhost", portOrNull, stderrPump, ct);
            }

            var connection = await result;

            // Verify protocol version compatibility
            await VerifyProtocolVersionAsync(connection, ct);
            await ConfigureSessionFsAsync(ct);

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[stomde]

Great approved