Add clarification and test to fixed SSS behaviour by erikjohnston · Pull Request #19734 · element-hq/synapse

erikjohnston · 2026-04-27T09:31:51Z

Follow on from #19714, where we should have had an extra comment and test.

Dev notes

Send a SSS response immediately if the config has changed and there are new results to sync #19714 (comment)
Send a SSS response immediately if the config has changed and there are new results to sync #19714 (comment)
Sliding Sync: Rooms with new requested required_state aren't sent until other activity occurs #18844 (comment)

MadLittleMods · 2026-04-28T20:40:45Z

+        # Advance past the timeout to make sure the request finishes. (We do this
+        # to ensure log contexts don't leak between tests).


Which logcontext is being leaked here?

Here was my gut reaction:

Overall, we shouldn't be worrying about LoggingContext problems in the tests,

[...] it's expected that test things run outside of Synapse and by their nature should happen in the sentinel logcontext. And if we really don't want to see these kind of errors, we would need to refactor get_success(...) to look more like get_success(func, OTHER_SERVER, ev) where we call the function with some homeserver_test_case logcontext instead of passing in an awaitable.

-- @MadLittleMods, #18937 (comment)

This was causing test failures. I'm not entirely sure what was going on, but the trial jobs started failing when running in parallel

Finishing a request to be a good citizen seems fine. The extra complexity in the test is a bit distracting and slightly tricky to figure out that it isn't related to what we're testing. Perhaps a better comment could dispel things better as I read but the current language is fine ⏩

Also makes me wonder what happens if the test fails in the middle of a request (seems like we would run into the same problem).

MadLittleMods · 2026-04-28T21:24:33Z

            response_body["rooms"][room_id]["required_state"][0]["event_id"],
            first_event_id,
        )
+


We even have a relevant test for this scenario. i.e. we should have a new test just like that but without the # Send a message so the room comes down sync.

-- @MadLittleMods, #18844 (comment)

Thoughts?

Isn't that basically what we're testing here?

Yes, but this reinvents the wheel. And it would be nice to see the same thing pass without message sending or possibly adapt it so that it doesn't send messages.

At the very least, we should update the comments there to indicate that the message sending isn't necessary anymore (see this test) and move this test closer and possibly align the name better to 'expand'/'retract' just to connect them better (although the current name is fine if it were standalone)

I think having the test cases be separate makes sense. The other ones are testing that we include/exclude the required_state even when the room does appear in the response (due to there being a message). The fact that we immediately return is a very related but distinct thing.

I think we should at-least point out the new test (test_changing_required_state_returns_immediately) in the docstring for test_rooms_required_state_expand_retract_expand

Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>

MadLittleMods · 2026-04-29T15:51:28Z

            response_body["rooms"][room_id]["required_state"][0]["event_id"],
            first_event_id,
        )
+


Yes, but this reinvents the wheel. And it would be nice to see the same thing pass without message sending or possibly adapt it so that it doesn't send messages.

At the very least, we should update the comments there to indicate that the message sending isn't necessary anymore (see this test) and move this test closer and possibly align the name better to 'expand'/'retract' just to connect them better (although the current name is fine if it were standalone)

MadLittleMods · 2026-04-29T15:58:01Z

+        # Advance past the timeout to make sure the request finishes. (We do this
+        # to ensure log contexts don't leak between tests).


Finishing a request to be a good citizen seems fine. The extra complexity in the test is a bit distracting and slightly tricky to figure out that it isn't related to what we're testing. Perhaps a better comment could dispel things better as I read but the current language is fine ⏩

Also makes me wonder what happens if the test fails in the middle of a request (seems like we would run into the same problem).

MadLittleMods · 2026-04-30T20:13:20Z

+        )
+
+        # We should see the new `required_state` immediately without waiting
+        # (for the full timeout, we may need to wait briefly).


Suggested change

# (for the full timeout, we may need to wait briefly).

# (for the full timeout, we may need to wait briefly just for the response to be assembled).

Although this may not be true, see other discussion

MadLittleMods · 2026-04-30T20:14:03Z

+
+        # We should see the new `required_state` immediately without waiting
+        # (for the full timeout, we may need to wait briefly).
+        channel.await_result(timeout_ms=100)


Perhaps this would pass if we just use 0? Are there any clock.call_later things we wait for in /sync response assembly?

Suggested change

channel.await_result(timeout_ms=100)

channel.await_result(timeout_ms=0)

MadLittleMods · 2026-04-30T20:16:19Z

            response_body["rooms"][room_id]["required_state"][0]["event_id"],
            first_event_id,
        )
+


I think we should at-least point out the new test (test_changing_required_state_returns_immediately) in the docstring for test_rooms_required_state_expand_retract_expand

Add note about using 'now_token'

d201b45

erikjohnston force-pushed the erikj/sss_clarification branch 2 times, most recently from 1d9fdc1 to 3149105 Compare April 27, 2026 10:03

erikjohnston added 2 commits April 27, 2026 11:52

Add test that required state is immediately returned

b580c1c

Newsfile

f86094c

erikjohnston force-pushed the erikj/sss_clarification branch from 3149105 to f86094c Compare April 27, 2026 10:52

erikjohnston marked this pull request as ready for review April 27, 2026 12:30

erikjohnston requested a review from a team as a code owner April 27, 2026 12:30

MadLittleMods added the A-Sync label Apr 27, 2026

MadLittleMods mentioned this pull request Apr 27, 2026

Send a SSS response immediately if the config has changed and there are new results to sync #19714

Merged

MadLittleMods reviewed Apr 28, 2026

View reviewed changes

erikjohnston and others added 4 commits April 29, 2026 13:10

Apply suggestions from code review

812fcbe

Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>

Update newsfile

c8e6e5d

Clarify why we need to use from_token

6d83c1f

Use Duration in tests

39b0b68

erikjohnston requested a review from MadLittleMods April 29, 2026 12:36

MadLittleMods reviewed Apr 29, 2026

View reviewed changes

erikjohnston added 2 commits April 30, 2026 13:16

Use await_result and assertRaises

9966496

Ensure we don't wait for the timeout

9727779

erikjohnston requested a review from MadLittleMods April 30, 2026 12:43

MadLittleMods reviewed Apr 30, 2026

View reviewed changes

		# Advance past the timeout to make sure the request finishes. (We do this
		# to ensure log contexts don't leak between tests).

	# (for the full timeout, we may need to wait briefly).
	# (for the full timeout, we may need to wait briefly just for the response to be assembled).

	channel.await_result(timeout_ms=100)
	channel.await_result(timeout_ms=0)

Conversation

erikjohnston commented Apr 27, 2026 • edited by MadLittleMods Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dev notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikjohnston Apr 29, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

erikjohnston commented Apr 27, 2026 •

edited by MadLittleMods

Loading

erikjohnston Apr 29, 2026 •

edited

Loading