Skip to content

Refactored DisconnectionHappened handler to always report errors #82

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

Merged
merged 1 commit into from
Oct 20, 2025
Merged

Refactored DisconnectionHappened handler to always report errors #82

Show file tree
Hide file tree
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
7 changes: 4 additions & 3 deletions src/GenerativeAI.Live/Logging/LoggingExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,11 @@ public static partial class MultiModalLiveClientLoggingExtensions
public static partial void LogFunctionCall(this ILogger logger, string functionName);

/// <summary>
/// Logs an error message indicating that the WebSocket connection was closed due to an invalid payload.
/// Logs an error message indicating that the WebSocket connection was closed with a status code and description.
/// </summary>
/// <param name="logger">The logger to log the message to.</param>
/// <param name="closeStatus">Indicates the reason why the remote endpoint initiated the close handshake.</param>
/// <param name="closeStatusDescription">The description of the close status that caused the connection to close.</param>
[LoggerMessage(EventId = 113, Level = LogLevel.Error, Message = "WebSocket connection closed caused by invalid payload: {CloseStatusDescription}")]
public static partial void LogConnectionClosedWithInvalidPyload(this ILogger logger, string closeStatusDescription);
[LoggerMessage(EventId = 114, Level = LogLevel.Error, Message = "WebSocket connection closed with status {CloseStatus}: {CloseStatusDescription}")]
public static partial void LogConnectionClosedWithStatus(this ILogger logger, WebSocketCloseStatus? closeStatus, string closeStatusDescription);
}
23 changes: 11 additions & 12 deletions src/GenerativeAI.Live/Models/MultiModalLiveClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,26 +551,25 @@ public async Task ConnectAsync(bool autoSendSetup = true,CancellationToken cance

_client.DisconnectionHappened.Subscribe(info =>
{
if (info.Type == DisconnectionType.Error)
if (info.Exception is not null)
{
_logger?.LogConnectionClosedWithError(info.Type, info.Exception!);
ErrorOccurred?.Invoke(this, new ErrorEventArgs(info.Exception!));
ErrorOccurred?.Invoke(this, new ErrorEventArgs(info.Exception!));
}
else if (info.CloseStatus == WebSocketCloseStatus.InvalidPayloadData)
{
//log info.CloseStatusDescription
_logger?.LogConnectionClosedWithInvalidPyload(info.CloseStatusDescription!);
}
else if (info.CloseStatus == WebSocketCloseStatus.InternalServerError && !string.IsNullOrEmpty(info.CloseStatusDescription))
{
_logger?.LogConnectionClosedWithError(info.Type, info.Exception!);
Disconnected?.Invoke(this, new ErrorMessageEventArgs(info.CloseStatusDescription));

if (!string.IsNullOrEmpty(info.CloseStatusDescription))
{
_logger?.LogConnectionClosedWithStatus(info.CloseStatus, info.CloseStatusDescription);
}
else
{
_logger?.LogConnectionClosed();
Disconnected?.Invoke(this, EventArgs.Empty);
}

var eventArgs = !string.IsNullOrEmpty(info.CloseStatusDescription) ?
new ErrorMessageEventArgs(info.CloseStatusDescription) :
EventArgs.Empty;
Disconnected?.Invoke(this, eventArgs);
});
Comment on lines +554 to 573
Copy link

@coderabbitai coderabbitai bot Oct 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Normal closure with a description is logged as Error; also skip-status when description is null.

  • Any NormalClosure that includes a description (e.g., client Stop("Client Disconnecting")) will be logged via LogConnectionClosedWithStatus at Error level. This skews telemetry for expected disconnects.
  • If a status exists but CloseStatusDescription is null/empty, you fall back to LogConnectionClosed and lose the status detail, contrary to the PR goal of “always report status/description when present.”
  • Consider emitting Disconnected args that include both status and description (string) for now.

Proposed fix (choose severity by status, always include status when available, and enrich Disconnected args):

 _client.DisconnectionHappened.Subscribe(info =>
 {
     if (info.Exception is not null)
     {
         _logger?.LogConnectionClosedWithError(info.Type, info.Exception!);
         ErrorOccurred?.Invoke(this, new ErrorEventArgs(info.Exception!));
     }
-
-    if (!string.IsNullOrEmpty(info.CloseStatusDescription))
-    { 
-        _logger?.LogConnectionClosedWithStatus(info.CloseStatus, info.CloseStatusDescription);
-    }
-    else
-    {
-        _logger?.LogConnectionClosed();
-    }
-
-    var eventArgs = !string.IsNullOrEmpty(info.CloseStatusDescription) ?
-        new ErrorMessageEventArgs(info.CloseStatusDescription) :
-        EventArgs.Empty;
-    Disconnected?.Invoke(this, eventArgs);
+    var hasStatus = info.CloseStatus.HasValue;
+    var hasDescription = !string.IsNullOrEmpty(info.CloseStatusDescription);
+    var description = hasDescription ? info.CloseStatusDescription! : "(no description)";
+
+    if (hasStatus || hasDescription)
+    {
+        if (info.CloseStatus == WebSocketCloseStatus.NormalClosure)
+            _logger?.LogConnectionClosedWithStatusInfo(info.CloseStatus, description);
+        else
+            _logger?.LogConnectionClosedWithStatus(info.CloseStatus, description);
+    }
+    else
+    {
+        _logger?.LogConnectionClosed();
+    }
+
+    var disconnectedArgs = (hasStatus || hasDescription)
+        ? new ErrorMessageEventArgs(hasStatus ? $"{info.CloseStatus}: {description}" : description)
+        : EventArgs.Empty;
+    Disconnected?.Invoke(this, disconnectedArgs);
 });

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In src/GenerativeAI.Live/Models/MultiModalLiveClient.cs around lines 554 to 573,
the current logic logs NormalClosure with a description as an error, drops
CloseStatus when Description is null/empty (calling LogConnectionClosed), and
raises Disconnected with only a description; change it to (1) map log level to
severity based on CloseStatus (treat NormalClosure as Information, others as
Error/Warning as appropriate) and call LogConnectionClosedWithStatus whenever
CloseStatus is present regardless of description, (2) avoid using
LogConnectionClosedWithError for expected NormalClosure with description, and
(3) create and raise a richer Disconnected event args type that includes both
CloseStatus (nullable) and CloseStatusDescription (string) and pass that instead
of EventArgs.Empty so consumers always get status+description when available.

Copy link
Contributor Author

@ladenedge ladenedge Oct 20, 2025

Choose a reason for hiding this comment

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

This is not how my tests behave, but it's probably a good code review. I'll see if I can adjust the handler a bit to account for these comments.

Copy link

@coderabbitai coderabbitai bot Oct 20, 2025

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the LogConnectionClosedWithStatus method definition to verify log level
cat src/GenerativeAI.Live/Logging/LoggingExtensions.cs

Length of output: 8067

@ladenedge Thank you for taking another look at this!

I've confirmed that LogConnectionClosedWithStatus is indeed defined with LogLevel.Error (EventId 114) in the LoggingExtensions.cs file, so when CloseStatusDescription is present, it will log as an error regardless of whether it's a NormalClosure or not. The fallback LogConnectionClosed() method correctly uses LogLevel.Information.

Your tests might not be hitting the path where a description is present with NormalClosure, which would explain the different behavior you're seeing.

If you'd like, you could consider:

  1. Adding a separate logging method like LogConnectionClosedWithStatusInfo at Information level for normal closures
  2. Or parameterizing the log level based on the CloseStatus value within the handler

Either way, I appreciate your openness to the feedback! Let me know if you'd like any help thinking through the adjustments. 🐰

🧬 Code Graph Analysis Results

<src/GenerativeAI.Live/Extensions/WebSocketClientExtensions.cs> (lines 17-26)

public static IWebsocketClient WithReconnect(this ClientWebSocket webSocketClient, string url)
{
    var client = new WebsocketClient(new Uri(url), () => webSocketClient)
    {
        IsReconnectionEnabled = true,
        ReconnectTimeout = TimeSpan.FromSeconds(30)
    };

    return client;
}

<src/GenerativeAI.Live/Events/MessageReceivedEventArgs.cs> (lines 8-23)

public class MessageReceivedEventArgs : EventArgs
{
    /// <summary>
    /// Gets the payload of the received message.
    /// </summary>
    public BidiResponsePayload Payload { get; }

    /// <summary>
    /// Initializes a new instance of the MessageReceivedEventArgs class.
    /// </summary>
    /// <param name="payload">The payload of the received message.</param>
    public MessageReceivedEventArgs(BidiResponsePayload payload)
    {
        Payload = payload;
    }
}

<src/GenerativeAI.Live/Events/AudioBufferReceivedEventArgs.cs> (lines 8-40)

public class AudioBufferReceivedEventArgs : EventArgs
{
    /// <summary>
    /// Gets or sets the audio data buffer.
    /// </summary>
    public byte[] Buffer { get; set; }

    /// <summary>
    /// Gets or sets the header information for the audio data.
    /// </summary>
    public AudioHeaderInfo HeaderInfo { get; set; }
    
    /// <summary>
    /// Gets or sets the transcription of the input audio.
    /// </summary>
    public Transcription? InputTranscription { get; set; }
    
    /// <summary>
    /// Gets or sets the transcription of the output audio.
    /// </summary>
    public Transcription? OutputTranscription { get; set; }

    /// <summary>
    /// Initializes a new instance of the AudioBufferReceivedEventArgs class.
    /// </summary>
    /// <param name="buffer">The audio buffer data.</param>
    /// <param name="audioHeaderInfo">The audio header information.</param>
    public AudioBufferReceivedEventArgs(byte[] buffer, AudioHeaderInfo audioHeaderInfo)
    {
        this.Buffer = buffer;
        HeaderInfo = audioHeaderInfo;
    }
}

<src/GenerativeAI.Live/Events/ErrorMessageEventArgs.cs> (lines 6-20)

public class ErrorMessageEventArgs : EventArgs
{
    /// <summary>
    /// Gets the payload of the received message.
    /// </summary>
    public string ErrorMessage { get; }

    /// <summary>
    /// Initializes a new instance of the class.
    /// </summary>
    public ErrorMessageEventArgs(string errorMessage)
    {
        ErrorMessage = errorMessage;
    }
}

<src/GenerativeAI.Types/MultimodalLive/BidiClientPayload.cs> (lines 10-35)

public class BidiClientPayload
{
    /// <summary>
    /// Gets or sets the setup message for a bidirectional content generation session.
    /// </summary>
    [JsonPropertyName("setup")]
    public BidiGenerateContentSetup? Setup { get; set; }

    /// <summary>
    /// Gets or sets an incremental update of the current conversation delivered from the client.
    /// </summary>
    [JsonPropertyName("clientContent")]
    public BidiGenerateContentClientContent? ClientContent { get; set; }

    /// <summary>
    /// Gets or sets user input that is sent in real time.
    /// </summary>
    [JsonPropertyName("realtimeInput")]
    public BidiGenerateContentRealtimeInput? RealtimeInput { get; set; }

    /// <summary>
    /// Gets or sets a client generated response to a <see cref="FunctionCall"/> received from the server.
    /// </summary>
    [JsonPropertyName("toolResponse")]
    public BidiGenerateContentToolResponse? ToolResponse { get; set; }
}

<src/GenerativeAI.Types/MultimodalLive/BidiGenerateContentSetup.cs> (lines 10-76)

public class BidiGenerateContentSetup
{
    /// <summary>
    /// Required. The model's resource name. This serves as an ID for the Model to use.
    /// Format: <c>models/{model}</c>
    /// </summary>
    [JsonPropertyName("model")]
    public string? Model { get; set; }

    /// <summary>
    /// Generation config.
    /// The following fields are not supported:
    /// - <c>responseLogprobs</c>
    /// - <c>responseMimeType</c>
    /// - <c>logprobs</c>
    /// - <c>responseSchema</c>
    /// - <c>stopSequence</c>
    /// - <c>routingConfig</c>
    /// - <c>audioTimestamp</c>
    /// </summary>
    [JsonPropertyName("generationConfig")]
    public GenerationConfig? GenerationConfig { get; set; }

    /// <summary>
    /// The user provided system instructions for the model.
    /// Note: Only text should be used in parts. Content in each part will be in a separate paragraph.
    /// </summary>
    [JsonPropertyName("systemInstruction")]
    public Content? SystemInstruction { get; set; }

    /// <summary>
    /// A list of <see cref="Tool"/> the model may use to generate the next response.
    /// A <see cref="Tool"/> is a piece of code that enables the system to interact with external systems to perform an action, or set of actions, outside of knowledge and scope of the model.
    /// </summary>
    [JsonPropertyName("tools")]
    public Tool[]? Tools { get; set; }

    /// <summary>
    /// Configures output audio transcription settings.
    /// </summary>
    [JsonPropertyName("outputAudioTranscription")]
    public AudioTranscriptionConfig? OutputAudioTranscription { get; set; }

    /// <summary>
    /// Configures input audio transcription settings.
    /// </summary>
    [JsonPropertyName("inputAudioTranscription")]
    public AudioTranscriptionConfig? InputAudioTranscription { get; set; }
    /// <summary>
    /// Configures context window compression mechanism. If included, server will compress context window to fit into given length.
    /// </summary>
    [JsonPropertyName("contextWindowCompression")]
    public ContextWindowCompressionConfig? ContextWindowCompression { get; set; }
    
    /// <summary>
    /// Configures the proactivity of the model. This allows the model to respond proactively to the input and to ignore irrelevant input.
    /// </summary>
    [JsonPropertyName("proactivity")]
    public ProactivityConfig? Proactivity { get; set; }

    /// <summary>
    /// Configures session resumption mechanism. If included server will send SessionResumptionUpdate messages.
    /// </summary>
    [JsonPropertyName("sessionResumption")]
    public SessionResumptionConfig? SessionResumption { get; set; }

}

<src/GenerativeAI.Types/MultimodalLive/BidiGenerateContentToolCall.cs> (lines 9-16)

public class BidiGenerateContentToolCall
{
    /// <summary>
    /// Output only. The function call to be executed.
    /// </summary>
    [JsonPropertyName("functionCalls")]
    public FunctionCall[]? FunctionCalls { get; set; }
}

<src/GenerativeAI.Types/MultimodalLive/BidiGenerateContentToolResponse.cs> (lines 10-19)

public class 
    
    BidiGenerateContentToolResponse
{
    /// <summary>
    /// The response to the function calls.
    /// </summary>
    [JsonPropertyName("functionResponses")]
    public FunctionResponse[]? FunctionResponses { get; set; }
}

<src/GenerativeAI.Types/MultimodalLive/BidiGenerateContentRealtimeInput.cs> (lines 15-22)

public class BidiGenerateContentRealtimeInput
{
    /// <summary>
    /// Inlined bytes data for media input.
    /// </summary>
    [JsonPropertyName("mediaChunks")]
    public Blob[]? MediaChunks { get; set; }
}

<src/GenerativeAI.Types/ContentGeneration/Tools/Tool.cs> (lines 11-49)

public class Tool
{
    /// <summary>
    /// Optional. A list of FunctionDeclarations available to the model that can be used for function calling.
    /// The model or system does not execute the function. Instead the defined function may be returned
    /// as a <see cref="Part.FunctionCall"/> with arguments to the client side for execution.
    /// The model may decide to call a subset of these functions by populating
    /// <see cref="Part.FunctionCall"/> in the response. The next conversation turn may contain
    /// a <see cref="Part.FunctionResponse"/> with the <see cref="Content.Role"/> "function"
    /// generation context for the next model turn.
    /// </summary>
    [JsonPropertyName("functionDeclarations")]
    public List<FunctionDeclaration>? FunctionDeclarations { get; set; }

    /// <summary>
    /// Optional. Retrieval tool that is powered by Google search.
    /// </summary>
    [JsonPropertyName("googleSearchRetrieval")]
    public GoogleSearchRetrievalTool? GoogleSearchRetrieval { get; set; }

    /// <summary>
    /// Optional. Enables the model to execute code as part of generation.
    /// </summary>
    [JsonPropertyName("codeExecution")]
    public CodeExecutionTool? CodeExecution { get; set; }

    /// <summary>
    /// Optional. GoogleSearch tool type. Tool to support Google Search in Model. Powered by Google.
    /// </summary>
    [JsonPropertyName("googleSearch")]
    public GoogleSearchTool? GoogleSearch { get; set; }
    
    /// <summary>
    /// Optional. Retrieval tool type. System will always execute the provided retrieval tool(s) to get external knowledge to answer the prompt. Retrieval results are presented to the model for generation.
    /// </summary>
    [JsonPropertyName("retrieval")]
    public VertexRetrievalTool? Retrieval { get; set; }
    
}

<src/GenerativeAI.Live/Events/TextChunkReceivedArgs.cs> (lines 9-31)

public class TextChunkReceivedArgs : EventArgs
{
    /// <summary>
    /// Gets or sets the text of the received chunk.
    /// </summary>
    public string Text { get; set; }

    /// <summary>
    /// Gets or sets a value indicating whether the turn is finished.
    /// </summary>
    public bool IsTurnFinish { get; set; }

    /// <summary>
    /// Initializes a new instance of the TextChunkReceivedArgs class.
    /// </summary>
    /// <param name="text">The text of the received chunk.</param>
    /// <param name="isTurnFinish">A value indicating whether the turn is finished.</param>
    public TextChunkReceivedArgs(string text, bool isTurnFinish)
    {
        this.Text = text;
        this.IsTurnFinish = isTurnFinish;
    }
}

<src/GenerativeAI.Live/Helper/AudioHelper.cs> (lines 9-130)

public static class AudioHelper
{
    /// <summary>
    /// Adds a WAV file header to the given raw audio data.
    /// </summary>
    public static byte[] AddWaveHeader(byte[] audioData, int numberOfChannels, int sampleRate, int bitsPerSample2)
    {
#if NET6_0_OR_GREATER
        ArgumentNullException.ThrowIfNull(audioData);
#else
        if (audioData == null)
            throw new ArgumentNullException(nameof(audioData));
#endif
        // ... constructs WAV header and returns combined buffer ...
    }

    /// <summary>
    /// Validates whether the given byte array contains a valid WAV file header.
    /// </summary>
    public static bool IsValidWaveHeader(byte[] buffer)
    {
        if (buffer == null || buffer.Length < 44) // Minimum WAV header size
        {
            return false;
        }

        using (var stream = new MemoryStream(buffer))
        using (var reader = new BinaryReader(stream))
        {
            try
            {
                // RIFF, fmt, data checks...
                // returns true/false accordingly
            }
            catch (Exception)
            {
                return false;
            }
        }
    }
}

<src/GenerativeAI/Constants/Roles.cs> (lines 6-28)

public static class Roles
{
    /// <summary>
    /// Represents the role of a user interacting with the system.
    /// </summary>
    public const string User = "user";

    /// <summary>
    /// Represents the role assigned to the AI model in the system.
    /// </summary>

    public const string Model = "model";

    /// <summary>
    /// Represents the role for functions invoked during the system's operation.
    /// </summary>
    public const string Function = "function";

    /// <summary>
    /// Represents the system's internal role for handling instructions or operations.
    /// </summary>
    public const string System = "system";
}

<src/GenerativeAI/Types/MultimodalLive/BidiGenerateContentServerContent.cs> (lines 63-76)

public class Transcription
{
    /// <summary>
    /// The bool indicates the end of the transcription.
    /// </summary>
    [JsonPropertyName("finished")]
    public bool? Finished { get; set; }

    /// <summary>
    /// Transcription text.
    /// </summary>
    [JsonPropertyName("text")]
    public string? Text { get; set; }
}

<src/GenerativeAI/Types/MultimodalLive/LiveServerSessionResumptionUpdate.cs> (lines 8-29)

public class LiveServerSessionResumptionUpdate
{
    /// <summary>
    /// A token that can be used by the client to resume the session.
    /// This token encapsulates the state of the session on the server side.
    /// Optional: This field might be present if the server successfully captured a resumption point.
    /// </summary>
    [JsonPropertyName("resumptionToken")]
    public string? ResumptionToken { get; set; }

    /// <summary>
    /// Optional. A message from the server regarding the session resumption status (e.g., success, error, pending).
    /// </summary>
    [JsonPropertyName("message")]
    public string? Message { get; set; }

    /// <summary>
    /// Optional. Indicates the status of the session resumption process.
    /// </summary>
    [JsonPropertyName("status")]
    public SessionResumptionStatus? Status { get; set; }
}

<src/GenerativeAI/Types/MultimodalLive/BidiGenerateContentServerContent.cs> (not listed in snippets but contextually related)


try
Expand Down
Loading