fix(signaling): cancel in-flight connect on disconnect (#1093)

* fix(signaling): cancel in-flight connect on disconnect via generation counter

disconnect() returned early when _client==null (connect still awaiting
factory), leaving _connecting=true and the in-flight _connectAsync
running. The next connect() was silently dropped, and if the stale
factory eventually resolved, the module set _client and emitted
SignalingConnected even though disconnect() had been called.

Root cause: the guard in disconnect() (`if (client == null) return`)
prevented it from resetting _connecting or invalidating the async task.

Fix: introduce a monotonically increasing _generation counter.
- connect() captures the current generation before starting _connectAsync
- disconnect() increments _generation and resets _connecting=false
- _connectAsync checks its generation at every suspension point; if it
  no longer matches, it cleans up its client without emitting events
- the finally block only resets _connecting for the current generation

Adds four tests reproducing the race: two document the broken behavior
(expected to pass after the fix), two verify correct behavior.

* fix(signaling): add diagnostic logs for cancelled in-flight connect

* refactor(signaling): replace generation counter with Object identity token

* fix(signaling): address Copilot review comments

* fix(signaling): restore library directive to silence dangling doc comment warning

Author: SERDUN

packages/webtrit_signaling_service/webtrit_signaling_service_platform_interface/lib/src/signaling_module_impl.dart

@@ -111,7 +111,14 @@ class SignalingModuleImpl implements SignalingModule {
 
   WebtritSignalingClient? _client;
   bool _disposed = false;
-  bool _connecting = false;
+
+  /// Identity token for the active connect attempt.
+  ///
+  /// Non-null while a [_connectAsync] is in progress; null when idle.
+  /// [connect] creates a fresh [Object] and passes it to [_connectAsync].
+  /// [disconnect] sets it to null, which [_connectAsync] detects after each
+  /// suspension point — the "latest wins" pattern for non-cancellable Futures.
+  Object? _connectToken;
 
   /// Completer resolved by [_onDisconnect] after a graceful [disconnect] call.
   Completer<void>? _disconnectAck;
@@ -181,15 +188,22 @@ class SignalingModuleImpl implements SignalingModule {
   /// Clears the session buffer on each call.
   @override
   void connect() {
-    if (_disposed || _connecting) return;
+    if (_disposed || _connectToken != null) return;
+    final token = _connectToken = Object();
     _eventBuffer.clear();
-    unawaited(_connectAsync());
+    unawaited(_connectAsync(token));
   }
 
   @override
   Future<void> disconnect() async {
+    final hadInFlightConnect = _connectToken != null;
+    _connectToken = null; // invalidate any in-flight _connectAsync
+
     final client = _client;
-    if (client == null) return;
+    if (client == null) {
+      if (hadInFlightConnect) _logger.fine('disconnect: in-flight connect cancelled');
+      return;
+    }
     _client = null;
     _intentionalDisconnect = true;
     _disconnectAck = Completer<void>();
@@ -220,9 +234,7 @@ class SignalingModuleImpl implements SignalingModule {
   // Internal
   // ---------------------------------------------------------------------------
 
-  Future<void> _connectAsync() async {
-    if (_connecting) return;
-    _connecting = true;
+  Future<void> _connectAsync(Object connectToken) async {
     try {
       final existing = _client;
       if (existing != null) {
@@ -234,7 +246,7 @@ class SignalingModuleImpl implements SignalingModule {
         }
       }
 
-      if (_disposed) return;
+      if (_connectToken != connectToken || _disposed) return;
 
       _emit(SignalingConnecting());
 
@@ -249,11 +261,14 @@ class SignalingModuleImpl implements SignalingModule {
           force: true,
         );
 
-        if (_disposed) {
+        if (_connectToken != connectToken || _disposed) {
+          // This connect was superseded by disconnect() or a newer connect() —
+          // clean up the client without emitting events.
+          _logger.fine('_connectAsync: stale connect discarded');
           try {
             await client.disconnect(SignalingDisconnectCode.normalClosure.code);
           } catch (e, s) {
-            _logger.warning('_connectAsync dispose-disconnect error', e, s);
+            _logger.warning('_connectAsync stale-disconnect error', e, s);
           }
           return;
         }
@@ -270,7 +285,7 @@ class SignalingModuleImpl implements SignalingModule {
         _emit(SignalingConnected());
         unawaited(_requestQueue.flush(execute: client.execute, isActive: () => identical(_client, client)));
       } catch (e, s) {
-        if (_disposed) return;
+        if (_connectToken != connectToken || _disposed) return;
         _logger.warning('_connectAsync failed', e, s);
 
         final errorString = e.toString();
@@ -280,7 +295,9 @@ class SignalingModuleImpl implements SignalingModule {
         _emit(SignalingConnectionFailed(error: e, isRepeated: isRepeated, recommendedReconnectDelay: reconnectDelay));
       }
     } finally {
-      _connecting = false;
+      // Only clear the token if this is still the active connect — a superseded
+      // _connectAsync must not remove the token owned by the active one.
+      if (_connectToken == connectToken) _connectToken = null;
     }
   }
 

packages/webtrit_signaling_service/webtrit_signaling_service_platform_interface/test/signaling_module_impl_test.dart

@@ -0,0 +1,212 @@
+/// Unit tests for [SignalingModuleImpl].
+///
+/// Reproduces the disconnect-during-connect race condition:
+///
+/// When [SignalingModuleImpl.disconnect] is called while [_connectAsync] is
+/// still awaiting the client factory (_client == null, _connectToken != null),
+/// disconnect() must cancel the in-flight attempt by clearing _connectToken.
+/// The next connect() call must then be allowed to start a fresh attempt
+/// instead of being blocked by the stale in-flight operation.
+///
+/// Tests labelled "BUG:" reproduce the broken behavior on the unfixed code.
+/// Tests labelled "after fix:" verify the correct behavior.
+library;
+
+import 'dart:async';
+
+import 'package:flutter_test/flutter_test.dart';
+import 'package:ssl_certificates/ssl_certificates.dart';
+import 'package:webtrit_signaling/webtrit_signaling.dart';
+import 'package:webtrit_signaling_service_platform_interface/webtrit_signaling_service_platform_interface.dart';
+
+// ---------------------------------------------------------------------------
+// Fake client
+// ---------------------------------------------------------------------------
+
+class _FakeClient extends Fake implements WebtritSignalingClient {
+  DisconnectHandler? _onDisconnect;
+
+  @override
+  void listen({
+    required StateHandshakeHandler onStateHandshake,
+    required EventHandler onEvent,
+    required ErrorHandler onError,
+    required DisconnectHandler onDisconnect,
+  }) {
+    _onDisconnect = onDisconnect;
+  }
+
+  @override
+  Future<void> disconnect([int? code, String? reason]) async {
+    _onDisconnect?.call(code, reason);
+  }
+
+  @override
+  Future<void> execute(Request request, [Duration? timeout]) async {}
+}
+
+// ---------------------------------------------------------------------------
+// Controlled factory
+// ---------------------------------------------------------------------------
+
+/// A factory where each call gets its own [Completer].
+/// Tracks how many times the factory was called and allows releasing each
+/// call independently.
+class _ControlledFactory {
+  final _calls = <Completer<WebtritSignalingClient>>[];
+  int get callCount => _calls.length;
+
+  Completer<WebtritSignalingClient> get lastCall => _calls.last;
+
+  SignalingClientFactory get factory =>
+      ({
+        required Uri url,
+        required String tenantId,
+        required String token,
+        required Duration connectionTimeout,
+        required TrustedCertificates certs,
+        required bool force,
+      }) {
+        final c = Completer<WebtritSignalingClient>();
+        _calls.add(c);
+        return c.future;
+      };
+
+  void release(int callIndex, WebtritSignalingClient client) => _calls[callIndex].complete(client);
+}
+
+// ---------------------------------------------------------------------------
+// Helper
+// ---------------------------------------------------------------------------
+
+SignalingModuleImpl _buildModule(SignalingClientFactory factory) => SignalingModuleImpl(
+  coreUrl: 'https://example.com',
+  tenantId: 'tenant',
+  token: 'token',
+  trustedCertificates: TrustedCertificates.empty,
+  clientFactory: factory,
+);
+
+// ---------------------------------------------------------------------------
+// Tests
+// ---------------------------------------------------------------------------
+
+void main() {
+  group('SignalingModuleImpl — disconnect() during in-flight connect()', () {
+    // -----------------------------------------------------------------------
+    // Core bug: disconnect while _client==null leaves _connecting=true
+    // -----------------------------------------------------------------------
+
+    test('BUG: second connect() is silently dropped because _connecting stays true after disconnect()', () async {
+      // Arrange
+      final factory = _ControlledFactory();
+      final module = _buildModule(factory.factory);
+
+      // Act: connect #1 → factory called, hangs
+      module.connect();
+      await Future<void>.delayed(Duration.zero);
+      expect(factory.callCount, 1, reason: 'first connect() must call the factory');
+
+      // disconnect() while _client==null — the bug: does not reset _connecting
+      await module.disconnect();
+
+      // connect #2 — must call the factory again; currently it is silently dropped
+      module.connect();
+      await Future<void>.delayed(Duration.zero);
+
+      // BUG: factory was not called a second time — second connect() was dropped
+      expect(
+        factory.callCount,
+        2,
+        reason: 'second connect() was silently dropped: factory call count did not increase',
+      );
+    });
+
+    // -----------------------------------------------------------------------
+    // Consequence: stale connect completes and leaves module in wrong state
+    // -----------------------------------------------------------------------
+
+    test('BUG: stale in-flight connect completes after disconnect() and leaves module connected', () async {
+      // Arrange
+      final factory = _ControlledFactory();
+      final module = _buildModule(factory.factory);
+      final events = <SignalingModuleEvent>[];
+      module.events.listen(events.add);
+
+      // connect #1 → factory hangs
+      module.connect();
+      await Future<void>.delayed(Duration.zero);
+
+      // disconnect() — no-op when _client==null (the bug)
+      await module.disconnect();
+
+      // Release the stale factory (simulates slow WebSocket that eventually connects)
+      factory.release(0, _FakeClient());
+      await Future<void>.delayed(Duration.zero);
+
+      // BUG: module is now "connected" even though we explicitly disconnected
+      expect(
+        module.isConnected,
+        isFalse,
+        reason: 'stale connect() completed after disconnect() and incorrectly set _client',
+      );
+      expect(
+        events.whereType<SignalingConnected>(),
+        isEmpty,
+        reason: 'SignalingConnected must not fire for a connect() superseded by disconnect()',
+      );
+    });
+
+    // -----------------------------------------------------------------------
+    // Expected correct behavior after the fix
+    // -----------------------------------------------------------------------
+
+    test('after fix: disconnect() cancels in-flight connect so second connect() reaches the factory', () async {
+      final factory = _ControlledFactory();
+      final module = _buildModule(factory.factory);
+      final events = <SignalingModuleEvent>[];
+      module.events.listen(events.add);
+
+      // connect #1 → hangs
+      module.connect();
+      await Future<void>.delayed(Duration.zero);
+
+      // disconnect() → must cancel connect #1 and reset _connecting
+      await module.disconnect();
+
+      // connect #2 → must be accepted (factory called again)
+      module.connect();
+      await Future<void>.delayed(Duration.zero);
+      expect(factory.callCount, 2, reason: 'second connect() must call the factory');
+
+      // Release connect #2 — module should become connected
+      factory.release(1, _FakeClient());
+      await Future<void>.delayed(Duration.zero);
+
+      expect(module.isConnected, isTrue);
+      expect(events.whereType<SignalingConnected>(), isNotEmpty);
+    });
+
+    test('after fix: stale factory result is discarded, SignalingConnected not emitted', () async {
+      final factory = _ControlledFactory();
+      final module = _buildModule(factory.factory);
+      final events = <SignalingModuleEvent>[];
+      module.events.listen(events.add);
+
+      // connect #1 → hangs
+      module.connect();
+      await Future<void>.delayed(Duration.zero);
+
+      // disconnect() cancels connect #1
+      await module.disconnect();
+
+      // Release the stale factory call AFTER disconnect
+      factory.release(0, _FakeClient());
+      await Future<void>.delayed(Duration.zero);
+
+      // Stale result must be discarded
+      expect(module.isConnected, isFalse);
+      expect(events.whereType<SignalingConnected>(), isEmpty);
+    });
+  });
+}