[ganfra]

Pass &RoomEventCache to aggregate_timeline_for_threads instead of a held StateLockReadGuard, so find_event can re-acquire the state lock without deadlocking.

Should fix deadlock which was randomly happening, preventing event_cache to be updated (and so the room list and timeline).

• I've documented the public API changes in the appropriate changelog files (see Writing changelog entries).
• This PR was made with the help of AI.

Signed-off-by:

[ganfra]

This is not explicitly testing against the deadlock because it's not really easy to test, but ensure the flow still work.

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
2942	1	2941	0

View the top 1 failed test(s) by shortest run time

matrix-sdk-ui::integration::room_list_service::test_room_sorting

Stack Traces | 0.174s run time

thread 'room_list_service::test_room_sorting' (29476) panicked at .../tests/integration/room_list_service.rs:1972:5:
assertion failed: `Some(Remove { index: 4 })` does not match `Some(& VectorDiff :: Set { index : 0, ref value })`
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

[codspeed]

Merging this PR will not alter performance

✅ 50 untouched benchmarks

---

_{Comparing ganfra/event_cache_deadlock (40a664f) with main (8587361)}

[ganfra]

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
2942 1 2941 0
View the top 1 failed test(s) by shortest run time

matrix-sdk-ui::integration::room_list_service::test_room_sorting

Stack Traces | 0.174s run time

thread 'room_list_service::test_room_sorting' (29476) panicked at .../tests/integration/room_list_service.rs:1972:5:
assertion failed: `Some(Remove { index: 4 })` does not match `Some(& VectorDiff :: Set { index : 0, ref value })`
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

To view more test analytics, go to the Test Analytics Dashboard 📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

I need to check if it's a regression or just an assertion change. Looks like the test passes in isolation.

EDIT

The failure is a pre-existing test fragility that this change surfaces, not a regression in behaviour.
test_room_sorting asserts an exact ordering of two diffs: a recency move and a read-receipt Set. These are produced by two different tasks writing to the same RoomInfoNotableUpdate channel:

• the recency move comes from the sync task
• the read-receipt Set comes from the event cache's own spawned room_updates_task

Their relative order is decided by the scheduler, which is why it only fails under CI load. This PR removes a room.state().read() acquisition from the event-cache task's thread-aggregation step, which used to partially serialise it against the sync task. Removing it lets the two tasks drift and resolve the race differently.

[Hywan]

Thanks for the contribution.

Can you explain what problem this PR fixes please? What problem does it solve?

[Hywan]

This function takes a StateLockReadGuard<'_, RoomEventCacheState> to avoid re-acquiring the lock every time we need to find an event (which can happen frequently in this function).

However, I don't see how it could create a deadlock. Can you help me please?

[ganfra]

Holding a read guard across the loop and then calling find_event (which itself does state.read().await) is a recursive read-lock.
From what I understood, writes are prioritized over reads, so if a writer queues in between, the second read().await blocks behind that writer , and the writer blocks behind the still-held first read => deadlock.

[Hywan]

StateLockReadGuard<'_, RoomEventCacheState derefs to RoomEventCacheState. Calling find_event on it doesn't acquire the lock as it's been already acquired:

matrix-rust-sdk/crates/matrix-sdk/src/event_cache/caches/room/state.rs

Lines 210 to 215 in 0cce76d

	pub async fn find_event(
	&self,
	event_id: &EventId,
	) -> Result<Option<(EventLocation, Event)>, EventCacheError> {
	find_event(event_id, &self.room_id, &self.room_linked_chunk, &self.store).await
	}

However, find_event on RoomEventCache does acquire the lock because it needs to access the state (i.e. the RoomEventCacheState):

matrix-rust-sdk/crates/matrix-sdk/src/event_cache/caches/room/mod.rs

Lines 168 to 179 in 0cce76d

	pub async fn find_event(&self, event_id: &EventId) -> Result<Option<Event>> {
	Ok(self
	.inner
	.state
	.read()
	.await?
	.find_event(event_id)
	.await
	.ok()
	.flatten()
	.map(|(_loc, event)| event))
	}

So, no, there is no recursive lock here.

Also, writes are not prioritized over reads:

• When a write lock is acquired, it has an exclusive access to the state, i.e. the lock cannot be acquired by another caller.
• When a read lock is acquire, it has a shared access to the state, i.e. the lock can be acquired by other caller only with a read access.

This lock is fair, i.e. the calling order is preserved, there is no priority, which avoids starvation (all callers are guaranteed to have access to the lock — except if there is a deadlock obviously).

What write operation could forbid the read acquisition here?

[ganfra]

Ok so, let me details a bit more here :

1. There is a single RwLock for the whole event cache.

room.state() and every thread.state() are not independent locks, they're views over one shared StateLock, right? So room.state().read() and thread.state().read() both go through the same RwLock<State>.

2. The re-acquisition is thread.find_event, not room.find_event.

find_event on the guard reuses the already-held state/store. But in the redaction branch of aggregate_timeline_for_threads we also call thread.find_event(...) and that one does re-acquire, because it goes through self.inner.state.read():

So the flow is: hold room.state().read() (read lock 1 on the shared RwLock) for the entire function → inside, thread.find_event takes read lock 2 on that same RwLock, while 1 is still held.

3. Re: "writes are not prioritized over reads" => for tokio::sync::RwLock they are.

The state lock is a tokio::sync::RwLock
Tokio's RwLock is write-preferring for starvation-avoidance: it uses a FIFO queue, and if a writer is waiting, new read acquisitions are blocked until that write completes.

The priority policy of Tokio’s read-write lock is fair (or write-preferring), in order to ensure that readers cannot starve writers. Fairness is ensured using a first-in, first-out queue for the tasks awaiting the lock; a read lock will not be given out until all write lock requests that were queued before it have been acquired and released. This is in contrast to the Rust standard library’s std::sync::RwLock, where the priority policy is dependent on the operating system’s implementation.

Putting it together :

• Task A holds read 1 (room.state().read()), held across the whole aggregation.
• Task B calls state.write() → queued as a pending writer
• Task A reaches thread.find_event → wants read 2 → blocked, because a writer is queued ahead of it in the fair queue.
• B can't proceed because A still holds read 1. A can't proceed because it's waiting on read 2 behind B. → deadlock.

[ganfra]

A very reachable one in practice is automatic back-pagination: RoomPagination → load_more_events_backwards takes state.write() on a task independent of room_updates_task.
Could also be auto_shrink_linked_chunk_task

[Hywan]

After further discussion, it appears the semantics of tokio::sync::RwLock has “changed”, tokio-rs/tokio#7919. The documentation has been updated to bring clarification, but it appears the semantics is very different from what we assumed.

I've 3 solutions but I need to think about it. Meanwhile, we can accept your patch. Please fix the CI and we will be ready.

[Hywan]

Replaced by #6869.