Deleting an external knowledge base now clears its connection only when an admin removes the last knowledge base referencing it, matching the connection delete route.
Seven route handlers declare a request-scoped database session as a FastAPI dependency and then never touch it. Three of them are `GET /api/v1/users/user/settings`, `/user/status` and `/user/info`, which the frontend hits on every page load, and all three carry a comment saying the user object is already available, so the parameter is leftover from the refactor that removed the refetch. The other four are admin-only external-knowledge connection endpoints that read their data from the config store.
Measured on a route with and without the dependency, 20k requests, best of 5:
| | µs per request |
| --- | --- |
| no dependency | 16.18 |
| unused session dependency | 62.85 |
The dependency costs about three times as much as everything else the request does put together. It is worth being precise about why, because the obvious guess is wrong: this is not database I/O and not connection pool pressure. SQLAlchemy connects lazily, so a session that is never used checks out zero connections, verified by watching the pool's counter stay at zero across the request. The cost is FastAPI resolving an extra async-generator dependency onto the request's exit stack, plus constructing and closing the session object.
Deleting the seven parameters is the whole change. An AST scan over the backend finds exactly these seven handlers before and none after.
With database session sharing enabled, which the docs recommend for PostgreSQL and for multi-replica deployments, the knowledge pending-files and file process-status endpoints each pinned one pooled connection for as long as their SSE stream stayed open, up to one and two hours respectively. A file wedged in processing keeps a stream open for the full duration, so a handful of users sitting on that page can consume every connection in the pool, and the held transactions sit idle and block autovacuum on those tables.
Both handlers took a request-scoped session for their access checks, and FastAPI only releases a yield dependency once the response body has finished streaming, so the session outlived the handler by the whole life of the stream. Neither generator ever used it. They no longer take that dependency, and the queries they run already open their own short-lived sessions when none is passed. This is the approach the chat completion endpoints already use for the same long-response problem.
Measured against a pool with capacity 11: before, at most 11 concurrent streams could ever be open and every further attempt failed, deterministically across repeat runs. After, 25 of 25 opened. Non-stream latency is unchanged, within run-to-run noise, and behaviour is identical whether session sharing is on or off.
Raising GLOBAL_LOG_LEVEL to WARNING buys quieter output but not less work: 241 INFO call sites interpolate their payload into an f-string before the logging call gets to drop it. The heaviest is get_doc, which logs every chunk id and metadata dict in a collection, so on the full-context retrieval path that is the entire knowledge base, once per chat request.
That one line at WARNING, CPython 3.12:
| knowledge base | payload | before | after |
| -------------- | ------- | -------- | ------- |
| top-k of 3 | 1.2 kB | 3.8 us | 0.07 us |
| 500 chunks | 201 kB | 583.6 us | 0.08 us |
| 5000 chunks | 2.0 MB | 5.8 ms | 0.15 us |
The lazy form log.info('query_doc:result %s %s', result.ids, result.metadatas) hands the payload to record.getMessage(), which the InterceptHandler only reaches once a record has passed the level check. Output at INFO is byte-identical. Two sites that already built their message eagerly, one str concat and one % operator, move to the same lazy form.
GLOBAL_LOG_LEVEL defaults to INFO, so every log.debug(...) in the backend is discarded, but the message is built first: 187 call sites interpolate their payload into an f-string before the logging call runs, so the work happens on every request and the result is thrown away. The worst one sits in process_chat_payload and stringifies the whole request body, full conversation history included, once per chat completion.
That one line with DEBUG disabled, CPython 3.12:
| conversation | payload | before | after |
| ------------ | ------- | -------- | ------- |
| 4 messages | 1.2 kB | 3.4 us | 0.07 us |
| 20 messages | 17 kB | 24.8 us | 0.07 us |
| 60 messages | 123 kB | 216.6 us | 0.07 us |
The lazy form log.debug('form_data: %s', form_data) hands the payload to record.getMessage(), which the InterceptHandler only reaches once a record has passed the level check. With DEBUG enabled the emitted lines are byte-identical, f'{x=}' sites included: those map to %r. MistralLoader._debug_log callers get the same treatment, since that wrapper already forwards *args.
POST /knowledge/{id}/sync/cleanup verified write access to the knowledge base in the URL but then acted on the caller-supplied file_ids and dir_ids without checking they belong to that knowledge base. A user with write access to any knowledge base could pass another knowledge base's directory id to delete its directory subtree and knowledge_file associations, or another file's id to drop its file-{file_id} vector collection. Fetch each directory and skip any whose knowledge_id does not match the URL id (matching the explicit directory-delete endpoint), and gate the per-file vector cleanup on Knowledges.has_file(id, file_id) so a foreign file id cannot trigger collection deletion. Legitimate same-knowledge-base cleanup is unchanged.
Co-authored-by: whyiug <whyiug@users.noreply.github.com>
After set_access_grants, the handler was reloading the same knowledge
record via get_knowledge_by_id, which triggers an extra SELECT plus a
nested fetch of access grants. set_access_grants already returns the
newly-written grants and the local knowledge object is otherwise
unchanged, so update it in place and reuse it for the response.
https://claude.ai/code/session_01S18Lgqbih7Ry2JZUUv8TxF
Co-authored-by: Claude <noreply@anthropic.com>
* fix(retrieval): offload sync VECTOR_DB_CLIENT calls in async paths via AsyncVectorDBClient
The vector DB backends (Chroma, pgvector, Qdrant, Milvus, Pinecone,
Weaviate, …) are uniformly synchronous and their methods perform
blocking network or disk I/O. Multiple async route handlers and helpers
were calling them directly on the event loop — file processing,
memories, knowledge bases, hybrid search bookkeeping — so a single
upsert/delete/search would freeze every other in-flight request for the
duration of the call.
Introduce `AsyncVectorDBClient`, a thin async facade that wraps the
existing sync client and dispatches each method through
`asyncio.to_thread`. It mirrors `VectorDBBase` exactly and forwards
*args/**kwargs so backend-specific extra parameters keep working.
Update every async-context call site (routers/retrieval, routers/files,
routers/memories, routers/knowledge, retrieval/utils,
tools/builtin) to await `ASYNC_VECTOR_DB_CLIENT` instead of calling the
sync client directly. Two helpers that were sync-only also acquire
async siblings or are awaited via `asyncio.to_thread` at their async
call site (`remove_knowledge_base_metadata_embedding`,
`get_all_items_from_collections`, `query_doc`).
The original sync `VECTOR_DB_CLIENT` is unchanged, so callers that
already run inside `run_in_threadpool` (e.g. `save_docs_to_vector_db`
and the sync `query_doc`/`get_doc` helpers) are unaffected.
https://claude.ai/code/session_01JSr4NZSskEUQvoJnavVXh8
* fix(retrieval): restore explicit AsyncVectorDBClient signatures matching VectorDBBase
Per PR review: the original *args/**kwargs forwarding lost type
safety and IDE/static-analysis support. Restore explicit signatures
that mirror VectorDBBase exactly, so:
* Bad kwargs fail at the facade boundary instead of inside the
worker thread (where the resulting TypeError tends to be
swallowed by surrounding `try/except`).
* IDE autocomplete and static analysis work as expected.
* The stated intent ("mirror VectorDBBase exactly") now holds at
the API contract level, not just behaviourally.
While doing this, surface a pre-existing bug in
`delete_entries_from_collection` that the stricter typing flagged:
the call passed `metadata={'hash': hash}` which is not a parameter
on `VectorDBBase.delete` nor any backend. The TypeError raised
inside the sync delete was silently swallowed by `except Exception`
so the endpoint always reported `{'status': False}` for every
request instead of actually deleting matching vectors. Replace with
`filter=...` to do what the endpoint name promises.
The thorough review's other note (no concurrency/backpressure on
the shared default threadpool) is intentionally not addressed here:
asyncio.to_thread on the shared executor is the right primitive for
this use case; per-domain bounded executors would add lifecycle
complexity disproportionate to the problem and the loop is no
longer blocked, which was the actual bug.
https://claude.ai/code/session_01JSr4NZSskEUQvoJnavVXh8
* fix(retrieval): parallelize hybrid-search collection prefetch; document async facade contracts
Address PR review findings:
1. Hybrid-search prefetch was sequential
`query_collection_with_hybrid_search` previously awaited
`ASYNC_VECTOR_DB_CLIENT.get(name)` once per collection in a for
loop. Each call already off-loaded to a worker thread, but
awaiting them serially meant total prefetch latency scaled
linearly with the number of collections. Run them concurrently
with `asyncio.gather` so multi-collection queries actually
benefit from the threadpool. Per-collection exception handling
is preserved by wrapping each fetch in a small helper that
logs and returns `(name, None)` on failure, so a single bad
collection cannot poison the whole gather.
2. Document the thread-safety expectation explicitly
The facade now formally states what was always implicit: the
sync `VECTOR_DB_CLIENT` is shared across worker threads, so the
underlying backend driver must be thread-safe. This is not a
new exposure — `save_docs_to_vector_db` already called the sync
client from `run_in_threadpool`. Adding a global lock here
would defeat the responsiveness the facade exists to provide;
backends that cannot tolerate concurrent access should grow
their own internal serialization.
3. Document the API-surface choice and `.sync` escape hatch
The strict `VectorDBBase` mirror was a deliberate choice (the
previous `*args/**kwargs` revision let a `metadata=` typo
silently break an endpoint). Document it, and call out the
`.sync` escape hatch with an example for callers that genuinely
need a backend-specific parameter not on `VectorDBBase`.
https://claude.ai/code/session_01JSr4NZSskEUQvoJnavVXh8
* fix(retrieval): guard /delete against null file.hash and let HTTPException reach the client
Address PR review finding on the `metadata=` → `filter=` change in
`delete_entries_from_collection`.
The new `filter={'hash': hash}` query was correct for files that
have a hash, but did not handle `file.hash is None` (unprocessed,
failed, or legacy records). The match semantics of a null filter
value are backend-dependent — some ignore the key entirely, some
treat it as "metadata field absent" and match every such row — so
issuing the query risked deleting unrelated entries.
* Reject `hash is None` up front with a 400 explaining the file
has no hash to target.
* Narrow the surrounding `except Exception` so it no longer
swallows `HTTPException`. Without this fix the new 400 (and the
pre-existing 404 for missing files) would be silently re-shaped
into `{'status': False}` and the caller could not distinguish a
bad-request input from a backend error.
https://claude.ai/code/session_01JSr4NZSskEUQvoJnavVXh8
---------
Co-authored-by: Claude <noreply@anthropic.com>
Timothy Jaeryang Baek
Classic298andGitHub
G30andGitHub
