Critical: the seq counter was emitter-local and reset to 0 whenever a
new emitter was created for the same message_id (crash-retry,
continuation, etc.). The client's dedupe guard drops any frame with
seq <= lastSeq, so a second emitter starting at 1 after the first hit
120 meant every live frame from the retry was silently dropped AND
the replay filter (seq > after_seq) excluded them too. Exactly the
failure scenarios this feature is for.
Fix: seed the counter from the log's existing max seq on emitter
construction via a new _stream_log_max_seq (XREVRANGE + COUNT 1 —
cheap tail read). Retries now continue the sequence instead of
colliding with it.
Warning: _stream_log_truncate became dead code after the switch to
TTL-shortening at done. Removed — if destructive reset is ever
needed again it can come back with a specific call site.
Suggestion: _stream_log_append / _stream_log_read / done-TTL-shorten
upgraded from log.debug to log.warning. Silent Redis failures in
debug-level were effectively invisible in prod; warnings surface the
outage at the right severity without being noisy when Redis is
healthy (these paths only log on exceptions).
Two review findings:
1. clearAllResumeFences was firing unawaited async flushes from
lifecycle transitions (disconnect, initNewChat, loadChat). That
undermined the ordering guarantees the fence exists to provide,
because the flushed events could land on a component that had
already moved to a different chat or connection state.
Split into two explicit verbs:
- dropResumeFence / dropAllResumeFences — synchronous, no flush.
Used in lifecycle transitions where the buffered events refer
to state that is about to become stale.
- clearResumeFence — async, flushes before dropping. Used by the
replay-ack handler (happy path) and the fence timeout (safety).
2. Unconditional _stream_log_truncate at emitter creation could wipe
an actively-streaming log if two emitters happened to overlap for
the same (user_id, message_id). Removed the truncate entirely and
switched XADD from explicit `0-{seq}` IDs to Redis-generated IDs,
so overlapping emitters cannot collide on stream IDs regardless.
seq now lives as a field on each entry and _stream_log_read filters
by it in Python (full scan bounded by MAXLEN=2000, a few ms worst
case, cost irrelevant for a user-driven event).
Suggestion on replay payload size deferred: in practice resumes are
tiny (handful of frames during a brief disconnect) and MAXLEN already
caps the worst case at ~1MB. Chunking would add protocol complexity
for a ceiling that isn't being hit. Easy to add later if telemetry
shows real reconnect-storm spikes.
Three correctness fixes from review:
Critical: in non-Redis deployments the old resume_stream handler
returned early without clearing the client fence, so any call to
requestResumeForMessage would buffer live frames forever and freeze
the UI. Server now always emits exactly one reply (with an empty
envelope list when Redis is disabled or the log is empty), so the
client's fence always clears regardless of backend configuration.
Warning: a lost ack — whether from a transient disconnect between
emit and reply, a server-side exception, a not-yet-registered client
handler, or anything else — would deadlock a message's UI updates
indefinitely. Added:
- 10s per-message fence timeout that clears the fence and flushes
buffered frames if no reply arrives
- disconnect listener that drops all fences on socket loss so the
reconnect-triggered fresh resume doesn't inherit a stale timer
- try/finally around replay application so a malformed envelope
can't skip the fence clear
Suggestion: replay is now a single batch emit instead of N sequential
emits. The server bundles all envelopes (possibly zero) into one
`resume-stream:replay` message; the client applies them in order and
immediately flushes the live-frame fence. Collapses "replay stream
+ completion ack" into a single round-trip, removes the per-envelope
emit loop latency, and simplifies the state machine.
Critical: replay and live frames can interleave on the client.
Previously the seq dedupe guard would drop replay frames whose seq was
already covered by a racing live frame that arrived first, permanently
losing the content between last_seq and the racing live seq.
Fix with a replay fence scoped per message:
- Server tags replay frames with `_replayed: true` and emits a
`resume-stream:complete` event after the last one.
- Client sets a fence (empty queue) BEFORE emitting resume-stream,
so any live frame arriving while the server reads XRANGE is
buffered — not applied, not advancing lastSeq.
- Replay frames (`_replayed: true`) bypass the fence, apply in order,
advance lastSeq.
- On `resume-stream:complete`, client flushes the buffered live
frames in seq order. The dedupe guard naturally drops any that were
already covered by replay.
Warning: removed the `taskIds && taskIds.length > 0` gate from the
loadChat resume trigger. getTaskIdsByChatId is not an authoritative
source of "should we resume" — it can fail, race, or return stale
data, which would leave in-progress assistant messages unrecovered.
requestResumeForAllInProgress already filters to assistants with
done !== true, and the server replay is a no-op when no log exists,
so the gate was only adding fragility.
Comment cleanup: cut the essay-length explanations on the resume-stream
feature down to single-line navigation aids. Logic is unchanged by the
tone-down itself.
Finding 1 (resume depended on DB presence of in-flight message):
scoped the Redis stream key by user_id — `stream:{user_id}:{message_id}`
instead of `stream:{message_id}`. An authenticated session can now only
ever read its own user's logs, so the separate chat-ownership +
message-in-chat DB lookups are no longer needed for auth. This also
fixes the case the review flagged: if the assistant stub has not yet
been persisted to the DB at refresh time (the exact scenario
ENABLE_REALTIME_CHAT_SAVE=False exposes), the prior message-in-chat
check would reject resume; now it doesn't apply. Frontend drops the
now-unused chat_id from the resume payload.
Finding 2 (resumeSeqByMessageId accumulated on non-done terminal paths):
prune the map in every terminal transition, not just the happy-path
chatCompletionEventHandler done branch:
- chat:tasks:cancel (both same-message and sibling arms)
- the HTTP error-finalization path (responseMessage.done + error)
- the generic error handler that marks all siblings done
Long-lived sessions with cancelled/errored generations no longer leak
stale seq entries.
The background asyncio task scheduled in the previous fix had a race:
if a second emitter for the same message_id started within the 30s
grace window, the pending DELETE would fire mid-stream and wipe the
new run's log.
Switch to shortening the key's TTL instead. EXPIRE is race-free —
a second emitter's eager truncate at creation resets the key and its
subsequent EXPIRE calls in _stream_log_append extend the TTL normally.
Same 30s post-done retention window, same clean-up behavior, no
background task to reason about.
Fixes a silent-correctness gap flagged in review: explicit stream IDs
of the form `0-{seq}` combined with a per-emitter seq counter that
resets to 0 mean a second emitter for the same message_id (continuation,
regeneration-into-same-id, or a retried producer after a crashed
worker) would try to XADD `0-1` against a stream whose top item is
`0-{N>1}`. Redis rejects the append, our try/except swallows it, and
resume logging silently degrades exactly in the flows where resume
matters most.
Fix: when get_event_emitter is constructed, await a _stream_log_truncate
for the message_id before any XADD. This guarantees our first XADD
(`0-1`) is accepted and that the log reflects only the current run,
not a mix of a crashed prior attempt and the retry.
A background _delayed_truncate task from a previously-completed run is
harmless here — it fires 30s after done:True on the OLD emitter, by
which time either (a) no new emitter has started, in which case the
delete is a legitimate cleanup, or (b) this new emitter has already
truncated + started appending, in which case the delayed delete racing
with the new run could wipe live data. To rule that out, the eager
truncate at emitter start supersedes any pending delayed truncate for
the same key; the next XADD then resets the stream, and when the new
run's delayed truncate eventually fires, it just repeats the cleanup.
Three review findings, all valid:
1. Race: log-then-emit instead of emit-then-log
The previous ordering emitted the live WS frame first and appended
to Redis afterward. A client disconnecting in that window and
reconnecting before the append completed would issue resume-stream,
see nothing newer, and never ask again — permanently losing that
frame (most painfully the terminal done:True frame). Inverted the
order: the log is now the source of truth for what was "sent", and
the live emit follows. A reconnect that races the append now may
see a replayed frame before the live frame arrives in the new
session, but the seq idempotency guard in chatEventHandler drops
the duplicate harmlessly. Duplicates are safe; losses are not.
2. Resume reads no longer full-scan
XADD now uses an explicit stream ID of `0-{seq}` (the per-emitter
seq counter guarantees strict monotonicity), so _stream_log_read
can start XRANGE at `0-{after_seq+1}` and let Redis skip entries
already delivered. Near-tail resumes (the common case) go from
O(MAXLEN) to O(missed frames). Dropped the now-redundant seq field
from the stream entries and the Python-side seq filter — the
envelope still carries seq internally for client-side idempotency.
3. Client map is now pruned
resumeSeqByMessageId was growing unboundedly across the session.
Now:
- cleared on loadChat (chat switch)
- cleared on initNewChat (new chat)
- individual entries deleted in chatCompletionEventHandler's
done:true branch (most messages evict quickly this way)
Long-lived sessions no longer accumulate dead keys for every
completed message they've ever seen.
Two review findings:
1. Backend: the `done:True` detection in get_event_emitter called
`.get('done')` on whatever `event_data['data']` happened to be. For
most event types that's a dict, but some custom/pipeline events can
legitimately emit non-dict inner payloads (list/string/None), which
would raise AttributeError and break emission for that event. Now
narrowed properly:
inner = event_data.get('data') if isinstance(event_data, dict) else None
if isinstance(inner, dict) and inner.get('done') is True:
...
2. Frontend: previously stored `message.lastSeq = incomingSeq` directly
on the message object, which is part of `history` and gets serialized
by saveChatHandler. That leaked transport-level resume metadata into
persisted chat state. Moved bookkeeping into a component-local
`resumeSeqByMessageId: Map<string, number>` so it stays in memory
only and never touches the persisted schema. All reads/writes of
lastSeq now go through the map.
Addresses review finding: multi-model (arena) chats have multiple
sibling assistant responses streaming concurrently. The previous code
only called resume on `history.currentId`, so any non-current sibling
would silently lose frames that landed during a disconnect window.
Replace `requestResumeForCurrentIfInProgress` with
`requestResumeForAllInProgress`, which iterates `history.messages` and
triggers a resume request for every assistant entry with `done !==
true`. Applied to both the loadChat (refresh) trigger and the
socket-reconnect trigger, and the onDestroy cleanup updated to match.
Also fix a comment that referenced a `resume-stream:ack` event that
was removed in the previous review round but still lingered in docs.
Deferred from this review: the replay read path (`_stream_log_read`)
still does `XRANGE - +` and filters by seq in Python. With MAXLEN
~2000 entries bounding the scan and resume being a rare, user-driven
event (bounded rate), this is O(a few ms) worst case and not worth
the added protocol complexity of tracking Redis stream IDs client-side.
Easy to revisit if profiling shows it matters.
- Critical (auth): the resume-stream handler verified chat ownership but
not that the requested message_id actually lives in that chat. Because
the Redis log is keyed by message_id alone, an attacker who learned a
victim's message_id could pass one of their own chat_ids to satisfy
the ownership check and replay the victim's stream. Added an explicit
message-to-chat binding check via Chats.get_message_by_id_and_message_id
after the ownership check.
- Safety: reject non-dict payloads (`if not isinstance(data, dict)`) so
stray client input — string/list/null — doesn't raise AttributeError
on `data.get(...)`.
- Perf: the per-token log append was doing two Redis round-trips (XADD +
EXPIRE) on the streaming hot path. Collapse them into a single pipeline
execute (one RTT), and refresh the TTL only every 64 appends instead
of every append. With a 1h TTL that still leaves comfortable headroom
for even pathologically long responses without EXPIRE ever risking
mid-stream expiry.
- Protocol cleanup: removed the resume-stream:ack emission. The frontend
doesn't consume it and YAGNI — the seq idempotency guard in
chatEventHandler already delivers the observability (you can see
last_seq advance as replays arrive). Can be added back when a concrete
client-side use case appears.
Adds a bounded Redis stream log for every in-flight assistant message so
clients can reconnect mid-stream (page refresh, network drop, device
switch) and catch up on frames they missed without re-fetching the full
chat from the database.
Problem this solves
-------------------
With ENABLE_REALTIME_CHAT_SAVE=False (default), the backend does not
write the assistant message to the DB until the stream finishes. If the
client refreshes the page mid-stream, chat load from the DB returns
nothing for the in-progress message and the response appears to vanish
until the stream eventually completes. Users staring at an empty chat
while the backend quietly keeps emitting tokens into the void.
Design
------
* Every outbound WS envelope gets stamped with a monotonic per-message
`seq` inside `get_event_emitter` and appended to a bounded Redis
stream keyed `{REDIS_KEY_PREFIX}:stream:{message_id}`.
MAXLEN ~ 2000 entries, TTL 1h as a safety net.
* Clients track `message.lastSeq` in Chat.svelte as events arrive. On
chat load (mid-stream refresh) and on socket reconnect they emit
`resume-stream {chat_id, message_id, last_seq}`.
* The server authenticates the session, verifies the user owns the
chat, XRANGEs the log, filters by `seq > last_seq`, and emits the
missed envelopes to THAT session only (via `to=sid`) so live listeners
in the user room keep receiving their normal live stream unchanged.
* The existing chat event handler drops any envelope with
`seq <= message.lastSeq`, making replay idempotent against live
frames that race the replay after a reconnect.
* When an event with `done: True` fires, a background task truncates
the log after a 30s grace window so late reconnects still catch the
finalization; anything beyond that resumes from the now-up-to-date DB.
Orthogonality
-------------
Zero touches to middleware.py or the streaming hot path. The log stores
whatever gets emitted; any future change to emit shape
(chat:message:delta, per-block ops, JSON Patch, ...) is logged and
replayed verbatim with no coupling.
Graceful degradation
--------------------
No-op when Redis is not configured (WEBSOCKET_MANAGER != 'redis'). In
that deployment mode, refresh during streaming retains the current
behavior of waiting for the stream to complete.
Auth model
----------
The log is keyed by message_id only. The resume handler must do a chat
ownership check (Chats.get_chat_by_id_and_user_id) before replaying, so
a malicious client cannot read another user's stream by guessing a
message_id.
Loader.load() dispatches to the underlying langchain document loaders
(PyMuPDF, Unstructured, python-docx, Tika, …) which are all
synchronous and CPU/IO-bound. process_file() awaited it directly on
the event loop, so parsing a non-trivial PDF/DOCX would freeze the
entire FastAPI app for the duration of the parse — which is what users
experience as "the server hangs whenever I upload a file."
Add an `aload()` async wrapper on Loader that runs the sync load on a
worker thread via asyncio.to_thread, and update process_file() to
await it. The sync API is preserved so existing callers that already
run inside run_in_threadpool (e.g. save_docs_to_vector_db) are
unaffected.
https://claude.ai/code/session_01JSr4NZSskEUQvoJnavVXh8
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>
* fix(middleware): replace BaseHTTPMiddleware HTTP middlewares with pure ASGI implementations
Starlette's BaseHTTPMiddleware (and the @app.middleware('http')
decorator that uses it) wraps the downstream app in an anyio task
group whose cancel scope tears down the inner task on every exit —
client disconnect, response complete, or any outer middleware bailing.
That CancelledError gets injected into whatever the inner task was
awaiting, so DB queries, embedding calls, and other long awaits get
killed mid-flight. Under aiosqlite the cleanup path then logs a
multi-page `terminate_force_close() not implemented` traceback at
ERROR for every cancelled DB call.
Open WebUI had four such middlewares stacked
(`commit_session_after_request`, `check_url`, `inspect_websocket`,
`RedirectMiddleware`) so a single cancellation would compound through
all four.
Move the four middlewares to a new `open_webui.utils.asgi_middleware`
module as plain ASGI classes (`__call__(scope, receive, send)`):
* `CommitSessionMiddleware` — was `commit_session_after_request`;
now also rolls back if commit fails
before releasing the connection.
* `AuthTokenMiddleware` — was `check_url`; sets request.state
token + enable_api_keys + stamps
X-Process-Time via a wrapped send.
* `WebsocketUpgradeGuardMiddleware`
— was `inspect_websocket`; rejects
/ws/socket.io HTTP requests that
claim transport=websocket without a
proper Upgrade/Connection header.
* `RedirectMiddleware` — was the BaseHTTPMiddleware subclass;
same /watch + share-target rewrites.
Pure ASGI does not introduce a cancel scope around the downstream app,
so client disconnects propagate via `receive()` (the way ASGI was
designed) instead of being injected as CancelledError. Middleware
ordering is preserved.
https://claude.ai/code/session_01JSr4NZSskEUQvoJnavVXh8
* fix(middleware): CommitSessionMiddleware — rollback on downstream error, never commit failed requests
The first cut put commit() in a finally block, which meant that even
when a downstream handler raised, the middleware would still commit
whatever partial sync writes that handler had made before the
failure. That regressed the previous BaseHTTPMiddleware semantics
where commit only ran on the success path.
Restructure the failure handling:
* Downstream raised → rollback any pending sync work, release the
connection, re-raise so the outer error middleware turns it into
an error response. We never commit a request that did not complete.
* Downstream returned → commit. On commit failure, log loudly,
rollback, and re-raise. ScopedSession.remove() always runs in
finally so the connection cannot leak.
Document the inherent pure-ASGI limitation explicitly: by the time
`await self.app(...)` returns the response messages have already
been emitted, so a commit failure can no longer change what the
client sees on the wire. Buffering the response to gate it on commit
success would break streaming responses (chat completions, SSE) which
are core to Open WebUI; the trade-off is intentional. Routes that
need commit-before-send must manage the sync session explicitly.
Also drop unused `typing` imports flagged by review.
https://claude.ai/code/session_01JSr4NZSskEUQvoJnavVXh8
---------
Co-authored-by: Claude <noreply@anthropic.com>
* fix: drop extra='allow' on FolderForm and FolderUpdateForm
These request models were configured to accept arbitrary extra fields,
which were then merged into the folder row via form_data.model_dump().
In insert_new_folder the server-assigned user_id is placed before the
form spread, so a client-supplied user_id in the request body would
override it and the folder would be persisted against another account.
Strictly typed inputs are the correct shape for these endpoints — the
client has no legitimate reason to send fields beyond the declared
ones, and dropping extra='allow' closes the mass-assignment sink at
the validation layer instead of relying on every callsite to merge
fields in the right order.
* fix: reject unknown fields on FolderForm and FolderUpdateForm
Address review feedback: dropping extra='allow' fell back to Pydantic
v2's default extra='ignore', which only silently drops unknown fields
instead of rejecting them. The intent for these request models is a
strict input contract — fail fast when a client sends anything the
server does not expect — so explicitly set extra='forbid'. This also
makes the hardening visible in the form definition rather than implicit
in the default.
