Skip to content
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.

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

MSC4140 delayed event rate limit failures are not handled #18021

Open
hughns opened this issue Dec 10, 2024 · 2 comments · May be fixed by #18018
Open

MSC4140 delayed event rate limit failures are not handled #18021

hughns opened this issue Dec 10, 2024 · 2 comments · May be fixed by #18018
Assignees
Labels

Comments

@hughns
Copy link
Member

hughns commented Dec 10, 2024

As per https://github.com/matrix-org/matrix-spec-proposals/blob/toger5/expiring-events-keep-alive/proposals/4140-delayed-events-futures.md#rate-limiting-at-the-point-of-sending we are applying rate limiting at the point of the delayed event being entered into the DAG.

However, we don't handle this temporary failure and retry.

This means that the delayed event then gets dropped.

This was observed in the wild yesterday.

@hughns hughns linked a pull request Dec 10, 2024 that will close this issue
3 tasks
@hughns
Copy link
Member Author

hughns commented Dec 10, 2024

#18018 contains a proposed fix.

However, the approach in that PR is to simply disable that rate limit.

If that PR is accepted then the should be some follow-up on this issue here to get the original security consideration mitigated. For example, one of:

  • taking account of delayed event scheduling when accepting the delayed event in the first place
  • adding the rate limit back in but handling the rate limit failure sensibly

@anoadragon453
Copy link
Member

@hughns @AndrewFerr I agree that adding handling for exceptions raised by the ratelimiting code and then attempting to send the event with the delay suggested by the rate-limiter makes sense.

My only worry is that you could end up with a very large "backlog" of events, which is completely invisible to the client. I'm not sure if this would hurt the UX of any use cases you're aware of?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants