diff --git a/CHANGELOG.md b/CHANGELOG.md index 1acf1c2f..40c19cae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Remove the semantic workspace search integration. [PR#373](https://github.com/microsoft/copilot-for-eclipse/pull/373) ### Fixed +- Keep Eclipse responsive while saving pending chat preferences on exit, with at most two seconds of additional waiting. + Exit continues without a prompt if saving fails or exceeds the budget; unsaved choices may be lost. [#408](https://github.com/microsoft/copilot-for-eclipse/issues/408) - Provide a default MCP auto-approve configuration. [#358](https://github.com/microsoft/copilot-for-eclipse/issues/358) - Prevent Agent mode from hanging when a PowerShell prompt fails. [#375](https://github.com/microsoft/copilot-for-eclipse/issues/375) diff --git a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelServiceTests.java b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelServiceTests.java index 80dec988..7de85c34 100644 --- a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelServiceTests.java +++ b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelServiceTests.java @@ -99,11 +99,13 @@ void setUp() { } @AfterEach - void tearDown() { + void tearDown() throws Exception { if (modelService != null) { modelService.dispose(); } featureFlags.setClientPreviewFeatureEnabled(previewFeaturesEnabled); + // JUnit removes @TempDir next; accepted account-bound writes must finish before that cleanup. + preferenceStorage.beginShutdown().toCompletableFuture().get(5, TimeUnit.SECONDS); preferenceStorage.dispose(); } diff --git a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceShutdownTest.java b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceShutdownTest.java new file mode 100644 index 00000000..6dd649d0 --- /dev/null +++ b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceShutdownTest.java @@ -0,0 +1,279 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +package com.microsoft.copilot.eclipse.ui.chat.services; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Queue; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; + +import org.eclipse.e4.core.services.events.IEventBroker; +import org.eclipse.e4.ui.workbench.UIEvents; +import org.eclipse.jface.databinding.swt.DisplayRealm; +import org.eclipse.swt.widgets.Display; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.Mockito; +import org.osgi.service.event.EventHandler; + +import com.microsoft.copilot.eclipse.core.AuthStatusManager; +import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection; +import com.microsoft.copilot.eclipse.core.lsp.protocol.ChatPersistence; + +class PreferenceShutdownTest { + @TempDir + Path directory; + private final Display display = Display.getDefault(); + private final IEventBroker broker = mock(IEventBroker.class); + private final CopilotLanguageServerConnection connection = mock(CopilotLanguageServerConnection.class); + private final AuthStatusManager auth = mock(AuthStatusManager.class); + private final ExecutorService worker = mock(ExecutorService.class); + private final ScheduledExecutorService timer = mock(ScheduledExecutorService.class); + private final Queue work = new ConcurrentLinkedQueue<>(); + private final AtomicLong clock = new AtomicLong(); + private final AtomicInteger quiesced = new AtomicInteger(); + private PreferenceStorage storage; + private PreferenceShutdown shutdown; + private EventHandler shutdownEvent; + + @BeforeEach + void setUp() throws Exception { + when(auth.isSignedIn()).thenReturn(true); + when(auth.getUserName()).thenReturn("alice"); + ChatPersistence persistence = new ChatPersistence(); + persistence.setPath(directory.toString()); + when(connection.persistence()).thenReturn(CompletableFuture.completedFuture(persistence)); + Mockito.doAnswer(invocation -> { + work.add(invocation.getArgument(0)); + return null; + }).when(worker).execute(any(Runnable.class)); + when(broker.subscribe(eq(UIEvents.UILifeCycle.APP_SHUTDOWN_STARTED), any(EventHandler.class))) + .thenAnswer(invocation -> { + shutdownEvent = invocation.getArgument(1); + return true; + }); + onUi(() -> { + storage = new PreferenceStorage(connection, auth, DisplayRealm.getRealm(display), + worker, timer, clock::get, new PreferenceFileAccess()); + shutdown = new PreferenceShutdown(broker, display, storage, quiesced::incrementAndGet, clock::get); + storage.initialize(); + }); + drainWork(); + onUi(() -> assertEquals(PreferenceStorage.State.READY, storage.getState())); + } + + @AfterEach + void tearDown() throws Exception { + onUi(() -> { + shutdown.dispose(); + storage.dispose(); + }); + } + + @Test + void testWorkbenchShutdown_PendingSave_DispatchesSwtAndContinuesAsSoonAsSaved() throws Exception { + AtomicBoolean actionProcessed = new AtomicBoolean(); + onUi(() -> { + storage.update(preference -> preference.setChatModel("final")); + display.asyncExec(() -> { + actionProcessed.set(true); + CompletableFuture.runAsync(this::drainWork); + }); + shutdownEvent.handleEvent(null); + assertTrue(actionProcessed.get()); + assertTrue(storage.beginShutdown().toCompletableFuture().getNow(false)); + assertEquals(1, quiesced.get()); + assertFalse(storage.update(preference -> preference.setChatModel("late"))); + }); + assertTrue(Files.readString(directory.resolve("alice").resolve("pref.json")).contains("\"chatModel\":\"final\"")); + verify(connection).persistence(); + } + + @Test + void testWorkbenchShutdown_ExactDeadline_ContinuesWithoutCompletingWriter() throws Exception { + AtomicBoolean beforeDeadline = new AtomicBoolean(); + AtomicBoolean afterDeadline = new AtomicBoolean(); + onUi(() -> { + storage.update(preference -> preference.setChatModel("final")); + display.asyncExec(() -> { + clock.set(TimeUnit.MILLISECONDS.toNanos(1999)); + display.asyncExec(() -> { + beforeDeadline.set(true); + clock.set(TimeUnit.SECONDS.toNanos(2)); + display.asyncExec(() -> afterDeadline.set(true)); + }); + }); + shutdownEvent.handleEvent(null); + assertTrue(beforeDeadline.get(), "Must keep dispatching before the deadline"); + assertFalse(afterDeadline.get(), "Must stop waiting at exactly two seconds"); + assertFalse(storage.beginShutdown().toCompletableFuture().isDone()); + shutdownEvent.handleEvent(null); + assertEquals(1, quiesced.get(), "Repeated notifications cannot start another wait"); + storage.dispose(); + }); + drainWork(); + assertTrue(Files.readString(directory.resolve("alice").resolve("pref.json")).contains("\"chatModel\":\"final\"")); + onUi(() -> assertEquals(PreferenceStorage.State.DISPOSED, storage.getState())); + verify(connection).persistence(); + } + + @Test + void testWorkbenchShutdown_ReentrantNotificationAndDisposal_ReleasesOriginalExitOnce() throws Exception { + onUi(() -> { + storage.update(preference -> preference.setChatModel("final")); + display.asyncExec(() -> { + shutdownEvent.handleEvent(null); + shutdown.dispose(); + storage.dispose(); + }); + shutdownEvent.handleEvent(null); + assertEquals(1, quiesced.get()); + assertEquals(PreferenceStorage.State.DISPOSED, storage.getState()); + }); + drainWork(); + verify(broker).unsubscribe(any(EventHandler.class)); + verify(connection).persistence(); + } + + @Test + void testWorkbenchShutdown_Clean_DoesNotDispatchOrWait() throws Exception { + AtomicBoolean queued = new AtomicBoolean(); + onUi(() -> { + display.asyncExec(() -> queued.set(true)); + shutdownEvent.handleEvent(null); + assertFalse(queued.get()); + assertTrue(storage.beginShutdown().toCompletableFuture().getNow(false)); + }); + assertFalse(Files.exists(directory.resolve("alice").resolve("pref.json"))); + verify(connection).persistence(); + } + + @Test + void testWorkbenchShutdown_QuiesceThrows_StillStopsPreferenceWorkAndContinuesExit() throws Exception { + onUi(() -> { + shutdown.dispose(); + shutdown = new PreferenceShutdown(broker, display, storage, () -> { + throw new IllegalStateException("mode cleanup failed"); + }, clock::get); + shutdownEvent.handleEvent(null); + assertFalse(storage.update(preference -> preference.setChatModel("must-not-save"))); + }); + verify(connection).persistence(); + } + + @Test + void testWorkbenchShutdown_DeadlineWake_ContinuesWithoutWriterAndCancelsTimer() throws Exception { + AtomicInteger cancelled = new AtomicInteger(); + AtomicInteger delay = new AtomicInteger(); + AtomicBoolean elapsed = new AtomicBoolean(); + onUi(() -> { + shutdown.dispose(); + shutdown = new PreferenceShutdown(broker, display, storage, quiesced::incrementAndGet, clock::get, + (milliseconds, callback) -> { + if (milliseconds >= 0) { + delay.set(milliseconds); + display.asyncExec(() -> { + elapsed.set(true); + callback.run(); + }); + } else { + cancelled.incrementAndGet(); + } + }); + storage.update(preference -> preference.setChatModel("unsaved")); + shutdownEvent.handleEvent(null); + assertTrue(elapsed.get()); + assertEquals(2000, delay.get()); + assertEquals(1, cancelled.get()); + assertFalse(storage.beginShutdown().toCompletableFuture().isDone()); + }); + } + + @Test + void testWorkbenchShutdown_FileFailure_ContinuesEarlyAndKeepsOriginalTarget() throws Exception { + Path target = directory.resolve("alice").resolve("pref.json"); + Files.createDirectories(target); + Files.writeString(target.resolve("keep"), "original"); + onUi(() -> { + storage.update(preference -> preference.setChatModel("unsaved")); + display.asyncExec(() -> CompletableFuture.runAsync(this::drainWork)); + shutdownEvent.handleEvent(null); + var result = storage.beginShutdown().toCompletableFuture(); + assertTrue(result.isDone()); + assertFalse(result.getNow(true)); + }); + assertEquals("original", Files.readString(target.resolve("keep"))); + verify(connection).persistence(); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void testWorkbenchShutdown_NoResolvedPath_DoesNotWaitOrStartRpc(boolean requested) throws Exception { + CompletableFuture pending = new CompletableFuture<>(); + when(connection.persistence()).thenReturn(pending); + Mockito.clearInvocations(connection); + onUi(() -> { + shutdown.dispose(); + storage.dispose(); + storage = new PreferenceStorage(connection, auth, DisplayRealm.getRealm(display), + worker, timer, clock::get, new PreferenceFileAccess()); + shutdown = new PreferenceShutdown(broker, display, storage, quiesced::incrementAndGet, clock::get); + if (requested) { + storage.initialize(); + } + }); + drainWork(); + AtomicBoolean dispatched = new AtomicBoolean(); + onUi(() -> { + display.asyncExec(() -> dispatched.set(true)); + shutdownEvent.handleEvent(null); + assertFalse(dispatched.get()); + assertTrue(storage.beginShutdown().toCompletableFuture().getNow(false)); + }); + verify(connection, times(requested ? 1 : 0)).persistence(); + } + + private void drainWork() { + Runnable next; + while ((next = work.poll()) != null) { + next.run(); + } + } + + private void onUi(Runnable action) throws Exception { + AtomicReference failure = new AtomicReference<>(); + display.syncExec(() -> { + try { + action.run(); + } catch (Throwable throwable) { + failure.set(throwable); + } + }); + if (failure.get() != null) { + throw new AssertionError("UI callback failed", failure.get()); + } + } +} diff --git a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorageTest.java b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorageTest.java index d350b019..38501e96 100644 --- a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorageTest.java +++ b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorageTest.java @@ -34,6 +34,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; import org.eclipse.core.databinding.observable.Realm; import org.eclipse.e4.core.services.events.IEventBroker; @@ -41,6 +42,7 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.NullAndEmptySource; import org.junit.jupiter.params.provider.ValueSource; @@ -92,6 +94,199 @@ void tearDown() { realm.drain(); } + @Test + void testShutdown_LatestDirtySnapshot_SavesOnceAndRejectsOrdinaryWork() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(); + storage.update(preference -> preference.setChatModel("first")); + storage.update(preference -> preference.setChatModel("final")); + + var completion = storage.beginShutdown().toCompletableFuture(); + assertFalse(completion.isDone()); + storage.beginShutdown(); + assertFalse(storage.update(preference -> preference.setChatModel("too-late"))); + storage.retry(); + storage.persist(); + recovered(1); + drainWork(); + realm.drain(); + + assertTrue(completion.getNow(false)); + ArgumentCaptor saved = ArgumentCaptor.forClass(String.class); + verify(files).write(eq(Path.of(persistence().getPath(), "alice", "pref.json")), saved.capture()); + assertTrue(saved.getValue().contains("\"chatModel\":\"final\"")); + verify(connection).persistence(); + } + + @Test + void testShutdown_PendingRpc_DoesNotWaitOrAllowLateRecovery() throws Exception { + response = new UncancellableFuture(); + storage.initialize(); + drainWork(); + CopilotAuthStatusListener listener = authListener(); + assertTrue(storage.beginShutdown().toCompletableFuture().getNow(false)); + response.complete(persistence()); + when(auth.getUserName()).thenReturn("bob"); + listener.onDidCopilotStatusChange(new CopilotStatusResult()); + recovered(1); + storage.initialize(); + storage.retry(); + drainWork(); + realm.drain(); + + assertNull(storage.getReadyPreferences()); + verify(connection).persistence(); + verifyNoInteractions(files); + } + + @Test + void testShutdown_UnavailableOrClean_DoesNotStartWork() throws Exception { + assertTrue(storage.beginShutdown().toCompletableFuture().getNow(false)); + storage.initialize(); + storage.retry(); + drainWork(); + verifyNoInteractions(connection, files); + } + + @Test + void testShutdown_FailedFinalSave_CompletesWithoutRetryingOrChangingChoices() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(); + Mockito.doThrow(new IOException("write denied")).when(files).write(any(), any()); + storage.update(preference -> preference.setChatModel("unsaved")); + var completion = storage.beginShutdown().toCompletableFuture(); + drainWork(); + + assertTrue(completion.isDone()); + assertFalse(completion.getNow(true)); + storage.beginShutdown(); + storage.persist(); + recovered(1); + drainWork(); + assertEquals("unsaved", storage.getReadyPreferences().getChatModel()); + assertTrue(storage.isDirty()); + verify(files).write(any(), any()); + verify(connection).persistence(); + } + + @Test + void testShutdown_PreviousSaveFailed_RetriesLatestSnapshotOnce() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(); + Mockito.doThrow(new IOException("temporary failure")).when(files).write(any(), any()); + storage.update(preference -> preference.setChatModel("final")); + drainWork(); + Mockito.doNothing().when(files).write(any(), any()); + var completion = storage.beginShutdown().toCompletableFuture(); + drainWork(); + + assertTrue(completion.getNow(false)); + assertFalse(storage.isDirty()); + verify(files, times(2)).write(any(), any()); + verify(connection).persistence(); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void testShutdown_OlderWriteInFlight_FinalCompleteFileWins(boolean oldFails, @TempDir Path directory) throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(directory); + AtomicInteger writes = new AtomicInteger(); + AtomicReference> completion = new AtomicReference<>(); + PreferenceFileAccess disk = new PreferenceFileAccess(); + Mockito.doAnswer(invocation -> { + if (writes.incrementAndGet() == 1) { + storage.update(preference -> preference.setChatModel("final")); + completion.set(storage.beginShutdown().toCompletableFuture()); + assertFalse(completion.get().isDone()); + if (oldFails) { + throw new IOException("superseded revision failed"); + } + } else { + assertFalse(completion.get().isDone(), "The old revision must not release shutdown"); + } + disk.write(invocation.getArgument(0), invocation.getArgument(1)); + return null; + }).when(files).write(any(), any()); + storage.update(preference -> preference.setChatModel("old")); + drainWork(); + + assertTrue(completion.get().getNow(false)); + assertTrue(Files.readString(directory.resolve("alice").resolve("pref.json")).contains("\"chatModel\":\"final\"")); + assertEquals(2, writes.get()); + verify(connection).persistence(); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void testShutdown_OldAccountWriteInFlight_KeepsBothDestinationsFixed(boolean oldFails, + @TempDir Path directory) throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(directory); + AtomicInteger writes = new AtomicInteger(); + AtomicReference> completion = new AtomicReference<>(); + PreferenceFileAccess disk = new PreferenceFileAccess(); + Mockito.doAnswer(invocation -> { + if (writes.incrementAndGet() == 1) { + when(auth.getUserName()).thenReturn("bob"); + response = new CompletableFuture<>(); + load(directory); + storage.update(preference -> preference.setChatModel("bob-final")); + completion.set(storage.beginShutdown().toCompletableFuture()); + if (oldFails) { + throw new IOException("old account save failed"); + } + } else { + assertFalse(completion.get().isDone()); + } + disk.write(invocation.getArgument(0), invocation.getArgument(1)); + return null; + }).when(files).write(any(), any()); + storage.update(preference -> preference.setChatModel("alice-final")); + drainWork(); + + assertTrue(completion.get().isDone()); + assertEquals(!oldFails, completion.get().getNow(false)); + if (!oldFails) { + assertTrue(Files.readString(directory.resolve("alice").resolve("pref.json")).contains("alice-final")); + } + assertTrue(Files.readString(directory.resolve("bob").resolve("pref.json")).contains("bob-final")); + assertEquals(2, writes.get()); + verify(connection, times(2)).persistence(); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void testShutdown_OldAccountSaveAndUnmodifiedCurrentAccount_WaitsOnlyForAcceptedSave(boolean currentLoaded, + @TempDir Path directory) throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(directory); + AtomicReference> completion = new AtomicReference<>(); + PreferenceFileAccess disk = new PreferenceFileAccess(); + Mockito.doAnswer(invocation -> { + when(auth.getUserName()).thenReturn("bob"); + response = new CompletableFuture<>(); + if (currentLoaded) { + load(directory); + } else { + storage.initialize(); + drainWork(); + } + assertFalse(storage.isDirty()); + completion.set(storage.beginShutdown().toCompletableFuture()); + assertFalse(completion.get().isDone(), "An accepted old-account save is still pending"); + disk.write(invocation.getArgument(0), invocation.getArgument(1)); + return null; + }).when(files).write(any(), any()); + storage.update(preference -> preference.setChatModel("alice-final")); + drainWork(); + + assertTrue(completion.get().getNow(false)); + assertTrue(Files.readString(directory.resolve("alice").resolve("pref.json")).contains("alice-final")); + assertFalse(Files.exists(directory.resolve("bob").resolve("pref.json"))); + verify(connection, times(2)).persistence(); + } + @Test void testRecovery_FailureAndDuplicateSignals_RetriesOncePerConnectionAndAllowsManualRetry() throws Exception { storage.initialize(); @@ -1001,9 +1196,15 @@ private void expire() { } private void load() { + load(Path.of(persistence().getPath())); + } + + private void load(Path directory) { storage.initialize(); drainWork(); - response.complete(persistence()); + ChatPersistence persistence = new ChatPersistence(); + persistence.setPath(directory.toString()); + response.complete(persistence); drainWork(); realm.drain(); } 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 acddae41..e4f7b268 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 @@ -113,6 +113,31 @@ void tearDown() { }); } + @Test + void testShutdown_PendingModeDiscoveryAndRecovery_CannotPublishOrRestart() throws Exception { + writePreferences("{\"chatModeName\":\"Plan\"}"); + CompletableFuture modes = new CompletableFuture<>(); + when(connection.listConversationModes(any())).thenReturn(modes); + startAuthenticated(CompletableFuture.completedFuture(persistence())); + verify(connection, timeout(5000)).listConversationModes(any()); + awaitUi(() -> storage.getState() == PreferenceStorage.State.READY); + runOnUi(() -> { + storage.beginShutdown(); + service.quiesce(); + service.quiesce(); + modes.complete(new ConversationMode[] {builtInMode("after-shutdown", "Plan")}); + service.retryModeDiscovery(); + NotificationMessage initialized = new NotificationMessage(); + initialized.setMethod("initialized"); + new LsStreamConnectionProvider().handleMessage(initialized, mock(LanguageServer.class), null); + assertNull(BuiltInChatModeManager.INSTANCE.getBuiltInModeById("after-shutdown")); + assertEquals(UserPreferenceService.ModeDiscoveryState.UNAVAILABLE, service.getModeDiscoveryState()); + }); + runOnUi(() -> assertNull(BuiltInChatModeManager.INSTANCE.getBuiltInModeById("after-shutdown"))); + verify(connection).listConversationModes(any()); + verify(connection).persistence(); + } + @Test void testAuthentication_QuickSignOutAndBackIn_RejectsOldModeDiscovery() throws Exception { CompletableFuture old = new CompletableFuture<>(); diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatServiceManager.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatServiceManager.java index 6f356be9..d74e63b0 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatServiceManager.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatServiceManager.java @@ -3,6 +3,10 @@ package com.microsoft.copilot.eclipse.ui.chat.services; +import org.eclipse.e4.core.services.events.IEventBroker; +import org.eclipse.swt.widgets.Display; +import org.eclipse.ui.PlatformUI; + import com.microsoft.copilot.eclipse.core.AuthStatusManager; import com.microsoft.copilot.eclipse.core.CopilotCore; import com.microsoft.copilot.eclipse.core.chat.service.CustomizationFileService; @@ -25,6 +29,7 @@ public class ChatServiceManager implements IChatServiceManager { private ByokService byokService; private UserPreferenceService userPreferenceService; private PreferenceStorage preferenceStorage; + private PreferenceShutdown preferenceShutdown; private AvatarService avatarService; private AgentToolService agentToolService; private FileToolService fileToolService; @@ -61,6 +66,8 @@ public ChatServiceManager() { persistenceManager = new ConversationPersistenceManager(this.authStatusManager); chatFontService = new ChatFontService(); contextWindowService = new ContextWindowService(modelService); + preferenceShutdown = new PreferenceShutdown(PlatformUI.getWorkbench().getService(IEventBroker.class), + Display.getDefault(), preferenceStorage, userPreferenceService::quiesce, System::nanoTime); } /** @@ -215,6 +222,7 @@ public ContextWindowService getContextWindowService() { * Dispose of the chat services. */ public void dispose() { + this.preferenceShutdown.dispose(); this.avatarService.dispose(); this.chatCompletionService.dispose(); this.modelService.dispose(); diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceShutdown.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceShutdown.java new file mode 100644 index 00000000..f1d4cc0a --- /dev/null +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceShutdown.java @@ -0,0 +1,124 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +package com.microsoft.copilot.eclipse.ui.chat.services; + +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.BiConsumer; +import java.util.function.LongSupplier; + +import org.eclipse.e4.core.services.events.IEventBroker; +import org.eclipse.e4.ui.workbench.UIEvents; +import org.eclipse.swt.SWT; +import org.eclipse.swt.SWTException; +import org.eclipse.swt.widgets.Display; +import org.osgi.service.event.EventHandler; + +import com.microsoft.copilot.eclipse.core.CopilotCore; + +/** + * Participates in the committed workbench exit, after all shutdown veto opportunities have passed. + * Eclipse sends this event synchronously before disposing services. A bounded nested SWT loop keeps + * dispatching events while the shared writer finishes; it never vetoes or reissues a close request. + * The budget bounds our waiting, not arbitrary OS I/O or other participants' event handlers. + */ +final class PreferenceShutdown { + private static final long WAIT_NANOS = TimeUnit.SECONDS.toNanos(2); + + private final IEventBroker broker; + private final Display display; + private final PreferenceStorage storage; + private final Runnable quiesce; + private final LongSupplier clock; + private final BiConsumer schedule; + private final AtomicBoolean started = new AtomicBoolean(); + private final EventHandler listener = event -> shutdown(); + private volatile boolean disposed; + + PreferenceShutdown(IEventBroker broker, Display display, PreferenceStorage storage, Runnable quiesce, + LongSupplier clock) { + this(broker, display, storage, quiesce, clock, display::timerExec); + } + + PreferenceShutdown(IEventBroker broker, Display display, PreferenceStorage storage, Runnable quiesce, + LongSupplier clock, BiConsumer schedule) { + this.broker = broker; + this.display = display; + this.storage = storage; + this.quiesce = quiesce; + this.clock = clock; + this.schedule = schedule; + if (broker == null || !broker.subscribe(UIEvents.UILifeCycle.APP_SHUTDOWN_STARTED, listener)) { + CopilotCore.LOGGER.error(new IllegalStateException("Cannot subscribe to workbench preference shutdown")); + } + } + + private void shutdown() { + if (disposed || !started.compareAndSet(false, true)) { + return; + } + long start = clock.getAsLong(); + AtomicBoolean expired = new AtomicBoolean(); + Runnable deadline = () -> expired.set(true); + try { + CompletableFuture saved = storage.beginShutdown().toCompletableFuture(); + try { + quiesce.run(); + } catch (RuntimeException exception) { + CopilotCore.LOGGER.error("Failed to stop chat mode discovery during shutdown", exception); + } + if (!saved.isDone() && !display.isDisposed() && !disposed) { + long remaining = WAIT_NANOS - (clock.getAsLong() - start); + if (remaining > 0) { + // Wake an idle nested event loop at the deadline, even when the writer never completes. + schedule.accept((int) Math.max(1, TimeUnit.NANOSECONDS.toMillis(remaining)), deadline); + saved.whenComplete((result, failure) -> wake()); + while (!disposed && !display.isDisposed() && !saved.isDone() && !expired.get() + && clock.getAsLong() - start < WAIT_NANOS) { + // A writer wakeup can be consumed by dispatch; recheck before going back to sleep. + if (!display.readAndDispatch() && !saved.isDone() && !disposed && !expired.get() + && clock.getAsLong() - start < WAIT_NANOS) { + display.sleep(); + } + } + } + } + if (!saved.getNow(false)) { + CopilotCore.LOGGER.error(new IllegalStateException( + "Exiting with unsaved chat preferences after failure or the two-second save budget")); + } + } catch (RuntimeException exception) { + CopilotCore.LOGGER.error("Failed to finish saving chat preferences during shutdown; continuing exit", exception); + } finally { + if (!display.isDisposed()) { + schedule.accept(-1, deadline); + } + } + } + + private void wake() { + if (disposed || display.isDisposed()) { + return; + } + try { + display.wake(); + } catch (SWTException exception) { + if (exception.code != SWT.ERROR_DEVICE_DISPOSED) { + throw exception; + } + } + } + + void dispose() { + if (disposed) { + return; + } + wake(); + disposed = true; + if (broker != null) { + broker.unsubscribe(listener); + } + } +} diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorage.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorage.java index d29a9cf7..2dcddf81 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorage.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorage.java @@ -10,8 +10,11 @@ import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Deque; +import java.util.HashSet; import java.util.Objects; +import java.util.Set; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionStage; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.FutureTask; @@ -94,6 +97,12 @@ public enum SaveState { private final Deque pendingSaves = new ArrayDeque<>(); private Save activeSave; private boolean writing; + private volatile boolean shuttingDown; + private boolean listenersDetached; + private CompletableFuture shutdownSave; + private long shutdownGeneration; + private long shutdownRevision; + private final Set shutdownFailedPaths = new HashSet<>(); /** * Creates storage without starting RPC or file work. @@ -126,7 +135,7 @@ public PreferenceStorage(CopilotLanguageServerConnection connection, AuthStatusM this.connectionListener = event -> { if (event.getProperty(IEventBroker.DATA) instanceof Long incarnation) { synchronized (lock) { - if (state == State.DISPOSED || incarnation <= connectionIncarnation) { + if (shuttingDown || state == State.DISPOSED || incarnation <= connectionIncarnation) { return; } connectionIncarnation = incarnation; @@ -220,7 +229,7 @@ public boolean update(Consumer edit) { Objects.requireNonNull(edit); refreshAccount(); synchronized (lock) { - if (state != State.READY || !Objects.equals(account, currentAccount())) { + if (shuttingDown || state != State.READY || !Objects.equals(account, currentAccount())) { CopilotCore.LOGGER.error(new IllegalStateException("Cannot update chat preferences before they are ready")); return false; } @@ -243,7 +252,8 @@ public boolean update(Consumer edit) { public void persist() { refreshAccount(); synchronized (lock) { - if (state != State.READY || revision == savedRevision || !Objects.equals(account, currentAccount())) { + if (shuttingDown || state != State.READY || revision == savedRevision + || !Objects.equals(account, currentAccount())) { return; } enqueueSave(); @@ -251,6 +261,45 @@ public void persist() { dispatchWriter(); } + /** + * Stops ordinary work and recovery, and submits the latest dirty snapshot to the existing writer. + * Previously accepted account-bound writes participate even if the current account has no edits. + * Never resolves a path or waits for loading or file I/O. Repeated calls share the same outcome. + * The workbench owns the bounded wait; an accepted OS write may finish after that wait or disposal. + * + * @return a read-only completion indicating whether all pending saves succeeded (or nothing needed saving) + */ + public CompletionStage beginShutdown() { + Attempt previous; + boolean nothingToSave; + synchronized (lock) { + if (shutdownSave != null) { + return shutdownSave.minimalCompletionStage(); + } + shuttingDown = true; + shutdownSave = new CompletableFuture<>(); + previous = attempt; + attempt = null; + if (state == State.READY && preferencePath != null && revision != savedRevision) { + enqueueSave(); + } + Save last = pendingSaves.isEmpty() ? activeSave : pendingSaves.peekLast(); + nothingToSave = last == null; + if (last != null) { + shutdownGeneration = last.generation; + shutdownRevision = last.revision; + } + } + detachListeners(); + cancel(previous); + if (nothingToSave) { + shutdownSave.complete(true); + } else { + dispatchWriter(); + } + return shutdownSave.minimalCompletionStage(); + } + private void enqueueSave() { if (activeSave != null && activeSave.generation == generation && activeSave.revision == revision) { return; @@ -271,6 +320,7 @@ private void dispatchWriter() { try { worker.execute(this::writePending); } catch (RuntimeException exception) { + CompletableFuture completion; synchronized (lock) { writing = false; pendingSaves.clear(); @@ -278,14 +328,19 @@ private void dispatchWriter() { saveState = SaveState.FAILED; publishSaveStatus(generation); } + completion = shutdownSave; } CopilotCore.LOGGER.error("Failed to schedule chat preference saving", exception); + if (completion != null) { + completion.complete(false); + } } } private void writePending() { while (true) { Save save; + CompletableFuture completion; synchronized (lock) { save = pendingSaves.pollFirst(); activeSave = save; @@ -295,6 +350,7 @@ private void writePending() { } } boolean successful = false; + boolean shutdownSuccessful; try { files.write(save.path, GSON.toJson(save.preferences)); successful = true; @@ -313,15 +369,30 @@ private void writePending() { ? SaveState.SAVING : SaveState.FAILED; publishSaveStatus(generation); } + completion = save.generation == shutdownGeneration && save.revision == shutdownRevision ? shutdownSave : null; + if (shuttingDown) { + if (successful) { + shutdownFailedPaths.remove(save.path); + } else { + shutdownFailedPaths.add(save.path); + } + } + shutdownSuccessful = shutdownFailedPaths.isEmpty(); + } + if (completion != null) { + completion.complete(shutdownSuccessful); } } } private void publishSaveStatus(long version) { + if (shuttingDown) { + return; + } saveStatus.getRealm().asyncExec(() -> { refreshAccount(); synchronized (lock) { - if (state != State.DISPOSED && generation == version && !saveStatus.isDisposed()) { + if (!shuttingDown && state != State.DISPOSED && generation == version && !saveStatus.isDisposed()) { saveStatus.setValue(saveState); } } @@ -344,21 +415,23 @@ private static UserPreference copy(UserPreference source) { */ public void dispose() { Attempt previous; + CompletableFuture completion; synchronized (lock) { if (state == State.DISPOSED) { return; } previous = clear(State.DISPOSED); + completion = shutdownSave; } - auth.removeCopilotAuthStatusListener(authListener); - if (eventBroker != null) { - eventBroker.unsubscribe(connectionListener); - } + detachListeners(); cancel(previous); // Accepted snapshots may finish on the shared writer; disposal never waits for OS I/O. dispatchWriter(); worker.shutdown(); timer.shutdownNow(); + if (completion != null) { + completion.complete(false); + } readiness.getRealm().asyncExec(() -> { if (!readiness.isDisposed()) { readiness.setValue(State.DISPOSED); @@ -370,6 +443,19 @@ public void dispose() { }); } + private void detachListeners() { + synchronized (lock) { + if (listenersDetached) { + return; + } + listenersDetached = true; + } + auth.removeCopilotAuthStatusListener(authListener); + if (eventBroker != null) { + eventBroker.unsubscribe(connectionListener); + } + } + private static Thread daemonThread(Runnable runnable, String name) { Thread thread = new Thread(runnable, name); thread.setDaemon(true); @@ -386,7 +472,7 @@ private void refreshAccount() { long version; synchronized (lock) { String current = currentAccount(); - if (state == State.DISPOSED || Objects.equals(account, current)) { + if (shuttingDown || state == State.DISPOSED || Objects.equals(account, current)) { return; } account = current; @@ -428,7 +514,7 @@ private void start(boolean retry) { refreshAccount(); Attempt next; synchronized (lock) { - if (account == null || state == State.DISPOSED || state == State.LOADING || state == State.READY + if (shuttingDown || account == null || state == State.DISPOSED || state == State.LOADING || state == State.READY || (!retry && state == State.FAILED)) { return; } @@ -597,7 +683,7 @@ private void publishPreferences(Attempt pending, Path path, UserPreference resto } private boolean matches(Attempt pending) { - return state == State.LOADING && attempt == pending && generation == pending.generation + return !shuttingDown && state == State.LOADING && attempt == pending && generation == pending.generation && Objects.equals(account, pending.account) && Objects.equals(account, currentAccount()); } @@ -636,7 +722,7 @@ private void publishState(long version, State next) { readiness.getRealm().asyncExec(() -> { refreshAccount(); synchronized (lock) { - if (generation != version || state != next || readiness.isDisposed()) { + if (shuttingDown || generation != version || state != next || readiness.isDisposed()) { return; } readiness.setValue(next); 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 1c58197c..ac3ee054 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 @@ -60,6 +60,7 @@ public enum ModeDiscoveryState { private final PreferenceStorage preferenceStorage; private ISideEffect readinessSideEffect; private volatile boolean disposed; + private volatile boolean quiesced; private final Object authenticationLock = new Object(); private final AtomicLong authenticationGeneration = new AtomicLong(); private String notifiedAccount; @@ -147,7 +148,7 @@ private void authenticationChanged() { long generation; synchronized (authenticationLock) { String current = authStatusManager.isSignedIn() ? authStatusManager.getUserName() : null; - if (disposed || Objects.equals(notifiedAccount, current)) { + if (disposed || quiesced || Objects.equals(notifiedAccount, current)) { return; } notifiedAccount = current; @@ -180,7 +181,12 @@ private void reloadBuiltInModes(boolean retry) { long authentication = authenticationGeneration.get(); modeDiscoveryState.setValue(ModeDiscoveryState.LOADING); CompletableFuture> discovery = CompletableFuture - .supplyAsync(() -> new BuiltInChatModeService().loadBuiltInModes(lsConnection)) + .supplyAsync(() -> { + if (disposed || quiesced || authentication != authenticationGeneration.get()) { + return CompletableFuture.>completedFuture(List.of()); + } + return new BuiltInChatModeService().loadBuiltInModes(lsConnection); + }) .thenCompose(result -> result); modeDiscovery = discovery; discovery.thenAccept(modes -> ensureRealm(() -> { @@ -781,6 +787,28 @@ public void dispose() { } } + /** + * Stops mode discovery and its recovery callbacks before the committed workbench exit. + * Observable disposal remains with the normal service lifecycle. + */ + void quiesce() { + if (disposed || quiesced) { + return; + } + quiesced = true; + authenticationGeneration.incrementAndGet(); + authStatusManager.removeCopilotAuthStatusListener(authListener); + if (eventBroker != null) { + eventBroker.unsubscribe(connectionInitializedEventHandler); + eventBroker.unsubscribe(featureFlagNotifiedEventHandler); + } + super.ensureRealm(() -> { + if (!disposed) { + cancelModeDiscovery(); + } + }); + } + private boolean isModeAvailable(String modeNameOrId) { String[] available = getAvailableChatModes(); // Check built-in modes (matched by display name) @@ -796,27 +824,27 @@ private boolean isModeAvailable(String modeNameOrId) { } private UserPreference getUserPreference() { - return disposed ? null : preferenceStorage.getReadyPreferences(); + return disposed || quiesced ? null : preferenceStorage.getReadyPreferences(); } /** * Queues the latest unsaved preferences without waiting for disk or resolving another persistence path. */ public void persistUserPreference() { - if (!disposed) { + if (!disposed && !quiesced) { preferenceStorage.persist(); } } @Override protected void ensureRealm(Runnable runnable) { - if (disposed) { + if (disposed || quiesced) { return; } String account = authStatusManager.getUserName(); long generation = authenticationGeneration.get(); super.ensureRealm(() -> { - if (!disposed && generation == authenticationGeneration.get() + if (!disposed && !quiesced && generation == authenticationGeneration.get() && Objects.equals(account, authStatusManager.getUserName())) { runnable.run(); }