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.
This commit is contained in:
Joseph Doherty
@@ -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.
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user