Fix initization race (fixes #637) #1029
Conversation
DTTerastar
had a problem deploying
to
CI - release environment
GitHub Actions
Failure
📝 WalkthroughWalkthroughThe pull request introduces a new file containing deep clone implementations for multiple configuration classes and updates existing configuration class declarations to use partial classes. It removes obsolete MQTT record types and enhances MQTT coordination by integrating a new Supervisor configuration loader service. The MQTT coordinator now accepts an additional dependency and includes thread safety, configuration change detection, and improved logging. Additionally, a new service, SupervisorConfigLoader, is added to asynchronously fetch and cache MQTT configuration from the Supervisor API. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Loader as SupervisorConfigLoader
participant API as Supervisor API
Caller->>Loader: GetSupervisorConfig()
alt SUPERVISOR_TOKEN Missing
Loader-->>Caller: null
else Token Exists
Loader->>Loader: Check Cached Config
alt Config Cached
Loader-->>Caller: Cached ConfigMqtt
else No Cached Config
Loader->>Loader: Acquire Semaphore Lock
Loader->>Loader: Start LoadConfigAsync()
Loader->>API: HTTP Request with Token
API-->>Loader: JSON Response
Loader->>Loader: Deserialize & Cache ConfigMqtt
Loader-->>Caller: New ConfigMqtt
end
end
sequenceDiagram
participant Req as MQTT Client Requestor
participant Coord as MqttCoordinator
participant Loader as SupervisorConfigLoader
Req->>Coord: GetClient()
Coord->>Coord: Check _reconnectRequired & Acquire Lock
alt Reconnection Needed
Coord->>Coord: Cleanup Existing Client
end
Coord->>Loader: GetSupervisorConfig()
Loader-->>Coord: ConfigMqtt (or null)
Coord->>Coord: InitializeClientAsync() [Setup MQTT Client]
Coord-->>Req: Return MQTT Client
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
DTTerastar
had a problem deploying
to
CI - release environment
GitHub Actions
Failure
DTTerastar
had a problem deploying
to
CI - release environment
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/Services/MqttCoordinator.cs (1)
392-414: 💡 Verification agent🧩 Analysis chain
Fix async method warning in ProcessDiscoveryMessage.
The pipeline reports that this method lacks 'await' operators and will run synchronously.
🏁 Script executed:
#!/bin/bash # Check if there are any awaited operations in the method grep -A 25 "ProcessDiscoveryMessage" src/Services/MqttCoordinator.cs | grep "await"Length of output: 309
Refactor ProcessDiscoveryMessage to address async warning
The method currently runs synchronously (no awaits within its body), yet it’s marked async. Since the await operators shown in your grep output come from the call sites rather than inside this method, please either remove the async keyword and return a completed Task (e.g., using Task.CompletedTask) or, if asynchronous behavior is truly needed, insert an awaitable operation.
- File:
src/Services/MqttCoordinator.cs(lines 392–414)- Suggestion: Remove
asyncfrom the signature and change the method’s return accordingly if there are no asynchronous operations, or include an await operation if intended.🧰 Tools
🪛 GitHub Actions: Build and test
[warning] 401-401: CS8625: Cannot convert null literal to non-nullable reference type.
[warning] 408-408: CS8601: Possible null reference assignment.
[warning] 392-392: CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
🧹 Nitpick comments (1)
src/Services/MqttCoordinator.cs (1)
401-401: Address nullable reference warnings.The pipeline reports nullable reference warnings on lines 401 and 408:
- Line 401: CS8625: Cannot convert null literal to non-nullable reference type
- Line 408: CS8601: Possible null reference assignment
These should be addressed to ensure type safety.
Also applies to: 408-408
🧰 Tools
🪛 GitHub Actions: Build and test
[warning] 401-401: CS8625: Cannot convert null literal to non-nullable reference type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Services/MqttCoordinator.cs(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build and test
src/Services/MqttCoordinator.cs
[warning] 401-401: CS8625: Cannot convert null literal to non-nullable reference type.
[warning] 408-408: CS8601: Possible null reference assignment.
[warning] 392-392: CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Deploy to Docker add-on with tag
- GitHub Check: Analyze (csharp)
🔇 Additional comments (7)
src/Services/MqttCoordinator.cs (7)
32-37: Well-structured thread safety and configuration change detection implementation.The new fields provide a solid foundation for addressing thread safety and configuration management:
_supervisorConfigLoaderfor loading supervisor configurations_initLockto ensure thread-safe client initialization_initTaskto avoid multiple concurrent initializations_lastConfigfor config comparison_reconnectRequiredflag to trigger reconnection when neededThese changes effectively address the initialization race condition mentioned in the PR title (fixes #637).
39-55: Good implementation of configuration change detection.The constructor now properly:
- Accepts the new
SupervisorConfigLoaderdependency- Subscribes to configuration changes
- Logs when MQTT configuration changes
- Sets the reconnection flag when necessary
This approach allows for dynamic handling of configuration changes without requiring application restart.
57-66: Thorough configuration comparison.The
ConfigChangedmethod properly checks if MQTT connection properties have changed by:
- Handling null configurations correctly
- Using case-insensitive comparison for host names
- Comparing all relevant connection properties (host, port, credentials, SSL)
This ensures that reconnection happens only when necessary.
68-75: Clear handling of incomplete configuration.The method appropriately returns null when the host is not specified, indicating that a valid MQTT configuration is unavailable and allowing the code to fall back to supervisor config.
77-119: Excellent thread-safe client access implementation.The
GetClientmethod:
- Uses a proper double-check locking pattern for efficiency
- Ensures thread safety with
SemaphoreSlim- Properly cleans up resources during reconnection
- Caches initialization tasks to prevent duplicate work
- Guarantees semaphore release with a
finallyblockThis implementation effectively prevents race conditions during client initialization.
121-188: Robust client initialization with fallback configuration.The initialization process successfully:
- Tries user config first, then falls back to supervisor config
- Properly configures the MQTT client with appropriate options
- Sets up event handlers for connection status
- Stores a clone of the configuration for future comparison
This provides a flexible approach to configuration management.
261-282: Enhanced message publishing with proper error handling.The
EnqueueAsyncmethod has been improved with:
- Thread-safe client access via
GetClient()- Explicit error handling with informative logging
- Sanitized topic names in logging statements to prevent log injection
These changes enhance the reliability and security of message publishing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Services/MqttCoordinator.cs (1)
263-276: Good error handling, but topic sanitization could be improved.The error handling is now robust, but the topic sanitization logic appears in two places (lines 273-274 and line 280). Consider extracting this to a helper method to avoid duplication.
- var sanitizedTopic = topic.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", ""); - _logger.LogError(ex, "Failed to enqueue MQTT message to {Topic}", sanitizedTopic); + _logger.LogError(ex, "Failed to enqueue MQTT message to {Topic}", SanitizeTopic(topic));And add a private helper method:
private string SanitizeTopic(string topic) { return topic.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", ""); }Then use this in line 280 as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Services/MqttCoordinator.cs(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Deploy to Docker
src/Services/MqttCoordinator.cs
[warning] 402-402: Cannot convert null literal to non-nullable reference type.
[warning] 409-409: Possible null reference assignment.
[warning] 393-393: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
🪛 GitHub Actions: Build and test
src/Services/MqttCoordinator.cs
[warning] 402-402: CS8625: Cannot convert null literal to non-nullable reference type.
[warning] 409-409: CS8601: Possible null reference assignment.
[warning] 393-393: CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
🔇 Additional comments (7)
src/Services/MqttCoordinator.cs (7)
32-37: Good addition of thread safety controls and state tracking.The new fields for controlling thread safety (
_initLock,_initTask) and tracking configuration state (_lastConfig,_reconnectRequired) are well-designed to address the initialization race condition mentioned in the PR title.
39-55: Well-implemented configuration change detection.The updated constructor now correctly handles the new dependency and sets up a configuration change listener that detects changes and flags when reconnection is needed. This is a good pattern for reactive configuration updates.
57-66: Comprehensive configuration comparison logic.The
ConfigChangedmethod provides a thorough comparison of all connection-relevant MQTT configuration properties. This ensures that the client reconnects whenever any connection parameter changes.
77-119: Excellent thread-safe client initialization implementation.The refactored
GetClientmethod properly addresses the initialization race condition with:
- SemaphoreSlim for thread synchronization
- Double-checking pattern to avoid unnecessary locking
- Proper cleanup of existing clients during reconnection
- Task caching to prevent duplicate initialization
This is a robust implementation of the thread-safe singleton pattern.
121-188: Well-structured client initialization with proper configuration fallback.The
InitializeClientAsyncmethod:
- First tries to use user configuration, then falls back to supervisor configuration
- Properly clones the config for comparison later
- Uses appropriate logging for connection status
This separation of concerns improves code organization and maintainability.
177-180: Be careful with logging sensitive information.While you've improved the logging implementation, consider masking the password if it's ever included in logs. Currently, you're only logging the username (or ""), which is appropriate.
393-415: Address pipeline warnings in ProcessDiscoveryMessage.The pipeline reports three warnings for this method:
- Line 402: "Cannot convert null literal to non-nullable reference type"
- Line 409: "Possible null reference assignment"
- Line 393: "This async method lacks 'await' operators"
While not directly related to your changes, consider fixing these issues to clean up the build warnings.
🧰 Tools
🪛 GitHub Actions: Deploy to Docker
[warning] 402-402: Cannot convert null literal to non-nullable reference type.
[warning] 409-409: Possible null reference assignment.
[warning] 393-393: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
🪛 GitHub Actions: Build and test
[warning] 402-402: CS8625: Cannot convert null literal to non-nullable reference type.
[warning] 409-409: CS8601: Possible null reference assignment.
[warning] 393-393: CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.
DTTerastar
temporarily deployed
to
CI - release environment
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Services/SupervisorConfigLoader.cs (1)
86-89: Consider adding a timeout for the API request.The HTTP request to the Supervisor API doesn't specify a timeout, which could cause the application to hang if the API is unresponsive.
- var (_, _, data) = await "http://supervisor/services/mqtt" - .WithOAuthBearerToken(token) - .GetJsonAsync<HassIoResult>(); + var (_, _, data) = await "http://supervisor/services/mqtt" + .WithOAuthBearerToken(token) + .WithTimeout(TimeSpan.FromSeconds(10)) + .GetJsonAsync<HassIoResult>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Models/Config.Clone.cs(1 hunks)src/Models/Config.cs(13 hunks)src/Models/HassIoMqtt.cs(0 hunks)src/Program.cs(1 hunks)src/Services/MqttCoordinator.cs(3 hunks)src/Services/SupervisorConfigLoader.cs(1 hunks)tests/DeviceTrackerTests.cs(1 hunks)
💤 Files with no reviewable changes (1)
- src/Models/HassIoMqtt.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Program.cs
- src/Models/Config.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Deploy to Docker add-on with tag
- GitHub Check: Analyze (csharp)
🔇 Additional comments (17)
tests/DeviceTrackerTests.cs (1)
32-32: LGTM: Test updated to accommodate new constructor parameter.The test has been correctly updated to match the new
MqttCoordinatorconstructor signature which now requires aSupervisorConfigLoaderparameter. Passing null is appropriate for this test context.src/Services/MqttCoordinator.cs (7)
32-38: Good: Added thread safety mechanisms and tracking fields.Adding the
SupervisorConfigLoaderfield along with thread synchronization primitives (SemaphoreSlimand task tracking) is a good approach to fix the initialization race condition mentioned in the PR title.
39-55: Good implementation of configuration change tracking.The updated constructor now properly initializes the SupervisorConfigLoader dependency and subscribes to configuration changes. The subscription uses a separate method to check for relevant changes and sets the reconnect flag accordingly, which helps maintain clean code structure.
57-66: Well-designed configuration comparison method.This method properly compares the relevant fields of the MQTT configuration to determine if a reconnection is needed. The null-handling is also correct.
68-75: Good separation of concerns in configuration retrieval.Extracting the user configuration retrieval to a separate method improves code organization. The null check on the host makes sense as a validation mechanism.
77-119: Great implementation of thread-safe client retrieval.The
GetClientmethod is well-implemented with:
- Fast path for common case (client exists and no reconnect needed)
- Thread safety using semaphore
- Double-check after lock acquisition
- Proper cleanup of existing client when reconnecting
- Smart handling of concurrent initialization requests
This will effectively resolve race conditions during initialization.
121-188: Well-structured client initialization with fallback mechanism.The
InitializeClientAsyncmethod:
- Attempts to get user config first, then falls back to supervisor config
- Throws a clear exception if no configuration is available
- Properly builds and starts the client
- Caches the configuration for change detection
- Handles various MQTT client events with appropriate logging
This graceful fallback approach ensures the system can work in different environments.
261-277: Improved error handling and thread safety in EnqueueAsync.The changes to EnqueueAsync properly utilize the new GetClient method and add robust error handling with sanitized logging, preventing potential log injection issues.
src/Services/SupervisorConfigLoader.cs (5)
11-16: Good use of record types for API response modeling.Using C# records for the API response structure is a clean approach that provides immutability and concise syntax.
19-27: Well-structured data model for MQTT configuration.The HassIoMqtt record properly maps all necessary fields from the Supervisor API with appropriate JSON property names.
29-37: Good initialization of fields in constructor.The class properly initializes the logger and necessary synchronization primitives.
39-80: Excellent thread-safe implementation of configuration retrieval.The
GetSupervisorConfigmethod:
- Handles the case where the environment variable isn't set (fast return path)
- Uses caching for performance
- Implements proper thread safety with semaphore
- Avoids duplicate work by reusing in-progress tasks
- Ensures the semaphore is always released with try/finally
This implementation efficiently handles concurrent access scenarios.
82-111: Robust error handling in LoadConfigAsync.The method:
- Uses proper logging at appropriate log levels
- Has a clean HTTP request using Flurl
- Handles default values and parsing for configuration
- Properly catches and logs exceptions
- Caches successful results
The error handling allows the system to gracefully degrade if the Supervisor API is unavailable.
src/Models/Config.Clone.cs (4)
9-30: Good implementation of deep cloning for Config class.The
Clonemethod properly creates a new instance and handles all nested objects by calling their respective Clone methods. The handling of array properties with LINQ expressions is also correct.
32-43: LGTM: ConfigLocators clone implementation.The implementation correctly handles all nested objects.
151-165: Good implementation of ConfigMqtt cloning.This Clone method is particularly important for the race condition fix as it's used in
MqttCoordinatorto create an immutable snapshot of the configuration for change detection.
1-221: Comprehensive implementation of the cloning pattern.This file provides a complete set of Clone methods for all configuration classes, ensuring proper deep copying behavior. Using partial classes is a good approach to separate this functionality from the main class definitions.
Some suggestions for future improvements (not critical for this PR):
- Consider using a generic cloning pattern or AutoMapper to reduce boilerplate
- Add unit tests to verify the deep cloning behavior
New Features
Refactor