From f739f7c2818c114187f475cee9edbffc2036c8db Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 3 Sep 2026 16:50:29 +0000 Subject: [PATCH] Use session detach for SDK cleanup Add real-runtime cold resume coverage across SDKs so disconnect preserves persisted sessions across client/runtime restart. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- dotnet/src/Session.cs | 22 ++++-- dotnet/test/E2E/ClientLifecycleE2ETests.cs | 2 +- dotnet/test/E2E/SessionE2ETests.cs | 44 ++++++++++++ dotnet/test/Harness/E2ETestBase.cs | 2 +- dotnet/test/Harness/E2ETestContext.cs | 2 +- .../test/Unit/ClientSessionLifetimeTests.cs | 6 +- dotnet/test/Unit/GitHubTelemetryTests.cs | 2 +- go/client_test.go | 4 +- go/github_token_provider_test.go | 10 +-- go/internal/e2e/client_options_e2e_test.go | 4 ++ go/internal/e2e/session_e2e_test.go | 58 +++++++++++++++ go/session.go | 17 ++++- go/types.go | 9 ++- .../com/github/copilot/CopilotSession.java | 22 +++++- .../github/copilot/ClientOptionsE2ETest.java | 2 + .../com/github/copilot/CopilotClientTest.java | 4 +- .../github/copilot/CopilotSessionTest.java | 45 ++++++++++++ .../github/copilot/GitHubTelemetryTest.java | 3 +- .../com/github/copilot/McpAndAgentsTest.java | 2 +- .../McpAuthInterestRegistrationTest.java | 5 +- .../copilot/SessionEventHandlingTest.java | 4 +- .../github/copilot/TimeoutEdgeCaseTest.java | 49 +++++++++---- .../copilot/ZeroTimeoutContractTest.java | 6 +- nodejs/src/session.ts | 15 +++- nodejs/test/e2e/client.e2e.test.ts | 2 +- nodejs/test/e2e/client_options.e2e.test.ts | 5 ++ nodejs/test/e2e/session.e2e.test.ts | 53 ++++++++++++++ nodejs/test/github-token-provider.test.ts | 2 +- python/copilot/session.py | 21 +++--- python/e2e/test_client_options_e2e.py | 4 ++ python/e2e/test_session_e2e.py | 52 ++++++++++++++ python/test_github_token_provider.py | 4 +- rust/src/errors.rs | 6 +- rust/src/lib.rs | 48 ++++++++----- rust/src/session.rs | 13 +--- rust/tests/e2e/client_options.rs | 4 ++ rust/tests/e2e/session.rs | 70 +++++++++++++++++++ rust/tests/session_test.rs | 29 ++++---- ..._cold_resume_with_explicit_session_id.yaml | 16 +++++ 39 files changed, 564 insertions(+), 104 deletions(-) create mode 100644 test/snapshots/session/should_recover_marker_after_cold_resume_with_explicit_session_id.yaml diff --git a/dotnet/src/Session.cs b/dotnet/src/Session.cs index 24450a7c7d..1a468802cb 100644 --- a/dotnet/src/Session.cs +++ b/dotnet/src/Session.cs @@ -1962,8 +1962,12 @@ public async ValueTask DisposeAsync() try { - await InvokeRpcAsync( - "session.destroy", [new SessionDestroyRequest() { SessionId = SessionId }], CancellationToken.None); + var response = await InvokeRpcAsync( + "session.detach", [new SessionDetachRequest() { SessionId = SessionId }], CancellationToken.None); + if (!response.Success) + { + LogSessionDetachFailed(SessionId, response.Error ?? "unknown error"); + } } catch (ObjectDisposedException) { @@ -2000,6 +2004,9 @@ await InvokeRpcAsync( [LoggerMessage(Level = LogLevel.Debug, Message = "Failed to fetch tool metadata for {toolName}")] private partial void LogToolMetadataFetchFailed(Exception exception, string toolName); + [LoggerMessage(Level = LogLevel.Warning, Message = "Failed to detach session {sessionId}: {error}")] + private partial void LogSessionDetachFailed(string sessionId, string error); + [LoggerMessage(Level = LogLevel.Error, Message = "Permission handler or response delivery failed. SessionId={SessionId}, RequestId={RequestId}")] private partial void LogPermissionHandlerOrDeliveryFailed(Exception exception, string sessionId, string requestId); @@ -2037,11 +2044,17 @@ internal record SessionAbortRequest public string SessionId { get; init; } = string.Empty; } - internal record SessionDestroyRequest + internal record SessionDetachRequest { public string SessionId { get; init; } = string.Empty; } + internal record SessionDetachResponse + { + public bool Success { get; init; } + public string? Error { get; init; } + } + internal void ThrowIfDisposed() { ObjectDisposedException.ThrowIf(Volatile.Read(ref _isDisposed) != 0, this); @@ -2074,7 +2087,8 @@ internal void ThrowIfDisposed() [JsonSerializable(typeof(SendMessageRequest))] [JsonSerializable(typeof(SendMessageResponse))] [JsonSerializable(typeof(SessionAbortRequest))] - [JsonSerializable(typeof(SessionDestroyRequest))] + [JsonSerializable(typeof(SessionDetachRequest))] + [JsonSerializable(typeof(SessionDetachResponse))] [JsonSerializable(typeof(SessionEndHookInput))] [JsonSerializable(typeof(SessionEndHookOutput))] [JsonSerializable(typeof(SessionStartHookInput))] diff --git a/dotnet/test/E2E/ClientLifecycleE2ETests.cs b/dotnet/test/E2E/ClientLifecycleE2ETests.cs index 4b09c695d4..82b2d2badf 100644 --- a/dotnet/test/E2E/ClientLifecycleE2ETests.cs +++ b/dotnet/test/E2E/ClientLifecycleE2ETests.cs @@ -121,7 +121,7 @@ public async Task Should_Receive_Session_Deleted_Lifecycle_Event_When_Deleted() } }); - // Do NOT DisposeAsync the session before deleting: dispose sends session.destroy + // Do NOT DisposeAsync the session before deleting: dispose sends session.detach // which closes in-memory state but does not remove the disk file; calling // delete afterwards still succeeds, but skipping dispose keeps the test minimal. await Client.DeleteSessionAsync(sessionId); diff --git a/dotnet/test/E2E/SessionE2ETests.cs b/dotnet/test/E2E/SessionE2ETests.cs index aababb670b..fab84bc439 100644 --- a/dotnet/test/E2E/SessionE2ETests.cs +++ b/dotnet/test/E2E/SessionE2ETests.cs @@ -295,6 +295,50 @@ public async Task Resumes_A_Persisted_Session_From_A_New_Client_When_An_Mcp_OAut Assert.Equal(sessionId, session2.SessionId); } + [Fact] + public async Task Should_Recover_Marker_After_Cold_Resume_With_Explicit_Session_Id() + { + await using var isolatedCtx = await E2ETestContext.CreateAsync(); + await isolatedCtx.ConfigureForTestAsync("session", nameof(Should_Recover_Marker_After_Cold_Resume_With_Explicit_Session_Id)); + + var sessionId = $"e2e-cold-resume-{Guid.NewGuid()}"; + + var client1 = isolatedCtx.CreateClient(); + var session1 = await isolatedCtx.CreateSessionAsync(client1, new SessionConfig + { + SessionId = sessionId, + OnPermissionRequest = PermissionHandler.ApproveAll, + }); + Assert.Equal(sessionId, session1.SessionId); + + var answer = await session1.SendAndWaitAsync(new MessageOptions + { + Prompt = "Please remember this exact secret marker for later - MARKER-7f3ac21e. Reply with only the single word \"Acknowledged\".", + }); + Assert.NotNull(answer); + Assert.Contains("Acknowledged", answer!.Data.Content ?? string.Empty); + + await session1.DisposeAsync(); + await client1.ForceStopAsync(); + + var client2 = isolatedCtx.CreateClient(); + var session2 = await isolatedCtx.ResumeSessionAsync(client2, sessionId, new ResumeSessionConfig + { + OnPermissionRequest = PermissionHandler.ApproveAll, + }); + Assert.Equal(sessionId, session2.SessionId); + + var answer2 = await session2.SendAndWaitAsync(new MessageOptions + { + Prompt = "What was the exact secret marker I asked you to remember earlier? Reply with only that marker value and nothing else.", + }); + Assert.NotNull(answer2); + Assert.Contains("MARKER-7f3ac21e", answer2!.Data.Content ?? string.Empty); + + await session2.DisposeAsync(); + await client2.ForceStopAsync(); + } + [Fact] public async Task Should_Throw_Error_When_Resuming_Non_Existent_Session() { diff --git a/dotnet/test/Harness/E2ETestBase.cs b/dotnet/test/Harness/E2ETestBase.cs index 3eb0f0e97a..f664812d58 100644 --- a/dotnet/test/Harness/E2ETestBase.cs +++ b/dotnet/test/Harness/E2ETestBase.cs @@ -113,7 +113,7 @@ protected static async Task SuspendAndUntrackSessionForResumeAsync(CopilotSessio { await session.Rpc.SuspendAsync(); - // In-process clients host separate runtimes, while session.destroy removes the + // In-process clients host separate runtimes, while session.detach releases the // session from the current runtime. Untrack locally to exercise resume without // either replacing an active wrapper or destroying the session first. var removeFromClient = typeof(CopilotSession).GetMethod( diff --git a/dotnet/test/Harness/E2ETestContext.cs b/dotnet/test/Harness/E2ETestContext.cs index 2554c6a4d1..5341e24b68 100644 --- a/dotnet/test/Harness/E2ETestContext.cs +++ b/dotnet/test/Harness/E2ETestContext.cs @@ -597,7 +597,7 @@ private static async Task StopClientForCleanupAsync(CopilotClient client) $"Graceful in-process client cleanup exceeded {s_gracefulClientStopTimeout}; forcing shutdown."); await client.ForceStopAsync(); - // Disposing the connection completes any session.destroy RPC that + // Disposing the connection completes any session.detach RPC that // blocked graceful cleanup. Observe that task before continuing. await gracefulStop.WaitAsync(s_gracefulClientStopTimeout); } diff --git a/dotnet/test/Unit/ClientSessionLifetimeTests.cs b/dotnet/test/Unit/ClientSessionLifetimeTests.cs index a59257b3da..ee6b1002b7 100644 --- a/dotnet/test/Unit/ClientSessionLifetimeTests.cs +++ b/dotnet/test/Unit/ClientSessionLifetimeTests.cs @@ -1671,7 +1671,7 @@ private async Task HandleRequestAsync(Stream stream, JsonElement request, Cancel { ["success"] = true }, - "session.destroy" => await DestroySessionAsync(cancellationToken), + "session.detach" => await DetachSessionAsync(cancellationToken), "runtime.shutdown" => HandleRuntimeShutdown(), _ => throw new InvalidOperationException($"Unexpected RPC method '{method}'.") }; @@ -1708,7 +1708,7 @@ private async Task HandleRequestAsync(Stream stream, JsonElement request, Cancel }; } - private async Task> DestroySessionAsync(CancellationToken cancellationToken) + private async Task> DetachSessionAsync(CancellationToken cancellationToken) { if (_delayDestroy) { @@ -1716,7 +1716,7 @@ private async Task HandleRequestAsync(Stream stream, JsonElement request, Cancel await _allowDestroy.Task.WaitAsync(cancellationToken); } - return []; + return new Dictionary { ["success"] = true }; } private Dictionary HandleRuntimeShutdown() diff --git a/dotnet/test/Unit/GitHubTelemetryTests.cs b/dotnet/test/Unit/GitHubTelemetryTests.cs index 6d76af5742..ec69a8668d 100644 --- a/dotnet/test/Unit/GitHubTelemetryTests.cs +++ b/dotnet/test/Unit/GitHubTelemetryTests.cs @@ -492,7 +492,7 @@ private async Task HandleRequestAsync(Stream stream, JsonElement request, Cancel "session.create" => CaptureCreate(request), "session.resume" => CaptureResume(request), "session.send" => new Dictionary { ["messageId"] = "message-1" }, - "session.destroy" => new Dictionary(), + "session.detach" => new Dictionary { ["success"] = true }, "session.options.update" => new Dictionary { ["success"] = true }, "runtime.shutdown" => new Dictionary(), _ => throw new InvalidOperationException($"Unexpected RPC method '{method}'."), diff --git a/go/client_test.go b/go/client_test.go index ee0e078fb1..9c3c1c0737 100644 --- a/go/client_test.go +++ b/go/client_test.go @@ -2902,8 +2902,10 @@ func serveInMemoryRuntime(t *testing.T, stdinR *io.PipeReader, stdoutW *io.PipeW result = map[string]any{"id": "interest-1"} case "session.options.update": result = map[string]any{"success": true} - case "session.skills.reload", "session.destroy": + case "session.skills.reload": result = map[string]any{} + case "session.detach": + result = map[string]any{"success": true} default: t.Errorf("unexpected JSON-RPC method %s", request.Method) return diff --git a/go/github_token_provider_test.go b/go/github_token_provider_test.go index f00837379f..51e8f3f7f0 100644 --- a/go/github_token_provider_test.go +++ b/go/github_token_provider_test.go @@ -48,8 +48,8 @@ func TestGitHubTokenProviderCreateRequestAndCallback(t *testing.T) { sessionID := sessionIDFromParams(t, params) return []byte(`{"sessionId":"` + sessionID + `","workspacePath":"/workspace"}`), nil }) - server.SetRequestHandler("session.destroy", func(json.RawMessage) (json.RawMessage, *jsonrpc2.Error) { - return []byte(`{}`), nil + server.SetRequestHandler("session.detach", func(json.RawMessage) (json.RawMessage, *jsonrpc2.Error) { + return []byte(`{"success":true}`), nil }) var gotArgs GitHubTokenProviderArgs @@ -201,8 +201,8 @@ func TestGitHubTokenStringRedactsAccessToken(t *testing.T) { func TestGitHubTokenProviderCleanupOnDisconnectError(t *testing.T) { rpcClient, server, _ := newRuntimeShutdownRpcPair(t) t.Cleanup(server.Stop) - server.SetRequestHandler("session.destroy", func(json.RawMessage) (json.RawMessage, *jsonrpc2.Error) { - return nil, &jsonrpc2.Error{Code: -32000, Message: "destroy failed"} + server.SetRequestHandler("session.detach", func(json.RawMessage) (json.RawMessage, *jsonrpc2.Error) { + return nil, &jsonrpc2.Error{Code: -32000, Message: "detach failed"} }) client := &Client{} registrationID := client.registerGitHubTokenProvider(func(GitHubTokenProviderArgs) (*GitHubTokenProviderResult, error) { @@ -213,7 +213,7 @@ func TestGitHubTokenProviderCleanupOnDisconnectError(t *testing.T) { client.unregisterGitHubTokenProvider(registrationID) }) - if err := session.Disconnect(); err == nil || !strings.Contains(err.Error(), "destroy failed") { + if err := session.Disconnect(); err == nil || !strings.Contains(err.Error(), "detach failed") { t.Fatalf("Disconnect error = %v", err) } if len(client.gitHubTokenProviders) != 0 { diff --git a/go/internal/e2e/client_options_e2e_test.go b/go/internal/e2e/client_options_e2e_test.go index 9c7b77c984..54461443ca 100644 --- a/go/internal/e2e/client_options_e2e_test.go +++ b/go/internal/e2e/client_options_e2e_test.go @@ -869,6 +869,10 @@ function handleMessage(message) { writeResponse(message.id, { sessionId, workspacePath: null, capabilities: null }); return; } + if (message.method === "session.detach") { + writeResponse(message.id, { success: true }); + return; + } if (message.method === "session.resume") { const sessionId = (message.params && message.params.sessionId) || "fake-session"; writeResponse(message.id, { sessionId, workspacePath: null, capabilities: null }); diff --git a/go/internal/e2e/session_e2e_test.go b/go/internal/e2e/session_e2e_test.go index efc20250b1..12550e6e2d 100644 --- a/go/internal/e2e/session_e2e_test.go +++ b/go/internal/e2e/session_e2e_test.go @@ -10,6 +10,8 @@ import ( "testing" "time" + "github.com/google/uuid" + copilot "github.com/github/copilot-sdk/go" "github.com/github/copilot-sdk/go/internal/e2e/testharness" "github.com/github/copilot-sdk/go/rpc" @@ -526,6 +528,62 @@ func TestSessionE2E(t *testing.T) { } }) + t.Run("should recover marker after cold resume with explicit session id", func(t *testing.T) { + ctx.ConfigureForTest(t) + + sessionID := "e2e-cold-resume-" + uuid.NewString() + + client1 := ctx.NewClient() + session1, err := client1.CreateSession(t.Context(), &copilot.SessionConfig{ + OnPermissionRequest: copilot.PermissionHandler.ApproveAll, + SessionID: sessionID, + }) + if err != nil { + t.Fatalf("Failed to create session: %v", err) + } + if session1.SessionID != sessionID { + t.Fatalf("Expected explicit session ID %q, got %q", sessionID, session1.SessionID) + } + + answer, err := session1.SendAndWait(t.Context(), copilot.MessageOptions{ + Prompt: `Please remember this exact secret marker for later - MARKER-7f3ac21e. Reply with only the single word "Acknowledged".`, + }) + if err != nil { + t.Fatalf("Failed to send message: %v", err) + } + if ad, ok := answer.Data.(*copilot.AssistantMessageData); !ok || !strings.Contains(ad.Content, "Acknowledged") { + t.Errorf("Expected answer to contain 'Acknowledged', got %v", answer.Data) + } + + if err := session1.Disconnect(); err != nil { + t.Fatalf("Failed to disconnect session: %v", err) + } + client1.ForceStop() + + client2 := ctx.NewClient() + defer client2.ForceStop() + + session2, err := client2.ResumeSession(t.Context(), sessionID, &copilot.ResumeSessionConfig{ + OnPermissionRequest: copilot.PermissionHandler.ApproveAll, + }) + if err != nil { + t.Fatalf("Failed to resume session: %v", err) + } + if session2.SessionID != sessionID { + t.Errorf("Expected resumed session ID to match, got %q vs %q", session2.SessionID, sessionID) + } + + answer2, err := session2.SendAndWait(t.Context(), copilot.MessageOptions{ + Prompt: "What was the exact secret marker I asked you to remember earlier? Reply with only that marker value and nothing else.", + }) + if err != nil { + t.Fatalf("Failed to send message after resume: %v", err) + } + if ad, ok := answer2.Data.(*copilot.AssistantMessageData); !ok || !strings.Contains(ad.Content, "MARKER-7f3ac21e") { + t.Errorf("Expected resumed answer to contain marker, got %v", answer2.Data) + } + }) + t.Run("should throw error when resuming non-existent session", func(t *testing.T) { ctx.ConfigureForTest(t) diff --git a/go/session.go b/go/session.go index 4c31c01e8c..19fec16bb9 100644 --- a/go/session.go +++ b/go/session.go @@ -4,6 +4,7 @@ package copilot import ( "context" "encoding/json" + "errors" "fmt" "log" "sync" @@ -1740,8 +1741,22 @@ func (s *Session) GetEvents(ctx context.Context) ([]SessionEvent, error) { // log.Printf("Failed to disconnect session: %v", err) // } func (s *Session) Disconnect() error { - _, err := s.client.Request(context.Background(), "session.destroy", sessionDestroyRequest{SessionID: s.SessionID}) + result, err := s.client.Request(context.Background(), "session.detach", sessionDetachRequest{SessionID: s.SessionID}) + if err == nil { + var response sessionDetachResponse + if decodeErr := json.Unmarshal(result, &response); decodeErr != nil { + err = fmt.Errorf("failed to decode session detach response: %w", decodeErr) + } else if !response.Success { + if response.Error == "" { + response.Error = "unknown error" + } + err = errors.New(response.Error) + } + } + // Local cleanup always runs, even if the detach RPC failed, so callers + // don't leak in-memory resources (event goroutines, registered + // providers/handlers) just because the runtime couldn't be reached. s.stopEventProcessing() s.releaseGitHubTokenProviderRegistration() diff --git a/go/types.go b/go/types.go index 772202a0bc..fa5ccd8333 100644 --- a/go/types.go +++ b/go/types.go @@ -2890,11 +2890,16 @@ type sessionGetMessagesResponse struct { Events []SessionEvent `json:"events"` } -// sessionDestroyRequest is the request for session.destroy -type sessionDestroyRequest struct { +// sessionDetachRequest is the request for session.detach. +type sessionDetachRequest struct { SessionID string `json:"sessionId"` } +type sessionDetachResponse struct { + Success bool `json:"success"` + Error string `json:"error,omitempty"` +} + // sessionAbortRequest is the request for session.abort type sessionAbortRequest struct { SessionID string `json:"sessionId"` diff --git a/java/sdk/src/main/java/com/github/copilot/CopilotSession.java b/java/sdk/src/main/java/com/github/copilot/CopilotSession.java index f3a35967d3..9144e76c7f 100644 --- a/java/sdk/src/main/java/com/github/copilot/CopilotSession.java +++ b/java/sdk/src/main/java/com/github/copilot/CopilotSession.java @@ -2318,10 +2318,20 @@ public void close() { timeoutScheduler.shutdownNow(); releaseGitHubTokenProviderRegistration(); + RuntimeException detachFailure = null; try { - rpc.invoke("session.destroy", Map.of("sessionId", sessionId), Void.class).get(5, TimeUnit.SECONDS); + SessionDetachResponse response = rpc + .invoke("session.detach", Map.of("sessionId", sessionId), SessionDetachResponse.class) + .get(5, TimeUnit.SECONDS); + if (response == null || !response.success()) { + String detail = response != null && response.error() != null ? response.error() : "unknown error"; + detachFailure = new IllegalStateException("Failed to detach session " + sessionId + ": " + detail); + } } catch (Exception e) { - LOG.log(Level.FINE, "Error destroying session", e); + if (e instanceof InterruptedException) { + Thread.currentThread().interrupt(); + } + detachFailure = new IllegalStateException("Failed to detach session " + sessionId, e); } eventHandlers.clear(); @@ -2333,10 +2343,18 @@ public void close() { exitPlanModeHandler.set(null); autoModeSwitchHandler.set(null); hooksHandler.set(null); + + if (detachFailure != null) { + throw detachFailure; + } } // ===== Internal response types for agent API ===== + @JsonIgnoreProperties(ignoreUnknown = true) + record SessionDetachResponse(@JsonProperty("success") boolean success, @JsonProperty("error") String error) { + } + @JsonIgnoreProperties(ignoreUnknown = true) private record AgentListResponse(@JsonProperty("agents") List agents) { } diff --git a/java/sdk/src/test/java/com/github/copilot/ClientOptionsE2ETest.java b/java/sdk/src/test/java/com/github/copilot/ClientOptionsE2ETest.java index 45056afdb4..a2561434d8 100644 --- a/java/sdk/src/test/java/com/github/copilot/ClientOptionsE2ETest.java +++ b/java/sdk/src/test/java/com/github/copilot/ClientOptionsE2ETest.java @@ -263,6 +263,8 @@ function resultFor(message) { return { sessionId: message.params?.sessionId ?? 'fake-session', openCanvases: [] }; case 'session.resume': return { sessionId: message.params?.sessionId ?? 'fake-session', openCanvases: [] }; + case 'session.detach': + return { success: true }; case 'session.options.update': return { success: true }; default: diff --git a/java/sdk/src/test/java/com/github/copilot/CopilotClientTest.java b/java/sdk/src/test/java/com/github/copilot/CopilotClientTest.java index db40d425f4..ba161ef998 100644 --- a/java/sdk/src/test/java/com/github/copilot/CopilotClientTest.java +++ b/java/sdk/src/test/java/com/github/copilot/CopilotClientTest.java @@ -120,8 +120,8 @@ void testDeleteSessionReleasesGitHubTokenProvider() throws Exception { var rpc = mock(JsonRpcClient.class); when(rpc.invoke(eq("session.delete"), any(), eq(DeleteSessionResponse.class))) .thenReturn(CompletableFuture.completedFuture(new DeleteSessionResponse(true, null))); - when(rpc.invoke(eq("session.destroy"), any(), eq(Void.class))) - .thenReturn(CompletableFuture.completedFuture(null)); + when(rpc.invoke(eq("session.detach"), any(), eq(CopilotSession.SessionDetachResponse.class))) + .thenReturn(CompletableFuture.completedFuture(new CopilotSession.SessionDetachResponse(true, null))); setConnectionFuture(client, rpc, null); var registry = new GitHubTokenProviderRegistry(); diff --git a/java/sdk/src/test/java/com/github/copilot/CopilotSessionTest.java b/java/sdk/src/test/java/com/github/copilot/CopilotSessionTest.java index eb061b029d..667769fab2 100644 --- a/java/sdk/src/test/java/com/github/copilot/CopilotSessionTest.java +++ b/java/sdk/src/test/java/com/github/copilot/CopilotSessionTest.java @@ -352,6 +352,51 @@ void testShouldResumeSessionUsingNewClient() throws Exception { } } + @Test + @Tag("isolated-resume") + void testShouldRecoverMarkerAfterColdResumeWithExplicitSessionId() throws Exception { + final String snapshot = "should_recover_marker_after_cold_resume_with_explicit_session_id"; + ctx.configureForTest("session", snapshot); + + String sessionId = "e2e-cold-resume-" + java.util.UUID.randomUUID(); + + try (CopilotClient client1 = ctx.createClient()) { + CopilotSession session1 = client1.createSession( + new SessionConfig().setSessionId(sessionId).setOnPermissionRequest(PermissionHandler.APPROVE_ALL)) + .get(30, TimeUnit.SECONDS); + assertEquals(sessionId, session1.getSessionId()); + + AssistantMessageEvent answer = session1.sendAndWait(new MessageOptions() + .setPrompt("Please remember this exact secret marker for later - MARKER-7f3ac21e. " + + "Reply with only the single word \"Acknowledged\".")) + .get(60, TimeUnit.SECONDS); + assertNotNull(answer); + assertTrue(answer.getData().content().contains("Acknowledged"), + "Response should contain Acknowledged: " + answer.getData().content()); + + session1.close(); + client1.forceStop().get(30, TimeUnit.SECONDS); + } + + try (CopilotClient client2 = ctx.createClient()) { + CopilotSession session2 = client2 + .resumeSession(sessionId, + new ResumeSessionConfig().setOnPermissionRequest(PermissionHandler.APPROVE_ALL)) + .get(30, TimeUnit.SECONDS); + assertEquals(sessionId, session2.getSessionId()); + + AssistantMessageEvent answer2 = session2.sendAndWait( + new MessageOptions().setPrompt("What was the exact secret marker I asked you to remember earlier? " + + "Reply with only that marker value and nothing else.")) + .get(60, TimeUnit.SECONDS); + assertNotNull(answer2); + assertTrue(answer2.getData().content().contains("MARKER-7f3ac21e"), + "Resumed response should contain marker: " + answer2.getData().content()); + + session2.close(); + } + } + /** * Verifies that sessions work with appended system message configuration. * diff --git a/java/sdk/src/test/java/com/github/copilot/GitHubTelemetryTest.java b/java/sdk/src/test/java/com/github/copilot/GitHubTelemetryTest.java index 5a6e378b2a..b406fc1522 100644 --- a/java/sdk/src/test/java/com/github/copilot/GitHubTelemetryTest.java +++ b/java/sdk/src/test/java/com/github/copilot/GitHubTelemetryTest.java @@ -365,7 +365,8 @@ private void acceptLoop() { respond(rpc, id, Map.of("sessionId", params.path("sessionId").asText("resume-1"), "workspacePath", "/workspace")); }); - rpc.registerMethodHandler("session.destroy", (id, params) -> respond(rpc, id, Map.of())); + rpc.registerMethodHandler("session.detach", + (id, params) -> respond(rpc, id, Map.of("success", true))); rpc.registerMethodHandler("runtime.shutdown", (id, params) -> respond(rpc, id, Map.of())); }); ready.complete(server); diff --git a/java/sdk/src/test/java/com/github/copilot/McpAndAgentsTest.java b/java/sdk/src/test/java/com/github/copilot/McpAndAgentsTest.java index 06d9dca39e..018204d030 100644 --- a/java/sdk/src/test/java/com/github/copilot/McpAndAgentsTest.java +++ b/java/sdk/src/test/java/com/github/copilot/McpAndAgentsTest.java @@ -451,7 +451,7 @@ void testShouldAcceptDefaultAgentConfigurationOnSessionResume() throws Exception assertNotNull(session.getSessionId()); String sessionId = session.getSessionId(); - // Do not call session.close() here — that invokes session.destroy on the + // Do not call session.close() here — that invokes session.detach on the // server, // which removes the session and causes the subsequent resumeSession to fail // with "Session not found". The session handle is simply abandoned and the diff --git a/java/sdk/src/test/java/com/github/copilot/McpAuthInterestRegistrationTest.java b/java/sdk/src/test/java/com/github/copilot/McpAuthInterestRegistrationTest.java index 06ac08a2a4..b83cedc70a 100644 --- a/java/sdk/src/test/java/com/github/copilot/McpAuthInterestRegistrationTest.java +++ b/java/sdk/src/test/java/com/github/copilot/McpAuthInterestRegistrationTest.java @@ -255,7 +255,10 @@ private static JsonNode resultFor(String method, JsonNode params) { } case "session.eventLog.registerInterest" -> result.put("id", "interest-1"); case "session.options.update" -> result.put("success", true); - case "session.skills.reload", "session.destroy" -> { + case "session.skills.reload" -> { + } + case "session.detach" -> { + result.put("success", true); } default -> throw new IllegalStateException("Unexpected RPC method " + method); } diff --git a/java/sdk/src/test/java/com/github/copilot/SessionEventHandlingTest.java b/java/sdk/src/test/java/com/github/copilot/SessionEventHandlingTest.java index b75e710720..5e502459c3 100644 --- a/java/sdk/src/test/java/com/github/copilot/SessionEventHandlingTest.java +++ b/java/sdk/src/test/java/com/github/copilot/SessionEventHandlingTest.java @@ -96,8 +96,8 @@ void testSendAndWaitSkipsAutopilotContinuationIdle() throws Exception { var rpc = mock(JsonRpcClient.class); when(rpc.invoke(eq("session.send"), any(), eq(SendMessageResponse.class))) .thenReturn(CompletableFuture.completedFuture(new SendMessageResponse("message-1"))); - when(rpc.invoke(eq("session.destroy"), any(), eq(Void.class))) - .thenReturn(CompletableFuture.completedFuture(null)); + when(rpc.invoke(eq("session.detach"), any(), eq(CopilotSession.SessionDetachResponse.class))) + .thenReturn(CompletableFuture.completedFuture(new CopilotSession.SessionDetachResponse(true, null))); session = new CopilotSession("test-session-id", rpc); try { diff --git a/java/sdk/src/test/java/com/github/copilot/TimeoutEdgeCaseTest.java b/java/sdk/src/test/java/com/github/copilot/TimeoutEdgeCaseTest.java index 17e1851bb4..01530bc25f 100644 --- a/java/sdk/src/test/java/com/github/copilot/TimeoutEdgeCaseTest.java +++ b/java/sdk/src/test/java/com/github/copilot/TimeoutEdgeCaseTest.java @@ -10,7 +10,11 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; +import java.io.OutputStream; +import java.io.PipedInputStream; +import java.io.PipedOutputStream; import java.net.Socket; +import java.nio.charset.StandardCharsets; import java.util.concurrent.CompletableFuture; import org.junit.jupiter.api.Test; @@ -34,29 +38,46 @@ public class TimeoutEdgeCaseTest { /** - * Creates a {@link JsonRpcClient} whose {@code invoke()} returns futures that - * never complete. The reader thread blocks forever on the input stream, and - * writes go to a no-op output stream. + * Creates a {@link JsonRpcClient} whose prompt requests never complete but + * whose cleanup detach request succeeds. */ private JsonRpcClient createHangingRpcClient() throws Exception { - InputStream blockingInput = new InputStream() { + PipedInputStream input = new PipedInputStream(); + PipedOutputStream responseWriter = new PipedOutputStream(input); + OutputStream sinkOutput = new ByteArrayOutputStream() { @Override - public int read() throws IOException { + public synchronized void flush() throws IOException { + super.flush(); + // Each JsonRpcClient.sendMessage() call writes exactly one + // complete message before calling flush(), so the buffer must + // be cleared after every flush; otherwise a later request's + // "id" lookup can match a stale, already-processed message + // still sitting in the buffer. try { - Thread.sleep(Long.MAX_VALUE); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - return -1; + String request = toString(StandardCharsets.UTF_8); + if (!request.contains("\"method\":\"session.detach\"")) { + return; + } + + int idIndex = request.indexOf("\"id\":"); + int idEnd = request.indexOf(",", idIndex); + String id = request.substring(idIndex + "\"id\":".length(), idEnd); + String response = "{\"jsonrpc\":\"2.0\",\"id\":" + id + ",\"result\":{\"success\":true}}"; + byte[] responseBytes = response.getBytes(StandardCharsets.UTF_8); + responseWriter.write( + ("Content-Length: " + responseBytes.length + "\r\n\r\n").getBytes(StandardCharsets.UTF_8)); + responseWriter.write(responseBytes); + responseWriter.flush(); + } finally { + reset(); } - return -1; } }; - ByteArrayOutputStream sinkOutput = new ByteArrayOutputStream(); var ctor = JsonRpcClient.class.getDeclaredConstructor(InputStream.class, java.io.OutputStream.class, Socket.class, Process.class); ctor.setAccessible(true); - return (JsonRpcClient) ctor.newInstance(blockingInput, sinkOutput, null, null); + return (JsonRpcClient) ctor.newInstance(input, sinkOutput, null, null); } /** @@ -64,7 +85,7 @@ public int read() throws IOException { * completed by a stale timeout. *

* Contract: {@code close()} shuts down the timeout scheduler before the - * blocking {@code session.destroy} RPC call, so any pending timeout task is + * blocking {@code session.detach} RPC call, so any pending timeout task is * cancelled and the future remains incomplete (not exceptionally completed with * {@code TimeoutException}). */ @@ -79,7 +100,7 @@ void testTimeoutDoesNotFireAfterSessionClose() throws Exception { assertFalse(result.isDone(), "Future should be pending before timeout fires"); - // close() blocks up to 5s on session.destroy RPC. The 2s timeout + // close() blocks up to 5s on session.detach RPC. The 2s timeout // fires during that window with the current per-call scheduler. session.close(); diff --git a/java/sdk/src/test/java/com/github/copilot/ZeroTimeoutContractTest.java b/java/sdk/src/test/java/com/github/copilot/ZeroTimeoutContractTest.java index 3d986566dc..25a0e892cc 100644 --- a/java/sdk/src/test/java/com/github/copilot/ZeroTimeoutContractTest.java +++ b/java/sdk/src/test/java/com/github/copilot/ZeroTimeoutContractTest.java @@ -31,9 +31,9 @@ void sendAndWaitWithZeroTimeoutShouldNotTimeOut() throws Exception { var mockRpc = mock(JsonRpcClient.class); when(mockRpc.invoke(any(), any(), any())).thenAnswer(invocation -> { Object method = invocation.getArgument(0); - if ("session.destroy".equals(method)) { - // Make session.close() non-blocking by completing destroy immediately - return CompletableFuture.completedFuture(null); + if ("session.detach".equals(method)) { + // Make session.close() non-blocking by completing detach immediately + return CompletableFuture.completedFuture(new CopilotSession.SessionDetachResponse(true, null)); } // For other calls (e.g., message send), return an incomplete future so the // sendAndWait result does not complete due to a mock response. diff --git a/nodejs/src/session.ts b/nodejs/src/session.ts index eb4c6b7561..f06f114d08 100644 --- a/nodejs/src/session.ts +++ b/nodejs/src/session.ts @@ -2017,10 +2017,19 @@ export class CopilotSession { if (this.disconnected) { return; } - await this.connection.sendRequest("session.destroy", { - sessionId: this.sessionId, - }); + let response: { success: boolean; error?: string } = { success: false }; + for (let attempt = 0; attempt < 2 && !response.success; attempt++) { + response = (await this.connection.sendRequest("session.detach", { + sessionId: this.sessionId, + })) as { success: boolean; error?: string }; + } + if (!response.success) { + throw new Error( + `Failed to disconnect session ${this.sessionId}: ${response.error || "Unknown error"}` + ); + } this._markDisconnected(); + this.onDisconnected?.(); } /** Enables `await using session = ...` syntax for automatic cleanup. */ diff --git a/nodejs/test/e2e/client.e2e.test.ts b/nodejs/test/e2e/client.e2e.test.ts index bc3421bfa1..a529ea8e4c 100644 --- a/nodejs/test/e2e/client.e2e.test.ts +++ b/nodejs/test/e2e/client.e2e.test.ts @@ -107,7 +107,7 @@ describe("Client", () => { expect(errors[0].message).toContain("Failed to disconnect session"); } }, - // Generous timeout: client.stop() must wait for session.destroy to time out + // Generous timeout: client.stop() must wait for session.detach to time out // when the server process is dead. The default 30s can flake on slow CI under load. 60_000 ); diff --git a/nodejs/test/e2e/client_options.e2e.test.ts b/nodejs/test/e2e/client_options.e2e.test.ts index 2f1dee6cb5..4d261bea52 100644 --- a/nodejs/test/e2e/client_options.e2e.test.ts +++ b/nodejs/test/e2e/client_options.e2e.test.ts @@ -99,6 +99,11 @@ function handleMessage(message) { return; } + if (message.method === "session.detach") { + writeResponse(message.id, { success: true }); + return; + } + if (message.method === "session.resume") { const sessionId = message.params?.sessionId ?? message.params?.[0]?.sessionId ?? "fake-session"; writeResponse(message.id, { diff --git a/nodejs/test/e2e/session.e2e.test.ts b/nodejs/test/e2e/session.e2e.test.ts index bab0efc687..4c20acb345 100644 --- a/nodejs/test/e2e/session.e2e.test.ts +++ b/nodejs/test/e2e/session.e2e.test.ts @@ -95,6 +95,59 @@ describe("Sessions", () => { await resumedSession.disconnect(); await originalSession.disconnect(); }); + + it("should recover marker after cold resume with explicit session id", async () => { + const sessionId = `e2e-resume-${Date.now()}`; + const marker = "MARKER-7f3ac21e"; + const firstClient = new CopilotClient({ + workingDirectory: workDir, + env, + connection: RuntimeConnection.forStdio({ path: process.env.COPILOT_CLI_PATH }), + }); + onTestFinished(async () => { + try { + await firstClient.stop(); + } catch { + // ignore + } + }); + + const session = await firstClient.createSession({ + sessionId, + onPermissionRequest: approveAll, + model: "claude-sonnet-4.5", + }); + await session.sendAndWait({ + prompt: `Please remember this exact secret marker for later - ${marker}. Reply with only the single word "Acknowledged".`, + }); + await session.disconnect(); + await firstClient.stop(); + + const secondClient = new CopilotClient({ + workingDirectory: workDir, + env, + connection: RuntimeConnection.forStdio({ path: process.env.COPILOT_CLI_PATH }), + }); + onTestFinished(async () => { + try { + await secondClient.stop(); + } catch { + // ignore + } + }); + const resumedSession = await secondClient.resumeSession(sessionId, { + onPermissionRequest: approveAll, + model: "claude-sonnet-4.5", + }); + const response = await resumedSession.sendAndWait({ + prompt: "What was the exact secret marker I asked you to remember earlier? Reply with only that marker value and nothing else.", + }); + + expect(response?.data.content).toContain(marker); + await resumedSession.disconnect(); + await secondClient.stop(); + }); + it("should create and disconnect sessions", async () => { await using session = await client.createSession({ onPermissionRequest: approveAll, diff --git a/nodejs/test/github-token-provider.test.ts b/nodejs/test/github-token-provider.test.ts index 2202f466a1..661b05c39f 100644 --- a/nodejs/test/github-token-provider.test.ts +++ b/nodejs/test/github-token-provider.test.ts @@ -177,7 +177,7 @@ describe("session GitHub token providers", () => { const client = createMockClient(async (method, params) => { if (method === "session.create") return { sessionId: params.sessionId }; - if (method === "session.destroy") return {}; + if (method === "session.detach") return { success: true }; if (method === "session.delete") return { success: true }; throw new Error(`Unexpected method: ${method}`); }); diff --git a/python/copilot/session.py b/python/copilot/session.py index 3c6d3d54a4..0664cc866e 100644 --- a/python/copilot/session.py +++ b/python/copilot/session.py @@ -1602,6 +1602,7 @@ def __init__( self._open_canvases_lock = threading.Lock() self._rpc: SessionRpc | None = None self._destroyed = False + self._disconnect_lock = asyncio.Lock() self._on_disconnect = on_disconnect def _set_disconnect_callback(self, callback: Callable[[], None]) -> None: @@ -2976,19 +2977,19 @@ async def disconnect(self) -> None: >>> # Clean up when done — session can still be resumed later >>> await session.disconnect() """ - # Ensure that the check and update of _destroyed are atomic so that - # only the first caller proceeds to send the destroy RPC. - with self._event_handlers_lock: - if self._destroyed: - return - self._destroyed = True + async with self._disconnect_lock: + with self._event_handlers_lock: + if self._destroyed: + return + + response = await self._client.request("session.detach", {"sessionId": self.session_id}) + if not response.get("success"): + detail = response.get("error") or "unknown error" + raise RuntimeError(f"Failed to detach session {self.session_id}: {detail}") - try: - await self._client.request("session.destroy", {"sessionId": self.session_id}) - finally: self._run_disconnect_callback() - # Clear handlers even if the request fails. with self._event_handlers_lock: + self._destroyed = True self._event_handlers.clear() with self._tool_handlers_lock: self._tool_handlers.clear() diff --git a/python/e2e/test_client_options_e2e.py b/python/e2e/test_client_options_e2e.py index 4bceabcb5e..b07e9a5402 100644 --- a/python/e2e/test_client_options_e2e.py +++ b/python/e2e/test_client_options_e2e.py @@ -168,6 +168,10 @@ def _get_available_port() -> int: writeResponse(message.id, { success: true }); return; } + if (message.method === "session.detach") { + writeResponse(message.id, { success: true }); + return; + } writeResponse(message.id, {}); } diff --git a/python/e2e/test_session_e2e.py b/python/e2e/test_session_e2e.py index a126eabc40..f57b9f5736 100644 --- a/python/e2e/test_session_e2e.py +++ b/python/e2e/test_session_e2e.py @@ -2,6 +2,7 @@ import base64 import os +import uuid from datetime import datetime import pytest @@ -315,6 +316,57 @@ def on_mcp_auth_request(_request, _invocation): finally: await new_client.force_stop() + async def test_should_recover_marker_after_cold_resume_with_explicit_session_id( + self, ctx: E2ETestContext + ): + session_id = f"e2e-cold-resume-{uuid.uuid4()}" + github_token = DEFAULT_GITHUB_TOKEN if os.environ.get("GITHUB_ACTIONS") == "true" else None + + client1 = CopilotClient( + connection=RuntimeConnection.for_stdio(path=ctx.cli_path), + working_directory=ctx.work_dir, + env=ctx.get_env(), + github_token=github_token, + ) + try: + session1 = await client1.create_session( + on_permission_request=PermissionHandler.approve_all, + session_id=session_id, + ) + assert session1.session_id == session_id + + answer = await session1.send_and_wait( + "Please remember this exact secret marker for later - MARKER-7f3ac21e. " + 'Reply with only the single word "Acknowledged".' + ) + assert answer is not None + assert "Acknowledged" in answer.data.content + + await session1.disconnect() + finally: + await client1.force_stop() + + client2 = CopilotClient( + connection=RuntimeConnection.for_stdio(path=ctx.cli_path), + working_directory=ctx.work_dir, + env=ctx.get_env(), + github_token=github_token, + ) + try: + session2 = await client2.resume_session( + session_id, on_permission_request=PermissionHandler.approve_all + ) + assert session2.session_id == session_id + + answer2 = await session2.send_and_wait( + "What was the exact secret marker I asked you to remember earlier? " + "Reply with only that marker value and nothing else." + ) + assert answer2 is not None + assert "MARKER-7f3ac21e" in answer2.data.content + finally: + await client2.force_stop() + async def test_should_throw_error_resuming_nonexistent_session(self, ctx: E2ETestContext): with pytest.raises(Exception): await ctx.client.resume_session( diff --git a/python/test_github_token_provider.py b/python/test_github_token_provider.py index 5b203f3e34..9566a4f817 100644 --- a/python/test_github_token_provider.py +++ b/python/test_github_token_provider.py @@ -31,8 +31,8 @@ async def request(self, method: str, params: dict[str, Any], **kwargs: Any) -> d if callback is not None: callback(response) return response - if method == "session.destroy": - return {} + if method == "session.detach": + return {"success": True} if method == "session.delete": return {"success": True} raise RuntimeError(f"Unexpected method: {method}") diff --git a/rust/src/errors.rs b/rust/src/errors.rs index 70f4c14ff1..3bf5becbda 100644 --- a/rust/src/errors.rs +++ b/rust/src/errors.rs @@ -152,6 +152,9 @@ pub enum SessionErrorKind { /// Session ID returned by the CLI. returned: SessionId, }, + + /// The CLI could not detach the session. + DetachFailed, } impl fmt::Display for SessionErrorKind { @@ -186,6 +189,7 @@ impl fmt::Display for SessionErrorKind { f, "CLI returned session ID {returned} after SDK registered {requested}" ), + SessionErrorKind::DetachFailed => write!(f, "failed to detach session"), } } } @@ -400,7 +404,7 @@ fn capture_backtrace() -> Option> { /// /// `Client::stop` performs cooperative shutdown across every active /// session before killing the CLI child process. Errors from any -/// per-session `session.destroy` RPC and from the terminal child-kill +/// per-session `session.detach` RPC and from the terminal child-kill /// step are collected here rather than short-circuiting on the first /// failure, so callers see the full picture of what went wrong during /// teardown. diff --git a/rust/src/lib.rs b/rust/src/lib.rs index fd7f12cf14..13b51215a1 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -66,6 +66,12 @@ pub mod session_events; /// [`Client::rpc`] and [`session::Session::rpc`](crate::session::Session::rpc). pub mod rpc; +#[derive(serde::Deserialize)] +struct SessionDetachResponse { + success: bool, + error: Option, +} + // Auto-generated protocol-type modules. Crate-private so the only public // access path is via the `session_events` and `rpc` facade modules above — // callers can never depend on the implementation-detail layout under @@ -2259,6 +2265,25 @@ impl Client { self.call_with_inline_callback(method, params, None).await } + pub(crate) async fn detach_session(&self, session_id: &str) -> Result<()> { + let value = self + .call( + "session.detach", + Some(serde_json::json!({ "sessionId": session_id })), + ) + .await?; + let response: SessionDetachResponse = serde_json::from_value(value)?; + if response.success { + return Ok(()); + } + Err(Error::with_message( + ErrorKind::Session(SessionErrorKind::DetachFailed), + response + .error + .unwrap_or_else(|| "unknown error".to_string()), + )) + } + /// Same as [`call`](Self::call), but installs an `inline_callback` /// that runs synchronously on the JSON-RPC read task the instant the /// successful response is parsed, before it is delivered to this @@ -2599,12 +2624,7 @@ impl Client { let mut first_error = None; for session_id in self.inner.router.session_ids() { - if let Err(error) = self - .call( - "session.destroy", - Some(serde_json::json!({ "sessionId": session_id })), - ) - .await + if let Err(error) = self.detach_session(&session_id).await && first_error.is_none() { first_error = Some(error); @@ -2730,10 +2750,10 @@ impl Client { /// Cooperatively shut down the client and the CLI child process. /// - /// Walks every still-registered session and sends `session.destroy` + /// Walks every still-registered session and sends `session.detach` /// for each one, asks SDK-owned runtimes to shut down, terminates the /// Windows-owned CLI Job Object when present, and reaps the root process. - /// Errors from per-session destroys, runtime shutdown, and final process + /// Errors from per-session detaches, runtime shutdown, and final process /// termination are collected into [`StopErrors`] rather than /// short-circuiting on the first failure — so callers see the full picture /// of teardown. @@ -2762,21 +2782,15 @@ impl Client { self.inner.extension_launch_provider.clear(); // Snapshot the registered session IDs without holding the router - // lock across the destroy RPCs. + // lock across the detach RPCs. for session_id in self.inner.router.session_ids() { - match self - .call( - "session.destroy", - Some(serde_json::json!({ "sessionId": session_id })), - ) - .await - { + match self.detach_session(&session_id).await { Ok(_) => {} Err(e) => { warn!( session_id = %session_id, error = %e, - "session.destroy failed during Client::stop", + "session.detach failed during Client::stop", ); errors.push(e); } diff --git a/rust/src/session.rs b/rust/src/session.rs index 509485e39e..9b9c5e2764 100644 --- a/rust/src/session.rs +++ b/rust/src/session.rs @@ -570,7 +570,7 @@ impl Session { /// Disconnect this session from the CLI. /// - /// Sends the `session.destroy` RPC, stops the event loop, and unregisters + /// Sends the `session.detach` RPC, stops the event loop, and unregisters /// the session from the client. **Session state on disk** (conversation /// history, planning state, artifacts) is **preserved**, so the /// conversation can be resumed later via [`Client::resume_session`] @@ -585,21 +585,14 @@ impl Session { /// [`Client::delete_session`]: crate::Client::delete_session /// [`send_and_wait`]: Self::send_and_wait pub async fn disconnect(&self) -> Result<(), Error> { - self.client - .call( - "session.destroy", - Some(serde_json::json!({ "sessionId": self.id })), - ) - .await?; + self.client.detach_session(&self.id).await?; self.stop_event_loop().await; self.client.unregister_session(&self.id); self.github_token_registration.lock().take(); Ok(()) } - /// Deprecated alias for [`disconnect`](Self::disconnect). The - /// underlying wire RPC happens to be named `session.destroy`, but it - /// only severs the connection — on-disk session state is preserved. + /// Deprecated alias for [`disconnect`](Self::disconnect). /// Prefer `disconnect` in new code. #[deprecated(since = "0.1.0", note = "Use `disconnect()` instead")] pub async fn destroy(&self) -> Result<(), Error> { diff --git a/rust/tests/e2e/client_options.rs b/rust/tests/e2e/client_options.rs index fa8e6f68f7..51880803d3 100644 --- a/rust/tests/e2e/client_options.rs +++ b/rust/tests/e2e/client_options.rs @@ -491,6 +491,10 @@ function handleMessage(message) { writeResponse(message.id, { success: true }); return; } + if (message.method === "session.detach") { + writeResponse(message.id, { success: true }); + return; + } writeResponse(message.id, {}); } diff --git a/rust/tests/e2e/session.rs b/rust/tests/e2e/session.rs index 04c99422ea..02a13e7810 100644 --- a/rust/tests/e2e/session.rs +++ b/rust/tests/e2e/session.rs @@ -627,6 +627,76 @@ async fn should_resume_a_session_using_a_new_client() { .await; } +#[tokio::test] +async fn should_recover_marker_after_cold_resume_with_explicit_session_id() { + super::support::with_dedicated_e2e_context( + "session", + "should_recover_marker_after_cold_resume_with_explicit_session_id", + |ctx| { + Box::pin(async move { + ctx.set_default_copilot_user(); + + let session_id = SessionId::from(format!( + "e2e-cold-resume-{}", + uuid::Uuid::new_v4().simple() + )); + + let client1 = ctx.start_client().await; + let session1 = client1 + .create_session( + ctx.approve_all_session_config() + .with_session_id(session_id.clone()), + ) + .await + .expect("create session"); + assert_eq!(session1.id(), &session_id); + + let first = session1 + .send_and_wait( + "Please remember this exact secret marker for later - MARKER-7f3ac21e. Reply with only the single word \"Acknowledged\".", + ) + .await + .expect("send") + .expect("assistant message"); + assert!(assistant_message_content(&first).contains("Acknowledged")); + + session1 + .disconnect() + .await + .expect("disconnect first session"); + client1.stop().await.expect("stop first client"); + + let new_client = ctx.start_client().await; + let resumed = new_client + .resume_session( + ResumeSessionConfig::new(session_id.clone()) + .with_permission_handler(Arc::new(ApproveAllHandler)) + .with_github_token(super::support::DEFAULT_TEST_TOKEN), + ) + .await + .expect("resume session"); + assert_eq!(resumed.id(), &session_id); + + let second = resumed + .send_and_wait( + "What was the exact secret marker I asked you to remember earlier? Reply with only that marker value and nothing else.", + ) + .await + .expect("send after resume") + .expect("assistant message"); + assert!(assistant_message_content(&second).contains("MARKER-7f3ac21e")); + + resumed + .disconnect() + .await + .expect("disconnect resumed session"); + new_client.stop().await.expect("stop new client"); + }) + }, + ) + .await; +} + #[tokio::test] async fn resumes_a_persisted_session_from_a_new_client_when_an_mcp_oauth_handler_is_configured() { super::support::with_dedicated_e2e_context( diff --git a/rust/tests/session_test.rs b/rust/tests/session_test.rs index 778e13e71f..ad4c8abe42 100644 --- a/rust/tests/session_test.rs +++ b/rust/tests/session_test.rs @@ -1884,7 +1884,7 @@ async fn session_rpc_methods_send_correct_method_names() { let cases: Vec<(&str, Option<&str>)> = vec![ ("session.abort", None), ("session.log", Some("message")), - ("session.destroy", None), + ("session.detach", None), ]; for (expected_method, extra_param_key) in cases { @@ -1893,7 +1893,7 @@ async fn session_rpc_methods_send_correct_method_names() { match expected_method { "session.abort" => s.abort().await.map(|_| ()), "session.log" => s.log("test msg", None).await, - "session.destroy" => s.disconnect().await, + "session.detach" => s.disconnect().await, _ => unreachable!(), } }); @@ -1911,6 +1911,7 @@ async fn session_rpc_methods_send_correct_method_names() { "session.log" => { serde_json::json!({ "eventId": "00000000-0000-0000-0000-000000000000" }) } + "session.detach" => serde_json::json!({ "success": true }), _ => serde_json::json!({}), }; server.respond(&request, response).await; @@ -4841,7 +4842,7 @@ async fn rpc_namespace_client_models_list_dispatches_correctly() { #[tokio::test] async fn client_stop_sends_session_destroy_for_each_active_session() { // One client, two registered sessions. Client::stop must send - // session.destroy for each before returning Ok. + // session.detach for each before returning Ok. let (client, server_read, server_write) = make_client(); let mut server = FakeServer { @@ -4891,31 +4892,33 @@ async fn client_stop_sends_session_destroy_for_each_active_session() { .await; let _session_b = timeout(TIMEOUT, create_b).await.unwrap(); - // Drive Client::stop and respond to each destroy in turn. + // Drive Client::stop and respond to each detach in turn. let stop_handle = tokio::spawn({ let client = client.clone(); async move { client.stop().await } }); - let mut destroyed = Vec::new(); + let mut detached = Vec::new(); for _ in 0..2 { let req = server.read_request().await; - assert_eq!(req["method"], "session.destroy"); - destroyed.push(req["params"]["sessionId"].as_str().unwrap().to_string()); - server.respond(&req, serde_json::json!(null)).await; + assert_eq!(req["method"], "session.detach"); + detached.push(req["params"]["sessionId"].as_str().unwrap().to_string()); + server + .respond(&req, serde_json::json!({ "success": true })) + .await; } - destroyed.sort(); + detached.sort(); let mut expected = [session_id_a.clone(), session_id_b.clone()]; expected.sort(); - assert_eq!(destroyed, expected); + assert_eq!(detached, expected); let stop_result = timeout(TIMEOUT, stop_handle).await.unwrap().unwrap(); assert!(stop_result.is_ok(), "stop returned errors: {stop_result:?}"); } #[tokio::test] -async fn client_stop_aggregates_session_destroy_errors() { - // session.destroy fails on the wire — Client::stop returns +async fn client_stop_aggregates_session_detach_errors() { + // session.detach fails on the wire — Client::stop returns // StopErrors carrying the failure rather than short-circuiting. let (session, mut server) = create_session_pair().await; let client = session.client().clone(); @@ -4923,7 +4926,7 @@ async fn client_stop_aggregates_session_destroy_errors() { let stop_handle = tokio::spawn(async move { client.stop().await }); let req = server.read_request().await; - assert_eq!(req["method"], "session.destroy"); + assert_eq!(req["method"], "session.detach"); let id = req["id"].as_u64().unwrap(); let response = serde_json::json!({ "jsonrpc": "2.0", diff --git a/test/snapshots/session/should_recover_marker_after_cold_resume_with_explicit_session_id.yaml b/test/snapshots/session/should_recover_marker_after_cold_resume_with_explicit_session_id.yaml new file mode 100644 index 0000000000..f862aa80eb --- /dev/null +++ b/test/snapshots/session/should_recover_marker_after_cold_resume_with_explicit_session_id.yaml @@ -0,0 +1,16 @@ +models: + - claude-sonnet-4.5 +conversations: + - messages: + - role: system + content: ${system} + - role: user + content: Please remember this exact secret marker for later - MARKER-7f3ac21e. Reply with only the single word + "Acknowledged". + - role: assistant + content: Acknowledged + - role: user + content: What was the exact secret marker I asked you to remember earlier? Reply with only that marker value and nothing + else. + - role: assistant + content: MARKER-7f3ac21e