Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 27 additions & 14 deletions dotnet/src/Session.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1920,8 +1920,12 @@ public async ValueTask DisposeAsync()

try
{
await InvokeRpcAsync<object>(
"session.destroy", [new SessionDestroyRequest() { SessionId = SessionId }], CancellationToken.None);
var response = await InvokeRpcAsync<SessionDetachResponse>(
"session.detach", [new SessionDetachRequest() { SessionId = SessionId }], CancellationToken.None);
if (!response.Success)
{
LogSessionDetachFailed(SessionId, response.Error ?? "unknown error");
}
}
catch (ObjectDisposedException)
{
Expand All @@ -1934,18 +1938,17 @@ await InvokeRpcAsync<object>(
finally
{
RemoveFromClient();
_eventHandlers = ImmutableInterlocked.InterlockedExchange(ref _eventHandlers, ImmutableArray<EventSubscription>.Empty);
_toolHandlers.Clear();
_commandHandlers.Clear();

_permissionHandler = null;
_userInputHandler = null;
_elicitationHandler = null;
_exitPlanModeHandler = null;
_autoModeSwitchHandler = null;
GC.SuppressFinalize(this);
}

_eventHandlers = ImmutableInterlocked.InterlockedExchange(ref _eventHandlers, ImmutableArray<EventSubscription>.Empty);
_toolHandlers.Clear();
_commandHandlers.Clear();

_permissionHandler = null;
_userInputHandler = null;
_elicitationHandler = null;
_exitPlanModeHandler = null;
_autoModeSwitchHandler = null;
}

[LoggerMessage(Level = LogLevel.Error, Message = "Unhandled exception in broadcast event handler")]
Expand All @@ -1957,6 +1960,9 @@ await InvokeRpcAsync<object>(
[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);

internal record SendMessageRequest
{
public string SessionId { get; init; } = string.Empty;
Expand Down Expand Up @@ -1991,11 +1997,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);
Expand Down Expand Up @@ -2028,7 +2040,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))]
Expand Down
2 changes: 1 addition & 1 deletion dotnet/test/E2E/ClientLifecycleE2ETests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 4 additions & 0 deletions dotnet/test/E2E/ClientOptionsE2ETests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1068,6 +1068,10 @@ function handleMessage(message) {
writeResponse(message.id, { messageId: "fake-message" });
return;
}
if (message.method === "session.detach") {
writeResponse(message.id, { success: true });
return;
}

writeResponse(message.id, {});
}
Expand Down
2 changes: 1 addition & 1 deletion dotnet/test/Harness/E2ETestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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 removes 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(
Expand Down
2 changes: 1 addition & 1 deletion dotnet/test/Harness/E2ETestContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,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);
}
Expand Down
31 changes: 28 additions & 3 deletions dotnet/test/Unit/ClientSessionLifetimeTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,23 @@ public async Task Disposed_Session_Is_Removed_From_Client()
AssertSessionCount(client, sessions: 0);
}

[Fact]
public async Task DisposeAsync_Does_Not_Throw_When_Detach_Fails()
{
await using var server = await FakeCopilotServer.StartAsync();
server.FailDetach();
await using var client = new CopilotClient(new CopilotClientOptions { Connection = RuntimeConnection.ForUri(server.Url) });

var session = await client.CreateSessionAsync(new SessionConfig
{
OnPermissionRequest = PermissionHandler.ApproveAll
});

await session.DisposeAsync();

AssertSessionCount(client, sessions: 0);
}

[Fact]
public async Task Disposing_Session_Remains_Rooted_Until_Destroy_Completes()
{
Expand Down Expand Up @@ -692,6 +709,7 @@ private sealed class FakeCopilotServer : IAsyncDisposable
private readonly object _requestsLock = new();
private string? _lastSessionId;
private bool _delayDestroy;
private bool _failDetach;
private bool _failRuntimeShutdown;

private FakeCopilotServer(TcpListener listener)
Expand Down Expand Up @@ -754,6 +772,11 @@ public void FailRuntimeShutdown()
_failRuntimeShutdown = true;
}

public void FailDetach()
{
_failDetach = true;
}

public async ValueTask DisposeAsync()
{
_allowDestroy.TrySetResult();
Expand Down Expand Up @@ -847,7 +870,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}'.")
};
Expand Down Expand Up @@ -884,15 +907,17 @@ private async Task HandleRequestAsync(Stream stream, JsonElement request, Cancel
};
}

private async Task<Dictionary<string, object?>> DestroySessionAsync(CancellationToken cancellationToken)
private async Task<Dictionary<string, object?>> DetachSessionAsync(CancellationToken cancellationToken)
{
if (_delayDestroy)
{
_destroyStarted.TrySetResult();
await _allowDestroy.Task.WaitAsync(cancellationToken);
}

return [];
return _failDetach
? new Dictionary<string, object?> { ["success"] = false, ["error"] = "detach failed" }
: new Dictionary<string, object?> { ["success"] = true };
}

private Dictionary<string, object?> HandleRuntimeShutdown()
Expand Down
2 changes: 1 addition & 1 deletion dotnet/test/Unit/GitHubTelemetryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ private async Task HandleRequestAsync(Stream stream, JsonElement request, Cancel
"session.create" => CaptureCreate(request),
"session.resume" => CaptureResume(request),
"session.send" => new Dictionary<string, object?> { ["messageId"] = "message-1" },
"session.destroy" => new Dictionary<string, object?>(),
"session.detach" => new Dictionary<string, object?> { ["success"] = true },
"session.options.update" => new Dictionary<string, object?> { ["success"] = true },
"runtime.shutdown" => new Dictionary<string, object?>(),
_ => throw new InvalidOperationException($"Unexpected RPC method '{method}'."),
Expand Down
14 changes: 14 additions & 0 deletions go/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -924,6 +924,13 @@ func (c *Client) CreateSession(ctx context.Context, config *SessionConfig) (*Ses
"",
hasManagedSettings(config.EnableManagedSettings, config.ManagedSettings),
)
s.onDisconnected = func() {
c.sessionsMux.Lock()
defer c.sessionsMux.Unlock()
if c.sessions[sessionID] == s {
delete(c.sessions, sessionID)
}
}

s.registerTools(config.Tools)
s.registerPermissionHandler(config.OnPermissionRequest)
Expand Down Expand Up @@ -1258,6 +1265,13 @@ func (c *Client) ResumeSessionWithOptions(ctx context.Context, sessionID string,
"",
hasManagedSettings(config.EnableManagedSettings, config.ManagedSettings),
)
session.onDisconnected = func() {
c.sessionsMux.Lock()
defer c.sessionsMux.Unlock()
if c.sessions[sessionID] == session {
delete(c.sessions, sessionID)
}
}

session.registerTools(config.Tools)
session.registerPermissionHandler(config.OnPermissionRequest)
Expand Down
34 changes: 33 additions & 1 deletion go/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1939,6 +1939,36 @@ func TestClient_MCPAuthInterestRegistration(t *testing.T) {
})
}

func TestSessionDisconnectUnregistersBeforeClientStop(t *testing.T) {
client, requests, cleanup := newInMemoryClient(t)
defer cleanup()

session, err := client.CreateSession(t.Context(), &SessionConfig{
OnPermissionRequest: PermissionHandler.ApproveAll,
})
if err != nil {
t.Fatalf("CreateSession failed: %v", err)
}
requests.clear()

if err := session.Disconnect(); err != nil {
t.Fatalf("Disconnect failed: %v", err)
}
if err := client.Stop(); err != nil {
t.Fatalf("Stop failed: %v", err)
}

detachCount := 0
for _, request := range requests.snapshot() {
if request.Method == "session.detach" {
detachCount++
}
}
if detachCount != 1 {
t.Fatalf("expected exactly one session.detach request, got %d", detachCount)
}
}

type recordedRequest struct {
Method string
Params map[string]any
Expand Down Expand Up @@ -2032,8 +2062,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
Expand Down
4 changes: 4 additions & 0 deletions go/internal/e2e/client_options_e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -874,6 +874,10 @@ function handleMessage(message) {
writeResponse(message.id, { sessionId, workspacePath: null, capabilities: null });
return;
}
if (message.method === "session.detach") {
writeResponse(message.id, { success: true });
return;
}
writeResponse(message.id, {});
}

Expand Down
29 changes: 26 additions & 3 deletions go/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,11 @@ type Session struct {

// eventCh serializes user event handler dispatch. dispatchEvent enqueues;
// a single goroutine (processEvents) dequeues and invokes handlers in FIFO order.
eventCh chan SessionEvent
closeOnce sync.Once // guards eventCh close so Disconnect is safe to call more than once
eventCh chan SessionEvent
closeOnce sync.Once // guards eventCh close so Disconnect is safe to call more than once
disconnectMu sync.Mutex
disconnected bool
onDisconnected func()

// RPC provides typed session-scoped RPC methods.
RPC *rpc.SessionRPC
Expand Down Expand Up @@ -1716,11 +1719,28 @@ 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})
s.disconnectMu.Lock()
defer s.disconnectMu.Unlock()
if s.disconnected {
return nil
}

result, err := s.client.Request(context.Background(), "session.detach", sessionDetachRequest{SessionID: s.SessionID})
if err != nil {
return fmt.Errorf("failed to disconnect session: %w", err)
}
var response sessionDetachResponse
if err := json.Unmarshal(result, &response); err != nil {
return fmt.Errorf("failed to decode session detach response: %w", err)
}
if !response.Success {
if response.Error == "" {
response.Error = "unknown error"
}
return fmt.Errorf("failed to disconnect session: %s", response.Error)
}

s.disconnected = true
s.closeOnce.Do(func() { close(s.eventCh) })

// Clear handlers
Expand All @@ -1744,6 +1764,9 @@ func (s *Session) Disconnect() error {
s.elicitationHandler = nil
s.elicitationMu.Unlock()

if s.onDisconnected != nil {
s.onDisconnected()
}
return nil
}

Expand Down
9 changes: 7 additions & 2 deletions go/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -2698,11 +2698,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"`
Expand Down
Loading
Loading