diff --git a/code-reviews/Driver.Mqtt.Transport/findings.md b/code-reviews/Driver.Mqtt.Transport/findings.md index 8aa803d5..64c220a2 100644 --- a/code-reviews/Driver.Mqtt.Transport/findings.md +++ b/code-reviews/Driver.Mqtt.Transport/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Reviewed | -| Open findings | 1 | +| Open findings | 0 | ## Checklist coverage @@ -110,7 +110,7 @@ existing assertion depends on the branch being silent (713 tests green). | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Transport/MqttClientOptionsFactory.cs:97-100` | -| Status | Open | +| Status | Resolved | **Description:** Credentials are attached only when `Username` is non-empty: `if (!string.IsNullOrEmpty(options.Username)) builder.WithCredentials(options.Username, options.Password ?? "")`. @@ -129,4 +129,15 @@ credential is visible; optionally send password-only credentials under `V500`, w permits it. Do **not** log the password itself — nothing on this path formats it today, and the options record's `PrintMembers` override exists to keep it that way. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed `5d4dd17d` — the recommendation's first half only. A password +configured with a blank username now logs a warning saying the connection is being made +**anonymously** and naming the two ways to make that intentional; the credential is still not sent, +and the secret is never formatted into the message. The optional half (a password-only CONNECT +under `V500`) was deliberately **not** taken: 3.1.1 forbids it, so it cannot be the uniform +behaviour of a factory that serves both versions, and adding it under `V500` alone would change the +wire behaviour of deployments that connect happily today — the same "the explicit config stays +authoritative, say so rather than second-guess it" line Driver.Mqtt.Transport-001 settled on. +TDD: the warning assertion failed RED (`log.CountContaining("password") should be 1 but was 0`) +before the change. Paired with a falsifying control — a config carrying **neither** credential must +stay silent, or the fix would warn on every ordinary anonymous deployment and the first test would +still pass. Suite green (734). diff --git a/code-reviews/Driver.Mqtt/findings.md b/code-reviews/Driver.Mqtt/findings.md index c5dfd3de..e213e728 100644 --- a/code-reviews/Driver.Mqtt/findings.md +++ b/code-reviews/Driver.Mqtt/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Reviewed | -| Open findings | 2 | +| Open findings | 0 | ## Checklist coverage @@ -214,7 +214,7 @@ with a comment naming the extension method that needs it. Build verified. | Severity | Low | | Category | Concurrency & thread safety | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/Sparkplug/SparkplugPrimaryHost.cs:278` | -| Status | Open | +| Status | Resolved | **Description:** `DisposeAsync` calls `_gate.Dispose()`. `StopAsync` cancels the reconcile loop but does not await it, so a `ReconcileAsync` already in flight can be sitting in @@ -236,7 +236,16 @@ stated rationale — the gate holds no timer and no surviving registration), or the reconcile loop before dispose returns. The former is the smaller change and the one already argued for in this module. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed `3f85ba98` — the first option. `DisposeAsync` no longer disposes +`_gate`, and both the field and the dispose site now carry the rule and the reason, so the module +states **one** answer for its lifecycle primitives rather than two. Nothing is leaked (the gate +holds no timer and no registration) and `ClaimAsync` still re-checks `_disposed` under it, so no +claim can be created after dispose either way. TDD: a test that disposes while a reconcile is in +flight failed RED with `log.Captured should not contain an element satisfying (name == +"ObjectDisposedException") but does`. It is deterministic rather than timing-dependent — the gate is +taken *before* the connect starts, so a `MiniBroker` in blackhole mode (TCP accepted, CONNECT never +answered) having accepted the socket proves the claim is inside the gate when dispose runs. Suite +green (734). ### Driver.Mqtt-007 @@ -245,7 +254,7 @@ argued for in this module. | Severity | Low | | Category | Error handling & resilience | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttSubscriptionManager.cs:406-424` (`OnReconnectedAsync`) | -| Status | Open | +| Status | Resolved | **Description:** When the reconnect path finds live subscribed topics but resolves **zero** filters for them, it logs a warning and returns normally. The type's own contract, stated two paragraphs @@ -263,7 +272,19 @@ which is an argument for leaving the code alone, not for leaving the contract am *this* deaf-reconnect shape is exempt from the type's own rule. Do not leave the two answers side by side unexplained. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed `2298485b` — it throws, and the remarks now state the single rule. +Exempting it was considered and rejected: the reason the partial-rejection case is exempt is that +tearing the session down would punish the tags still flowing, and here there are none — every live +topic is in the set that resolved to nothing — so the teardown costs nothing and the retry is not +futile, since it runs against whatever `Register` has published by then. The `subscribed.Count == 0` +branch stays a debug no-op: a reconnect with nothing subscribed yet re-establishes nothing because +there is nothing to establish. TDD: `Should.ThrowAsync` failed RED +(`should throw System.InvalidOperationException but did not`). The test reaches the state the only +way production can — `Register` prunes `_subscribedTopics` of the topics its new table dropped, but a +SUBSCRIBE already in flight records its own topic *after* that prune, so a deploy landing inside the +round-trip leaves a live topic no authored tag resolves to — driven through a `DuringSubscribe` hook +on the existing fake transport. Paired with the control that a reconnect with no live subscriptions +stays a no-op, which a blanket `filters.Count == 0` throw would have broken. Suite green (734). ### Driver.Mqtt-008