Joseph Doherty
9ca6042f57
All 10 checklist categories completed for both modules. 12 findings total: 1 High + 2 Medium fixed under TDD (6eace4bb,31182137), 1 Medium fixed in the browser, 8 Low recorded Open with concrete recommendations. README.md deliberately NOT regenerated — regen-readme.py is left for the caller.
201 lines
14 KiB
Markdown
201 lines
14 KiB
Markdown
# Code Review — Driver.Mqtt.Browser
|
||
|
||
| Field | Value |
|
||
|---|---|
|
||
| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser` |
|
||
| Reviewer | Claude |
|
||
| Review date | 2026-08-10 |
|
||
| Commit reviewed | `b990adae` |
|
||
| Status | Reviewed |
|
||
| Open findings | 4 |
|
||
|
||
## Scope
|
||
|
||
The whole module: `MqttDriverBrowser.cs` (192 LOC), `MqttBrowseSession.cs` (1,213 LOC)
|
||
and the csproj. No generated code in this project.
|
||
|
||
## Checklist coverage
|
||
|
||
A comprehensive review completes every category, recording "No issues found" where
|
||
a category produced nothing rather than leaving it blank.
|
||
|
||
| # | Category | Result |
|
||
|---|---|---|
|
||
| 1 | Correctness & logic bugs | 2 findings — `Driver.Mqtt.Browser-001`, `-004` |
|
||
| 2 | OtOpcUa conventions | No issues found. The browser parses the driver's `DriverConfig` through the shared `MqttJson.Options` rather than a private `JsonSerializerOptions`, which is what keeps the picker from accepting a `mode`/`protocolVersion` spelling the deployed driver rejects (the repo's documented systemic enum bug). `DriverType` is taken from `DriverTypeNames.Mqtt`, not a literal. No authorization decision is made here — `BrowserSessionService` enforces `DriverOperator`, and the session explicitly holds no user identity, matching the "authorization at the server layer, never in driver-level code" rule. |
|
||
| 3 | Concurrency & thread safety | 2 findings — `Driver.Mqtt.Browser-003` and the ordering half of `-004`; both benign. The design is otherwise sound: the tree is built from `ConcurrentDictionary` with `StringComparer.Ordinal`, every mutation is lock-free, per-node observation records are swapped as whole references under `Volatile.Read`/`Write` so a UI thread never sees a half-written value, `_disposed` is an `Interlocked.Exchange` one-shot, and `Observe` is written to never throw on MQTTnet's dispatcher thread. |
|
||
| 4 | Error handling & resilience | No issues found. `OpenAsync` bounds connect + subscribe with a clamped budget (5–30 s) and disposes the session — which owns the client — on any failure. `DisposeAsync` is idempotent, safe on a never-connected client, bounds the courtesy DISCONNECT at 5 s and swallows-and-logs both teardown steps. `ExpandAsync` returns empty for an unknown node id rather than throwing, correctly treating a stale id as an ordinary race against a tree that grows while it is browsed. |
|
||
| 5 | Security | 1 finding — `Driver.Mqtt.Browser-001`. The module's central safety property is otherwise well built and genuinely checkable: every outbound message funnels through one private counted `PublishAsync`, reached only via a private nested `RebirthTransport` (so holding the session does not confer a publish-anything capability), and `PublishCountForTest` makes "browsing publishes nothing" assertable rather than aspirational. The Last-Will landmine is documented at length and guarded by a reflection test. Logs carry host/port/mode/filter only — never credentials or payload bodies. Metric names off the wire are **rejected** rather than scrubbed, precisely because a scrubbed name would be committed as a different binding key. |
|
||
| 6 | Performance & resource management | 1 finding — `Driver.Mqtt.Browser-005`. Bounds are otherwise thorough and correctly reasoned: `PayloadInspectMaxBytes` caps decode work per message (the node cap alone would not, since a large payload on a known topic creates no node), `MaxBirthPayloadBytes` bounds the protobuf parse, `Utf8Strict` is cached rather than allocated per message, and `Scrub` stack-allocates under 128 chars. |
|
||
| 7 | Design-document adherence | No issues found. Matches `docs/plans/2026-05-28-driver-browsers-design.md` and the MQTT design's §3.1/§4: births-only Sparkplug discovery (a DATA metric carries an alias and no name), node-level metrics hung directly under their edge node rather than under a synthesised "(node)" folder, no payload sniffing in Sparkplug mode, and `AddressFields` keyed by `MqttTagConfigKeys` so the commit mapper reads the address off fields rather than re-parsing a node id. The Gitea #512 layering fix is real and measured — the csproj comment even corrects its own earlier over-claim about `Core.dll`. |
|
||
| 8 | Code organization & conventions | No issues found. Two files, one responsibility each (open/connect vs. accumulate/serve). The `MetricSeparator` (`::`) decision — so a node-level metric `Plant1/EdgeA/Temperature` cannot be confused with a device folder of the same shape — is correct and, importantly, nothing re-parses it: the rebirth target and the address tuple are both **read** off the scope stamped on the node, mirroring the v3 `AddressSpaceRealm` discipline. |
|
||
| 9 | Testing coverage | No issues found. `MqttBrowseSessionTests` + `MqttBrowserLayeringTests` (in `Driver.Mqtt.Tests`, which reaches internals via `InternalsVisibleTo`) cover both tree shapes, the caps, the markers, rebirth scoping and the publish-count guarantee. The Sparkplug test seams feed a **real** encoded birth through the real `Observe` path — topic parse, protobuf decode and tree build all exercised rather than mocked around — which is why `-001` was reachable as a test at all. |
|
||
| 10 | Documentation & comments | 1 finding — the false claim documented in `Driver.Mqtt.Browser-001` was in a *sibling* file (`SparkplugRebirthScope`) asserting a guarantee this module did not provide; it is now true. Otherwise the XML docs are unusually good, and several carry the reasoning that makes the code reviewable at all (why a browse client id must differ from the driver's; why a device folder states its kind rather than inferring it from child count; why a DataSet cell is always `{column}/{row}`). |
|
||
|
||
## Findings
|
||
|
||
<!-- One ### entry per finding. IDs are <Module>-NNN, sequential within the module,
|
||
never reused. Findings are never deleted — close them by changing Status and
|
||
completing Resolution. -->
|
||
|
||
### Driver.Mqtt.Browser-001
|
||
|
||
| Field | Value |
|
||
|---|---|
|
||
| Severity | Medium |
|
||
| Category | Security |
|
||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:898`, `:712` |
|
||
| Status | Resolved |
|
||
|
||
**Description:** `ResolveRebirthTargets`' third branch — a bare `{group}/{edgeNode}` the
|
||
window has never observed, honoured on purpose because a node that has not birthed is the
|
||
prime rebirth target — gated its two segments on `IsUsableLabel`, which checks length and
|
||
control characters only. MQTT wildcards passed. A scope of `Plant1/#` therefore assembled
|
||
and published `spBv1.0/Plant1/NCMD/#`: a PUBLISH the MQTT spec forbids (MQTT-3.3.2-2),
|
||
which a conformant broker answers by closing the connection, killing the operator's browse
|
||
session mid-pick. `Plant1/+` is the worse shape — a broker that tolerates it would fan one
|
||
operator click across every matching edge node, the exact broker-storm `MaxGroupRebirthNodes`
|
||
exists to prevent, and with no cap applied because the group-enumeration branch was never
|
||
entered.
|
||
|
||
What makes this more than theoretical is the documented division of responsibility.
|
||
`SparkplugRebirthScope` (the AdminUI's typed input) rejects wildcards and states in its own
|
||
remarks that it "is advisory, not the gate" because *"`MqttBrowseSession.ResolveRebirthTargets`
|
||
re-resolves the scope and refuses an unusable one **before any message leaves**"* — a
|
||
guarantee this module did not actually provide. Anything reaching `BrowserSessionService`
|
||
without going through that Razor component (the API surface) had no wildcard check at all.
|
||
|
||
Branches 1 and 2 were never affected: their ids come off `SparkplugTopic.TryParse`, which
|
||
already rejects `+`/`#` per segment.
|
||
|
||
**Recommendation:** Reject wildcard segments on the raw-input branch. Do **not** tighten
|
||
`IsUsableLabel` itself — it also judges Sparkplug *metric* names, which are arbitrary
|
||
publisher-chosen UTF-8, are never part of a topic, and legitimately contain characters a
|
||
topic segment cannot; tightening it there would silently drop legitimate metrics from the
|
||
picker tree.
|
||
|
||
**Resolution:** Fixed in `31182137` (2026-08-10) — a separate `IsUsableRebirthSegment`
|
||
guards branch 3 (`IsUsableLabel` plus no `+`/`#`), leaving metric-name handling untouched;
|
||
pinned by `MqttBrowseSessionTests.RequestRebirthAsync_WildcardScope_PublishesNothing` over
|
||
four wildcard shapes, each asserting `PublishCountForTest == 0`.
|
||
|
||
### Driver.Mqtt.Browser-002
|
||
|
||
| Field | Value |
|
||
|---|---|
|
||
| Severity | Low |
|
||
| Category | Correctness & logic bugs |
|
||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:469` |
|
||
| Status | Open |
|
||
|
||
**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
|
||
through `IsUsableLabel`, whose doc explains the reason: "a metric name is arbitrary UTF-8
|
||
chosen by the publisher, and an embedded newline or ANSI escape would break the picker's
|
||
single-line rows." `ObservePlainTopic` applies only the length half of that (`MaxTopicChars`,
|
||
`MaxSegmentChars`) and no control-character check, so a plain topic segment goes into
|
||
`TopicNode.Segment` — the picker's displayed label — unfiltered. MQTT forbids only `U+0000`
|
||
in a topic name, so a publisher can legally put `\n` or an ANSI escape in one.
|
||
|
||
Related and even smaller: a topic with an empty segment (`a//b`, or a trailing `/`) creates a
|
||
node whose label is the empty string, rendering as a blank picker row. The committed address
|
||
is still correct — `AddressFields[topic]` carries `node.Path`, the true topic — so this is a
|
||
display defect, not a mis-binding.
|
||
|
||
**Recommendation:** Extend the existing pre-pass in `ObservePlainTopic` (which already walks
|
||
every segment to check length, specifically so a rejected topic leaves no half-built path
|
||
behind) to reject a segment containing a control character, setting the same `_oversized`
|
||
marker. The plain-mode marker text would need widening from "Some topics were too long 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)_
|
||
|
||
### Driver.Mqtt.Browser-003
|
||
|
||
| Field | Value |
|
||
|---|---|
|
||
| Severity | Low |
|
||
| Category | Concurrency & thread safety |
|
||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:685` |
|
||
| Status | Open |
|
||
|
||
**Description:** `GetOrCreate` reads `_nodeCount` and compares it to `_nodeCap`, then
|
||
increments after a successful `GetOrAdd`. The read and the increment are not atomic with
|
||
respect to each other, so N threads can each observe `_nodeCount == _nodeCap - 1` and all
|
||
proceed, overshooting the cap by up to N-1 nodes. MQTTnet delivers on a single dispatcher
|
||
per client, so N is 1 in practice today and the overshoot is unobservable; it becomes real
|
||
only if a future change fans observation across threads.
|
||
|
||
There is a second, already-documented race in the same method: a node is inserted into its
|
||
parent's `Children` before it is published to `_byPath`, so an `ExpandAsync` landing in that
|
||
window returns empty for a node the tree already contains. The class remarks accept this
|
||
explicitly ("a stale id is an ordinary race, not an error") and it self-heals on the next
|
||
call, so it is noted rather than filed.
|
||
|
||
**Recommendation:** If the concurrency assumption ever changes, replace the read/increment
|
||
pair with a single `Interlocked.Increment` compared against the cap, decrementing on the
|
||
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)_
|
||
|
||
### Driver.Mqtt.Browser-004
|
||
|
||
| Field | Value |
|
||
|---|---|
|
||
| Severity | Low |
|
||
| Category | Correctness & logic bugs |
|
||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:1079` |
|
||
| Status | Open |
|
||
|
||
**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
|
||
Sparkplug mode two siblings *can* share a `Segment`: a device folder `Plant1/EdgeA/Temp` and
|
||
an edge-node-level metric `Plant1/EdgeA::Temp` are different nodes with the same label.
|
||
`OrderBy` is stable with respect to the input sequence, but the input is
|
||
`ConcurrentDictionary.Values`, whose enumeration order is not guaranteed and can change as
|
||
the tree grows. So two such siblings can swap places between successive `ExpandAsync` calls
|
||
— the picker's list reorders under the operator mid-browse, and the two rows are visually
|
||
identical, so it is not obvious which one is being clicked.
|
||
|
||
The comment on `Project` claims "ordered so the picker is stable", which is only true when
|
||
labels are unique.
|
||
|
||
**Recommendation:** Make the sort total by adding `.ThenBy(n => n.Path, StringComparer.Ordinal)`,
|
||
and/or order folders before leaves so a device and a same-named metric are visually
|
||
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)_
|
||
|
||
### Driver.Mqtt.Browser-005
|
||
|
||
| Field | Value |
|
||
|---|---|
|
||
| Severity | Low |
|
||
| Category | Performance & resource management |
|
||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:59`, `:81` |
|
||
| Status | Open |
|
||
|
||
**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`
|
||
(up to `MaxTopicChars` = 1,024 chars), its `Segment` (up to 256) and a snippet (up to 64),
|
||
plus its own `ConcurrentDictionary`. Every node is also held twice — once in its parent's
|
||
`Children`, once in the flat `_byPath` index. A worst case near those bounds is on the order
|
||
of 100+ MB for a single browse window, and a plain-mode open with no `TopicPrefix`
|
||
subscribes `#`, i.e. the entire broker, which is exactly the shape that would approach it.
|
||
Several concurrent pickers multiply it.
|
||
|
||
This is bounded and self-limiting, not a leak — `DisposeAsync` clears both maps and the
|
||
session reaper (outside this module) bounds lifetime — and the `MaxTopicChars` remarks show
|
||
the authors reasoned about precisely this ("50 000 nodes near that ceiling is a multi-GB
|
||
tree"). What is missing is any bound on the *aggregate* across sessions.
|
||
|
||
**Recommendation:** No code change needed in this module. Worth confirming that
|
||
`BrowserSessionService` caps concurrent sessions per user and that its idle reaper interval
|
||
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)_
|