wip don't use fallback hosts on Realtime token errors

we seem to have it for REST — see line 616 of lib/ably/rest/client.rb

Author: lawrence-forooghian

lib/ably/realtime/connection.rb

@@ -391,17 +391,26 @@ def __incoming_protocol_msgbus__
       def determine_host
         raise ArgumentError, 'Block required' unless block_given?
 
+        @determine_host_call_count ||= 0
+        @determine_host_call_count += 1
+
+        if @determine_host_call_count == 3
+          logger.debug { "LAWRENCE: On #{@determine_host_call_count} call to determine_host" }
+        end
+        
         if should_use_fallback_hosts?
           internet_up? do |internet_is_up_result|
             @current_host = if internet_is_up_result
               client.fallback_endpoint.host
             else
               client.endpoint.host
             end
+            logger.debug { "LAWRENCE: current_host from fallbacks: #{current_host}" }
             yield current_host
           end
         else
           @current_host = client.endpoint.host
+          logger.debug { "LAWRENCE: current_host from not fallbacks: #{current_host}" }
           yield current_host
         end
       end
@@ -674,6 +683,17 @@ def custom_host?
       end
 
       def should_use_fallback_hosts?
+        logger.debug { "LAWRENCE: should_use_fallback_hosts? #{state_history.inspect}" }
+
+        # Don't use fallback hosts if the last state change was due to a token error
+        # if state_history.any?
+        #   last_state_change = state_history.last
+        #   if last_state_change[:reason].respond_to?(:code) && last_state_change[:reason].code
+        #     return false if last_state_change[:reason].code >= 40140 && last_state_change[:reason].code < 40150
+        #   end
+        # end
+
+        # This logic is utterly bizarre. It pays no attention to the error that caused the disconnection.
         if client.fallback_hosts && !client.fallback_hosts.empty?
           if connecting? && previous_state && !disconnected_from_connected_state?
             use_fallback_if_disconnected? || use_fallback_if_suspended?
@@ -690,7 +710,24 @@ def disconnected_from_connected_state?
       end
 
       def use_fallback_if_disconnected?
-        second_reconnect_attempt_for(:disconnected, 1)
+        unless second_reconnect_attempt_for(:disconnected, 1)
+          return false
+        end
+
+        last_state_change = state_history.last
+        logger.debug { "LAWRENCE: last_state_change in disconnected thing: #{last_state_change.inspect} with reason #{last_state_change[:metadata].reason}" }
+        logger.debug { "LAWRENCE: last_disconnected_reason: #{last_disconnected_reason.inspect}" }
+
+        does_error_necessitate_fallback(last_disconnected_reason)
+      end
+
+      def does_error_necessitate_fallback(error)
+        # RSC15l
+        if error.respond_to?(:status_code) && error.status_code == 401 && error.respond_to?(:code) && Ably::Exceptions::TOKEN_EXPIRED_CODE.include?(error.code)
+          return false
+        end 
+
+        true
       end
 
       def use_fallback_if_suspended?
@@ -700,6 +737,12 @@ def use_fallback_if_suspended?
       def second_reconnect_attempt_for(state, first_attempt_count)
         previous_state == state && manager.retry_count_for_state(state) >= first_attempt_count
       end
+
+      def last_disconnected_reason
+        history_item = state_history.reverse.find do |history_item|
+          history_item.fetch(:state) == :disconnected
+        end.fetch(:metadata).reason
+      end
     end
   end
 end

lib/ably/realtime/connection/connection_manager.rb

@@ -12,6 +12,7 @@ class Connection
     class ConnectionManager
       # Error codes from the server that can potentially be resolved
       RESOLVABLE_ERROR_CODES = {
+        # TODO what is this
         token_expired: Ably::Exceptions::TOKEN_EXPIRED_CODE
       }
       using Ably::Util::AblyExtensions

spec/acceptance/realtime/connection_failures_spec.rb

@@ -1358,6 +1358,7 @@ def kill_connection_transport_and_prevent_valid_resume
             end)
           end
 
+          # TODO what's going on in this test?
           it 'triggers a re-authentication and then resumes the connection' do
             # [PENDING] After sandbox env update connection isn't found and a new connection is created. Spec fails
             #
@@ -1367,8 +1368,24 @@ def kill_connection_transport_and_prevent_valid_resume
               connecting_attempts = 0
               connection.on(:connecting) { connecting_attempts += 1 }
 
+              # My understanding of what's going on here:
+              #
+              # 1. Connect with a token that expires in 4 seconds
+              # 2. Make sure that we don't receive the token DISCONNECTED from Ably, so that RTN15h1 doesn't happen (i.e. we don't try and get a new token yet)
+              # 3. Then we wait for Ably to close the transport (actually, we simulate a closing of the transport)
+              #    - This should take us from CONNECTED to CONNECTING per RTN15a
+              # 4. So then we should attempt to resume, with the same expired token
+              # 5. This resume should fail with a token error, so per RTN15c5 we need to follow RTN15h, i.e. remain CONNECTING, fetch another token, and reconnect
+              # 6. So then we should end up in CONNECTED
+              
+              # So, things I'm seeing in the failing test logs:
+              # - the transport-disconnected reconnect (i.e. 4) happens to the primary host
+              # - the post-renew reconnect (i.e. 5) happens to a fallback host
+
               connection.once(:connected) do
                 expect(@auth_requests).to eql(2) # initial + reconnect fails due to expiry & then obtains new token
+                # I don't think that this is correct per the spec, there's not meant to be a DISCONNECTED at all, no? Well, that's a separate matter
+                # Seems, from looking at logs, that in both 3 and 5 there is a DISCONNECTED where I wouldn't expect one
                 expect(connecting_attempts).to eql(2) # reconnect with failed token, then reconnect with successful token
                 expect(connection.id).to eql(connection_id)
                 stop_reactor

spec/support/debug_failure_helper.rb

@@ -30,7 +30,6 @@
   config.after(:example) do |example|
     next if example.metadata[:prevent_log_stubbing]
 
-    exception = example.exception
-    puts "\n#{'-'*34}\n\e[36mVerbose Ably log from test failure\e[0m\n#{'-'*34}\n#{@log_output.join("\n")}\n\n" if exception
+    puts "\n#{'-'*34}\n\e[36mVerbose Ably log\e[0m\n#{'-'*34}\n#{@log_output.join("\n")}\n\n"
   end
 end