diff --git a/src/main/java/com/github/copilot/sdk/CopilotSession.java b/src/main/java/com/github/copilot/sdk/CopilotSession.java index 61ff49174..0a81cd719 100644 --- a/src/main/java/com/github/copilot/sdk/CopilotSession.java +++ b/src/main/java/com/github/copilot/sdk/CopilotSession.java @@ -98,6 +98,9 @@ public final class CopilotSession implements AutoCloseable { private volatile EventErrorHandler eventErrorHandler; private volatile EventErrorPolicy eventErrorPolicy = EventErrorPolicy.PROPAGATE_AND_LOG_ERRORS; + /** Tracks whether this session instance has been terminated via close(). */ + private volatile boolean isTerminated = false; + /** * Creates a new session with the given ID and RPC client. *

@@ -186,11 +189,14 @@ public String getWorkspacePath() { * @param handler * the error handler, or {@code null} to use only the default logging * behavior + * @throws IllegalStateException + * if this session has been terminated * @see EventErrorHandler * @see #setEventErrorPolicy(EventErrorPolicy) * @since 1.0.8 */ public void setEventErrorHandler(EventErrorHandler handler) { + ensureNotTerminated(); this.eventErrorHandler = handler; } @@ -224,11 +230,14 @@ public void setEventErrorHandler(EventErrorHandler handler) { * @param policy * the error policy (default is * {@link EventErrorPolicy#PROPAGATE_AND_LOG_ERRORS}) + * @throws IllegalStateException + * if this session has been terminated * @see EventErrorPolicy * @see #setEventErrorHandler(EventErrorHandler) * @since 1.0.8 */ public void setEventErrorPolicy(EventErrorPolicy policy) { + ensureNotTerminated(); if (policy == null) { throw new NullPointerException("policy must not be null"); } @@ -244,9 +253,12 @@ public void setEventErrorPolicy(EventErrorPolicy policy) { * @param prompt * the message text to send * @return a future that resolves with the message ID assigned by the server + * @throws IllegalStateException + * if this session has been terminated * @see #send(MessageOptions) */ public CompletableFuture send(String prompt) { + ensureNotTerminated(); return send(new MessageOptions().setPrompt(prompt)); } @@ -260,9 +272,12 @@ public CompletableFuture send(String prompt) { * the message text to send * @return a future that resolves with the final assistant message event, or * {@code null} if no assistant message was received + * @throws IllegalStateException + * if this session has been terminated * @see #sendAndWait(MessageOptions) */ public CompletableFuture sendAndWait(String prompt) { + ensureNotTerminated(); return sendAndWait(new MessageOptions().setPrompt(prompt)); } @@ -275,10 +290,13 @@ public CompletableFuture sendAndWait(String prompt) { * @param options * the message options containing the prompt and attachments * @return a future that resolves with the message ID assigned by the server + * @throws IllegalStateException + * if this session has been terminated * @see #sendAndWait(MessageOptions) * @see #send(String) */ public CompletableFuture send(MessageOptions options) { + ensureNotTerminated(); var request = new SendMessageRequest(); request.setSessionId(sessionId); request.setPrompt(options.getPrompt()); @@ -304,10 +322,13 @@ public CompletableFuture send(MessageOptions options) { * {@code null} if no assistant message was received. The future * completes exceptionally with a TimeoutException if the timeout * expires. + * @throws IllegalStateException + * if this session has been terminated * @see #sendAndWait(MessageOptions) * @see #send(MessageOptions) */ public CompletableFuture sendAndWait(MessageOptions options, long timeoutMs) { + ensureNotTerminated(); var future = new CompletableFuture(); var lastAssistantMessage = new AtomicReference(); @@ -365,9 +386,12 @@ public CompletableFuture sendAndWait(MessageOptions optio * the message options containing the prompt and attachments * @return a future that resolves with the final assistant message event, or * {@code null} if no assistant message was received + * @throws IllegalStateException + * if this session has been terminated * @see #sendAndWait(MessageOptions, long) */ public CompletableFuture sendAndWait(MessageOptions options) { + ensureNotTerminated(); return sendAndWait(options, 60000); } @@ -397,11 +421,14 @@ public CompletableFuture sendAndWait(MessageOptions optio * @param handler * a callback to be invoked when a session event occurs * @return a Closeable that, when closed, unsubscribes the handler + * @throws IllegalStateException + * if this session has been terminated * @see #on(Class, Consumer) * @see AbstractSessionEvent * @see #setEventErrorPolicy(EventErrorPolicy) */ public Closeable on(Consumer handler) { + ensureNotTerminated(); eventHandlers.add(handler); return () -> eventHandlers.remove(handler); } @@ -447,10 +474,13 @@ public Closeable on(Consumer handler) { * @param handler * a callback invoked when events of this type occur * @return a Closeable that unsubscribes the handler when closed + * @throws IllegalStateException + * if this session has been terminated * @see #on(Consumer) * @see AbstractSessionEvent */ public Closeable on(Class eventType, Consumer handler) { + ensureNotTerminated(); Consumer wrapper = event -> { if (eventType.isInstance(event)) { handler.accept(eventType.cast(event)); @@ -708,9 +738,12 @@ CompletableFuture handleHooksInvoke(String hookType, JsonNode input) { * assistant responses, tool invocations, and other session events. * * @return a future that resolves with a list of all session events + * @throws IllegalStateException + * if this session has been terminated * @see AbstractSessionEvent */ public CompletableFuture> getMessages() { + ensureNotTerminated(); return rpc.invoke("session.getMessages", Map.of("sessionId", sessionId), GetMessagesResponse.class) .thenApply(response -> { var events = new ArrayList(); @@ -737,20 +770,42 @@ public CompletableFuture> getMessages() { * continuing to generate a response. * * @return a future that completes when the abort is acknowledged + * @throws IllegalStateException + * if this session has been terminated */ public CompletableFuture abort() { + ensureNotTerminated(); return rpc.invoke("session.abort", Map.of("sessionId", sessionId), Void.class); } + /** + * Verifies that this session has not yet been terminated. + * + * @throws IllegalStateException + * if close() has already been invoked + */ + private void ensureNotTerminated() { + if (isTerminated) { + throw new IllegalStateException("Session is closed"); + } + } + /** * Disposes the session and releases all associated resources. *

* This destroys the session on the server, clears all event handlers, and * releases tool and permission handlers. After calling this method, the session - * cannot be used again. + * cannot be used again. Subsequent calls to this method have no effect. */ @Override public void close() { + synchronized (this) { + if (isTerminated) { + return; // Already terminated - no-op + } + isTerminated = true; + } + try { rpc.invoke("session.destroy", Map.of("sessionId", sessionId), Void.class).get(5, TimeUnit.SECONDS); } catch (Exception e) { diff --git a/src/test/java/com/github/copilot/sdk/ClosedSessionGuardTest.java b/src/test/java/com/github/copilot/sdk/ClosedSessionGuardTest.java new file mode 100644 index 000000000..f4606c646 --- /dev/null +++ b/src/test/java/com/github/copilot/sdk/ClosedSessionGuardTest.java @@ -0,0 +1,339 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + *--------------------------------------------------------------------------------------------*/ + +package com.github.copilot.sdk; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import com.github.copilot.sdk.events.AssistantMessageEvent; +import com.github.copilot.sdk.json.MessageOptions; +import com.github.copilot.sdk.json.SessionConfig; + +/** + * Tests for closed-session guard functionality in CopilotSession. + * + *

+ * Verifies that all public methods that interact with session state throw + * IllegalStateException when invoked after close(), and that close() itself is + * idempotent. + *

+ */ +public class ClosedSessionGuardTest { + + private static E2ETestContext ctx; + + @BeforeAll + static void setup() throws Exception { + ctx = E2ETestContext.create(); + } + + @AfterAll + static void teardown() throws Exception { + if (ctx != null) { + ctx.close(); + } + } + + /** + * Verifies that send(String) throws IllegalStateException after session is + * terminated. + */ + @Test + void testSendStringThrowsAfterTermination() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + session.close(); + + IllegalStateException thrown = assertThrows(IllegalStateException.class, () -> { + session.send("test message"); + }); + assertTrue(thrown.getMessage().contains("closed"), "Exception message should mention session is closed"); + } + } + + /** + * Verifies that send(MessageOptions) throws IllegalStateException after session + * is terminated. + */ + @Test + void testSendOptionsThrowsAfterTermination() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + session.close(); + + IllegalStateException thrown = assertThrows(IllegalStateException.class, () -> { + session.send(new MessageOptions().setPrompt("test message")); + }); + assertTrue(thrown.getMessage().contains("closed"), "Exception message should mention session is closed"); + } + } + + /** + * Verifies that sendAndWait(String) throws IllegalStateException after session + * is terminated. + */ + @Test + void testSendAndWaitStringThrowsAfterTermination() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + session.close(); + + IllegalStateException thrown = assertThrows(IllegalStateException.class, () -> { + session.sendAndWait("test message"); + }); + assertTrue(thrown.getMessage().contains("closed"), "Exception message should mention session is closed"); + } + } + + /** + * Verifies that sendAndWait(MessageOptions) throws IllegalStateException after + * session is terminated. + */ + @Test + void testSendAndWaitOptionsThrowsAfterTermination() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + session.close(); + + IllegalStateException thrown = assertThrows(IllegalStateException.class, () -> { + session.sendAndWait(new MessageOptions().setPrompt("test message")); + }); + assertTrue(thrown.getMessage().contains("closed"), "Exception message should mention session is closed"); + } + } + + /** + * Verifies that sendAndWait(MessageOptions, long) throws IllegalStateException + * after session is terminated. + */ + @Test + void testSendAndWaitWithTimeoutThrowsAfterTermination() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + session.close(); + + IllegalStateException thrown = assertThrows(IllegalStateException.class, () -> { + session.sendAndWait(new MessageOptions().setPrompt("test message"), 5000); + }); + assertTrue(thrown.getMessage().contains("closed"), "Exception message should mention session is closed"); + } + } + + /** + * Verifies that on(Consumer) throws IllegalStateException after session is + * terminated. + */ + @Test + void testOnConsumerThrowsAfterTermination() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + session.close(); + + IllegalStateException thrown = assertThrows(IllegalStateException.class, () -> { + session.on(evt -> { + // Handler should never be registered + }); + }); + assertTrue(thrown.getMessage().contains("closed"), "Exception message should mention session is closed"); + } + } + + /** + * Verifies that on(Class, Consumer) throws IllegalStateException after session + * is terminated. + */ + @Test + void testOnTypedConsumerThrowsAfterTermination() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + session.close(); + + IllegalStateException thrown = assertThrows(IllegalStateException.class, () -> { + session.on(AssistantMessageEvent.class, msg -> { + // Handler should never be registered + }); + }); + assertTrue(thrown.getMessage().contains("closed"), "Exception message should mention session is closed"); + } + } + + /** + * Verifies that getMessages() throws IllegalStateException after session is + * terminated. + */ + @Test + void testGetMessagesThrowsAfterTermination() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + session.close(); + + IllegalStateException thrown = assertThrows(IllegalStateException.class, () -> { + session.getMessages(); + }); + assertTrue(thrown.getMessage().contains("closed"), "Exception message should mention session is closed"); + } + } + + /** + * Verifies that abort() throws IllegalStateException after session is + * terminated. + */ + @Test + void testAbortThrowsAfterTermination() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + session.close(); + + IllegalStateException thrown = assertThrows(IllegalStateException.class, () -> { + session.abort(); + }); + assertTrue(thrown.getMessage().contains("closed"), "Exception message should mention session is closed"); + } + } + + /** + * Verifies that setEventErrorHandler() throws IllegalStateException after + * session is terminated. + */ + @Test + void testSetEventErrorHandlerThrowsAfterTermination() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + session.close(); + + IllegalStateException thrown = assertThrows(IllegalStateException.class, () -> { + session.setEventErrorHandler((event, ex) -> { + // Handler should never be set + }); + }); + assertTrue(thrown.getMessage().contains("closed"), "Exception message should mention session is closed"); + } + } + + /** + * Verifies that setEventErrorPolicy() throws IllegalStateException after + * session is terminated. + */ + @Test + void testSetEventErrorPolicyThrowsAfterTermination() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + session.close(); + + IllegalStateException thrown = assertThrows(IllegalStateException.class, () -> { + session.setEventErrorPolicy(EventErrorPolicy.SUPPRESS_AND_LOG_ERRORS); + }); + assertTrue(thrown.getMessage().contains("closed"), "Exception message should mention session is closed"); + } + } + + /** + * Verifies that getSessionId() still works after session is terminated (it's + * just a field read). + */ + @Test + void testGetSessionIdWorksAfterTermination() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + String sessionIdBeforeClose = session.getSessionId(); + session.close(); + + String sessionIdAfterClose = session.getSessionId(); + assertEquals(sessionIdBeforeClose, sessionIdAfterClose, "Session ID should remain accessible after close"); + } + } + + /** + * Verifies that getWorkspacePath() still works after session is terminated + * (it's just a field read). + */ + @Test + void testGetWorkspacePathWorksAfterTermination() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + String pathBeforeClose = session.getWorkspacePath(); + session.close(); + + String pathAfterClose = session.getWorkspacePath(); + assertEquals(pathBeforeClose, pathAfterClose, "Workspace path should remain accessible after close"); + } + } + + /** + * Verifies that close() is idempotent and can be called multiple times safely. + */ + @Test + void testCloseIsIdempotent() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + + // First close should succeed + assertDoesNotThrow(() -> session.close()); + + // Second close should also succeed (no-op) + assertDoesNotThrow(() -> session.close()); + + // Third close should also succeed (no-op) + assertDoesNotThrow(() -> session.close()); + } + } + + /** + * Verifies that try-with-resources double-close scenario works correctly. + */ + @Test + void testTryWithResourcesDoubleClose() throws Exception { + ctx.configureForTest("session", "should_receive_session_events"); + + try (CopilotClient client = ctx.createClient()) { + CopilotSession session = client.createSession(new SessionConfig().setModel("fake-test-model")).get(); + + try (session) { + // Manual close within try-with-resources + session.close(); + // Automatic close will happen at end of block + } // Second close happens here + + // Should be able to verify it's closed + assertThrows(IllegalStateException.class, () -> { + session.send("test"); + }); + } + } +} diff --git a/src/test/java/com/github/copilot/sdk/CopilotSessionTest.java b/src/test/java/com/github/copilot/sdk/CopilotSessionTest.java index 398e6f2f8..781fb9a9a 100644 --- a/src/test/java/com/github/copilot/sdk/CopilotSessionTest.java +++ b/src/test/java/com/github/copilot/sdk/CopilotSessionTest.java @@ -79,13 +79,17 @@ void testShouldReceiveSessionEvents_createAndDestroy() throws Exception { session.close(); - // Session should no longer be accessible + // Session should no longer be accessible - now throws IllegalStateException try { session.getMessages().get(); fail("Expected exception for closed session"); } catch (Exception e) { - assertTrue(e.getMessage().toLowerCase().contains("not found") - || e.getCause().getMessage().toLowerCase().contains("not found")); + // After our changes, we now get IllegalStateException directly + String message = e.getMessage(); + String causeMessage = e.getCause() != null ? e.getCause().getMessage() : null; + boolean matchesClosed = message != null && message.toLowerCase().contains("closed"); + boolean matchesNotFound = causeMessage != null && causeMessage.toLowerCase().contains("not found"); + assertTrue(matchesClosed || matchesNotFound); } } }