Joseph Doherty
2dc2075d05
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.
264 lines
20 KiB
Markdown
264 lines
20 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 | 0 |
|
||
|
||
## 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 | 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
|
||
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 (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
|
||
|
||
| Field | Value |
|
||
|---|---|
|
||
| Severity | Low |
|
||
| Category | Concurrency & thread safety |
|
||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:685` |
|
||
| 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
|
||
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 (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
|
||
|
||
| Field | Value |
|
||
|---|---|
|
||
| Severity | Low |
|
||
| Category | Correctness & logic bugs |
|
||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:1079` |
|
||
| 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
|
||
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 (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
|
||
|
||
| Field | Value |
|
||
|---|---|
|
||
| Severity | Low |
|
||
| Category | Performance & resource management |
|
||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:59`, `:81` |
|
||
| 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`
|
||
(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 (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.
|