From 4858efb602118fa5636f7e9802a3d7723e259255 Mon Sep 17 00:00:00 2001 From: Sheng Chen Date: Mon, 3 Aug 2026 14:26:11 +0800 Subject: [PATCH 1/8] refactor: replace GithubPanicErrorReport with a server-backed ExceptionReporter Exception reporting previously fell back to a direct HTTP client (GithubPanicErrorReport) whenever the language server was unavailable, and routed every report through LanguageServerWrapper.execute(), which starts the server as a side effect. Exception reporting is now best-effort and server-only: - Add ExceptionReporter, which owns the platform log listener and dispatches reports on a bounded, daemon-backed single-thread executor so reporting never blocks or delays the caller. - Cache the language server sink once at connection construction instead of re-entering the wrapper per report, so reporting cannot start the server. - Drop reports silently when no running server connection is available; clear the cached sink only when the server is actually stopped, so a transient send failure does not silence reporting for the rest of the session. - Delete GithubPanicErrorReport and its now-unused httpcomponents and org.eclipse.core.net bundle dependencies. - Make plug-in shutdown cancel and await the initialization job so the bundle is not unloaded while the language server is still being set up. Document the "Exception report" concept in CONTEXT.md and cover the new behaviour with ExceptionReporterTests and CopilotLanguageServerConnectionTests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: edb91a65-6071-4ed5-84a9-7e534ea391ad --- CONTEXT.md | 5 + .../META-INF/MANIFEST.MF | 1 - .../core/logger/ExceptionReporterTests.java | 92 ++++++ .../CopilotLanguageServerConnectionTests.java | 126 ++++++++ .../META-INF/MANIFEST.MF | 3 - .../copilot/eclipse/core/CopilotCore.java | 122 ++++---- .../core/logger/ExceptionReporter.java | 120 ++++++++ .../core/logger/GithubPanicErrorReport.java | 287 ------------------ .../lsp/CopilotLanguageServerConnection.java | 63 +++- .../protocol/TelemetryExceptionParams.java | 9 +- .../LanguageServerSettingManager.java | 14 +- 11 files changed, 461 insertions(+), 381 deletions(-) create mode 100644 com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/logger/ExceptionReporterTests.java create mode 100644 com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/CopilotLanguageServerConnectionTests.java create mode 100644 com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/logger/ExceptionReporter.java delete mode 100644 com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/logger/GithubPanicErrorReport.java diff --git a/CONTEXT.md b/CONTEXT.md index bb6b45cb7..e100ed737 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -36,3 +36,8 @@ project's files are semantically indexed. ### Workspace instructions Custom instructions associated with workspace folders and loaded into chat. They are distinct from semantic search over project files. + +### Exception report +A best-effort diagnostic record of an unexpected failure. Exception reporting +must not start or delay the language server and may be dropped when no running +server connection is available. diff --git a/com.microsoft.copilot.eclipse.core.test/META-INF/MANIFEST.MF b/com.microsoft.copilot.eclipse.core.test/META-INF/MANIFEST.MF index efcd8dba1..f95dcaa4f 100644 --- a/com.microsoft.copilot.eclipse.core.test/META-INF/MANIFEST.MF +++ b/com.microsoft.copilot.eclipse.core.test/META-INF/MANIFEST.MF @@ -20,7 +20,6 @@ Require-Bundle: com.microsoft.copilot.eclipse.core;bundle-version="0.20.0", org.eclipse.lsp4j, org.eclipse.core.jobs, org.eclipse.equinox.common, - org.eclipse.core.net, org.eclipse.core.resources, org.eclipse.text, org.eclipse.core.runtime diff --git a/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/logger/ExceptionReporterTests.java b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/logger/ExceptionReporterTests.java new file mode 100644 index 000000000..69d4a994f --- /dev/null +++ b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/logger/ExceptionReporterTests.java @@ -0,0 +1,92 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +package com.microsoft.copilot.eclipse.core.logger; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.jupiter.api.Test; + +class ExceptionReporterTests { + + @Test + void testReport_dispatchesOnReporterThread() throws InterruptedException { + Thread callerThread = Thread.currentThread(); + AtomicReference reporterThread = new AtomicReference<>(); + CountDownLatch reported = new CountDownLatch(1); + + try (ExceptionReporter reporter = new ExceptionReporter()) { + reporter.setSink(exception -> { + reporterThread.set(Thread.currentThread()); + reported.countDown(); + }); + reporter.report(new IllegalStateException("test")); + + assertTrue(reported.await(5, TimeUnit.SECONDS)); + assertNotEquals(callerThread, reporterThread.get()); + } + } + + @Test + void testReport_withoutSinkIsDiscarded() { + AtomicInteger reportCount = new AtomicInteger(); + try (ExceptionReporter reporter = new ExceptionReporter()) { + reporter.report(new IllegalStateException("test")); + reporter.setSink(exception -> reportCount.incrementAndGet()); + + assertEquals(0, reportCount.get()); + } + } + + @Test + void testSetSink_afterCloseIsIgnored() { + AtomicInteger reportCount = new AtomicInteger(); + ExceptionReporter reporter = new ExceptionReporter(); + reporter.close(); + + reporter.setSink(exception -> reportCount.incrementAndGet()); + reporter.report(new IllegalStateException("test")); + + assertEquals(0, reportCount.get()); + } + + @Test + void testReport_discardsWhenQueueIsFull() throws InterruptedException { + AtomicInteger reportCount = new AtomicInteger(); + CountDownLatch firstReportStarted = new CountDownLatch(1); + CountDownLatch releaseFirstReport = new CountDownLatch(1); + CountDownLatch secondReportFinished = new CountDownLatch(1); + + try (ExceptionReporter reporter = new ExceptionReporter(1)) { + reporter.setSink(exception -> { + int count = reportCount.incrementAndGet(); + if (count == 1) { + firstReportStarted.countDown(); + try { + releaseFirstReport.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } else { + secondReportFinished.countDown(); + } + }); + reporter.report(new IllegalStateException("first")); + assertTrue(firstReportStarted.await(5, TimeUnit.SECONDS)); + + reporter.report(new IllegalStateException("second")); + reporter.report(new IllegalStateException("discarded")); + releaseFirstReport.countDown(); + + assertTrue(secondReportFinished.await(5, TimeUnit.SECONDS)); + assertEquals(2, reportCount.get()); + } + } +} diff --git a/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/CopilotLanguageServerConnectionTests.java b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/CopilotLanguageServerConnectionTests.java new file mode 100644 index 000000000..53302318e --- /dev/null +++ b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/CopilotLanguageServerConnectionTests.java @@ -0,0 +1,126 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +package com.microsoft.copilot.eclipse.core.lsp; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Function; + +import org.eclipse.lsp4e.LanguageServerWrapper; +import org.eclipse.lsp4j.services.LanguageServer; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import com.microsoft.copilot.eclipse.core.lsp.protocol.TelemetryExceptionParams; + +@ExtendWith(MockitoExtension.class) +class CopilotLanguageServerConnectionTests { + + @Mock + private LanguageServerWrapper languageServerWrapper; + @Mock + private CopilotLanguageServer languageServer; + + private final AtomicReference>> sinkInitializer = + new AtomicReference<>(); + private CopilotLanguageServerConnection connection; + + @BeforeEach + void setUp() { + when(languageServerWrapper.execute(any())).thenAnswer(invocation -> { + sinkInitializer.set(invocation.getArgument(0)); + return new CompletableFuture(); + }); + connection = new CopilotLanguageServerConnection(languageServerWrapper); + } + + @Test + void testSendExceptionTelemetry_beforeInitialization_doesNotReenterWrapper() { + assertNull(connection.sendExceptionTelemetry(new IllegalStateException("test")).join()); + + verify(languageServerWrapper, times(1)).execute(any()); + verify(languageServer, never()).sendExceptionTelemetry(any()); + } + + @Test + void testSendExceptionTelemetry_afterInitialization_usesCachedSink() { + sinkInitializer.get().apply(languageServer).join(); + when(languageServer.sendExceptionTelemetry(any())).thenReturn(CompletableFuture.completedFuture(null)); + + assertNull(connection.sendExceptionTelemetry(new IllegalStateException("test")).join()); + + ArgumentCaptor paramsCaptor = ArgumentCaptor.forClass(TelemetryExceptionParams.class); + verify(languageServer).sendExceptionTelemetry(paramsCaptor.capture()); + verify(languageServerWrapper, times(1)).execute(any()); + assertEquals(1, paramsCaptor.getValue().getExceptionDetail().size()); + } + + @Test + void testSendExceptionTelemetry_afterFailureOnActiveServerKeepsSink() { + sinkInitializer.get().apply(languageServer).join(); + when(languageServerWrapper.isActive()).thenReturn(true); + when(languageServer.sendExceptionTelemetry(any())) + .thenReturn(CompletableFuture.failedFuture(new IllegalStateException("transient"))); + + assertNull(connection.sendExceptionTelemetry(new IllegalStateException("first")).join()); + assertNull(connection.sendExceptionTelemetry(new IllegalStateException("second")).join()); + + verify(languageServer, times(2)).sendExceptionTelemetry(any()); + } + + @Test + void testSendExceptionTelemetry_afterFailureOnStoppedServerClearsSink() { + sinkInitializer.get().apply(languageServer).join(); + when(languageServerWrapper.isActive()).thenReturn(false); + when(languageServer.sendExceptionTelemetry(any())) + .thenReturn(CompletableFuture.failedFuture(new IllegalStateException("closed"))); + + assertNull(connection.sendExceptionTelemetry(new IllegalStateException("first")).join()); + assertNull(connection.sendExceptionTelemetry(new IllegalStateException("second")).join()); + + verify(languageServer, times(1)).sendExceptionTelemetry(any()); + } + + @Test + void testStop_clearsExceptionSinkBeforeStoppingWrapper() { + sinkInitializer.get().apply(languageServer).join(); + + connection.stop(); + assertNull(connection.sendExceptionTelemetry(new IllegalStateException("test")).join()); + + verify(languageServerWrapper).stop(); + verify(languageServer, never()).sendExceptionTelemetry(any()); + } + + @Test + void testStop_beforeInitializationPreventsLateSinkRegistration() { + connection.stop(); + sinkInitializer.get().apply(languageServer).join(); + + assertNull(connection.sendExceptionTelemetry(new IllegalStateException("test")).join()); + + verify(languageServerWrapper).stop(); + verify(languageServer, never()).sendExceptionTelemetry(any()); + } + + @Test + void testStop_isIdempotent() { + connection.stop(); + connection.stop(); + + verify(languageServerWrapper, times(1)).stop(); + } +} diff --git a/com.microsoft.copilot.eclipse.core/META-INF/MANIFEST.MF b/com.microsoft.copilot.eclipse.core/META-INF/MANIFEST.MF index bfa152564..d85526e04 100644 --- a/com.microsoft.copilot.eclipse.core/META-INF/MANIFEST.MF +++ b/com.microsoft.copilot.eclipse.core/META-INF/MANIFEST.MF @@ -41,12 +41,9 @@ Require-Bundle: org.eclipse.lsp4e;bundle-version="0.18.1", org.eclipse.jface.text;bundle-version="3.24.200", com.google.gson;bundle-version="2.10.1", org.eclipse.wildwebdeveloper.embedder.node;bundle-version="1.0.3";resolution:=optional, - org.eclipse.core.net;bundle-version="1.5.200", org.eclipse.core.resources;bundle-version="3.20.0", org.eclipse.core.filesystem;bundle-version="1.10.200", org.eclipse.core.runtime;bundle-version="[3.30.0,4.0.0)", - org.apache.httpcomponents.client5.httpclient5;bundle-version="5.2.1", - org.apache.httpcomponents.core5.httpcore5;bundle-version="5.2.3", org.osgi.service.event;bundle-version="1.4.1", org.eclipse.e4.core.services;bundle-version="2.4.200", org.eclipse.e4.core.contexts;bundle-version="1.12.400" diff --git a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/CopilotCore.java b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/CopilotCore.java index a51e4280c..f5f23149a 100644 --- a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/CopilotCore.java +++ b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/CopilotCore.java @@ -4,10 +4,10 @@ package com.microsoft.copilot.eclipse.core; import java.util.Objects; +import java.util.concurrent.atomic.AtomicBoolean; import org.eclipse.core.runtime.IProgressMonitor; import org.eclipse.core.runtime.IStatus; -import org.eclipse.core.runtime.Platform; import org.eclipse.core.runtime.Plugin; import org.eclipse.core.runtime.Status; import org.eclipse.core.runtime.jobs.Job; @@ -22,7 +22,7 @@ import com.microsoft.copilot.eclipse.core.completion.CompletionProvider; import com.microsoft.copilot.eclipse.core.format.FormatOptionProvider; import com.microsoft.copilot.eclipse.core.logger.CopilotForEclipseLogger; -import com.microsoft.copilot.eclipse.core.logger.GithubPanicErrorReport; +import com.microsoft.copilot.eclipse.core.logger.ExceptionReporter; import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection; import com.microsoft.copilot.eclipse.core.nes.NextEditSuggestionProvider; @@ -32,15 +32,17 @@ */ public class CopilotCore extends Plugin { - private CopilotLanguageServerConnection copilotLanguageServer; + private volatile CopilotLanguageServerConnection copilotLanguageServer; private AuthStatusManager authStatusManager; private CompletionProvider completionProvider; private NextEditSuggestionProvider nextEditSuggestionProvider; private FormatOptionProvider formatOptionProvider; - private GithubPanicErrorReport githubPanicErrorReport; private ChatEventsManager chatEventsManager; private IChatServiceManager chatServiceManager; private FeatureFlags featureFlags; + private final ExceptionReporter exceptionReporter; + private final AtomicBoolean stopping = new AtomicBoolean(); + private volatile Job initJob; private static CopilotCore COPILOT_CORE_PLUGIN = null; public static final CopilotForEclipseLogger LOGGER = new CopilotForEclipseLogger(CopilotCore.class.getName()); @@ -57,6 +59,7 @@ public class CopilotCore extends Plugin { public CopilotCore() { super(); COPILOT_CORE_PLUGIN = this; + exceptionReporter = new ExceptionReporter(); } public static CopilotCore getPlugin() { @@ -65,40 +68,66 @@ public static CopilotCore getPlugin() { @Override public void start(BundleContext context) throws Exception { + exceptionReporter.start(); init(context); } @Override public void stop(BundleContext context) throws Exception { - if (copilotLanguageServer != null) { - copilotLanguageServer.stop(); + stopping.set(true); + exceptionReporter.close(); + try { + if (initJob != null) { + // Blocking is intentional: the bundle must not be unloaded while the initialization job is + // still touching the language server, so wait for it to observe the cancellation. + initJob.cancel(); + initJob.join(); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw e; + } finally { + CopilotLanguageServerConnection connection = copilotLanguageServer; + if (connection != null) { + connection.stop(); + } } } @SuppressWarnings("restriction") void init(BundleContext context) { - final Runnable initRunnable = () -> { - addPlatformLogListener(); - LanguageServersRegistry.LanguageServerDefinition serverDef = LanguageServersRegistry.getInstance() - .getDefinition(CopilotLanguageServerConnection.SERVER_ID); - if (serverDef == null) { - var ex = new IllegalStateException( - "Language server definition not found for " + CopilotLanguageServerConnection.SERVER_ID); - CopilotCore.LOGGER.error(ex); - throw ex; - } + initJob = new Job("GitHub Copilot Initialization...") { + @Override + protected IStatus run(IProgressMonitor monitor) { + if (monitor.isCanceled() || stopping.get()) { + return Status.CANCEL_STATUS; + } - LanguageServerWrapper wrapper = LanguageServiceAccessor.startLanguageServer(serverDef); - this.copilotLanguageServer = new CopilotLanguageServerConnection(wrapper); - this.authStatusManager = new AuthStatusManager(this.copilotLanguageServer); - this.completionProvider = new CompletionProvider(this.copilotLanguageServer, authStatusManager); - this.githubPanicErrorReport = new GithubPanicErrorReport(); - this.featureFlags = new FeatureFlags(); - }; + LanguageServersRegistry.LanguageServerDefinition serverDef = LanguageServersRegistry.getInstance() + .getDefinition(CopilotLanguageServerConnection.SERVER_ID); + if (serverDef == null) { + var ex = new IllegalStateException( + "Language server definition not found for " + CopilotLanguageServerConnection.SERVER_ID); + CopilotCore.LOGGER.error(ex); + throw ex; + } - Job initJob = new Job("GitHub Copilot Initialization...") { - protected IStatus run(IProgressMonitor monitor) { - initRunnable.run(); + LanguageServerWrapper wrapper = LanguageServiceAccessor.startLanguageServer(serverDef); + CopilotLanguageServerConnection connection = new CopilotLanguageServerConnection(wrapper); + copilotLanguageServer = connection; + if (monitor.isCanceled() || stopping.get()) { + connection.stop(); + return Status.CANCEL_STATUS; + } + + exceptionReporter.setSink(connection::sendExceptionTelemetry); + authStatusManager = new AuthStatusManager(connection); + completionProvider = new CompletionProvider(connection, authStatusManager); + featureFlags = new FeatureFlags(); + if (monitor.isCanceled() || stopping.get()) { + connection.stop(); + return Status.CANCEL_STATUS; + } return Status.OK_STATUS; } @@ -111,33 +140,6 @@ public boolean belongsTo(Object family) { initJob.schedule(); } - /** - * Add platform level log listener to catch the uncaught exceptions. - */ - private void addPlatformLogListener() { - Platform.addLogListener((status, plugin) -> { - if (status.getSeverity() != IStatus.ERROR || plugin.equals(Constants.PLUGIN_ID)) { - // only send telemetry for those errors that are not from the plugin itself - return; - } - Throwable rawException = status.getException(); - if (rawException == null) { - return; - } - Throwable currentException = rawException; - do { - StackTraceElement[] traces = currentException.getStackTrace(); - for (StackTraceElement trace : traces) { - if (!trace.getClassName().startsWith(Constants.PLUGIN_ID)) { - continue; - } - reportException(rawException); - return; - } - } while ((currentException = currentException.getCause()) != null); - }); - } - public CopilotLanguageServerConnection getCopilotLanguageServer() { return copilotLanguageServer; } @@ -160,10 +162,6 @@ public NextEditSuggestionProvider getNextEditSuggestionProvider() { return nextEditSuggestionProvider; } - public GithubPanicErrorReport getGithubPanicErrorReport() { - return githubPanicErrorReport; - } - public FeatureFlags getFeatureFlags() { return featureFlags; } @@ -179,18 +177,12 @@ public FormatOptionProvider getFormatOptionProvider() { } /** - * Report the exception to the telemetry. + * Report an exception without blocking the caller. * * @param ex the exception to report */ public void reportException(Throwable ex) { - if (this.copilotLanguageServer != null) { - this.copilotLanguageServer.sendExceptionTelemetry(ex); - } else { - if (this.githubPanicErrorReport != null) { - this.githubPanicErrorReport.report(ex); - } - } + exceptionReporter.report(ex); } /** diff --git a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/logger/ExceptionReporter.java b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/logger/ExceptionReporter.java new file mode 100644 index 000000000..62d350549 --- /dev/null +++ b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/logger/ExceptionReporter.java @@ -0,0 +1,120 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +package com.microsoft.copilot.eclipse.core.logger; + +import java.util.Objects; +import java.util.concurrent.ArrayBlockingQueue; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; + +import org.eclipse.core.runtime.ILogListener; +import org.eclipse.core.runtime.IStatus; +import org.eclipse.core.runtime.Platform; + +import com.microsoft.copilot.eclipse.core.Constants; + +/** + * Collects and dispatches best-effort exception reports without blocking the logging thread. + */ +public final class ExceptionReporter implements AutoCloseable { + private static final int DEFAULT_QUEUE_CAPACITY = 32; + private static final long SHUTDOWN_TIMEOUT_SECONDS = 1L; + + private final ThreadPoolExecutor executor; + private final AtomicBoolean closed = new AtomicBoolean(); + private final AtomicReference> sink = new AtomicReference<>(); + private final ILogListener platformLogListener = this::handlePlatformLog; + + /** + * Creates an exception reporter. + */ + public ExceptionReporter() { + this(DEFAULT_QUEUE_CAPACITY); + } + + ExceptionReporter(int queueCapacity) { + this.executor = new ThreadPoolExecutor(1, 1, 0L, TimeUnit.MILLISECONDS, + new ArrayBlockingQueue<>(queueCapacity), runnable -> { + Thread thread = new Thread(runnable, "Copilot exception reporter"); + thread.setDaemon(true); + return thread; + }, new ThreadPoolExecutor.DiscardPolicy()); + } + + /** + * Starts collecting exceptions from the Eclipse platform log. + */ + public void start() { + Platform.addLogListener(platformLogListener); + } + + /** + * Sets the destination for exception reports. Reports are discarded until a sink is available. + * + * @param reportSink consumes exceptions on the reporter thread + */ + public void setSink(Consumer reportSink) { + if (closed.get()) { + return; + } + sink.set(Objects.requireNonNull(reportSink)); + // close() may have run between the check above and the set, in which case it already cleared the + // sink; re-check so a late registration cannot resurrect it. + if (closed.get()) { + sink.set(null); + } + } + + /** + * Queues an exception report. Reports are silently discarded when the queue is full or the + * reporter is closed. + * + * @param exception the exception to report + */ + public void report(Throwable exception) { + Consumer currentSink = sink.get(); + if (currentSink != null) { + executor.execute(() -> currentSink.accept(exception)); + } + } + + private void handlePlatformLog(IStatus status, String plugin) { + if (status.getSeverity() != IStatus.ERROR || Constants.PLUGIN_ID.equals(plugin)) { + return; + } + + Throwable rawException = status.getException(); + if (rawException == null) { + return; + } + + Throwable currentException = rawException; + do { + for (StackTraceElement trace : currentException.getStackTrace()) { + if (trace.getClassName().startsWith(Constants.PLUGIN_ID)) { + report(rawException); + return; + } + } + } while ((currentException = currentException.getCause()) != null); + } + + @Override + public void close() { + if (!closed.compareAndSet(false, true)) { + return; + } + Platform.removeLogListener(platformLogListener); + sink.set(null); + executor.shutdownNow(); + try { + executor.awaitTermination(SHUTDOWN_TIMEOUT_SECONDS, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } +} diff --git a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/logger/GithubPanicErrorReport.java b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/logger/GithubPanicErrorReport.java deleted file mode 100644 index 6adf11188..000000000 --- a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/logger/GithubPanicErrorReport.java +++ /dev/null @@ -1,287 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -package com.microsoft.copilot.eclipse.core.logger; - -import java.io.IOException; -import java.net.NetworkInterface; -import java.security.KeyManagementException; -import java.security.KeyStoreException; -import java.security.MessageDigest; -import java.security.NoSuchAlgorithmException; -import java.security.cert.X509Certificate; -import java.time.Instant; -import java.time.ZoneOffset; -import java.time.format.DateTimeFormatter; -import java.util.ArrayList; -import java.util.Collections; -import java.util.Enumeration; -import java.util.HashMap; -import java.util.Locale; -import java.util.Map; -import java.util.UUID; -import javax.net.ssl.SSLContext; - -import com.google.gson.Gson; -import com.google.gson.GsonBuilder; -import org.apache.hc.client5.http.impl.classic.CloseableHttpClient; -import org.apache.hc.client5.http.impl.classic.HttpClientBuilder; -import org.apache.hc.client5.http.impl.io.BasicHttpClientConnectionManager; -import org.apache.hc.client5.http.socket.ConnectionSocketFactory; -import org.apache.hc.client5.http.socket.PlainConnectionSocketFactory; -import org.apache.hc.client5.http.ssl.SSLConnectionSocketFactory; -import org.apache.hc.core5.http.ClassicHttpRequest; -import org.apache.hc.core5.http.HttpHost; -import org.apache.hc.core5.http.config.Registry; -import org.apache.hc.core5.http.config.RegistryBuilder; -import org.apache.hc.core5.http.io.support.ClassicRequestBuilder; -import org.apache.hc.core5.ssl.SSLContextBuilder; -import org.apache.hc.core5.ssl.TrustStrategy; -import org.eclipse.core.net.proxy.IProxyData; -import org.osgi.framework.Bundle; - -import com.microsoft.copilot.eclipse.core.CopilotCore; -import com.microsoft.copilot.eclipse.core.lsp.protocol.TelemetryExceptionParams; -import com.microsoft.copilot.eclipse.core.utils.PlatformUtils; - -/** - * The panic error report for Github. - */ -public class GithubPanicErrorReport { - private static final String PANIC_ENDPOINT = "https://copilot-telemetry.githubusercontent.com/telemetry"; - private static final Gson GSON = new GsonBuilder().create(); - private static final DateTimeFormatter dateFormat = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSSSSSSSX") - .withZone(ZoneOffset.UTC); - public static final String PLUGIN_VERSION = "editor_plugin_version"; - public static final String VERSION = "editor_version"; - private static String sessionId = UUID.randomUUID().toString(); - private static BasicHttpClientConnectionManager connectionManager = null; - private IProxyData proxyData; - private boolean proxyStrictSsl; - - public void setProxyData(IProxyData proxyData) { - this.proxyData = proxyData; - } - - public void setProxyStrictSsl(boolean proxyStrictSsl) { - this.proxyStrictSsl = proxyStrictSsl; - } - - /** - * The message. - * - * @throws IOException dsfdsg. - */ - public void report(Throwable ex) { - // payload - TelemetryExceptionParams params = new TelemetryExceptionParams(ex); - var failbotPayload = Map.of("context", Map.of(), "app", "copilot-eclipse", "catalog_service", "CopilotEclipse", - "release", "copilot-eclipse@" + PlatformUtils.getBundleVersion(), "rollup_id", "auto", "platform", "eclipse", - "exception_detail", params.getExceptionDetail()); - var payload = createExceptionPayload(TelemetryChannel.Standard, ex, - Map.of("failbot_payload", GSON.toJson(failbotPayload))); - String payloadStr = GSON.toJson(new Object[] { payload }); - - // build client - HttpClientBuilder clientBuilder = HttpClientBuilder.create(); - clientBuilder = configureHttpClient(clientBuilder, proxyData, proxyStrictSsl); - CloseableHttpClient client = clientBuilder.build(); - - // send panic report - ClassicHttpRequest req = ClassicRequestBuilder.post(PANIC_ENDPOINT).addHeader("accept", "application/json") - .addHeader("Content-Type", "application/json").setEntity(payloadStr).build(); - try { - client.execute(req, response -> { - if (response.getCode() != 200) { - throw new RuntimeException("Failed to send panic report: " + response.getCode()); - } - return null; - }); - } catch (IOException e) { - // do nothing if panic log fails. - } - } - - private static HttpClientBuilder configureHttpClient(HttpClientBuilder builder, IProxyData proxyData, - boolean proxyStrictSsl) { - if (proxyData == null || proxyData.getHost().isEmpty()) { - return builder; - } - - HttpHost proxy = new HttpHost(proxyData.getType(), proxyData.getHost(), proxyData.getPort()); - builder = builder.setProxy(proxy); - if (proxyStrictSsl) { - return builder; - } - if (connectionManager != null) { - return builder.setConnectionManager(connectionManager); - } - try { - TrustStrategy acceptingTrustStrategy = (X509Certificate[] chain, String authType) -> true; - SSLContext sslContext = SSLContextBuilder.create().loadTrustMaterial(null, acceptingTrustStrategy).build(); - SSLConnectionSocketFactory sslsf = new SSLConnectionSocketFactory(sslContext, (hostname, session) -> true); - Registry socketFactoryRegistry = RegistryBuilder.create() - .register("https", sslsf).register("http", new PlainConnectionSocketFactory()).build(); - - connectionManager = new BasicHttpClientConnectionManager(socketFactoryRegistry); - builder = builder.setConnectionManager(connectionManager); - } catch (KeyManagementException e) { - // do nothing if panic log fails. - } catch (NoSuchAlgorithmException e) { - // do nothing if panic log fails. - } catch (KeyStoreException e) { - // do nothing if panic log fails. - } - return builder; - } - - private static Map createExceptionPayload(TelemetryChannel channel, Throwable throwable, - Map properties) { - HashMap dataProperties = new HashMap<>(); - dataProperties.putAll(CopilotTelemetryContext.staticProperties); - dataProperties.putAll(properties); - return createPayload(channel, "Error", - Map.of( - "baseType", "ExceptionData", "baseData", Map.of("ver", 2, "severityLevel", "Error", "exceptions", - new ArrayList(), "name", "agent/error.exception", "properties", dataProperties), - "measurements", Map.of())); - } - - /** - * Computes the file name from the stack trace element. - * - * @param element the stack trace element - * @return the computed file name - */ - public static String computeFileName(StackTraceElement element) { - String[] classNameParts = element.getClassName().split("\\."); - classNameParts[classNameParts.length - 1] = element.getFileName(); - return String.join("/", classNameParts); - } - - private static Map createPayload(TelemetryChannel channel, String severityLevel, - Map dataProperties) { - return Map.of("ver", 1, "time", dateFormat.format(Instant.now()), "severityLevel", severityLevel, "name", - "Microsoft.ApplicationInsights.standard.Event", "iKey", channel.getKey(), "data", dataProperties); - } - - /** - * The telemetry channel. - */ - public enum TelemetryChannel { - Standard, Restricted; - - String getKey() { - switch (this) { - case Standard: - return "7d7048df-6dd0-4048-bb23-b716c1461f8f"; - case Restricted: - return "3fdd7f28-937a-48c8-9a21-ba337db23bd1"; - default: - throw new IllegalStateException("Unsupported value: " + this); - } - } - } - - final class CopilotTelemetryContext { - - private CopilotTelemetryContext() { - } - - static final Map staticProperties; - - static { - HashMap properties = new HashMap<>(); - // machine id - properties.put("common_vscodemachineid", getMacMachineId()); - properties.put("client_machineid", getMacMachineId()); - - // // session - properties.put("common_vscodesessionid", sessionId); - properties.put("client_sessionid", sessionId); - - // os - properties.put("common_os", vscodeOsName()); - properties.put("common_platformversion", getOsVersion()); - properties.put("common_uikind", "desktop"); - - // ide version - properties.put(VERSION, "Eclipse/" + PlatformUtils.getEclipseVersionString()); - properties.put("common_extname", "copilot-eclipse"); - - String bundleVersion = PlatformUtils.getBundleVersion(); - properties.put("common_extversion", bundleVersion); - properties.put(PLUGIN_VERSION, "copilot-eclipse/" + bundleVersion); - // build - properties.put("copilot_build", build()); - properties.put("copilot_buildType", String.valueOf(isDeveloperMode())); - - staticProperties = Collections.unmodifiableMap(properties); - } - - /** - * Mimics VSCode's OS name. - */ - private static String vscodeOsName() { - if (PlatformUtils.isWindows()) { - return "win32"; - } - if (PlatformUtils.isLinux()) { - return "linux"; - } - if (PlatformUtils.isMac()) { - return "darwin"; - } - return "unknown"; - } - - static boolean isDeveloperMode() { - return true; - } - - /** - * Mimics VSCode's build property. - */ - static String build() { - if (isDeveloperMode()) { - return "dev"; - } - return ""; - } - - } - - private static String getMacMachineId() { - try { - Enumeration interfaces = NetworkInterface.getNetworkInterfaces(); - while (interfaces.hasMoreElements()) { - NetworkInterface iface = interfaces.nextElement(); - byte[] mac = iface.getHardwareAddress(); - if (mac != null && mac.length > 0) { - MessageDigest digest = MessageDigest.getInstance("SHA-256"); - digest.update(mac); - - // Convert the hash to a hexadecimal string - byte[] hashBytes = digest.digest(); - StringBuilder sb = new StringBuilder(); - for (byte b : hashBytes) { - sb.append(String.format("%02x", b)); - } - return sb.toString(); - } - } - } catch (Throwable t) { - // do nothing because nothing can handle this exception when panic - // throw new RuntimeException(t); - return "unknown"; - } - // do nothing because nothing can handle this exception when panic - // throw new RuntimeException("Failed to retrieve MAC address"); - return "unknown"; - } - - private static String getOsVersion() { - String version = System.getProperty("os.version").toLowerCase(Locale.ENGLISH); - return version; - } -} 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 e0f533be0..467bb5131 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 @@ -6,6 +6,8 @@ import java.net.URI; import java.util.List; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; import org.apache.commons.lang3.StringUtils; @@ -91,7 +93,10 @@ public class CopilotLanguageServerConnection { public static final String SERVER_ID = "com.microsoft.copilot.eclipse.ls"; - private LanguageServerWrapper languageServerWrapper; + private final LanguageServerWrapper languageServerWrapper; + private final AtomicBoolean stopped = new AtomicBoolean(); + private final AtomicReference>> exceptionSink = + new AtomicReference<>(); /** * Constructor for the CopilotLanguageServer. @@ -100,6 +105,22 @@ public class CopilotLanguageServerConnection { */ public CopilotLanguageServerConnection(LanguageServerWrapper languageServerWrapper) { this.languageServerWrapper = languageServerWrapper; + initializeExceptionSink(); + } + + private void initializeExceptionSink() { + Function> fn = server -> { + if (!stopped.get()) { + exceptionSink.set(((CopilotLanguageServer) server)::sendExceptionTelemetry); + // stop() may have run between the check above and the set, in which case it already cleared + // the sink; re-check so a late registration cannot resurrect it. + if (stopped.get()) { + exceptionSink.set(null); + } + } + return CompletableFuture.completedFuture(null); + }; + this.languageServerWrapper.execute(fn).exceptionally(exception -> null); } /** @@ -248,16 +269,35 @@ public CompletableFuture notifyRejected(NotifyRejectedParams params) { } /** - * Send the exception telemetry to the language server. + * Send the exception telemetry to the language server. Reporting is best-effort: the report is silently dropped + * when no running server connection is available, and a failed send never propagates to the caller. */ public CompletableFuture sendExceptionTelemetry(Throwable ex) { - TelemetryExceptionParams telemParams = new TelemetryExceptionParams(ex); - Function> fn = server -> ((CopilotLanguageServer) server) - .sendExceptionTelemetry(telemParams); - return this.languageServerWrapper.execute(fn).exceptionally(exception -> { - // Ignore exceptions to avoid infinite loop. - return null; - }); + Function> sink = exceptionSink.get(); + if (sink == null) { + return CompletableFuture.completedFuture(null); + } + + try { + return sink.apply(new TelemetryExceptionParams(ex)).exceptionally(exception -> { + discardSinkIfServerStopped(sink); + return null; + }); + } catch (RuntimeException exception) { + discardSinkIfServerStopped(sink); + return CompletableFuture.completedFuture(null); + } + } + + /** + * Drops the cached sink only when the server is no longer running. A single failed send is not enough to infer + * that the connection is gone, and clearing the sink eagerly would silence exception reporting for the rest of + * the session. + */ + private void discardSinkIfServerStopped(Function> sink) { + if (stopped.get() || !languageServerWrapper.isActive()) { + exceptionSink.compareAndSet(sink, null); + } } /** @@ -694,7 +734,10 @@ public CompletableFuture acceptNextEditSuggestion(Command command) { * Stop the language server. */ public void stop() { - this.languageServerWrapper.stop(); + if (stopped.compareAndSet(false, true)) { + exceptionSink.set(null); + this.languageServerWrapper.stop(); + } } private String getModelName(CopilotModel activeModel) { diff --git a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/protocol/TelemetryExceptionParams.java b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/protocol/TelemetryExceptionParams.java index 03b38c4c5..22161946c 100644 --- a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/protocol/TelemetryExceptionParams.java +++ b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/protocol/TelemetryExceptionParams.java @@ -10,7 +10,6 @@ import com.google.gson.annotations.SerializedName; import org.apache.commons.lang3.builder.ToStringBuilder; -import com.microsoft.copilot.eclipse.core.logger.GithubPanicErrorReport; import com.microsoft.copilot.eclipse.core.utils.AnonymizeUtils; /** @@ -111,6 +110,12 @@ public String toString() { return builder.toString(); } + private static String computeFileName(StackTraceElement element) { + String[] classNameParts = element.getClassName().split("\\."); + classNameParts[classNameParts.length - 1] = element.getFileName(); + return String.join("/", classNameParts); + } + /** * The type of the exception. */ @@ -160,7 +165,7 @@ public void setStacktrace(StackTraceElement[] stacktrace) { int idx = 0; for (StackTraceElement element : stacktrace) { var cst = new CopilotStackTraceElement(); - cst.setFilename(GithubPanicErrorReport.computeFileName(element)); + cst.setFilename(computeFileName(element)); cst.setLineno(String.valueOf(element.getLineNumber())); // colno is not available in StackTraceElement cst.setColno(String.valueOf(0)); diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/LanguageServerSettingManager.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/LanguageServerSettingManager.java index 26b2c0241..452dc9292 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/LanguageServerSettingManager.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/preferences/LanguageServerSettingManager.java @@ -56,7 +56,6 @@ public class LanguageServerSettingManager implements IProxyChangeListener, IProp CopilotLanguageServerSettings settings = new CopilotLanguageServerSettings(); CopilotLanguageServerConnection copilotLanguageServerConnection = null; IPreferenceStore preferenceStore; - IProxyData proxyData = null; private IEventBroker eventBroker; /** @@ -137,7 +136,6 @@ public LanguageServerSettingManager(CopilotLanguageServerConnection conn, IProxy @Override public void proxyInfoChanged(IProxyChangeEvent event) { updateProxySettings(); - updateGithubPanicErrorReport(); syncSingleConfiguration(new CopilotLanguageServerSettings(null, settings.getHttp(), null, null)); } @@ -156,7 +154,6 @@ public void propertyChange(PropertyChangeEvent event) { case Constants.ENABLE_STRICT_SSL: settings.getHttp().setProxyStrictSsl(preferenceStore.getBoolean(Constants.ENABLE_STRICT_SSL)); singleSetting = new CopilotLanguageServerSettings(null, settings.getHttp(), null, null); - updateGithubPanicErrorReport(); break; case Constants.PROXY_KERBEROS_SP: settings.getHttp().setProxyKerberosServicePrincipal(preferenceStore.getString(Constants.PROXY_KERBEROS_SP)); @@ -232,7 +229,6 @@ public void propertyChange(PropertyChangeEvent event) { public void syncConfiguration() { DidChangeConfigurationParams params = new DidChangeConfigurationParams(); params.setSettings(settings); - updateGithubPanicErrorReport(); this.copilotLanguageServerConnection.updateConfig(params); } @@ -245,14 +241,6 @@ public void syncSingleConfiguration(CopilotLanguageServerSettings singleSetting) this.copilotLanguageServerConnection.updateConfig(params); } - private void updateGithubPanicErrorReport() { - CopilotCore copilotCore = CopilotCore.getPlugin(); - if (copilotCore != null && copilotCore.getGithubPanicErrorReport() != null) { - copilotCore.getGithubPanicErrorReport().setProxyStrictSsl(settings.getHttp().isProxyStrictSsl()); - copilotCore.getGithubPanicErrorReport().setProxyData(proxyData); - } - } - /** * Sync MCP registration from both extension points and preference store. */ @@ -518,7 +506,7 @@ private void updateMcpToolsStatus(String mcpToolsStatus, String modeId) { * Updates the proxy settings. */ public void updateProxySettings() { - proxyData = getProxy(); + IProxyData proxyData = getProxy(); settings.getHttp().setProxy(createProxyString(proxyData)); settings.getHttp().setProxyAuthorization(createProxyAuthString(proxyData)); settings.getHttp().setNoProxy(getNonProxiedHosts()); From df3fa1f0d1b0d6fad9ebeab9da45f927722ce9a3 Mon Sep 17 00:00:00 2001 From: Sheng Chen Date: Mon, 3 Aug 2026 15:52:53 +0800 Subject: [PATCH 2/8] fix: bound the shutdown wait for the initialization job CopilotCore.stop() cancelled the initialization job and then joined it without a timeout. Job.cancel() only raises a flag that the job has to poll, and the job can be blocked inside LanguageServiceAccessor.startLanguageServer(...) where there is no such checkpoint, so the join could wait forever and stall the whole OSGi framework shutdown. When that happens in a test JVM the process never exits and the CI job hangs until the runner limit. Join with a 30 s bound instead and log an error when it expires. Giving up is safe: `stopping` is set before the join and the job re-checks it right after it publishes the connection, so the job stops the connection itself; CopilotLanguageServerConnection.stop() is guarded by a compareAndSet and is therefore idempotent. Also give the CI job an explicit timeout-minutes. Without one a hung test JVM keeps the job alive for the 6 hour default and the logs are never published, which is what made these hangs impossible to diagnose. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: edb91a65-6071-4ed5-84a9-7e534ea391ad --- .github/workflows/ci.yml | 3 +++ .../copilot/eclipse/core/CopilotCore.java | 23 +++++++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a77cc8025..611188bb5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,6 +13,9 @@ jobs: build: runs-on: ${{ matrix.os }} + # Without an explicit limit a hung test JVM keeps the job alive for the 6 hour default and the + # logs are never published, which makes such hangs impossible to diagnose. + timeout-minutes: 45 strategy: matrix: os: [ubuntu-latest, macos-latest, windows-latest] diff --git a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/CopilotCore.java b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/CopilotCore.java index f5f23149a..27b72a2c8 100644 --- a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/CopilotCore.java +++ b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/CopilotCore.java @@ -8,6 +8,7 @@ import org.eclipse.core.runtime.IProgressMonitor; import org.eclipse.core.runtime.IStatus; +import org.eclipse.core.runtime.NullProgressMonitor; import org.eclipse.core.runtime.Plugin; import org.eclipse.core.runtime.Status; import org.eclipse.core.runtime.jobs.Job; @@ -52,6 +53,12 @@ public class CopilotCore extends Plugin { */ public static final String INIT_JOB_FAMILY = "com.microsoft.copilot.eclipse.core.initJob"; + /** + * How long {@link #stop(BundleContext)} waits for the initialization job to observe the + * cancellation before it gives up and lets the framework shutdown continue. + */ + private static final long INIT_JOB_SHUTDOWN_TIMEOUT_MS = 30_000L; + /** * Creates the Copilot core plugin. The plugin is created automatically by the Eclipse framework. Clients must not * call this constructor. @@ -77,11 +84,19 @@ public void stop(BundleContext context) throws Exception { stopping.set(true); exceptionReporter.close(); try { - if (initJob != null) { + Job job = initJob; + if (job != null) { // Blocking is intentional: the bundle must not be unloaded while the initialization job is - // still touching the language server, so wait for it to observe the cancellation. - initJob.cancel(); - initJob.join(); + // still touching the language server, so wait for it to observe the cancellation. The wait + // is bounded because cancel() only raises a flag the job has to poll, and the job may be + // blocked inside language server startup where there is no such checkpoint; waiting forever + // there would stall the whole framework shutdown. Giving up is safe: `stopping` is already + // set, so the job stops the connection itself at its next checkpoint. + job.cancel(); + if (!job.join(INIT_JOB_SHUTDOWN_TIMEOUT_MS, new NullProgressMonitor())) { + LOGGER.error(new IllegalStateException("Gave up waiting for the GitHub Copilot initialization" + + " job to stop after " + INIT_JOB_SHUTDOWN_TIMEOUT_MS + " ms.")); + } } } catch (InterruptedException e) { Thread.currentThread().interrupt(); From e8efc91e8c9f87d9c4ac6a7f477eac8ee4b189dd Mon Sep 17 00:00:00 2001 From: Sheng Chen Date: Mon, 3 Aug 2026 16:16:24 +0800 Subject: [PATCH 3/8] fix: keep the class name when a stack frame has no source file StackTraceElement.getFileName() returns null when the class carries no SourceFile attribute, which happens for code compiled with -g:none and for some generated classes. computeFileName() assigned that null into the class name parts, so String.join spliced the literal string "null" into the reported path and the telemetry frame came out as "com/microsoft/copilot/null". Fall back to the simple class name. Contrary to the review comment this was never an NPE: String.join appends "null" for a null element rather than throwing, so the payload was still built, just with a corrupted file name. Also add a regression test showing report() does not throw when close() races it. ThreadPoolExecutor routes a post-shutdown submission through the rejection handler, and DiscardPolicy drops it silently, so the best-effort contract already held; the test pins that behaviour. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: edb91a65-6071-4ed5-84a9-7e534ea391ad --- .../core/logger/ExceptionReporterTests.java | 30 ++++++++++++++++ .../TelemetryExceptionParamsTests.java | 36 +++++++++++++++++++ .../protocol/TelemetryExceptionParams.java | 7 +++- 3 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/protocol/TelemetryExceptionParamsTests.java diff --git a/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/logger/ExceptionReporterTests.java b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/logger/ExceptionReporterTests.java index 69d4a994f..7d14065c5 100644 --- a/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/logger/ExceptionReporterTests.java +++ b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/logger/ExceptionReporterTests.java @@ -5,6 +5,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.concurrent.CountDownLatch; @@ -89,4 +90,33 @@ void testReport_discardsWhenQueueIsFull() throws InterruptedException { assertEquals(2, reportCount.get()); } } + + @Test + void testReport_concurrentWithCloseDoesNotThrow() throws InterruptedException { + ExceptionReporter reporter = new ExceptionReporter(); + reporter.setSink(exception -> { + }); + AtomicReference failure = new AtomicReference<>(); + CountDownLatch finished = new CountDownLatch(1); + + // report() reads the sink and submits to the executor in two steps, so close() can land in + // between. Reporting runs on the platform logging thread and must never propagate a failure + // there, so the rejected submission has to be discarded instead. + Thread reporting = new Thread(() -> { + try { + for (int i = 0; i < 2000; i++) { + reporter.report(new IllegalStateException("test")); + } + } catch (Throwable e) { + failure.set(e); + } finally { + finished.countDown(); + } + }); + reporting.start(); + reporter.close(); + + assertTrue(finished.await(10, TimeUnit.SECONDS)); + assertNull(failure.get()); + } } diff --git a/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/protocol/TelemetryExceptionParamsTests.java b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/protocol/TelemetryExceptionParamsTests.java new file mode 100644 index 000000000..8a7e86d80 --- /dev/null +++ b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/lsp/protocol/TelemetryExceptionParamsTests.java @@ -0,0 +1,36 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +package com.microsoft.copilot.eclipse.core.lsp.protocol; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.junit.jupiter.api.Test; + +class TelemetryExceptionParamsTests { + + @Test + void testConstructor_withSourceFileBuildsPath() { + var params = new TelemetryExceptionParams(exceptionWithFrame("SampleClass.java")); + + assertEquals("com/microsoft/copilot/SampleClass.java", firstFrameFilename(params)); + } + + @Test + void testConstructor_withoutSourceFileKeepsClassName() { + var params = new TelemetryExceptionParams(exceptionWithFrame(null)); + + assertEquals("com/microsoft/copilot/SampleClass", firstFrameFilename(params)); + } + + private static Throwable exceptionWithFrame(String fileName) { + var exception = new IllegalStateException("boom"); + exception.setStackTrace(new StackTraceElement[] { + new StackTraceElement("com.microsoft.copilot.SampleClass", "run", fileName, 42) }); + return exception; + } + + private static String firstFrameFilename(TelemetryExceptionParams params) { + return params.getExceptionDetail().get(0).getStacktrace()[0].getFilename(); + } +} diff --git a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/protocol/TelemetryExceptionParams.java b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/protocol/TelemetryExceptionParams.java index 22161946c..f2451fe1a 100644 --- a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/protocol/TelemetryExceptionParams.java +++ b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/lsp/protocol/TelemetryExceptionParams.java @@ -112,7 +112,12 @@ public String toString() { private static String computeFileName(StackTraceElement element) { String[] classNameParts = element.getClassName().split("\\."); - classNameParts[classNameParts.length - 1] = element.getFileName(); + String fileName = element.getFileName(); + // getFileName() is null when the class carries no SourceFile attribute; keep the simple class + // name then, otherwise the literal string "null" would be spliced into the reported path. + if (fileName != null) { + classNameParts[classNameParts.length - 1] = fileName; + } return String.join("/", classNameParts); } From a666b299fdbc13e37244a1de201a6f1bf958515a Mon Sep 17 00:00:00 2001 From: Sheng Chen Date: Mon, 3 Aug 2026 16:59:36 +0800 Subject: [PATCH 4/8] fix: bound the whole language server wind-down on shutdown Bounding only the initialization job join was not enough. Once the join gives up, stop() still called connection.stop() on the caller thread, and LanguageServerWrapper guards start() and stop() with the same monitor: if the init job is parked inside an in-flight start() the stop() call blocks behind it for as long as that start() takes. On CI the server never finishes starting. The macOS runner has no usable keychain, so the language server logs "getMasterKey: timeout after 5000ms" and stalls, which leaves start() holding the monitor indefinitely. CopilotCore.stop() then never returns, the OSGi framework never finishes stopping, and the surefire JVM never exits. Move the join and the connection stop onto a daemon thread and wait on that thread with a single budget, so a wedged server can only delay the framework shutdown rather than block it forever. The thread is a daemon so it cannot by itself keep the JVM alive once the framework is down. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: edb91a65-6071-4ed5-84a9-7e534ea391ad --- .../copilot/eclipse/core/CopilotCore.java | 67 +++++++++++++------ 1 file changed, 45 insertions(+), 22 deletions(-) diff --git a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/CopilotCore.java b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/CopilotCore.java index 27b72a2c8..f9412c413 100644 --- a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/CopilotCore.java +++ b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/CopilotCore.java @@ -8,7 +8,6 @@ import org.eclipse.core.runtime.IProgressMonitor; import org.eclipse.core.runtime.IStatus; -import org.eclipse.core.runtime.NullProgressMonitor; import org.eclipse.core.runtime.Plugin; import org.eclipse.core.runtime.Status; import org.eclipse.core.runtime.jobs.Job; @@ -54,10 +53,10 @@ public class CopilotCore extends Plugin { public static final String INIT_JOB_FAMILY = "com.microsoft.copilot.eclipse.core.initJob"; /** - * How long {@link #stop(BundleContext)} waits for the initialization job to observe the - * cancellation before it gives up and lets the framework shutdown continue. + * How long {@link #stop(BundleContext)} waits for the language server wind-down before it gives + * up and lets the framework shutdown continue. */ - private static final long INIT_JOB_SHUTDOWN_TIMEOUT_MS = 30_000L; + private static final long SHUTDOWN_TIMEOUT_MS = 30_000L; /** * Creates the Copilot core plugin. The plugin is created automatically by the Eclipse framework. Clients must not @@ -83,29 +82,53 @@ public void start(BundleContext context) throws Exception { public void stop(BundleContext context) throws Exception { stopping.set(true); exceptionReporter.close(); - try { - Job job = initJob; - if (job != null) { - // Blocking is intentional: the bundle must not be unloaded while the initialization job is - // still touching the language server, so wait for it to observe the cancellation. The wait - // is bounded because cancel() only raises a flag the job has to poll, and the job may be - // blocked inside language server startup where there is no such checkpoint; waiting forever - // there would stall the whole framework shutdown. Giving up is safe: `stopping` is already - // set, so the job stops the connection itself at its next checkpoint. - job.cancel(); - if (!job.join(INIT_JOB_SHUTDOWN_TIMEOUT_MS, new NullProgressMonitor())) { - LOGGER.error(new IllegalStateException("Gave up waiting for the GitHub Copilot initialization" - + " job to stop after " + INIT_JOB_SHUTDOWN_TIMEOUT_MS + " ms.")); + + Job job = initJob; + if (job != null) { + // cancel() only raises a flag the job has to poll, so it cannot unblock a job that is already + // inside language server startup; the wait below is what keeps that bounded. + job.cancel(); + } + + // The wind-down can block for an unbounded time: LanguageServerWrapper guards start() and + // stop() with the same monitor, so stop() waits for an in-flight start() that may itself be + // waiting on a server process that never finishes initializing. Run it on a daemon thread with + // a budget so a wedged server delays the framework shutdown instead of blocking it forever. + Thread shutdown = new Thread(() -> { + try { + if (job != null) { + job.join(); } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } finally { + stopLanguageServer(); } + }, "Copilot shutdown"); + shutdown.setDaemon(true); + shutdown.start(); + + try { + shutdown.join(SHUTDOWN_TIMEOUT_MS); } catch (InterruptedException e) { Thread.currentThread().interrupt(); throw e; - } finally { - CopilotLanguageServerConnection connection = copilotLanguageServer; - if (connection != null) { - connection.stop(); - } + } + if (shutdown.isAlive()) { + LOGGER.error(new IllegalStateException("Gave up waiting for the GitHub Copilot language server" + + " to shut down after " + SHUTDOWN_TIMEOUT_MS + " ms.")); + } + } + + private void stopLanguageServer() { + CopilotLanguageServerConnection connection = copilotLanguageServer; + if (connection == null) { + return; + } + try { + connection.stop(); + } catch (RuntimeException e) { + LOGGER.error(e); } } From 42d478bb55fb2c82794cabda3e2954c1f5427bd6 Mon Sep 17 00:00:00 2001 From: Sheng Chen Date: Tue, 4 Aug 2026 10:43:14 +0800 Subject: [PATCH 5/8] fix: stop the macOS login shell probe from pinning the test JVM The exception reporter in this PR removes the start/stop deadlock, but on macOS one more thing can keep a test JVM alive after the tests have finished, and it is macOS-only - which matches macOS accounting for 20 of the 27 hung CI runs. createProcessBuilder() calls getLoginShellEnvironment(), which is gated on isMac() and runs "/bin/zsh -i -l -c env". Its output was read through Executors.newSingleThreadExecutor(), whose default factory produces a *non-daemon* thread. Reads on a process pipe are not interruptible, so neither future.cancel(true) nor shutdownNow() can stop that reader once the shell goes quiet: the five second timeout returns to the caller, but the thread stays blocked and the JVM can no longer exit. An interactive login shell on a headless CI runner with no tty is exactly the case where that happens. The shell's stderr was a pipe nobody drained, too, so a profile chatty enough to fill the buffer could wedge the shell before it ever printed the environment. Make the reader a daemon so a stuck read can never outvote JVM shutdown, discard the shell's stderr, and actually tear the shell down instead of asking it once and moving on. Also bound the forked test JVM. timeout-minutes stops a hung job from burning six hours, but it kills the job before Maven prints anything; tycho-surefire only installs a watchdog when forkedProcessTimeoutInSeconds is set, so setting it makes a future hang fail with a stack trace rather than silence. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../core/lsp/LsStreamConnectionProvider.java | 87 ++++++++++++++----- pom.xml | 3 + 2 files changed, 68 insertions(+), 22 deletions(-) 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 925dcc086..99a38b246 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 @@ -47,6 +47,16 @@ public class LsStreamConnectionProvider extends ProcessStreamConnectionProvider public static final String EDITOR_NAME = "Eclipse"; public static final String EDITOR_PLUGIN_NAME = "copilot-eclipse"; + /** + * How long a process gets to exit on its own after being asked politely, before it is killed. + */ + private static final long PROCESS_TERMINATION_GRACE_MS = 2000L; + + /** + * How long the login shell gets to report its environment before the probe gives up. + */ + private static final long LOGIN_SHELL_TIMEOUT_SECONDS = 5L; + @Override public Object getInitializationOptions(@Nullable URI rootUri) { NameAndVersion editorInfo = new NameAndVersion(EDITOR_NAME, PlatformUtils.getEclipseVersionString()); @@ -107,6 +117,27 @@ private void startJsLspAgent() throws IOException { CopilotCore.LOGGER.info("JS agent started successfully."); } + /** + * Terminates a process and its descendants, first politely and then forcibly. + * + * @param process the process to terminate, may be {@code null} + */ + private static void terminateProcess(@Nullable Process process) { + if (process == null || !process.isAlive()) { + return; + } + process.destroy(); + try { + if (process.waitFor(PROCESS_TERMINATION_GRACE_MS, TimeUnit.MILLISECONDS)) { + return; + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + process.descendants().forEach(ProcessHandle::destroyForcibly); + process.destroyForcibly(); + } + private List getBinaryLspCommands() throws IOException { Path binary = findAndValidateBinary(); return buildCommands(binary.toString()); @@ -332,6 +363,9 @@ private Map getLoginShellEnvironment() { try { // Execute login shell and get environment variables ProcessBuilder pb = new ProcessBuilder("/bin/zsh", "-i", "-l", "-c", "env"); + // Nobody drains the shell's stderr, so a chatty shell profile would otherwise fill the pipe + // buffer and wedge the shell before it ever prints the environment. + pb.redirectError(ProcessBuilder.Redirect.DISCARD); process = pb.start(); env = getEnvironmentVariables(process); @@ -350,44 +384,53 @@ private Map getLoginShellEnvironment() { } catch (IOException e) { CopilotCore.LOGGER.error("IOException while getting login shell environment variables", e); } finally { - if (process != null && process.isAlive()) { - process.destroy(); - } + terminateProcess(process); } return env; } - private Map getEnvironmentVariables(Process process) throws InterruptedException { - // Create a separate thread to read the process output with a timeout, this avoids blocking the original thread - ExecutorService executor = Executors.newSingleThreadExecutor(); - Future> future = executor.submit(() -> { - Map result = new HashMap<>(); - try (BufferedReader reader = new BufferedReader( - new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8))) { - String line; - while ((line = reader.readLine()) != null) { - int separator = line.indexOf('='); - if (separator > 0) { - String key = line.substring(0, separator); - String value = line.substring(separator + 1); - result.put(key, value); - } - } - } - return result; + private Map getEnvironmentVariables(Process process) { + // Reads on a process pipe are not interruptible, so neither cancel(true) nor shutdownNow() can + // stop the reader once the shell stops producing output. The thread must therefore be a daemon, + // otherwise a wedged shell keeps the whole JVM alive. + ExecutorService executor = Executors.newSingleThreadExecutor(runnable -> { + Thread thread = new Thread(runnable, "GitHub Copilot login shell environment reader"); + thread.setDaemon(true); + return thread; }); + Future> future = executor.submit(() -> readEnvironmentVariables(process)); try { - return future.get(5, TimeUnit.SECONDS); + return future.get(LOGIN_SHELL_TIMEOUT_SECONDS, TimeUnit.SECONDS); } catch (TimeoutException e) { CopilotCore.LOGGER.error("Timed out waiting for login shell environment variables", e); future.cancel(true); } catch (ExecutionException e) { CopilotCore.LOGGER.error("Error reading login shell environment variables", e); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + CopilotCore.LOGGER.error("Interrupted while reading login shell environment variables", e); } finally { executor.shutdownNow(); } return new HashMap<>(); } + + private Map readEnvironmentVariables(Process process) throws IOException { + Map result = new HashMap<>(); + try (BufferedReader reader = new BufferedReader( + new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8))) { + String line; + while ((line = reader.readLine()) != null) { + int separator = line.indexOf('='); + if (separator > 0) { + String key = line.substring(0, separator); + String value = line.substring(separator + 1); + result.put(key, value); + } + } + } + return result; + } } diff --git a/pom.xml b/pom.xml index ae073c4a9..d731f0f83 100644 --- a/pom.xml +++ b/pom.xml @@ -15,6 +15,9 @@ GitHub Copilot 4.0.13 3.6.0 + + 900 From 580d542e612a757d0824972056c0cd3b166de131 Mon Sep 17 00:00:00 2001 From: Sheng Chen Date: Tue, 4 Aug 2026 11:24:44 +0800 Subject: [PATCH 6/8] fix: skip the SWTBot probe module when no probe script is given com.microsoft.copilot.eclipse.swtbot.test owns no tests of its own. ProbeRunner is a driver for the JSON script named by -Dprobe.script, and the default build never supplies one, so the module contributes exactly one skipped test. Getting to that skip is not free. It is the only module whose target-platform-configuration pulls com.microsoft.copilot.eclipse.feature into the test runtime, which is what makes the language server agent resolvable. Tycho therefore provisions and launches a full workbench, the Copilot plugin starts a real language server, and only then does ProbeRunner's @BeforeClass Assume fire. The server outlives the fork while still holding the inherited stderr file descriptor, and the Maven-side stream pump waits on an EOF that never comes - the build hangs, most often on macOS. Skip the execution instead. The new swtbot-probe profile switches it back on whenever -Dprobe.script is supplied, so the ui-action workflow is unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../pom.xml | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/com.microsoft.copilot.eclipse.swtbot.test/pom.xml b/com.microsoft.copilot.eclipse.swtbot.test/pom.xml index a01fae630..6c6903574 100644 --- a/com.microsoft.copilot.eclipse.swtbot.test/pom.xml +++ b/com.microsoft.copilot.eclipse.swtbot.test/pom.xml @@ -15,6 +15,14 @@ true + + true + + @@ -30,6 +38,19 @@ -XstartOnFirstThread + + + swtbot-probe + + + probe.script + + + + false + + @@ -59,6 +80,7 @@ tycho-surefire-plugin ${tycho-version} + ${swtbot.skipTests} true false From ee0909408ce70636270ee5a36932f478f71ed5bb Mon Sep 17 00:00:00 2001 From: Sheng Chen Date: Tue, 4 Aug 2026 11:33:52 +0800 Subject: [PATCH 7/8] test: kill the language server the LSP test starts testStartLanguageServer launches a real language server and then relies on LSP4E's stop(), which only sends a termination request and returns without waiting. On Unix that request can be ignored, and the surviving server still holds the stderr file descriptor it inherited from the test JVM. Inside a Tycho fork that descriptor is Maven's pipe, so the Maven-side stream pump waits for an EOF that never arrives and the build hangs after the tests have already passed. Observed directly: ten orphaned language-server processes from earlier runs of this module were still alive at the start of a later build. Terminate the process and its descendants from the test itself and assert it is gone, so the guarantee lives in test scope rather than in the runtime's language server lifecycle. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../lsp/LsStreamConnectionProviderTests.java | 48 ++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) 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 9aca6a8fc..95ac9fc7c 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 @@ -5,8 +5,10 @@ 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 java.io.IOException; +import java.util.concurrent.TimeUnit; import org.eclipse.core.runtime.preferences.IEclipsePreferences; import org.eclipse.core.runtime.preferences.InstanceScope; @@ -18,6 +20,7 @@ class LsStreamConnectionProviderTests { private static final String LEGACY_WORKSPACE_CONTEXT_PREFERENCE = "workspaceContextEnabled"; private static final String UI_PREFERENCE_NODE = "com.microsoft.copilot.eclipse.ui"; + private static final long TERMINATION_GRACE_MS = 2000L; @Test void testInitializationOptions() { @@ -52,11 +55,54 @@ void testInitializationIgnoresLegacyWorkspaceContextPreference() { @Test void testStartLanguageServer() throws IOException { - LsStreamConnectionProvider provider = new LsStreamConnectionProvider(); + TestableLsStreamConnectionProvider provider = new TestableLsStreamConnectionProvider(); + Process process = null; try { provider.start(); + process = provider.process(); + assertNotNull(process, "starting the provider must create a language server process"); } finally { provider.stop(); + forceTerminate(process); + } + + assertFalse(process.isAlive(), "the language server must not outlive the test"); + } + + /** + * Makes sure the language server is really gone before the test JVM exits. + * + *

LSP4E's {@code stop()} only asks the process to terminate and returns without waiting, and on + * Unix that request can be ignored. A surviving server still holds the stderr file descriptor it + * inherited from this JVM; inside a Tycho test fork that descriptor is Maven's pipe, so the + * Maven-side stream pump waits for an EOF that never arrives and the build hangs long after the + * tests have passed. Killing the process here keeps that failure mode out of CI without changing + * how the plugin manages language servers at runtime. + * + * @param process the language server process, or {@code null} if none was created + */ + private static void forceTerminate(Process process) { + if (process == null) { + return; + } + process.destroy(); + try { + if (process.waitFor(TERMINATION_GRACE_MS, TimeUnit.MILLISECONDS)) { + return; + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + process.descendants().forEach(ProcessHandle::destroyForcibly); + process.destroyForcibly(); + } + + /** + * Exposes the language server process so the test can assert on the real operating system process. + */ + private static final class TestableLsStreamConnectionProvider extends LsStreamConnectionProvider { + Process process() { + return getProcess(); } } } From 574064a3d8261e4f7b37cd14947bc7cfa4201244 Mon Sep 17 00:00:00 2001 From: Sheng Chen Date: Tue, 4 Aug 2026 14:29:22 +0800 Subject: [PATCH 8/8] refactor: reduce the login shell fix to its load-bearing part The macOS login shell probe reads `env` from an interactive zsh. Reads on a process pipe are not interruptible, so once that shell stops producing output neither cancel(true) nor shutdownNow() can stop the reader. On a non-daemon executor thread that alone keeps the JVM alive after the framework is down, which is one of the ways the macOS CI job hung. Making the reader thread a daemon is what actually fixes that. The rest of the original change was defence against failures that were never observed, so drop it and keep the diff to what carries weight: - Drop terminateProcess(). It only had one caller left once the stderr change was reverted, and the plain destroy() it replaced was never seen to fail; the descendants() sweep was speculative. - Drop the local InterruptedException handling. getLoginShellEnvironment() already catches InterruptedException around this call, so this only moved where the same exception was handled. - Drop the extracted readEnvironmentVariables() helper and the two constants. They are cosmetic and only added diff noise. Keep redirectError(DISCARD): nothing drains the shell's stderr, so a chatty shell profile can fill the pipe buffer and wedge the shell before it prints anything. Also wait after destroyForcibly() in the test helper. destroyForcibly() only delivers the signal, so asserting isAlive() right after it could fail while the process was still on its way out. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../lsp/LsStreamConnectionProviderTests.java | 17 +++-- .../core/lsp/LsStreamConnectionProvider.java | 76 +++++-------------- .../pom.xml | 2 - 3 files changed, 33 insertions(+), 62 deletions(-) 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 95ac9fc7c..a805907df 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 @@ -86,15 +86,22 @@ private static void forceTerminate(Process process) { return; } process.destroy(); + if (awaitExit(process)) { + return; + } + process.descendants().forEach(ProcessHandle::destroyForcibly); + process.destroyForcibly(); + // destroyForcibly() only delivers the signal; without waiting, isAlive() can still be true. + awaitExit(process); + } + + private static boolean awaitExit(Process process) { try { - if (process.waitFor(TERMINATION_GRACE_MS, TimeUnit.MILLISECONDS)) { - return; - } + return process.waitFor(TERMINATION_GRACE_MS, TimeUnit.MILLISECONDS); } catch (InterruptedException e) { Thread.currentThread().interrupt(); + return false; } - process.descendants().forEach(ProcessHandle::destroyForcibly); - process.destroyForcibly(); } /** 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 99a38b246..548526349 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 @@ -47,16 +47,6 @@ public class LsStreamConnectionProvider extends ProcessStreamConnectionProvider public static final String EDITOR_NAME = "Eclipse"; public static final String EDITOR_PLUGIN_NAME = "copilot-eclipse"; - /** - * How long a process gets to exit on its own after being asked politely, before it is killed. - */ - private static final long PROCESS_TERMINATION_GRACE_MS = 2000L; - - /** - * How long the login shell gets to report its environment before the probe gives up. - */ - private static final long LOGIN_SHELL_TIMEOUT_SECONDS = 5L; - @Override public Object getInitializationOptions(@Nullable URI rootUri) { NameAndVersion editorInfo = new NameAndVersion(EDITOR_NAME, PlatformUtils.getEclipseVersionString()); @@ -117,27 +107,6 @@ private void startJsLspAgent() throws IOException { CopilotCore.LOGGER.info("JS agent started successfully."); } - /** - * Terminates a process and its descendants, first politely and then forcibly. - * - * @param process the process to terminate, may be {@code null} - */ - private static void terminateProcess(@Nullable Process process) { - if (process == null || !process.isAlive()) { - return; - } - process.destroy(); - try { - if (process.waitFor(PROCESS_TERMINATION_GRACE_MS, TimeUnit.MILLISECONDS)) { - return; - } - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } - process.descendants().forEach(ProcessHandle::destroyForcibly); - process.destroyForcibly(); - } - private List getBinaryLspCommands() throws IOException { Path binary = findAndValidateBinary(); return buildCommands(binary.toString()); @@ -384,13 +353,15 @@ private Map getLoginShellEnvironment() { } catch (IOException e) { CopilotCore.LOGGER.error("IOException while getting login shell environment variables", e); } finally { - terminateProcess(process); + if (process != null && process.isAlive()) { + process.destroy(); + } } return env; } - private Map getEnvironmentVariables(Process process) { + private Map getEnvironmentVariables(Process process) throws InterruptedException { // Reads on a process pipe are not interruptible, so neither cancel(true) nor shutdownNow() can // stop the reader once the shell stops producing output. The thread must therefore be a daemon, // otherwise a wedged shell keeps the whole JVM alive. @@ -399,38 +370,33 @@ private Map getEnvironmentVariables(Process process) { thread.setDaemon(true); return thread; }); - Future> future = executor.submit(() -> readEnvironmentVariables(process)); + Future> future = executor.submit(() -> { + Map result = new HashMap<>(); + try (BufferedReader reader = new BufferedReader( + new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8))) { + String line; + while ((line = reader.readLine()) != null) { + int separator = line.indexOf('='); + if (separator > 0) { + String key = line.substring(0, separator); + String value = line.substring(separator + 1); + result.put(key, value); + } + } + } + return result; + }); try { - return future.get(LOGIN_SHELL_TIMEOUT_SECONDS, TimeUnit.SECONDS); + return future.get(5, TimeUnit.SECONDS); } catch (TimeoutException e) { CopilotCore.LOGGER.error("Timed out waiting for login shell environment variables", e); future.cancel(true); } catch (ExecutionException e) { CopilotCore.LOGGER.error("Error reading login shell environment variables", e); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - CopilotCore.LOGGER.error("Interrupted while reading login shell environment variables", e); } finally { executor.shutdownNow(); } return new HashMap<>(); } - - private Map readEnvironmentVariables(Process process) throws IOException { - Map result = new HashMap<>(); - try (BufferedReader reader = new BufferedReader( - new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8))) { - String line; - while ((line = reader.readLine()) != null) { - int separator = line.indexOf('='); - if (separator > 0) { - String key = line.substring(0, separator); - String value = line.substring(separator + 1); - result.put(key, value); - } - } - } - return result; - } } diff --git a/com.microsoft.copilot.eclipse.swtbot.test/pom.xml b/com.microsoft.copilot.eclipse.swtbot.test/pom.xml index 6c6903574..93543d165 100644 --- a/com.microsoft.copilot.eclipse.swtbot.test/pom.xml +++ b/com.microsoft.copilot.eclipse.swtbot.test/pom.xml @@ -21,8 +21,6 @@ starts a language server that then outlives the fork. Skip the whole execution instead, and let the swtbot-probe profile below switch it back on when a script is supplied. --> true - -