test(e2e): batch/file API e2e suite - #31369
Conversation
Greptile SummaryThis PR introduces a black-box e2e suite under
Confidence Score: 4/5All changes are confined to tests/e2e/ and touch no production code; the proxy, SDK, and existing tests are unaffected. The memory sampler in memory.py returns -1 when it cannot read the cgroup, causing growth_bytes to resolve to 0 and the ratio assertion to pass unconditionally. The test designed to catch the LIT-3382 OOM buffering regression would give a green result even on a fully broken proxy as long as the cgroup is unavailable. The rest of the suite is well-constructed. tests/e2e/batches/memory.py and tests/e2e/batches/test_batch_upload_memory_e2e.py — the memory sampler needs an early failure when the cgroup read returns -1 so the OOM guard does not silently pass when the sampler is non-functional.
|
| Filename | Overview |
|---|---|
| tests/e2e/batches/memory.py | DockerCgroupSampler silently passes the memory assertion when _read_bytes() consistently returns -1 (cgroup v2 unavailable or docker exec fails), masking the LIT-3382 regression. |
| tests/e2e/batches/test_batch_upload_memory_e2e.py | When the DockerCgroupSampler fails to read cgroup data, growth_bytes=0 and the ratio assert vacuously passes — the OOM regression the test is meant to detect goes unnoticed. |
| tests/e2e/perkeep_openai/app.py | Upload path correctly streams to disk, but file_content uses subprocess.run(capture_output=True) which buffers the entire file in memory — contradicts the module docstring's streaming claim. |
| tests/e2e/batches/batch_client.py | Clean HTTP client wrapping the proxy's file/batch endpoints. parse_unified_file_id correctly decodes the proxy's base64 id and enforces the managed-id contract. |
| tests/e2e/batches/poll_cap.py | ManagedObjectSeeder/ProxyLog helpers look correct; reset() intentionally clears all pollcap- rows for cross-run hygiene. |
| tests/e2e/batches/test_managed_poll_cap_e2e.py | Black-box poll-cap regression test. Seeds rows directly in Postgres and observes proxy logs; uses pytest.skip when poll doesn't run in time rather than failing. |
| tests/e2e/batches/test_batch_lifecycle_e2e.py | Full file lifecycle plus create_batch error-path pin. Clean and straightforward. |
| tests/e2e/batches/test_batch_large_upload_e2e.py | ~1GB upload-to-perkeep test gated behind E2E_LARGE_BATCH_UPLOAD=1. Checks blob-store growth and memory ratio correctly when the sampler works. |
| tests/e2e/docker-compose.yml | New docker-compose adding perkeep and perkeep-openai services. LITELLM_LICENSE sourced from gitignored .env, not hardcoded. |
Reviews (1): Last reviewed commit: "test(e2e): comprehensive batch/file API ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def measure(self, during: Callable[[], T]) -> tuple[T, PeakMemory]: | ||
| baseline = self._read_bytes() | ||
| peak = baseline | ||
| stop = threading.Event() | ||
|
|
||
| def sample() -> None: | ||
| nonlocal peak | ||
| while not stop.is_set(): | ||
| current = self._read_bytes() | ||
| if current > peak: | ||
| peak = current | ||
| time.sleep(self.interval_seconds) | ||
|
|
||
| sampler_thread = threading.Thread(target=sample) | ||
| sampler_thread.start() | ||
| try: | ||
| result = during() | ||
| finally: | ||
| stop.set() | ||
| sampler_thread.join() | ||
| return result, PeakMemory(baseline_bytes=baseline, peak_bytes=peak) |
There was a problem hiding this comment.
When _read_bytes() can't find the anon field (cgroup v2 unavailable, docker exec fails, or the kernel emits a different layout), it returns -1. Both baseline and peak are then -1, so growth_bytes = max(0, -1 - (-1)) = 0 and ratio = 0 / file_size = 0.0 < MAX_GROWTH_RATIO. The test passes as if the proxy used zero memory — the LIT-3382 buffering regression would go completely undetected.
A guard is needed at the top of measure() that raises RuntimeError when baseline < 0, ensuring a cgroup-unavailable environment fails loudly instead of vacuously passing.
| async def file_content(request: Request) -> Response: | ||
| file_id = request.path_params["file_id"] | ||
| if file_id not in _FILES: | ||
| return JSONResponse({"error": {"message": "file not found"}}, status_code=404) | ||
| got = subprocess.run( | ||
| ["pk-get", "-contents", file_id], capture_output=True, timeout=7200 | ||
| ) | ||
| if got.returncode != 0: | ||
| return JSONResponse( | ||
| {"error": {"message": f"perkeep read failed: {got.stderr[:400]!r}"}}, | ||
| status_code=502, | ||
| ) | ||
| return Response(content=got.stdout, media_type="application/octet-stream") |
There was a problem hiding this comment.
The module docstring states "a gigabyte upload can't OOM the adapter" because uploads stream to a spool file. However, file_content uses subprocess.run(capture_output=True), which collects all of pk-get's stdout into memory before sending it. A 1 GB file retrieved through this endpoint would allocate ~1 GB in the adapter process. The current test suite calls file_content only on tiny payloads, so it doesn't trigger in practice — but the comment creates a false expectation of safety for large files. Consider using subprocess.Popen with a streaming StreamingResponse, or at minimum update the docstring to note that downloads are not streaming.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Adds a black-box e2e suite under tests/e2e/batches/ for the batches and files API. Nothing is imported from the litellm codebase; the tests drive the live proxy over HTTP and verify state through the generated prisma client, so they catch real regressions rather than re-asserting internal calls. It covers the gemini managed-files upload, verified by reading the file back ACTIVE from the provider; a managed-object poll-cap guard for the #23472 OOM that seeds more than one page of rows into real Postgres and watches the poll cycle in the proxy logs; and a vertex streaming-upload memory guard for the LIT-3382 OOM (gated, for an environment with real memory headroom). The pure helpers have unit coverage, and the memory sampler raises rather than passing vacuously when the cgroup read is unavailable
ea17dd9 to
ae5e7ab
Compare
mubashir1osmani
changed the title
1 participant
Relevant issues
Regression coverage for the managed-object poll OOM (#23472) and the batch-upload transform OOM (LIT-3382)
Linear ticket
LIT-3382
Pre-Submission checklist
make test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewType
Test
Changes
A black-box e2e suite under
tests/e2e/batches/for the batches and files API. Nothing is imported from the litellm codebase; the tests drive the live proxy over HTTP and verify state through the generated prisma client, so they catch real regressions rather than re-asserting internal calls.Coverage is the gemini managed-files upload, verified by reading the file back ACTIVE from the provider's own API; a managed-object poll-cap guard for the #23472 OOM that seeds more than one page of rows into real Postgres and confirms from the proxy logs that each poll cycle caps at exactly one page (drop the take and the newest rows show up, failing the assert); and a vertex streaming-upload memory guard for the LIT-3382 OOM, gated behind an env flag for a deployment with real memory headroom. The pure helpers have unit coverage, and the memory sampler now raises instead of passing vacuously when the cgroup read is unavailable (a -1 baseline would otherwise report zero growth and hide the regression).
Screenshots / Proof of Fix
gemini managed batch-file upload through the live proxy, then read back ACTIVE from the provider: