lmxopcua/code-reviews/Driver.Mqtt.Browser/findings.md

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

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.