dohertj2/lmxopcua
1
0
Fork 0
Code Issues 14 Pull Requests Actions Packages Projects Releases Wiki Activity

merge: MQTT browser+contracts review findings resolved (Driver.Mqtt.Browser-002..-005, Driver.Mqtt.Contracts-004..-007)

Browse Source
This commit is contained in:
Joseph Doherty
2026-08-10 08:23:16 -04:00
parent a54661ded8 2dc2075d05
commit acea65eb96
15 changed files with 859 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Driver.Mqtt.Browser/findings.md
+72 -9
View File
@@ -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.
code-reviews/Driver.Mqtt.Contracts/findings.md
+113 -9
View File
@@ -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.