fix(go): ensure reliable session event handler unsubscription #24
Conversation
… reliable removal.
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the Go SDK's session event handler unsubscription mechanism. The original implementation used invalid function pointer comparison (&h == &handler) which would never match, preventing handlers from being properly unsubscribed. The fix introduces a unique ID system for tracking handlers, ensuring reliable cleanup while maintaining the order of handler dispatch.
Changes:
- Introduced
sessionHandlerstruct with unique ID tracking for event handlers - Fixed unsubscribe logic to use ID comparison instead of invalid function pointer comparison
- Updated
dispatchEventto properly extract handler functions from the wrapper struct
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| id := s.nextHandlerID | ||
| s.nextHandlerID++ | ||
| s.handlers = append(s.handlers, sessionHandler{id: id, fn: handler}) |
There was a problem hiding this comment.
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.
| handlers := make([]SessionEventHandler, 0, len(s.handlers)) | ||
| for _, h := range s.handlers { | ||
| handlers = append(handlers, h.fn) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Tests verify: - Multiple handlers can be registered and all receive events - Unsubscribing one handler doesn't affect others - Calling unsubscribe multiple times is safe - Handlers are called in registration order - Concurrent subscribe/unsubscribe is safe Addresses review feedback from PR #24.
|
Thanks @nathfavour! I'll merge this now and am adding related tests in #48 |
2 participants
I'm building vibeauracle, a CLI tool leveraging the Copilot agent, and found that the Go SDK's unsubscribe function was failing due to invalid function pointer comparisons. I've refactored the handler registration to use unique IDs, ensuring reliable cleanup while maintaining dispatch order.