Repository navigation
fix(vertex): stop O(n^2) re-parse of accumulated Gemini stream JSON - #31297
yassin-berriai merged 1 commit into
Conversation
|
|
Greptile SummaryThis PR fixes an O(n²) performance regression in
Confidence Score: 5/5Safe to merge — the change is a six-line, narrowly-scoped guard in a single method, the correctness oracle (json.loads + JSONDecodeError fallback) is preserved, and two new regression tests directly cover the fixed path. The heuristic gate is correct for the Gemini streaming format (responses are always JSON objects or arrays). The rare edge case where a mid-stream fragment ends with No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/llms/vertex_ai/gemini/vertex_and_google_ai_studio_gemini.py | Six-line change in handle_accumulated_json_chunk: adds a heuristic early-exit before json.loads that checks the buffer's last non-whitespace byte. Logic is correct for all Gemini response shapes (always JSON objects/arrays); edge cases where a string value happens to end with } or ] mid-stream will still call json.loads but will get a JSONDecodeError and continue accumulating — unchanged from before for those rare cases. |
| tests/test_litellm/llms/vertex_ai/gemini/test_vertex_and_google_ai_studio_gemini.py | Two new mock-only tests added: one regression test that uses patch("json.loads", wraps=json.loads) to assert parse calls ≤ 2 across 50+ fragments of a 200k-char payload, and one that asserts zero parse calls for a clearly partial fragment. No existing tests are modified. Tests use only mocks and are appropriate for the unit test folder. |
Reviews (1): Last reviewed commit: "fix(vertex): stop O(n^2) re-parse of acc..." | Re-trigger Greptile
Greptile SummaryFixes an O(n²) GIL-holding
Confidence Score: 5/5Safe to merge — the change is a targeted, narrow optimization in a single method with no semantic side-effects and verified benchmark and regression-test evidence. The guard is strictly additive: it skips No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/llms/vertex_ai/gemini/vertex_and_google_ai_studio_gemini.py | Adds an early-exit guard in handle_accumulated_json_chunk that skips json.loads unless the buffer's last non-whitespace byte is } or ], eliminating the O(n²) re-parse that froze the asyncio event loop on large Gemini streaming responses. |
| tests/test_litellm/llms/vertex_ai/gemini/test_vertex_and_google_ai_studio_gemini.py | Adds two mock-based regression tests: one verifying json.loads is called at most twice for a multi-fragment 200 KB payload, and one confirming an incomplete fragment never triggers a parse attempt; both correctly use MagicMock with no real network calls. |
Reviews (2): Last reviewed commit: "fix(vertex): stop O(n^2) re-parse of acc..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
handle_accumulated_json_chunk re-ran json.loads on the entire accumulated buffer after every fragment. For a streaming response fragmented across many chunks that is O(n^2) total work in a single GIL-holding C call, so a large enough Gemini response freezes the asyncio event loop for seconds, liveness probes time out, and the proxy pod gets killed and restarted. A complete Gemini stream value is a JSON object or array, so the buffer can only become parseable once its last non-whitespace byte can close one. Gate the json.loads attempt on that, which makes the common fragmented-response case parse roughly once instead of once per fragment. An 8MB payload drops from a 6.9s event-loop freeze to ~0.3s with identical parsed output. Resolves LIT-3503 Fixes #26181
0dd04e3 to
9a5127b
Compare
| # chunk is a JSON object/array, so only attempt the parse once the | ||
| # buffer's last non-whitespace byte can close one. | ||
| stripped = self.accumulated_json.rstrip() | ||
| if not stripped or stripped[-1] not in "}]": |
There was a problem hiding this comment.
Medium: Stream parsing denial of service
This gate treats any trailing } or ] as a possible JSON terminator, including those inside a quoted model response or tool argument. An authenticated user can request a large output consisting of repeated closing brackets; when that JSON is fragmented, each prefix passes this check and json.loads rescans the entire growing buffer, allowing the request to stall the shared event loop. Track JSON string/escape and nesting state incrementally, or use a bounded incremental parser rather than relying on the final character.
PR overviewThis pull request updates the Vertex AI / Google AI Studio Gemini streaming code to avoid repeatedly reparsing an accumulated JSON buffer while handling streamed Gemini responses. It focuses on improving the stream JSON parsing path for performance during incremental response assembly. There is still an open availability concern in the updated stream parsing logic: certain fragmented streamed outputs containing repeated closing brackets can cause repeated full-buffer JSON parsing. An authenticated user who can trigger large Gemini streaming responses may be able to stall the shared event loop, so the current security posture still has a meaningful denial-of-service risk despite the intended performance fix. Open issues (1)
Fixed/addressed: 0 · PR risk: 5/10 |
Relevant issues
Fixes #26181
Linear ticket
Resolves LIT-3503 (the large-streaming-payload half; the mid-stream 429 half is handled in a separate PR)
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
make test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer review (got 5/5)Type
🐛 Bug Fix
Changes
ModelResponseIterator.handle_accumulated_json_chunkreassembles a Gemini streaming chunk that arrived fragmented across SSE events. It appended each fragment toself.accumulated_jsonand then calledjson.loadson the whole buffer after every fragment. When a single value is split across many fragments that is O(n^2) total work, and eachjson.loadsis one CPython C call that holds the GIL, so a large enough response blocks the asyncio event loop. In the default single-process uvicorn proxy that freezes every handler, the k8s liveness probe times out, and the pod is killed and restarted. This is the behavior the py-spy dumps in #26181 captured (MainThread pinned injson.loadscalled fromhandle_accumulated_json_chunk)A complete Gemini stream value is a JSON object or array, so the buffer can only become parseable once its last non-whitespace byte can close one. The fix gates the
json.loadsattempt on that, so the common fragmented-response case parses roughly once instead of once per fragment.json.loadsstays the correctness oracle (a premature attempt simply raises and keeps accumulating); the gate only decides when it is worth attempting, so output is unchangedScope is the Vertex/Gemini path that LIT-3503's evidence points at. The same copy-paste pattern exists in the Anthropic and SageMaker iterators and can follow in separate PRs
Screenshots / Proof of Fix
Live proxy, one model pointing at a local mock that streams a 3 MB Gemini response fragmented into 64-byte SSE data lines (the accumulate-growth trigger). A background poller hits
/health/livelinessevery 100 ms for 30 s while one streaming request runs, so the worst single probe latency is how long the event loop was frozenSame request, same assembled output (3,145,728 chars), only the proxy code differs:
Before (current
litellm_internal_staging):After (this PR):
The unfixed event loop is frozen for 12.3 s in a single stretch, so only 14 liveness probes get answered in 30 s and any probe with a sub-12 s timeout fails. With the fix the worst probe is 8 ms and the same content streams through correctly
Driving the exact production method directly (so the quadratic is unambiguous), event-loop-blocked time by payload size, 2 KB fragments:
Tests (the regression test fails on current code and passes with the fix):