Draft: Make sure historical state events don't come down `/transactions` for application services (MSC2716) #221

MadLittleMods · 2021-11-05T22:01:51Z

Make sure historical state events doesn't come down /transactions for application services (AS API).

Adding regression tests for matrix-org/synapse#11241

Synapse changes: matrix-org/synapse#11265

Part of MSC2716: matrix-org/matrix-spec-proposals#2716 (comment)

Dev notes

COMPLEMENT_DEBUG=1 COMPLEMENT_ALWAYS_PRINT_SERVER_LOGS=1 COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh TestImportHistoricalMessages/parallel/Historical_events_from_batch_send_do_not_come_down_in_an_incremental_sync

COMPLEMENT_ALWAYS_PRINT_SERVER_LOGS=1 COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh TestImportHistoricalMessages/parallel/Historical_events_from_batch_send_do_not_get_pushed_out_as_application_service_transactions

			logrus.WithFields(logrus.Fields{
				"eventIDAfterHistoricalImport": eventIDAfterHistoricalImport,
				"historicalEventIDs":           historicalEventIDs,
				"historicalStateEventIDs":      historicalStateEventIDs,
			}).Error("afewfeew")

				logrus.WithFields(logrus.Fields{
					"event_id":  r.Get("event_id").Str,
					"type":      r.Get("type").Str,
					"state_key": r.Get("state_key").Str,
				}).Error("event from sync")

https://pkg.go.dev/net/http/httptest#example-Server

ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		fmt.Fprintln(w, "Hello, client")
	}))
	defer ts.Close()

For reference, state events do come down /sync via rooms.join.{roomID}.timeline.events:

{
    "next_batch": "s4278_187546_4_182_390_3_3_40_1",
    "device_one_time_keys_count": {
        "signed_curve25519": 50
    },
    "org.matrix.msc2732.device_unused_fallback_key_types": [
        "signed_curve25519"
    ],
    "rooms": {
        "join": {
            "!mYxoEVsOGiIokKBBOl:my.matrix.host": {
                "timeline": {
                    "events": [
                        {
                            "type": "m.room.topic",
                            "sender": "@gitter-badger:my.matrix.host",
                            "content": {
                                "topic": "foo foo foo bar baz foo foo foo"
                            },
                            "state_key": "",
                            "origin_server_ts": 1636143765046,
                            "unsigned": {
                                "replaces_state": "$MWH9pp5_S6lFcPB_EDDLOnzTxzrUBnSqIhPv3ltZEwg",
                                "prev_content": {
                                    "topic": "foo foo foo bar baz"
                                },
                                "prev_sender": "@gitter-badger:my.matrix.host",
                                "age": 416
                            },
                            "event_id": "$vpQDndTxiKXMXwoNBajmeuTzBnsqmhtLjoCqRwO26Yg"
                        }
                    ],
                    "prev_batch": "s4277_187546_4_182_390_3_3_40_1",
                    "limited": false
                },
                "state": {
                    "events": []
                },
                "account_data": {
                    "events": []
                },
                "ephemeral": {
                    "events": []
                },
                "unread_notifications": {
                    "notification_count": 3,
                    "highlight_count": 0
                },
                "summary": {},
                "org.matrix.msc2654.unread_count": 12
            }
        }
    }
}

MadLittleMods · 2021-11-05T22:03:02Z

tests/msc2716_test.go

@@ -255,7 +256,7 @@ func TestImportHistoricalMessages(t *testing.T) {
 			})
 		})

-		t.Run("Historical events from /batch_send do not come down in an incremental sync", func(t *testing.T) {
+		t.Run("Historical events from batch_send do not come down in an incremental sync", func(t *testing.T) {


Removing the / in the test name so I can actually run it individually, COMPLEMENT_ALWAYS_PRINT_SERVER_LOGS=1 COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh TestImportHistoricalMessages/parallel/Historical_events_from_batch_send_do_not_come_down_in_an_incremental_sync

See https://gist.github.com/MadLittleMods/4ab08f51609fab759247f299a4e33406 for why there is a problem using a / to match a single test.

MadLittleMods · 2021-11-05T22:07:09Z

tests/msc2716_test.go

+			// eventIDAfterHistoricalImport without any the
+			// historicalEventIDs/historicalStateEventIDs in between, we're probably
+			// safe to assume it won't sync.
+			alice.SyncUntil(t, since, "", "rooms.join."+client.GjsonEscape(roomID)+".timeline.events", func(r gjson.Result) bool {


Made this test more clear on what's happening. We now paginate sync from before we /batch_send until an event that occurred after /batch_send and just make sure we didnt' see any historical state or events in between.

MadLittleMods · 2021-11-05T22:07:28Z

tests/msc2716_test.go

+			// historicalEventIDs/historicalStateEventIDs in between, we're probably
+			// safe to assume it won't sync.
+			alice.SyncUntil(t, since, "", "rooms.join."+client.GjsonEscape(roomID)+".timeline.events", func(r gjson.Result) bool {
+				if includes(r.Get("event_id").Str, historicalEventIDs) || includes(r.Get("event_id").Str, historicalStateEventIDs) {


Here is where we additionally check for the historicalStateEventIDs now

Regression tests for matrix-org/synapse#11241 Synapse changes: matrix-org/synapse#11265

…n Complement Complement tests: matrix-org/complement#221

tests/msc2716_test.go

MadLittleMods · 2021-11-06T07:06:09Z

internal/b/blueprints.go

 	"strconv"
 	"strings"
 )

+var (
+	// HostnameRunningComplement is the hostname of Complement from the perspective of a Homeserver.
+	HostnameRunningComplement = "host.docker.internal"


Can't import this from docker.HostnameRunningComplement because then we will have a circular dependency

That doesn't mean you should make the constant here, that means you should move the logic to where we actually care about the host/port aka /docker.

MadLittleMods · 2021-11-06T07:09:04Z

tests/msc2716_test.go

+			if asURLMatches == nil {
+				t.Fatalf("Unable to find application service `url` in registration=%s", asRegistration)
+			}
+			asPort := asURLMatches[2]


Parsing yaml with regex probably isn't very good, "not a regular language, blah" but didn't want to pull in a yaml parser here. Feel free to poke in a different direction.

Just pull in a YAML parser.

MadLittleMods · 2021-11-06T07:10:43Z

internal/b/blueprints.go

+			return as, err
+		}
+
+		as.URL = fmt.Sprintf("http://%s:%d", HostnameRunningComplement, port)


Just checking, do we even care about having a random port for the application service?

Previously I just had it run on port 9111 but changed it to a random port in 06935a7

Yes, tests are run concurrently so we can't bind to the same port all the time.

MadLittleMods · 2021-11-06T07:12:21Z

tests/msc2716_test.go

+			asPort := asURLMatches[2]
+
+			// Create a listener and handler to stub an application service listening
+			// for transactions from a homeserver.


Perhaps we want to create NewApplicationService to mirror federation.NewServer?

The Application service here just needs to listen to a single endpoint though (simple http listener), nothing special.

MadLittleMods · 2021-11-06T07:13:32Z

tests/msc2716_test.go

+				if err != nil {
+					t.Fatalf("Failed to shutdown our fake application service: %s", err)
+				}
+			}()


This is inspired by the Server.Listen code

…tate Conflicts: tests/msc2716_test.go

…via `/transactions` (MSC2716) (#11265) Mark historical state from the MSC2716 `/batch_send` endpoint as `historical` which makes it `backfilled` and have a negative `stream_ordering` so it doesn't get queried by `/transactions`. Fix #11241 Complement tests: matrix-org/complement#221

kegsay

Complement lacks full AS support. This PR adds enough to make it work but in a way I'm unhappy with. This PR also adds in regression tests for /sync which I'm happy with.

Please split out the /sync stuff into a different PR (which can be accepted) and leave this PR with me. I do not want to merge this AS work in its current shape because:

It has concurrency issues.
It asks the test writer to do way too much to set up an AS listener (parsing the AS YAML, manually setting up routers/servers and listening and serving, etc)
The locations of some of this code feel extremely out of place with the rest of the surrounding code.

I'll go through and actually add AS support to Complement, using this test as a guide for how tests will make use of the functionality, and then we can merge in the /transactions bits.

kegsay · 2021-11-19T12:04:57Z

internal/b/blueprints.go

+	if as.URL == "" {
+		// Since, we're just checking and not reserving the port, we could
+		// potentially run into an issue where the port is no longer available when
+		// we actually try to bind to it later on


Tests are run concurrently, so we really do care about this.

kegsay · 2021-11-19T12:05:43Z

internal/b/blueprints.go

@@ -190,6 +196,22 @@ func normaliseUser(u string, hsName string) (string, error) {
 	return u, nil
 }

+// Asks the kernel for a free open port that is ready to use.
+// via https://github.com/phayes/freeport/blob/95f893ade6f232a5f1511d61735d89b1ae2df543/freeport.go#L7-L20
+func getFreePort() (int, error) {


This isn't the right place for this.

kegsay · 2021-11-19T12:06:07Z

internal/b/blueprints.go

+			return as, err
+		}
+
+		as.URL = fmt.Sprintf("http://%s:%d", HostnameRunningComplement, port)


Yes, tests are run concurrently so we can't bind to the same port all the time.

kegsay · 2021-11-19T12:07:29Z

internal/b/blueprints.go

 	"strconv"
 	"strings"
 )

+var (
+	// HostnameRunningComplement is the hostname of Complement from the perspective of a Homeserver.
+	HostnameRunningComplement = "host.docker.internal"


That doesn't mean you should make the constant here, that means you should move the logic to where we actually care about the host/port aka /docker.

kegsay · 2021-11-19T12:09:40Z

tests/msc2716_test.go

+			if asURLMatches == nil {
+				t.Fatalf("Unable to find application service `url` in registration=%s", asRegistration)
+			}
+			asPort := asURLMatches[2]


Just pull in a YAML parser.

Part of MSC2716: matrix-org/matrix-spec-proposals#2716 Related to matrix-org/synapse#11241 Split off from #221

MadLittleMods · 2021-11-24T00:30:59Z

tests/msc2716_test.go

 				}

 				return r.Get("event_id").Str == eventIDAfterHistoricalImport
 			})
 		})

+		t.Run("Historical events from batch_send do not get pushed out as application service transactions", func(t *testing.T) {


Please split out the /sync stuff into a different PR (which can be accepted) and leave this PR with me.

Thanks for the review @kegsay 🐢, I've split out the /sync stuff to #235

…#235) Part of MSC2716: matrix-org/matrix-spec-proposals#2716 Related to matrix-org/synapse#11241 Split off from #221

…tate

kegsay · 2022-02-07T18:53:45Z

I hope to get around to adding AS support to Complement soon.

Also make sure historical state events don't come down /sync

6c1f6a0

MadLittleMods marked this pull request as draft November 5, 2021 22:01

MadLittleMods commented Nov 5, 2021

View reviewed changes

Clean up test

eeb5076

MadLittleMods commented Nov 5, 2021

View reviewed changes

Add comment for includes function

865848d

MadLittleMods mentioned this pull request Nov 5, 2021

Prevent historical state from being pushed to an application service via /transactions (MSC2716) matrix-org/synapse#11265

Merged

4 tasks

Make sure historical events aren't sent as AS transactions

7ab71d4

Regression tests for matrix-org/synapse#11241 Synapse changes: matrix-org/synapse#11265

MadLittleMods added a commit to matrix-org/synapse that referenced this pull request Nov 6, 2021

Add useful debug logs while making a fake AS /transactions endpoint i…

c30a35b

…n Complement Complement tests: matrix-org/complement#221

MadLittleMods changed the title ~~Draft: Make sure historical state events doesn't come down /sync or /transactions (MSC2716)~~ Make sure historical state events doesn't come down incremental /sync or /transactions (MSC2716) Nov 6, 2021

MadLittleMods commented Nov 6, 2021

View reviewed changes

tests/msc2716_test.go Outdated Show resolved Hide resolved

Use random port for application service

06935a7

MadLittleMods commented Nov 6, 2021

View reviewed changes

Explain registration regex

4268459

MadLittleMods commented Nov 6, 2021

View reviewed changes

Unshadow variable

c40bdf1

MadLittleMods marked this pull request as ready for review November 6, 2021 07:18

MadLittleMods requested a review from kegsay November 6, 2021 07:19

MadLittleMods changed the title ~~Make sure historical state events doesn't come down incremental /sync or /transactions (MSC2716)~~ Make sure historical state events don't come down incremental /sync or /transactions (MSC2716) Nov 10, 2021

Merge branch 'master' into madlittlemods/msc2716-no-push-historical-s…

d920d70

…tate Conflicts: tests/msc2716_test.go

kegsay requested changes Nov 19, 2021

View reviewed changes

kegsay mentioned this pull request Nov 19, 2021

Support application services #228

Open

MadLittleMods added a commit that referenced this pull request Nov 23, 2021

Make sure historical state events don't come down incremental /sync

6cf9986

Part of MSC2716: matrix-org/matrix-spec-proposals#2716 Related to matrix-org/synapse#11241 Split off from #221

MadLittleMods mentioned this pull request Nov 23, 2021

Make sure historical state events don't come down incremental /sync (MSC2716) #235

Merged

MadLittleMods commented Nov 24, 2021

View reviewed changes

MadLittleMods changed the title ~~Make sure historical state events don't come down incremental /sync or /transactions (MSC2716)~~ Draft: Make sure historical state events don't come down incremental /sync or /transactions (MSC2716) Nov 24, 2021

MadLittleMods marked this pull request as draft November 24, 2021 00:31

MadLittleMods assigned kegsay Nov 24, 2021

MadLittleMods added a commit that referenced this pull request Nov 24, 2021

Make sure historical state events don't come down incremental /sync (…

6126e9c

…#235) Part of MSC2716: matrix-org/matrix-spec-proposals#2716 Related to matrix-org/synapse#11241 Split off from #221

Merge branch 'master' into madlittlemods/msc2716-no-push-historical-s…

7ce9749

…tate

MadLittleMods changed the title ~~Draft: Make sure historical state events don't come down incremental /sync or /transactions (MSC2716)~~ Draft: Make sure historical state events don't come down /transactions for application services (MSC2716) Nov 24, 2021

kegsay added the blocked label May 11, 2022

MadLittleMods mentioned this pull request Jul 11, 2022

Application service can join remote federated room without profile set #399

Merged

MadLittleMods added 2 commits November 23, 2022 12:06

Merge branch 'main' into madlittlemods/msc2716-no-push-historical-state

4e00b9f

Fix tech debt

12ef80b

MadLittleMods mentioned this pull request Nov 24, 2022

Test to make sure a remote invite event over federation makes it to application service #561

Closed

MadLittleMods closed this Apr 12, 2023

MadLittleMods mentioned this pull request Apr 12, 2023

MSC2716 leftover todo tasks matrix-org/synapse#10737

Closed

27 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Make sure historical state events don't come down `/transactions` for application services (MSC2716) #221

Draft: Make sure historical state events don't come down `/transactions` for application services (MSC2716) #221

MadLittleMods commented Nov 5, 2021 •

edited

Loading

MadLittleMods Nov 5, 2021

MadLittleMods Nov 5, 2021

MadLittleMods Nov 5, 2021

MadLittleMods Nov 6, 2021

kegsay Nov 19, 2021

MadLittleMods Nov 6, 2021 •

edited

Loading

kegsay Nov 19, 2021

MadLittleMods Nov 6, 2021

kegsay Nov 19, 2021

MadLittleMods Nov 6, 2021 •

edited

Loading

MadLittleMods Nov 6, 2021

kegsay left a comment

kegsay Nov 19, 2021

kegsay Nov 19, 2021

kegsay Nov 19, 2021

kegsay Nov 19, 2021

kegsay Nov 19, 2021

MadLittleMods Nov 24, 2021

kegsay commented Feb 7, 2022

Draft: Make sure historical state events don't come down /transactions for application services (MSC2716) #221

Draft: Make sure historical state events don't come down /transactions for application services (MSC2716) #221

Conversation

MadLittleMods commented Nov 5, 2021 • edited Loading

Dev notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadLittleMods Nov 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadLittleMods Nov 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kegsay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kegsay commented Feb 7, 2022

Draft: Make sure historical state events don't come down `/transactions` for application services (MSC2716) #221

Draft: Make sure historical state events don't come down `/transactions` for application services (MSC2716) #221

MadLittleMods commented Nov 5, 2021 •

edited

Loading

MadLittleMods Nov 6, 2021 •

edited

Loading

MadLittleMods Nov 6, 2021 •

edited

Loading