CU-868fdadum PR#166 fixes

ucswift · ucswift · commit 83c07047b095 · 2025-08-30T11:22:50.000-07:00
diff --git a/docs/signalr-reconnect-self-blocking-fix.md b/docs/signalr-reconnect-self-blocking-fix.md
@@ -0,0 +1,83 @@
+# SignalR Service Reconnection Self-Blocking Fix
+
+## Problem
+
+The SignalR service had a self-blocking issue where reconnection attempts would prevent direct connection attempts. Specifically:
+
+1. When a hub was in "reconnecting" state, direct connection attempts would be blocked
+2. This could lead to scenarios where:
+   - A reconnection attempt was in progress
+   - A user tried to manually connect
+   - The manual connection would be rejected
+   - If the reconnection failed, the hub would be stuck in reconnecting state
+   - Future manual connection attempts would continue to be blocked
+
+## Root Cause
+
+The service used a single `reconnectingHubs` Set to track both:
+- Automatic reconnection attempts 
+- Direct connection attempts
+
+This caused the guard logic in `_connectToHubInternal` (lines 240-246) to block direct connections when hubs were in reconnecting state.
+
+## Solution
+
+Implemented a more granular state management system:
+
+### 1. New State Enum
+
+```typescript
+export enum HubConnectingState {
+  IDLE = 'idle',
+  RECONNECTING = 'reconnecting', 
+  DIRECT_CONNECTING = 'direct-connecting',
+}
+```
+
+### 2. State Management
+
+- Added `hubStates: Map<string, HubConnectingState>` to track individual hub states
+- Added `setHubState()` method to manage state transitions and maintain backward compatibility
+- Added helper methods: `isHubConnecting()`, `isHubReconnecting()`
+
+### 3. Updated Connection Logic
+
+**Direct Connections (`_connectToHubInternal` and `_connectToHubWithEventingUrlInternal`):**
+- Only block duplicate direct connections (same `DIRECT_CONNECTING` state)
+- Allow direct connections even when hub is in `RECONNECTING` state  
+- Log reconnecting state but proceed with connection attempt
+- Set state to `DIRECT_CONNECTING` during connection attempt
+- Clean up state on both success and failure
+
+**Automatic Reconnections:**
+- Set state to `RECONNECTING` during reconnection attempts
+- Clean up state on both success and failure
+- Maintain existing reconnection logic and limits
+
+### 4. Backward Compatibility
+
+- Maintained the `reconnectingHubs` Set for existing API compatibility
+- `setHubState()` automatically manages the legacy set alongside the new state map
+- All existing methods continue to work as expected
+
+## Key Changes
+
+1. **Lines 240-246**: Changed from blocking all connections during reconnect to only blocking duplicate direct connections
+2. **State Management**: Added proper state tracking with cleanup in success/failure paths
+3. **Connection Isolation**: Reconnection attempts and direct connections now operate independently
+4. **Cleanup**: Ensured state cleanup happens in all code paths to prevent stuck states
+
+## Testing
+
+- Updated existing tests to use new state management system
+- All existing tests continue to pass
+- Tests verify that direct connections are allowed during reconnection
+- Tests verify proper state cleanup in success and failure scenarios
+
+## Benefits
+
+- Eliminates self-blocking behavior during reconnections
+- Allows users to manually retry connections even during automatic reconnection
+- Prevents permanent stuck states 
+- Maintains full backward compatibility
+- Provides better separation of concerns between automatic and manual connections
diff --git a/src/services/__tests__/signalr.service.reconnect-fix.test.ts b/src/services/__tests__/signalr.service.reconnect-fix.test.ts
@@ -0,0 +1,216 @@
+import { HubConnection, HubConnectionBuilder, LogLevel } from '@microsoft/signalr';
+
+import { logger } from '@/lib/logging';
+import { HubConnectingState } from '../signalr.service';
+
+// Mock the required modules
+jest.mock('@/lib/env', () => ({
+  Env: {
+    REALTIME_GEO_HUB_NAME: 'geolocationHub',
+  },
+}));
+
+jest.mock('@/lib/logging', () => ({
+  logger: {
+    info: jest.fn(),
+    warn: jest.fn(),
+    error: jest.fn(),
+    debug: jest.fn(),
+  },
+}));
+
+jest.mock('@/stores/auth/store', () => ({
+  __esModule: true,
+  default: {
+    getState: jest.fn(() => ({
+      accessToken: 'mock-token',
+      refreshAccessToken: jest.fn().mockResolvedValue(undefined),
+    })),
+  },
+}));
+
+jest.mock('@microsoft/signalr', () => ({
+  HubConnectionBuilder: jest.fn(),
+  LogLevel: {
+    Information: 'Information',
+  },
+  HubConnectionState: {
+    Connected: 'Connected',
+    Disconnected: 'Disconnected',
+  },
+}));
+
+// Import after mocking
+import { SignalRService } from '../signalr.service';
+
+describe('SignalRService - Reconnect Self-Blocking Fix', () => {
+  let signalRService: any;
+  let mockConnection: jest.Mocked<HubConnection>;
+  let mockBuilderInstance: jest.Mocked<HubConnectionBuilder>;
+  const mockLogger = logger as jest.Mocked<typeof logger>;
+
+  const mockHubConnectionBuilder = HubConnectionBuilder as jest.MockedClass<typeof HubConnectionBuilder>;
+  const mockStart = jest.fn().mockResolvedValue(undefined);
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+    // Reset the singleton instance
+    SignalRService.resetInstance();
+    signalRService = SignalRService.getInstance();
+
+    // Mock HubConnection
+    mockConnection = {
+      start: mockStart,
+      stop: jest.fn().mockResolvedValue(undefined),
+      on: jest.fn(),
+      onclose: jest.fn(),
+      onreconnecting: jest.fn(),
+      onreconnected: jest.fn(),
+      invoke: jest.fn().mockResolvedValue(undefined),
+      state: 'Connected',
+    } as any;
+
+    // Mock HubConnectionBuilder
+    mockBuilderInstance = {
+      withUrl: jest.fn().mockReturnThis(),
+      withAutomaticReconnect: jest.fn().mockReturnThis(),
+      configureLogging: jest.fn().mockReturnThis(),
+      build: jest.fn().mockReturnValue(mockConnection),
+    } as any;
+
+    mockHubConnectionBuilder.mockImplementation(() => mockBuilderInstance);
+  });
+
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
+
+  const mockConfig = {
+    name: 'testHub',
+    eventingUrl: 'https://api.example.com/',
+    hubName: 'eventingHub',
+    methods: ['method1'],
+  };
+
+  describe('Direct connection during reconnection', () => {
+    it('should allow direct connection attempts when hub is in reconnecting state', async () => {
+      // Set hub to reconnecting state
+      (signalRService as any).setHubState(mockConfig.name, HubConnectingState.RECONNECTING);
+      
+      // Verify hub is in reconnecting state
+      expect(signalRService.isHubReconnecting(mockConfig.name)).toBe(true);
+      
+      // Attempt direct connection - should not be blocked
+      await signalRService.connectToHubWithEventingUrl(mockConfig);
+      
+      // Should have attempted the connection
+      expect(mockHubConnectionBuilder).toHaveBeenCalled();
+      expect(mockLogger.info).toHaveBeenCalledWith({
+        message: `Hub ${mockConfig.name} is currently reconnecting, proceeding with direct connection attempt`,
+      });
+    });
+
+    it('should prevent duplicate direct connections', async () => {
+      // Set hub to direct-connecting state
+      (signalRService as any).setHubState(mockConfig.name, HubConnectingState.DIRECT_CONNECTING);
+      
+      // Attempt another direct connection - should be blocked
+      await signalRService.connectToHubWithEventingUrl(mockConfig);
+      
+      // Should not have attempted the connection
+      expect(mockHubConnectionBuilder).not.toHaveBeenCalled();
+      expect(mockLogger.info).toHaveBeenCalledWith({
+        message: `Hub ${mockConfig.name} is already in direct-connecting state, skipping duplicate connection attempt`,
+      });
+    });
+
+    it('should clean up direct-connecting state on successful connection', async () => {
+      // Start with idle state
+      expect((signalRService as any).isHubConnecting(mockConfig.name)).toBe(false);
+      
+      // Attempt connection
+      await signalRService.connectToHubWithEventingUrl(mockConfig);
+      
+      // Should be back to idle state after successful connection
+      expect((signalRService as any).isHubConnecting(mockConfig.name)).toBe(false);
+      expect((signalRService as any).hubStates.get(mockConfig.name)).toBeUndefined();
+    });
+
+    it('should clean up direct-connecting state on failed connection', async () => {
+      // Mock connection failure
+      mockStart.mockRejectedValueOnce(new Error('Connection failed'));
+      
+      // Start with idle state
+      expect((signalRService as any).isHubConnecting(mockConfig.name)).toBe(false);
+      
+      // Attempt connection (should fail)
+      await expect(signalRService.connectToHubWithEventingUrl(mockConfig)).rejects.toThrow('Connection failed');
+      
+      // Should be back to idle state after failed connection
+      expect((signalRService as any).isHubConnecting(mockConfig.name)).toBe(false);
+      expect((signalRService as any).hubStates.get(mockConfig.name)).toBeUndefined();
+      
+      // Reset mock for future tests
+      mockStart.mockResolvedValue(undefined);
+    });
+
+    it('should maintain backward compatibility with legacy reconnectingHubs set', async () => {
+      // Set hub to reconnecting state
+      (signalRService as any).setHubState(mockConfig.name, HubConnectingState.RECONNECTING);
+      
+      // Legacy reconnectingHubs set should also be updated
+      expect((signalRService as any).reconnectingHubs.has(mockConfig.name)).toBe(true);
+      expect(signalRService.isHubReconnecting(mockConfig.name)).toBe(true);
+      
+      // Clear state
+      (signalRService as any).setHubState(mockConfig.name, HubConnectingState.IDLE);
+      
+      // Legacy set should be cleared too
+      expect((signalRService as any).reconnectingHubs.has(mockConfig.name)).toBe(false);
+      expect(signalRService.isHubReconnecting(mockConfig.name)).toBe(false);
+    });
+  });
+
+  describe('State management', () => {
+    it('should distinguish between reconnecting and direct-connecting states', () => {
+      // Set to reconnecting
+      (signalRService as any).setHubState(mockConfig.name, HubConnectingState.RECONNECTING);
+      expect(signalRService.isHubReconnecting(mockConfig.name)).toBe(true);
+      expect((signalRService as any).isHubConnecting(mockConfig.name)).toBe(true);
+      
+      // Set to direct-connecting
+      (signalRService as any).setHubState(mockConfig.name, HubConnectingState.DIRECT_CONNECTING);
+      expect(signalRService.isHubReconnecting(mockConfig.name)).toBe(false);
+      expect((signalRService as any).isHubConnecting(mockConfig.name)).toBe(true);
+      
+      // Set to idle
+      (signalRService as any).setHubState(mockConfig.name, HubConnectingState.IDLE);
+      expect(signalRService.isHubReconnecting(mockConfig.name)).toBe(false);
+      expect((signalRService as any).isHubConnecting(mockConfig.name)).toBe(false);
+    });
+
+    it('should properly manage isHubAvailable with new states', () => {
+      const hubName = 'testHub';
+      
+      // Not connected, not connecting
+      expect(signalRService.isHubAvailable(hubName)).toBe(false);
+      
+      // Reconnecting
+      (signalRService as any).setHubState(hubName, HubConnectingState.RECONNECTING);
+      expect(signalRService.isHubAvailable(hubName)).toBe(true);
+      
+      // Direct connecting
+      (signalRService as any).setHubState(hubName, HubConnectingState.DIRECT_CONNECTING);
+      expect(signalRService.isHubAvailable(hubName)).toBe(true);
+      
+      // Add actual connection
+      (signalRService as any).connections.set(hubName, mockConnection);
+      (signalRService as any).setHubState(hubName, HubConnectingState.IDLE);
+      expect(signalRService.isHubAvailable(hubName)).toBe(true);
+      
+      // Clean up
+      (signalRService as any).connections.delete(hubName);
+      expect(signalRService.isHubAvailable(hubName)).toBe(false);
+    });
+  });
+});
diff --git a/src/services/__tests__/signalr.service.test.ts b/src/services/__tests__/signalr.service.test.ts
@@ -1,6 +1,7 @@
 import { HubConnection, HubConnectionBuilder, LogLevel } from '@microsoft/signalr';
 
 import { logger } from '@/lib/logging';
+import { HubConnectingState } from '../signalr.service';
 
 // Mock the env module
 jest.mock('@/lib/env', () => ({
@@ -509,8 +510,8 @@ describe('SignalRService', () => {
     });
 
     it('should return true for isHubAvailable when hub is reconnecting', () => {
-      // Manually set reconnecting state to test
-      (signalRService as any).reconnectingHubs.add(mockConfig.name);
+      // Manually set reconnecting state to test using new state management
+      (signalRService as any).setHubState(mockConfig.name, HubConnectingState.RECONNECTING);
       expect(signalRService.isHubAvailable(mockConfig.name)).toBe(true);
     });
 
@@ -519,22 +520,23 @@ describe('SignalRService', () => {
     });
 
     it('should return true for isHubReconnecting when hub is reconnecting', () => {
-      // Manually set reconnecting state to test
-      (signalRService as any).reconnectingHubs.add(mockConfig.name);
+      // Manually set reconnecting state to test using new state management
+      (signalRService as any).setHubState(mockConfig.name, HubConnectingState.RECONNECTING);
       expect(signalRService.isHubReconnecting(mockConfig.name)).toBe(true);
     });
 
     it('should skip connection if hub is already reconnecting', async () => {
-      // Set reconnecting state
-      (signalRService as any).reconnectingHubs.add(mockConfig.name);
+      // Set reconnecting state using new state management
+      (signalRService as any).setHubState(mockConfig.name, HubConnectingState.RECONNECTING);
       
       // Try to connect
       await signalRService.connectToHubWithEventingUrl(mockConfig);
 
-      // Should not have started a new connection
-      expect(mockHubConnectionBuilder).not.toHaveBeenCalled();
+      // Should not have started a new connection because the new logic allows connections but logs it
+      // The new logic doesn't block direct connections anymore, so this test behavior changes
+      expect(mockHubConnectionBuilder).toHaveBeenCalled();
       expect(mockLogger.info).toHaveBeenCalledWith({
-        message: `Hub ${mockConfig.name} is currently reconnecting, skipping duplicate connection attempt`,
+        message: `Hub ${mockConfig.name} is currently reconnecting, proceeding with direct connection attempt`,
       });
     });
   });
diff --git a/src/services/signalr.service.ts b/src/services/signalr.service.ts
