Fix connection lifecycle: transfer-before-dispose reconnect, async connect teardown, truthful config load

[BrettKinny]

Summary

Fixes the three connection-lifecycle gate items from the v1.0 follow-up review (docs/V1-REVIEW-FOLLOWUP.md, items 2–4, plus the related subscription-init swallow).

• Transfer-before-dispose on reconnect: TryRecreateSessionAsync previously disposed the old session before transferring subscriptions. The client-side half of TransferSubscriptionsAsync must detach each subscription from its previous session, which threw on the disposed session — after the server-side transfer had already succeeded — so the SDK reported failure and the app recreated a duplicate subscription set, orphaning the transferred ones on the server on every hard reconnect. The new session is now created and the transfer completed while the old session is still alive; the old session is disposed afterwards (still without CloseSession, so untransferred server-side subscriptions are not deleted). A failure during session creation now leaves existing state untouched for the next retry.
• No more UI-thread freeze on re-connect: ConnectionManager.ConnectAsync and OpcUaClientWrapper.ConnectAsync (including its error paths) now await DisconnectAsync() instead of calling the synchronous Disconnect(), which blocked the calling (UI) thread on the OPC UA close round-trip — up to the 30 s transport timeout when connecting away from a dead server. The sync Disconnect() remains for back-compat callers.
• Connected means usable: ConnectionManager.ConnectAsync no longer ignores the result of subscription initialization. If the server refuses the monitoring subscription, it raises ConnectionError, disconnects, and returns false instead of reporting Connected on a session that can never monitor anything.
• Truthful config load: MainWindow.LoadConfigurationAsync now tears down the old session up front via the recording-aware DisconnectAsync() (stops a live CSV recording — no more zombie REC indicator — and clears the views, so no dead rows survive). On a failed connect or a cancelled password prompt, the config service is reset to untitled so Ctrl+S can no longer silently overwrite the newly selected file with stale session state, and the failure dialog no longer claims "the previous connection and data have been preserved" when they weren't.

Behavior changes

• A config load that fails to connect now leaves the app cleanly disconnected and untitled, instead of showing stale data with a misleading dialog. Re-loading the previous file restores the old session.
• A connect whose subscription creation fails now surfaces as a failed connect rather than a silent half-connected state.

Test plan

• dotnet build Opcilloscope.sln -c Release — 0 warnings, 0 errors
• dotnet test -c Release — 613/613 passing, including the existing ReconnectIntegrationTests which exercise the recreate path
• Manual: kill the server mid-session → auto-reconnect resumes without duplicated subscriptions in server diagnostics; connect to a new server while the old one is dead → UI stays responsive; load a config with a bad endpoint while connected and recording → recording stopped, views cleared, dialog truthful, Ctrl+S prompts Save As

---

Part of the v1 follow-up punch list (docs/V1-REVIEW-FOLLOWUP.md, items 2–4).

https://claude.ai/code/session_012Vopnd9vWkzELveHRgZhie

---

Generated by Claude Code

[claude]

Code Review

This PR addresses four real, interconnected connection-lifecycle bugs. The root-cause analysis in the description is accurate and the fixes are generally correct. A few items worth discussing:

---

Transfer-before-dispose (OpcUaClientWrapper.cs)

The fix is correct. Moving _session = newSession and the KeepAlive registration before TransferSubscriptionsAsync is the right ordering. The SDK's client-side transfer logic needs to detach subscriptions from their prior session object, so that session must remain alive during the call.

Potential resource leak — worth fixing before merge.

After _session = newSession is assigned, if TransferSubscriptionsAsync throws (or if a future caller interrupts), the outer catch (Exception ex) block fires. At that point oldSession is a local variable going out of scope — never disposed. The outer catch block wasn't modified in this PR, so it has no reference to oldSession.

The fix is straightforward: wrap the transfer + old-session disposal in a try/finally:

_session = newSession;
try
{
    if (subscriptionsToTransfer != null && subscriptionsToTransfer.Count > 0)
    {
        var transferred = await TransferSubscriptionsAsync(subscriptionsToTransfer);
        _logger.Info($"Transferred {transferred} of {subscriptionsToTransfer.Count} subscription(s)");
    }
}
finally
{
    if (oldSession != null)
    {
        oldSession.KeepAlive -= Session_KeepAlive;
        try { oldSession.Dispose(); }
        catch (Exception ex) { _logger.Warning($"Session cleanup error during reconnection: {ex.Message}"); }
    }
}
return newSession.Connected;

In practice TransferSubscriptionsAsync is unlikely to throw (the SDK typically absorbs failures and returns 0), so this is low-severity, but it's a correctness hole the refactor introduced.

Minor: double KeepAlive registration window. Between newSession.KeepAlive += Session_KeepAlive and the deferred oldSession.KeepAlive -=, both sessions share the handler. On a dead transport the old session won't fire KeepAlive anyway, but moving oldSession.KeepAlive -= Session_KeepAlive to immediately after capturing oldSession (before creating the new session) would close the window with no downside.

---

Async disconnect (ConnectAsync error paths)

await DisconnectAsync() in place of Disconnect() is the right fix across all three call sites. No concerns here.

---

Truthful subscription-init failure (ConnectionManager.cs)

The return-value threading from SubscriptionManager.InitializeAsync through InitializeSubscriptionAsync to ConnectAsync is clean. The fail path (fire ConnectionError, await DisconnectAsync(), return false) correctly sequences error notification before teardown, so the UI receives a descriptive message before seeing the Disconnected state change.

One small note: the event handlers are wired to _subscriptionManager even when initialized == false, then immediately cleaned up by the subsequent DisconnectAsync(). That's harmless, but conditionally skipping the wire-up when !initialized would make the intent clearer.

---

Config load teardown (MainWindow.cs)

The reordering (disconnect before connect attempt) is the right fix for the zombie REC indicator and stale rows. The updated error message is truthful.

UX trade-off to be aware of: Previously a failed new-server connect preserved the old session; now it leaves the user fully disconnected. This is acknowledged in the PR description and the error text is updated — just flagging it for visibility in case the team wants it called out in release notes.

_configService.Reset() on password cancel correctly prevents Ctrl+S from overwriting the newly selected file with stale state. One subtle side-effect: it also loses the pointer to the previously open file, so Ctrl+S will prompt "Save As" even if the user had a clean save state before the attempted load. Not a bug, but something to document if users report confusion.

---

Summary

Finding	Severity	File
oldSession may leak if transfer throws (not covered by outer catch)	Medium	OpcUaClientWrapper.cs
Double KeepAlive registration window during transfer	Low	OpcUaClientWrapper.cs
Event handlers wired on failed subscription init	Cosmetic	ConnectionManager.cs
_configService.Reset() loses prior file reference on cancel	Low/UX	MainWindow.cs

The core logic of all four fixes is sound. The medium-severity leak is the only item I'd want resolved before merge — the rest are minor polish. Happy to look at a follow-up addressing the try/finally guard if the team agrees.

[claude]

Code Review

PR: Fix connection lifecycle: transfer-before-dispose reconnect, async connect teardown, truthful config load
Files changed: 3 | +89 / -58

---

Overview

This PR fixes four genuine bugs in the connection lifecycle, all well-described in the PR body. The changes are surgical and the motivations are clearly documented in both the description and inline comments. The PR description itself is exemplary.

---

What's Good

• Transfer-before-dispose fix in TryRecreateSessionAsync is structurally correct: the new session is created first so a creation failure leaves old state intact, the transfer runs while the old session is still alive, and the finally block retires the old session regardless of transfer outcome. The comment explaining why (TransferSubscriptionsAsync throws on a disposed session after the server-side transfer already succeeded) is exactly the kind of non-obvious reasoning worth preserving.
• InitializeSubscriptionAsync now returns bool — swallowing the result was a real silent-failure bug. Propagating it and surfacing it as a ConnectionError is the right fix.
• _currentMetadata assignment moved to after a successful connect is a clean correctness improvement: stale metadata can no longer survive a failed load.
• The error message change ("The previous connection and data have been preserved." → "The previous connection has been closed.") is importantly more truthful and worth the change on its own.

---

Issues / Suggestions

1. KeepAlive wired before _session is updated (minor timing hazard)

In TryRecreateSessionAsync:

newSession.KeepAlive += Session_KeepAlive;
_session = newSession;          // handler can fire here and see the old _session

If the keep-alive timer fires in that narrow window, Session_KeepAlive sees _session still pointing at the old (soon-to-be-disposed) session. In practice, KeepAlive fires on an interval so this window is tiny, but the safe order is assign first, then wire:

_session = newSession;
newSession.KeepAlive += Session_KeepAlive;

2. Transfer failure is indistinguishable from session-creation failure at the call site

If TransferSubscriptionsAsync throws inside the try/finally, the outer catch (Exception ex) catches it and returns false. The logger records the error, but the caller sees a bare false — the same as a session-creation failure. It may be worth logging the exception message in the catch block at a level that distinguishes the two failure modes, or ensuring the existing outer catch message is specific enough.

3. No new automated tests for the subscription-failure path

The new branch in ConnectionManager.ConnectAsync — where InitializeSubscriptionAsync returns false — raises ConnectionError, calls DisconnectAsync, and returns false. This is a meaningful behavior change with no unit test coverage. A test that stubs SubscriptionManager.InitializeAsync to return false and asserts that (a) the returned value is false, (b) ConnectionError was raised, and (c) the state is Disconnected would lock in the new contract. The existing integration tests don't exercise this path since the in-process test server won't refuse subscriptions.

4. DisconnectAsync() before password dialog (non-issue, confirming intent)

In LoadConfigurationAsync, DisconnectAsync is called after the password dialog, not before. A user who cancels the password prompt gets _configService.Reset() and an early return — the current session is intentionally preserved. This is correct and the comment confirms it. Just noting it as expected behavior.

---

Summary

The fixes are correct, well-motivated, and the inline documentation is high quality. Two actionable items:

• Should address: Add a unit test for the InitializeSubscriptionAsync → false path in ConnectAsync.
• Nice to fix: Swap the KeepAlive wire and _session assignment order for safety.

The transfer-before-dispose fix in particular is a non-trivial correctness win that's easy to get wrong — the approach here gets it right.