Skip to content

Coordinate Copilot CLI stderr pump cleanup #1136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xoofx wants to merge 4 commits into
base: main
Choose a base branch
from
+303 −102
Open

Coordinate Copilot CLI stderr pump cleanup #1136

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cdb1d21
Coordinate Copilot CLI stderr pump cleanup
xoofx Apr 25, 2026
edf6f6f
Harden stderr pump cleanup paths
xoofx Apr 25, 2026
066b970
Close stderr cleanup startup gaps
xoofx Apr 25, 2026
3452e5b
Improve TCP startup failure cleanup
xoofx Apr 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 144 additions & 34 deletions dotnet/src/Client.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,9 @@ async Task<Connection> StartCoreAsync(CancellationToken ct)
else
{
// Child process (stdio or TCP)
var (cliProcess, portOrNull, stderrBuffer) = await StartCliServerAsync(_options, _logger, ct);
var (cliProcess, portOrNull, stderrPump) = await StartCliServerAsync(_options, _logger, ct);
_actualPort = portOrNull;
result = ConnectToServerAsync(cliProcess, portOrNull is null ? null : "localhost", portOrNull, stderrBuffer, ct);
result = ConnectToServerAsync(cliProcess, portOrNull is null ? null : "localhost", portOrNull, stderrPump, ct);
}

var connection = await result;
Expand Down Expand Up @@ -358,12 +358,23 @@ private async Task CleanupConnectionAsync(List<Exception>? errors)

if (ctx.CliProcess is { } childProcess)
{
ctx.StderrPump?.Cancel();

try
{
if (!childProcess.HasExited) childProcess.Kill();
childProcess.Dispose();
}
catch (Exception ex) { errors?.Add(ex); }

if (ctx.StderrPump is not null)
{
try { await ctx.StderrPump.WaitForCompletionAsync(); }
Comment thread
xoofx marked this conversation as resolved.
Outdated
catch (Exception ex) { errors?.Add(ex); }
finally { ctx.StderrPump.Dispose(); }
}

try { childProcess.Dispose(); }
catch (Exception ex) { errors?.Add(ex); }
}
}

Expand Down Expand Up @@ -1152,7 +1163,7 @@ private async Task VerifyProtocolVersionAsync(Connection connection, Cancellatio
_negotiatedProtocolVersion = serverVersion;
}

private static async Task<(Process Process, int? DetectedLocalhostTcpPort, StringBuilder StderrBuffer)> StartCliServerAsync(CopilotClientOptions options, ILogger logger, CancellationToken cancellationToken)
private static async Task<(Process Process, int? DetectedLocalhostTcpPort, ProcessStderrPump StderrPump)> StartCliServerAsync(CopilotClientOptions options, ILogger logger, CancellationToken cancellationToken)
{
// Use explicit path, COPILOT_CLI_PATH env var (from options.Environment or process env), or bundled CLI - no PATH fallback
var envCliPath = options.Environment is not null && options.Environment.TryGetValue("COPILOT_CLI_PATH", out var envValue) ? envValue
Expand Down Expand Up @@ -1242,47 +1253,61 @@ private async Task VerifyProtocolVersionAsync(Connection connection, Cancellatio
var cliProcess = new Process { StartInfo = startInfo };
cliProcess.Start();

Comment thread
xoofx marked this conversation as resolved.
// Capture stderr for error messages and forward to logger
var stderrBuffer = new StringBuilder();
_ = Task.Run(async () =>
// Capture stderr for error messages and forward to logger.
// The pump has its own lifetime token and is later cancelled/observed
// by the owning Connection before the process is disposed.
var stderrPump = ProcessStderrPump.Start(cliProcess, logger);

var detectedLocalhostTcpPort = (int?)null;
try
{
while (cliProcess != null && !cliProcess.HasExited)
if (!options.UseStdio)
{
var line = await cliProcess.StandardError.ReadLineAsync(cancellationToken);
if (line != null)
{
lock (stderrBuffer)
{
stderrBuffer.AppendLine(line);
}
// Wait for port announcement
using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
cts.CancelAfter(TimeSpan.FromSeconds(30));

if (logger.IsEnabled(LogLevel.Debug))
while (!cts.Token.IsCancellationRequested)
{
var line = await cliProcess.StandardOutput.ReadLineAsync(cts.Token) ?? throw new IOException("CLI process exited unexpectedly");
if (ListeningOnPortRegex().Match(line) is { Success: true } match)
{
logger.LogDebug("[CLI] {Line}", line);
detectedLocalhostTcpPort = int.Parse(match.Groups[1].Value, CultureInfo.InvariantCulture);
break;
}
}
Comment thread
xoofx marked this conversation as resolved.
Outdated
}
}, cancellationToken);

var detectedLocalhostTcpPort = (int?)null;
if (!options.UseStdio)
return (cliProcess, detectedLocalhostTcpPort, stderrPump);
}
catch
{
// Wait for port announcement
using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
cts.CancelAfter(TimeSpan.FromSeconds(30));
stderrPump.Cancel();

while (!cts.Token.IsCancellationRequested)
try
{
var line = await cliProcess.StandardOutput.ReadLineAsync(cts.Token) ?? throw new IOException("CLI process exited unexpectedly");
if (ListeningOnPortRegex().Match(line) is { Success: true } match)
if (!cliProcess.HasExited)
{
detectedLocalhostTcpPort = int.Parse(match.Groups[1].Value, CultureInfo.InvariantCulture);
break;
cliProcess.Kill();
}
}
}
catch (Exception ex)
{
logger.LogDebug(ex, "Failed to kill CLI process after startup failure");
}

return (cliProcess, detectedLocalhostTcpPort, stderrBuffer);
try
{
await stderrPump.WaitForCompletionAsync();
}
finally
{
stderrPump.Dispose();
cliProcess.Dispose();
}

throw;
}
}

private static string? GetBundledCliPath(out string searchedPath)
Expand Down Expand Up @@ -1326,7 +1351,7 @@ private static (string FileName, IEnumerable<string> Args) ResolveCliCommand(str
return (cliPath, args);
}

private async Task<Connection> ConnectToServerAsync(Process? cliProcess, string? tcpHost, int? tcpPort, StringBuilder? stderrBuffer, CancellationToken cancellationToken)
private async Task<Connection> ConnectToServerAsync(Process? cliProcess, string? tcpHost, int? tcpPort, ProcessStderrPump? stderrPump, CancellationToken cancellationToken)
{
Stream inputStream, outputStream;
TcpClient? tcpClient = null;
Comment thread
xoofx marked this conversation as resolved.
Expand Down Expand Up @@ -1384,7 +1409,7 @@ private async Task<Connection> ConnectToServerAsync(Process? cliProcess, string?

_rpc = new ServerRpc(rpc);

return new Connection(rpc, cliProcess, tcpClient, networkStream, stderrBuffer);
return new Connection(rpc, cliProcess, tcpClient, networkStream, stderrPump);
}

[UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "Using happy path from https://microsoft.github.io/vs-streamjsonrpc/docs/nativeAOT.html")]
Expand Down Expand Up @@ -1613,13 +1638,98 @@ private class Connection(
Process? cliProcess, // Set if we created the child process
TcpClient? tcpClient, // Set if using TCP
NetworkStream? networkStream, // Set if using TCP
StringBuilder? stderrBuffer = null) // Captures stderr for error messages
ProcessStderrPump? stderrPump = null) // Captures stderr for error messages
{
public Process? CliProcess => cliProcess;
public TcpClient? TcpClient => tcpClient;
public JsonRpc Rpc => rpc;
public NetworkStream? NetworkStream => networkStream;
public StringBuilder? StderrBuffer => stderrBuffer;
public ProcessStderrPump? StderrPump => stderrPump;
public StringBuilder? StderrBuffer => stderrPump?.Buffer;
}

private sealed class ProcessStderrPump : IDisposable
{
private readonly CancellationTokenSource _cancellationTokenSource = new();
private readonly Task _completion;
private bool _disposed;

private ProcessStderrPump(Process process, ILogger logger)
{
_completion = Task.Run(() => PumpAsync(process, logger, _cancellationTokenSource.Token));
}

public StringBuilder Buffer { get; } = new();

public static ProcessStderrPump Start(Process process, ILogger logger)
{
return new ProcessStderrPump(process, logger);
}

public void Cancel()
{
if (!_disposed)
{
_cancellationTokenSource.Cancel();
}
Comment thread
xoofx marked this conversation as resolved.
}

public async Task WaitForCompletionAsync()
{
await _completion;
}

public void Dispose()
{
if (_disposed)
{
return;
}

_disposed = true;
_cancellationTokenSource.Dispose();
}

private async Task PumpAsync(Process process, ILogger logger, CancellationToken cancellationToken)
{
try
{
while (true)
{
var line = await process.StandardError.ReadLineAsync(cancellationToken);
if (line is null)
{
break;
}

lock (Buffer)
{
Buffer.AppendLine(line);
}

if (logger.IsEnabled(LogLevel.Debug))
{
logger.LogDebug("[CLI] {Line}", line);
}
}
}
catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
{
}
catch (InvalidOperationException) when (cancellationToken.IsCancellationRequested)
{
}
catch (ObjectDisposedException) when (cancellationToken.IsCancellationRequested)
{
}
catch (IOException) when (cancellationToken.IsCancellationRequested)
{
}
catch (Exception ex)
{
logger.LogDebug(ex, "CLI stderr pump stopped unexpectedly");
}
}
}

private static class ProcessArgumentEscaper
Expand Down
Loading