feat(sdk): Allow to refresh the token in Client::fetch_server_versions

We need to handle 2 possible deadlocks for this:

1. We cannot try to refresh an expired access token if this call happens
   while we are currently trying to refresh the token. The easiest way
   to handle this is to never try to refresh the token when making this
   call inside `get_path_builder_input()` so we implement a "failsafe"
   mode that disables refreshing the access token in case it expired.
   However it attempts the GET /versions again without the token.
2. We cannot access the cached supported versions if we are in the
   process of refreshing that cache because the RwLock has a write lock.
   So if the access token has expired and we try to refresh it, the
   possible calls to `get_path_builder_input()` must not wait for a read
   lock to be available. So the solution is to never wait for a read
   lock, and skip the cache if a read lock is not available.

This also gets rid of workarounds in other functions.

Signed-off-by: Kévin Commaille <[email protected]>

Author: zecakeh

crates/matrix-sdk-ui/src/room_list_service/mod.rs

@@ -63,7 +63,7 @@ use eyeball::Subscriber;
 use futures_util::{Stream, StreamExt, pin_mut};
 use matrix_sdk::{
     Client, Error as SlidingSyncError, Room, SlidingSync, SlidingSyncList, SlidingSyncMode,
-    config::RequestConfig, event_cache::EventCacheError, timeout::timeout,
+    event_cache::EventCacheError, timeout::timeout,
 };
 pub use room_list::*;
 use ruma::{
@@ -165,14 +165,8 @@ impl RoomListService {
             {
                 cached.features
             } else {
-                // Our `/versions` calls don't support token refresh as of now (11.11.2025), so
-                // we're going to skip the sending of the authentication headers in case they
-                // might have expired.
-                //
-                // We only care about a feature which is advertised without being authenticaded
-                // anyways.
                 client
-                    .fetch_server_versions(Some(RequestConfig::new().skip_auth()))
+                    .fetch_server_versions(None)
                     .await
                     .map_err(|e| Error::SlidingSync(e.into()))?
                     .as_supported_versions()

crates/matrix-sdk/src/authentication/oauth/mod.rs

@@ -510,26 +510,17 @@ impl OAuth {
                 .is_some_and(|err| err.status_code == http::StatusCode::NOT_FOUND)
         };
 
-        let response = self
-            .client
-            .send(get_authorization_server_metadata::v1::Request::new())
-            // Skip auth while sending this request. This request itself might not require auth,
-            // but as part of the building of the correct URL we might do a `/versions` call which
-            // optionally accepts it.
-            //
-            // If we're fetching the server metadata to refresh our token, then we don't have valid
-            // auth headers so the `/versions` call will fail and subsequently the refresh of the
-            // token as well.
-            .with_request_config(self.client.request_config().skip_auth())
-            .await
-            .map_err(|error| {
-                // If the endpoint returns a 404, i.e. the server doesn't support the endpoint.
-                if is_endpoint_unsupported(&error) {
-                    OAuthDiscoveryError::NotSupported
-                } else {
-                    error.into()
-                }
-            })?;
+        let response =
+            self.client.send(get_authorization_server_metadata::v1::Request::new()).await.map_err(
+                |error| {
+                    // If the endpoint returns a 404, i.e. the server doesn't support the endpoint.
+                    if is_endpoint_unsupported(&error) {
+                        OAuthDiscoveryError::NotSupported
+                    } else {
+                        error.into()
+                    }
+                },
+            )?;
 
         let metadata = response.metadata.deserialize()?;
 

crates/matrix-sdk/src/client/caches.rs

@@ -22,9 +22,13 @@ use tokio::sync::RwLock;
 /// A collection of in-memory data that the `Client` might want to cache to
 /// avoid hitting the homeserver every time users request the data.
 pub(crate) struct ClientCaches {
-    /// Supported versions, either prefilled during building or fetched from the
-    /// server.
-    pub(super) supported_versions: RwLock<CachedValue<SupportedVersions>>,
+    /// The supported versions of the homeserver.
+    ///
+    /// We only want to cache:
+    ///
+    /// - The versions prefilled with `ClientBuilder::server_versions()`
+    /// - The versions fetched from an *authenticated* request to the server.
+    pub(crate) supported_versions: RwLock<CachedValue<SupportedVersions>>,
     /// Well-known information.
     pub(super) well_known: RwLock<CachedValue<Option<WellKnownResponse>>>,
     pub(crate) server_metadata: tokio::sync::Mutex<TtlCache<String, AuthorizationServerMetadata>>,
@@ -34,7 +38,7 @@ pub(crate) struct ClientCaches {
 /// between a value that is set to `None` (because it doesn't exist) and a value
 /// that has not been cached yet.
 #[derive(Clone)]
-pub(super) enum CachedValue<Value> {
+pub(crate) enum CachedValue<Value> {
     /// A value has been cached.
     Cached(Value),
     /// Nothing has been cached yet.

crates/matrix-sdk/src/client/mod.rs

@@ -1973,11 +1973,73 @@ impl Client {
         &self,
         request_config: Option<RequestConfig>,
     ) -> HttpResult<get_supported_versions::Response> {
-        let server_versions = self
-            .send_inner(get_supported_versions::Request::new(), request_config, Default::default())
-            .await?;
+        // Since this was called by the user, try to refresh the access token if
+        // necessary.
+        self.fetch_server_versions_inner(false, request_config).await
+    }
+
+    /// Fetches server versions from network; no caching.
+    ///
+    /// If the access token is expired and `failsafe` is `false`, this will
+    /// attempt to refresh the access token, otherwise this will try to make an
+    /// unauthenticated request instead.
+    pub(crate) async fn fetch_server_versions_inner(
+        &self,
+        failsafe: bool,
+        request_config: Option<RequestConfig>,
+    ) -> HttpResult<get_supported_versions::Response> {
+        if !failsafe {
+            // `Client::send()` handles refreshing access tokens.
+            return self
+                .send(get_supported_versions::Request::new())
+                .with_request_config(request_config)
+                .await;
+        }
 
-        Ok(server_versions)
+        let homeserver = self.homeserver().to_string();
+
+        // If we have a fresh access token, try with it first.
+        if !request_config.as_ref().is_some_and(|config| config.skip_auth && !config.force_auth)
+            && self.auth_ctx().has_valid_access_token()
+            && let Some(access_token) = self.access_token()
+        {
+            let result = self
+                .inner
+                .http_client
+                .send(
+                    get_supported_versions::Request::new(),
+                    request_config,
+                    homeserver.clone(),
+                    Some(&access_token),
+                    (),
+                    Default::default(),
+                )
+                .await;
+
+            if let Err(Some(ErrorKind::UnknownToken { .. })) =
+                result.as_ref().map_err(HttpError::client_api_error_kind)
+            {
+                // If the access token is actually expired, mark it as expired and fallback to
+                // the unauthenticated request below.
+                self.auth_ctx().set_access_token_expired(&access_token);
+            } else {
+                // If the request succeeded or it's an other error, just stop now.
+                return result;
+            }
+        }
+
+        // Try without authentication.
+        self.inner
+            .http_client
+            .send(
+                get_supported_versions::Request::new(),
+                request_config,
+                homeserver.clone(),
+                None,
+                (),
+                Default::default(),
+            )
+            .await
     }
 
     /// Fetches client well_known from network; no caching.
@@ -2017,7 +2079,13 @@ impl Client {
 
     /// Load supported versions from storage, or fetch them from network and
     /// cache them.
-    async fn load_or_fetch_supported_versions(&self) -> HttpResult<SupportedVersionsResponse> {
+    ///
+    /// If `failsafe` is true, this will try to minimize side effects to avoid
+    /// possible deadlocks.
+    async fn load_or_fetch_supported_versions(
+        &self,
+        failsafe: bool,
+    ) -> HttpResult<SupportedVersionsResponse> {
         match self.state_store().get_kv_data(StateStoreDataKey::SupportedVersions).await {
             Ok(Some(stored)) => {
                 if let Some(supported_versions) =
@@ -2035,7 +2103,7 @@ impl Client {
             }
         }
 
-        let server_versions = self.fetch_server_versions(None).await?;
+        let server_versions = self.fetch_server_versions_inner(failsafe, None).await?;
         let supported_versions = SupportedVersionsResponse {
             versions: server_versions.versions,
             unstable_features: server_versions.unstable_features,
@@ -2097,6 +2165,18 @@ impl Client {
     /// # anyhow::Ok(()) };
     /// ```
     pub async fn supported_versions(&self) -> HttpResult<SupportedVersions> {
+        self.supported_versions_inner(false).await
+    }
+
+    /// Get the Matrix versions and features supported by the homeserver by
+    /// fetching them from the server or the cache.
+    ///
+    /// If `failsafe` is true, this will try to minimize side effects to avoid
+    /// possible deadlocks.
+    pub(crate) async fn supported_versions_inner(
+        &self,
+        failsafe: bool,
+    ) -> HttpResult<SupportedVersions> {
         let cached_supported_versions = &self.inner.caches.supported_versions;
         if let CachedValue::Cached(val) = &*cached_supported_versions.read().await {
             return Ok(val.clone());
@@ -2107,7 +2187,7 @@ impl Client {
             return Ok(val.clone());
         }
 
-        let supported = self.load_or_fetch_supported_versions().await?;
+        let supported = self.load_or_fetch_supported_versions(failsafe).await?;
 
         // Fill both unstable features and server versions at once.
         let mut supported_versions = supported.supported_versions();
@@ -3567,6 +3647,7 @@ pub(crate) mod tests {
 
         let versions_mock = server
             .mock_versions()
+            .expect_default_access_token()
             .ok_with_unstable_features()
             .named("first versions mock")
             .expect(1)
@@ -3588,6 +3669,8 @@ pub(crate) mod tests {
 
         assert!(client.server_versions().await.unwrap().contains(&MatrixVersion::V1_0));
 
+        // The result was cached.
+        assert_matches!(client.supported_versions_cached().await, Ok(Some(_)));
         // This subsequent call hits the in-memory cache.
         assert!(client.server_versions().await.unwrap().contains(&MatrixVersion::V1_0));
 

crates/matrix-sdk/src/http_client/mod.rs

@@ -38,7 +38,7 @@ use ruma::api::{
 use tokio::sync::{Semaphore, SemaphorePermit};
 use tracing::{debug, field::debug, instrument, trace};
 
-use crate::{HttpResult, config::RequestConfig, error::HttpError};
+use crate::{HttpResult, client::caches::CachedValue, config::RequestConfig, error::HttpError};
 
 #[cfg(not(target_family = "wasm"))]
 mod native;
@@ -263,7 +263,26 @@ impl SupportedPathBuilder for path_builder::VersionHistory {
         client: &crate::Client,
         skip_auth: bool,
     ) -> HttpResult<Cow<'static, SupportedVersions>> {
-        if skip_auth {
+        // We always enable "failsafe" mode for the GET /versions requests in this
+        // function. It disables trying to refresh the access token for those requests,
+        // to avoid possible deadlocks.
+
+        if !client.auth_ctx().has_valid_access_token() {
+            // Get the value in the cache without waiting. If the lock is not available, we
+            // are in the middle of refreshing the cache so waiting for it would result in a
+            // deadlock.
+            if let Ok(CachedValue::Cached(versions)) =
+                client.inner.caches.supported_versions.try_read().as_deref()
+            {
+                return Ok(Cow::Owned(versions.clone()));
+            }
+
+            // The request will skip auth so we might not get all the supported features, so
+            // just fetch the supported versions and don't cache them.
+            let response = client.fetch_server_versions_inner(true, None).await?;
+
+            Ok(Cow::Owned(response.as_supported_versions()))
+        } else if skip_auth {
             let cached_versions = client.get_cached_supported_versions().await;
 
             let versions = if let Some(versions) = cached_versions {
@@ -272,14 +291,15 @@ impl SupportedPathBuilder for path_builder::VersionHistory {
                 // If we're skipping auth we might not get all the supported features, so just
                 // fetch the versions and don't cache them.
                 let request_config = RequestConfig::default().retry_limit(5).skip_auth();
-                let response = client.fetch_server_versions(Some(request_config)).await?;
+                let response =
+                    client.fetch_server_versions_inner(true, Some(request_config)).await?;
 
                 response.as_supported_versions()
             };
 
             Ok(Cow::Owned(versions))
         } else {
-            client.supported_versions().await.map(Cow::Owned)
+            client.supported_versions_inner(true).await.map(Cow::Owned)
         }
     }
 }