Skip to content

Fix flaky 3PID inhibit error unit tests #19913

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
anoadragon453 merged 2 commits into from
Jul 6, 2026
+21 −2
Merged

Fix flaky 3PID inhibit error unit tests #19913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76c5665
Fix flaky 3PID inhibit error tests
anoadragon453 Jul 6, 2026
3664bdb
newsfile
anoadragon453 Jul 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/19913.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a flake in 3PID inhibit error unit tests, causing occasional failures in CI.
10 changes: 9 additions & 1 deletion tests/rest/client/test_account.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,13 @@ def test_password_reset_bad_email_inhibit_error(self) -> None:
email = "test@example.com"

client_secret = "foobar"
session_id = self._request_token(email, client_secret)
session_id = self._request_token(
email,
client_secret,
# The endpoint intentionally adds up to 1000ms of jitter to avoid
# leaking whether the email address is bound to an account.
timeout_ms=3000,
Comment on lines +331 to +333

@MadLittleMods MadLittleMods Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests were only waiting 1000ms for the endpoint to return, so a randomly-chosen jitter any higher than ~900ms would cause the test to fail.

I'm not understanding the logic behind 3000ms?

@MadLittleMods MadLittleMods Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should actually be fine with 1000ms (the default).

The only reason it's not is because await_result(...) is a bit flaky. For example, the first advance(0.1) in await_result(...) triggers any scheduled work (like database ThreadPool queries as part of the request), then the 1000ms jitter sleep starts, but await_result(...) can give up after another 0.9s (still 1s total but 0.1s short of the sleep finishing). This edge case happens when the floating point math satisfies self._reactor.seconds() > end_time one iteration early so it waits exactly 1s (10 iterations). The normal case (when everything passes) is because it gets an extra self._reactor.advance(0.1) iteration (11 iterations -> waits 1.1s).

To make it more clear, here is how we could manually drive this to completion reliably:

(test_request_token_existing_email_inhibit_error in tests/rest/client/test_register.py)

channel = self.make_request(
    "POST",
    b"register/email/requestToken",
    {"client_secret": "foobar", "email": email, "send_attempt": 1},
    # We're going to drive the reactor ourselves (just below)
    await_result=False
)
# Wait for database queries to run so we can get the jitter sleep started
self.reactor.advance(0)
# Wait for any potential jitter (up to 1 second)
self.reactor.advance(Duration(seconds=1).as_secs())

Related to #19394 (comment) and #19734 (comment)

I think a better fix would be to update await_result(...) to self._reactor.advance(0) before it starts its loop. This way, we don't burn any reactor advance time on things that are actually scheduled now (get rid of the foot-guns).

I'm working on improving FakeChannel.await_result(...) in #19879 so I'll include the fix there ⏩

Overall, I think this PR should probably be reverted. We could still benefit from the comments pointing out the jitter here though.

@anoadragon453 anoadragon453 Jul 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for diving in deep here! That makes sense, I didn't consider that other work could be delaying the response before the jitter even started.

I noticed that the fix was included in #19879, and the PR has now merged. Nice!

I agree that we no longer need to mess with the timeout in tests if await_result acts as expected, and actually it may be a bad smell to encourage tests to do so (leading to overzealous delays in unit tests). Happy to revert this PR minus the jitter comments.

I'm not understanding the logic behind 3000ms?

It was just an arbitrarily higher value to get substantially far away from 1000ms.

)

self.assertIsNotNone(session_id)

Expand Down Expand Up @@ -364,6 +370,7 @@ def _request_token(
client_secret: str,
ip: str = "127.0.0.1",
next_link: str | None = None,
timeout_ms: int = 1000,

@MadLittleMods MadLittleMods Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use the Duration class

@MadLittleMods MadLittleMods Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 1000 default if that doesn't work in these cases?

@anoadragon453 anoadragon453 Jul 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning was that the jitter was an edge case that only applied to this endpoint, and thus these tests.

@MadLittleMods MadLittleMods Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_request_token(...) is a custom method for the tests for this endpoint (not a general method)

) -> str:
body = {"client_secret": client_secret, "email": email, "send_attempt": 1}
if next_link is not None:
Expand All @@ -373,6 +380,7 @@ def _request_token(
b"account/password/email/requestToken",
body,
client_ip=ip,
timeout_ms=timeout_ms,
)

if channel.code != 200:
Expand Down
3 changes: 3 additions & 0 deletions tests/rest/client/test_register.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,9 @@ def test_request_token_existing_email_inhibit_error(self) -> None:
"POST",
b"register/email/requestToken",
{"client_secret": "foobar", "email": email, "send_attempt": 1},
# The endpoint intentionally adds up to 1000ms of jitter to avoid
# leaking whether the email address is already bound to an account.
timeout_ms=3000,
)
self.assertEqual(200, channel.code, channel.result)

Expand Down
5 changes: 4 additions & 1 deletion tests/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,7 @@ def make_request(
await_result: bool = True,
custom_headers: Iterable[CustomHeaderType] | None = None,
client_ip: str = "127.0.0.1",
timeout_ms: int = 1000,

@MadLittleMods MadLittleMods Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historically, we instead used this pattern:

channel = self.make_request(
    "GET",
    f"/_matrix/media/r0/thumbnail/{self.media_id}{params}",
    await_result=False,
)
channel.await_result(timeout_ms=1000)

The new shortcut doesn't seem bad though.

) -> FakeChannel:
"""
Make a web request using the given method, path and content, and render it
Expand Down Expand Up @@ -399,6 +400,8 @@ def make_request(
custom_headers: (name, value) pairs to add as request headers
client_ip: The IP to use as the requesting IP. Useful for testing
ratelimiting.
timeout_ms: if `await_result` is `True`, the amount of time to wait on
the request before timing out. Ignored otherwise.

Returns:
channel
Expand Down Expand Up @@ -483,7 +486,7 @@ def make_request(
req.requestReceived(method, path, b"1.1")

if await_result:
channel.await_result()
channel.await_result(timeout_ms=timeout_ms)

return channel

Expand Down
4 changes: 4 additions & 0 deletions tests/unittest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,7 @@ def make_request(
await_result: bool = True,
custom_headers: Iterable[CustomHeaderType] | None = None,
client_ip: str = "127.0.0.1",
timeout_ms: int = 1000,
) -> FakeChannel:
"""
Create a SynapseRequest at the path using the method and containing the
Expand Down Expand Up @@ -599,6 +600,8 @@ def make_request(

client_ip: The IP to use as the requesting IP. Useful for testing
ratelimiting.
timeout_ms: if `await_result` is `True`, the amount of time to wait on
the request before timing out. Ignored otherwise.

Returns:
The FakeChannel object which stores the result of the request.
Expand All @@ -618,6 +621,7 @@ def make_request(
await_result,
custom_headers,
client_ip,
timeout_ms,
)

def setup_test_homeserver(
Expand Down
Loading