Skip to content

fix(go): ensure reliable session event handler unsubscription #24

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nathfavour wants to merge 1 commit into
base: main
Choose a base branch
from
+16 −7
Open

fix(go): ensure reliable session event handler unsubscription #24

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions go/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ import (
"github.com/github/copilot-sdk/go/generated"
)

type sessionHandler struct {
id uint64
fn SessionEventHandler
}

// Session represents a single conversation session with the Copilot CLI.
//
// A session maintains conversation state, handles events, and manages tool execution.
Expand Down Expand Up @@ -44,7 +49,8 @@ type Session struct {
// SessionID is the unique identifier for this session.
SessionID string
client *JSONRPCClient
handlers []SessionEventHandler
handlers []sessionHandler
nextHandlerID uint64
handlerMutex sync.RWMutex
toolHandlers map[string]ToolHandler
toolHandlersM sync.RWMutex
Expand All @@ -60,7 +66,7 @@ func NewSession(sessionID string, client *JSONRPCClient) *Session {
return &Session{
SessionID: sessionID,
client: client,
handlers: make([]SessionEventHandler, 0),
handlers: make([]sessionHandler, 0),
toolHandlers: make(map[string]ToolHandler),
}
}
Expand Down Expand Up @@ -139,16 +145,17 @@ func (s *Session) On(handler SessionEventHandler) func() {
s.handlerMutex.Lock()
defer s.handlerMutex.Unlock()

s.handlers = append(s.handlers, handler)
id := s.nextHandlerID
s.nextHandlerID++
s.handlers = append(s.handlers, sessionHandler{id: id, fn: handler})
Comment on lines +148 to +150
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

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

The nextHandlerID counter uses uint64 which will eventually overflow after 2^64 increments. While this is extremely unlikely in practice (would require registering handlers trillions of times per second for years), consider documenting this assumption or adding a check for wraparound if this is a long-running system. Alternatively, using a map[uint64]SessionEventHandler would allow for ID reuse after unsubscription, though the current approach is likely sufficient for all practical purposes.

Copilot uses AI. Check for mistakes.

// Return unsubscribe function
return func() {
s.handlerMutex.Lock()
defer s.handlerMutex.Unlock()

for i, h := range s.handlers {
// Compare function pointers
if &h == &handler {
if h.id == id {
s.handlers = append(s.handlers[:i], s.handlers[i+1:]...)
break
}
Expand Down Expand Up @@ -236,8 +243,10 @@ func (s *Session) handlePermissionRequest(requestData map[string]interface{}) (P
// are recovered to prevent crashing the event dispatcher.
func (s *Session) dispatchEvent(event SessionEvent) {
s.handlerMutex.RLock()
handlers := make([]SessionEventHandler, len(s.handlers))
copy(handlers, s.handlers)
handlers := make([]SessionEventHandler, 0, len(s.handlers))
for _, h := range s.handlers {
handlers = append(handlers, h.fn)
}
Comment on lines +246 to +249
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

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

The unsubscribe functionality lacks dedicated test coverage. While the helper function GetFinalAssistantMessage in testharness/helper.go uses unsubscribe, there are no tests that verify: (1) multiple handlers can be registered and all receive events, (2) unsubscribing one handler doesn't affect others, (3) calling unsubscribe multiple times is safe as documented. Consider adding unit tests for these scenarios to prevent regression of the bug this PR fixes.

Copilot uses AI. Check for mistakes.
s.handlerMutex.RUnlock()

for _, handler := range handlers {
Expand Down
Loading