Define delayed event ratelimit category #18019
Conversation
Apply ratelimiting on delayed event management separately from messages.
| This is a ratelimiting option that ratelimits | ||
| attempts to restart, cancel, or view delayed events | ||
| based on the account the client is using. | ||
| It defaults to: `per_second: 10`, `burst_count: 100`. |
There was a problem hiding this comment.
Why are the defaults so different from rc_message which is what is currently used? I expect the default for rc_delayed_event to be similar.
There was a problem hiding this comment.
The default is more lax to permit refreshing delayed events often / in a loop without risk of ratelimiting, whereas "actual" messages are sent less frequently / not periodically and benefit from a stricter ratelimit.
There was a problem hiding this comment.
@AndrewFerr could you put that justification in the config documentation? I think it would help sysadmins understand why they may want to configure this new ratelimit.
| This is a ratelimiting option that ratelimits | ||
| attempts to restart, cancel, or view delayed events | ||
| based on the account the client is using. | ||
| It defaults to: `per_second: 10`, `burst_count: 100`. |
There was a problem hiding this comment.
@AndrewFerr could you put that justification in the config documentation? I think it would help sysadmins understand why they may want to configure this new ratelimit.
| @@ -227,6 +228,8 @@ async def add( | |||
| Raises: | |||
| SynapseError: if the delayed event fails validation checks. | |||
| """ | |||
| # Use standard request limiter for scheduling new delayed events. | |||
| # TODO: Instead apply rateliming based on the scheduled send time. | |||
There was a problem hiding this comment.
| # TODO: Instead apply rateliming based on the scheduled send time. | |
| # TODO: Instead apply rateliming based on the scheduled send time. | |
| # See https://github.com/element-hq/synapse/issues/18021 |
| self.rc_delayed_event = RatelimitSettings.parse( | ||
| config, | ||
| "rc_delayed_event", | ||
| defaults={"per_second": 10, "burst_count": 100}, |
There was a problem hiding this comment.
10 per second seems quite high - may as well not have a rate-limit at all at that point?
The default for rc_message is 0.2/second. How often are you expecting to refresh delayed events? Would it help to have a rate-limit per-device instead of just per-user?
| @unittest.override_config({"rc_delayed_event": {"per_second": 0.5, "burst_count": 1}}) | ||
| def test_cancel_delayed_event_ratelimit(self) -> None: | ||
| delay_ids = [] | ||
| for i in range(2): |
There was a problem hiding this comment.
| for i in range(2): | |
| for _ in range(2): |
Apply ratelimiting on delayed event management separately from messages.
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.(run the linters)