Address PR review feedback on cleanup and tests

Agent-Logs-Url: https://github.com/github/copilot-sdk-java/sessions/38b90523-b5be-4dbd-9d33-36361110399b

Co-authored-by: edburns <75821+edburns@users.noreply.github.com>

Author: Copilot

src/main/java/com/github/copilot/sdk/CliServerManager.java

@@ -230,8 +230,7 @@ private Integer waitForPortAnnouncement(Process process) throws IOException {
                 if (line == null) {
                     awaitStderrReader();
                     String stderr = getStderrOutput();
-                    throw new IOException(
-                            CopilotClient.formatCliExitedMessage("CLI process exited unexpectedly.", stderr));
+                    throw new IOException(formatCliExitedMessage("CLI process exited unexpectedly.", stderr));
                 }
 
                 Matcher matcher = portPattern.matcher(line);
@@ -268,6 +267,13 @@ private void clearStderrBuffer() {
         }
     }
 
+    static String formatCliExitedMessage(String message, String stderrOutput) {
+        if (stderrOutput == null || stderrOutput.isEmpty()) {
+            return message;
+        }
+        return message + "\nstderr: " + stderrOutput;
+    }
+
     private List<String> resolveCliCommand(String cliPath, List<String> args) {
         boolean isJsFile = cliPath.toLowerCase().endsWith(".js");
 

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

@@ -76,6 +76,7 @@ public final class CopilotClient implements AutoCloseable {
      * shutdown via {@link #stop()}.
      */
     public static final int AUTOCLOSEABLE_TIMEOUT_SECONDS = 10;
+    private static final long FORCE_KILL_TIMEOUT_SECONDS = 10;
     private final CopilotClientOptions options;
     private final CliServerManager serverManager;
     private final LifecycleEventManager lifecycleManager = new LifecycleEventManager();
@@ -216,20 +217,13 @@ private Connection startCoreBody() {
         } catch (Exception e) {
             String stderr = serverManager.getStderrOutput();
             if (!stderr.isEmpty()) {
-                throw new CompletionException(
-                        new IOException(formatCliExitedMessage("CLI process exited unexpectedly.", stderr), e));
+                throw new CompletionException(new IOException(
+                        CliServerManager.formatCliExitedMessage("CLI process exited unexpectedly.", stderr), e));
             }
             throw new CompletionException(e);
         }
     }
 
-    static String formatCliExitedMessage(String message, String stderrOutput) {
-        if (stderrOutput == null || stderrOutput.isEmpty()) {
-            return message;
-        }
-        return message + "\nstderr: " + stderrOutput;
-    }
-
     private static final int MIN_PROTOCOL_VERSION = 2;
     private static final int METHOD_NOT_FOUND_ERROR_CODE = -32601;
 
@@ -360,8 +354,14 @@ private CompletableFuture<Void> cleanupConnection() {
             if (connection.process != null) {
                 try {
                     if (connection.process.isAlive()) {
-                        connection.process.destroyForcibly().waitFor();
+                        Process destroyedProcess = connection.process.destroyForcibly();
+                        if (!destroyedProcess.waitFor(FORCE_KILL_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
+                            LOG.fine("Process did not terminate within force kill timeout");
+                        }
                     }
+                } catch (InterruptedException e) {
+                    Thread.currentThread().interrupt();
+                    LOG.log(Level.FINE, "Interrupted while killing process", e);
                 } catch (Exception e) {
                     LOG.log(Level.FINE, "Error killing process", e);
                 }

src/test/java/com/github/copilot/sdk/PermissionsTest.java

@@ -456,7 +456,8 @@ void testShouldWaitForSlowPermissionHandler() throws Exception {
                                 v -> new PermissionRequestResult().setKind(PermissionRequestResultKind.APPROVED));
                     })).get();
 
-            // Use send (non-blocking) so we can interact with the handler
+            // Capture the sendAndWait future before awaiting it so we can interact with the
+            // handler
             CompletableFuture<AssistantMessageEvent> responseFuture = session
                     .sendAndWait(new MessageOptions().setPrompt("Run 'echo slow_handler_test'"));
 

src/test/java/com/github/copilot/sdk/ToolsTest.java

@@ -13,7 +13,10 @@
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
@@ -374,6 +377,10 @@ void testShouldExecuteMultipleCustomToolsInParallelSingleTurn() throws Exception
 
         var toolACalled = new CompletableFuture<String>();
         var toolBCalled = new CompletableFuture<String>();
+        var handlersStarted = new CountDownLatch(2);
+        var releaseHandlers = new CountDownLatch(1);
+        var activeHandlers = new AtomicInteger();
+        var handlersOverlapped = new AtomicBoolean(false);
 
         Map<String, Object> cityParams = Map.of("type", "object", "properties",
                 Map.of("city", Map.of("type", "string", "description", "City name")), "required", List.of("city"));
@@ -385,28 +392,36 @@ void testShouldExecuteMultipleCustomToolsInParallelSingleTurn() throws Exception
                 (invocation) -> {
                     String city = (String) invocation.getArguments().get("city");
                     toolACalled.complete(city);
-                    return CompletableFuture.completedFuture("CITY_" + city.toUpperCase());
+                    return executeParallelHandler(city, "CITY_", handlersStarted, releaseHandlers, activeHandlers,
+                            handlersOverlapped);
                 });
 
         ToolDefinition lookupCountry = ToolDefinition.create("lookup_country", "Looks up country information",
                 countryParams, (invocation) -> {
                     String country = (String) invocation.getArguments().get("country");
                     toolBCalled.complete(country);
-                    return CompletableFuture.completedFuture("COUNTRY_" + country.toUpperCase());
+                    return executeParallelHandler(country, "COUNTRY_", handlersStarted, releaseHandlers, activeHandlers,
+                            handlersOverlapped);
                 });
 
         try (CopilotClient client = ctx.createClient()) {
             CopilotSession session = client.createSession(new SessionConfig()
                     .setTools(List.of(lookupCity, lookupCountry)).setOnPermissionRequest(PermissionHandler.APPROVE_ALL))
                     .get();
 
-            AssistantMessageEvent response = session.sendAndWait(new MessageOptions().setPrompt(
-                    "Use lookup_city with 'Paris' and lookup_country with 'France' at the same time, then combine both results in your reply."))
-                    .get(60, TimeUnit.SECONDS);
+            CompletableFuture<AssistantMessageEvent> responseFuture = session
+                    .sendAndWait(new MessageOptions().setPrompt(
+                            "Use lookup_city with 'Paris' and lookup_country with 'France' at the same time, then combine both results in your reply."));
+
+            assertTrue(handlersStarted.await(10, TimeUnit.SECONDS), "Both tool handlers should start");
+            releaseHandlers.countDown();
+
+            AssistantMessageEvent response = responseFuture.get(60, TimeUnit.SECONDS);
 
             // Both tools should have been called
             assertEquals("Paris", toolACalled.get(10, TimeUnit.SECONDS));
             assertEquals("France", toolBCalled.get(10, TimeUnit.SECONDS));
+            assertTrue(handlersOverlapped.get(), "Tool handlers should overlap in execution");
 
             assertNotNull(response);
             String content = response.getData().content();
@@ -417,6 +432,32 @@ void testShouldExecuteMultipleCustomToolsInParallelSingleTurn() throws Exception
         }
     }
 
+    private CompletableFuture<Object> executeParallelHandler(String value, String prefix,
+            CountDownLatch handlersStarted, CountDownLatch releaseHandlers, AtomicInteger activeHandlers,
+            AtomicBoolean handlersOverlapped) {
+        int currentActive = activeHandlers.incrementAndGet();
+        if (currentActive > 1) {
+            handlersOverlapped.set(true);
+        }
+
+        handlersStarted.countDown();
+        try {
+            if (!handlersStarted.await(10, TimeUnit.SECONDS)) {
+                return CompletableFuture.failedFuture(new IllegalStateException("Tool handlers did not overlap"));
+            }
+            if (!releaseHandlers.await(10, TimeUnit.SECONDS)) {
+                return CompletableFuture
+                        .failedFuture(new IllegalStateException("Timed out waiting to release handlers"));
+            }
+            return CompletableFuture.completedFuture(prefix + value.toUpperCase());
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+            return CompletableFuture.failedFuture(e);
+        } finally {
+            activeHandlers.decrementAndGet();
+        }
+    }
+
     /**
      * Verifies that excludedTools are respected even when also listed in
      * availableTools.