Skip to content

Commit 7b94959

Browse files
stephentoubCopilot
andcommitted
Address graceful shutdown review feedback
Narrow .NET shutdown exception handling, avoid blocking Python's event loop while waiting for CLI process exit, clear external force-stop process references, and apply Java formatting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 577df8b commit 7b94959

5 files changed

Lines changed: 46 additions & 10 deletions

File tree

dotnet/src/Client.cs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -439,7 +439,11 @@ private async Task CleanupConnectionAsync(Connection ctx, List<Exception>? error
439439
"CopilotClient.StopAsync runtime shutdown complete. Elapsed={Elapsed}",
440440
runtimeShutdownTimestamp);
441441
}
442-
catch (Exception ex)
442+
catch (Exception ex) when (ex is OperationCanceledException
443+
or InvalidOperationException
444+
or ObjectDisposedException
445+
or IOException
446+
or SocketException)
443447
{
444448
LoggingHelpers.LogTiming(_logger, LogLevel.Debug, ex,
445449
"CopilotClient.StopAsync runtime shutdown failed. Elapsed={Elapsed}",

java/src/main/java/com/github/copilot/CopilotClient.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -381,7 +381,8 @@ public CompletableFuture<Void> forceStop() {
381381
// executor, so a plain whenComplete(...) here could land the awaitTermination
382382
// call on one of the very threads it is waiting to drain, forcing the full
383383
// AUTOCLOSEABLE_TIMEOUT_SECONDS timeout followed by shutdownNow().
384-
return cleanupConnection(false).whenCompleteAsync((ignored, error) -> shutdownOwnedExecutor(), SHUTDOWN_DISPATCHER);
384+
return cleanupConnection(false).whenCompleteAsync((ignored, error) -> shutdownOwnedExecutor(),
385+
SHUTDOWN_DISPATCHER);
385386
}
386387

387388
private CompletableFuture<Void> cleanupConnection(boolean gracefulRuntimeShutdown) {

java/src/test/java/com/github/copilot/CopilotClientTest.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,8 @@ static void setup() {
4646
void testStopRequestsRuntimeShutdownForOwnedProcess() throws Exception {
4747
var client = new CopilotClient(new CopilotClientOptions().setAutoStart(false));
4848
var rpc = mock(JsonRpcClient.class);
49-
when(rpc.invoke(eq("runtime.shutdown"), any(), eq(Void.class))).thenReturn(CompletableFuture.completedFuture(null));
49+
when(rpc.invoke(eq("runtime.shutdown"), any(), eq(Void.class)))
50+
.thenReturn(CompletableFuture.completedFuture(null));
5051
var process = mock(Process.class);
5152
when(process.isAlive()).thenReturn(true);
5253
when(process.waitFor(anyLong(), any(TimeUnit.class))).thenReturn(true);

python/copilot/client.py

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1528,15 +1528,24 @@ async def stop(self) -> None:
15281528
if is_running:
15291529
if runtime_shutdown_completed:
15301530
try:
1531-
self._cli_process.wait(timeout=_RUNTIME_SHUTDOWN_TIMEOUT_SECONDS)
1531+
await asyncio.to_thread(
1532+
self._cli_process.wait,
1533+
timeout=_RUNTIME_SHUTDOWN_TIMEOUT_SECONDS,
1534+
)
15321535
except subprocess.TimeoutExpired:
15331536
self._cli_process.terminate()
15341537
try:
1535-
self._cli_process.wait(timeout=_CLI_PROCESS_EXIT_TIMEOUT_SECONDS)
1538+
await asyncio.to_thread(
1539+
self._cli_process.wait,
1540+
timeout=_CLI_PROCESS_EXIT_TIMEOUT_SECONDS,
1541+
)
15361542
except subprocess.TimeoutExpired:
15371543
self._cli_process.kill()
15381544
try:
1539-
self._cli_process.wait(timeout=_CLI_PROCESS_EXIT_TIMEOUT_SECONDS)
1545+
await asyncio.to_thread(
1546+
self._cli_process.wait,
1547+
timeout=_CLI_PROCESS_EXIT_TIMEOUT_SECONDS,
1548+
)
15401549
except subprocess.TimeoutExpired as e:
15411550
errors.append(
15421551
StopError(
@@ -1549,17 +1558,22 @@ async def stop(self) -> None:
15491558
else:
15501559
self._cli_process.terminate()
15511560
try:
1552-
self._cli_process.wait(timeout=_CLI_PROCESS_EXIT_TIMEOUT_SECONDS)
1561+
await asyncio.to_thread(
1562+
self._cli_process.wait,
1563+
timeout=_CLI_PROCESS_EXIT_TIMEOUT_SECONDS,
1564+
)
15531565
except subprocess.TimeoutExpired:
15541566
self._cli_process.kill()
15551567
try:
1556-
self._cli_process.wait(timeout=_CLI_PROCESS_EXIT_TIMEOUT_SECONDS)
1568+
await asyncio.to_thread(
1569+
self._cli_process.wait,
1570+
timeout=_CLI_PROCESS_EXIT_TIMEOUT_SECONDS,
1571+
)
15571572
except subprocess.TimeoutExpired as e:
15581573
errors.append(
15591574
StopError(
15601575
message=(
1561-
"Timed out waiting for CLI process to exit after kill: "
1562-
f"{e}"
1576+
f"Timed out waiting for CLI process to exit after kill: {e}"
15631577
)
15641578
)
15651579
)
@@ -1602,6 +1616,8 @@ async def force_stop(self) -> None:
16021616
if self._is_external_server:
16031617
if self._process is not None:
16041618
self._process.terminate() # closes the TCP socket
1619+
self._process = None
1620+
self._cli_process = None
16051621
else:
16061622
if self._process is not None and self._process is not self._cli_process:
16071623
self._process.terminate()

python/test_client.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,20 @@ async def shutdown(self):
7979

8080
assert calls == []
8181

82+
@pytest.mark.asyncio
83+
async def test_force_stop_external_server_clears_process_references(self):
84+
process = Mock()
85+
client = CopilotClient(connection=RuntimeConnection.for_uri("localhost:1234"))
86+
client._is_external_server = True
87+
client._process = process
88+
client._cli_process = process
89+
90+
await client.force_stop()
91+
92+
process.terminate.assert_called_once()
93+
assert client._process is None
94+
assert client._cli_process is None
95+
8296

8397
class TestPermissionHandlerOptional:
8498
@pytest.mark.asyncio

0 commit comments

Comments
 (0)