# 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.