Use session.detach for SDK session cleanup

[jmoseley]

Session.disconnect() and equivalent cleanup APIs were documented as preserving resumable session state, but the SDKs sent the global session.destroy RPC. A client that only attached to a shared session could therefore tear it down for every owner.

This updates Node, Python, Go, .NET, Java, and Rust to use the released ownership-aware session.detach RPC for session disposal, client shutdown, and initialization rollback. Detach failures are surfaced from the { success, error } response, while successful cleanup still removes local handlers and routing state. deleteSession remains the explicit path for deleting persisted session data.

The Node coverage includes a multi-client regression proving one client can disconnect without killing the owning client's live session. Lifecycle tests and fake runtimes across the other SDKs now use the same detach contract.

Validated with:

• Node client tests, typecheck, and build
• Python client tests
• Go full test suite and targeted fake-CLI lifecycle coverage
• .NET lifecycle and telemetry unit tests
• Rust formatting, Clippy, and session tests

Java tests were not run locally because this environment does not have a JRE installed.

[Copilot]

Pull request overview

Updates SDK session cleanup to use ownership-aware session.detach, preserving shared and resumable sessions.

Changes:

• Replaces session destruction with detach across six SDKs.
• Validates detach responses and updates cleanup routing.
• Adds lifecycle and multi-client regression coverage.

Show a summary per file

File	Description
rust/tests/session_test.rs	Updates detach lifecycle tests.
rust/src/session.rs	Uses detach for disconnect.
rust/src/lib.rs	Adds detach handling and shutdown cleanup.
rust/src/errors.rs	Adds detach failure errors.
python/copilot/session.py	Uses detach during disconnect.
nodejs/test/e2e/session.e2e.test.ts	Updates disconnected-session assertions.
nodejs/test/e2e/multi-client.e2e.test.ts	Tests shared-session detachment.
nodejs/test/e2e/client.e2e.test.ts	Updates shutdown terminology.
nodejs/test/client.test.ts	Adds detach and rollback coverage.
nodejs/src/session.ts	Implements detach and disconnected guards.
nodejs/src/client.ts	Adds routing cleanup and rollback detachment.
java/src/test/java/com/github/copilot/ZeroTimeoutContractTest.java	Updates cleanup mock.
java/src/test/java/com/github/copilot/TimeoutEdgeCaseTest.java	Updates timeout documentation.
java/src/test/java/com/github/copilot/McpAuthInterestRegistrationTest.java	Adds detach response fixture.
java/src/test/java/com/github/copilot/McpAndAgentsTest.java	Clarifies concurrent attachment behavior.
java/src/test/java/com/github/copilot/GitHubTelemetryTest.java	Updates telemetry test server.
java/src/main/java/com/github/copilot/CopilotSession.java	Uses detach during close.
go/types.go	Defines detach wire types.
go/session.go	Implements validated detach cleanup.
go/internal/e2e/client_options_e2e_test.go	Adds detach fake-runtime support.
go/client_test.go	Updates runtime test responses.
dotnet/test/Unit/GitHubTelemetryTests.cs	Updates telemetry test server.
dotnet/test/Unit/ClientSessionLifetimeTests.cs	Updates lifetime detach behavior.
dotnet/test/Harness/E2ETestContext.cs	Updates cleanup documentation.
dotnet/test/Harness/E2ETestBase.cs	Updates resume documentation.
dotnet/test/E2E/ClientLifecycleE2ETests.cs	Updates lifecycle documentation.
dotnet/src/Session.cs	Implements validated detach disposal.

Review details

Tip

Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

• Files reviewed: 27/27 changed files
• Comments generated: 3
• Review effort level: Balanced

[github-actions]

Generated by SDK Consistency Review Agent for #2307 · sonnet46 123.4 AIC · ⌖ 6.01 AIC · ⊞ 6.6K

Comments that could not be inline-anchored

python/copilot/session.py:2932

Cross-SDK consistency: Python is missing the onDisconnected/onClosed client callback

Node.js, Go, and Java all add a callback that fires on successful disconnect() to automatically remove the session from the client's internal session map:

• Node.js (client.ts): onDisconnected: (disconnectedSession) => { if (this.sessions.get(sessionId) === disconnectedSession) { this.sessions.delete(sessionId); } }
• Go (client.go): `s.onDisconnected = func() { c.sessionsMux.Lock(); .…

python/copilot/session.py:1647

Cross-SDK consistency: Python send() / get_events() are missing use-after-disconnect guards

Node.js adds ensureConnected() to send(), getEvents(), and the rpc getter that throws immediately if the session has been disconnected:

private ensureConnected(): void {
    if (this.disconnected) {
        throw new Error(`Session ${this.sessionId} has been disconnected`);
    }
}

This is called at the top of send(), getEvents(), and the rpc getter in `session.…

[jmoseley]

.NET silences { success: false }

This is intentional following the .NET API review: DisposeAsync() is terminal, offers no useful recovery path, and should not throw for a non-egregious cleanup failure. The failure remains observable through the configured warning logger while local cleanup always completes.

Node.js retries session.detach up to 2 times

This is also intentional and narrowly covered by regression tests. The in-process runtime can return { success: false } after teardown has already removed the attachment; because session.detach is idempotent, one immediate retry confirms that completed state as a successful no-op. Persistent failures still throw after the bounded retry.

[github-actions]

Generated by SDK Consistency Review Agent for #2307 · sonnet46 48.5 AIC · ⌖ 5.54 AIC · ⊞ 6.6K

[github-actions]

Cross-SDK Consistency Review ✅

This PR maintains consistent behavior across all six SDK implementations.

Summary: The session.detach RPC migration is applied to all six languages:

SDK	Status
Node.js/TypeScript	✅ Updated in this PR
Python	✅ Updated in this PR
Go	✅ Updated in this PR
.NET	✅ Updated in this PR
Java	✅ Updated in this PR
Rust	✅ Already used session.detach (no changes needed)

API surface consistency: Each SDK now sends session.detach (instead of session.destroy) during session disposal and client shutdown, and each surfaces the { success, error } response consistently with language-appropriate error handling patterns.

No cross-SDK consistency issues found.

Generated by SDK Consistency Review Agent for #2307 · sonnet46 24.8 AIC · ⌖ 5.37 AIC · ⊞ 6.6K · ◷

[SteveSandersonMS]

I reproduced the real persistence bug: after actual conversation state is written, main's session.destroy path prevents a fresh client from resuming by explicit session ID, while this branch's session.detach path preserves and resumes it. I also added real-runtime cold-resume E2E coverage across all six SDKs using the shared replay snapshot, and validated the Node and .NET targeted E2Es locally. Rust test compilation passes with cargo check --tests; the full Rust E2E run was killed by the sandbox during compilation, not by a test failure. Python/Go/Java targeted runs were not available in this environment due missing toolchains/runners.

[SteveSandersonMS]

I reproduced the persistence bug and completed the PR on current main. The branch now uses session.detach for SDK cleanup across Node, Python, Go, .NET, Java, and Rust, and adds a real-runtime cold-resume E2E test in all six SDKs with a shared deterministic replay snapshot.

[SteveSandersonMS]

Re-approving after the current-main rewrite fix. The branch remains scoped to replacing cleanup-time session.destroy with session.detach across SDKs and adding real-runtime cold-resume E2E coverage.

[SteveSandersonMS]

Re-approving after fixing the Node in-process fake CLI detach response. The branch is mergeable and the change remains the same scoped detach cleanup plus real-runtime cold-resume coverage.

[SteveSandersonMS]

Re-approving after fixing Java and Node fake-runtime detach responses and Java formatting. The implementation still preserves the original scope: cleanup-time detach across all SDKs plus cold-resume E2E coverage.

[SteveSandersonMS]

Re-approving after updating the language fake runtimes to return the new session.detach success payload. The real runtime behavior and scope remain unchanged.

[SteveSandersonMS]

Re-approving after the final test-fake/expectation updates for detach. Rust test compilation passes locally; hosted CI should now validate the full matrix.

[SteveSandersonMS]

Fixed a real Go regression found in the port: Disconnect() was returning early on session.detach RPC failure before running local cleanup (event processing stop, GitHub token provider release, handler map clearing), unlike the original session.destroy path which always cleaned up locally regardless of RPC outcome. Restored unconditional local cleanup; only the returned error depends on RPC outcome. Verified locally: go build ./..., go vet ./..., gofmt -l . clean, and 148/148 relevant Go unit tests pass including the previously-failing TestGitHubTokenProviderCleanupOnDisconnectError.

[SteveSandersonMS]

Fixed the remaining Java InProcess/JDK test failure: the TimeoutEdgeCaseTest fake RPC stream's response-matching buffer wasn't reset after non-matching (hanging) requests, so a later session.detach request's response could be sent with a stale, already-consumed request's id, causing a spurious 5s timeout in close(). Fixed by always resetting the buffer after each flush (each sendMessage call flushes exactly once). Verified locally end-to-end with a real JDK 17 + JDK 25 Maven toolchain (downloaded fresh in this sandbox): mvn spotless:check and the full mvn test suite (all modules, including the new cold-resume E2E test) now pass with zero failures.