Add per-session notifications handling #46
Conversation
WalkthroughThe pull request updates the notification handling mechanism across several components. In the examples, the Changes
Possibly related PRs
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (10)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
server/server_test.go (2)
180-183: Check error return value from RegisterSession in testsThe error return value from
RegisterSessionshould be checked, even in test code.- server.RegisterSession(&fakeSession{ - sessionID: "test", - notificationChannel: notificationChannel, - }) + err := server.RegisterSession(&fakeSession{ + sessionID: "test", + notificationChannel: notificationChannel, + }) + require.NoError(t, err)🧰 Tools
🪛 golangci-lint (1.62.2)
180-180: Error return value of
server.RegisterSessionis not checked(errcheck)
209-213: Check error return value from RegisterSession in loopSimilar to above, the error return value from
RegisterSessionshould be checked within the loop.- server.RegisterSession(&fakeSession{ - sessionID: fmt.Sprintf("test%d", i), - notificationChannel: notificationChannel, - }) + err := server.RegisterSession(&fakeSession{ + sessionID: fmt.Sprintf("test%d", i), + notificationChannel: notificationChannel, + }) + require.NoError(t, err)🧰 Tools
🪛 golangci-lint (1.62.2)
209-209: Error return value of
server.RegisterSessionis not checked(errcheck)
server/server.go (2)
125-149: Potential data loss when broadcasting notifications
Currently, the non-blocking send with adefaultcase will silently drop notifications if a session’s channel is backlogged. Consider adding a mechanism for buffering or queuing notifications so they are not lost when clients temporarily cannot consume messages.
153-158: Non-blocking send may drop individual client notifications
Similar to the broadcast scenario, thedefaultbranch can lead to lost notifications if the channel is blocked. If guaranteed delivery is important, you may consider adding a buffer or alternative queuing strategy.Also applies to: 173-173
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/everything/main.go(2 hunks)server/server.go(6 hunks)server/server_test.go(13 hunks)server/sse.go(3 hunks)server/stdio.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
server/sse.go
160-160: Error return value of s.server.RegisterSession is not checked
(errcheck)
server/stdio.go
73-73: Error return value of s.server.RegisterSession is not checked
(errcheck)
server/server_test.go
180-180: Error return value of server.RegisterSession is not checked
(errcheck)
209-209: Error return value of server.RegisterSession is not checked
(errcheck)
240-240: Error return value of server.RegisterSession is not checked
(errcheck)
🔇 Additional comments (21)
examples/everything/main.go (2)
303-303: Parameter addition looks goodThe context parameter is now passed to
SendNotificationToClientwhich aligns with the per-session notification handling approach introduced in this PR.
340-340: Parameter addition looks goodThe context parameter is now correctly passed to
SendNotificationToClientin the long-running operation tool handler.server/sse.go (6)
21-22: Good addition of session-specific fieldsThe
sessionIDandnotificationChannelfields enable per-session notification handling, which aligns with the PR objective of removing global notification channels.
25-31: ClientSession interface implementationThese methods properly expose the session ID and notification channel, implementing the ClientSession interface.
33-33: Good compile-time interface checkThis static assertion ensures that
sseSessionproperly implements theClientSessioninterface.
155-156: Fields properly initializedThe session ID and notification channel fields are correctly initialized during session creation.
168-177: Using per-session notification channelThis code now correctly uses the session-specific notification channel instead of a global one, which improves encapsulation and aligns with the PR goals.
233-233: Context properly set with sessionThe context is now properly set with the session information for the request handling.
server/stdio.go (2)
25-42: Good implementation of ClientSession for stdioThe
stdioSessionstruct and its methods properly implement theClientSessioninterface for stdio connections. The singleton pattern makes sense since stdio only supports a single client.
83-93: Using per-session notification channelThis code now correctly uses the session-specific notification channel instead of a global one, consistent with the changes in the sse implementation.
server/server_test.go (3)
147-153: Updated test function signaturesThe test function signatures have been updated to accommodate the new notification handling mechanism, passing notification channels to the action and validate functions.
472-538: Great test coverage for SendNotificationToClientThis new test function thoroughly tests the
SendNotificationToClientmethod across multiple scenarios:
- No active session
- With an active session
- With a blocked notification channel
This ensures the new per-session notification mechanism works correctly under different conditions.
892-905: Good test helper implementationThe
fakeSessiontype properly implements theClientSessioninterface for testing purposes, including the compile-time interface check.server/server.go (8)
49-55: Well-defined interface for decoupled session-based notifications
This interface is concise and clearly separates session management concerns from the server. The use of a send-only channel for notifications is an effective way to prevent accidental reads on the receiving side.
57-59: Good practice for context key definition
Using an empty struct as a context key helps avoid key collisions and keeps the usage clean.
60-66: Helper function for extracting session from context
Cleanly checks if the context contains a valid session and returns nil otherwise, which aligns well with typical usage patterns.
99-105: Context wrapping for session is straightforward
Storing theClientSessionviacontext.WithValueis a standard approach and fits well into the rest of the codebase.
107-116: Safe session registration with concurrency awareness
UsingLoadOrStoreonsync.Mapeffectively prevents duplicate registrations and ensures thread-safety for session management.
118-124: Simple and effective session unregistration
Removing sessions by their ID cleanly completes the session lifecycle management with minimal overhead.
526-526: Automatic tool list change notification
Notifying all clients of tool additions right after server initialization is logical and keeps sessions updated in real time.
549-549: Consistent notification upon tool deletion
This mirrors the addition workflow, ensuring all sessions receive an up-to-date tool list.
torkve
force-pushed
the
issue42-notifications
branch
from
3cddf41 to
67c327e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/sse.go (1)
155-156: Buffer size consistency considerationThe hardcoded buffer size (100) is consistent with other channel buffer sizes in the codebase. This is appropriate for most use cases, but consider making this configurable if you anticipate high-volume notification scenarios.
server/stdio.go (1)
25-25: Fix typo in commentThere's a minor typo in the comment.
-// stdioSssion is a static client session, since stdio has only one client. +// stdioSession is a static client session, since stdio has only one client.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/everything/main.go(2 hunks)server/server.go(6 hunks)server/server_test.go(13 hunks)server/sse.go(3 hunks)server/stdio.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/everything/main.go
- server/server.go
🔇 Additional comments (15)
server/sse.go (6)
21-22: Improved architecture with per-session notification channelsThe addition of these session-specific fields improves isolation between sessions and makes the system more modular. This aligns well with the PR objective of enhancing notification handling.
25-31: Good implementation of accessor methodsThese accessor methods properly implement the required ClientSession interface, providing controlled access to the session's identifier and notification channel.
33-33: Good practice: Using type assertion to verify interface complianceThis compile-time check ensures that sseSession properly implements the ClientSession interface.
162-166: Properly implemented error handling for RegisterSessionGood implementation of error handling for RegisterSession and proper cleanup with deferred UnregisterSession. This addresses the issue raised in the previous review.
172-181: Improved notification handling with session-specific channelsThe notification handling now uses the per-session channel instead of a global channel, which aligns with the PR objective of simplifying notification processing in transports.
237-237: Updated context handling to use the session directlyThis change streamlines context creation by directly using the session object, which is more direct and less error-prone than using separate context components.
server/stdio.go (3)
26-42: Good implementation of per-session notification for stdioThe stdioSession implementation with a static instance is a clean approach for stdio which only supports a single client. The static instance with its notification channel properly isolates the stdio session from others.
72-76: Properly implemented error handling for RegisterSessionGood implementation of error handling for RegisterSession and proper cleanup with deferred UnregisterSession. This addresses the issue raised in the previous review.
84-94: Improved notification handling with session-specific channelsThe notification handling now properly uses the stdioSessionInstance's notification channel instead of a global channel, which aligns with the PR objective of making session workers receive only notifications relevant to their specific session.
server/server_test.go (6)
150-152: Updated test signatures to accommodate per-session notificationsThe test function signatures have been appropriately updated to work with the new per-session notification system, enabling more thorough testing of notification handling.
155-175: Good test case: Testing with no active sessionsThis test case verifies that no notifications are sent when there are no active sessions, which is an important edge case to test.
178-205: Good test case: Testing with a single active sessionThis test case properly verifies notification delivery with a single active session.
207-238: Good test case: Testing with multiple active sessionsThis test case ensures that notifications are properly delivered to multiple active sessions, which is crucial for verifying the scalability of the notification system.
476-542: Comprehensive test for SendNotificationToClientThe added test function thoroughly tests the SendNotificationToClient method under various conditions: no active session, active session, and blocked notification channel. This is essential for ensuring robust notification handling.
896-909: Well-designed test helper: fakeSession implementationThe fakeSession type is a clean and minimal implementation of the ClientSession interface for testing purposes, making the tests more readable and maintainable.
torkve
force-pushed
the
issue42-notifications
branch
from
67c327e to
4697e96
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/everything/main.go(2 hunks)server/server.go(6 hunks)server/server_test.go(13 hunks)server/sse.go(3 hunks)server/stdio.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/everything/main.go
- server/sse.go
- server/stdio.go
- server/server.go
🔇 Additional comments (9)
server/server_test.go (9)
6-6: Added necessary imports for enhanced test functionality.The addition of the
fmtpackage for string formatting andrequirefromtestifyfor more strict assertions improves the testing capabilities.Also applies to: 12-12
147-168: LGTM! Improved test structure with session-based notifications.The test function has been updated to accommodate the new per-session notification mechanism, testing the behavior when no active sessions are present. This correctly verifies that no notifications are sent when there are no active sessions.
177-204: Well-implemented test for single active session notifications.This test case properly verifies that when a single session is registered, exactly one notification is sent through that session's channel. The test correctly registers a fake session with a notification channel and validates that the tools list changed notification is received.
206-237: Good coverage for multiple session notifications.This test case thoroughly verifies that each active session receives its own notification when tools are updated. The loop creating multiple sessions with separate notification channels ensures comprehensive coverage of this scenario.
239-264: Successfully adapted AddTool test to use session-based notifications.The test case for AddTool has been properly updated to work with the new per-session notification approach, correctly verifying that multiple notifications are sent when tools are added individually.
305-309: Properly set up notification channel and collection for testing.The test now correctly creates a buffered notification channel and prepares an empty slice to collect notifications, then passes the channel to the action function for per-session notification handling.
311-312: Channel receiving correctly updated.The notification collection code has been updated to use the new notification type directly, which is consistent with the per-session notification approach.
476-542: Excellent new test for session-based client notifications.This comprehensive test validates the
SendNotificationToClientfunctionality with three important scenarios:
- No active session - correctly verifies an error is returned
- Active session - validates notifications are properly sent to the session's channel
- Blocked channel - confirms the appropriate error handling when a notification channel is full
The test covers all key aspects of the new per-session notification mechanism.
896-910: Well-designed fake session implementation.The
fakeSessionstruct and its methods provide a clean implementation of theClientSessioninterface for testing purposes. This implementation includes both the session identifier and notification channel, with proper accessor methods.
torkve
force-pushed
the
issue42-notifications
branch
2 times, most recently
from
71ca83f to
f416918
Compare
torkve
force-pushed
the
issue42-notifications
branch
from
f416918 to
3a67a53
Compare
|
@ezynda3 is there anything else needed from me here? |
|
gtg |
This feature removes global notification channel and notification context, and introduces per-session notification channels.
It also simplifies notification processing in transports as each session worker always receives notifications only for its session.
This fixes #42
Summary by CodeRabbit
New Features
Refactor
Tests