fix: prevent permission callback loss and event delivery to disposed sessions#4
Open
fix: prevent permission callback loss and event delivery to disposed sessions#4
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>
PureWeen
force-pushed
the
fix/permission-callback-disposed-session
branch
from
891e2f8 to
4cd8afb
Compare
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>
PureWeen
force-pushed
the
fix/permission-callback-disposed-session
branch
from
8ee8957 to
5817175
Compare
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>
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>
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
1 participant
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.
Problem
In headless persistent mode with a single client, when a session is disposed during reconnection:
Tracked in PureWeen/PolyPilot#300.
Root Cause
Session disposal nulls the permission handler (and other handlers), but the session stays in the client sessions map. When the CLI broadcasts a permission.requested event, the disposed session still receives it. The null handler check silently returns (assumes another client will handle it), but in single-client mode there is no other client.
Fix (3 parts, all platforms)
Applied identically to .NET, Node.js, and Go SDKs:
1. Disposed guard in event dispatch
Prevents any event from reaching a session after disposal begins.
2. OnDisposed callback (Session -> Client cleanup)
Session notifies client at the START of dispose (before RPC teardown). Client removes session from its map immediately (fail-closed).
3. Explicit denial when handler is null
Instead of silently returning, sends DeniedCouldNotRequestFromUser so the CLI does not hang.
Files Changed
.NET: dotnet/src/Session.cs, dotnet/src/Client.cs
Node.js: nodejs/src/session.ts, nodejs/src/client.ts
Go: go/session.go, go/client.go
Testing Note
The repo copilot-instructions.md bans InternalsVisibleTo. The fix is internal implementation. E2E tests are feasible (create session A with handler, dispose, create session B, trigger permission, assert B handler fires) but require snapshot YAML setup.
Validation approach: