Attempt to fix issues with locks throughout the application #2351

Open
Jade wants to merge 10 commits from jade/deadlocks into main
Owner
No description provided.
Jade added 10 commits 2026-10-12 17:48:50 +00:00
Do not hold state.mutex across join_remote_room to prevent an ABBA

deadlock against mutex_federation.
Drop old_room_state_lock and new_room_state_lock before updating

parent spaces and child rooms to prevent multi-room lock inversion.
Reuse the already-held main_lock when checking module staleness

in run() instead of re-acquiring server.mods.read().
Change admin service handle to SyncRwLock and copy the handler

function pointer before awaiting execution to prevent deadlock.
Avoid holding the services.manager lock across manager.start(),

manager.stop(), and manager.poll() async invocations.
Convert pending grants and tickets to SyncMutex and scope locks tightly

before awaiting external calls in grant_info and redeem_token.
Change threepid sessions and send_attempts to SyncMutex and avoid

holding the sessions mutex across callers of get_valid_session.
Replace std::sync::Mutex with parking_lot::Mutex (SyncMutex) in
core::metrics::runtime_intervals. Note that info::introspection retains
std::sync::Mutex because parking_lot panics in dtor after thread locals
are destroyed.
Convert in-memory data structures in appservice, rooms::typing, and sync
wakers to use SyncRwLock and SyncMutex. Drop appservice locks before
making outbound network requests in room alias queries and federation
invites.
refactor(uiaa): Defer uiaa_sessions lock until session insert
Some checks failed
Auto Labeler / Apply labels based on changed files (pull_request_target) Successful in 4s
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 7s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 8m11s
Documentation / Build and Deploy Documentation (pull_request) Successful in 10m39s
Checks / Prek / Pre-commit & Formatting (pull_request) Failing after 20m51s
772bfdeb3a
Do not hold the uiaa_sessions lock across device lookup in OAuth or
during initial validation. Lock the map only when inserting the newly
created session.
nex requested changes 2026-10-12 20:19:23 +00:00
Dismissed
@ -80,3 +79,3 @@
.stream_local_users()
.filter_map(async |user_id| {
if self.services.appservice.is_user_id(&user_id).await {
if self.services.appservice.is_user_id(&user_id) {
Owner

I think this can be refactored into

.ready_filter_map(|user_id| { self.services.appservice.is_user_id(&user_id).then_some(user_id) })
I think this can be refactored into ```rs .ready_filter_map(|user_id| { self.services.appservice.is_user_id(&user_id).then_some(user_id) }) ```
src/admin/mod.rs Outdated
@ -51,3 +47,3 @@
/// Uninstall the admin command handler
pub async fn fini(admin_service: &service::admin::Service) {
_ = admin_service.handle.write().await.take();
_ = admin_service.handle.write().take();
Owner

init and fini no longer need to be async

`init` and `fini` no longer need to be async
Lines 622-623
@ -619,6 +619,9 @@ pub(crate) async fn upgrade_room_route(
.await
.ok();
drop(old_room_state_lock);
drop(new_room_state_lock);
Owner

These locks are deliberately held until the end of the function from what I can tell? The space upgrade stuff will flake if the room states are mutated while it's in the middle of migrating

These locks are deliberately held until the end of the function from what I can tell? The space upgrade stuff will flake if the room states are mutated while it's in the middle of migrating
Author
Owner

One of them shouldn't be acquired in the first place, see like 416, but I'll leave that to you to handle in your lock-free events PR

One of them shouldn't be acquired in the first place, see like 416, but I'll leave that to you to handle in your lock-free events PR
nex marked this conversation as resolved
@ -120,65 +121,68 @@ impl Service {
client_secret: &ClientSecret,
send_attempt: usize,
) -> Result<OwnedSessionId> {
let mailer = self.services.mailer.expect_mailer()?;
Owner

Why was this removed?

Why was this removed?
Author
Owner

It wasn't, it was just moved wayyyy down to the bottom of the function

It wasn't, it was just moved wayyyy down to the bottom of the function
Owner

But doesn't having it right at the start mean we can short-circuit any sort of computational logic? Seems like an anti-optimisation?

But doesn't having it right at the start mean we can short-circuit any sort of computational logic? Seems like an anti-optimisation?
Author
Owner

Yeah, probably.

Yeah, probably.
Jade force-pushed jade/deadlocks from 772bfdeb3a
Some checks failed
Auto Labeler / Apply labels based on changed files (pull_request_target) Successful in 4s
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 7s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 8m11s
Documentation / Build and Deploy Documentation (pull_request) Successful in 10m39s
Checks / Prek / Pre-commit & Formatting (pull_request) Failing after 20m51s
to 740f9ef0d6
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 10s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Prek / Pre-commit & Formatting (pull_request) Failing after 1m16s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m26s
Documentation / Build and Deploy Documentation (pull_request) Successful in 10m28s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 8m27s
2026-10-15 10:53:05 +00:00
Compare
Jade force-pushed jade/deadlocks from 740f9ef0d6
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 10s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Prek / Pre-commit & Formatting (pull_request) Failing after 1m16s
Update flake hashes / update-flake-hashes (pull_request) Successful in 1m26s
Documentation / Build and Deploy Documentation (pull_request) Successful in 10m28s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 8m27s
to d9896d7475
Some checks failed
Checks / Changelog / Check changelog is added (pull_request_target) Failing after 6s
Documentation / Build and Deploy Documentation (pull_request) Successful in 1m9s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m12s
Checks / Prek / Clippy and Cargo Tests (pull_request) Has been cancelled
2026-10-15 11:45:43 +00:00
Compare
chore: Changelog
All checks were successful
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 6s
Documentation / Build and Deploy Documentation (pull_request) Successful in 1m8s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m16s
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 8m28s
a23c662e71
Jade changed title from WIP: Attempt to fix issues with locks throughout the application to Attempt to fix issues with locks throughout the application 2026-10-15 11:53:15 +00:00
Jade requested reviews from nex, Owners 2026-10-15 11:53:20 +00:00
nex approved these changes 2026-10-15 14:13:50 +00:00
All checks were successful
Checks / Changelog / Check changelog is added (pull_request_target) Successful in 6s
Required
Details
Documentation / Build and Deploy Documentation (pull_request) Successful in 1m8s
Checks / Prek / Check changed files (pull_request) Successful in 6s
Required
Details
Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m16s
Required
Details
Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 8m28s
Required
Details
This pull request has changes conflicting with the target branch.
  • src/service/rooms/membership/mod.rs
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin jade/deadlocks:jade/deadlocks
git switch jade/deadlocks
Sign in to join this conversation.
No reviewers
nex
No milestone
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
continuwuation/continuwuity!2351
No description provided.