Adopt the bot's suggested alternative to a DB-level message-in-chat
binding check: put chat_id directly into the Redis key structure
so message-to-chat binding is enforced by key construction.
Key changes:
stream:{user_id}:{message_id} -> stream:{user_id}:{chat_id}:{message_id}
streamseq:{user_id}:{message_id} -> streamseq:{user_id}:{chat_id}:{message_id}
A resume request carrying the wrong chat_id reads a non-existent key
and returns empty — same outcome as a DB binding check but with:
- no extra DB round-trip
- no regression on the "stub not yet persisted in DB" case that
killed the earlier DB binding attempt three rounds ago
- defense enforced at the data layer, not at a check that could be
skipped or bypassed
All four helpers (_stream_key, _stream_seq_key, _stream_log_append,
_stream_log_read, _stream_seq_allocate) and both callers (emitter +
resume_stream handler) updated consistently.
Not addressed:
- Truncation fidelity with automatic reload: deferred. Adding a
deterministic client fallback is invasive and the \`truncated\`
console warning is sufficient until this shows up in telemetry.
- Prune resumeSeqByMessageId per-message: fifth round on this. The
current design (no per-message prune) was chosen to avoid
continuation-reuses-message_id replay duplication; memory is
int-per-message bounded by chat size.
Warning: breaker was being reset by _stream_seq_allocate's INCR
success before each append attempt. Under "INCR always succeeds,
XADD always times out" the failure count never accumulated to the
threshold, so the breaker never tripped for the exact failure mode
it was supposed to short-circuit. Removed the success call from
seq allocation; only end-to-end append success now counts as
"Redis is healthy," which means a sustained append-only failure
pattern will now correctly trip the breaker after 3 frames.
Warning: replay byte-cap truncation was silent. Added \`truncated\`
flag to the resume-stream:replay payload so the client can log /
react to the case. Frontend currently logs a console warning; a
future improvement could trigger an automatic chat reload to
recover non-DB-backed side-channel events (sources, embeds) that
the final done checkpoint doesn't include.
Deferred: prune resumeSeqByMessageId on per-message terminal. Fourth
round of flip-flop on this one; the current design (no per-message
prune) was chosen to avoid continuation-reuses-message_id replay
duplication. Memory is int-per-message bounded by chat size.
Warning: the replay-cap \`and capped\` guard was letting the first
envelope through even when its own size exceeded the cap, defeating
the safety goal. Skip individual oversized envelopes entirely — the
final done checkpoint reconciles missed content.
Suggestion: the fence-fallback setTimeout called clearResumeFence
fire-and-forget. Wrap in .catch() so any future throw path doesn't
surface as an unhandled promise rejection.
Suggestion: call-style events carrying an ack callback now bypass
the replay fence. Holding them for up to RESUME_FENCE_TIMEOUT_MS
(10s) could exceed the backend's WEBSOCKET_EVENT_CALLER_TIMEOUT,
causing sio.call() to time out even though the client eventually
processes the event. Ack events don't carry seq and don't mutate
streamed content, so bypassing is safe against the replay race the
fence exists to prevent.
- resume_stream now requires both message_id AND chat_id in the
payload; rejects otherwise. Previously chat_id was optional and
replay would fall through to user-scoped-key-only validation when
missing. With the frontend now always sending chat_id (since
72429ea), the fallback was effectively dead code and weakened the
auth posture. Made the requirement explicit and fail-closed.
Collapsed the \`if chat_id:\` branch that handled the optional
case into unconditional ownership validation.
- Cap resume-stream:replay payload to 900KB (under Socket.IO's
default 1MB buffer). The most recent entries that fit are kept;
older ones are dropped. Older content is already reflected in the
DB-backed content the client loads on refresh, and the final
done:True checkpoint reconciles anything else. Prevents the
pathological case of a 2000-entry log full of large structural
envelopes blowing past the Socket.IO buffer limit.
Deferred: cross-worker live-frame ordering. Still a documented
limitation in get_event_emitter's block comment. A distributed lock
per message_id is the only real fix and is a significant addition
of infrastructure dependency for a rare scenario.
Critical: the message-in-chat check introduced in 72429ea would reject
resume for exactly the flow this feature exists for. Traced the
frontend submitPrompt → sendMessage → sendMessageSocket path: the
assistant stub is added to in-memory history before the stream
starts, but saveChatHandler/initChatHandler that persists it to DB
runs inside the completion event handler, not before. During active
streaming, the stub isn't in the DB yet, so `message_id in
chat.history.messages` would return False and block replay. Dropped
that portion. Kept the chat ownership check since that's checking a
pre-existing chat — which IS in the DB — and the user-scoped log
key already enforces cross-user isolation even when message binding
isn't validated.
Warning: seq-key TTL was only refreshed every 64 appends. A sparse
stream emitting fewer than 64 frames per RESUME_STREAM_TTL_SEC (1h)
could let the counter expire mid-stream and INCR would restart at 1,
corrupting replay. Refresh on every append instead — pipelined with
the XADD so no extra round-trip. Removed the unused refresh cadence
constant.
Deferred: prune resumeSeqByMessageId on per-message terminal. This
is the third time the bot has flip-flopped on this; the current
design (no per-message prune, cleared at chat/navigation boundaries)
was chosen in response to its earlier finding that pruning caused
continuation-reuses-message_id to replay duplicate content. Memory
is int-per-message bounded by chat size.
_float_env now falls back to default when parsed value is <= 0.
asyncio.wait_for with a 0 or negative timeout would fail immediately
every call, silently disabling the resume log — misconfiguration
now degrades to default behavior with a warning instead.
- Added message-in-chat binding check alongside the existing chat-
ownership check. Both use the same already-fetched chat object, so
this is still one DB round-trip. Defense-in-depth for the case
where a client supplies one of its own chat_ids but a different
chat's message_id — the user-scoped key would still serve the
request otherwise.
- requestResumeForMessage now early-returns if a fence already
exists for the same message. Fixes two operational concerns:
* Mixed-version deployments where an older backend doesn't echo
request_id: previously every reconnect reset the fence timer
AND set a new request_id, so no reply could ever match; now
the first fence runs to completion or timeout and only
subsequent requests after that get a fresh chance.
* Rapid reconnect churn: used to kick the timer forward
indefinitely, now one fence per message lifecycle.
- onResumeStreamReplay no longer blindly clears the fence in finally.
It stops deleting the active request_id on entry and instead checks,
in the finally, whether it is still the active request before
calling clearResumeFence. A newer request arriving during replay
sets its own id; the older handler now leaves that state alone and
lets the newer request's lifecycle drive cleanup.
- requestResumeForAllInProgress filters by `done === false` instead of
`!done`. Legacy messages with a missing `done` field used to fan
out as spurious resume requests on every reconnect/load; now only
explicitly in-progress assistants get targeted.
- Terminal-event detection for TTL shortening no longer treats any
event with `data.error` present as terminal. Restricted to
`type === 'chat:completion' AND data.error`, matching the exact
shape middleware.py emits when the provider actually errors out.
A transient warning on some other event type won't shorten the log
TTL prematurely anymore.
- loadChat now calls requestResumeForAllInProgress BEFORE the
interrupted-generation mark-done heuristic. That heuristic flips
done=true when getTaskIdsByChatId returns empty/null, which includes
API failures; previously that path would prevent resume from being
attempted even when the stream was still alive server-side. Spurious
resume requests cost one round trip and clear the fence via empty
reply, so calling it unconditionally first is strictly safer.
- Env-var float parsing wrapped in a _float_env helper that falls back
to default with a warning on invalid input instead of raising at
import time. Previous code would ValueError out of the module and
prevent socket service startup on operator typo.
Pushed back: bot flagged asyncio.Lock as unable to be stored in a
WeakValueDictionary. Verified directly on CPython — both weakref.ref
and WeakValueDictionary insertion work fine for asyncio.Lock. No
change needed.
- chatEventHandler queue now stores {event, cb} pairs. Live frames
buffered during a replay fence used to lose their Socket.IO ack
callback because we only pushed the event. When replayed, callback-
style events (confirmation / execute / input) would silently never
respond. Now we preserve cb and pass it through to chatEventHandler
on flush.
- resume_stream ownership check is now fail-CLOSED on DB exception.
Previous fail-open skipped the check precisely when infra was
unstable. Client still gets a deterministic reply (empty envelopes)
so the fence clears; it just can't resume in that specific failure
mode. Absent chat_id still falls through to the user-scoped key
guarantee.
- Added per-message asyncio.Lock via a WeakValueDictionary keyed by
(user_id, message_id). Serializes seq-alloc + log-append + emit so
concurrent emitters for the same message_id within a worker can't
interleave and produce out-of-seq live frames. WeakValueDictionary
cleans up automatically once no coroutine holds the lock.
Cross-worker concurrency is still unprotected (would need a
distributed lock) but that's an even rarer scenario.
- Seq key moved from `{prefix}:stream:{user}:{msg}:seq` to
`{prefix}:streamseq:{user}:{msg}`. A message_id containing a literal
`:seq` suffix would otherwise make its stream key equal to another
message's seq key, which under a user-controlled message_id would
collide INCR against an XADD on the same Redis key.
- clearResumeFence swap-and-iterate instead of shift() in a loop.
Array.shift is O(n) per call, so the old drain was O(n²). Practical
frame counts during a fence window are small (dozens worst-case) so
this was never a user-visible problem, but the fix is two lines and
removes a Big-O footgun. Concurrency behavior unchanged: the batch
is captured via reference-swap so frames arriving during an await
continue buffering into the fresh empty array still present in the
map, and the outer while loop drains those in the next iteration.
Not addressed:
- Concurrent-emitter out-of-seq live frames — already documented as a
known limitation with a block comment at get_event_emitter; fix
requires distributed locking.
- Prune resumeSeqByMessageId on per-message terminal events —
deliberately removed two rounds ago because it caused continuation-
reuses-message_id to replay duplicate content; the trade-off
(bounded int-per-message memory vs. correctness) is already
captured in that commit message.
Matches the existing guard in clearResumeFence. A malformed entry
from the server (version skew, serialization mishap) would otherwise
throw on `envelope._replayed = true` mid-iteration and leave the
fence half-flushed via the finally-block.
- resume_stream now short-circuits when ENABLE_REALTIME_CHAT_SAVE is
on, mirroring the write-side gate in _stream_seq_allocate. Without
this, stale logs written before the flag was flipped (or by a
mixed-version peer) could still replay on top of DB-backed content
and double-apply.
- Added optional chat-ownership check: if the client sends chat_id,
we validate via Chats.get_chat_by_id_and_user_id and reject on
failure. Defense-in-depth alongside the user-scoped key. Fails
OPEN when the chat lookup errors or chat_id is absent, so we
don't regress the "stub not persisted in DB" refresh scenario
that motivated removing the earlier stricter check. Frontend
updated to include chat_id in the resume payload.
- Client now requires an exact request_id match when expected is
set. A reply without a request_id used to fall through and clear
the active fence, which could prematurely flush a newer in-flight
request's buffer. Now rejects both missing and mismatched.
Deferred: suggestion to prune resumeSeqByMessageId on per-message
terminal events. That was intentionally removed two rounds ago
because the same pattern caused continuation-reuses-message_id
to replay duplicate content. The memory footprint is int-per-
message bounded by chat size and is cleared at chat/navigation
boundaries — this is a deliberate trade-off, not an oversight.
Critical: last round's fence-flush fix (keep the queue in the map
while draining) turned into an infinite loop because every shifted
event went back through chatEventHandler's fence-check branch and
was re-queued into the same queue we were trying to drain. Mark
each flushed event `_replayed = true` before dispatch so the
chatEventHandler fence-buffering branch short-circuits. Re-entrancy
ordering is still preserved: live frames arriving during an await
inside a flushed-event's handler still don't carry `_replayed` and
continue to buffer into the same queue, staying ordered after the
current flush batch.
Warning: under sustained Redis slowness every emit was paying
100ms on INCR and another 100ms on XADD+EXPIRE, so live token
delivery could stall 200ms/frame during an outage. Added a
module-level circuit breaker that trips after 3 consecutive
failures/timeouts and short-circuits the hot-path Redis calls for
a 10s cooldown window. First successful call after cooldown resets
the counter. Replay reads deliberately bypass the breaker because
they're user-initiated and worth the slower timeout.
clearResumeFence previously deleted the queue from the map before
iterating it via for-of over the detached reference. Because
chatEventHandler awaits tick() internally and yields control, a live
frame arriving during that yield would find no queue in the map,
bypass buffering entirely, advance lastSeq via the dedupe guard, and
cause any still-pending earlier-seq frame in the flush iteration to
be dropped on its next chatEventHandler call.
Switch to a while+shift drain that keeps the queue present in the
map for the whole flush. Frames arriving during any yield inside
chatEventHandler push into the same queue the while loop is draining,
so they stay ordered after earlier frames. The map entry is deleted
only after the queue has been fully drained — between the final
length check and the delete there is no await, so no new frame can
slip in and be lost.
Also moves the resumeActiveRequestIdByMessageId cleanup here so the
request-id map doesn't accumulate stale entries over long-lived
sessions.
Not addressed: suggestion to add explicit chat_id validation to the
resume endpoint. The per-user key scoping already enforces that a
session can only ever read its own logs, and adding a DB
chat-ownership check would re-introduce the "stub not yet persisted"
failure mode we removed earlier. Deferring as defense-in-depth that
doesn't correspond to a real threat under the current key design.
- Stopped deleting resumeSeqByMessageId entries on done / cancel /
error. The map now only clears on chat/navigation transitions
(loadChat, initNewChat, component unmount). Fixes a continuation-
reuses-message_id duplication hole: if a message completes and its
id is immediately reused for a continued/extended response, the
log's 30s grace window can still serve old envelopes; resetting
the client's lastSeq to 0 on done made those old envelopes replay.
Keeping the seq alive until the message is truly retired avoids
re-applying them.
- resume_stream now always replies when the payload has a message_id,
even on auth failure (empty envelopes). Silent early-return branches
could previously leave the client fence raised for the full 10s
fallback timeout during reconnect/auth churn. Silent drops only
remain for cases where the client couldn't have raised a fence.
- Terminal TTL shortening now also triggers on chat:tasks:cancel and
error-finalized completions, not just done:True. Cancelled/errored
streams no longer leak log+seq keys for the full 1h TTL.
- Timeouts split and env-configurable. RESUME_STREAM_REDIS_TIMEOUT_SEC
(hot path, default 100ms) stays tight to protect live streaming.
RESUME_STREAM_READ_TIMEOUT_SEC (replay XRANGE, default 1s) is looser
since a resume is user-blocking and silently timing out is worse
than a brief extra wait. Cross-region / non-colocated Redis setups
can override either.
- resume-stream now carries a request_id; the server echoes it in
resume-stream:replay and the client ignores replies whose id does
not match the active request for that message. Prevents reconnect
churn where a stale reply could clear a fence belonging to a
newer in-flight request.
- Documented the concurrent-emitter live ordering caveat in
get_event_emitter. Single-emitter flows are safe (streaming loop
awaits sequentially). Replay is safe (sorted by seq on read).
Overlapping emitters for the same message_id can produce live frames
out of seq order — accepting as a known limitation; proper fix
needs either a distributed per-message lock or a client reorder
buffer, neither justified by the rarity of the scenario.
- seq key now gets EXPIRE on the same periodic cadence as the stream
key, not just on the first INCR. Prevents INCR restarting at 1 on
responses longer than RESUME_STREAM_TTL_SEC (1h), which would have
corrupted replay ordering.
- Hot-path Redis timeout trimmed 200ms → 100ms. Halves worst-case
stall under degraded Redis (now ~200ms max per frame if both INCR
and append time out); healthy Redis (<1ms) is unaffected.
Not addressed: the fundamental 2-RTT-per-frame cost of separate INCR
and XADD. Collapsing them would require either a Lua script (cluster
hash-tag concerns) or switching to Redis stream IDs as the seq cursor
(invasive protocol change). Leaving the structure intact — if
profiling shows real stalls in prod, stream-ID-as-seq is the cleaner
follow-up.
Tone down verbose comments added over the last few review rounds
(-79 lines net). Logic unchanged by the tone-down itself.
Also addresses three new findings:
- Hot-path Redis timeout dropped 500ms → 200ms. Halves worst-case
stall under degraded Redis; healthy Redis (<1ms) is unaffected.
- onDestroy now calls dropAllResumeFences so pending fence timers
can't fire post-unmount against stale component state.
- Fence flush uses insertion order instead of partition+sort. Live
frames were emitted sequentially by one emitter and preserved by
socket.io transport order, so push-order already == seq-order;
sorting was mixing seq and seq-less frames incorrectly in the
Redis-degraded path.
Critical: INCR and XADD are separate calls, so with overlapping
emitters the stream's append order can diverge from seq order (a
concurrent emitter can win INCR for seq N+1 yet lose XADD and land
ahead of seq N in the stream). Replay returned entries in append
order, which meant the later-seq-but-earlier-in-stream frame would
advance lastSeq, permanently dropping the earlier-seq-but-later-in-
stream frame via the client dedupe guard. Fix: _stream_log_read now
sorts envelopes by seq before returning, making replay order
independent of append order.
Warning: loadChat now unconditionally triggers resume for unfinished
assistants with last_seq=0, which in ENABLE_REALTIME_CHAT_SAVE mode
(DB is kept up-to-date per token) would replay content already
loaded from the DB and produce duplicates. Gate _stream_seq_allocate
on `not ENABLE_REALTIME_CHAT_SAVE` — in realtime mode frames emit
without a seq, nothing is logged, client bypasses dedupe/resume, DB
stays authoritative for refresh recovery.
Suggestion: fence flush sort comparator returned 0 for mixed
seq/no-seq pairs, which is not strict ordering. Replaced with a
partition (seq-bearing vs. seq-less) + sort the seq-bearing subset +
concat. Deterministic regardless of engine sort stability.
Three review findings:
- _stream_log_read now wraps xrange in asyncio.wait_for with the same
RESUME_STREAM_REDIS_TIMEOUT_SEC used by writes. Returns empty on
timeout so a degraded Redis can't hang resume-stream handlers.
- resume_stream docstring clarified: we always emit the replay ack
when a message_id is present (including the Redis-down and
empty-log cases), but auth-rejection branches (non-dict payload,
missing session, missing message_id) intentionally drop silently —
those callers either didn't raise a client fence at all or aren't
legitimate sessions.
- Fence flush sort now stable for seq-less events: only orders when
both sides have a numeric seq, otherwise preserves insertion order.
Prevents graceful-degradation seq-less frames from being forced to
the front and misordering state transitions like replace / done.
Critical: seq was an in-memory per-emitter counter (seeded from the
log's max) which two concurrent emitters for the same message_id could
both read + advance independently, producing duplicate seqs and causing
the client dedupe guard to drop one frame per collision. Replace with a
per-message Redis INCR on a dedicated `:seq` key — atomic by
construction, correct under overlap regardless of how rare overlap is
in practice. On Redis unavailability or timeout, emit the frame
without a seq and skip the log append; the client treats seq-less
frames as "apply directly, no dedupe, no resume" — live streaming
survives, resume is the thing that degrades.
Warning: adding INCR put two Redis RTTs in the streaming hot path
(INCR then XADD), so a slow Redis could stall live tokens. Wrap both
calls with asyncio.wait_for(..., timeout=0.5s) and emit anyway on
timeout. Under Redis hiccups, frames still reach the user; only resume
for those specific frames is lost. Done-TTL shortening also goes
through a pipelined wait_for so it can't stall the completion path
either.
Suggestion: stale reference to the removed `resume-stream:complete`
event in a comment. Updated to reference the current single-batch
`resume-stream:replay` that serves as both payload and completion
signal.
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>
