fix(build): pin patched Scriban.Signed 7.2.1 + SonarCloud code-quality sweep#90
Conversation
…nce 10.7.0 Reapplies the dependency bumps from Dependabot PR #67, which went stale after the surrounding dependency PRs (#64/#68/#69/#70) merged and could no longer be rebased automatically — @dependabot rebase/recreate stopped responding. This re-creates the identical version set on a fresh branch off current main: Microsoft.Extensions.* 10.0.8 -> 10.0.9 (12 packages) Microsoft.Extensions.Http.Resilience 10.6.0 -> 10.7.0 Intentionally-older sample pins (10.0.0 / 10.0.4) are left untouched, matching #67's scope. Supersedes #67. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change updates repository dependencies and test build settings, refactors event dispatch, startup validation, MQTT reconnection, NDJSON migration parsing, sample output, and template logging, and adds dispatch-count and MQTT robustness assertions. ChangesApplication and build updates
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant EventPump
participant ChannelActionDispatcher
participant Validator
participant ActionPlugin
EventPump->>ChannelActionDispatcher: Enqueue matched action
ChannelActionDispatcher->>Validator: Run configured validators
Validator-->>ChannelActionDispatcher: Accept or reject
ChannelActionDispatcher->>ActionPlugin: Execute accepted action
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tools/FrigateRelay.MigrateConf/Reconciler.cs (1)
52-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant null-forgiving operator.
row = default;preserves the current false-path initialization and resolves the reported Sonar finding.Proposed fix
- row = default!; + row = default;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/FrigateRelay.MigrateConf/Reconciler.cs` at line 52, In the false-path initialization near Reconciler’s row assignment, remove the redundant null-forgiving operator by changing the default assignment to use plain default. Preserve the existing initialization behavior.Source: Linters/SAST tools
src/FrigateRelay.Host/StartupValidation.cs (1)
57-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant nullable-only casts/operators (nullable disabled in both files). SonarCloud flags several
(string?)casts and null-forgiving!operators as unnecessary since nullable warnings are disabled here, so removing them is purely cosmetic cleanup with no behavior change.
src/FrigateRelay.Host/StartupValidation.cs#L57-L64: drop the(string?)casts on lines 58, 59, 60, 64 and the!one.Validators!(line 63).src/FrigateRelay.Sources.FrigateMqtt/EventContextProjector.cs#L55-L55: drop the!incontext = default!;.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/FrigateRelay.Host/StartupValidation.cs` around lines 57 - 64, Remove the redundant nullable-only syntax: in src/FrigateRelay.Host/StartupValidation.cs lines 57-64, remove the four string nullable casts and the null-forgiving operator on e.Validators; in src/FrigateRelay.Sources.FrigateMqtt/EventContextProjector.cs line 55, remove the null-forgiving operator from the default assignment. Preserve behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/check-doc-samples.sh:
- Around line 21-26: Update the error handling in check-doc-samples.sh for the
missing document and samples directory so each GitHub ::error:: annotation is
written to stdout without redirection; if additional diagnostics are needed,
emit them separately to stderr while preserving the existing exit behavior.
In `@src/FrigateRelay.Sources.FrigateMqtt/FrigateMqttEventSource.cs`:
- Around line 163-190: Update EnsureConnectedAsync to track subscription health
separately from _client.IsConnected, using the existing connection-status state
or a dedicated field. Only short-circuit when both TCP connectivity and the
requested subscription are healthy; when ProcessSubscribeResult reports no
granted topics, retain an unhealthy subscription state so the next loop
reconnects and re-runs SubscribeAsync.
In `@tools/FrigateRelay.MigrateConf/Reconciler.cs`:
- Around line 63-84: Update the NDJSON parsing in the reconciliation flow around
the ActionRow construction to skip malformed records without throwing: validate
that `@mt`, `@t`, Camera, and Label are JSON strings before reading them, and use
non-throwing timestamp parsing for `@t`. Return false for non-string or invalid
values while preserving the existing action matching and valid-row creation
behavior.
---
Nitpick comments:
In `@src/FrigateRelay.Host/StartupValidation.cs`:
- Around line 57-64: Remove the redundant nullable-only syntax: in
src/FrigateRelay.Host/StartupValidation.cs lines 57-64, remove the four string
nullable casts and the null-forgiving operator on e.Validators; in
src/FrigateRelay.Sources.FrigateMqtt/EventContextProjector.cs line 55, remove
the null-forgiving operator from the default assignment. Preserve behavior.
In `@tools/FrigateRelay.MigrateConf/Reconciler.cs`:
- Line 52: In the false-path initialization near Reconciler’s row assignment,
remove the redundant null-forgiving operator by changing the default assignment
to use plain default. Preserve the existing initialization behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 072e9667-3d69-467c-b753-832e501384cb
📒 Files selected for processing (34)
.github/scripts/check-doc-samples.shsamples/FrigateRelay.Samples.PluginGuide/FrigateRelay.Samples.PluginGuide.csprojsamples/FrigateRelay.Samples.PluginGuide/Program.cssrc/FrigateRelay.Abstractions/FrigateRelay.Abstractions.csprojsrc/FrigateRelay.Host/Configuration/ActionEntryTypeConverter.cssrc/FrigateRelay.Host/Dispatch/ChannelActionDispatcher.cssrc/FrigateRelay.Host/EventPump.cssrc/FrigateRelay.Host/HostBootstrap.cssrc/FrigateRelay.Host/StartupValidation.cssrc/FrigateRelay.Plugins.BlueIris/FrigateRelay.Plugins.BlueIris.csprojsrc/FrigateRelay.Plugins.CodeProjectAi/FrigateRelay.Plugins.CodeProjectAi.csprojsrc/FrigateRelay.Plugins.Doods2/FrigateRelay.Plugins.Doods2.csprojsrc/FrigateRelay.Plugins.FrigateSnapshot/FrigateRelay.Plugins.FrigateSnapshot.csprojsrc/FrigateRelay.Plugins.Pushover/FrigateRelay.Plugins.Pushover.csprojsrc/FrigateRelay.Plugins.Pushover/PushoverActionPlugin.cssrc/FrigateRelay.Plugins.Roboflow/FrigateRelay.Plugins.Roboflow.csprojsrc/FrigateRelay.Sources.FrigateMqtt/EventContextProjector.cssrc/FrigateRelay.Sources.FrigateMqtt/FrigateMqttEventSource.cssrc/FrigateRelay.Sources.FrigateMqtt/FrigateRelay.Sources.FrigateMqtt.csprojtemplates/FrigateRelay.Plugins.Template/src/FrigateRelay.Plugins.Example/ExampleActionPlugin.cstests/Directory.Build.propstests/FrigateRelay.Host.Tests/EventPumpDispatchTests.cstests/FrigateRelay.Host.Tests/FrigateRelay.Host.Tests.csprojtests/FrigateRelay.MigrateConf.Tests/FrigateRelay.MigrateConf.Tests.csprojtests/FrigateRelay.Plugins.BlueIris.Tests/FrigateRelay.Plugins.BlueIris.Tests.csprojtests/FrigateRelay.Plugins.CodeProjectAi.Tests/FrigateRelay.Plugins.CodeProjectAi.Tests.csprojtests/FrigateRelay.Plugins.Doods2.Tests/FrigateRelay.Plugins.Doods2.Tests.csprojtests/FrigateRelay.Plugins.FrigateSnapshot.Tests/FrigateRelay.Plugins.FrigateSnapshot.Tests.csprojtests/FrigateRelay.Plugins.Pushover.Tests/FrigateRelay.Plugins.Pushover.Tests.csprojtests/FrigateRelay.Plugins.Roboflow.Tests/FrigateRelay.Plugins.Roboflow.Tests.csprojtests/FrigateRelay.Sources.FrigateMqtt.Tests/FrigateRelay.Sources.FrigateMqtt.Tests.csprojtests/FrigateRelay.TestHelpers/FrigateRelay.TestHelpers.csprojtools/FrigateRelay.MigrateConf/AppsettingsWriter.cstools/FrigateRelay.MigrateConf/Reconciler.cs
| echo "::error::Doc file not found: $DOC" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [[ ! -d "$SAMPLES_DIR" ]]; then | ||
| echo "::error::Samples directory not found: $SAMPLES_DIR" | ||
| echo "::error::Samples directory not found: $SAMPLES_DIR" >&2 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching check-doc-samples.sh:"
fd -a 'check-doc-samples\.sh$' . || true
echo
if [ -f .github/scripts/check-doc-samples.sh ]; then
echo "Relevant lines from .github/scripts/check-doc-samples.sh:"
nl -ba .github/scripts/check-doc-samples.sh | sed -n '1,50p'
fi
echo
echo "Search for nearby workflow command redirects in repo:"
rg -n 'echo "::error|'>&2|::group|::' .github || trueRepository: blehnen/FrigateRelay
Length of output: 295
🌐 Web query:
GitHub Actions workflow commands commands documentation stdout echo ::error
💡 Result:
GitHub Actions uses specific workflow commands that you can trigger by echoing a formatted string to stdout [1][2]. These commands allow you to communicate with the runner, such as setting environment variables, logging messages, or creating annotations [1][3]. To create an error annotation in the logs, use the following syntax [1][3]: echo "::error file={name},line={line},endLine={endLine},title={title}::{message}" For example: echo "::error file=app.js,line=1::Missing semicolon" Key details for using workflow commands: Syntax: Commands must be printed to stdout on a new line using the:: syntax [2][4]. Error Annotations: The error command creates an annotation in the workflow summary and logs [1][5]. While the error command itself logs the message, it does not automatically stop the workflow; to fail the step, you must also exit the process with a non-zero exit code (e.g., exit 1) [1][5]. Alternatively, using core.setFailed in a script acts as a shortcut for both creating an error annotation and setting the step status to failed [1][4]. Echo Control: You can control whether these workflow commands are echoed (printed) to the logs themselves using the echo command (e.g., echo "::echo::on" or echo "::echo::off") [2]. Note that the error command itself does not support echoing [2]. Command Prompt Differences: On Windows CMD, you may need to omit the double quotes when echoing commands to avoid issues with how the shell processes them [2]. Documentation for these commands is maintained as part of the official GitHub Actions workflow commands documentation [1][6].
Citations:
- 1: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands
- 2: https://github.com/actions/toolkit/blob/main/docs/commands.md
- 3: https://github.com/github/docs/blob/main/content/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions.md
- 4: https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/workflow-commands.md
- 5: https://stackoverflow.com/questions/71087094/how-to-make-a-github-workflow-error-message-show-up-in-log-and-annotations
- 6: https://docs.github.com/en/enterprise-server@3.19/actions/reference/workflows-and-actions/workflow-commands
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files matching check-doc-samples.sh:"
find . -name check-doc-samples.sh -type f 2>/dev/null || true
echo
echo "Read relevant file if present:"
if [ -f .github/scripts/check-doc-samples.sh ]; then
awk '{printf "%6d\t%s\n", NR, $0}' .github/scripts/check-doc-samples.sh | sed -n '1,80p'
fi
echo
echo "Search for workflow command strings in .github:"
grep -RInE 'echo[[:space:]]+(\'::|echo[[:space:]]+"::|::error|::warning|::notice|::group|::set-env|::save-state|::add-matcher|::stop-commands' .github 2>/dev/null || trueRepository: blehnen/FrigateRelay
Length of output: 3384
Keep GitHub error annotations on stdout.
Workflow command syntax like ::error::... must echo to the runner’s stdout; redirecting these annotations to stderr can prevent GitHub from rendering them as annotations. Print the annotation on stdout and emit any extra diagnostic text to stderr separately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/check-doc-samples.sh around lines 21 - 26, Update the error
handling in check-doc-samples.sh for the missing document and samples directory
so each GitHub ::error:: annotation is written to stdout without redirection; if
additional diagnostics are needed, emit them separately to stderr while
preserving the existing exit behavior.
Source: MCP tools
The WireMock.Net bump on this branch pulls a transitive Scriban.Signed 7.2.0, which carries two moderate advisories (GHSA-6q7j-xr26-3h2c / GHSA-q6rr-fm2g-g5x8). With NuGet audit + TreatWarningsAsErrors the NU1902 warning fails the Release build across every test project. Build fix: - tests/Directory.Build.props: central transitive pin Scriban.Signed 7.2.1 (patched), chained to the repo-root props via explicit parent import. - Host.Tests: align WireMock.Net 2.9.0 -> 2.10.0 with the other projects. SonarCloud sweep (behavior preserved; all unit tests green): - Cognitive complexity (S3776): extract ProcessItemAsync/RunValidatorsAsync, EnsureConnectedAsync, ValidateNames pass, DispatchMatchedSubscriptionAsync, TryParseNdjsonLine. - S3267 x7: fold filter loops into LINQ Where/Select in StartupValidation. - S907/S1116: replace goto+label with continue in the dispatcher. - S125/S1066/S108/S3358/S6966/S927/S3903/S4487: prose rewords, merged conditionals, documented empty catches, ternary extraction, await WriteLineAsync, ct rename, namespaced Log, options use. - S2699 x3 (BLOCKER): explicit enqueue-count assertions in EventPumpDispatchTests. Review follow-ups (CodeRabbit + Sonar PR analysis on #90): - FrigateMqttEventSource: track subscription health separately from TCP connectivity so a SUBACK denial re-subscribes instead of getting stuck until the socket drops (+2 regression tests). - Reconciler.TryParseNdjsonLine: guard non-string JSON fields and use DateTimeOffset.TryParse so a malformed NDJSON line is skipped, not fatal. - check-doc-samples.sh: keep ::error:: annotations on stdout (revert the S7677 stderr redirect — GitHub requires stdout for annotation commands). - docs/plugin-author-guide.md: sync the Program.cs fence with the sample (await WriteLineAsync + namespaced Log) so the doc-sample gate passes. - StartupValidation.ValidateNames: drop unnecessary casts + null-forgiving (S1905/S8970) by iterating with a shared helper. Not code-changed (accepted in SonarCloud as by-design/false-positive): the per-plugin TLS opt-in S4830 x4, localhost health-poll S5332, two S107 DI/interface signatures, S2325 interface-member false positives, docker S8431 tag+digest pins, and idiomatic out-param null-forgiving (S8970). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fe9194f to
f2e7d63
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/FrigateRelay.Sources.FrigateMqtt/EventContextProjector.cs (1)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant null-forgiving operators.
Nullable warnings are disabled, so both
default!assignments are unnecessary and trigger SonarCloud findings.
src/FrigateRelay.Sources.FrigateMqtt/EventContextProjector.cs#L55-L55: changecontext = default!;tocontext = default;.tools/FrigateRelay.MigrateConf/Reconciler.cs#L52-L52: changerow = default!;torow = default;.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/FrigateRelay.Sources.FrigateMqtt/EventContextProjector.cs` at line 55, Remove the redundant null-forgiving operators by changing context = default! to context = default in src/FrigateRelay.Sources.FrigateMqtt/EventContextProjector.cs:55 and row = default! to row = default in tools/FrigateRelay.MigrateConf/Reconciler.cs:52.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/FrigateRelay.Sources.FrigateMqtt/FrigateMqttEventSource.cs`:
- Around line 162-195: In the generic exception handler of
RunReconnectLoopAsync, reset _subscriptionHealthy to false alongside
_connectionStatus.SetConnected(false). Ensure this reset occurs whenever
reconnect processing fails so EnsureConnectedAsync cannot short-circuit on a
stale subscription grant, and cover the state reset with the relevant tests.
In `@tools/FrigateRelay.MigrateConf/Reconciler.cs`:
- Around line 92-95: Update GetStringOrNull to verify that element is a JSON
object before calling TryGetProperty. Return null for non-object roots, while
preserving the existing property-name and string-value checks for object
elements.
---
Nitpick comments:
In `@src/FrigateRelay.Sources.FrigateMqtt/EventContextProjector.cs`:
- Line 55: Remove the redundant null-forgiving operators by changing context =
default! to context = default in
src/FrigateRelay.Sources.FrigateMqtt/EventContextProjector.cs:55 and row =
default! to row = default in tools/FrigateRelay.MigrateConf/Reconciler.cs:52.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ecff2d7f-81ca-48b1-b477-9410fc3c9dfc
📒 Files selected for processing (18)
.github/scripts/check-doc-samples.shdocs/plugin-author-guide.mdsamples/FrigateRelay.Samples.PluginGuide/Program.cssrc/FrigateRelay.Host/Configuration/ActionEntryTypeConverter.cssrc/FrigateRelay.Host/Dispatch/ChannelActionDispatcher.cssrc/FrigateRelay.Host/EventPump.cssrc/FrigateRelay.Host/HostBootstrap.cssrc/FrigateRelay.Host/StartupValidation.cssrc/FrigateRelay.Plugins.Pushover/PushoverActionPlugin.cssrc/FrigateRelay.Sources.FrigateMqtt/EventContextProjector.cssrc/FrigateRelay.Sources.FrigateMqtt/FrigateMqttEventSource.cstemplates/FrigateRelay.Plugins.Template/src/FrigateRelay.Plugins.Example/ExampleActionPlugin.cstests/Directory.Build.propstests/FrigateRelay.Host.Tests/EventPumpDispatchTests.cstests/FrigateRelay.Host.Tests/FrigateRelay.Host.Tests.csprojtests/FrigateRelay.Sources.FrigateMqtt.Tests/MqttRobustnessTests.cstools/FrigateRelay.MigrateConf/AppsettingsWriter.cstools/FrigateRelay.MigrateConf/Reconciler.cs
🚧 Files skipped from review as they are similar to previous changes (9)
- src/FrigateRelay.Host/HostBootstrap.cs
- tests/Directory.Build.props
- samples/FrigateRelay.Samples.PluginGuide/Program.cs
- src/FrigateRelay.Host/StartupValidation.cs
- src/FrigateRelay.Host/Dispatch/ChannelActionDispatcher.cs
- src/FrigateRelay.Host/EventPump.cs
- tests/FrigateRelay.Host.Tests/FrigateRelay.Host.Tests.csproj
- tests/FrigateRelay.Host.Tests/EventPumpDispatchTests.cs
- tools/FrigateRelay.MigrateConf/AppsettingsWriter.cs
| /// <summary> | ||
| /// Ensures both the TCP link AND the topic subscription are healthy. Short-circuits (no ping, | ||
| /// no connect, no subscribe) only when the client is connected and the last subscribe was | ||
| /// granted — issue #19. Reconnects the TCP link when it is down, then (re)subscribes whenever | ||
| /// the subscription is not confirmed healthy, so a SUBACK denial recovers without waiting for | ||
| /// the socket to drop — issue #16 / CodeRabbit PR #90. | ||
| /// </summary> | ||
| private async Task EnsureConnectedAsync( | ||
| MqttClientOptions clientOptions, MqttClientSubscribeOptions subOptions, CancellationToken ct) | ||
| { | ||
| // #19: fully healthy (connected AND subscribed) — do nothing. Checking this first means we | ||
| // don't even ping on the happy path, avoiding a redundant ConnectAsync race during slow | ||
| // CONNACK. Subscription health is tracked separately so a topic denial does not get stuck | ||
| // here just because the socket is still up. | ||
| if (_client.IsConnected && _subscriptionHealthy) | ||
| return; | ||
|
|
||
| // (Re)establish the TCP link only when it is actually down (and a ping doesn't prove it is | ||
| // still alive). A subscription-only failure — socket up, topic denied — skips this and | ||
| // falls straight through to the re-subscribe below. | ||
| if (!_client.IsConnected && !await _client.TryPingAsync(ct).ConfigureAwait(false)) | ||
| await _client.ConnectAsync(clientOptions, ct).ConfigureAwait(false); | ||
|
|
||
| // #16: MQTTnet's SubscribeAsync does NOT throw when the broker returns a SUBACK failure | ||
| // reason code such as an ACL-denied subscribe. Inspect the per-topic result codes; if no | ||
| // topic was granted, mark the connection unhealthy so /healthz returns 503 and the next | ||
| // loop iteration retries the subscribe. Per-denial diagnostics are logged in the helper. | ||
| var subscribeResult = await _client.SubscribeAsync(subOptions, ct).ConfigureAwait(false); | ||
| _subscriptionHealthy = ProcessSubscribeResult(subscribeResult); | ||
| _connectionStatus.SetConnected(_subscriptionHealthy); | ||
| if (_subscriptionHealthy) | ||
| LogMqttConnected(_logger, null); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
MQTTnet 5.1 SubscribeAsync throws MqttCommunicationTimedOutException while IsConnected true
💡 Result:
In MQTTnet, encountering an MqttCommunicationTimedOutException during SubscribeAsync while IsConnected returns true is a known behavior stemming from how MQTTnet manages connection state and packet acknowledgement [1][2]. The core issue is that IsConnected does not guarantee a fully functional, bi-directional network connection [1]. Because TCP connections can become half-open or silent-dead (where the client is unaware the connection is broken until it attempts to send data and fails to receive a timely response), IsConnected often remains true until a keep-alive ping or data transmission attempt definitively fails [1][3]. When you call SubscribeAsync: 1. The client sends a SUBSCRIBE packet and begins waiting for a SUBACK packet from the broker [2]. 2. If the underlying network connection has silently died or the broker is unresponsive, the SUBACK never arrives [1][3]. 3. Once the internal timeout period (defined in your MqttClientOptions) is reached, the library throws an MqttCommunicationTimedOutException [2][4]. Common causes and recommended mitigations include: Half-Open Connections: The TCP connection may be dead at the broker or network level, but the client hasn't detected it yet [3]. Ensure your Keep-Alive interval is configured appropriately in MqttClientOptions to detect these silent failures more quickly [3]. Broker Latency or Load: If the broker is under heavy load or network latency is high, it may not be able to process and acknowledge your subscription request within the configured timeout [5]. You may need to increase the Timeout property in your MqttClientOptions if the environment is prone to latency [4][5]. Internal Race Conditions: Older versions of MQTTnet sometimes experienced issues where concurrent operations or rapid re-connections could lead to misleading states [6]. Ensure you are using the latest stable release of MQTTnet to benefit from fixes regarding packet dispatching and connection state management [3]. Handling the Exception: Because MQTTnet cannot predict connection health with 100% certainty, your application must be designed to catch MqttCommunicationTimedOutException [1][2]. Upon catching this exception, it is standard practice to treat the connection as compromised, perform a clean-up, and trigger a reconnection sequence [1][4]. If you are using the ManagedMqttClient, this process is generally handled automatically; if using the base MqttClient, you must implement this logic manually [7].
Citations:
- 1: MqttClient.PublishAsync(...) throws exception with message "The client is not connected.", but MqttClient.IsConnected returns true. dotnet/MQTTnet#899
- 2: https://github.com/dotnet/MQTTnet/blob/247988e6/Source/MQTTnet/MqttClient.cs
- 3: MqttClient keep-alive silently swallows PING timeout, never raises
DisconnectedAsyncdotnet/MQTTnet#2248 - 4: MQTTnet.Exceptions.MqttCommunicationTimedOutException HResult=0x80131500 Message=The operation has timed out. Source=mscorlib StackTrace: at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) in System.Runtime.CompilerServices\TaskAwaiter.cs:line 78 此异常最初是在此调用堆栈中引发的: dotnet/MQTTnet#1921
- 5: Connection timeout error due to high latency on the network dotnet/MQTTnet#1956
- 6: Client: Tolerate late acknowledgement packets (#2078, #2079) dotnet/MQTTnet#2247
- 7: Connection Issue - Timeout while waiting for packet of type 'MqttConnAckPacket'. dotnet/MQTTnet#195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate file =="
fd -a 'FrigateMqttEventSource\.cs$' . || true
echo "== Relevant file outline =="
file="$(fd 'FrigateMqttEventSource\.cs$' . | head -n 1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
ast-grep outline "$file" --view expanded || true
echo "== Relevant sections =="
sed -n '1,240p' "$file" | cat -n
fi
echo "== Search subscription健康/healthy symbols =="
rg -n "_subscriptionHealthy|SubscribeAsync|SetConnected|RunReconnectLoopAsync|EnsureConnectedAsync|TryPingAsync|Catch|catch \\(Exception ex\\)" .Repository: blehnen/FrigateRelay
Length of output: 22139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package references for MQTTnet =="
rg -n 'MQTTnet|Docker.Image|PackageReference|ProjectReference' -S . \
-g '*.csproj' -g 'Directory.Packages.props' -g '*.props' -g 'global.json' || true
echo "== target framework =="
rg -n '<TargetFramework|TargetFramework' -S . -g '*.csproj' || true
echo "== MqttRobustnessTests relevant section =="
sed -n '1,240p' tests/FrigateRelay.Sources.FrigateMqtt.Tests/MqttRobustnessTests.cs | cat -n
echo "== deterministic state-flow probe =="
python3 - <<'PY'
class State:
def __init__(self, client_connected=True, subscription_healthy=True):
self.client_is_connected = client_connected
self.subscription_healthy = subscription_healthy
# Exact short-circuit from EnsureConnectedAsync lines 176-177
def should_short_circuit_on_next_ensure_connected(self):
return self.client_is_connected and self.subscription_healthy
# Scenario: one successful previous subscription left state healthy; SubscribeAsync throws
# generic catch sets client to disconnected via SetConnected(false), but this probe covers
# the other reachable path: TCP remains reporting connected despite earlier SUBACK timeout.
state = State(client_connected=True, subscription_healthy=True)
state.subscription_healthy = False # if it were reset, Next iteration would NOT short-circuit
print("With reset:", not state.should_short_circuit_on_next_ensure_connected())
state = State(client_connected=True, subscription_healthy=True)
# stale path with no reset (current generic catch)
print("Without reset:", state.should_short_circuit_on_next_ensure_connected())
PYRepository: blehnen/FrigateRelay
Length of output: 48268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Fetch MQTTnet 5.1.0.1559 source for SubscribeAsync timeout handling =="
tmp="$(mktemp -d)"
cd "$tmp"
git clone --filter=blob:none --depth=1 --branch v5.1.0.1559 https://github.com/dotnet/MQTTnet.git MQTTnet-src >/dev/null
cd MQTTnet-src
echo "== File list for relevant source =="
fd -a 'MqttClient.cs|MqttChannel.cs|MqttPacketSender.cs|MqttPacketDispatcher.cs|MqttPacketQueue.cs|MqttPacketProcessor.cs|MqttPacketFactory.cs|MqttClientSession.cs' . || true
echo "== SubscribeAsync implementations/usages =="
rg -n "SubscribeAsync|MqttSubscribePacket|Timeout|CancellationTokenRegister|TaskCompletionSource|SendSubscribe|CommunicationTimedOut|MqttCommunicationTimedOutException" Source/MQTTnet -S
echo "== Context around selected matches =="
matches="$(rg -n "PublishAsync|SubscribeAsync|MqttSubscribePacket" Source/MQTTnet -S | head -n 20 || true)"
echo "$matches"Repository: blehnen/FrigateRelay
Length of output: 450
🌐 Web query:
MQTTnet source Submit MqttPacket SendTimeout MqttClient SubscribeAsync
💡 Result:
In MQTTnet, the SubscribeAsync method does not have a dedicated individual parameter for a send timeout; instead, it relies on the CancellationToken provided during the call or the global Timeout setting configured in the MqttClientOptions [1][2][3]. When calling SubscribeAsync, the library behaves as follows: 1. Global Timeout: The MqttClientOptions class includes a Timeout property (defaulting to 100 seconds) [4]. This timeout is applied to socket-level communication and internal operations [5][4]. 2. Internal Logic: In the implementation of SubscribeAsync, if no CancellationToken is provided (or if the provided token cannot be canceled), the client automatically uses a CancellationTokenSource configured with the MqttClientOptions.Timeout value to enforce the wait duration for the server's acknowledgement (SUBACK packet) [1][3]. 3. Custom Timeout: If you require a specific timeout for a single SubscribeAsync operation that differs from the global setting, you can pass a custom CancellationToken created with a linked CancellationTokenSource (e.g., using new CancellationTokenSource(timeSpan)) to the SubscribeAsync method [1][3]. Regarding "Submit MqttPacket," the MQTTnet library generally handles packet creation and transmission internally via higher-level methods like SubscribeAsync or PublishAsync [2][3]. While internal methods (such as Send or SendAndReceiveAsync) manage the actual transmission of packets, these are typically abstracted away from the public API to ensure correct MQTT protocol handling, such as tracking packet identifiers and awaiting specific acknowledgement packets [1][3]. Direct submission of arbitrary MqttPacket objects is generally not supported through the public IMqttClient interface [6].
Citations:
- 1: https://github.com/dotnet/MQTTnet/blob/247988e6/Source/MQTTnet/MqttClient.cs
- 2: https://docs.dndocs.com/n/MQTTnet/5.0.1.1416/api/MQTTnet.MqttClient.html
- 3: https://github.com/dotnet/MQTTnet/blob/2aa4219838a67e336ae4f5902d956c455bc48c11/Source/MQTTnet/Client/MqttClient.cs
- 4: https://github.com/dotnet/MQTTnet/blob/247988e6/Source/MQTTnet/Options/MqttClientOptions.cs
- 5: https://docs.dndocs.com/n/MQTTnet/5.0.1.1416/api/MQTTnet.MqttClientOptions.html
- 6: https://docs.dndocs.com/n/MQTTnet/5.0.1.1416/api/MQTTnet.IMqttClient.html
Reset _subscriptionHealthy in the generic reconnect-loop catch.
EnsureConnectedAsync only updates _subscriptionHealthy after SubscribeAsync completes successfully. If SubscribeAsync throws, RunReconnectLoopAsync sets _connectionStatus.SetConnected(false) but leaves _subscriptionHealthy unchanged, so a stale previous grant can let the next loop short-circuit and avoid re-verifying the subscription. Set _subscriptionHealthy = false alongside _connectionStatus.SetConnected(false) in the generic exception handler and cover this state reset.
🔧 Proposed fix
catch (Exception ex)
{
_connectionStatus.SetConnected(false);
+ _subscriptionHealthy = false;
LogMqttConnectFailed(_logger, ex, null);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/FrigateRelay.Sources.FrigateMqtt/FrigateMqttEventSource.cs` around lines
162 - 195, In the generic exception handler of RunReconnectLoopAsync, reset
_subscriptionHealthy to false alongside _connectionStatus.SetConnected(false).
Ensure this reset occurs whenever reconnect processing fails so
EnsureConnectedAsync cannot short-circuit on a stale subscription grant, and
cover the state reset with the relevant tests.
| private static string? GetStringOrNull(JsonElement element, string propertyName) => | ||
| element.TryGetProperty(propertyName, out var prop) && prop.ValueKind == JsonValueKind.String | ||
| ? prop.GetString() | ||
| : null; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard TryGetProperty against non-object roots.
JsonDocument.Parse accepts arrays, strings, numbers, and null; calling TryGetProperty on those root values throws InvalidOperationException, so reconciliation can abort before the line is skipped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/FrigateRelay.MigrateConf/Reconciler.cs` around lines 92 - 95, Update
GetStringOrNull to verify that element is a JSON object before calling
TryGetProperty. Return null for non-object roots, while preserving the existing
property-name and string-value checks for object elements.
1 participant
Summary
Fixes the Release build break introduced on this branch: the WireMock.Net bump pulls a transitive
Scriban.Signed 7.2.0, which carries two moderate advisories (GHSA-6q7j-xr26-3h2c / GHSA-q6rr-fm2g-g5x8). With NuGet audit +TreatWarningsAsErrors, the NU1902 warning fails the build in every test project. A newtests/Directory.Build.propscentrally pins the patchedScriban.Signed 7.2.1(chained to the repo-root props via an explicit parent import so test projects keepTargetFramework/warnings-as-errors/etc.), andHost.Testsis aligned to WireMock.Net 2.10.0.Bundled in the same commit (per CodeRabbit rate-limit preference): a SonarCloud code-quality sweep clearing 36 issues — five cognitive-complexity extractions (
ProcessItemAsync/RunValidatorsAsync,EnsureConnectedAsync, theValidateNamesuniform pass 56→low,DispatchMatchedSubscriptionAsync,TryParseNdjsonLine), seven S3267 LINQ simplifications,goto→continue, the 3 BLOCKER test-assertion additions, and assorted mechanical fixes. All behavior-preserving; 313 unit tests still pass.Another 11 SonarCloud issues were reviewed and accepted as by-design / false-positive directly in SonarCloud (not code-changed): the per-plugin TLS opt-in (S4830 ×4), the localhost health-poll (S5332), the two S107 locked DI/interface signatures, the two S2325 interface-member false positives, and the two docker S8431 tag+digest supply-chain pins.
Type of change
Checklist
dotnet build FrigateRelay.sln -c Release(0 warnings / 0 errors)bash .github/scripts/run-tests.sh(313 unit tests; integration skipped — needs Docker).Result/.Wait()calls in sourceCHANGELOG.mdupdated under## [Unreleased]if user-visible — not updated (internal build/quality fix; happy to add a note if you consider the CVE bump user-visible)Notes for reviewer
Risk areas — three refactors touch behavior-critical paths and are the main things to eyeball:
ChannelActionDispatcher.ProcessItemAsync: cancellation now rethrows from the per-item helper soConsumeAsync's outer handler still stops the consumer gracefully (previously an innerreturn). Poison-item swallow semantics unchanged. Covered by existing Host.Tests dispatcher + observability tests.FrigateMqttEventSource.EnsureConnectedAsync: the connect guard was inverted to an early-return; short-circuit semantics (IsConnected || ping) are equivalent to the original!IsConnected && !ping.StartupValidation.ValidateNames: six nested loops collapsed into one uniform pass; error ordering preserved (subscriptions before profiles).Not changed in code (accepted in SonarCloud): the S4830 TLS opt-in is the documented
AllowInvalidCertificatesinvariant; the S8431 tag+digest pins are architecture decision B3 (Dependabot rewrites both atomically); the S2325 hits are interface members that cannot be static.Summary by CodeRabbit