[CrOS Tether] Update connection retry attempt logic.

Before this CL, we allowed 3 retries for the entire connection flow,
which includes advertising, scanning, creating a GATT connection, and
exchanging messages. However, it makes more sense to think of this as a
two-part process:
  (1) Advertising and scanning. This portion always occurs when
      attempting a BLE connection, even if the device we're trying to
      contact is not nearby and cannot respond.
  (2) Creating a GATT connection and exchanging message. This portion
      only occurs when an Android host is nearby to respond to the
      Chromebook.

Connection attempts which fail in part (2) above indicate that the
device actually is nearby and can potentially connect; additionally
connection failures during this part of the connection often will
succeed on a retry. Thus, this CL adds extra connection retry attempts
if a previous attempt failed in part (2).

Bug: 805218, 672263
Change-Id: Ifbedc004b96905d3f77662c45bcf2a9210b06c57
Reviewed-on: https://chromium-review.googlesource.com/889991Reviewed-by: Jeremy Klein <jlklein@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533073}

chromeos/components/tether/connect_tethering_operation_unittest.cc

@@ -32,8 +32,6 @@ namespace {
 const char kTestSsid[] = "testSsid";
 const char kTestPassword[] = "testPassword";
 
-const size_t kMaxConnectionAttemptsPerDevice = 3;
-
 constexpr base::TimeDelta kConnectTetheringResponseTime =
     base::TimeDelta::FromSeconds(15);
 
@@ -113,11 +111,7 @@ class ConnectTetheringOperationTest : public testing::Test {
   ConnectTetheringOperationTest()
       : connect_tethering_request_string_(
             CreateConnectTetheringRequestString()),
-        test_device_(cryptauth::GenerateTestRemoteDevices(1)[0]) {
-    // These tests are written under the assumption that there are a maximum of
-    // 3 connection attempts; they need to be edited if this value changes.
-    EXPECT_EQ(3u, MessageTransferOperation::kMaxConnectionAttemptsPerDevice);
-  }
+        test_device_(cryptauth::GenerateTestRemoteDevices(1)[0]) {}
 
   void SetUp() override {
     fake_ble_connection_manager_ = std::make_unique<FakeBleConnectionManager>();
@@ -278,8 +272,9 @@ TEST_F(ConnectTetheringOperationTest, TestCannotConnect) {
       .Times(0);
 
   // Simulate the device failing to connect.
-  fake_ble_connection_manager_->SimulateFailedConnectionAttempts(
-      test_device_.GetDeviceId(), kMaxConnectionAttemptsPerDevice);
+  fake_ble_connection_manager_->SimulateUnansweredConnectionAttempts(
+      test_device_.GetDeviceId(),
+      MessageTransferOperation::kMaxEmptyScansPerDevice);
 
   // The maximum number of connection failures has occurred.
   EXPECT_TRUE(test_observer_->has_received_failure());

chromeos/components/tether/fake_ble_connection_manager.cc

@@ -61,7 +61,7 @@ void FakeBleConnectionManager::SetMessageSent(int sequence_number) {
   NotifyMessageSent(sequence_number);
 }
 
-void FakeBleConnectionManager::SimulateFailedConnectionAttempts(
+void FakeBleConnectionManager::SimulateUnansweredConnectionAttempts(
     const std::string& device_id,
     size_t num_attempts) {
   for (size_t i = 0; i < num_attempts; ++i) {
@@ -73,6 +73,22 @@ void FakeBleConnectionManager::SimulateFailedConnectionAttempts(
   }
 }
 
+void FakeBleConnectionManager::SimulateGattErrorConnectionAttempts(
+    const std::string& device_id,
+    size_t num_attempts) {
+  for (size_t i = 0; i < num_attempts; ++i) {
+    SetDeviceStatus(device_id, cryptauth::SecureChannel::Status::CONNECTING,
+                    StateChangeDetail::STATE_CHANGE_DETAIL_NONE);
+    SetDeviceStatus(device_id, cryptauth::SecureChannel::Status::CONNECTED,
+                    StateChangeDetail::STATE_CHANGE_DETAIL_NONE);
+    SetDeviceStatus(device_id, cryptauth::SecureChannel::Status::AUTHENTICATING,
+                    StateChangeDetail::STATE_CHANGE_DETAIL_NONE);
+    SetDeviceStatus(
+        device_id, cryptauth::SecureChannel::Status::DISCONNECTED,
+        StateChangeDetail::STATE_CHANGE_DETAIL_GATT_CONNECTION_WAS_ATTEMPTED);
+  }
+}
+
 bool FakeBleConnectionManager::IsRegistered(const std::string& device_id) {
   return base::ContainsKey(device_id_map_, device_id);
 }

chromeos/components/tether/fake_ble_connection_manager.h

@@ -33,11 +33,19 @@ class FakeBleConnectionManager : public BleConnectionManager {
   void ReceiveMessage(const std::string& device_id, const std::string& payload);
   void SetMessageSent(int sequence_number);
 
-  // Simulates |num_attempts| consecutive failed connection attempts for the
-  // device with ID |device_id|. Specifically, this function updates the
-  // device's status to CONNECTING then DISCONNECTED on each attempt.
-  void SimulateFailedConnectionAttempts(const std::string& device_id,
-                                        size_t num_attempts);
+  // Simulates |num_attempts| consecutive failed "unanswered" connection
+  // attempts for the device with ID |device_id|. Specifically, this function
+  // updates the device's status to CONNECTING then DISCONNECTED on each
+  // attempt.
+  void SimulateUnansweredConnectionAttempts(const std::string& device_id,
+                                            size_t num_attempts);
+
+  // Simulates |num_attempts| consecutive failed "GATT error" connection
+  // attempts for the device with ID |device_id|. Specifically, this function
+  // updates the device's status to CONNECTING, then CONNECTED, then
+  // AUTHENTICATING, then DISCONNECTED on each attempt.
+  void SimulateGattErrorConnectionAttempts(const std::string& device_id,
+                                           size_t num_attempts);
 
   std::vector<SentMessage>& sent_messages() { return sent_messages_; }
   // Returns -1 if no sequence numbers have been used yet.

chromeos/components/tether/host_scanner_operation_unittest.cc

@@ -30,8 +30,6 @@ namespace tether {
 
 namespace {
 
-const size_t kMaxConnectionAttemptsPerDevice = 3;
-
 const char kDefaultCarrier[] = "Google Fi";
 
 constexpr base::TimeDelta kTetherAvailabilityResponseTime =
@@ -129,11 +127,7 @@ class HostScannerOperationTest : public testing::Test {
   HostScannerOperationTest()
       : tether_availability_request_string_(
             CreateTetherAvailabilityRequestString()),
-        test_devices_(cryptauth::GenerateTestRemoteDevices(5)) {
-    // These tests are written under the assumption that there are a maximum of
-    // 3 connection attempts; they need to be edited if this value changes.
-    EXPECT_EQ(3u, MessageTransferOperation::kMaxConnectionAttemptsPerDevice);
-  }
+        test_devices_(cryptauth::GenerateTestRemoteDevices(5)) {}
 
   void SetUp() override {
     fake_ble_connection_manager_ = std::make_unique<FakeBleConnectionManager>();
@@ -375,8 +369,9 @@ TEST_F(HostScannerOperationTest, TestMultipleDevices) {
       kTetherAvailabilityResponseTime, 2);
 
   // Simulate device 1 failing to connect.
-  fake_ble_connection_manager_->SimulateFailedConnectionAttempts(
-      test_devices_[1].GetDeviceId(), kMaxConnectionAttemptsPerDevice);
+  fake_ble_connection_manager_->SimulateUnansweredConnectionAttempts(
+      test_devices_[1].GetDeviceId(),
+      MessageTransferOperation::kMaxEmptyScansPerDevice);
 
   // The scan should still not be over, and no new scan results should have
   // come in.
@@ -384,8 +379,9 @@ TEST_F(HostScannerOperationTest, TestMultipleDevices) {
   EXPECT_EQ(2u, test_observer_->scanned_devices_so_far().size());
 
   // Simulate device 3 failing to connect.
-  fake_ble_connection_manager_->SimulateFailedConnectionAttempts(
-      test_devices_[3].GetDeviceId(), kMaxConnectionAttemptsPerDevice);
+  fake_ble_connection_manager_->SimulateUnansweredConnectionAttempts(
+      test_devices_[3].GetDeviceId(),
+      MessageTransferOperation::kMaxEmptyScansPerDevice);
 
   // The scan should still not be over, and no new scan results should have
   // come in.

chromeos/components/tether/keep_alive_operation_unittest.cc

@@ -22,8 +22,6 @@ namespace tether {
 
 namespace {
 
-const size_t kMaxConnectionAttemptsPerDevice = 3;
-
 constexpr base::TimeDelta kKeepAliveTickleResponseTime =
     base::TimeDelta::FromSeconds(3);
 
@@ -142,8 +140,9 @@ TEST_F(KeepAliveOperationTest, TestSendsKeepAliveTickleAndReceivesResponse) {
 
 TEST_F(KeepAliveOperationTest, TestCannotConnect) {
   // Simulate the device failing to connect.
-  fake_ble_connection_manager_->SimulateFailedConnectionAttempts(
-      test_device_.GetDeviceId(), kMaxConnectionAttemptsPerDevice);
+  fake_ble_connection_manager_->SimulateUnansweredConnectionAttempts(
+      test_device_.GetDeviceId(),
+      MessageTransferOperation::kMaxEmptyScansPerDevice);
 
   // The maximum number of connection failures has occurred.
   EXPECT_TRUE(test_observer_->has_run_callback());

chromeos/components/tether/message_transfer_operation.cc

@@ -34,12 +34,6 @@ std::vector<cryptauth::RemoteDevice> RemoveDuplicatesFromVector(
 
 }  // namespace
 
-// static
-uint32_t MessageTransferOperation::kMaxConnectionAttemptsPerDevice = 3;
-
-// static
-uint32_t MessageTransferOperation::kDefaultTimeoutSeconds = 10;
-
 MessageTransferOperation::MessageTransferOperation(
     const std::vector<cryptauth::RemoteDevice>& devices_to_connect,
     BleConnectionManager* connection_manager)
@@ -109,42 +103,22 @@ void MessageTransferOperation::OnSecureChannelStatusChanged(
     return;
   }
 
-  // TODO(khorimoto): Use |status_change_detail| to provide better retry
-  // handling. See https://crbug.com/805218.
-
-  if (new_status == cryptauth::SecureChannel::Status::AUTHENTICATED) {
-    StartTimerForDevice(*remote_device);
-    OnDeviceAuthenticated(*remote_device);
-  } else if (new_status == cryptauth::SecureChannel::Status::DISCONNECTED) {
-    // Note: In success cases, the channel advances from DISCONNECTED to
-    // CONNECTING to CONNECTED to AUTHENTICATING to AUTHENTICATED. If the
-    // channel fails to advance at any of those stages, it transitions back to
-    // DISCONNECTED and starts over.
-
-    uint32_t num_attempts_so_far;
-    if (remote_device_to_num_attempts_map_.find(*remote_device) ==
-        remote_device_to_num_attempts_map_.end()) {
-      num_attempts_so_far = 0;
-    } else {
-      num_attempts_so_far = remote_device_to_num_attempts_map_[*remote_device];
-    }
-
-    num_attempts_so_far++;
-    remote_device_to_num_attempts_map_[*remote_device] = num_attempts_so_far;
-
-    PA_LOG(INFO) << "Connection attempt failed for device with ID "
-                 << remote_device->GetTruncatedDeviceIdForLogs() << ". "
-                 << "Number of failures so far: " << num_attempts_so_far;
-
-    if (num_attempts_so_far >= kMaxConnectionAttemptsPerDevice) {
-      PA_LOG(INFO) << "Connection retry limit reached for device with ID "
-                   << remote_device->GetTruncatedDeviceIdForLogs() << ". "
-                   << "Unregistering device.";
-
-      // If the number of failures so far is equal to the maximum allowed number
-      // of connection attempts, give up and unregister the device.
-      UnregisterDevice(*remote_device);
-    }
+  switch (new_status) {
+    case cryptauth::SecureChannel::Status::AUTHENTICATED:
+      StartTimerForDevice(*remote_device);
+      OnDeviceAuthenticated(*remote_device);
+      break;
+    case cryptauth::SecureChannel::Status::DISCONNECTED:
+      HandleDeviceDisconnection(*remote_device, status_change_detail);
+      break;
+    default:
+      // Note: In success cases, the channel advances from DISCONNECTED to
+      // CONNECTING to CONNECTED to AUTHENTICATING to AUTHENTICATED. If the
+      // channel fails to advance at any of those stages, it transitions back to
+      // DISCONNECTED and starts over. There is no need for special handling for
+      // any of these interim states since they will eventually progress to
+      // either AUTHENTICATED or DISCONNECTED.
+      break;
   }
 }
 
@@ -174,7 +148,7 @@ void MessageTransferOperation::UnregisterDevice(
   // cause the original reference to be deleted.
   cryptauth::RemoteDevice remote_device_copy = remote_device;
 
-  remote_device_to_num_attempts_map_.erase(remote_device_copy);
+  remote_device_to_attempts_map_.erase(remote_device_copy);
   remote_devices_.erase(std::remove(remote_devices_.begin(),
                                     remote_devices_.end(), remote_device_copy),
                         remote_devices_.end());
@@ -203,6 +177,70 @@ void MessageTransferOperation::SetTimerFactoryForTest(
   timer_factory_ = std::move(timer_factory_for_test);
 }
 
+void MessageTransferOperation::HandleDeviceDisconnection(
+    const cryptauth::RemoteDevice& remote_device,
+    BleConnectionManager::StateChangeDetail status_change_detail) {
+  ConnectAttemptCounts& attempts_for_device =
+      remote_device_to_attempts_map_[remote_device];
+
+  switch (status_change_detail) {
+    case BleConnectionManager::StateChangeDetail::STATE_CHANGE_DETAIL_NONE:
+      PA_LOG(ERROR) << "State transitioned to DISCONNECTED, but no "
+                    << "StateChangeDetail was provided. Treating this as a "
+                    << "failure to discover the device.";
+    // Note: Intentional fall-through to next case.
+    case BleConnectionManager::StateChangeDetail::
+        STATE_CHANGE_DETAIL_COULD_NOT_ATTEMPT_CONNECTION:
+      ++attempts_for_device.empty_scan_attempts;
+      PA_LOG(INFO) << "Connection attempt failed; could not discover the "
+                   << "device with ID "
+                   << remote_device.GetTruncatedDeviceIdForLogs() << ". "
+                   << "Number of failures to establish connection: "
+                   << attempts_for_device.empty_scan_attempts;
+
+      if (attempts_for_device.empty_scan_attempts >= kMaxEmptyScansPerDevice) {
+        PA_LOG(INFO) << "Reached retry limit for failing to discover the "
+                     << "device with ID "
+                     << remote_device.GetTruncatedDeviceIdForLogs() << ". "
+                     << "Unregistering device.";
+        UnregisterDevice(remote_device);
+      }
+      break;
+    case BleConnectionManager::StateChangeDetail::
+        STATE_CHANGE_DETAIL_GATT_CONNECTION_WAS_ATTEMPTED:
+      ++attempts_for_device.gatt_connection_attempts;
+      PA_LOG(INFO) << "Connection attempt failed; GATT connection error for "
+                   << "device with ID "
+                   << remote_device.GetTruncatedDeviceIdForLogs() << ". "
+                   << "Number of GATT error: "
+                   << attempts_for_device.gatt_connection_attempts;
+
+      if (attempts_for_device.gatt_connection_attempts >=
+          kMaxGattConnectionAttemptsPerDevice) {
+        PA_LOG(INFO) << "Reached retry limit for GATT connection errors for "
+                     << "device with ID "
+                     << remote_device.GetTruncatedDeviceIdForLogs() << ". "
+                     << "Unregistering device.";
+        UnregisterDevice(remote_device);
+      }
+      break;
+    case BleConnectionManager::StateChangeDetail::
+        STATE_CHANGE_DETAIL_INTERRUPTED_BY_HIGHER_PRIORITY:
+      // If the connection attempt was interrupted by a higher-priority message,
+      // this is not a true failure. There is nothing to do until the next state
+      // change occurs.
+      break;
+    case BleConnectionManager::StateChangeDetail::
+        STATE_CHANGE_DETAIL_DEVICE_WAS_UNREGISTERED:
+      // This state change is expected to be handled as a result of calls to
+      // UnregisterDevice(). There is no need for special handling.
+      break;
+    default:
+      NOTREACHED();
+      break;
+  }
+}
+
 void MessageTransferOperation::StartTimerForDevice(
     const cryptauth::RemoteDevice& remote_device) {
   PA_LOG(INFO) << "Starting timer for operation with message type "

chromeos/components/tether/message_transfer_operation.h

@@ -23,6 +23,21 @@ class TimerFactory;
 // from remote devices.
 class MessageTransferOperation : public BleConnectionManager::Observer {
  public:
+  // The number of times to attempt to connect to a device without receiving any
+  // response before giving up. When a connection to a device is attempted, a
+  // BLE discovery session listens for advertisements from the remote device as
+  // the first step of the connection; if no advertisement is picked up, it is
+  // likely that the remote device is not nearby or is not currently responding
+  // to Instant Tethering requests.
+  static constexpr const uint32_t kMaxEmptyScansPerDevice = 3;
+
+  // The number of times to attempt a GATT connection to a device, after a BLE
+  // discovery session has already detected a nearby device. GATT connections
+  // may fail for a variety of reasons, but most failures are ephemeral. Thus,
+  // more connection attempts are allowed in such cases since it is likely that
+  // a subsequent attempt will succeed. See https://crbug.com/805218.
+  static constexpr const uint32_t kMaxGattConnectionAttemptsPerDevice = 6;
+
   MessageTransferOperation(
       const std::vector<cryptauth::RemoteDevice>& devices_to_connect,
       BleConnectionManager* connection_manager);
@@ -91,22 +106,29 @@ class MessageTransferOperation : public BleConnectionManager::Observer {
   friend class HostScannerOperationTest;
   friend class MessageTransferOperationTest;
 
-  static uint32_t kMaxConnectionAttemptsPerDevice;
-
   // The default number of seconds an operation should wait before a timeout
   // occurs. Once this amount of time passes, the connection will be closed.
   // Classes deriving from MessageTransferOperation should override
   // GetTimeoutSeconds() if they desire a different duration.
-  static uint32_t kDefaultTimeoutSeconds;
+  static constexpr const uint32_t kDefaultTimeoutSeconds = 10;
 
-  void SetTimerFactoryForTest(
-      std::unique_ptr<TimerFactory> timer_factory_for_test);
+  struct ConnectAttemptCounts {
+    uint32_t empty_scan_attempts = 0;
+    uint32_t gatt_connection_attempts = 0;
+  };
+
+  void HandleDeviceDisconnection(
+      const cryptauth::RemoteDevice& remote_device,
+      BleConnectionManager::StateChangeDetail status_change_detail);
   void StartTimerForDevice(const cryptauth::RemoteDevice& remote_device);
   void StopTimerForDeviceIfRunning(
       const cryptauth::RemoteDevice& remote_device);
   void OnTimeout(const cryptauth::RemoteDevice& remote_device);
   cryptauth::RemoteDevice* GetRemoteDevice(const std::string& device_id);
 
+  void SetTimerFactoryForTest(
+      std::unique_ptr<TimerFactory> timer_factory_for_test);
+
   std::vector<cryptauth::RemoteDevice> remote_devices_;
   BleConnectionManager* connection_manager_;
   std::unique_ptr<TimerFactory> timer_factory_;
@@ -114,8 +136,8 @@ class MessageTransferOperation : public BleConnectionManager::Observer {
   bool initialized_ = false;
   bool shutting_down_ = false;
   MessageType message_type_for_connection_;
-  std::map<cryptauth::RemoteDevice, uint32_t>
-      remote_device_to_num_attempts_map_;
+  std::map<cryptauth::RemoteDevice, ConnectAttemptCounts>
+      remote_device_to_attempts_map_;
   std::map<cryptauth::RemoteDevice, std::unique_ptr<base::Timer>>
       remote_device_to_timer_map_;
   base::WeakPtrFactory<MessageTransferOperation> weak_ptr_factory_;

chromeos/components/tether/message_transfer_operation_unittest.cc

@@ -21,8 +21,6 @@ namespace tether {
 
 namespace {
 
-const size_t kMaxConnectionAttemptsPerDevice = 3;
-
 // Arbitrarily chosen value. The MessageType used in this test does not matter
 // except that it must be consistent throughout the test.
 const MessageType kTestMessageType = MessageType::TETHER_AVAILABILITY_REQUEST;
@@ -159,8 +157,11 @@ class MessageTransferOperationTest : public testing::Test {
   MessageTransferOperationTest()
       : test_devices_(cryptauth::GenerateTestRemoteDevices(4)) {
     // These tests are written under the assumption that there are a maximum of
-    // 3 connection attempts; they need to be edited if this value changes.
-    EXPECT_EQ(3u, MessageTransferOperation::kMaxConnectionAttemptsPerDevice);
+    // 3 "empty scan" connection attempts and 6 "GATT" connection attempts; the
+    // tests need to be edited if these values change.
+    EXPECT_EQ(3u, MessageTransferOperation::kMaxEmptyScansPerDevice);
+    EXPECT_EQ(6u,
+              MessageTransferOperation::kMaxGattConnectionAttemptsPerDevice);
   }
 
   void SetUp() override {
@@ -240,7 +241,7 @@ class MessageTransferOperationTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(MessageTransferOperationTest);
 };
 
-TEST_F(MessageTransferOperationTest, TestCannotConnectAndReachesRetryLimit) {
+TEST_F(MessageTransferOperationTest, CannotReceiveResponse_RetryLimitReached) {
   ConstructOperation(std::vector<cryptauth::RemoteDevice>{test_devices_[0]});
   InitializeOperation();
   EXPECT_TRUE(fake_ble_connection_manager_->IsRegistered(
@@ -292,6 +293,52 @@ TEST_F(MessageTransferOperationTest, TestCannotConnectAndReachesRetryLimit) {
   EXPECT_TRUE(operation_->GetReceivedMessages(test_devices_[0]).empty());
 }
 
+TEST_F(MessageTransferOperationTest,
+       CannotCompleteGattConnection_RetryLimitReached) {
+  ConstructOperation(std::vector<cryptauth::RemoteDevice>{test_devices_[0]});
+  InitializeOperation();
+  EXPECT_TRUE(fake_ble_connection_manager_->IsRegistered(
+      test_devices_[0].GetDeviceId()));
+
+  fake_ble_connection_manager_->SimulateGattErrorConnectionAttempts(
+      test_devices_[0].GetDeviceId(),
+      MessageTransferOperation::kMaxGattConnectionAttemptsPerDevice);
+  EXPECT_FALSE(fake_ble_connection_manager_->IsRegistered(
+      test_devices_[0].GetDeviceId()));
+
+  VerifyOperationStartedAndFinished(true /* has_started */,
+                                    true /* has_finished */);
+  EXPECT_FALSE(operation_->HasDeviceAuthenticated(test_devices_[0]));
+  EXPECT_TRUE(operation_->GetReceivedMessages(test_devices_[0]).empty());
+}
+
+TEST_F(MessageTransferOperationTest, MixedConnectionAttemptFailures) {
+  ConstructOperation(std::vector<cryptauth::RemoteDevice>{test_devices_[0]});
+  InitializeOperation();
+  EXPECT_TRUE(fake_ble_connection_manager_->IsRegistered(
+      test_devices_[0].GetDeviceId()));
+
+  // Fail to establish a connection one fewer time than the maximum allowed. The
+  // device should still be registered since the maximum was not hit.
+  fake_ble_connection_manager_->SimulateUnansweredConnectionAttempts(
+      test_devices_[0].GetDeviceId(),
+      MessageTransferOperation::kMaxEmptyScansPerDevice - 1);
+  EXPECT_TRUE(fake_ble_connection_manager_->IsRegistered(
+      test_devices_[0].GetDeviceId()));
+
+  // Now
+  fake_ble_connection_manager_->SimulateGattErrorConnectionAttempts(
+      test_devices_[0].GetDeviceId(),
+      MessageTransferOperation::kMaxGattConnectionAttemptsPerDevice);
+  EXPECT_FALSE(fake_ble_connection_manager_->IsRegistered(
+      test_devices_[0].GetDeviceId()));
+
+  VerifyOperationStartedAndFinished(true /* has_started */,
+                                    true /* has_finished */);
+  EXPECT_FALSE(operation_->HasDeviceAuthenticated(test_devices_[0]));
+  EXPECT_TRUE(operation_->GetReceivedMessages(test_devices_[0]).empty());
+}
+
 TEST_F(MessageTransferOperationTest, TestFailsThenConnects) {
   ConstructOperation(std::vector<cryptauth::RemoteDevice>{test_devices_[0]});
   InitializeOperation();
@@ -377,22 +424,22 @@ TEST_F(MessageTransferOperationTest, TestDevicesUnregisteredAfterDeletion) {
 
 TEST_F(MessageTransferOperationTest,
        TestSuccessfulConnectionAndReceiveMessage_TimeoutSeconds) {
-  const uint32_t timeout_seconds = 90;
+  const uint32_t kTimeoutSeconds = 90;
 
   ConstructOperation(std::vector<cryptauth::RemoteDevice>{test_devices_[0]});
   InitializeOperation();
   EXPECT_TRUE(fake_ble_connection_manager_->IsRegistered(
       test_devices_[0].GetDeviceId()));
 
-  operation_->set_timeout_seconds(timeout_seconds);
+  operation_->set_timeout_seconds(kTimeoutSeconds);
 
   TransitionDeviceStatusFromDisconnectedToAuthenticated(test_devices_[0]);
   EXPECT_TRUE(fake_ble_connection_manager_->IsRegistered(
       test_devices_[0].GetDeviceId()));
   EXPECT_TRUE(operation_->HasDeviceAuthenticated(test_devices_[0]));
-  VerifyTimerCreatedForDevice(test_devices_[0], timeout_seconds);
+  VerifyTimerCreatedForDevice(test_devices_[0], kTimeoutSeconds);
 
-  EXPECT_EQ(base::TimeDelta::FromSeconds(timeout_seconds),
+  EXPECT_EQ(base::TimeDelta::FromSeconds(kTimeoutSeconds),
             GetTimerForDevice(test_devices_[0])->GetCurrentDelay());
 
   fake_ble_connection_manager_->ReceiveMessage(
@@ -565,8 +612,9 @@ TEST_F(MessageTransferOperationTest, MultipleDevices) {
   // Fail 3 times to connect to |test_devices_[1]|.
   test_timer_factory_->set_device_id_for_next_timer(
       test_devices_[1].GetDeviceId());
-  fake_ble_connection_manager_->SimulateFailedConnectionAttempts(
-      test_devices_[1].GetDeviceId(), kMaxConnectionAttemptsPerDevice);
+  fake_ble_connection_manager_->SimulateUnansweredConnectionAttempts(
+      test_devices_[1].GetDeviceId(),
+      MessageTransferOperation::kMaxEmptyScansPerDevice);
   EXPECT_FALSE(operation_->HasDeviceAuthenticated(test_devices_[1]));
   EXPECT_FALSE(fake_ble_connection_manager_->IsRegistered(
       test_devices_[1].GetDeviceId()));
@@ -584,8 +632,9 @@ TEST_F(MessageTransferOperationTest, MultipleDevices) {
   // Fail 3 times to connect to |test_devices_[3]|.
   test_timer_factory_->set_device_id_for_next_timer(
       test_devices_[3].GetDeviceId());
-  fake_ble_connection_manager_->SimulateFailedConnectionAttempts(
-      test_devices_[3].GetDeviceId(), kMaxConnectionAttemptsPerDevice);
+  fake_ble_connection_manager_->SimulateUnansweredConnectionAttempts(
+      test_devices_[3].GetDeviceId(),
+      MessageTransferOperation::kMaxEmptyScansPerDevice);
   EXPECT_FALSE(operation_->HasDeviceAuthenticated(test_devices_[3]));
   EXPECT_FALSE(fake_ble_connection_manager_->IsRegistered(
       test_devices_[3].GetDeviceId()));