From a78740c44cdbc6c3653f128f64d6aae2a17b4fa0 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 10 Jun 2026 15:18:32 +0000 Subject: [PATCH 1/2] Fix connection lifecycle: transfer-before-dispose reconnect, async connect teardown, truthful config load - TryRecreateSessionAsync now creates the new session and transfers subscriptions while the old session is still alive, then disposes it. Previously the old session was disposed first, so the client-side half of TransferSubscriptionsAsync always threw on the disposed session after the server-side transfer had already succeeded - every hard reconnect orphaned the transferred subscriptions on the server and fell back to creating a duplicate set. - ConnectAsync (wrapper and ConnectionManager) now awaits DisconnectAsync instead of calling the synchronous Disconnect(), which blocked the UI thread on the close round-trip - up to the 30s transport timeout when reconnecting over a dead server. - ConnectionManager.ConnectAsync no longer reports Connected when the monitoring subscription cannot be created: it raises ConnectionError, disconnects, and returns false instead of silently leaving a session that can never monitor anything. - LoadConfigurationAsync tears down the old session up front via the recording-aware DisconnectAsync (no more zombie REC indicator, no dead table rows), and on a failed connect or cancelled password prompt reverts the config service to untitled so Ctrl+S can no longer overwrite the newly selected file with stale session state. The failure dialog no longer claims the previous connection was preserved. https://claude.ai/code/session_012Vopnd9vWkzELveHRgZhie --- App/MainWindow.cs | 42 ++++++++++++---------- OpcUa/ConnectionManager.cs | 23 ++++++++++--- OpcUa/OpcUaClientWrapper.cs | 69 ++++++++++++++++++++----------------- 3 files changed, 79 insertions(+), 55 deletions(-) diff --git a/App/MainWindow.cs b/App/MainWindow.cs index efc81a5..a96a158 100644 --- a/App/MainWindow.cs +++ b/App/MainWindow.cs @@ -1207,9 +1207,6 @@ private async Task LoadConfigurationAsync(string filePath) var config = await _configService.LoadAsync(filePath); - // Store metadata - _currentMetadata = config.Metadata; - // Connect to server and subscribe to nodes if (!string.IsNullOrEmpty(config.Server.EndpointUrl)) { @@ -1226,6 +1223,11 @@ private async Task LoadConfigurationAsync(string filePath) if (!pwDialog.Confirmed) { + // Nothing was torn down, but the load already switched the Ctrl+S + // target to this file; revert to untitled so a save cannot write + // the still-running session's state over it. + _configService.Reset(); + UpdateWindowTitle(); _logger.Info("Password prompt cancelled - skipping connection"); return; } @@ -1236,16 +1238,17 @@ private async Task LoadConfigurationAsync(string filePath) pwDialog.Password); } + // Tear down the current session first: stops any active recording and + // clears the views, so the UI cannot keep showing dead rows from the + // old server while (or after) the new connection is attempted. + await DisconnectAsync(); + var connected = await _connectionManager.ConnectAsync(config.Server.EndpointUrl, config.Settings.PublishingIntervalMs, credentials); if (connected) { _lastEndpoint = config.Server.EndpointUrl; - - // Only after successful connection, disconnect old and clear views - _addressSpaceView.Clear(); - _monitoredVariablesView.Clear(); - _nodeDetailsView.Clear(); + _currentMetadata = config.Metadata; _addressSpaceView.Initialize(_connectionManager.NodeBrowser); @@ -1271,24 +1274,25 @@ private async Task LoadConfigurationAsync(string filePath) } else { + // Revert to untitled: leaving the failed file as the Ctrl+S target + // would let a save overwrite it with the now-empty session state. + _configService.Reset(); + _currentMetadata = null; + UpdateWindowTitle(); + _logger.Error($"Failed to connect to {config.Server.EndpointUrl}"); - MessageBox.ErrorQuery("Connection Failed", - $"Could not connect to server:\n{config.Server.EndpointUrl}\n\nThe previous connection and data have been preserved.", + MessageBox.ErrorQuery("Connection Failed", + $"Could not connect to server:\n{config.Server.EndpointUrl}\n\nThe previous connection has been closed. Use Connect to reconnect.", "OK"); } } else { - // No endpoint URL, just clear views and apply settings - if (_connectionManager.IsConnected) - { - await _connectionManager.DisconnectAsync(); - } + // No endpoint URL: tear down any current session (stops recording, + // clears views) and just adopt the loaded settings. + await DisconnectAsync(); - _addressSpaceView.Clear(); - _monitoredVariablesView.Clear(); - _nodeDetailsView.Clear(); - + _currentMetadata = config.Metadata; _recentFiles.Add(filePath); UpdateWindowTitle(); _logger.Info("Configuration loaded (no server connection)"); diff --git a/OpcUa/ConnectionManager.cs b/OpcUa/ConnectionManager.cs index b96c877..b958538 100644 --- a/OpcUa/ConnectionManager.cs +++ b/OpcUa/ConnectionManager.cs @@ -123,7 +123,10 @@ public async Task ConnectAsync( string? securityMode = null, string? securityPolicy = null) { - Disconnect(); + // Async teardown: the synchronous Disconnect() blocks on the OPC UA close + // round-trip (up to the transport timeout against a dead server), which froze + // the UI thread when reconnecting over an existing or dead connection. + await DisconnectAsync(); _lastEndpoint = endpoint; _credentials = credentials ?? ConnectionCredentials.Anonymous; @@ -135,7 +138,17 @@ public async Task ConnectAsync( if (success) { - await InitializeSubscriptionAsync(publishingInterval); + if (!await InitializeSubscriptionAsync(publishingInterval)) + { + // Without a subscription the session is useless for monitoring; + // fail the connect rather than reporting Connected. + var msg = "Connected, but the server refused the monitoring subscription. Disconnecting."; + _logger.Error(msg); + ConnectionError?.Invoke(msg); + await DisconnectAsync(); + return false; + } + StateChanged?.Invoke(ConnectionState.Connected); } else @@ -300,11 +313,11 @@ public Task UnsubscribeAsync(uint clientHandle) return _client.WriteValueAsync(nodeId, value); } - private async Task InitializeSubscriptionAsync(int publishingInterval = 250) + private async Task InitializeSubscriptionAsync(int publishingInterval = 250) { _subscriptionManager = new SubscriptionManager(_client, _logger); _subscriptionManager.PublishingInterval = publishingInterval; - await _subscriptionManager.InitializeAsync(); + var initialized = await _subscriptionManager.InitializeAsync(); // Store handler references for proper unsubscription _valueChangedHandler = node => ValueChanged?.Invoke(node); @@ -314,6 +327,8 @@ private async Task InitializeSubscriptionAsync(int publishingInterval = 250) _subscriptionManager.ValueChanged += _valueChangedHandler; _subscriptionManager.VariableAdded += _variableAddedHandler; _subscriptionManager.VariableRemoved += _variableRemovedHandler; + + return initialized; } private void DisposeSubscription() diff --git a/OpcUa/OpcUaClientWrapper.cs b/OpcUa/OpcUaClientWrapper.cs index 50fcf5e..add7b16 100644 --- a/OpcUa/OpcUaClientWrapper.cs +++ b/OpcUa/OpcUaClientWrapper.cs @@ -181,7 +181,7 @@ public async Task ConnectAsync( { try { - Disconnect(); + await DisconnectAsync(); _credentials = credentials ?? ConnectionCredentials.Anonymous; _securityMode = securityMode; @@ -231,14 +231,14 @@ public async Task ConnectAsync( : "Authentication rejected by server: " + sre.Message; _logger.Error(msg); ConnectionError?.Invoke(msg); - Disconnect(); + await DisconnectAsync(); return false; } catch (Exception ex) { _logger.Error($"Connection failed: {ex.Message}"); ConnectionError?.Invoke(ex.Message); - Disconnect(); + await DisconnectAsync(); return false; } } @@ -406,42 +406,26 @@ private async Task TryRecreateSessionAsync() var config = await GetApplicationConfigAsync(); - // Capture existing subscriptions before closing old session + var oldSession = _session; + + // Capture existing subscriptions before replacing the session SubscriptionCollection? subscriptionsToTransfer = null; - if (_session?.Subscriptions != null && _session.Subscriptions.Any()) + if (oldSession?.Subscriptions != null && oldSession.Subscriptions.Any()) { - subscriptionsToTransfer = new SubscriptionCollection(_session.Subscriptions); + subscriptionsToTransfer = new SubscriptionCollection(oldSession.Subscriptions); _logger.Info($"Captured {subscriptionsToTransfer.Count} subscription(s) for transfer"); } - // Clean up old session without deleting subscriptions on server - if (_session != null) - { - _session.KeepAlive -= Session_KeepAlive; - try - { - // Dispose without CloseAsync() - this intentionally skips sending CloseSession - // to the server, allowing server-side subscriptions to remain active for transfer. - // With DeleteSubscriptionsOnClose=false, we want the subscriptions to persist - // on the server so we can transfer them to the new session. - _session.Dispose(); - } - catch (Exception ex) - { - _logger.Warning($"Session cleanup error during reconnection: {ex.Message}"); - } - _session = null; - } - // Rediscover endpoint in case server configuration changed var selectedEndpoint = await DiscoverAndSelectEndpointAsync(config, _currentEndpoint, _securityMode, _securityPolicy); var endpointConfig = EndpointConfiguration.Create(config); var endpoint = new ConfiguredEndpoint(null, selectedEndpoint, endpointConfig); _lastConfiguredEndpoint = endpoint; - // Create new session + // Create the new session before touching the old one, so a failure here + // leaves the existing state unchanged for the next retry attempt. #pragma warning disable CS0618 - _session = await Opc.Ua.Client.Session.Create( + var newSession = await Opc.Ua.Client.Session.Create( config, endpoint, false, @@ -452,18 +436,39 @@ private async Task TryRecreateSessionAsync() ); #pragma warning restore CS0618 - _session.DeleteSubscriptionsOnClose = false; - _session.TransferSubscriptionsOnReconnect = true; - _session.KeepAlive += Session_KeepAlive; + newSession.DeleteSubscriptionsOnClose = false; + newSession.TransferSubscriptionsOnReconnect = true; + newSession.KeepAlive += Session_KeepAlive; + _session = newSession; - // Transfer subscriptions to new session + // Transfer subscriptions while the old session is still alive. The client-side + // half of TransferSubscriptionsAsync detaches each subscription from its previous + // session; if that session is already disposed this throws, the SDK swallows it + // and reports failure - after the server-side transfer already succeeded - leaving + // orphaned subscriptions on the server and forcing a duplicate recreate. if (subscriptionsToTransfer != null && subscriptionsToTransfer.Count > 0) { var transferred = await TransferSubscriptionsAsync(subscriptionsToTransfer); _logger.Info($"Transferred {transferred} of {subscriptionsToTransfer.Count} subscription(s)"); } - return _session.Connected; + // Retire the old session. Dispose without CloseAsync() - sending CloseSession + // would delete any server-side subscriptions that were not transferred, and the + // transport is typically already dead on this path. + 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; } catch (Exception ex) { From ee7302bbff2e8632cbd014e5331c6d194d24c627 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 11 Jun 2026 02:52:14 +0000 Subject: [PATCH 2/2] Dispose old session in finally during session recreation Once _session points at the new session, an exception from the subscription transfer would leave the old session undisposed - the outer catch has no reference to it. https://claude.ai/code/session_012Vopnd9vWkzELveHRgZhie --- OpcUa/OpcUaClientWrapper.cs | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/OpcUa/OpcUaClientWrapper.cs b/OpcUa/OpcUaClientWrapper.cs index add7b16..a68c12c 100644 --- a/OpcUa/OpcUaClientWrapper.cs +++ b/OpcUa/OpcUaClientWrapper.cs @@ -446,25 +446,32 @@ private async Task TryRecreateSessionAsync() // session; if that session is already disposed this throws, the SDK swallows it // and reports failure - after the server-side transfer already succeeded - leaving // orphaned subscriptions on the server and forcing a duplicate recreate. - if (subscriptionsToTransfer != null && subscriptionsToTransfer.Count > 0) - { - var transferred = await TransferSubscriptionsAsync(subscriptionsToTransfer); - _logger.Info($"Transferred {transferred} of {subscriptionsToTransfer.Count} subscription(s)"); - } - - // Retire the old session. Dispose without CloseAsync() - sending CloseSession - // would delete any server-side subscriptions that were not transferred, and the - // transport is typically already dead on this path. - if (oldSession != null) + try { - oldSession.KeepAlive -= Session_KeepAlive; - try + if (subscriptionsToTransfer != null && subscriptionsToTransfer.Count > 0) { - oldSession.Dispose(); + var transferred = await TransferSubscriptionsAsync(subscriptionsToTransfer); + _logger.Info($"Transferred {transferred} of {subscriptionsToTransfer.Count} subscription(s)"); } - catch (Exception ex) + } + finally + { + // Retire the old session even if the transfer throws - once _session + // points at the new session the old one would otherwise leak. Dispose + // without CloseAsync() - sending CloseSession would delete any + // server-side subscriptions that were not transferred, and the + // transport is typically already dead on this path. + if (oldSession != null) { - _logger.Warning($"Session cleanup error during reconnection: {ex.Message}"); + oldSession.KeepAlive -= Session_KeepAlive; + try + { + oldSession.Dispose(); + } + catch (Exception ex) + { + _logger.Warning($"Session cleanup error during reconnection: {ex.Message}"); + } } }