Harden OPC UA layer: reconnect races, handle allocation, interval/security wiring, status semantics

[BrettKinny]

Summary

Stacked on #167 (fix/connection-lifecycle-2) — review/merge that first; this PR will retarget to main once it merges.

Hardens the OPC UA layer per the v1.0 follow-up review (docs/V1-REVIEW-FOLLOWUP.md, items 10–12 and the interval/security/status lows):

• Reconnect/disconnect races closed: ReconnectAsync now works against a locally captured CancellationTokenSource; the field swap in DisposeReconnectCts happens under a lock (no more double-dispose/NRE TOCTOU with a concurrent Disconnect), and if the user disconnects while a session recreate is in flight, the freshly created session is closed instead of living on as a ghost connection behind a "Disconnected" UI.
• Client-handle allocation race: AddNodeAsync allocates _nextClientHandle inside the dedup lock — the unsynchronized ++ let two concurrent adds of different nodes collide on one handle and silently overwrite each other's entries in the three handle-keyed dictionaries (same TOCTOU class PR Harden SubscriptionManager: dedup race, handler leaks, dead field #158 closed for the NodeId dedup, one line away).
• No more stranded rows: the last-resort restore path (recreate failed → fresh subscription) now raises VariableRemoved for every lost node, so the UI no longer shows rows stuck at "(reconnecting...)" with handles the new subscription manager knows nothing about.
• settings.samplingIntervalMs is finally honored — it was a documented no-op (hardcoded 250 in both AddNodeAsync and RecreateSubscriptionsAsync). SubscriptionManager gains a clamped SamplingInterval property; ConnectionManager stores both intervals from ConnectAsync and reuses them when subscriptions are restored after reconnect (restores previously fell back to the 250 ms defaults too).
• Config security settings wired (closes the declared loose end from PR Fix connection lifecycle: auto-reconnect, secure-by-default certs, endpoint selection, async disconnect #160): MainWindow.LoadConfigurationAsync now passes securityMode/securityPolicy/samplingIntervalMs from the config file to ConnectAsync. A warning is logged when credentials are about to be sent over a SecurityMode=None endpoint (the other declared loose end).
• Status semantics: MonitoredNode treats Good-with-info-bits codes (e.g. GoodClamped 0x00300000) as Good — severity is the top two bits, not == 0 — so such values no longer render as raw hex with non-good row styling.

Test plan

• dotnet build Opcilloscope.sln -c Release — 0 warnings, 0 errors
• dotnet test -c Release — 620/620 passing (7 new tests: Good-substatus semantics, SamplingInterval clamping)
• Manual: load a config with samplingIntervalMs: 5000 → server diagnostics show 5000 ms sampling; disconnect during an auto-reconnect → no ghost session

---

Part of the v1 follow-up punch list (docs/V1-REVIEW-FOLLOWUP.md, items 10–12 + lows).

https://claude.ai/code/session_012Vopnd9vWkzELveHRgZhie

---

Generated by Claude Code

[claude]

Code Review

This is a solid hardening PR that closes several real races and wiring gaps. The fixes are well-targeted and the test coverage is good.

---

What's done well

• DisposeReconnectCts lock + local capture — the textbook fix for the TOCTOU double-dispose. Nulling the field under the lock and then operating on the captured reference outside is exactly right.
• Handle allocation inside the dedup lock — same lock, one line move, closes the collision race cleanly. No new lock needed.
• VariableRemoved for the last-resort path — collecting handles before DisposeSubscription() and firing them after is correct order.
• Status severity bit mask — (StatusCode & 0xC0000000) == 0 is the correct OPC UA Part 4 test. The old == 0 would misfire on any Good-with-sub-status code (e.g. GoodClamped 0x00300000).
• Test quality — boundary tests for SamplingInterval clamping and the GoodClamped semantics tests are exactly what's needed.

---

Issues

1. Narrow reconnect/disconnect race remains

In ReconnectAsync there is a window between DisposeReconnectCts() and the lock-guarded field assignment:

DisposeReconnectCts();                    // clears _reconnectCts to null
var cts = new CancellationTokenSource();  // Disconnect() can fire HERE
lock (_reconnectCtsLock) { _reconnectCts = cts; }
var token = cts.Token;

If Disconnect calls DisposeReconnectCts in that window it sees _reconnectCts == null and does nothing, so the new cts is never cancelled. The reconnect loop then proceeds with an un-cancelled token even though the user disconnected — same ghost-session class as the bug described in the PR, just narrower. The cancellation check after TryRecreateSessionAsync only helps if the CTS actually got cancelled.

One safe fix: keep the entire create-and-assign sequence inside a single lock acquisition so Disconnect either races before the whole block or cancels the fresh CTS, with no gap in between.

2. Potential null dereference in RestoreSubscriptionsAsync

var lostHandles = _subscriptionManager.MonitoredVariables
    .Select(v => v.ClientHandle)
    .ToList();

_subscriptionManager should be non-null here in normal flow, but this is a last-resort error recovery path. A null-conditional (_subscriptionManager?.MonitoredVariables ?? []) or an early return would guard it defensively.

3. Leftover blank line in SubscriptionManager.cs

After moving the handle allocation out of the try block, the try { opening is now followed by a blank line before the first comment. Minor, but easy to clean up.

---

Questions / suggestions

• SamplingInterval upper bound of 60 s: consistent with the existing PublishingInterval cap (10 s), but some industrial sensors run at 1-minute or longer intervals. Worth a comment in the property summary if 60 s is a deliberate policy limit rather than an oversight.
• StatusString special-case: the if (StatusCode == 0) return "Good" fast-path is fine. Could fall through to the new Good (0x...) branch for consistency, but not a bug.
• Security warning only logs: appropriate level for the declared loose end from PR Fix connection lifecycle: auto-reconnect, secure-by-default certs, endpoint selection, async disconnect #160. Worth confirming no UI-level escalation is planned for v1.

---

Summary

The three core fixes (CTS dispose race, handle allocation race, stranded UI rows) are correct and the sampling/security wiring fills genuine gaps. Item 1 above is in the same bug class as what's being fixed and worth sealing before merge. Items 2 and 3 are low-risk and easy to address.