Ensure only one transaction is used for RS input per room (#2178)

* Ensure the input API only uses a single transaction * Remove more of the dead query API call * Tidy up * Fix tests hopefully * Don't do unnecessary work for rooms that don't exist * Improve error, fix another case where transaction wasn't used properly * Add a unit test for checking single transaction on RS input API * Fix logic oops when deciding whether to use a transaction in storeEvent
matrix-org · Feb 11, 2022 · 5106cc8 · 5106cc8
1 parent a4e7d47
commit 5106cc8
Show file tree

Hide file tree

Showing 13 changed files with 209 additions and 212 deletions.
diff --git a/federationapi/routing/send_test.go b/federationapi/routing/send_test.go
@@ -93,11 +93,10 @@ func (o *testEDUProducer) InputCrossSigningKeyUpdate(
 
 type testRoomserverAPI struct {
 	api.RoomserverInternalAPITrace
-	inputRoomEvents            []api.InputRoomEvent
-	queryMissingAuthPrevEvents func(*api.QueryMissingAuthPrevEventsRequest) api.QueryMissingAuthPrevEventsResponse
-	queryStateAfterEvents      func(*api.QueryStateAfterEventsRequest) api.QueryStateAfterEventsResponse
-	queryEventsByID            func(req *api.QueryEventsByIDRequest) api.QueryEventsByIDResponse
-	queryLatestEventsAndState  func(*api.QueryLatestEventsAndStateRequest) api.QueryLatestEventsAndStateResponse
+	inputRoomEvents           []api.InputRoomEvent
+	queryStateAfterEvents     func(*api.QueryStateAfterEventsRequest) api.QueryStateAfterEventsResponse
+	queryEventsByID           func(req *api.QueryEventsByIDRequest) api.QueryEventsByIDResponse
+	queryLatestEventsAndState func(*api.QueryLatestEventsAndStateRequest) api.QueryLatestEventsAndStateResponse
 }
 
 func (t *testRoomserverAPI) InputRoomEvents(
@@ -140,20 +139,6 @@ func (t *testRoomserverAPI) QueryStateAfterEvents(
 	return nil
 }
 
-// Query the state after a list of events in a room from the room server.
-func (t *testRoomserverAPI) QueryMissingAuthPrevEvents(
-	ctx context.Context,
-	request *api.QueryMissingAuthPrevEventsRequest,
-	response *api.QueryMissingAuthPrevEventsResponse,
-) error {
-	response.RoomVersion = testRoomVersion
-	res := t.queryMissingAuthPrevEvents(request)
-	response.RoomExists = res.RoomExists
-	response.MissingAuthEventIDs = res.MissingAuthEventIDs
-	response.MissingPrevEventIDs = res.MissingPrevEventIDs
-	return nil
-}
-
 // Query a list of events by event ID.
 func (t *testRoomserverAPI) QueryEventsByID(
 	ctx context.Context,
@@ -312,15 +297,7 @@ func assertInputRoomEvents(t *testing.T, got []api.InputRoomEvent, want []*gomat
 // The purpose of this test is to check that receiving an event over federation for which we have the prev_events works correctly, and passes it on
 // to the roomserver. It's the most basic test possible.
 func TestBasicTransaction(t *testing.T) {
-	rsAPI := &testRoomserverAPI{
-		queryMissingAuthPrevEvents: func(req *api.QueryMissingAuthPrevEventsRequest) api.QueryMissingAuthPrevEventsResponse {
-			return api.QueryMissingAuthPrevEventsResponse{
-				RoomExists:          true,
-				MissingAuthEventIDs: []string{},
-				MissingPrevEventIDs: []string{},
-			}
-		},
-	}
+	rsAPI := &testRoomserverAPI{}
 	pdus := []json.RawMessage{
 		testData[len(testData)-1], // a message event
 	}
@@ -332,15 +309,7 @@ func TestBasicTransaction(t *testing.T) {
 // The purpose of this test is to check that if the event received fails auth checks the event is still sent to the roomserver
 // as it does the auth check.
 func TestTransactionFailAuthChecks(t *testing.T) {
-	rsAPI := &testRoomserverAPI{
-		queryMissingAuthPrevEvents: func(req *api.QueryMissingAuthPrevEventsRequest) api.QueryMissingAuthPrevEventsResponse {
-			return api.QueryMissingAuthPrevEventsResponse{
-				RoomExists:          true,
-				MissingAuthEventIDs: []string{},
-				MissingPrevEventIDs: []string{},
-			}
-		},
-	}
+	rsAPI := &testRoomserverAPI{}
 	pdus := []json.RawMessage{
 		testData[len(testData)-1], // a message event
 	}

diff --git a/roomserver/api/api.go b/roomserver/api/api.go
@@ -83,13 +83,6 @@ type RoomserverInternalAPI interface {
 		response *QueryStateAfterEventsResponse,
 	) error
 
-	// Query whether the roomserver is missing any auth or prev events.
-	QueryMissingAuthPrevEvents(
-		ctx context.Context,
-		request *QueryMissingAuthPrevEventsRequest,
-		response *QueryMissingAuthPrevEventsResponse,
-	) error
-
 	// Query a list of events by event ID.
 	QueryEventsByID(
 		ctx context.Context,

diff --git a/roomserver/api/api_trace.go b/roomserver/api/api_trace.go
@@ -129,16 +129,6 @@ func (t *RoomserverInternalAPITrace) QueryStateAfterEvents(
 	return err
 }
 
-func (t *RoomserverInternalAPITrace) QueryMissingAuthPrevEvents(
-	ctx context.Context,
-	req *QueryMissingAuthPrevEventsRequest,
-	res *QueryMissingAuthPrevEventsResponse,
-) error {
-	err := t.Impl.QueryMissingAuthPrevEvents(ctx, req, res)
-	util.GetLogger(ctx).WithError(err).Infof("QueryMissingAuthPrevEvents req=%+v res=%+v", js(req), js(res))
-	return err
-}
-
 func (t *RoomserverInternalAPITrace) QueryEventsByID(
 	ctx context.Context,
 	req *QueryEventsByIDRequest,

diff --git a/roomserver/api/query.go b/roomserver/api/query.go
@@ -83,27 +83,6 @@ type QueryStateAfterEventsResponse struct {
 	StateEvents []*gomatrixserverlib.HeaderedEvent `json:"state_events"`
 }
 
-type QueryMissingAuthPrevEventsRequest struct {
-	// The room ID to query the state in.
-	RoomID string `json:"room_id"`
-	// The list of auth events to check the existence of.
-	AuthEventIDs []string `json:"auth_event_ids"`
-	// The list of previous events to check the existence of.
-	PrevEventIDs []string `json:"prev_event_ids"`
-}
-
-type QueryMissingAuthPrevEventsResponse struct {
-	// Does the room exist on this roomserver?
-	// If the room doesn't exist all other fields will be empty.
-	RoomExists bool `json:"room_exists"`
-	// The room version of the room.
-	RoomVersion gomatrixserverlib.RoomVersion `json:"room_version"`
-	// The event IDs of the auth events that we don't know locally.
-	MissingAuthEventIDs []string `json:"missing_auth_event_ids"`
-	// The event IDs of the previous events that we don't know locally.
-	MissingPrevEventIDs []string `json:"missing_prev_event_ids"`
-}
-
 // QueryEventsByIDRequest is a request to QueryEventsByID
 type QueryEventsByIDRequest struct {
 	// The event IDs to look up.

diff --git a/roomserver/internal/input/input_events.go b/roomserver/internal/input/input_events.go
@@ -128,20 +128,24 @@ func (r *Inputer) processRoomEvent(
 		}
 	}
 
-	missingRes := &api.QueryMissingAuthPrevEventsResponse{}
+	// Don't waste time processing the event if the room doesn't exist.
+	// A room entry locally will only be created in response to a create
+	// event.
+	isCreateEvent := event.Type() == gomatrixserverlib.MRoomCreate && event.StateKeyEquals("")
+	if !updater.RoomExists() && !isCreateEvent {
+		return rollbackTransaction, fmt.Errorf("room %s does not exist for event %s", event.RoomID(), event.EventID())
+	}
+
+	var missingAuth, missingPrev bool
 	serverRes := &fedapi.QueryJoinedHostServerNamesInRoomResponse{}
-	if event.Type() != gomatrixserverlib.MRoomCreate || !event.StateKeyEquals("") {
-		missingReq := &api.QueryMissingAuthPrevEventsRequest{
-			RoomID:       event.RoomID(),
-			AuthEventIDs: event.AuthEventIDs(),
-			PrevEventIDs: event.PrevEventIDs(),
-		}
-		if err := r.Queryer.QueryMissingAuthPrevEvents(ctx, missingReq, missingRes); err != nil {
-			return rollbackTransaction, fmt.Errorf("r.Queryer.QueryMissingAuthPrevEvents: %w", err)
+	if !isCreateEvent {
+		missingAuthIDs, missingPrevIDs, err := updater.MissingAuthPrevEvents(ctx, event)
+		if err != nil {
+			return rollbackTransaction, fmt.Errorf("updater.MissingAuthPrevEvents: %w", err)
 		}
+		missingAuth = len(missingAuthIDs) > 0
+		missingPrev = !input.HasState && len(missingPrevIDs) > 0
 	}
-	missingAuth := len(missingRes.MissingAuthEventIDs) > 0
-	missingPrev := !input.HasState && len(missingRes.MissingPrevEventIDs) > 0
 
 	if missingAuth || missingPrev {
 		serverReq := &fedapi.QueryJoinedHostServerNamesInRoomRequest{
@@ -246,14 +250,13 @@ func (r *Inputer) processRoomEvent(
 			missingState := missingStateReq{
 				origin:     input.Origin,
 				inputer:    r,
-				queryer:    r.Queryer,
 				db:         updater,
 				federation: r.FSAPI,
 				keys:       r.KeyRing,
 				roomsMu:    internal.NewMutexByRoom(),
 				servers:    serverRes.ServerNames,
 				hadEvents:  map[string]bool{},
-				haveEvents: map[string]*gomatrixserverlib.HeaderedEvent{},
+				haveEvents: map[string]*gomatrixserverlib.Event{},
 			}
 			if stateSnapshot, err := missingState.processEventWithMissingState(ctx, event, headered.RoomVersion); err != nil {
 				// Something went wrong with retrieving the missing state, so we can't