Pre-filtering load limits are not discussed in the spec

[Benjamin-L]

Link to problem area:

I think this should be discussed with the filter schema, and possibly in endpoints that support filtering:

• GET /client/v3/rooms/{roomId}/messages
• GET /client/v3/sync
• GET /client/v3/rooms/{roomId}/context/{eventId}
• POST /client/v3/search

Issue

In a practical filtering implementation, the server has to handle the case where a filter rejects a significant fraction of events. In this situation, finding limit accepted events may involve loading a very large number of rejected events from the database first. This is a DoS vector, and also may result in unacceptably long response latency.

The way current implementations deal with this is imposing a separate "load limit" on the number of events that will be loaded from the database pre-filtering. If the server hits the load limit, it will return the accepted events that it has found so far even if it is less than limit from the filter.

In synapse, load limit is set to max(limit * FACTOR, 10), where FACTOR is a admin-configurable value that defaults to 2. We intend to use the same formula in grapevine's filtering implementation.

The load limit situation is not discussed directly anywhere in the spec. The text for the filter parameter on the /client/v3/rooms/{roomId}/context/{eventId} endpoint does state that

The filter may be applied before or/and after the limit parameter - whichever the homeserver prefers.

Which is similar, although not identical to the load limit behavior in synapse and grapevine. This text states that the server may choose to limit the number of events loaded from the database pre-filtering, but implies that this load limit should be equal to the limit parameter, rather than allowing the load limit to be greater than limit the way that synapse and grapevine do.

I think "the load limit is an implementation detail and doesn't belong in the spec" is a reasonable position, but believe that it should be mentioned in the spec because it has some non-obvious consequences for sync gaps and /messages pagination and because client developers may (incorrectly) expect that limit = N means that they will get N allowed events if N are available.