fix(sidecar): cap concurrent ML segmentation proxy requests - #1631
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe segmentation proxy rejects uploads over 100 MiB and requests above four concurrent operations. It returns HTTP 413 or 429 as applicable and releases in-flight slots after processing. Tests cover both limits and cleanup. ChangesSegmentation proxy safeguards
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ForwardSegment
participant SegmentationBackend
Client->>ForwardSegment: Submit segmentation request
ForwardSegment->>ForwardSegment: Validate size and in-flight count
ForwardSegment->>SegmentationBackend: Stream request body
SegmentationBackend-->>ForwardSegment: Return response or upstream error
ForwardSegment-->>Client: Return response, 413, 429, or 502
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
🔍 Cloudflare PR preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/geolibre_server/geolibre_server/app/ml.py`:
- Around line 77-80: Update _body_iter() to count streamed request bytes and
raise HTTP 413 once the cumulative body exceeds _MAX_SEGMENT_BODY_BYTES,
including requests without Content-Length while preserving normal streaming
below the limit. Add a regression test using a headerless request stream and a
small monkeypatched limit to verify oversized chunked bodies are rejected.
In `@backend/geolibre_server/tests/test_ml.py`:
- Around line 259-278: Update test_segment_releases_slot_after_success to
configure _FakeHttpx.post() to raise its configured HTTPError, then assert the
request returns HTTP 502 and ml._segment_in_flight is 0, covering cleanup after
an upstream failure rather than successful completion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fbab1801-71df-4807-b5bf-8363906af400
📒 Files selected for processing (2)
backend/geolibre_server/geolibre_server/app/ml.pybackend/geolibre_server/tests/test_ml.py
Contributor
🔍 GitHub Pages PR preview
|
Member
|
/claude-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Count bytes in _body_iter() and raise 413 when cumulative size exceeds the cap, so chunked-transfer uploads without Content-Length are also bounded. Add test for upstream failure slot cleanup (502 path) and test for oversized chunked stream rejection.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/geolibre_server/tests/test_ml.py`:
- Around line 358-363: Remove the ineffective TestClient.post assertion from
test_segment_rejects_oversized_chunked_stream, since it does not invoke
_FakeHttpx.request.stream() or exercise the streamed-body limit. Keep the test
focused on the custom _FakeHttpx streaming path that validates the 413 response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e914e472-b23c-40f2-b25a-b006b8128fda
📒 Files selected for processing (2)
backend/geolibre_server/geolibre_server/app/ml.pybackend/geolibre_server/tests/test_ml.py
…limit Use a generator body (no Content-Length) so the oversized-chunked-stream test exercises _body_iter's streaming byte-count check instead of the Content-Length header check. Narrow _FakeHttpx.HTTPError so it does not swallow the FastAPI HTTPException(413) raised inside the generator.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/geolibre_server/tests/test_ml.py`:
- Around line 361-362: Add the project-standard return annotation to the private
generator helper _oversized_body, using the annotation convention already used
for generator helpers in the surrounding test module, while preserving its
existing yielded bytes and behavior.
- Around line 65-66: Annotate the shared calls attribute on _FakeHttpx as a
ClassVar while preserving its existing list initialization and class-level
sharing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 48682917-f46e-45d8-8b0a-9ea2d8278985
📒 Files selected for processing (1)
backend/geolibre_server/tests/test_ml.py
Ayush7614
force-pushed
the
harden/ml-segment-concurrency
branch
from
August 1, 2026 23:18
fc84cf2 to
02322e8
Compare
- Update the stale `_MAX_SEGMENT_BODY_BYTES` comment. It still claimed the cap was unenforced for chunked uploads, which stopped being true in d70a7a4 when `_body_iter` started counting cumulative bytes. Documented both enforcement points (Content-Length up front, streamed bytes as the real backstop). - Document that a concurrency slot is deliberately held for the whole proxied call — launch plus inference — and why that differs from the conversion and Whitebox caps, which enqueue a background job and return immediately. - Annotate the shared `calls` buffers on `_FakeHttpx` / `_ErrorHttpx` as `ClassVar[list]` so the intentional class-level sharing is explicit. - Add the missing `-> Iterator[bytes]` return annotation to `_oversized_body`.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/geolibre_server/tests/test_ml.py (1)
294-312: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
_FakeHTTPErrorin_ErrorHttpx.
HTTPError = Exceptionmakes every exception frompost()match the proxy's HTTPX-error handler. The test can pass whenpost()raises an unrelatedValueErrororTypeError, so it does not isolate upstream failure mapping. (raw.githubusercontent.com)Proposed test fix
class _ErrorHttpx: calls: ClassVar[list] = [] - HTTPError = Exception + HTTPError = _FakeHTTPError ... - raise Exception("backend down") + raise _FakeHTTPError("backend down")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/geolibre_server/tests/test_ml.py` around lines 294 - 312, Update the test double _ErrorHttpx to define and use the dedicated _FakeHTTPError for HTTPError instead of aliasing Exception. Make AsyncClient.post raise _FakeHTTPError for the simulated backend failure, so only the intended HTTPX error path is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/geolibre_server/tests/test_ml.py`:
- Around line 294-312: Update the test double _ErrorHttpx to define and use the
dedicated _FakeHTTPError for HTTPError instead of aliasing Exception. Make
AsyncClient.post raise _FakeHTTPError for the simulated backend failure, so only
the intended HTTPX error path is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b19a0ade-780d-4b1e-b21a-1dbf06cd25bf
📒 Files selected for processing (2)
backend/geolibre_server/geolibre_server/app/ml.pybackend/geolibre_server/tests/test_ml.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/ml/segment/*proxy requests (default 4) with HTTP 429 when the slot is full, matching conversion/Whitebox job caps.finally.Test plan
uv run --project backend/geolibre_server --extra test pytest backend/geolibre_server/tests/test_ml.py -qSummary by CodeRabbit