Skip to content

Use session.detach for SDK session cleanup - #2307

Open
jmoseley wants to merge 12 commits into
mainfrom
jmoseley-fix-node-session-disconnect
Open

Use session.detach for SDK session cleanup#2307
jmoseley wants to merge 12 commits into
mainfrom
jmoseley-fix-node-session-disconnect

Conversation

@jmoseley

Copy link
Copy Markdown
Contributor

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.

Detach resumed sessions without destroying another client's live session, while retaining owner teardown for client stop and initialization rollback.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 69723dd2-e732-43c6-9f9a-a16c2de3a628
Keep the established E2E title so the strengthened disconnect regression uses its authoritative replay snapshot.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 69723dd2-e732-43c6-9f9a-a16c2de3a628
Advertise protocol v4 across SDKs, use explicit detach only when negotiated, and preserve legacy destroy compatibility for protocol v3 runtimes. Keep protocol constants generated and checked in CI.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 69723dd2-e732-43c6-9f9a-a16c2de3a628
Keep protocol version 3 and negotiate session.detach through the additive connect capability. Preserve legacy destroy compatibility and ensure failed initialization rollback always releases local routing state.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 69723dd2-e732-43c6-9f9a-a16c2de3a628
Remove capability negotiation and the legacy session.destroy fallback. This SDK change will ship only after the detach-capable runtime is released and incorporated.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 69723dd2-e732-43c6-9f9a-a16c2de3a628
Resolve the Node client conflict by preserving session disconnect routing cleanup alongside managed permission settings from main.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 69723dd2-e732-43c6-9f9a-a16c2de3a628
Use the ownership-aware session.detach RPC for session disposal, client shutdown, and initialization rollback across every SDK. Validate unsuccessful detach responses and update lifecycle tests and fake runtimes for the released wire contract.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 69723dd2-e732-43c6-9f9a-a16c2de3a628
@jmoseley
jmoseley requested a review from a team as a code owner August 10, 2026 20:04
Copilot AI balanced review requested due to automatic review settings August 10, 2026 20:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread python/copilot/session.py Outdated
Comment thread dotnet/src/Session.cs Outdated
Comment thread java/src/main/java/com/github/copilot/CopilotSession.java Outdated
@github-actions

This comment has been minimized.

Comment thread dotnet/src/Session.cs Outdated
Serialize and finalize disconnect cleanup consistently across SDKs, unregister manually detached Go and Java sessions, propagate Java detach failures, and teach fake runtimes the detach response contract.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 69723dd2-e732-43c6-9f9a-a16c2de3a628
@github-actions

This comment has been minimized.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) =&gt; { 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.…

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 69723dd2-e732-43c6-9f9a-a16c2de3a628
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 69723dd2-e732-43c6-9f9a-a16c2de3a628
@github-actions

This comment has been minimized.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 69723dd2-e732-43c6-9f9a-a16c2de3a628
@jmoseley

Copy link
Copy Markdown
Contributor Author

.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

This comment has been minimized.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread nodejs/src/session.ts
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 69723dd2-e732-43c6-9f9a-a16c2de3a628
@github-actions

Copy link
Copy Markdown
Contributor

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 ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants