fix(webui): stage SSE-error reconnect probes so the live turn doesn't blank out - #5122
allenliang2022 wants to merge 3 commits into
Conversation
… blank out After the chat EventSource fires 'error', attachLiveStream made a single 1.5s reconnect probe and, if /api/chat/stream/status did not yet report active or replay_available, fell straight through to _handleStreamError(): clearing the owner INFLIGHT state, nulling S.activeStreamId, pushing a 'Connection interrupted' message and re-rendering. That wiped the live message DOM even when the backend was still producing tokens or the run-journal replay file was a beat away from becoming visible, so the settled response disappeared and then reappeared from sidecar/replay (or only on refresh). Replace the single probe with a short staged retry window (_retryDelays=[1500,3000,5000,8000] ms) driven by a recursive _probeReconnect. Each stage re-queries stream status and reconnects/replays when the backend is reachable; the live DOM and S.activeStreamId/INFLIGHT state are kept intact across the whole window, and _handleStreamError is only reached after every stage has failed. Adds tests/test_sse_error_multi_probe_reconnect.py source-locking the staged shape and the ordering invariant (terminal error only after the retry window is exhausted; no inflight/activeStreamId clear mid-window).
… window - messages.js: declare _retryDelays before the first setComposerStatus and show 'Reconnecting… (1/4)' on the initial probe so the counter progresses (1/4)->(2/4)->(3/4)->(4/4) instead of jumping from a bare 'Reconnecting…' to (2/4) (looked like it started at 2). - test: replace the fixed block_start+1600 char window with a brace-matched _reconnect_block() helper so the ordering / no-early-clear assertions cannot silently under-assert if the reconnect block grows. Add a source-lock for the (1/N) stage-counter start.
|
Thanks @greptile-apps — both P2s addressed in 1. Stage counter skipping 2. Magic-number test window (
|
|
Both fixes look correct and complete.
Both P2s are fully addressed. LGTM — this is ready to merge. Tip: You can customize Greptile's behavior for this repo with |
… literal
test_error_reconnect_path_can_restore_from_journal anchored on the exact string
setComposerStatus('Reconnecting — but the first status was changed to a template
literal `Reconnecting… (1/${_retryDelays.length})` for the staged-probe counter,
so that single-quoted anchor no longer exists (ValueError: substring not found in
CI shard 2). Re-anchor on the block's stable entry point _reconnectAttempted=true;
(unique in source) and widen the window to 1100 chars; the st.active /
st.replay_available / Restoring stream / _runJournalReplayParams() assertions are
unchanged.
…squena#5096 fork/edit/rewind context bundle nesquena#5124/nesquena#5125/nesquena#5126)
…oads new messages (nesquena#5177) After a hidden interval that added persisted messages (post-turn bg-review writes, sibling-tab writes, the just-finished main turn flushing), switching the tab back caused refreshActiveSessionIfExternallyUpdated('visible'|'focus') to hit remoteCount !== localCount and call loadSession(sid, {force:true}), which synchronously did S.messages = [] before awaiting the metadata + messages fetches. The visible result was the entire conversation transcript blanking for the round-trip and reappearing — '对话突然消失,重刷才回来'. sessions.js itself warns about this 'disappear and reappear' tradeoff at the top of the remoteCount/remoteLast guard, but only short-circuits the metadata-only branch (nesquena#5061). nesquena#5122's 4-probe staged reconnect covers a different path (SSE error with ready_state=2 on a visible tab) and does not apply when the SSE error arrives while visibility_state='hidden' — that bottoms out through _deferStreamErrorIfPageHidden before the reconnect block ever runs. Fix: - refreshActiveSessionIfExternallyUpdated maps the visibility/focus recovery reasons ({visible, focus}) to a new keepStaleUntilLoaded option, forwarded to loadSession. The post-stream idle reconcile and the poll/external paths keep their existing behaviour. - loadSession ANDs opts.keepStaleUntilLoaded with sameSessionForceReload (so cross-session switches still clear synchronously — leaving a prior session's transcript on screen during a navigation is the original bug the clear was written for) and, on that path, skips the synchronous S.messages/S.toolCalls/_messagesTruncated/_oldestIdx clear. The new transcript is SWAPPED into S.messages by _ensureMessagesLoaded(sid, {force:true}), so the user sees old DOM directly replaced by new DOM in a single render frame. - _ensureMessagesLoaded grows an opts.force escape hatch so its 'messages already populated' early-return cannot skip the swap when stale messages are still in place. Verified live on a source 8701 build: - before: minHtmlObserved 65874, minKidsObserved 11 — 372 samples at 16ms, DOM html length NEVER dipped below the pre-call value during the reload. - loadSession was called with force:true keepStale:true reason:'visible', outcome 'reloaded', sMsgs 21 → 1037, after kids 11 → 134 (single-frame swap). Tests: - tests/test_issue5177_hidden_tab_blank_gap.py (new, 7 cases): source-locks the keep-stale guard shape, the recovery-reason map, the _ensureMessagesLoaded opts.force escape hatch, and that the synchronous clear is wrapped in if(!_keepStaleUntilLoaded). - Updated the two existing source-lock tests whose anchor strings included the loadSession call signature (tests/test_webui_external_refresh_frontend.py + tests/test_tars_scroll_reset_regressions.py) to match the new loadSession(... keepStaleUntilLoaded:_keepStaleUntilLoaded) form. No behavioural assertions changed. Local: node --check static/sessions.js OK. 122 passed across test_issue5177_hidden_tab_blank_gap.py + the full adjacent set (external-refresh / journal-frontend / scroll-reset / reconnect-chronology / inflight-stream-reuse / sse-error-multi-probe / issue4295 / issue4856).
…oads new messages (nesquena#5177) After a hidden interval that added persisted messages (post-turn bg-review writes, sibling-tab writes, the just-finished main turn flushing), switching the tab back caused refreshActiveSessionIfExternallyUpdated('visible'|'focus') to hit remoteCount !== localCount and call loadSession(sid, {force:true}), which synchronously did S.messages = [] before awaiting the metadata + messages fetches. The visible result was the entire conversation transcript blanking for the round-trip and reappearing — '对话突然消失,重刷才回来'. sessions.js itself warns about this 'disappear and reappear' tradeoff at the top of the remoteCount/remoteLast guard, but only short-circuits the metadata-only branch (nesquena#5061). nesquena#5122's 4-probe staged reconnect covers a different path (SSE error with ready_state=2 on a visible tab) and does not apply when the SSE error arrives while visibility_state='hidden' — that bottoms out through _deferStreamErrorIfPageHidden before the reconnect block ever runs. Fix: - refreshActiveSessionIfExternallyUpdated maps the visibility/focus recovery reasons ({visible, focus}) to a new keepStaleUntilLoaded option, forwarded to loadSession. The post-stream idle reconcile and the poll/external paths keep their existing behaviour. - loadSession ANDs opts.keepStaleUntilLoaded with sameSessionForceReload (so cross-session switches still clear synchronously — leaving a prior session's transcript on screen during a navigation is the original bug the clear was written for) and, on that path, skips the synchronous S.messages/S.toolCalls/_messagesTruncated/_oldestIdx clear. The new transcript is SWAPPED into S.messages by _ensureMessagesLoaded(sid, {force:true}), so the user sees old DOM directly replaced by new DOM in a single render frame. - _ensureMessagesLoaded grows an opts.force escape hatch so its 'messages already populated' early-return cannot skip the swap when stale messages are still in place. Verified live on a source 8701 build: - before: minHtmlObserved 65874, minKidsObserved 11 — 372 samples at 16ms, DOM html length NEVER dipped below the pre-call value during the reload. - loadSession was called with force:true keepStale:true reason:'visible', outcome 'reloaded', sMsgs 21 → 1037, after kids 11 → 134 (single-frame swap). Tests: - tests/test_issue5177_hidden_tab_blank_gap.py (new, 7 cases): source-locks the keep-stale guard shape, the recovery-reason map, the _ensureMessagesLoaded opts.force escape hatch, and that the synchronous clear is wrapped in if(!_keepStaleUntilLoaded). - Updated the two existing source-lock tests whose anchor strings included the loadSession call signature (tests/test_webui_external_refresh_frontend.py + tests/test_tars_scroll_reset_regressions.py) to match the new loadSession(... keepStaleUntilLoaded:_keepStaleUntilLoaded) form. No behavioural assertions changed. Local: node --check static/sessions.js OK. 122 passed across test_issue5177_hidden_tab_blank_gap.py + the full adjacent set (external-refresh / journal-frontend / scroll-reset / reconnect-chronology / inflight-stream-reuse / sse-error-multi-probe / issue4295 / issue4856).
…oads new messages (nesquena#5177) After a hidden interval that added persisted messages (post-turn bg-review writes, sibling-tab writes, the just-finished main turn flushing), switching the tab back caused refreshActiveSessionIfExternallyUpdated('visible'|'focus') to hit remoteCount !== localCount and call loadSession(sid, {force:true}), which synchronously did S.messages = [] before awaiting the metadata + messages fetches. The visible result was the entire conversation transcript blanking for the round-trip and reappearing — '对话突然消失,重刷才回来'. sessions.js itself warns about this 'disappear and reappear' tradeoff at the top of the remoteCount/remoteLast guard, but only short-circuits the metadata-only branch (nesquena#5061). nesquena#5122's 4-probe staged reconnect covers a different path (SSE error with ready_state=2 on a visible tab) and does not apply when the SSE error arrives while visibility_state='hidden' — that bottoms out through _deferStreamErrorIfPageHidden before the reconnect block ever runs. Fix: - refreshActiveSessionIfExternallyUpdated maps the visibility/focus recovery reasons ({visible, focus}) to a new keepStaleUntilLoaded option, forwarded to loadSession. The post-stream idle reconcile and the poll/external paths keep their existing behaviour. - loadSession ANDs opts.keepStaleUntilLoaded with sameSessionForceReload (so cross-session switches still clear synchronously — leaving a prior session's transcript on screen during a navigation is the original bug the clear was written for) and, on that path, skips the synchronous S.messages/S.toolCalls/_messagesTruncated/_oldestIdx clear. The new transcript is SWAPPED into S.messages by _ensureMessagesLoaded(sid, {force:true}), so the user sees old DOM directly replaced by new DOM in a single render frame. - _ensureMessagesLoaded grows an opts.force escape hatch so its 'messages already populated' early-return cannot skip the swap when stale messages are still in place. Verified live on a source 8701 build: - before: minHtmlObserved 65874, minKidsObserved 11 — 372 samples at 16ms, DOM html length NEVER dipped below the pre-call value during the reload. - loadSession was called with force:true keepStale:true reason:'visible', outcome 'reloaded', sMsgs 21 → 1037, after kids 11 → 134 (single-frame swap). Tests: - tests/test_issue5177_hidden_tab_blank_gap.py (new, 7 cases): source-locks the keep-stale guard shape, the recovery-reason map, the _ensureMessagesLoaded opts.force escape hatch, and that the synchronous clear is wrapped in if(!_keepStaleUntilLoaded). - Updated the two existing source-lock tests whose anchor strings included the loadSession call signature (tests/test_webui_external_refresh_frontend.py + tests/test_tars_scroll_reset_regressions.py) to match the new loadSession(... keepStaleUntilLoaded:_keepStaleUntilLoaded) form. No behavioural assertions changed. Local: node --check static/sessions.js OK. 122 passed across test_issue5177_hidden_tab_blank_gap.py + the full adjacent set (external-refresh / journal-frontend / scroll-reset / reconnect-chronology / inflight-stream-reuse / sse-error-multi-probe / issue4295 / issue4856).
…oads new messages (nesquena#5177) After a hidden interval that added persisted messages (post-turn bg-review writes, sibling-tab writes, the just-finished main turn flushing), switching the tab back caused refreshActiveSessionIfExternallyUpdated('visible'|'focus') to hit remoteCount !== localCount and call loadSession(sid, {force:true}), which synchronously did S.messages = [] before awaiting the metadata + messages fetches. The visible result was the entire conversation transcript blanking for the round-trip and reappearing — '对话突然消失,重刷才回来'. sessions.js itself warns about this 'disappear and reappear' tradeoff at the top of the remoteCount/remoteLast guard, but only short-circuits the metadata-only branch (nesquena#5061). nesquena#5122's 4-probe staged reconnect covers a different path (SSE error with ready_state=2 on a visible tab) and does not apply when the SSE error arrives while visibility_state='hidden' — that bottoms out through _deferStreamErrorIfPageHidden before the reconnect block ever runs. Fix: - refreshActiveSessionIfExternallyUpdated maps the visibility/focus recovery reasons ({visible, focus}) to a new keepStaleUntilLoaded option, forwarded to loadSession. The post-stream idle reconcile and the poll/external paths keep their existing behaviour. - loadSession ANDs opts.keepStaleUntilLoaded with sameSessionForceReload (so cross-session switches still clear synchronously — leaving a prior session's transcript on screen during a navigation is the original bug the clear was written for) and, on that path, skips the synchronous S.messages/S.toolCalls/_messagesTruncated/_oldestIdx clear. The new transcript is SWAPPED into S.messages by _ensureMessagesLoaded(sid, {force:true}), so the user sees old DOM directly replaced by new DOM in a single render frame. - _ensureMessagesLoaded grows an opts.force escape hatch so its 'messages already populated' early-return cannot skip the swap when stale messages are still in place. Verified live on a source 8701 build: - before: minHtmlObserved 65874, minKidsObserved 11 — 372 samples at 16ms, DOM html length NEVER dipped below the pre-call value during the reload. - loadSession was called with force:true keepStale:true reason:'visible', outcome 'reloaded', sMsgs 21 → 1037, after kids 11 → 134 (single-frame swap). Tests: - tests/test_issue5177_hidden_tab_blank_gap.py (new, 7 cases): source-locks the keep-stale guard shape, the recovery-reason map, the _ensureMessagesLoaded opts.force escape hatch, and that the synchronous clear is wrapped in if(!_keepStaleUntilLoaded). - Updated the two existing source-lock tests whose anchor strings included the loadSession call signature (tests/test_webui_external_refresh_frontend.py + tests/test_tars_scroll_reset_regressions.py) to match the new loadSession(... keepStaleUntilLoaded:_keepStaleUntilLoaded) form. No behavioural assertions changed. Local: node --check static/sessions.js OK. 122 passed across test_issue5177_hidden_tab_blank_gap.py + the full adjacent set (external-refresh / journal-frontend / scroll-reset / reconnect-chronology / inflight-stream-reuse / sse-error-multi-probe / issue4295 / issue4856).
Problem
On the source WebUI, after a turn finishes streaming the whole assistant message
could briefly blank out and then reappear (sometimes only after a refresh).
The trigger is a transient
errorevent on the chatEventSource.Root cause
In
attachLiveStream, theEventSource.onerrorhandler made a single 1.5sreconnect probe. If
/api/chat/stream/statusdid not yet reportactiveorreplay_availableat that one moment, it fell straight through to_handleStreamError(source), which:_clearOwnerInflightState()S.activeStreamId = nullConnection interrupted…messagerenderMessages()That wipes the live message DOM/state even when the backend was still
producing tokens, or when the run-journal replay file was a beat away from
becoming visible. The settled response then reappeared later from the
sidecar/replay path — producing the disappear-then-restore flicker users saw.
This was reproduced on a live source build: client
sse_errorevents on thechat response stream led to the live state being cleared while the backend
stream_end/donecompleted normally afterward — i.e. the frontend clearedtoo early, the backend was fine.
Fix
Replace the single 1.5s probe with a short staged retry window
_retryDelays = [1500, 3000, 5000, 8000]ms, driven by a recursive_probeReconnect(attempt):/api/chat/stream/statusand reconnects (active)or replays (
replay_available) as soon as the backend is reachable;S.activeStreamId/ INFLIGHT state are kept intact acrossthe whole window;
_handleStreamError(source)is only reached after every stage has failed.The existing offline / page-hidden deferrals and the
_isSessionCurrentPaneguard are preserved on every stage, so a backgrounded or switched-away session
still bails out instead of leaking a reconnect.
Verification
Reproduced and verified live on a source build of nesquena/hermes-webui at HEAD with an instrumented
EventSource (hooking the chat stream
onerror, a stubbed/api/chat/stream/status, and a 100ms sampler ofS.activeStreamId+ themessage DOM + the interrupted-marker text):
returns not-ready on probe Portability #1 then
activeon probe Hermes Web UI — Sprints 11-14: multi-provider models, settings, sessi… #2:S.activeStreamIdstayed non-null the entire time, no
Connection interruptedmarker everappeared, and the stream reattached at ~4.5s (
Reconnecting… (2/4)→Reconnected). The old single-probe path would have cleared at 1.5s.live state was held intact for the full window (probes at ~1.5s / 4.5s / 9.5s /
17.6s, status text
Reconnecting…→(2/4)→(3/4)→(4/4)), and onlyafter the last stage failed (~17.6s) did it fall through to
_handleStreamError(activeStreamId → null + interrupted marker). So the errorpath is preserved, just deferred until recovery is actually impossible.
Tests
tests/test_sse_error_multi_probe_reconnect.py(new) — source-locks the stagedshape (
_retryDelays,_probeReconnect, per-stage status re-query) and theordering invariant: the terminal
_handleStreamErrorcall sits after thenext-stage scheduling guard, and neither
_clearOwnerInflightState()norS.activeStreamId=nullappears in the reconnect block before the window isexhausted.
node --check static/messages.jspasses.test_webui_external_refresh_frontend.py,test_issue3916_external_refresh_poll.py,test_tars_scroll_reset_regressions.py,test_issue4811_reconnect_chronology.py,test_issue3877_midstream_flicker.py,test_issue3103_sse_no_connection_close.py,test_inflight_stream_reuse.py(84 passed) + the new file (7 passed).
Single-file behavior change (
static/messages.js) plus its source-lock test.