feat: add discovery_cache_timeout to ClientBuilder and rediscover() t…#6712
feat: add discovery_cache_timeout to ClientBuilder and rediscover() t…#6712SIDHARTH20K4 wants to merge 6 commits into
Conversation
…o Client Signed-off-by: SIDHARTH20K4 <91970588+SIDHARTH20K4@users.noreply.github.com>
Signed-off-by: SIDHARTH20K4 <91970588+SIDHARTH20K4@users.noreply.github.com>
Signed-off-by: SIDHARTH20K4 <91970588+SIDHARTH20K4@users.noreply.github.com>
Merging this PR will improve performance by 48.67%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
|
Signed-off-by: SIDHARTH20K4 <91970588+SIDHARTH20K4@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6712 +/- ##
==========================================
- Coverage 89.90% 89.90% -0.01%
==========================================
Files 398 398
Lines 111296 111325 +29
Branches 111296 111325 +29
==========================================
+ Hits 100062 100083 +21
+ Misses 7435 7433 -2
- Partials 3799 3809 +10 ☔ View full report in Codecov by Harness. |
Signed-off-by: SIDHARTH20K4 <91970588+SIDHARTH20K4@users.noreply.github.com>
Signed-off-by: SIDHARTH20K4 <91970588+SIDHARTH20K4@users.noreply.github.com>
SIDHARTH20K4
requested review from
andybalaam and
Copilot
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
Adds API surface to control Matrix discovery-data caching and to force-refresh that discovery state, aiming to reduce startup-time network requests and homeserver load.
Changes:
- Add
ClientBuilder::discovery_cache_timeout(Duration)with a default of 24h. - Add
Client::rediscover()to force-refresh supported versions, well-known, homeserver capabilities, and OAuth server metadata. - Add a basic unit test covering
rediscover()success with mocked endpoints.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/matrix-sdk/src/client/mod.rs | Wires discovery_cache_timeout through ClientInner, adds Client::rediscover(), and adds a rediscover test. |
| crates/matrix-sdk/src/client/caches.rs | Extends ClientCaches with a discovery_cache_timeout field. |
| crates/matrix-sdk/src/client/builder/mod.rs | Adds builder field/default and the public discovery_cache_timeout(...) setter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// By default, this is 24 hours | ||
| pub fn discovery_cache_timeout(mut self, stale_timeout: Duration) -> Self { | ||
| self.discovery_cache_timeout = stale_timeout; | ||
| self | ||
| } |
| /// If refreshing the well-known info fails, or if the OAuth 2.0 server | ||
| /// metadata request fails for any reason, it is silently ignored. | ||
| /// refresh failures are returned as an error, and any remaining | ||
| /// refreshes that haven't run yet are skipped. |
| match self.oauth().server_metadata().await { | ||
| Ok(_) | Err(_) => {} | ||
| } |
|
Thanks for the Copilot feedback. I noticed that I was thinking of adding a Happy to implement once there's some direction! |
There was a problem hiding this comment.
The formatting here is not right. I think you want backticks where you currently have backslashes.
| } | ||
|
|
||
| #[async_test] | ||
| async fn test_rediscover() { |
There was a problem hiding this comment.
You need a lot more tests. This test does not check that rediscover actually hits the endpoints, or that it uses what is being returned from them. You don't have any tests that check that the timeout is being used, and as far as I can see it's not being used, so as it stands this PR doesn't make any sense at all.
Please check out https://github.com/matrix-org/matrix-rust-sdk/blob/main/CONTRIBUTING.md#ai-policy . I am not convinced that a human has read and understood this code, which would be a violation of our contribution policy.
There was a problem hiding this comment.
Hey @andybalaam,
I'm new to Rust and OSS, and I used AI as a learning tool to understand the codebase and get unstuck on syntax issues. I'll be more careful going forward, especially with PR/issue text, to make sure it's my own words.
You're right about the test, it doesn't properly verify that rediscover() actually hits the endpoints or uses their responses. I'll fix that.
For the timeout issue (also flagged by Copilot, and in my earlier comment), it would help to know if you have a preferred approach before I dig in, since it touches TtlValue and a few call sites.
There was a problem hiding this comment.
Hey, thanks for getting back to me and good luck in your learning journey! I would strongly suggest learning by writing code yourself, rather than trying to adapt or understand code that has been generated by AI. It's particularly key (in my opinion) not to believe what AI says about the code it's written: it is usually giving incorrect information.
I've read your comment about timeouts above several times and I can't understand it. I think what is confusing me is that the code doesn't actually do what it's supposed to do. I suggest you try to write code that solves the problem you've described and then we can talk more easily about whether it's the best way to do it.
By the way, it would be interesting to know a bit more about why you've chosen to work on this problem: is it solving an issue that you are experiencing?
The SDK makes fresh network requests on every startup to discover homeserver versions, well-known info, and other discovery data, even though this data rarely changes. This adds unnecessary latency and load on the homeserver.
This PR adds:
ClientBuilder::discovery_cache_timeout(Duration)to configure how long discovery data is considered fresh (default: 24h)Client::rediscover()to force a refresh of all cached discovery dataCloses #1469
Signed-off-by: Sidharth sidharth.120504@gmail.com