fix(vector-stores): paginate managed resource list by unified_resource_id cursor#34596
fix(vector-stores): paginate managed resource list by unified_resource_id cursor#34596mateo-berri wants to merge 1 commit into
Conversation
…e_id cursor `list_user_resources`, which backs the managed vector store listing, carried the same broken cursor as the managed batch listing: `after` was applied as `where id > after` against the table's random-uuid primary key, while the value a client holds is the `unified_resource_id` returned as each item's `id`. That compares a base64 cursor to unrelated uuids, so a page could re-select the rows it just returned or skip rows that should have come next. Ordering by non-unique `created_at` alone left the order partial as well, so rows sharing a timestamp could shift between requests. The listing now uses a Prisma cursor on the unique `unified_resource_id` with `unified_resource_id` as a secondary sort key, rejects an `after` that does not resolve to a resource the caller can list with a 400 rather than an empty page, and derives `has_more` from an extra fetched row instead of from whether the page came back full. `build_list_page` read `first_id` / `last_id` through attribute access only, which raised AttributeError on every non-empty page of this listing because it builds plain dicts. It now reads the id from either shape.
Greptile SummaryThis PR corrects managed vector-store pagination and list response construction
Confidence Score: 4/5The PR is safe to merge with two non-blocking issues around the SDK dependency boundary and pagination metadata after parse failures The cursor and isolation fixes are well covered, but SDK-only calls now require FastAPI and skipped malformed rows can produce a next-page signal without a usable cursor Files Needing Attention: litellm/llms/base_llm/managed_resources/base_managed_resource.py
|
| Filename | Overview |
|---|---|
| litellm/llms/base_llm/managed_resources/base_managed_resource.py | Corrects cursor pagination and lookahead handling, but introduces a proxy-only dependency and inconsistent pagination metadata when rows fail to parse |
| litellm/llms/base_llm/managed_resources/isolation.py | Safely extracts pagination IDs from mapping-backed and attribute-backed list items |
| tests/test_litellm/llms/base_llm/test_base_managed_resource.py | Adds focused regression tests covering cursor shape, traversal, lookahead behavior, invalid cursors, and ownership scope |
| tests/test_litellm/llms/base_llm/test_managed_resource_isolation.py | Adds coverage for mapping-backed, object-backed, and empty list-page construction |
Reviews (1): Last reviewed commit: "fix(vector-stores): paginate managed res..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Returns: | ||
| Dictionary with list of resources and pagination info | ||
| """ | ||
| from fastapi import HTTPException |
There was a problem hiding this comment.
list_user_resources now imports FastAPI on every call, so SDK-only installations without the proxy extra raise ModuleNotFoundError before executing the listing. Keep proxy exception mapping outside this SDK-layer module or use an exception available in the core dependency set
Rule Used: What: Do not allow fastapi imports on files outsid... (source)
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!
|
|
||
| has_more = len(resources) > page_size | ||
|
|
||
| resource_objects: List[Any] = [] |
There was a problem hiding this comment.
has_more is calculated before resource parsing, while parse failures are silently removed from the returned page. When every sliced row is unparseable but a lookahead row exists, the response contains empty data, has_more=true, and no last_id, leaving clients without a cursor for the advertised next page. Derive pagination metadata from rows that can actually be emitted or preserve a cursor when rows are skipped
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
TLDR
Problem this solves:
aftercompared a base64 id against uuid primary keysbuild_list_pageraised AttributeError on dict itemsHow it solves it:
unified_resource_id, the id clients holdunified_resource_idadded as a total-order tie-breakerafterreturns 400, not an empty pageRelevant issues
Same bug class as #34192, in the sibling paginator
Linear ticket
Pre-Submission checklist
Screenshots / Proof of Fix
A note on how this is proven:
list_user_resourceshas no HTTP surface today.GET /v1/vector_storesroutes throughProxyBaseLLMRequestProcessingtoavector_store_liston the router, so it never consults the managed vector store hook the wayGET /v1/batchesconsults the managed files hook. There is therefore no curl that reaches this code. The runs below drivealist_vector_stores, the exact method that endpoint would call once it is wired up, against the real local Postgres through Prisma, with four managed vector stores seeded intoLiteLLM_ManagedVectorStoreTable(vs_000oldest ->vs_003newest, random uuid primary keys as@default(uuid())produces) and a caller scoped touser_id=lit-vs-proof-user. A client walks withlimit=2, feeding each response'slast_idback asafter. Labels are decoded from the base64unified_resource_idreturned as each item'sid.Before, at
7b019cf152(the base commit):Every non-empty page raised, because this listing builds plain dicts and
build_list_pagesourcedfirst_id/last_idthrough attribute access.Before, at
7b019cf152with only thebuild_list_pagepart of this PR applied, so the cursor behaviour is observable:After, at
f913d42abb:The second page is also proof that Postgres accepts the new query shape: Prisma really does take a
cursoronunified_resource_idtogether withskip=1and a two-keyorder, and returns the four rows in reverse-chronological order across exactly two round trips.Type
🐛 Bug Fix
Changes
list_user_resourcesonBaseManagedResourcebacks the managed vector store listing (alist_vector_stores). It carried the same broken cursor that #34192 fixes for managed batches:The
aftervalue a client sends back is a resource'sunified_resource_id, because that is what the listing returns as each item'sidand aslast_id. Comparing that base64 string against unrelated random uuids either re-selects the rows the previous page returned or excludes rows that should have come next, and ordering bycreated_at descwhile filtering withgtpoints the two in opposite directions. The live run above shows both symptoms at once: alast_idthat never advances and two of four resources that are never returned.The fix is the same as #34192: a Prisma cursor on the unique
unified_resource_idwithskip=1, plusunified_resource_idas a secondary sort key so the ordering is total. A cursor is only well-defined when the ordering fully determines row order;created_atalone is not unique, so rows sharing a timestamp can come back in an order that shifts between requests and slip through the seam between pages.Two related truncations are fixed with it. An
afterthat does not resolve to a resource the caller can list returned an empty page, which a client cannot distinguish from the end of the list; it now returns 400 naming the cursor. The lookup runs against the caller's own rows, so the cursor cannot be anchored to a resource the caller cannot see: a Prisma cursor resolves by unique column regardless of thewherefilter. Andhas_morecame from whether the page was full rather than whether another row exists, so an exact multiple oflimitcost an extra empty request and a page shortened by an unparseable row looked like the end of the list. The listing now fetcheslimit + 1rows and reportshas_morefrom the extra row.Separately,
build_list_pagereadfirst_idandlast_idthrough attribute access only. The file and batch listings pass pydantic objects so they were fine, but this listing passes plain dicts, so every non-empty page raised AttributeError. It now reads the id from either shape.Tests, all of which fail on the base commit and pass here:
test_list_resources_pagination_uses_unified_resource_id_cursorpins the cursor, theskip, and the two-key order, and asserts noidfilter leaks into the where clause;test_list_resources_pagination_walks_all_pages_without_loops_or_gapswalks every page the way a client does and asserts each resource comes back exactly once, newest first;test_list_resources_has_more_false_on_exactly_full_final_pagecovers the extra empty request;test_list_resources_rejects_unknown_after_cursorandtest_list_resources_cursor_lookup_is_scoped_to_the_callercover the 400 and the owner-scoped lookup;test_list_page_reads_ids_from_dict_itemscovers the AttributeError, alongside object-item and empty-page cases.Final Attestation