Fix ObjectDisposedException when disposing session after client.StopAsync()

[SteveSandersonMS]

Summary

Cherry-picked from #371 with merge conflicts resolved. Closes #306.

Original author: @parthtrivedi2492

When await client.StopAsync() is called before await using var session goes out of scope, session.DisposeAsync() is called twice — the second call throws ObjectDisposedException.

Solution

Make CopilotSession.DisposeAsync() idempotent and tolerant to already-disposed connections:

1. Added _isDisposed flag — Thread-safe using Interlocked.Exchange
2. Made DisposeAsync idempotent — Returns immediately if already disposed
3. Handle connection failures gracefully — Catches ObjectDisposedException and IOException

Changes

• dotnet/src/Session.cs: Add idempotency and exception handling to DisposeAsync()
• dotnet/test/ClientTests.cs: Add regression test

Conflict resolution

• Session.cs: Both this PR and main added fields at the same location — kept both _sessionRpc (from main's RPC codegen) and _isDisposed
• ClientTests.cs: Adapted test to use new CopilotClientOptions() since _cliPath was removed on main

[github-actions]

Cross-SDK Consistency Review ✅

I've reviewed this PR for consistency across all four SDK implementations (Node.js/TypeScript, Python, Go, and .NET).

Summary

This PR fixes a .NET-specific issue where disposing a session after stopping the client throws ObjectDisposedException or IOException. The fix:

1. Makes DisposeAsync() idempotent using an _isDisposed flag
2. Catches and ignores disposal exceptions when the connection is already closed
3. Adds a regression test

Cross-SDK Analysis

After reviewing the equivalent code in other SDKs, I found that this same issue exists in Node.js, Python, and Go, but they lack similar defensive handling:

Node.js/TypeScript (nodejs/src/session.ts):

• session.destroy() has no idempotency flag or error handling
• If a user manually calls session.destroy() after client.stop() (which already destroys sessions), it would throw

Python (python/copilot/session.py):

• session.destroy() has no idempotency flag or error handling
• Would throw if called after the client connection is closed

Go (go/session.go):

• session.Destroy() has no idempotency flag or error handling
• Would return an error if called after client.Stop() already destroyed it

Recommendation

Consider applying the same defensive pattern to the other SDKs:

• Node.js: Add try/catch in session.destroy() and track destroyed state
• Python: Add try/except in session.destroy() and track destroyed state
• Go: Ignore errors in session.Destroy() when connection is already closed and track destroyed state

This would ensure consistent, robust behavior across all SDK implementations, especially for users using automatic cleanup patterns (await using in .NET, defer in Go, context managers in Python).

Verdict

✅ This PR is good to merge. While it highlights a cross-SDK inconsistency, the fix is appropriate for .NET and doesn't introduce new inconsistencies. The issue in other SDKs should be addressed separately.

AI generated by SDK Consistency Review Agent

[Copilot]

Pull request overview

This PR fixes a crash when disposing a CopilotSession after the client has already been stopped, by making CopilotSession.DisposeAsync() idempotent and tolerant to disposed connections. The fix addresses issue #306 where calling client.StopAsync() before a session's await using cleanup would cause an ObjectDisposedException.

Changes:

• Added idempotency to DisposeAsync() using thread-safe Interlocked.Exchange pattern with _isDisposed flag
• Added exception handling to gracefully handle ObjectDisposedException and IOException during disposal
• Added regression test to verify the fix

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
dotnet/src/Session.cs	Implemented idempotent disposal with thread-safe flag and exception handling to prevent crashes when disposing after client shutdown
dotnet/test/ClientTests.cs	Added regression test verifying session disposal succeeds after client.StopAsync() without throwing exceptions

[Copilot]

Consider catching all exceptions here rather than specific types. Dispose methods should be resilient and never throw exceptions as per IAsyncDisposable best practices. While ObjectDisposedException and IOException cover the expected cases, other unexpected exceptions (e.g., from custom JsonRpc implementations) could still propagate. Consider using a catch-all:

catch (Exception)
{
    // Suppress all exceptions during disposal
}

This matches the pattern used in CopilotClient.CleanupConnectionAsync which catches all exceptions during cleanup.

Suggested change

	catch (ObjectDisposedException)
	{
	// Connection was already disposed (e.g., client.StopAsync() was called first)
	}
	catch (IOException)
	{
	// Connection is broken or closed
	}
	catch (Exception)
	{
	// Suppress all exceptions during disposal (connection may already be closed or broken)
	}