From 2dc2075d05111eb6be51c0d9853bbc33f35337eb Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:22:47 -0400 Subject: [PATCH] docs(code-review): close all 8 open Mqtt.Browser + Mqtt.Contracts findings Browser-002/-003/-004 fixed and -005 resolved as a documented constants change (eddf5f57); Contracts-004/-005/-006/-007 fixed (93f70504). Both module headers drop to 0 open findings. Each resolution records the disposition, the RED evidence, and the deviations from the recommendation where one was taken. --- code-reviews/Driver.Mqtt.Browser/findings.md | 81 ++++++++++-- .../Driver.Mqtt.Contracts/findings.md | 122 ++++++++++++++++-- 2 files changed, 185 insertions(+), 18 deletions(-) diff --git a/code-reviews/Driver.Mqtt.Browser/findings.md b/code-reviews/Driver.Mqtt.Browser/findings.md index 9308f7b3..b6f80e4f 100644 --- a/code-reviews/Driver.Mqtt.Browser/findings.md +++ b/code-reviews/Driver.Mqtt.Browser/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Reviewed | -| Open findings | 4 | +| Open findings | 0 | ## Scope @@ -86,7 +86,7 @@ four wildcard shapes, each asserting `PublishCountForTest == 0`. | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:469` | -| Status | Open | +| Status | Resolved | **Description:** The plain-mode and Sparkplug-mode paths disagree about what may become a tree label. `ObserveSparkplugBirth` runs every group/edge-node/device/metric identifier @@ -109,7 +109,26 @@ marker. The plain-mode marker text would need widening from "Some topics were to show" to cover malformed names — the Sparkplug branch already words its marker that way ("oversized or malformed names"), so the two can converge. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed in `eddf5f57` — `ObservePlainTopic`'s pre-pass now rejects a +segment carrying a control character as well as an oversized one, and the rule is single-sourced +in a new `HasControlCharacter` that `IsUsableLabel` also calls, so the two ingest paths cannot +drift again. The plain marker converges on the Sparkplug wording ("oversized or malformed +names"). Pinned by +`MqttBrowseSessionTests.PlainTopicSegmentWithAControlCharacter_IsRejectedWhole_AndSurfacesAMarker` +(every segment well under `MaxSegmentChars`, so the length guard cannot be what passes it) plus +`PlainTopic_EachControlCharacterForm_IsRejected` over `\n` / `\r` / `\t` / ESC / NUL — 6 RED +before the fix — with `PlainTopic_PrintableNonAsciiSegments_AreStillAccepted` as the +falsification control: the rule is *control characters*, not "anything unusual", and a topic +segment legitimately carries accents, spaces and punctuation. + +The second, smaller half — an **empty** topic level — was deliberately **not** converged with +`IsUsableLabel`'s non-empty rule, and `PlainTopic_WithAnEmptyLevel_IsStillRecorded` pins that +choice. `a//b` is legal MQTT and its terminal leaf is a real committable address (the commit +reads `AddressFields[topic]`, which carries the node's full path), so rejecting the whole topic +would cost the operator a legal address to tidy a blank row — a worse trade than the display +defect it fixes. The control-character half is the one that had to converge, because a label +carrying a newline breaks the row it renders in. The Sparkplug path has no equivalent case: every +id it labels came off `SparkplugTopic.TryParse`, which already rejects an empty segment. ### Driver.Mqtt.Browser-003 @@ -118,7 +137,7 @@ show" to cover malformed names — the Sparkplug branch already words its marker | Severity | Low | | Category | Concurrency & thread safety | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:685` | -| Status | Open | +| Status | Resolved | **Description:** `GetOrCreate` reads `_nodeCount` and compares it to `_nodeCap`, then increments after a successful `GetOrAdd`. The read and the increment are not atomic with @@ -138,7 +157,22 @@ pair with a single `Interlocked.Increment` compared against the cap, decrementin lost-race path (`!ReferenceEquals(child, created)`). Until then, a comment stating the single-dispatcher assumption the current shape relies on would be enough. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed in `eddf5f57` rather than commented — the recommendation's +own first option. `GetOrCreate` reserves the slot with a single `Interlocked.Increment` compared +against the cap and gives it back on **both** paths that do not consume it (the cap trip, and the +lost `GetOrAdd` race), so the count is exact rather than merely bounded. Pinned by +`MqttBrowseSessionTests.NodeCap_HoldsUnderConcurrentObservation`, which drives +`2 × ProcessorCount` dedicated threads through a `Barrier` at a cap of **1**, over 40 rounds: +against the read-then-increment shape it overshot to **31** nodes on round 0, and it stays at 1 +with the fix (falsifiability re-checked by reverting the production change after the test was +made fast). Dedicated threads rather than `Task.Run` on purpose — every worker blocks on the +barrier, and blocking that many pool threads makes the pool inject on its ~500 ms starvation +timer, which both slowed the test to 23 s and staggered the very arrivals it aligns. + +The second race the entry notes (a node visible in `Children` before `_byPath`) is unchanged and +still accepted: `ExpandAsync` returning empty for a node the tree already holds self-heals on the +next call, and the class remarks say so. The comment the recommendation offered as the minimum is +now moot — the shape no longer rests on a single-dispatcher assumption at all. ### Driver.Mqtt.Browser-004 @@ -147,7 +181,7 @@ single-dispatcher assumption the current shape relies on would be enough. | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:1079` | -| Status | Open | +| Status | Resolved | **Description:** `Project` orders a level by `n.Segment`, but the dictionary is keyed by `n.Path` — and the whole reason for that keying (documented on `GetOrCreate`) is that in @@ -167,7 +201,20 @@ and/or order folders before leaves so a device and a same-named metric are visua separated. Either is a one-line change; a test observing a device and an edge-node metric of the same name would pin it. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed in `eddf5f57` — `Project` now sorts +`.OrderBy(Segment).ThenBy(Path, Ordinal)`, making the order total. The folders-before-leaves +variant was not taken: the path tiebreaker already separates them (Ordinal puts `/` (0x2F) before +`:` (0x3A), so a device folder precedes its same-named node-level metric) without introducing a +second ordering concept. `Project`'s comment now states the reason instead of the over-claim the +entry quotes. + +⚠️ **The single-pair test the recommendation suggests passes vacuously**, and did: .NET string +hashing is randomised per process, so the tie order a segment-only sort falls through to is +whatever *that run's* `ConcurrentDictionary` yields — with one pair it lands the right way round +about half the time, and the first attempt at this test went GREEN against the unfixed code. +`SparkplugTree_SiblingsSharingALabel_AreOrderedDeterministically` therefore uses **24** colliding +device/metric pairs, observed metric-first, and asserts the whole level: it failed 3/3 runs +against the unfixed sort and passes with it. ### Driver.Mqtt.Browser-005 @@ -176,7 +223,7 @@ the same name would pin it. | Severity | Low | | Category | Performance & resource management | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:59`, `:81` | -| Status | Open | +| Status | Resolved | **Description:** The per-session memory ceiling is higher than the constants suggest at a glance. `DefaultNodeCap` is 50,000 nodes, and each `TopicNode` retains its full `Path` @@ -197,4 +244,20 @@ tree"). What is missing is any bound on the *aggregate* across sessions. is short enough to matter; if it does not, cap there rather than lowering `DefaultNodeCap`, which would make the picker less useful on a genuinely large broker. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Resolved as a **documentation change with no behaviour change**, in +`eddf5f57` — the recommendation's own verdict ("no code change needed in this module"). + +The confirmation it asks for was done, and the answer is **no**: `BrowseSessionRegistry` is a bare +`ConcurrentDictionary` that caps neither total sessions nor sessions per user. What bounds the +aggregate is **lifetime** — `BrowseSessionReaper` evicts and disposes anything untouched for a +2-minute `IdleTtl` on a 30-second tick, and `DisposeAsync` clears both maps — so the ceiling is +"sessions opened within a 2-minute window", not unbounded accumulation. That is short enough to +matter, so no cap was added; capping the session count there stays the right lever if one is ever +needed, rather than lowering `DefaultNodeCap`. + +`DefaultNodeCap`'s doc comment now carries the honest per-session arithmetic (each node retains +its full `Path`, its `Segment` and a snippet, plus its own child dictionary, and every node is +held twice — so a session near the ceilings costs on the order of 100 MB, not the few MB "50 000 +nodes" suggests), names the `#`-with-no-`TopicPrefix` open as the shape that approaches it, and +records where a future bound belongs. No test accompanies this: nothing behavioural changed, and +a test over a constant's doc comment would pin prose. diff --git a/code-reviews/Driver.Mqtt.Contracts/findings.md b/code-reviews/Driver.Mqtt.Contracts/findings.md index 348eb820..49269fab 100644 --- a/code-reviews/Driver.Mqtt.Contracts/findings.md +++ b/code-reviews/Driver.Mqtt.Contracts/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Reviewed | -| Open findings | 4 | +| Open findings | 0 | ## Scope @@ -162,7 +162,7 @@ deliberately excludes `metricName`); pinned by | Severity | Low | | Category | Documentation & comments | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttTagDefinitionFactory.cs:124`, `:234`, `:476` | -| Status | Open | +| Status | Resolved | **Description:** `retainSeed` is the one behavioural key still read leniently. `ReadBoolOrDefault` returns the default for any non-boolean JSON value, so @@ -186,7 +186,36 @@ so both gates stay in step; or (b) add an `Inspect` advisory naming the key and default; or (c) at minimum document the deliberate exception on `ReadBoolOrDefault`. Option (b) is lowest-risk — it changes no verdict. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed in `93f70504` — option (a), the strict read, chosen over the +"lowest-risk" advisory. The #549–#559 arc's settled contract is that **the parse is the accept +authority** and advisories ride blobs the parse has already ACCEPTED; an advisory here would have +left the driver still serving the inverse of what was authored, and the review's own reason for +recording rather than fixing ("the two surfaces are currently consistent") is satisfied without a +hand-mirrored rule — see below. Every sibling behavioural key in this factory (`payloadFormat`, +`dataType`, `qos`) is already strict, and `retainSeed` is the sharper case of the two: a defaulted +`qos` is a weaker delivery guarantee, whereas a defaulted `retainSeed` is the **inverse** of the +authored intent whenever the author wrote `false` — the only value worth writing at all. + +Both mappers now read it through a new `TryReadBoolStrict`, mirroring `TryReadQosStrict`'s +absent / valid / present-but-invalid split, and `DescribeRejection` gained a matching +`DescribeInvalidBool` branch (a rejection, never an advisory: unlike `payloadFormat`, both mappers +read the key, so it can never land on an accepted blob). + +**No second gate was bolted on outside Parse, and `MqttTagConfigModel` needed no mirrored rule.** +Its `Validate()` already ends in `TagConfigParseGate.Describe`, which asks *this* parse about the +blob the model was loaded from and words the refusal from *this* `Inspect` — so tightening the +factory tightened the authoring surface in the same change, with no drift surface created. The +model's `RetainSeed` doc now records that its own load stays lenient (a non-boolean reads as +"unset") and why that is safe. + +Pinned by `MqttTagDefinitionFactoryTests.FromTagConfig_MalformedRetainSeed_RejectsStrict` / +`FromSparkplugTagConfig_MalformedRetainSeed_RejectsStrict` (over `"false"`, `"true"`, `0`, `1`, +`"yes"`, `null`), `Inspect_MalformedRetainSeed_Warns`, and — as the falsification control that a +reader simply refusing every blob carrying the key would fail — +`FromTagConfig_WellFormedRetainSeed_IsStillHonoured`. 11 RED before the fix. The +authoring↔deploy parity is pinned separately, on the corpus that would catch a future divergence: +`TagConfigNonEnumParityTests.Mqtt_refuses_a_retainSeed_its_reader_refuses` and its paired +`Mqtt_accepts_a_retainSeed_its_reader_accepts`. ### Driver.Mqtt.Contracts-005 @@ -195,7 +224,7 @@ Option (b) is lowest-risk — it changes no verdict. | Severity | Low | | Category | Testing coverage | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttDriverOptions.cs:25`, `:71`, `:75`, `:79`, `:83`, `:99`, `:229` | -| Status | Open | +| Status | Resolved | **Description:** `MqttDriverOptions` / `MqttPlainOptions` carry seven `System.ComponentModel.DataAnnotations` `[Range]` attributes and **nothing ever runs them**. @@ -221,7 +250,35 @@ checks the probe and the AdminUI form already carry. If neither, note on each at that it is descriptive. The enforcing consumer (`MqttSubscriptionManager.BuildFilters`) lives in `Driver.Mqtt`, outside this module. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed in `93f70504` — the attributes were **deleted**, not wired up, +and the one consequence that actually escaped was closed at the enforcing consumer. + +**Wiring them would have broken deliberate behaviour**, which settles the "either/or" the +recommendation leaves open. Two of the seven *contradicted* the real authority: `[Range(1, …)]` +on `KeepAliveSeconds` forbids the **zero** `MqttDriver.Normalize` explicitly admits ("zero +keep-alive is legal MQTT; only a negative one is nonsense"), and the same attribute on +`MaxPayloadBytes` forbids the non-positive value this type's own remarks document as "falls back +to the driver's own 1 MiB default". Running the annotations would have refused two configurations +the driver is written to accept — the "two disagreeing authorities" outcome, arrived at by +enforcement rather than by neglect. So the bounds stay where they are applied +(`MqttDriver.Normalize`, `MqttSubscriptionManager`'s constructor, `MqttConnection.NextBackoff`), +each property names its own, and the type remarks record why there are no `DataAnnotations` here +and why adding one would be documentation that reads like enforcement. + +**`MqttPlainOptions.DefaultQos` — the escape the entry identifies — is now clamped** in +`MqttSubscriptionManager`'s constructor, the same single enforcement point that already bounds +`MaxPayloadBytes`. Clamp rather than fall back to the default, deliberately: it matches what +`MqttDriverFormModel.Normalize` already persists (`Math.Clamp(DefaultQos, 0, 2)`), so the +authoring surface and the driver cannot reach different answers for one authored number. Pinned +by `MqttSubscriptionManagerTests.DefaultQos_OutsideTheLegalRange_NeverReachesTheSubscribe`, which +asserts the QoS on the **fake transport's actual SUBSCRIBE** rather than on a property — `5 ⇒ 2` +and `-1 ⇒ 0` were RED, with `0 ⇒ 0` / `2 ⇒ 2` green throughout as the controls. + +Scope note: this closes the MQTT half only. The entry's observation that `[Range]` on driver +options is a **repo-wide** convention (`ModbusDriverOptions`, `S7DriverOptions`) still stands, and +those attributes are still unenforced — this change does not touch them, and the evidence above +argues the fleet-wide answer is deletion plus real bounds at each use site rather than a +composition-seam `Validator.TryValidateObject`. ### Driver.Mqtt.Contracts-006 @@ -230,7 +287,7 @@ lives in `Driver.Mqtt`, outside this module. | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttWriteEncoder.cs:281` | -| Status | Open | +| Status | Resolved | **Description:** Two edge cases in the `DriverDataType.DateTime` write arm. First, `(ulong)…ToUnixTimeMilliseconds()` on a pre-1970 value wraps a negative `long` into a very @@ -250,7 +307,27 @@ negative, and convert with an explicit `DateTime.SpecifyKind(…, DateTimeKind.U `Unspecified` input (or refuse it, which is more in keeping with the file's own posture). Add a test for each. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Both fixed in `93f70504`. + +The epoch bound: `ToUnixTimeMilliseconds()` is now taken into a `long`, and a negative one returns +`ValueNotRepresentable` before anything is written to the metric — Sparkplug's `DateTime` is an +*unsigned* ms-since-epoch, so a pre-1970 instant has no representation at all and the surrounding +"refused, never coerced" rule applies unchanged. + +The kind: an `Unspecified` input is converted with `DateTime.SpecifyKind(…, DateTimeKind.Utc)` +rather than `ToUniversalTime()`. The entry offers refusal as "more in keeping with the file's own +posture", and it was not taken — `Convert.ToDateTime("2026-08-10T12:00:00")`, an ordinary authored +string, produces exactly this kind, so refusing would reject a perfectly reasonable write to fix a +determinism problem that reading it as UTC also fixes. + +Pinned by `MqttWriteEncoderTests.A_pre_epoch_date_time_is_refused_rather_than_wrapped` (three +instants, incl. `DateTime.MinValue`) with `A_post_epoch_date_time_still_encodes_to_its_unix_milliseconds` +as the falsification control — a guard that refused *every* `DateTime` would satisfy the theory +alone — and `An_unspecified_kind_date_time_encodes_as_utc_not_as_node_local_time`, which asserts +the Unspecified and the same-instant-UTC encodings are byte-identical. ⚠️ That last one can only +go red on a host whose local timezone is not UTC (it was RED here under both `EDT` and +`TZ=Asia/Tokyo`, and is green under `TZ=UTC` either way) — which is the defect itself, and the +reason it is written as an equality between two encodings rather than against a hard-coded number. ### Driver.Mqtt.Contracts-007 @@ -259,7 +336,7 @@ Add a test for each. | Severity | Low | | Category | Error handling & resilience | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/Sparkplug/SparkplugTopic.cs:202`, `:232` | -| Status | Open | +| Status | Resolved | **Description:** `SparkplugTopic` validates asymmetrically. `TryParse` enforces `IsValidSegment` (non-empty, no `+`/`#`) on every segment it reads, but `Format` and @@ -279,4 +356,31 @@ succeeded, so the encoder's own guards must stay — `TryEncode` returns an erro throwing, and that contract must not change. A round-trip property test (`Parse(t.ToTopicString()) == t` over the parser's own accepted corpus) would pin it. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed in `93f70504` — `Format` and `FormatState` now apply the +parser's own `IsValidSegment` to `groupId` / `edgeNodeId` / `deviceId` / `hostId` through a shared +`ThrowIfNotASegment`, throwing `ArgumentException` as the existing contract does. + +One deliberate addition beyond the recommendation's literal wording: the format-side check also +rejects **`/`**. `IsValidSegment` has no reason to test for one — `TryParse` has already split on +it — but on this side it is the difference between one id and two, and it is precisely the shape +the entry describes as "silently adds a segment". Without it the round-trip property the +recommendation asks for would not actually be total, since such a topic re-parses to nothing. + +The caution the entry raises is respected: `MqttWriteEncoder`'s own guards stay, so `TryEncode` +still *returns* `NoWriteDestination` for these ids rather than propagating a throw, and that +contract is unchanged. The one production caller that can now throw where it previously produced +a malformed topic is `SparkplugPrimaryHost.StateTopic` (a wildcard `hostId`); it is reached only +from `MqttDriver.StartPrimaryHostAsync`, which is wrapped in a catch that logs a warning and +leaves ingest untouched by design — so the effect is "the host claim declines with a reason" +instead of "publish a protocol violation and lose the connection". + +Pinned by `SparkplugTopicTests.Format_SegmentThatIsNotALegalTopicSegment_Throws` (9 cases: +`+`/`#` in each id, `/` in group and device, and an empty `deviceId`) and +`FormatState_HostIdThatIsNotALegalTopicSegment_Throws` — 9 RED before the fix — with +`Format_UnusualButLegalSegments_AreStillAccepted` as the falsification control (a Sparkplug id is +otherwise opaque: spaces, dots, dashes and accents are all legitimate, and a guard that refused +everything would satisfy the throw theories). The closure property is +`Format_Output_AlwaysReParsesToTheSameInstance`, asserted over the **formatter's own output** +across every non-STATE message type plus `FormatState`, rather than over a hand-written topic +list, so a future formatting change cannot leave the corpus describing a shape the type no longer +emits.