Setting rate limits to 0 can confuse clients into infinite send loop

jryans commented 3 years ago

Description

I wanted to disable all Synapse send rate limits for local development. I tried:

rc_messages_per_second: 0
rc_message_burst_count: 0

After restarting and trying to send a message, Synapse would reply with:

{
    "errcode": "M_LIMIT_EXCEEDED",
    "error": "Too Many Requests",
    "retry_after_ms": -1612545994091
}

The negative retry_after_ms value may lead to unexpected client behaviour. Element Web has been fixed just now, but prior to that it would retry infinitely with no delay. 😱

Steps to reproduce

Set the ratelimiting config above
Send a message
Receive an unexpected negative retry_after_ms value in the error reply

Version information

Homeserver: Local dev server
Version: 1.25.0 (b=develop,21a296cd5,dirty)
Install method: pip install -e . from Git clone
Platform: macOS 10.15.7 with Python 3.8.6

helllynx commented 1 year ago

Faced with the same issue on matrixdotorg/synapse:v1.73.0

DMRobertson commented 1 year ago

The relevant logic is https://github.com/matrix-org/synapse/blob/6a6e1e8c0711939338f25d8d41d1e4d33d984949/synapse/api/ratelimiting.py#L294-L309 and https://github.com/matrix-org/synapse/blob/6a6e1e8c0711939338f25d8d41d1e4d33d984949/synapse/api/ratelimiting.py#L95-L204 if anyone wants to take a look.

I would guess that we hit this branch: https://github.com/matrix-org/synapse/blob/6a6e1e8c0711939338f25d8d41d1e4d33d984949/synapse/api/ratelimiting.py#L198-L202

(Aside: can_do_action probably ought to return a duration rather than a timepoint?)

matrix-org / synapse