Fix cross-test pollution from engineered homeservers re-using the same server names (fix `TestPartialStateJoin` flakes) by MadLittleMods · Pull Request #880 · matrix-org/complement

MadLittleMods · 2026-06-12T21:21:20Z

What problem are we trying to solve?

This PR is tackling this problem:

Many of the faster joins test flakes are due to the homeserver under test failing to contact Complement homeservers after they have been torn down. When this happens, subsequent tests can fail if they use a Complement homeserver that happens to have the same hostname:port as one which the homeserver under test has previously marked as offline.

-- #626

This problem occurs because as an optimization we try to share the deployment across many tests. To be clear, as a dumb-simple solution, if we created a new deployment for each test, we wouldn't run into this issue (but then the tests would be "slow" again).

What does this PR do?

Spawning from #878 (comment) where I originally thought we could prevent stray requests to a server just from the federation request authentication but turns out, it's already doing the right thing. The only thing we can do is compare the destination to our server name which VerifyHTTPRequest is already doing. And since the crux of this whole thing is that two Complement engineered servers can use the same recycled port and have identical server names (host.docker.internal:port), it doesn't help us here. The rest of the public key stuff that is part of federation requests is just for the origin server to sign itself with, for authenticity.

destination: [Added in v1.3] the server name of the receiving server. This is the same as the destination field from the JSON described in step 1. For compatibility with older servers, recipients should accept requests without this parameter, but MUST always send it. If this property is included, but the value does not match the receiving server’s name, the receiving server must deny the request with an HTTP status code 401 Unauthorized.

-- https://spec.matrix.org/v1.18/server-server-api/#authentication

Instead, this PR takes an alternative route where we instead never re-use the same server_name for a Complement engineered homeserver. This means that even if the real homeserver reaches out to the torn-down engineered homeservers, it just hits 'connection refused' instead of a live, unrelated server. No cross-test pollution.

In the context of tests/msc3902/federation_room_join_partial_state_test.go, it means the engineered homeserver will never see a t.Errorf("Received unexpected PDU", ...) problem.

How do we currently deal with this?

Currently, we try to workaround the stray request problem by making sure the engineered homeserver leaves any rooms it shares with the real homeserver. This means the real homeserver has no logical reason to contact the other server but it's still technically possible that it might reach out because of some queued federation traffic, etc. I find this approach/pattern pretty bad (especially in the way it's currently implemented). See the "Fix" explanation on #878 for more info.

See all of the logic around partialStateJoinResult.Destroy and WithWaitForLeave to see how messy this gets and all of the logic that we get to remove because of this new approach.

Dev notes

:0 behavior with https://pkg.go.dev/net#Listen

Todo

Remove cleanup from tests/msc3902/federation_room_join_partial_state_test.go which is no longer necessary
- To avoid muddying the purpose and focus of this PR I'll tackle this in a follow-up.
- -> Remove messy WithWaitForLeave test clean-up in TestPartialStateJoin #894
- -> Remove messy Destroy test clean-up in TestPartialStateJoin #895

Pull Request Checklist

Pull request includes a sign off

… process before)

MadLittleMods · 2026-06-12T21:26:58Z

+	// We use this sequential port scan strategy over guess and check with an OS-assigned
+	// port (by using `:0`) as it's more efficient. The OS may recycle and re-use freed
+	// ports meaning we could regress to O(n^2) behavior trying to search for each new
+	// port we want to find.
+	//
+	// Using `:0` means an unused port is automatically picked for us (could be random,
+	// could be the next sequential unused port, we don't know). Ideally, we could ask for
+	// the next unused port after X to avoid a bunch of work. When using using `:0`, the
+	// pathological case that is O(n^2) is if OS hands back next lowest unused port
+	// sequentially which would mean we would have to probe and hold each listener until
+	// we finally got something new.


If you want to compare to the :0 solution, see the implementation in 820b428

I think this is the better way to go.

MadLittleMods · 2026-07-08T16:56:40Z

 	port := ln.Addr().(*net.TCPAddr).Port
 	s.serverName = spec.ServerName(fmt.Sprintf("%s:%d", s.serverName, port))
 	s.listening = true



As a result of this change, we can also remove all of the specialty cleanup code from tests/msc3902/federation_room_join_partial_state_test.go. See all of the logic around partialStateJoinResult.Destroy and WithWaitForLeave to see how messy this gets and all of the logic that we get to remove because of this new approach.

To avoid muddying the purpose and focus of this PR I'll tackle this in a follow-up.

All of the clean up is happening in #894 and #895 🕳️

…eanup

MadLittleMods · 2026-07-13T19:37:43Z

Thanks for the review @erikjohnston 🐿️

…n` (#894) Follow-up to #880 which introduces a better way to avoid the test pollution and obsoletes the need for all of this cleanup logic. See the PR description there for a more complete context on why. `WithWaitForLeave` was originally introduced in this context: > Many of the faster joins test flakes are due to the homeserver under test failing to contact Complement homeservers after they have been torn down. When this happens, subsequent tests can fail if they use a Complement homeserver that happens to have the same `hostname:port` as one which the homeserver under test has previously marked as offline. > > *-- #626 But thanks to #880 we no longer use the same `hostname:port` for engineered homeservers anymore. And we can go even further with cleaning this up by removing the need for `Destroy(...)` altogether, see #895

MadLittleMods added 7 commits June 12, 2026 14:19

Listen for unused port (never used for a engineered homeserver in the…

83b9ceb

… process before)

Try more

c103b26

More better comment

2d51bcc

Try + 1 times

b7be7b6

Sequential port scan

820b428

Fix little things

9f186bc

Better errors

40e09fe

MadLittleMods mentioned this pull request Jun 12, 2026

Fix race in WithWaitForLeave that flaked TestPartialStateJoin/Outgoing_device_list_updates #878

Merged

MadLittleMods commented Jun 12, 2026

View reviewed changes

MadLittleMods commented Jul 8, 2026

View reviewed changes

Merge branch 'main' into madlittlemods/fix-test-partial-state-join-cl…

5eebb48

…eanup

MadLittleMods marked this pull request as ready for review July 8, 2026 17:22

MadLittleMods requested review from a team as code owners July 8, 2026 17:22

erikjohnston approved these changes Jul 13, 2026

View reviewed changes

MadLittleMods added 2 commits July 13, 2026 14:27

Merge branch 'main' into madlittlemods/fix-test-partial-state-join-cl…

b7da95e

…eanup

Fix double word using typo

57c7240

MadLittleMods merged commit f002aff into main Jul 13, 2026
5 of 6 checks passed

MadLittleMods deleted the madlittlemods/fix-test-partial-state-join-cleanup branch July 13, 2026 19:37

This was referenced Jul 13, 2026

Remove messy WithWaitForLeave test clean-up in TestPartialStateJoin #894

Merged

Remove messy Destroy test clean-up in TestPartialStateJoin #895

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix cross-test pollution from engineered homeservers re-using the same server names (fix `TestPartialStateJoin` flakes)#880

Fix cross-test pollution from engineered homeservers re-using the same server names (fix `TestPartialStateJoin` flakes)#880
MadLittleMods merged 10 commits into
mainfrom
madlittlemods/fix-test-partial-state-join-cleanup

MadLittleMods commented Jun 12, 2026 •

edited

Loading

Uh oh!

MadLittleMods Jun 12, 2026

Uh oh!

MadLittleMods Jul 8, 2026 •

edited

Loading

Uh oh!

MadLittleMods Jul 16, 2026

Uh oh!

Uh oh!

MadLittleMods commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

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

Uh oh!

What problem are we trying to solve?

What does this PR do?

How do we currently deal with this?

Dev notes

Todo

Pull Request Checklist

Uh oh!

MadLittleMods Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Jul 8, 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 Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MadLittleMods commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MadLittleMods commented Jun 12, 2026 •

edited

Loading

MadLittleMods Jul 8, 2026 •

edited

Loading