fix(federation): Queue PDUs during remote joins #2156
No reviewers
Labels
No labels
Abandoned
Blocked
Bug
Changelog
Added
Changelog
Missing
Changelog
None
Cherry-picking
Database
Dependencies
Dependencies/Renovate
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/E2EE
Matrix/Federation
Matrix/Hydra
Matrix/MSC
Matrix/Media
Matrix/T&S
Merge
Merge/Manual
Merge/Squash
Meta
Meta/CI
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
Support
bob the builder
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
continuwuation/continuwuity!2156
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "eleboucher/continuwuity:fix/remote-join-pdu-queue"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Queues federation PDUs received during remote joins until the room state is committed, preventing them from being dropped before promotion.
Fixes: #2063
Pull request checklist:
mainbranch, and the branch is named something other thanmain.myself, if applicable. This includes ensuring code compiles.
56ded533459a0c936927@ -0,0 +4,4 @@impl super::Service {pub fn begin_remote_join(&self, room_id: &RoomId) {let (tx, mut rx) = tokio::sync::mpsc::channel(100);The channel buffer capacity could probably be customisable
@ -0,0 +43,4 @@}pub fn cancel_remote_join(&self, room_id: &RoomId) {self.joining_rooms.write().remove(room_id);What happens if the join fails? I think this'll make the queue task leak, since the channel doesn't get closed, no?
if they join i made them closed and discard the queue
Are you sure? I think just removing the room ID from the map (called by the defer! in
join_remote_roomif any error is propagated or when the function ends) will cause therx.recv().awaitcall inbegin_remote_jointo hang forever if the recv channel is never closed and it never receives the Complete enum, leaving the task dangling with no way to interrupt it either. Are you sure you don't need to close the channel or send itsuper::PendingJoinPdu::Completeincancel_remote_join?remove drops the queue’s only Sender, so rx.recv().await returns None and the task exits without processing buffered PDUs.
Oh yes, right you are
@ -0,0 +62,4 @@};let pdu = super::PendingJoinPdu::Pdu(origin.to_owned(),RawJsonValue::from_string(pdu.get().to_owned()).expect("raw PDU JSON is valid"),This seems like one hell of a roundtrip, can't
pdujust be cloned?@ -300,6 +301,8 @@ impl Service {) -> Result {// public so the admin command force-join-room-remotely worksinfo!("Joining {room_id} over federation.");self.services.event_handler.begin_remote_join(room_id);There's no point starting the queue until just before we execute
send_join(since it's at that point the remote server may have persisted our join)@ -677,1 +680,4 @@self.services.sync.wake_all_joined(room_id).await;self.services.event_handler.process_pending_join_pdus(room_id)How does this work if we receive PDUs after the join finishes (meaning the channel is no longer receiving) while the queue is still processing? Won't the live incoming pdus and queued pdus race each other for the persistence lock?
Both queued and live PDUs acquire the same per-room federation mutex, so they cannot process concurrently.
They can't process concurrently, but they could process out of order I think?
Oh I see, the federation mutex is held for the entire queue processing duration
@ -0,0 +11,4 @@};self.services.server.runtime().spawn(async move {let mut pdus = Vec::new();There probably also needs to be a limit to how many PDUs we're willing to queue up to avoid malicious servers sending us many huge transactions to exhaust memory while joining
made it configurable max 50
9a0c9369272dffa36e5222bbce25b52dffa36e522dffa36e523c835ec86f3c835ec86fdf95abf5ab