test: events sent during remote join handshake should not be lost by FrenchGithubUser · Pull Request #847 · matrix-org/complement

FrenchGithubUser · 2026-03-04T11:23:22Z

Test for element-hq/synapse#19390

Disclaimer

As I'm not super fluent with go, I used a lot of AI to write this test. However, I did review it as best as I could together with someone else from Famedly.

Pull Request Checklist

Pull request includes a sign off

Signed-off-by: Thomas Traineau t.traineau@famedly.com

Copilot

Pull request overview

Adds a regression test to ensure events created during the federation join handshake (between /make_join and /send_join) remain discoverable to the joining server, preventing missed events / unreachable forward extremities during joins.

Changes:

Added TestEventBetweenMakeJoinAndSendJoinIsNotLost to assert intervening events become discoverable (directly or via prev_events references).
Implemented a custom federation /_matrix/federation/v1/send/{transactionID} handler to observe outbound PDUs before the Complement server is in the room.
Added io import to read raw transaction bodies.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

MadLittleMods · 2026-04-23T19:49:42Z

+	// We track the message event ID sent between make_join and send_join.
+	// After send_join, we wait for hs1 to send us either:
+	//  - the message event itself, or
+	//  - any event whose prev_events reference the message (e.g. a dummy event)
+	var messageEventID string
+	messageDiscoverableWaiter := helpers.NewWaiter()
+
+	srv := federation.NewServer(t, deployment,
+		federation.HandleKeyRequests(),
+	)
+	srv.UnexpectedRequestsAreErrors = false
+
+	// Custom /send handler: the Complement server won't be in the room until
+	// send_join completes, so we can't use HandleTransactionRequests (which
+	// requires the room in srv.rooms). Instead we parse the raw transaction.
+	srv.Mux().Handle("/_matrix/federation/v1/send/{transactionID}", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+		body, _ := io.ReadAll(req.Body)
+		txn := gjson.ParseBytes(body)
+		txn.Get("pdus").ForEach(func(_, pdu gjson.Result) bool {
+			eventID := pdu.Get("event_id").String()
+			eventType := pdu.Get("type").String()
+			t.Logf("Received PDU via /send: type=%s id=%s", eventType, eventID)
+
+			if messageEventID == "" {
+				return true
+			}


I'm not a Go expert so I don't know for sure whether this is relevant but it sounds plausible.

Based on https://tip.golang.org/ref/mem, without synchronization, writes are not guaranteed to be observed by other goroutines

the assignment to a is not followed by any synchronization event, so it is not guaranteed to be observed by any other goroutine. In fact, an aggressive compiler might delete the entire go statement.

If the effects of a goroutine must be observed by another goroutine, use a synchronization mechanism such as a lock or channel communication to establish a relative ordering.

MadLittleMods · 2026-03-18T22:20:43Z

+			// Check if this event's prev_events reference the message
+			// (e.g. a dummy event tying the forward extremities together).
+			pdu.Get("prev_events").ForEach(func(_, prevEvent gjson.Result) bool {
+				if prevEvent.String() == messageEventID {
+					messageDiscoverableWaiter.Finish()
+					return false
+				}
+				return true
+			})


Comment why this is good enough.

I assume because people can backfill to get the message.

But then this brings up the question, what if it's a prev_event further back in a chain? (we currently don't detect that as good enough) - we need to at-least explain this edge case although perhaps we should /backfill up to the beginning of the room and ensure the messageEventID was in there.

It looks like this is partially addressed below but we should also explain it here:

// Step 4: hs1 should make the missed message discoverable to the joining // server. We accept either receiving the message event directly, or // receiving any event whose prev_events reference it (allowing the // joining server to backfill).

This was updated to the following:

// Check if this event's prev_events directly reference the message // (e.g. a dummy event tying the two forward extremities together). // If so, the joining server can backfill from that event and will // discover the message. We only check one level of prev_events: // if the reference is deeper in the DAG the joining server can // still reach the message through backfill.

This sounds pretty good now but I think it needs a slight adjustment to set the right expecations:

// Check if this event's prev_events directly reference the message (e.g. a dummy // event tying the two forward extremities together). If so, the joining server // can backfill from that event and will discover the message. // // XXX: We only check one level of prev_events: if the reference is deeper in the // DAG, it's valid and the joining server can still reach the message through // backfill but our checks don't account for that yet (feel free to edit this // assertion if you run into this)

MadLittleMods

Thanks for the updates so far @FrenchGithubUser! This is looking close

MadLittleMods · 2026-04-23T19:38:43Z

+		if err != nil {
+			t.Fatalf("failed to read request body in /send handler: %v", err)
+			return
+		}


We can use must.NotError(...) here

MadLittleMods · 2026-04-23T19:44:04Z

+			// Check if this event's prev_events reference the message
+			// (e.g. a dummy event tying the forward extremities together).
+			pdu.Get("prev_events").ForEach(func(_, prevEvent gjson.Result) bool {
+				if prevEvent.String() == messageEventID {
+					messageDiscoverableWaiter.Finish()
+					return false
+				}
+				return true
+			})


This was updated to the following:

// Check if this event's prev_events directly reference the message // (e.g. a dummy event tying the two forward extremities together). // If so, the joining server can backfill from that event and will // discover the message. We only check one level of prev_events: // if the reference is deeper in the DAG the joining server can // still reach the message through backfill.

This sounds pretty good now but I think it needs a slight adjustment to set the right expecations:

// Check if this event's prev_events directly reference the message (e.g. a dummy // event tying the two forward extremities together). If so, the joining server // can backfill from that event and will discover the message. // // XXX: We only check one level of prev_events: if the reference is deeper in the // DAG, it's valid and the joining server can still reach the message through // backfill but our checks don't account for that yet (feel free to edit this // assertion if you run into this)

MadLittleMods · 2026-04-23T19:49:42Z

+	// We track the message event ID sent between make_join and send_join.
+	// After send_join, we wait for hs1 to send us either:
+	//  - the message event itself, or
+	//  - any event whose prev_events reference the message (e.g. a dummy event)
+	var messageEventID string
+	messageDiscoverableWaiter := helpers.NewWaiter()
+
+	srv := federation.NewServer(t, deployment,
+		federation.HandleKeyRequests(),
+	)
+	srv.UnexpectedRequestsAreErrors = false
+
+	// Custom /send handler: the Complement server won't be in the room until
+	// send_join completes, so we can't use HandleTransactionRequests (which
+	// requires the room in srv.rooms). Instead we parse the raw transaction.
+	srv.Mux().Handle("/_matrix/federation/v1/send/{transactionID}", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+		body, _ := io.ReadAll(req.Body)
+		txn := gjson.ParseBytes(body)
+		txn.Get("pdus").ForEach(func(_, pdu gjson.Result) bool {
+			eventID := pdu.Get("event_id").String()
+			eventType := pdu.Get("type").String()
+			t.Logf("Received PDU via /send: type=%s id=%s", eventType, eventID)
+
+			if messageEventID == "" {
+				return true
+			}


I'm not a Go expert so I don't know for sure whether this is relevant but it sounds plausible.

Based on https://tip.golang.org/ref/mem, without synchronization, writes are not guaranteed to be observed by other goroutines

the assignment to a is not followed by any synchronization event, so it is not guaranteed to be observed by any other goroutine. In fact, an aggressive compiler might delete the entire go statement.

If the effects of a goroutine must be observed by another goroutine, use a synchronization mechanism such as a lock or channel communication to establish a relative ordering.

FrenchGithubUser requested review from a team as code owners March 4, 2026 11:23

FrenchGithubUser force-pushed the remote-join-handshake branch from 3b8e96d to e5b41dc Compare March 4, 2026 11:30

anoadragon453 requested a review from Copilot March 18, 2026 16:26

Copilot started reviewing on behalf of anoadragon453 March 18, 2026 16:27 View session

Copilot AI reviewed Mar 18, 2026

View reviewed changes

MadLittleMods reviewed Mar 18, 2026

View reviewed changes

FrenchGithubUser added 2 commits March 24, 2026 12:15

test: events sent during remote join handshake should not be lost

f55f01b

apply suggestions

0b5cea2

FrenchGithubUser force-pushed the remote-join-handshake branch from 95d0c03 to 0b5cea2 Compare March 24, 2026 11:15

MadLittleMods mentioned this pull request Apr 22, 2026

fix: provide remote servers a way to find out about an event created during the remote join handshake element-hq/synapse#19390

Open

3 tasks

MadLittleMods reviewed Apr 22, 2026

View reviewed changes

Comment thread tests/federation_room_join_test.go

FrenchGithubUser requested a review from MadLittleMods April 23, 2026 08:18

MadLittleMods reviewed Apr 23, 2026

View reviewed changes

Conversation

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

Uh oh!

Disclaimer

Pull Request Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

MadLittleMods Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

FrenchGithubUser Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MadLittleMods left a comment

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FrenchGithubUser commented Mar 4, 2026 •

edited

Loading

MadLittleMods Mar 18, 2026 •

edited

Loading