Uh oh!
There was an error while loading. Please reload this page.
fix: bound streamable HTTP memory usage - #970
Conversation
fcc5928 to
d3176f8Compare| if auth_token.is_none() { | ||
| auth_token = Some(self.get_access_token().await?); | ||
| } | ||
| self.http_client |
There was a problem hiding this comment.
My only Q would be whether we can apply the new defaults at the HTTP client level for all requests, instead of providing these methods for per request type limit setting. If not, feel free to go ahead as-is.
There was a problem hiding this comment.
@alexhancock The value here is actually a single client-wide setting that gets threaded through each call. I see the method signature make it look like a per-request-type limit. I'll add some comment to clear that up.
There was a problem hiding this comment.
My question was more why we needed the new wrapper methods, and why the limit couldn't be applied internally in self.http_client.get_stream_with_max_sse_event_size for example
Just not yet understanding
There was a problem hiding this comment.
@alexhancock The limit isn't stored on the client, so there's nothing for AuthClient to read internally. It lives on the transport config, and the clients are separate objects passed into with_client(client, config). One of them is reqwest::Client, which is a foreign type we can't add a field to, so the value has to come from the worker at call time.
Also, AuthClient isn't where the bounding can happen. The cap is applied at the raw byte layer, where response.bytes_stream() gets wrapped into an SseStream. That only happens in the leaf client (reqwest or unix socket). AuthClient just adds the auth token and forwards to the inner client, so all it can do is pass max_sse_event_size along.
So the value flows from the worker, through AuthClient, down to the client that actually reads the bytes. Dropping the parameter would mean storing the limit on the client, but because it's implemented on the foreign reqwest::Client that needs a wrapper type and removing the bare impl. I'd rather keep that as a separate follow-up.
Motivation and Context
The Streamable HTTP client previously allowed an SSE event to grow indefinitely while waiting for its terminating blank line. The server similarly buffered complete POST bodies without a size limit, allowing a peer to exhaust process memory.
This PR applies configurable limits of 16 MiB per SSE event and 4 MiB per POST body by default. Oversized SSE streams terminate with a typed error without reconnecting, and oversized POST bodies receive HTTP 413.
How Has This Been Tested?
Added regression coverage for unterminated and multi-line SSE events.
Breaking Changes
No source-level breaking changes are expected.
Types of changes
Checklist