mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-10 22:41:41 +00:00
refactor(proxy/batches): make managed-file resolution purely additive, fall back like base (#34584)
Restore the original PR's behavior on every path that did not already resolve: no database, a lookup error, a missing managed-file row, or a row without a storage_url all fall back to dispatching the original id, which the managed-files deployment hook still maps. This drops the 404 and 503 fail-closed responses I had added, which were the only behaviors that diverged from litellm_internal_staging. The change is now strictly additive: when a managed-file row with a storage_url exists, the unified batch branch substitutes it so providers like Vertex receive a real gs:// path instead of the opaque token; every other path behaves exactly as before. Verified live that non-managed, managed-owner, multi-model load-balanced, and missing-row requests are byte-identical to base
This commit is contained in:
parent
579f41d57f
commit
fc5ab31fba
2 changed files with 21 additions and 33 deletions
|
|
@ -49,12 +49,11 @@ async def _resolve_managed_input_file_storage_url(input_file_id: str) -> "str |
|
|||
|
||||
Provider batch handlers (e.g. Vertex AI, which parses a `publishers/`
|
||||
segment out of the file URI) need a real storage location; the opaque
|
||||
unified token crashes them. Returns None only when there is no database or
|
||||
the row has no storage_url yet, so callers fall back to the original id
|
||||
(which the managed-files deployment hook can still map). Fails closed
|
||||
rather than dispatch a token that cannot be resolved: 404 when no
|
||||
managed-file row exists, 503 when the lookup itself errors so the caller
|
||||
can retry.
|
||||
unified token crashes them. Returns None whenever a storage_url cannot be
|
||||
produced (no database, lookup error, no managed-file row, or a row without
|
||||
a storage_url yet) so callers fall back to dispatching the original id,
|
||||
which the managed-files deployment hook still maps. This adds resolution
|
||||
without changing behavior on any path that did not resolve before.
|
||||
"""
|
||||
from litellm.proxy.proxy_server import prisma_client
|
||||
|
||||
|
|
@ -64,15 +63,9 @@ async def _resolve_managed_input_file_storage_url(input_file_id: str) -> "str |
|
|||
db_file = await ManagedFileRepository(prisma_client).table.find_first(where={"unified_file_id": input_file_id})
|
||||
except Exception as e:
|
||||
verbose_proxy_logger.warning("create_batch: managed file lookup failed for %s: %s", input_file_id, e)
|
||||
raise HTTPException(
|
||||
status_code=503,
|
||||
detail={"error": "Could not resolve managed file; please retry"},
|
||||
)
|
||||
return None
|
||||
if db_file is None:
|
||||
raise HTTPException(
|
||||
status_code=404,
|
||||
detail={"error": f"Managed file not found: {input_file_id}"},
|
||||
)
|
||||
return None
|
||||
return db_file.storage_url or None
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -561,10 +561,11 @@ async def test_create__unified_file_id_resolves_real_storage_url(harness):
|
|||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create__unified_file_id_db_error_fails_closed_503(harness):
|
||||
"""A lookup error leaves the token unresolved, so it fails closed with a
|
||||
retryable 503 rather than dispatching the opaque id into the provider crash
|
||||
it cannot parse. Nothing is dispatched."""
|
||||
async def test_create__unified_file_id_db_error_falls_back_to_raw_id(harness):
|
||||
"""Resolution is additive and best-effort: a lookup error leaves the id
|
||||
unresolved and dispatch falls back to the original id, exactly as before
|
||||
this change (the managed-files deployment hook still maps it). No new
|
||||
failure mode is introduced."""
|
||||
set_body(
|
||||
harness,
|
||||
{
|
||||
|
|
@ -585,12 +586,9 @@ async def test_create__unified_file_id_db_error_fails_closed_503(harness):
|
|||
patch.object(proxy_server, "prisma_client", MagicMock()),
|
||||
patch.object(endpoints, "ManagedFileRepository", fake_repo_cls),
|
||||
):
|
||||
with pytest.raises(ProxyException) as exc:
|
||||
await call_create(harness)
|
||||
await call_create(harness)
|
||||
|
||||
assert exc.value.code == "503"
|
||||
harness.router_acreate.assert_not_called()
|
||||
harness.litellm_acreate.assert_not_called()
|
||||
assert harness.router_kwargs()["input_file_id"] == "litellm_proxy_unified_id"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
@ -626,11 +624,11 @@ async def test_create__multi_model_unified_file_with_loadbalancing_keeps_router_
|
|||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create__unified_file_id_missing_row_fails_closed_404(harness):
|
||||
"""With a database present, a managed unified id that has no row cannot be
|
||||
resolved to a real storage location, so it fails closed with a 404 rather
|
||||
than dispatching the opaque token, which would hit the Vertex
|
||||
publishers-segment IndexError this PR exists to prevent."""
|
||||
async def test_create__unified_file_id_missing_row_falls_back_to_raw_id(harness):
|
||||
"""Resolution is additive: when no managed-file row exists there is nothing
|
||||
to substitute, so dispatch falls back to the original id exactly as before
|
||||
this change (the managed-files deployment hook still maps it). No new
|
||||
failure mode is introduced for this case."""
|
||||
set_body(
|
||||
harness,
|
||||
{
|
||||
|
|
@ -651,12 +649,9 @@ async def test_create__unified_file_id_missing_row_fails_closed_404(harness):
|
|||
patch.object(proxy_server, "prisma_client", MagicMock()),
|
||||
patch.object(endpoints, "ManagedFileRepository", fake_repo_cls),
|
||||
):
|
||||
with pytest.raises(ProxyException) as exc:
|
||||
await call_create(harness)
|
||||
await call_create(harness)
|
||||
|
||||
assert exc.value.code == "404"
|
||||
harness.router_acreate.assert_not_called()
|
||||
harness.litellm_acreate.assert_not_called()
|
||||
assert harness.router_kwargs()["input_file_id"] == "litellm_proxy_unified_id"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue