diff --git a/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/CopilotLanguageServerProcessTests.java b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/CopilotLanguageServerProcessTests.java new file mode 100644 index 00000000..41803fe3 --- /dev/null +++ b/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 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); + } +} diff --git a/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/LsStreamConnectionProviderTests.java b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/LsStreamConnectionProviderTests.java index a805907d..b06e3d5c 100644 --- a/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/LsStreamConnectionProviderTests.java +++ b/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; + } + } } diff --git a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/CopilotLanguageServerConnection.java b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/CopilotLanguageServerConnection.java index ee65010f..222f7320 100644 --- a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/CopilotLanguageServerConnection.java +++ b/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 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 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 descendants) { + awaitExit(process, STOP_GRACE_PERIOD_MILLIS); + Set 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 CompletableFuture noActiveModelFailure() { diff --git a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/LsStreamConnectionProvider.java b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/LsStreamConnectionProvider.java index 925dcc08..d0ee27ed 100644 --- a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/LsStreamConnectionProvider.java +++ b/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(); diff --git a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceServiceTest.java b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceServiceTest.java index 7b36a797..f998ba83 100644 --- a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceServiceTest.java +++ b/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 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 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 */ diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatBaseService.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatBaseService.java index 0ad57630..bc823f7d 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatBaseService.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatBaseService.java @@ -5,7 +5,12 @@ import java.nio.file.Path; import java.nio.file.Paths; +import java.time.Duration; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.locks.ReentrantLock; import com.google.gson.Gson; import com.google.gson.JsonSyntaxException; @@ -34,8 +39,10 @@ public abstract class ChatBaseService { protected CopilotLanguageServerConnection lsConnection; protected AuthStatusManager authStatusManager; - protected String persistentPath; + protected volatile String persistentPath; private static UserPreference userPreference; + // Not a monitor, so that shutdown can give up waiting while a preference load waits for the CLS + private final ReentrantLock preferenceLock = new ReentrantLock(); /** * Constructor for the ChatBaseService. @@ -48,7 +55,16 @@ protected ChatBaseService(CopilotLanguageServerConnection lsConnection, AuthStat /** * Get User Preference. */ - protected synchronized UserPreference getUserPreference() { + protected UserPreference getUserPreference() { + preferenceLock.lock(); + try { + return loadUserPreference(); + } finally { + preferenceLock.unlock(); + } + } + + private UserPreference loadUserPreference() { if (userPreference != null) { return userPreference; } @@ -82,14 +98,40 @@ protected void clearUserPreferenceCache() { /** * Persist User Preference. */ - public synchronized void persistUserPreference() { - Path path = getPersistentFilePath(); + public void persistUserPreference() { + persistUserPreference(null); + } + + /** + * Persist User Preference, waiting at most the given time for the language server to provide the storage path. + * + * @param timeout the maximum time to wait, or {@code null} to wait without limit + */ + protected void persistUserPreference(@Nullable Duration timeout) { + long deadline = timeout == null ? 0 : System.nanoTime() + timeout.toNanos(); + // Resolve the path outside the lock, a pending preference load may hold it while waiting for the CLS + Path path = getPersistentFilePath(timeout); if (path == null) { return; } - String jsonContent = gson.toJson(userPreference); - PlatformUtils.writeFileContent(path, jsonContent); + try { + if (timeout == null) { + preferenceLock.lock(); + } else if (!preferenceLock.tryLock(deadline - System.nanoTime(), TimeUnit.NANOSECONDS)) { + CopilotCore.LOGGER.info("User preference is still loading, skipping save"); + return; + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return; + } + try { + String jsonContent = gson.toJson(userPreference); + PlatformUtils.writeFileContent(path, jsonContent); + } finally { + preferenceLock.unlock(); + } } /** @@ -117,16 +159,30 @@ protected void ensureRealm(Runnable runnable) { * Get the path for the persistent file. */ private @Nullable Path getPersistentFilePath() { + return getPersistentFilePath(null); + } + + /** + * Get the path for the persistent file, waiting at most the given time for the language server to provide it. + * + * @param timeout the maximum time to wait, or {@code null} to wait without limit + */ + private @Nullable Path getPersistentFilePath(@Nullable Duration timeout) { if (!this.authStatusManager.isSignedIn()) { return null; } if (this.persistentPath == null) { try { - ChatPersistence chatPersistence = this.lsConnection.persistence().get(); + CompletableFuture request = this.lsConnection.persistence(); + ChatPersistence chatPersistence = timeout == null ? request.get() + : request.get(timeout.toMillis(), TimeUnit.MILLISECONDS); this.persistentPath = chatPersistence.getPath(); } catch (InterruptedException | ExecutionException e) { CopilotCore.LOGGER.error("Failed to get persistent path", e); return null; + } catch (TimeoutException e) { + CopilotCore.LOGGER.info("Language server did not provide the persistent path in time, skipping"); + return null; } } diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceService.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceService.java index 0ca4b969..2248dc3d 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceService.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/UserPreferenceService.java @@ -3,6 +3,7 @@ package com.microsoft.copilot.eclipse.ui.chat.services; +import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; @@ -47,6 +48,8 @@ * Service for managing chat modes and input navigation. */ public class UserPreferenceService extends ChatBaseService implements CopilotAuthStatusListener { + private static final Duration SHUTDOWN_PERSIST_TIMEOUT = Duration.ofSeconds(2); + private IObservableValue chatModeObservable; private IObservableValue activeChatModeObservable; // Controls which view to show: Ask or Agent private IObservableValue activeModeNameOrIdObservable; // Tracks current mode name/ID for UI elements @@ -574,7 +577,8 @@ public void setSkipGitHubJobConfirmDialog(boolean skip) { * Dispose of the service. */ public void dispose() { - persistUserPreference(); + // A busy CLS must not block IDE shutdown, otherwise the CLS is not stopped either + persistUserPreference(SHUTDOWN_PERSIST_TIMEOUT); // Ideally we should dispose all side effects and observable here. But since the service is // singleton and will only be disposed when the bundle is stopped. So right now they are not // explicitly disposed here.