Added error message propagation to client. #67
Conversation
In case of quote extension, SDK disconnected client without any info why.
WalkthroughIntroduces a new ErrorMessageEventArgs class for conveying error messages via events and updates MultiModalLiveClient to detect InternalServerError WebSocket closures with a description, log the error, and raise Disconnected with ErrorMessageEventArgs before the generic disconnection path. Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant MultiModalLiveClient as Client
participant Subscriber as Disconnected Handler
Server-->>Client: WebSocket Close (InternalServerError + description)
Client->Client: LogConnectionClosedWithError(description)
Client->>Subscriber: Disconnected(ErrorMessageEventArgs(description))
alt Other close statuses or no description
Server-->>Client: WebSocket Close (other)
Client->>Subscriber: Disconnected(EventArgs)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
🧹 Nitpick comments (7)
src/GenerativeAI.Live/Events/ErrorMessageEventArgs.cs (4)
8-11: Property XML doc is misleading; it should describe the error message, not a “payload”.The comment currently says “Gets the payload of the received message,” which is inconsistent with the property name and intended use. Recommend correcting to avoid confusion.
Apply this diff to update the XML doc:
/// <summary> - /// Gets the payload of the received message. + /// Gets the error message returned by the server. /// </summary> public string ErrorMessage { get; }
16-19: Add null guard for constructor argument to enforce non-null contract.Defensively guard against null to avoid propagating nulls into public API state. This also better documents the expected contract at runtime.
Apply this diff:
- public ErrorMessageEventArgs(string errorMessage) + public ErrorMessageEventArgs(string errorMessage) { - ErrorMessage = errorMessage; + ArgumentNullException.ThrowIfNull(errorMessage); + ErrorMessage = errorMessage; }
6-6: Seal the EventArgs-derived class.EventArgs types are typically sealed unless designed for inheritance. Sealing avoids unintended subclassing and can enable minor runtime optimizations.
Apply this diff:
-public class ErrorMessageEventArgs : EventArgs +public sealed class ErrorMessageEventArgs : EventArgs
13-19: Prefer standard XML tags over and document the parameter.isn’t a standard XML doc tag consumed by tooling. Replace with
and add a tag for clarity.
Apply this diff:
- /// <devdoc> - /// Initializes a new instance of the class. - /// </devdoc> - public ErrorMessageEventArgs(string errorMessage) + /// <summary> + /// Initializes a new instance containing the server-provided error message. + /// </summary> + /// <param name="errorMessage">The error message returned by the server.</param> + public ErrorMessageEventArgs(string errorMessage)src/GenerativeAI.Live/Models/MultiModalLiveClient.cs (3)
564-568: Consider propagating server-provided reasons for more close statuses, not just InternalServerError.The server might include meaningful CloseStatusDescription for other statuses (e.g., EndpointUnavailable, PolicyViolation). To make the API more broadly useful, consider emitting ErrorMessageEventArgs whenever a non-empty CloseStatusDescription is present (excluding NormalClosure).
If helpful, I can prepare a follow-up patch to generalize this branch to:
- check for any non-NormalClosure close status with a non-empty description, and
- emit ErrorMessageEventArgs in those cases.
186-189: Document that Disconnected may carry ErrorMessageEventArgs and guide consumers to inspect e.You’ve intentionally avoided a breaking change by keeping EventHandler instead of EventHandler. That’s fine, but the event’s XML doc should explicitly state that the EventArgs instance may be ErrorMessageEventArgs when a server reason is provided, so consumers know to pattern-match/cast.
Proposed doc update (outside the changed lines; illustrative snippet):
/// <summary> /// Event triggered when the WebSocket client is disconnected. /// When the server provides a reason, the <see cref="EventArgs"/> instance will be an /// <see cref="GenerativeAI.Live.Events.ErrorMessageEventArgs"/> containing the server-provided message. /// </summary> public event EventHandler? Disconnected;
564-568: Evaluate also raising ErrorOccurred with a synthetic exception for observability.In the InternalServerError path you now raise Disconnected with a reason, which solves the user-facing problem. For parity with the DisconnectionType.Error branch, consider optionally also emitting ErrorOccurred, wrapping the description in an Exception (e.g., InvalidOperationException). This is optional but can simplify telemetry for apps relying on ErrorOccurred.
Would you like me to prepare a patch to raise:
- ErrorOccurred?.Invoke(this, new ErrorEventArgs(new InvalidOperationException(info.CloseStatusDescription)));
in this branch as well?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/GenerativeAI.Live/Events/ErrorMessageEventArgs.cs(1 hunks)src/GenerativeAI.Live/Models/MultiModalLiveClient.cs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/GenerativeAI.Live/Models/MultiModalLiveClient.cs (1)
src/GenerativeAI.Live/Events/ErrorMessageEventArgs.cs (2)
ErrorMessageEventArgs(6-20)ErrorMessageEventArgs(16-19)
🔇 Additional comments (1)
src/GenerativeAI.Live/Models/MultiModalLiveClient.cs (1)
9-9: OK: New using for ErrorMessageEventArgs.The using directive is necessary to reference the new ErrorMessageEventArgs type.
| else if (info.CloseStatus == WebSocketCloseStatus.InternalServerError && !string.IsNullOrEmpty(info.CloseStatusDescription)) | ||
| { | ||
| _logger?.LogConnectionClosedWithError(info.Type, info.Exception!); | ||
| Disconnected?.Invoke(this, new ErrorMessageEventArgs(info.CloseStatusDescription)); | ||
| } |
There was a problem hiding this comment.
Avoid logging a null Exception; log the server’s close description instead.
In the InternalServerError path, info.Exception is often null for server-initiated closures. Passing info.Exception! to LogConnectionClosedWithError risks null-related issues and loses the actual reason provided by the server. Log the CloseStatus and CloseStatusDescription explicitly.
Apply this diff to make logging accurate and null-safe:
- else if (info.CloseStatus == WebSocketCloseStatus.InternalServerError && !string.IsNullOrEmpty(info.CloseStatusDescription))
- {
- _logger?.LogConnectionClosedWithError(info.Type, info.Exception!);
- Disconnected?.Invoke(this, new ErrorMessageEventArgs(info.CloseStatusDescription));
- }
+ else if (info.CloseStatus == WebSocketCloseStatus.InternalServerError && !string.IsNullOrWhiteSpace(info.CloseStatusDescription))
+ {
+ _logger?.LogError("Connection closed by server with status {Status}. Reason: {Reason}", info.CloseStatus, info.CloseStatusDescription);
+ Disconnected?.Invoke(this, new ErrorMessageEventArgs(info.CloseStatusDescription));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else if (info.CloseStatus == WebSocketCloseStatus.InternalServerError && !string.IsNullOrEmpty(info.CloseStatusDescription)) | |
| { | |
| _logger?.LogConnectionClosedWithError(info.Type, info.Exception!); | |
| Disconnected?.Invoke(this, new ErrorMessageEventArgs(info.CloseStatusDescription)); | |
| } | |
| else if (info.CloseStatus == WebSocketCloseStatus.InternalServerError && !string.IsNullOrWhiteSpace(info.CloseStatusDescription)) | |
| { | |
| _logger?.LogError( | |
| "Connection closed by server with status {Status}. Reason: {Reason}", | |
| info.CloseStatus, | |
| info.CloseStatusDescription); | |
| Disconnected?.Invoke(this, new ErrorMessageEventArgs(info.CloseStatusDescription)); | |
| } |
🤖 Prompt for AI Agents
In src/GenerativeAI.Live/Models/MultiModalLiveClient.cs around lines 564 to 568,
the InternalServerError branch currently calls LogConnectionClosedWithError with
info.Exception! which can be null and loses the server-provided reason; change
the logging to be null-safe and include the CloseStatus and
CloseStatusDescription (e.g., call a logger method that accepts the close status
and description or build a message string combining info.CloseStatus and
info.CloseStatusDescription), and keep invoking Disconnected with the
CloseStatusDescription so the server reason is propagated without dereferencing
info.Exception.
2 participants
Added that message on server error is propagated back to client.
In case of quote extension, Gemini Live returns Internal Server Error with message:
You exceeded your current quota, please check your plan and billing details. For more information on this error, head to: h.In such case SDK disconnected client without any info why.
Summary by CodeRabbit