MSC4140: update error responses by AndrewFerr · Pull Request #19539 · element-hq/synapse

AndrewFerr · 2026-03-09T20:45:50Z

Impose limit of scheduled delayed events
Update error codes to match latest draft of the MSC

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Code style is correct (run the linters)

Prevent users from having more than a configured number of delayed events scheduled at once.

Also don't return a custom error (that was never in the MSC) when adding a delayed event if not enabled in the config

tests/rest/client/test_rooms.py

synapse/config/experimental.py

synapse/rest/client/room.py

Set the new "max_delayed_events_per_user" config in the same module as where the other MSC4140 config ("max_event_delay_duration") is set. Also give it aliases for experimental & stable usage.

in an attempt to better explain what it is and why it is being examined

Don't want a stable alias for it yet, as it may change

Set its value to the remaining time until the requesting user's next delayed event will be sent. Also drop test against the error message of the thrown LimitExceededError, as now the Retry-After is there to clearly distinguish the thrown error as being related to a specific delayed event request.

If delayed events are to be disabled, it should be done by leaving the max_event_delay_duration config unspecified, not by setting the delayed event limit to 0. Also add test coverage, and tweak the error thrown for max_delay_event_ms to be consistent with the newly-added error.

MadLittleMods · 2026-03-16T21:43:18Z

synapse/config/server.py

+# Copyright (C) 2023-2025 New Vector Ltd
+# Copyright (C) 2025-2026 Element Creations Ltd


ehhhh, just leave it be

To be clear, https://gitlab.matrix.org/new-vector/internal/-/wikis/Copyright-headers seems to maybe align with what you're doing but I haven't seen a single other Synapse PR that does this.

Feels like something we should have a lint/tool for if we cared.

As an aside, these copyright headers are so annoying and backwards in my opinion.

Alright, then I'll just strip the related commit from this PR.

MadLittleMods · 2026-03-16T21:53:06Z

synapse/storage/databases/main/delayed_events.py

+            LimitExceededError: if the user has reached the limit of
+                how many delayed events they may have scheduled at once.
        """
+        assert limit > 0  # Should be enforced at config read time


Why do we enforce this? Seems valid

I see this commit message but none of that context made it into the code.

Enforce max delayed event config to be positive

If delayed events are to be disabled, it should be done by leaving the
max_event_delay_duration config unspecified, not by setting the delayed
event limit to 0.

-- 43e14e9

And I don't think it's something for us to worry about at this level (just in the config validation)

MadLittleMods · 2026-03-16T21:53:44Z

synapse/config/server.py

+                raise ConfigError(
+                    "Expected a positive value", ("max_event_delay_duration",)
+                )


Explain more why and what do do.

Enforce max delayed event config to be positive

If delayed events are to be disabled, it should be done by leaving the
max_event_delay_duration config unspecified, not by setting the delayed
event limit to 0.

-- 43e14e9

MadLittleMods · 2026-03-16T21:54:34Z

synapse/storage/databases/main/delayed_events.py

+                )
+                e = LimitExceededError(
+                    limiter_name="add_delayed_event",
+                    retry_after_ms=next_send_ms - creation_ts,


next_send_ms is a timestamp? Needs a better variable name

MadLittleMods · 2026-03-16T21:58:13Z

tests/rest/client/test_rooms.py

+            channel.json_body["errcode"],
+            channel.json_body,
+        )
+        step_ms = 100  # The simulated duration of each make_request


Is this related to the 100 ms from FakeChannel.await_result? (#19487 (comment))

Needs better comment

MadLittleMods · 2026-03-16T22:06:42Z

tests/rest/client/test_rooms.py

+        channel = self.make_request(*args)
+        self.assertEqual(HTTPStatus.TOO_MANY_REQUESTS, channel.code, channel.result)
+        self.assertEqual(
+            Codes.LIMIT_EXCEEDED,
+            channel.json_body["errcode"],
+            channel.json_body,
+        )


Could use a sanity check on the current time to make sure it's not past the first delayed event being sent.

MadLittleMods · 2026-03-16T22:07:10Z

tests/rest/client/test_rooms.py

+        expected_retry_after_ms = send_after_ms - wait_ms - step_ms
+        self.assertEqual(
+            expected_retry_after_ms,
+            channel.json_body["retry_after_ms"],
+            channel.json_body,
+        )
+        retry_header = channel.headers.getRawHeaders("Retry-After")
+        assert retry_header
+        self.assertSequenceEqual(
+            [str(math.ceil(expected_retry_after_ms / 1000))],
+            retry_header,
+        )


We probably don't care about testing the exact value. Just that it's > 0.

There is no comments here what we expect this to be

MadLittleMods · 2026-03-16T22:07:50Z

tests/rest/client/test_rooms.py

+        retry_header = channel.headers.getRawHeaders("Retry-After")
+        assert retry_header


Feels like it could be better to assert a single header and use the single value in whatever comparison we want to do.

Otherwise, it would be better renamed retry_header and explain things.

MadLittleMods · 2026-03-16T22:10:23Z

tests/config/test_server.py


        self.assertEqual(conf["listeners"], expected_listeners)

+    def test_max_delayed_events_enforces_positive(self) -> None:


Explain why (test doc comment)

MadLittleMods · 2026-03-16T22:10:42Z

tests/config/test_server.py

+        with self.assertRaises(ConfigError):
+            _read_config(generate_config(-1))
+
+    def test_max_delayed_events_per_user_enforces_positive(self) -> None:


Explain why (test doc comment)

AndrewFerr added 3 commits March 9, 2026 16:04

MSC4140: impose limit of scheduled delayed events

c2d1039

Prevent users from having more than a configured number of delayed events scheduled at once.

MSC4140: update error codes to match latest MSC

c70c428

Also don't return a custom error (that was never in the MSC) when adding a delayed event if not enabled in the config

Add changelogs

3b51b48

AndrewFerr requested a review from a team as a code owner March 9, 2026 20:45

MadLittleMods reviewed Mar 10, 2026

View reviewed changes

tests/rest/client/test_rooms.py Outdated Show resolved Hide resolved

synapse/config/experimental.py Outdated Show resolved Hide resolved

synapse/rest/client/room.py Show resolved Hide resolved

AndrewFerr added 2 commits March 12, 2026 08:43

Assert against variable message, not literal

ac2bed9

Put MSC4140 config near other one & allow stable

e02f554

Set the new "max_delayed_events_per_user" config in the same module as where the other MSC4140 config ("max_event_delay_duration") is set. Also give it aliases for experimental & stable usage.

MadLittleMods mentioned this pull request Mar 12, 2026

Respond with useful error codes when Content-Length header/s are invalid #19212

Merged

3 tasks

AndrewFerr added 6 commits March 13, 2026 08:12

In test, assign default error message to variable

2e01d7f

in an attempt to better explain what it is and why it is being examined

Explain reason for location of experimental config

79a0218

Leave new config as experimental only

a614ebb

Don't want a stable alias for it yet, as it may change

Update copyright dates

f69ddc1

MadLittleMods reviewed Mar 16, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSC4140: update error responses#19539

MSC4140: update error responses#19539
AndrewFerr wants to merge 11 commits intoelement-hq:developfrom
AndrewFerr:msc4140-error-updates

AndrewFerr commented Mar 9, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MadLittleMods Mar 16, 2026

Uh oh!

AndrewFerr Mar 17, 2026

Uh oh!

MadLittleMods Mar 16, 2026

Uh oh!

MadLittleMods Mar 16, 2026

Uh oh!

MadLittleMods Mar 16, 2026

Uh oh!

MadLittleMods Mar 16, 2026

Uh oh!

MadLittleMods Mar 16, 2026

Uh oh!

MadLittleMods Mar 16, 2026

Uh oh!

MadLittleMods Mar 16, 2026

Uh oh!

MadLittleMods Mar 16, 2026

Uh oh!

MadLittleMods Mar 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		# Copyright (C) 2023-2025 New Vector Ltd
		# Copyright (C) 2025-2026 Element Creations Ltd

		retry_header = channel.headers.getRawHeaders("Retry-After")
		assert retry_header


		self.assertEqual(conf["listeners"], expected_listeners)

		def test_max_delayed_events_enforces_positive(self) -> None:

Conversation

AndrewFerr commented Mar 9, 2026

Pull Request Checklist

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!

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!

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