fix: clean up disposed sessions from client map to prevent memory leak#1130
Closed
PureWeen wants to merge 5 commits intogithub:mainfrom
Closed
fix: clean up disposed sessions from client map to prevent memory leak#1130PureWeen wants to merge 5 commits intogithub:mainfrom
PureWeen wants to merge 5 commits intogithub:mainfrom
Conversation
…sessions When a session is disposed during reconnection in persistent headless mode: 1. DispatchEvent could deliver events to disposed sessions (handler already nulled) 2. Session remained in Client._sessions dictionary after disposal 3. Permission requests to disposed sessions silently returned (no denial sent) 4. In single-client mode, this caused CLI to hang forever waiting for permission response Three-part fix: - Add disposed guard in DispatchEvent to prevent event delivery after disposal - Add OnDisposed callback wired in Client to remove session from _sessions on dispose - Send explicit PermissionDenied when handler is null instead of silent return Includes 8 tests proving the bug exists and verifying the fix. Fixes PureWeen/PolyPilot#300 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The repo's copilot-instructions.md explicitly bans IVT: 'Never add InternalsVisibleTo to any project file when writing tests. Tests must only access public APIs.' The production fix (disposed guard, OnDisposed callback, explicit denial) is all internal implementation — no IVT needed for the fix itself. The unit tests that directly constructed internal types and called internal methods have been removed. The fix is best validated through: 1. Code review of the defensive changes in Session.cs and Client.cs 2. E2E integration testing of the reconnection scenario 3. Manual testing in PolyPilot with persistent headless mode Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Port the 3-part session disposal fix to all SDK platforms: Node.js (nodejs/src/session.ts, nodejs/src/client.ts): - Disposed guard in _dispatchEvent - onDisposed callback wired in createSession/resumeSession - Explicit denial when permissionHandler is undefined - Also clears commandHandlers, elicitationHandler, userInputHandler in disconnect() Go (go/session.go, go/client.go): - Disposed guard in dispatchEvent using atomic.Int32 - onDisposed callback wired in CreateSession/ResumeSessionWithOptions - Explicit denial via RPC.Permissions.HandlePendingPermissionRequest All three platforms now have identical behavior: disposed sessions are removed from the client map, late events are dropped, and missing permission handlers send explicit denials instead of hanging the CLI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tests the fix for issue github#300: when session A is disposed, creating session B and triggering a permission-requiring tool call should route the permission event to session B (not the disposed session A). Uses the existing E2E snapshot harness with a new YAML snapshot that has two conversations: one for session A's simple prompt and one for session B's tool-using prompt. Key assertions: - Session B's permission handler fires (session completes successfully) - Session A's handler does NOT fire (disposed session is removed from map) - 15s timeout ensures regression (infinite hang) fails fast in CI Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a deterministic hang where permission.requested broadcasts can be routed to a disposed session that no longer has a permission handler, causing the CLI to wait indefinitely for a decision (notably in single-client headless/persistent-server scenarios).
Changes:
- Adds disposal/disconnect guards to prevent event dispatch into disposed sessions (defense-in-depth).
- Introduces an “on disposed” callback so clients remove sessions from their session maps immediately at the start of disposal.
- Sends an explicit permission denial when no handler is available (Node.js/Go, and now .NET) to avoid indefinite hangs; adds a .NET E2E regression test + snapshot.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
test/snapshots/permissions/should_handle_permission_after_prior_session_disposed.yaml |
New replay snapshot for the disposal/permission regression scenario |
nodejs/src/session.ts |
Disposed guard, onDisposed callback hook, explicit denial when handler missing, expanded cleanup |
nodejs/src/client.ts |
Wires session.onDisposed to remove sessions from the client map |
go/session.go |
Disconnected guard (atomic), onDisposed callback hook, explicit denial when handler missing |
go/client.go |
Wires session.onDisposed to remove sessions from the client map |
dotnet/test/PermissionTests.cs |
Adds E2E regression test that reproduces the hang and validates correct routing post-disposal |
dotnet/src/Session.cs |
Disposed guard, OnDisposed callback hook, explicit denial when handler missing |
dotnet/src/Client.cs |
Wires session.OnDisposed to remove sessions from the client map |
Comment on lines
+993
to
+1008
| this._disposed = true; | ||
|
|
||
| // Remove from client's session map before the RPC so that events | ||
| // arriving during the round-trip are never dispatched (fail-closed). | ||
| this.onDisposed?.(this.sessionId); | ||
|
|
||
| await this.connection.sendRequest("session.destroy", { | ||
| sessionId: this.sessionId, | ||
| }); | ||
| this.eventHandlers.clear(); | ||
| this.typedEventHandlers.clear(); | ||
| this.toolHandlers.clear(); | ||
| this.commandHandlers.clear(); | ||
| this.permissionHandler = undefined; | ||
| this.elicitationHandler = undefined; | ||
| this.userInputHandler = undefined; |
Comment on lines
+1148
to
+1152
| s.disconnected.Store(1) | ||
|
|
||
| // Remove from client's session map before the RPC so the client stops | ||
| // routing events to this session. | ||
| if s.onDisposed != nil { |
Comment on lines
+993
to
+997
| this._disposed = true; | ||
|
|
||
| // Remove from client's session map before the RPC so that events | ||
| // arriving during the round-trip are never dispatched (fail-closed). | ||
| this.onDisposed?.(this.sessionId); |
The original fix incorrectly assumed permission events are broadcast across all sessions within a client. In reality, they are dispatched per-session by sessionId. A disposed session can never intercept events meant for another. Changes: - Remove explicit denial RPC in all 3 SDKs (.NET, Node.js, Go) — the null handler return is intentional for multi-client scenarios - Add try/finally (Node.js) and defer (Go) to guarantee handler cleanup even if session.destroy RPC fails - Rewrite E2E test with accurate description (cleanup validation, not permission routing fix) What remains (the actual fix): - OnDisposed callback removes session from Client._sessions on disposal (prevents memory leak — sessions accumulated without cleanup) - Disposed guard in DispatchEvent prevents race-condition event delivery during async disposal Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a
CopilotSessionis disposed viaDisposeAsync()(ordisconnect()/Disconnect()in Node.js/Go), the session is not removed from the owningClient._sessionsmap. OnlyStopAsync/ForceStopAsyncclear the map. This means disposed sessions accumulate in memory for the lifetime of the client — a memory leak.Additionally, during the async disposal window (between setting the disposed flag and completing the
session.destroyRPC round-trip), events can still be dispatched to the session even though its handlers are being torn down.Changes
All three SDKs (.NET, Node.js, Go):
OnDisposedcallback — New property onSessionthat the owningClientwires to remove the session from its_sessionsmap. Called at the start of disposal (before thesession.destroyRPC), ensuring events arriving during the round-trip are never dispatched (fail-closed).Disposed guard in
DispatchEvent— Checks the disposed flag and returns early, preventing race-condition event delivery during async disposal.Guaranteed handler cleanup — Node.js wraps
session.destroyintry/finally, Go usesdefer, ensuring event handlers are always cleared even if the RPC fails. (.NET already had propertry/catchinDisposeAsync.)What this does NOT fix
Permission events (
permission.requested) are dispatched per-session bysessionId— they are NOT broadcast across all sessions within a client. A disposed session A can never intercept permission events meant for session B. This PR does not change permission routing behavior; it only prevents memory leaks and eliminates a small race window.Testing
Should_Handle_Permission_After_Prior_Session_Disposed— creates session A, disposes it, creates session B on the same client, and verifies session B handles permissions normally. Exercises theOnDisposedcleanup path.Files Changed
dotnet/src/Session.cs,dotnet/src/Client.csnodejs/src/session.ts,nodejs/src/client.tsgo/session.go,go/client.godotnet/test/PermissionTests.cs,test/snapshots/permissions/should_handle_permission_after_prior_session_disposed.yaml