refactor(event cache): have the caches own the pagination task by bnjbvr · Pull Request #6304 · matrix-org/matrix-rust-sdk

bnjbvr · 2026-03-17T10:44:05Z

This opens the door for running paginations as background tasks. As such, it's a prerequisite for #6014.

The implementation follows the following scheme:

a pagination query will now create a shared() future, that can be polled from several tasks (and it will always return the same results for all the tasks awaiting upon it).
a task is also spawned, to immediately start polling the shared future, before it's returned to the caller
if a caller calls pagination while there's already a pagination happening, the shared future is returned to the caller: there's not a concurrent pagination happening. We simply wait on the current one to finish.

This changes a few test expectations in subtle ways, but that end up in the same timelines, so I didn't pay too much attention.

I've documented the public API Changes in the appropriate CHANGELOG.md files.
This PR was made with the help of AI.

codspeed-hq · 2026-03-17T11:08:45Z

Merging this PR will not alter performance

✅ 50 untouched benchmarks

_{Comparing bnjbvr/single-background-pagination-request (6159151) with main (daa1120)}

codecov · 2026-03-17T11:10:03Z

Codecov Report

❌ Patch coverage is 95.91837% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.82%. Comparing base (daa1120) to head (6159151).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...trix-sdk/src/event_cache/caches/room/pagination.rs	88.00%	3 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/pagination.rs	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6304      +/-   ##
==========================================
- Coverage   89.83%   89.82%   -0.02%     
==========================================
  Files         376      376              
  Lines      102447   102508      +61     
  Branches   102447   102508      +61     
==========================================
+ Hits        92033    92077      +44     
- Misses       6836     6858      +22     
+ Partials     3578     3573       -5

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

This makes it possible to share futures which output is a result that has the error type set to `EventCacheError`. See also #6304 for usage.

Hywan

First review.

Hywan · 2026-03-17T12:36:41Z

crates/matrix-sdk/src/event_cache/caches/pagination.rs

+
+    fn load_more_events_backwards(
+        &self,
+    ) -> impl Future<Output = Result<LoadMoreEventsBackwardsOutcome>> + Send;


Why don't you write async fn load_more_events_backwards instead of return an impl Future?

async is only syntactic sugar for returns a Future which output is the return type; to imply that the returned future is Send, you need to have it appear in the function's signature explicitly. (In a subsequent commit, it even becomes SendOutsideWasm, fwiw.)

crates/matrix-sdk/src/event_cache/caches/pagination.rs

crates/matrix-sdk/src/event_cache/caches/thread/pagination.rs

crates/matrix-sdk/src/event_cache/caches/pagination.rs

This makes it possible to share futures which output is a result that has the error type set to `EventCacheError`. See also #6304 for usage.

Hywan

We are progressing! I made a couple of suggestions, I believe we can simplify some parts.

I would love to see a bit more tests around this new shared mechanism. It works with the existing tests, but we are not testing concurrent pagination runs as far as I know.

crates/matrix-sdk-ui/tests/integration/timeline/pagination.rs

crates/matrix-sdk-ui/src/timeline/pagination.rs

crates/matrix-sdk-ui/tests/integration/timeline/pagination.rs

crates/matrix-sdk/src/event_cache/caches/thread/mod.rs

crates/matrix-sdk/src/event_cache/caches/pagination.rs

Hywan · 2026-03-18T09:30:12Z

crates/matrix-sdk/src/event_cache/caches/room/pagination.rs

+pin_project! {
+    /// A subscriber to a [`PaginationStatus`].
+    ///
+    /// This is a manual implementation of a map function on top of an internal type


I'm struggling to understand why we need a custom map implementation:

Subscriber implements Stream

We can use futures_utils::StreamExt::map to have our mapping.

It might be useful to clone the Subscriber to dealing with lifetime issues.

Did you consider this choice?

See comment below; the subscriber is used in more ways than just a stream.

Hywan · 2026-03-18T09:38:31Z

crates/matrix-sdk/src/event_cache/caches/room/pagination.rs

+    pub fn status(&self) -> PaginationStatusSubscriber {
+        PaginationStatusSubscriber { subscriber: self.0.cache.status().subscribe() }


With StreamExt::map, it might look like:

pub fn status(&self) -> impl Stream<Item = PaginationStatus> { self.0.cache.status().subscribe().map(From::from) }

with a custom:

impl From<SharedPaginationStatus> for PaginationStatus { … }

The thing is that status() is used in more ways: some callers will make use of get(), next() and next_now(), so we can't just replace with a stream and be done with it, unfortunately.

We could plain expose the raw Subscriber<SharedPaginationStatus>, but it seems wrong to have the callers having to call the From/Into implementations themselves?

crates/matrix-sdk/src/event_cache/caches/pagination.rs

bnjbvr · 2026-03-18T09:51:31Z

Thanks for the review! A general comment, before I dive into the detailed comments:

but we are not testing concurrent pagination runs as far as I know.

Have you seen test_concurrent_backpagination? 😁

Even though the task which started the back-pagination is aborted, since it's now the event cache owning it, it keeps on running in the background.

- use the modern MatrixMockServer facilities - provide a previous-batch token so as not to wait for the initial batch token - lower the delay for the /messages responses to 1 sec

…meline updates My only guess for this semantic change, is that the pagination status update and the end of the pagination now happen at different times, or close enough that they're regrouped in the same stream update. This doesn't fundamentally change the semantics, so we'll see if this holds on slower machines (e.g. CI).

For some reason, the previous test didn't take the initial skip count value into account, and now it does. Oh well.

…gination state

bnjbvr · 2026-03-18T10:19:33Z

@Hywan All the things for which you've requested tests are already tested, be it with tests that predated the PR, some of which have been tweaked, or new tests (test_concurrent_backpagination notably). PTAL!

bnjbvr added a commit that referenced this pull request Mar 17, 2026

feat(event cache): make EventCacheError cloneable

e00e051

This makes it possible to share futures which output is a result that has the error type set to `EventCacheError`. See also #6304 for usage.

bnjbvr mentioned this pull request Mar 17, 2026

feat(event cache): make EventCacheError cloneable #6305

Merged

2 tasks

bnjbvr added a commit that referenced this pull request Mar 17, 2026

feat(event cache): make EventCacheError cloneable

424e60a

This makes it possible to share futures which output is a result that has the error type set to `EventCacheError`. See also #6304 for usage.

bnjbvr force-pushed the bnjbvr/single-background-pagination-request branch from fea48b0 to 9e8ad72 Compare March 17, 2026 11:50

bnjbvr marked this pull request as ready for review March 17, 2026 11:54

bnjbvr requested a review from a team as a code owner March 17, 2026 11:54

bnjbvr requested review from poljar and removed request for a team March 17, 2026 11:54

Hywan reviewed Mar 17, 2026

View reviewed changes

bnjbvr added a commit that referenced this pull request Mar 17, 2026

feat(event cache): make EventCacheError cloneable

816530c

This makes it possible to share futures which output is a result that has the error type set to `EventCacheError`. See also #6304 for usage.

bnjbvr added a commit that referenced this pull request Mar 17, 2026

feat(event cache): make EventCacheError cloneable

44eb93f

This makes it possible to share futures which output is a result that has the error type set to `EventCacheError`. See also #6304 for usage.

bnjbvr force-pushed the bnjbvr/single-background-pagination-request branch from 78ff319 to e77b0b9 Compare March 17, 2026 15:42

bnjbvr requested a review from Hywan March 17, 2026 15:49

Hywan requested changes Mar 18, 2026

View reviewed changes

bnjbvr added 11 commits March 18, 2026 11:12

feat(event cache): have the event cache own the back-pagination task

f83776e

fix(test): fix test_back_pagination_aborted expectations

46b506e

Even though the task which started the back-pagination is aborted, since it's now the event cache owning it, it keeps on running in the background.

refactor(test): make test_back_pagination_aborted run faster

511daf7

- use the modern MatrixMockServer facilities - provide a previous-batch token so as not to wait for the initial batch token - lower the delay for the /messages responses to 1 sec

refactor(timeline): simplify code flow in live pagination

255427a

fix(test): fix test expectations in test_skip_count…

5160ea6

For some reason, the previous test didn't take the initial skip count value into account, and now it does. Oh well.

chore: add changelog entry for the new behavior of pagination

0565fed

chore: address first batch of review comments

f8f9129

refactor(event cache): merge the pagination status with the shared pa…

072e966

…gination state

chore: address review comments related to tests

2eb4ded

chore: rename SharedPagination to SharedPaginationTask

6159151

bnjbvr force-pushed the bnjbvr/single-background-pagination-request branch from e77b0b9 to 6159151 Compare March 18, 2026 10:13

bnjbvr requested a review from Hywan March 18, 2026 10:13

poljar removed their request for review March 18, 2026 12:42

		pub fn status(&self) -> PaginationStatusSubscriber {
		PaginationStatusSubscriber { subscriber: self.0.cache.status().subscribe() }

Conversation

bnjbvr commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codspeed-hq bot commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Uh oh!

codecov bot commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Hywan left a comment

Choose a reason for hiding this comment

Uh oh!

Hywan Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

bnjbvr Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Hywan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Hywan Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

bnjbvr Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

Hywan Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

bnjbvr Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bnjbvr commented Mar 18, 2026

Uh oh!

bnjbvr commented Mar 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bnjbvr commented Mar 17, 2026 •

edited

Loading

codspeed-hq bot commented Mar 17, 2026 •

edited

Loading

codecov bot commented Mar 17, 2026 •

edited

Loading