Go directly to Active when getting a new access token

Previously we would check if the access token we just got is still valid using
the local clock, and would stay in WaitingForAccessToken if we think it is
expired. Instead, assume that the just-acquired token is valid and go strait
to Active if we are in WaitingForAccessToken.

Fixes #4941.

CHANGELOG.md

@@ -6,6 +6,7 @@
 ### Fixed
 * SyncManager had some inconsistent locking which could result in data races and/or deadlocks, mostly in ways that would never be hit outside of tests doing very strange things (since v10.0.0).
 * Reduce the peak memory usage of changeset uploading by eliminating an extra copy of each changeset which was held in memory.
+* Don't keep trying to refresh the access token if the client's clock is more than 30 minutes fast. ([#4941](https://github.com/realm/realm-core/issues/4941))
 
 ### Breaking changes
 * None.

src/realm/object-store/sync/sync_session.cpp

@@ -45,7 +45,7 @@ constexpr const char SyncError::c_recovery_file_path_key[];
 ///
 /// WAITING_FOR_ACCESS_TOKEN: a request has been initiated to ask
 /// for an updated access token and the session is waiting for a response.
-/// From: ACTIVE
+/// From: INACTIVE, DYING
 /// To:
 ///    * ACTIVE: when the SDK successfully refreshes the token
 ///    * INACTIVE: if asked to log out, or if asked to close
@@ -57,8 +57,6 @@ constexpr const char SyncError::c_recovery_file_path_key[];
 ///    * INACTIVE: if asked to log out, or if asked to close and the stop policy
 ///                is Immediate.
 ///    * DYING: if asked to close and the stop policy is AfterChangesUploaded
-///    * WAITING_FOR_ACCESS_TOKEN: if the session tried to enter ACTIVE,
-///                                but the token is invalid or expired.
 ///
 /// DYING: the session is performing clean-up work in preparation to be destroyed.
 /// From: ACTIVE
@@ -67,6 +65,8 @@ constexpr const char SyncError::c_recovery_file_path_key[];
 ///                revived, or if explicitly asked to log out before the
 ///                clean-up work begins
 ///    * ACTIVE: if the session is revived
+///    * WAITING_FOR_ACCESS_TOKEN: if the session tried to enter ACTIVE,
+///                                but the token is invalid or expired.
 ///
 /// INACTIVE: the user owning this session has logged out, the `sync::Session`
 /// owned by this session is destroyed, and the session is quiescent.
@@ -75,26 +75,15 @@ constexpr const char SyncError::c_recovery_file_path_key[];
 /// From: initial, ACTIVE, DYING, WAITING_FOR_ACCESS_TOKEN
 /// To:
 ///    * ACTIVE: if the session is revived
+///    * WAITING_FOR_ACCESS_TOKEN: if the session tried to enter ACTIVE,
+///                                but the token is invalid or expired.
 
 void SyncSession::become_active(std::unique_lock<std::mutex>& lock)
 {
     REALM_ASSERT(lock.owns_lock());
     REALM_ASSERT(m_state != State::Active);
     m_state = State::Active;
 
-    const auto& user = m_config.user;
-    if (user && user->access_token_refresh_required()) {
-        become_waiting_for_access_token(lock);
-        // Release the lock for SDKs with a single threaded
-        // networking implementation such as our test suite
-        // so that the update can trigger a state change from
-        // the completion handler.
-        REALM_ASSERT(lock.owns_lock());
-        lock.unlock();
-        initiate_access_token_refresh();
-        return;
-    }
-
     // when entering from the Dying state the session will still be bound
     if (!m_session) {
         create_sync_session();
@@ -635,14 +624,28 @@ void SyncSession::revive_if_needed()
 {
     std::unique_lock<std::mutex> lock(m_state_mutex);
     switch (m_state) {
-        case State::Dying:
-        case State::Inactive:
-            // Revive.
-            become_active(lock);
-            break;
         case State::Active:
         case State::WaitingForAccessToken:
+            return;
+        case State::Dying:
+        case State::Inactive: {
+            // Revive.
+            const auto& user = m_config.user;
+            if (!user || !user->access_token_refresh_required()) {
+                become_active(lock);
+                return;
+            }
+
+            become_waiting_for_access_token(lock);
+            // Release the lock for SDKs with a single threaded
+            // networking implementation such as our test suite
+            // so that the update can trigger a state change from
+            // the completion handler.
+            REALM_ASSERT(lock.owns_lock());
+            lock.unlock();
+            initiate_access_token_refresh();
             break;
+        }
     }
 }
 

src/realm/object-store/sync/sync_user.cpp

@@ -519,7 +519,9 @@ bool SyncUser::access_token_refresh_required() const
     using namespace std::chrono;
     constexpr size_t buffer_seconds = 5; // arbitrary
     util::CheckedLockGuard lock(m_mutex);
-    auto threshold = duration_cast<seconds>(system_clock::now().time_since_epoch()).count() - buffer_seconds;
+    const auto now = duration_cast<seconds>(system_clock::now().time_since_epoch()).count() +
+                     m_seconds_to_adjust_time_for_testing.load(std::memory_order_relaxed);
+    const auto threshold = now - buffer_seconds;
     return do_is_logged_in() && m_access_token.expires_at < static_cast<int64_t>(threshold);
 }
 

src/realm/object-store/sync/sync_user.hpp

@@ -300,6 +300,10 @@ class SyncUser : public std::enable_shared_from_this<SyncUser>, public Subscriba
     /// @param service_name The name of the cluster
     app::MongoClient mongo_client(const std::string& service_name) REQUIRES(!m_mutex);
 
+    void set_seconds_to_adjust_time_for_testing(int seconds) {
+        m_seconds_to_adjust_time_for_testing.store(seconds);
+    }
+
 protected:
     friend class SyncManager;
     void detach_from_sync_manager() REQUIRES(!m_mutex);
@@ -349,6 +353,8 @@ class SyncUser : public std::enable_shared_from_this<SyncUser>, public Subscriba
     const std::string m_device_id;
 
     SyncManager* m_sync_manager;
+
+    std::atomic<int> m_seconds_to_adjust_time_for_testing = 0;
 };
 
 } // namespace realm

test/object-store/sync/app.cpp

@@ -1895,6 +1895,63 @@ TEST_CASE("app: sync integration", "[sync][app]") {
         }
         std::function<util::Optional<Response>(const Request)> hook;
     };
+    SECTION("Fast clock on client") {
+        {
+            TestSyncManager sync_manager(app_config, {});
+            auto app = sync_manager.app();
+            create_user_and_log_in(app);
+            std::shared_ptr<SyncUser> user = app->current_user();
+            SyncTestFile config(app, partition, schema);
+            auto r = Realm::get_shared_realm(config);
+
+            REQUIRE(get_dogs(r).size() == 0);
+            create_one_dog(r);
+            REQUIRE(get_dogs(r).size() == 1);
+        }
+
+        auto transport = std::make_shared<HookedTransport>();
+        app_config.transport = transport;
+
+        TestSyncManager sync_manager(app_config, {});
+        auto app = sync_manager.app();
+        auto creds = create_user_and_log_in(app);
+        std::shared_ptr<SyncUser> user = app->current_user();
+        REQUIRE(user);
+        REQUIRE(!user->access_token_refresh_required());
+        // Make the SyncUser behave as if the client clock is 31 minutes fast, so the token looks expired locallaly
+        // (access tokens have an lifetime of 30 mintutes today).
+        user->set_seconds_to_adjust_time_for_testing(31 * 60);
+        REQUIRE(user->access_token_refresh_required());
+
+        // This assumes that we make an http request for the new token while
+        // already in the WaitingForAccessToken state.
+        std::vector<SyncSession::State> seen_states;
+        transport->hook = [&](const Request) -> util::Optional<Response> {
+            auto user = app->current_user();
+            REQUIRE(user);
+            for (auto session : user->all_sessions()) {
+                seen_states.push_back(session->state());
+
+                // Prior to the fix for #4941, this callback would be called from an infinite loop, always in the
+                // WaitingForAccessToken state.
+                switch (session->state()) {
+                    case SyncSession::State::Active: puts("state: Active"); break;
+                    case SyncSession::State::Inactive: puts("state: Inactive"); break;
+                    case SyncSession::State::Dying: puts("state: Dying"); break;
+                    case SyncSession::State::WaitingForAccessToken: puts("state: WaitingForAccessToken"); break;
+                }
+            }
+            return util::none; // send all requests through http
+        };
+        SyncTestFile config(app, partition, schema);
+        auto r = Realm::get_shared_realm(config);
+        REQUIRE(std::find(begin(seen_states), end(seen_states), SyncSession::State::WaitingForAccessToken) !=
+                end(seen_states));
+        Results dogs = get_dogs(r);
+        REQUIRE(dogs.size() == 1);
+        REQUIRE(dogs.get(0).get<String>("breed") == "bulldog");
+        REQUIRE(dogs.get(0).get<String>("name") == "fido");
+    }
     SECTION("Expired Tokens") {
         sync::AccessToken token;
         {