Skip to content

Commit 509f8b5

Browse files
stephentoubCopilot
andcommitted
Fix Copilot client startup cleanup race
Clean up partially established CLI connections if startup fails during protocol negotiation, and avoid rethrowing failed startup tasks during force-stop cleanup. Add a TCP fake-CLI regression test that drops during startup verification and verifies ForceStopAsync remains cleanup-safe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 08b486d commit 509f8b5

2 files changed

Lines changed: 85 additions & 9 deletions

File tree

dotnet/src/Client.cs

Lines changed: 45 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -244,12 +244,20 @@ async Task<Connection> StartCoreAsync(CancellationToken ct)
244244

245245
var connection = await result;
246246

247-
// Verify protocol version compatibility
248-
await VerifyProtocolVersionAsync(connection, ct);
249-
await ConfigureSessionFsAsync(ct);
247+
try
248+
{
249+
// Verify protocol version compatibility
250+
await VerifyProtocolVersionAsync(connection, ct);
251+
await ConfigureSessionFsAsync(ct);
250252

251-
_logger.LogInformation("Copilot client connected");
252-
return connection;
253+
_logger.LogInformation("Copilot client connected");
254+
return connection;
255+
}
256+
catch
257+
{
258+
await CleanupConnectionAsync(connection, errors: null);
259+
throw;
260+
}
253261
}
254262
}
255263

@@ -353,11 +361,27 @@ private async Task CleanupConnectionAsync(List<Exception>? errors)
353361
return;
354362
}
355363

356-
var ctx = await _connectionTask;
364+
var connectionTask = _connectionTask;
357365
_connectionTask = null;
358366

367+
Connection ctx;
368+
try
369+
{
370+
ctx = await connectionTask;
371+
}
372+
catch (Exception ex)
373+
{
374+
_logger.LogDebug(ex, "Ignoring failed Copilot client startup during cleanup");
375+
return;
376+
}
377+
378+
await CleanupConnectionAsync(ctx, errors);
379+
}
380+
381+
private async Task CleanupConnectionAsync(Connection ctx, List<Exception>? errors)
382+
{
359383
try { ctx.Rpc.Dispose(); }
360-
catch (Exception ex) { errors?.Add(ex); }
384+
catch (Exception ex) { AddCleanupError(errors, ex); }
361385

362386
// Clear RPC and models cache
363387
_serverRpc = null;
@@ -366,7 +390,7 @@ private async Task CleanupConnectionAsync(List<Exception>? errors)
366390
if (ctx.NetworkStream is not null)
367391
{
368392
try { await ctx.NetworkStream.DisposeAsync(); }
369-
catch (Exception ex) { errors?.Add(ex); }
393+
catch (Exception ex) { AddCleanupError(errors, ex); }
370394
}
371395

372396
if (ctx.CliProcess is { } childProcess)
@@ -380,7 +404,19 @@ private async Task CleanupConnectionAsync(List<Exception>? errors)
380404
}
381405
childProcess.Dispose();
382406
}
383-
catch (Exception ex) { errors?.Add(ex); }
407+
catch (Exception ex) { AddCleanupError(errors, ex); }
408+
}
409+
}
410+
411+
private void AddCleanupError(List<Exception>? errors, Exception ex)
412+
{
413+
if (errors is not null)
414+
{
415+
errors.Add(ex);
416+
}
417+
else
418+
{
419+
_logger.LogDebug(ex, "Error while cleaning up Copilot CLI connection");
384420
}
385421
}
386422

dotnet/test/E2E/ClientOptionsE2ETests.cs

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,28 @@ public async Task Should_Propagate_Activity_TraceContext_To_Session_Create_And_S
202202
await session.DisposeAsync();
203203
}
204204

205+
[Fact]
206+
public async Task ForceStop_Does_Not_Rethrow_When_Tcp_Cli_Drops_During_Startup()
207+
{
208+
var cliPath = Path.Join(Ctx.WorkDir, $"fake-tcp-drop-cli-{Guid.NewGuid():N}.js");
209+
await File.WriteAllTextAsync(cliPath, FakeTcpDropDuringStartupCliScript);
210+
211+
await using var client = Ctx.CreateClient(
212+
useStdio: false,
213+
options: new CopilotClientOptions
214+
{
215+
AutoStart = false,
216+
CliPath = cliPath,
217+
UseLoggedInUser = false,
218+
});
219+
220+
var ex = await Assert.ThrowsAsync<IOException>(() => client.StartAsync());
221+
Assert.Contains("Communication error", ex.Message, StringComparison.Ordinal);
222+
223+
await client.ForceStopAsync();
224+
Assert.Equal(ConnectionState.Disconnected, client.State);
225+
}
226+
205227
[Fact]
206228
public async Task Should_Propagate_Activity_TraceContext_To_Session_Resume()
207229
{
@@ -362,6 +384,24 @@ private static JsonElement GetCapturedRequestParams(JsonElement captureRoot, str
362384
.GetProperty("params");
363385
}
364386

387+
private const string FakeTcpDropDuringStartupCliScript = """
388+
const net = require("net");
389+
390+
const server = net.createServer(socket => {
391+
socket.on("data", () => {
392+
socket.destroy();
393+
server.close(() => process.exit(0));
394+
});
395+
});
396+
397+
server.listen(0, "localhost", () => {
398+
const address = server.address();
399+
console.log(`listening on port ${address.port}`);
400+
});
401+
402+
setTimeout(() => process.exit(2), 30000).unref();
403+
""";
404+
365405
private const string FakeStdioCliScript = """
366406
const fs = require("fs");
367407

0 commit comments

Comments
 (0)