Terminate Copilot language server process tree on shutdown

com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/CopilotLanguageServerProcessTests.java

@@ -0,0 +1,134 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+package com.microsoft.copilot.eclipse.core.lsp;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.Optional;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.lsp4e.LanguageServerWrapper;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+class CopilotLanguageServerProcessTests {
+
+  // Launched as single-file source in a separate JVM: the OSGi test runtime offers no plain classpath to
+  // start a compiled test class with.
+  private static final String SLEEPER_SOURCE = """
+      import java.nio.file.Files;
+      import java.nio.file.Path;
+      import java.util.Arrays;
+
+      public class Sleeper {
+        public static void main(String[] args) throws Exception {
+          int first = 0;
+          if (args.length > 0 && args[0].startsWith("--await=")) {
+            // Spawn the child only once the trigger file exists
+            Path trigger = Path.of(args[0].substring("--await=".length()));
+            while (!Files.exists(trigger)) {
+              Thread.sleep(50);
+            }
+            first = 1;
+          }
+          if (args.length > first) {
+            new ProcessBuilder(Arrays.copyOfRange(args, first, args.length))
+                .redirectOutput(ProcessBuilder.Redirect.DISCARD)
+                .redirectError(ProcessBuilder.Redirect.DISCARD).start();
+          }
+          Thread.sleep(120_000);
+        }
+      }
+      """;
+
+  @TempDir
+  Path tempDir;
+
+  private Process server;
+  private ProcessHandle serverChild;
+
+  @AfterEach
+  void killLeftovers() {
+    if (serverChild != null) {
+      serverChild.destroyForcibly();
+    }
+    if (server != null) {
+      server.destroyForcibly();
+    }
+  }
+
+  @Test
+  void testStop_serverIgnoresShutdown_terminatesServerProcessTree() throws Exception {
+    Path sleeper = tempDir.resolve("Sleeper.java");
+    Files.writeString(sleeper, SLEEPER_SOURCE);
+    String java = ProcessHandle.current().info().command().orElseThrow();
+    // The "server" spawns a child, like the CLS spawns MCP servers.
+    server = new ProcessBuilder(List.of(java, sleeper.toString(), java, sleeper.toString()))
+        .redirectOutput(ProcessBuilder.Redirect.DISCARD).redirectError(ProcessBuilder.Redirect.DISCARD).start();
+    serverChild = awaitChild(server);
+
+    // A mocked wrapper does nothing on stop(), like LSP4E when the JVM exits before its async close runs.
+    LanguageServerWrapper wrapper = mock(LanguageServerWrapper.class);
+    when(wrapper.getProcessHandle()).thenReturn(server.toHandle());
+
+    new CopilotLanguageServerConnection(wrapper).stop();
+
+    server.onExit().get(10, TimeUnit.SECONDS);
+    assertFalse(server.isAlive(), "language server must be terminated");
+    serverChild.onExit().get(10, TimeUnit.SECONDS);
+    assertFalse(serverChild.isAlive(), "language server child must be terminated");
+  }
+
+  @Test
+  void testStop_childSpawnedWhileStopping_terminatesChild() throws Exception {
+    Path sleeper = tempDir.resolve("Sleeper.java");
+    Files.writeString(sleeper, SLEEPER_SOURCE);
+    Path trigger = tempDir.resolve("spawn-child");
+    String java = ProcessHandle.current().info().command().orElseThrow();
+    server = new ProcessBuilder(List.of(java, sleeper.toString(), "--await=" + trigger, java, sleeper.toString()))
+        .redirectOutput(ProcessBuilder.Redirect.DISCARD).redirectError(ProcessBuilder.Redirect.DISCARD).start();
+    LanguageServerWrapper wrapper = mock(LanguageServerWrapper.class);
+    when(wrapper.getProcessHandle()).thenReturn(server.toHandle());
+    // stop() captures the process tree first, so the child spawned here is new to it
+    doAnswer(invocation -> {
+      Files.createFile(trigger);
+      serverChild = awaitChild(server);
+      return null;
+    }).when(wrapper).stop();
+
+    new CopilotLanguageServerConnection(wrapper).stop();
+
+    serverChild.onExit().get(10, TimeUnit.SECONDS);
+    assertFalse(serverChild.isAlive(), "child spawned while stopping must be terminated");
+  }
+
+  private static ProcessHandle awaitChild(Process process) throws IOException, InterruptedException {
+    long deadline = System.currentTimeMillis() + 60_000;
+    while (System.currentTimeMillis() < deadline) {
+      Optional<ProcessHandle> child = process.children().filter(CopilotLanguageServerProcessTests::isJava)
+          .findFirst();
+      if (child.isPresent()) {
+        return child.get();
+      }
+      assertTrue(process.isAlive(), "test server process died unexpectedly");
+      Thread.sleep(100);
+    }
+    throw new IOException("Test server process did not start a child process");
+  }
+
+  // Skips helper processes such as conhost.exe on Windows
+  private static boolean isJava(ProcessHandle process) {
+    return process.info().command().map(command -> Path.of(command).getFileName().toString().startsWith("java"))
+        .orElse(false);
+  }
+}

com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/LsStreamConnectionProviderTests.java

@@ -6,6 +6,8 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import java.io.IOException;
 import java.util.concurrent.TimeUnit;
@@ -69,6 +71,16 @@ void testStartLanguageServer() throws IOException {
     assertFalse(process.isAlive(), "the language server must not outlive the test");
   }
 
+  @Test
+  void testStart_coreStopping_doesNotStartLanguageServer() {
+    StoppingLsStreamConnectionProvider provider = new StoppingLsStreamConnectionProvider();
+
+    assertThrows(IOException.class, provider::start);
+    Process process = provider.process();
+    forceTerminate(process);
+    assertNull(process, "no language server must be started while Copilot core is stopping");
+  }
+
   /**
    * Makes sure the language server is really gone before the test JVM exits.
    *
@@ -107,9 +119,16 @@ private static boolean awaitExit(Process process) {
   /**
    * Exposes the language server process so the test can assert on the real operating system process.
    */
-  private static final class TestableLsStreamConnectionProvider extends LsStreamConnectionProvider {
+  private static class TestableLsStreamConnectionProvider extends LsStreamConnectionProvider {
     Process process() {
       return getProcess();
     }
   }
+
+  private static final class StoppingLsStreamConnectionProvider extends TestableLsStreamConnectionProvider {
+    @Override
+    protected boolean isCoreStopping() {
+      return true;
+    }
+  }
 }

com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/CopilotLanguageServerConnection.java

@@ -4,8 +4,13 @@
 package com.microsoft.copilot.eclipse.core.lsp;
 
 import java.net.URI;
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 import java.util.function.Function;
 
 import org.apache.commons.lang3.StringUtils;
@@ -95,6 +100,9 @@ public class CopilotLanguageServerConnection {
 
   public static final String SERVER_ID = "com.microsoft.copilot.eclipse.ls";
 
+  // LSP4E waits up to 5 s for the shutdown response before sending exit.
+  private static final long STOP_GRACE_PERIOD_MILLIS = 6_000;
+
   private LanguageServerWrapper languageServerWrapper;
 
   /**
@@ -732,10 +740,39 @@ public CompletableFuture<Object> acceptNextEditSuggestion(Command command) {
   }
 
   /**
-   * Stop the language server.
+   * Stop the language server and force-kill its process tree if it does not exit within a grace period.
    */
   public void stop() {
+    ProcessHandle process = this.languageServerWrapper.getProcessHandle();
+    // Capture descendants before stopping, they are re-parented once the server exits.
+    List<ProcessHandle> descendants = process == null ? List.of() : process.descendants().toList();
     this.languageServerWrapper.stop();
+    if (process != null) {
+      terminate(process, descendants);
+    }
+  }
+
+  // Blocking on purpose: LSP4E stops asynchronously and a busy CLS ignores shutdown, so it would outlive the IDE.
+  private static void terminate(ProcessHandle process, List<ProcessHandle> descendants) {
+    awaitExit(process, STOP_GRACE_PERIOD_MILLIS);
+    Set<ProcessHandle> processTree = new LinkedHashSet<>(descendants);
+    if (process.isAlive()) {
+      // Include children spawned during the grace period
+      process.descendants().forEach(processTree::add);
+      CopilotCore.LOGGER.info("Copilot language server did not exit in time, killing it");
+      process.destroyForcibly();
+    }
+    processTree.stream().filter(ProcessHandle::isAlive).forEach(ProcessHandle::destroyForcibly);
+  }
+
+  private static void awaitExit(ProcessHandle process, long timeoutMillis) {
+    try {
+      process.onExit().get(timeoutMillis, TimeUnit.MILLISECONDS);
+    } catch (TimeoutException | ExecutionException e) {
+      // checked by the caller via isAlive()
+    } catch (InterruptedException e) {
+      Thread.currentThread().interrupt();
+    }
   }
 
   private static <T> CompletableFuture<T> noActiveModelFailure() {

com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/LsStreamConnectionProvider.java

@@ -59,6 +59,10 @@ public Object getInitializationOptions(@Nullable URI rootUri) {
 
   @Override
   public void start() throws IOException {
+    // LSP4E restarts a stopped server on any late request, which would orphan a new CLS on IDE exit.
+    if (isCoreStopping()) {
+      throw new IOException("Copilot core is stopping, not starting the language server");
+    }
     if ("true".equalsIgnoreCase(System.getenv("COPILOT_DEBUG_LOCAL"))) {
       CopilotCore.LOGGER.info("Forcing JS LSP agent start due to environment variable.");
       startJsLspAgent();
@@ -87,6 +91,14 @@ public void start() throws IOException {
     }
   }
 
+  /**
+   * Returns whether the Copilot core bundle is stopping or already stopped.
+   */
+  protected boolean isCoreStopping() {
+    int state = CopilotCore.getPlugin().getBundle().getState();
+    return state != Bundle.ACTIVE && state != Bundle.STARTING;
+  }
+
   @Override
   protected ProcessBuilder createProcessBuilder() {
     ProcessBuilder pb = super.createProcessBuilder();

com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceServiceTest.java

@@ -6,18 +6,23 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively;
 import static org.mockito.Mockito.when;
 
 import java.lang.reflect.Field;
+import java.nio.file.Path;
+import java.time.Duration;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.CompletableFuture;
 
 import org.eclipse.e4.core.services.events.IEventBroker;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 import org.osgi.service.event.Event;
@@ -27,6 +32,7 @@
 import com.microsoft.copilot.eclipse.core.chat.InputNavigation;
 import com.microsoft.copilot.eclipse.core.events.CopilotEventConstants;
 import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection;
+import com.microsoft.copilot.eclipse.core.lsp.protocol.ChatPersistence;
 import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotStatusResult;
 
 @ExtendWith(MockitoExtension.class)
@@ -119,6 +125,75 @@ void testAuthStatusChangedEventHandler_UserSignsIn_ReloadsBuiltInModes() {
     assertNotNull(authHandler, "Auth handler should still be functional after handling sign-in event");
   }
 
+  @Test
+  void testDispose_LanguageServerBusy_DoesNotBlockShutdown() {
+    userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager);
+    when(mockAuthStatusManager.isSignedIn()).thenReturn(true);
+    // A busy CLS never answers conversation/persistence
+    when(mockLsConnection.persistence()).thenReturn(new CompletableFuture<>());
+
+    UserPreferenceService service = userPreferenceService;
+    userPreferenceService = null;
+    assertTimeoutPreemptively(Duration.ofSeconds(10), service::dispose);
+  }
+
+  @Test
+  void testDispose_PreferenceLoadPending_DoesNotBlockShutdown() throws InterruptedException {
+    userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager);
+    when(mockAuthStatusManager.isSignedIn()).thenReturn(true);
+    CompletableFuture<ChatPersistence> persistence = new CompletableFuture<>();
+    when(mockLsConnection.persistence()).thenReturn(persistence);
+    UserPreferenceService service = userPreferenceService;
+    userPreferenceService = null;
+    service.clearUserPreferenceCache();
+
+    // Loading the preferences waits for the busy CLS while holding the service lock
+    Thread loader = new Thread(service::isSkipGitHubJobConfirmDialog);
+    loader.setDaemon(true);
+    loader.start();
+    while (loader.getState() != Thread.State.WAITING) {
+      Thread.sleep(10);
+    }
+    try {
+      assertTimeoutPreemptively(Duration.ofSeconds(10), service::dispose);
+    } finally {
+      persistence.completeExceptionally(new IllegalStateException("CLS stopped"));
+      loader.join(5000);
+      service.clearUserPreferenceCache();
+    }
+  }
+
+  @Test
+  void testDispose_PreferenceLoadPendingButPathAvailable_DoesNotBlockShutdown(@TempDir Path tempDir)
+      throws InterruptedException {
+    userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager);
+    when(mockAuthStatusManager.isSignedIn()).thenReturn(true);
+    when(mockAuthStatusManager.getUserName()).thenReturn("test-user");
+    CompletableFuture<ChatPersistence> loadPersistence = new CompletableFuture<>();
+    ChatPersistence shutdownPersistence = new ChatPersistence();
+    shutdownPersistence.setPath(tempDir.toString());
+    // The load's request stays pending, but shutdown's separate request returns a valid path
+    when(mockLsConnection.persistence()).thenReturn(loadPersistence,
+        CompletableFuture.completedFuture(shutdownPersistence));
+    UserPreferenceService service = userPreferenceService;
+    userPreferenceService = null;
+    service.clearUserPreferenceCache();
+
+    Thread loader = new Thread(service::isSkipGitHubJobConfirmDialog);
+    loader.setDaemon(true);
+    loader.start();
+    while (loader.getState() != Thread.State.WAITING) {
+      Thread.sleep(10);
+    }
+    try {
+      assertTimeoutPreemptively(Duration.ofSeconds(10), service::dispose);
+    } finally {
+      loadPersistence.completeExceptionally(new IllegalStateException("CLS stopped"));
+      loader.join(5000);
+      service.clearUserPreferenceCache();
+    }
+  }
+
   /**
    * Helper method to create an auth status changed event
    */