Skip to content

feat(sdk): Support refreshing the access token in Client::fetch_server_versions#5916

Merged
andybalaam merged 4 commits into
matrix-org:mainfrom
zecakeh:expired-access-token
Dec 2, 2025
Merged

feat(sdk): Support refreshing the access token in Client::fetch_server_versions#5916
andybalaam merged 4 commits into
matrix-org:mainfrom
zecakeh:expired-access-token

Conversation

@zecakeh
Copy link
Copy Markdown
Collaborator

@zecakeh zecakeh commented Dec 2, 2025

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 request 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.

Allows to have stricter visibility for the fields and put less code in
ClientBuilder.

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
@zecakeh zecakeh requested a review from a team as a code owner December 2, 2025 09:15
@zecakeh zecakeh requested review from andybalaam and removed request for a team December 2, 2025 09:15
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 2, 2025

Codecov Report

❌ Patch coverage is 90.85366% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.65%. Comparing base (ab98028) to head (564e652).
⚠️ Report is 6 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/client/mod.rs 94.87% 1 Missing and 3 partials ⚠️
crates/matrix-sdk/src/authentication/oauth/mod.rs 70.00% 1 Missing and 2 partials ⚠️
crates/matrix-sdk/src/http_client/mod.rs 72.72% 2 Missing and 1 partial ⚠️
crates/matrix-sdk/src/test_utils/mocks/mod.rs 91.17% 2 Missing and 1 partial ⚠️
crates/matrix-sdk/src/authentication/mod.rs 93.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5916      +/-   ##
==========================================
- Coverage   88.66%   88.65%   -0.01%     
==========================================
  Files         363      363              
  Lines      104579   104690     +111     
  Branches   104579   104690     +111     
==========================================
+ Hits        92726    92818      +92     
- Misses       7505     7518      +13     
- Partials     4348     4354       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This will allow to handle automatically whether to send an access token
or not on endpoints that don't require it in contexts were can't refresh
it.

We also don't cache calls to GET /versions that were not authenticated,
because they might lack some features compared to an authenticated
request.

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
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 <zecakeh@tedomum.fr>
@zecakeh zecakeh force-pushed the expired-access-token branch from e031e61 to 564e652 Compare December 2, 2025 09:34
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Dec 2, 2025

CodSpeed Performance Report

Merging #5916 will not alter performance

Comparing zecakeh:expired-access-token (564e652) with main (9f02dcd)

Summary

✅ 50 untouched

Copy link
Copy Markdown
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clear, thanks.

@andybalaam andybalaam merged commit c1302c4 into matrix-org:main Dec 2, 2025
53 checks passed
@zecakeh zecakeh deleted the expired-access-token branch December 2, 2025 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants