Repository: matrix-org/synapse

Issue:
`/state` requests can fail with `JSON response exceeded max size 104857600`
the faster joins work uses `/federation/v1/state` requests (they would rarely be used previously), so this has come to a head

```
2022-05-25 13:07:49,271 - synapse.http.matrixfederationclient - 245 - WARNING - sync_partial_state_room-0-$k09nja_-OhO47WmvZHM24HZFuz2YHqNyj3wp-YsUmcg-$foEN5-2T3Z_ADBZbTr_avOBmasj9SoMC1LtUpa8Yf7k - {GET-O-525} [matrix.org] JSON response exceeded max size 104857600 - GET matrix://matrix.org/_matrix/federation/v1/state/%21OGEhHVWSdvArJzumhm%3Amatrix.org?event_id=%24foEN5-2T3Z_ADBZbTr_avOBmasj9SoMC1LtUpa8Yf7k
2022-05-25 13:07:49,271 - synapse.handlers.federation_event - 889 - WARNING - sync_partial_state_room-0-$k09nja_-OhO47WmvZHM24HZFuz2YHqNyj3wp-YsUmcg - Error attempting to resolve state at missing prev_events
2022-05-25 13:07:49,271 - synapse.handlers.federation_event - 889 - WARNING - sync_partial_state_room-0-$k09nja_-OhO47WmvZHM24HZFuz2YHqNyj3wp-YsUmcg - Error attempting to resolve state at missing prev_events
Capture point (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/synapse/src/synapse/synapse/app/homeserver.py", line 424, in main
    run(hs)
  File "/home/synapse/src/synapse/synapse/app/homeserver.py", line 410, in run
    logger=logger,
  File "/home/synapse/src/synapse/synapse/app/_base.py", line 187, in start_reactor
    run()
  File "/home/synapse/src/synapse/synapse/app/_base.py", line 169, in run
    run_command()
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/base.py", line 1315, in run
    self.mainLoop()
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/base.py", line 1328, in mainLoop
    reactorBaseSelf.doIteration(t)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/epollreactor.py", line 244, in doPoll
    log.callWithLogger(selectable, _drdw, selectable, fd, event)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/python/log.py", line 96, in callWithLogger
    return callWithContext({"system": lp}, func, *args, **kw)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/python/log.py", line 80, in callWithContext
    return context.call({ILogContext: newCtx}, func, *args, **kw)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/python/context.py", line 117, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/python/context.py", line 82, in callWithContext
    return func(*args, **kw)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/posixbase.py", line 683, in _doReadOrWrite
    why = selectable.doRead()
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/tcp.py", line 248, in doRead
    return self._dataReceived(data)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/tcp.py", line 253, in _dataReceived
    rval = self.protocol.dataReceived(data)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/endpoints.py", line 147, in dataReceived
    return self._wrappedProtocol.dataReceived(data)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/protocols/tls.py", line 330, in dataReceived
    self._flushReceiveBIO()
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/protocols/tls.py", line 296, in _flushReceiveBIO
    ProtocolWrapper.dataReceived(self, bytes)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/protocols/policies.py", line 110, in dataReceived
    self.wrappedProtocol.dataReceived(data)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/web/_newclient.py", line 1651, in dataReceived
    self._parser.dataReceived(bytes)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/web/_newclient.py", line 384, in dataReceived
    HTTPParser.dataReceived(self, data)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/protocols/basic.py", line 555, in dataReceived
    why = self.rawDataReceived(data)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/web/_newclient.py", line 302, in rawDataReceived
    self.bodyDecoder.dataReceived(data)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/web/http.py", line 2060, in dataReceived
    goOn = getattr(self, "_dataReceived_" + self.state)()
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/web/http.py", line 2038, in _dataReceived_BODY
    self.dataCallback(chunk)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/web/_newclient.py", line 1043, in dispatcher
    return func(*args, **kwargs)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/web/_newclient.py", line 1242, in _bodyDataReceived_CONNECTED
    self._bodyProtocol.dataReceived(data)
  File "/home/synapse/src/synapse/synapse/http/client.py", line 863, in dataReceived
    self.deferred.errback(BodyExceededMaxSize())
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/defer.py", line 700, in errback
    self._startRunCallbacks(fail)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/defer.py", line 763, in _startRunCallbacks
    self._runCallbacks()
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/defer.py", line 858, in _runCallbacks
    current.result, *args, **kwargs
  File "/home/synapse/src/synapse/synapse/util/async_helpers.py", line 664, in failure_cb
    new_d.errback(val)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/defer.py", line 700, in errback
    self._startRunCallbacks(fail)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/defer.py", line 763, in _startRunCallbacks
    self._runCallbacks()
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/defer.py", line 858, in _runCallbacks
    current.result, *args, **kwargs
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/defer.py", line 1750, in gotResult
    current_context.run(_inlineCallbacks, r, gen, status)
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/internet/defer.py", line 1657, in _inlineCallbacks
    cast(Failure, result).throwExceptionIntoGenerator, gen
  File "/home/synapse/poetry-env/matrix-synapse-o-qzlfnx-py3.7/lib/python3.7/site-packages/twisted/python/failure.py", line 514, in throwExceptionIntoGenerator
    return g.throw(self.type, self.value, self.tb)
  File "/home/synapse/src/synapse/synapse/metrics/background_process_metrics.py", line 247, in run
    desc,
  File "/usr/lib/python3.7/logging/__init__.py", line 1418, in exception
    self.error(msg, *args, exc_info=exc_info, **kwargs)
  File "/usr/lib/python3.7/logging/__init__.py", line 1412, in error
    self._log(ERROR, msg, args, **kwargs)
  File "/usr/lib/python3.7/logging/__init__.py", line 1519, in _log
    self.handle(record)
  File "/usr/lib/python3.7/logging/__init__.py", line 1529, in handle
    self.callHandlers(record)
  File "/usr/lib/python3.7/logging/__init__.py", line 1591, in callHandlers
    hdlr.handle(record)
  File "/usr/lib/python3.7/logging/__init__.py", line 905, in handle
    self.emit(record)
  File "/usr/lib/python3.7/logging/handlers.py", line 1234, in emit
    self.flush()
  File "/usr/lib/python3.7/logging/handlers.py", line 1310, in flush
    self.target.handle(record)
  File "/usr/lib/python3.7/logging/__init__.py", line 905, in handle
    self.emit(record)
  File "/usr/lib/python3.7/logging/handlers.py", line 68, in emit
    if self.shouldRollover(record):
  File "/usr/lib/python3.7/logging/handlers.py", line 184, in shouldRollover
    msg = "%s\n" % self.format(record)
  File "/usr/lib/python3.7/logging/__init__.py", line 880, in format
    return fmt.format(record)
  File "/usr/lib/python3.7/logging/__init__.py", line 627, in format
    record.exc_text = self.formatException(record.exc_info)
  File "/home/synapse/src/synapse/synapse/logging/formatter.py", line 50, in formatException
    traceback.print_stack(tb.tb_frame.f_back, None, sio)
Traceback (most recent call last):
  File "/home/synapse/src/synapse/synapse/http/matrixfederationclient.py", line 234, in _handle_response
    length = await make_deferred_yieldable(d)
synapse.http.client.BodyExceededMaxSize

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/synapse/src/synapse/synapse/handlers/federation_event.py", line 853, in _resolve_state_at_missing_prevs
    dest, room_id, p
  File "/home/synapse/src/synapse/synapse/handlers/federation_event.py", line 970, in _get_state_after_missing_prev_event
    await self._get_state_and_persist(destination, room_id, event_id)
  File "/home/synapse/src/synapse/synapse/handlers/federation_event.py", line 1041, in _get_state_and_persist
    destination, room_id, event_id=event_id, room_version=room_version
  File "/home/synapse/src/synapse/synapse/federation/federation_client.py", line 448, in get_room_state
    event_id,
  File "/home/synapse/src/synapse/synapse/federation/transport/client.py", line 110, in get_room_state
    parser=_StateParser(room_version),
  File "/home/synapse/src/synapse/synapse/http/matrixfederationclient.py", line 1093, in get_json
    max_response_size=max_response_size,
  File "/home/synapse/src/synapse/synapse/http/matrixfederationclient.py", line 247, in _handle_response
    raise RequestSendFailed(e, can_retry=False) from e
synapse.api.errors.RequestSendFailed: Failed to send request: BodyExceededMaxSize: 
```


Discussion:

Patch:
diff --git a/changelog.d/12877.bugfix b/changelog.d/12877.bugfix
new file mode 100644
index 000000000..1ecf448ba
--- /dev/null
+++ b/changelog.d/12877.bugfix
@@ -0,0 +1,1 @@
+Fix a bug introduced in Synapse 1.54 which could sometimes cause exceptions when handling federated traffic.
diff --git a/synapse/federation/transport/client.py b/synapse/federation/transport/client.py
index 9ce06dfa2..25df1905c 100644
--- a/synapse/federation/transport/client.py
+++ b/synapse/federation/transport/client.py
@@ -49,11 +49,6 @@ from synapse.types import JsonDict
 
 logger = logging.getLogger(__name__)
 
-# Send join responses can be huge, so we set a separate limit here. The response
-# is parsed in a streaming manner, which helps alleviate the issue of memory
-# usage a bit.
-MAX_RESPONSE_SIZE_SEND_JOIN = 500 * 1024 * 1024
-
 
 class TransportLayerClient:
     """Sends federation HTTP requests to other servers"""
@@ -349,7 +344,6 @@ class TransportLayerClient:
             path=path,
             data=content,
             parser=SendJoinParser(room_version, v1_api=True),
-            max_response_size=MAX_RESPONSE_SIZE_SEND_JOIN,
         )
 
     async def send_join_v2(
@@ -372,7 +366,6 @@ class TransportLayerClient:
             args=query_params,
             data=content,
             parser=SendJoinParser(room_version, v1_api=False),
-            max_response_size=MAX_RESPONSE_SIZE_SEND_JOIN,
         )
 
     async def send_leave_v1(
@@ -1360,6 +1353,11 @@ class SendJoinParser(ByteParser[SendJoinResponse]):
 
     CONTENT_TYPE = "application/json"
 
+    # /send_join responses can be huge, so we override the size limit here. The response
+    # is parsed in a streaming manner, which helps alleviate the issue of memory
+    # usage a bit.
+    MAX_RESPONSE_SIZE = 500 * 1024 * 1024
+
     def __init__(self, room_version: RoomVersion, v1_api: bool):
         self._response = SendJoinResponse([], [], event_dict={})
         self._room_version = room_version
@@ -1427,6 +1425,9 @@ class _StateParser(ByteParser[StateRequestResponse]):
 
     CONTENT_TYPE = "application/json"
 
+    # As with /send_join, /state responses can be huge.
+    MAX_RESPONSE_SIZE = 500 * 1024 * 1024
+
     def __init__(self, room_version: RoomVersion):
         self._response = StateRequestResponse([], [])
         self._room_version = room_version
diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py
index 0b9475deb..db44721ef 100644
--- a/synapse/http/matrixfederationclient.py
+++ b/synapse/http/matrixfederationclient.py
@@ -92,9 +92,6 @@ incoming_responses_counter = Counter(
     "synapse_http_matrixfederationclient_responses", "", ["method", "code"]
 )
 
-# a federation response can be rather large (eg a big state_ids is 50M or so), so we
-# need a generous limit here.
-MAX_RESPONSE_SIZE = 100 * 1024 * 1024
 
 MAX_LONG_RETRIES = 10
 MAX_SHORT_RETRIES = 3
@@ -116,6 +113,11 @@ class ByteParser(ByteWriteable, Generic[T], abc.ABC):
     the content type doesn't match we fail the request.
     """
 
+    # a federation response can be rather large (eg a big state_ids is 50M or so), so we
+    # need a generous limit here.
+    MAX_RESPONSE_SIZE: int = 100 * 1024 * 1024
+    """The largest response this parser will accept."""
+
     @abc.abstractmethod
     def finish(self) -> T:
         """Called when response has finished streaming and the parser should
@@ -203,7 +205,6 @@ async def _handle_response(
     response: IResponse,
     start_ms: int,
     parser: ByteParser[T],
-    max_response_size: Optional[int] = None,
 ) -> T:
     """
     Reads the body of a response with a timeout and sends it to a parser
@@ -215,15 +216,12 @@ async def _handle_response(
         response: response to the request
         start_ms: Timestamp when request was made
         parser: The parser for the response
-        max_response_size: The maximum size to read from the response, if None
-            uses the default.
 
     Returns:
         The parsed response
     """
 
-    if max_response_size is None:
-        max_response_size = MAX_RESPONSE_SIZE
+    max_response_size = parser.MAX_RESPONSE_SIZE
 
     try:
         check_content_type_is(response.headers, parser.CONTENT_TYPE)
@@ -240,7 +238,7 @@ async def _handle_response(
             "{%s} [%s] JSON response exceeded max size %i - %s %s",
             request.txn_id,
             request.destination,
-            MAX_RESPONSE_SIZE,
+            max_response_size,
             request.method,
             request.uri.decode("ascii"),
         )
@@ -772,7 +770,6 @@ class MatrixFederationHttpClient:
         backoff_on_404: bool = False,
         try_trailing_slash_on_400: bool = False,
         parser: Literal[None] = None,
-        max_response_size: Optional[int] = None,
     ) -> Union[JsonDict, list]:
         ...
 
@@ -790,7 +787,6 @@ class MatrixFederationHttpClient:
         backoff_on_404: bool = False,
         try_trailing_slash_on_400: bool = False,
         parser: Optional[ByteParser[T]] = None,
-        max_response_size: Optional[int] = None,
     ) -> T:
         ...
 
@@ -807,7 +803,6 @@ class MatrixFederationHttpClient:
         backoff_on_404: bool = False,
         try_trailing_slash_on_400: bool = False,
         parser: Optional[ByteParser] = None,
-        max_response_size: Optional[int] = None,
     ):
         """Sends the specified json data using PUT
 
@@ -843,8 +838,6 @@ class MatrixFederationHttpClient:
                 enabled.
             parser: The parser to use to decode the response. Defaults to
                 parsing as JSON.
-            max_response_size: The maximum size to read from the response, if None
-                uses the default.
 
         Returns:
             Succeeds when we get a 2xx HTTP response. The
@@ -895,7 +888,6 @@ class MatrixFederationHttpClient:
             response,
             start_ms,
             parser=parser,
-            max_response_size=max_response_size,
         )
 
         return body
@@ -984,7 +976,6 @@ class MatrixFederationHttpClient:
         ignore_backoff: bool = False,
         try_trailing_slash_on_400: bool = False,
         parser: Literal[None] = None,
-        max_response_size: Optional[int] = None,
     ) -> Union[JsonDict, list]:
         ...
 
@@ -999,7 +990,6 @@ class MatrixFederationHttpClient:
         ignore_backoff: bool = ...,
         try_trailing_slash_on_400: bool = ...,
         parser: ByteParser[T] = ...,
-        max_response_size: Optional[int] = ...,
     ) -> T:
         ...
 
@@ -1013,7 +1003,6 @@ class MatrixFederationHttpClient:
         ignore_backoff: bool = False,
         try_trailing_slash_on_400: bool = False,
         parser: Optional[ByteParser] = None,
-        max_response_size: Optional[int] = None,
     ):
         """GETs some json from the given host homeserver and path
 
@@ -1043,9 +1032,6 @@ class MatrixFederationHttpClient:
             parser: The parser to use to decode the response. Defaults to
                 parsing as JSON.
 
-            max_response_size: The maximum size to read from the response. If None,
-                uses the default.
-
         Returns:
             Succeeds when we get a 2xx HTTP response. The
             result will be the decoded JSON body.
@@ -1090,7 +1076,6 @@ class MatrixFederationHttpClient:
             response,
             start_ms,
             parser=parser,
-            max_response_size=max_response_size,
         )
 
         return body


Test patch:
diff --git a/tests/http/test_fedclient.py b/tests/http/test_fedclient.py
index 638babae6..006dbab09 100644
--- a/tests/http/test_fedclient.py
+++ b/tests/http/test_fedclient.py
@@ -26,7 +26,7 @@ from twisted.web.http import HTTPChannel
 
 from synapse.api.errors import RequestSendFailed
 from synapse.http.matrixfederationclient import (
-    MAX_RESPONSE_SIZE,
+    JsonParser,
     MatrixFederationHttpClient,
     MatrixFederationRequest,
 )
@@ -609,9 +609,9 @@ class FederationClientTests(HomeserverTestCase):
         while not test_d.called:
             protocol.dataReceived(b"a" * chunk_size)
             sent += chunk_size
-            self.assertLessEqual(sent, MAX_RESPONSE_SIZE)
+            self.assertLessEqual(sent, JsonParser.MAX_RESPONSE_SIZE)
 
-        self.assertEqual(sent, MAX_RESPONSE_SIZE)
+        self.assertEqual(sent, JsonParser.MAX_RESPONSE_SIZE)
 
         f = self.failureResultOf(test_d)
         self.assertIsInstance(f.value, RequestSendFailed)