From eabbe9a821333eebc4e9ffca34115a84e0cb3e39 Mon Sep 17 00:00:00 2001 From: allenliang2022 Date: Tue, 30 Jun 2026 13:04:39 +0800 Subject: [PATCH] fix(webui): hidden-tab poll keeps retrying when attach defers in multi-pane (#5172 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #5172 (render server-initiated turns in a hidden tab, shipped v0.51.743 via #5218) has a non-blocking multi-pane edge flagged in review: when a server-initiated turn arrives for a session that is NOT the active pane, the hidden-tab status poll called _attachServerInitiatedStream() and could not tell whether the attach actually happened. _attachServerInitiatedStream returned undefined in all paths, so the poll had no signal — a stop-on-call would cancel polling while the turn was never rendered, leaving it invisible until the next user interaction. Fix: _attachServerInitiatedStream now returns a bool — true when this tab is responsible for rendering the stream (attached, or already attached to the same sid/streamId), false when the attach was not consummated (sid not the current pane, invalid input, or a thrown error). _startHiddenActiveStreamPoll captures the bool and only calls _stopHiddenActiveStreamPoll() when true; on false it keeps polling and re-tries on a later tick (e.g. once the user switches the pane back to sid, or the turn finishes and status returns active_stream_id=null). Tests in tests/test_hidden_tab_server_initiated_turn.py: assert all return paths are explicit bool (no bare return), the non-current-pane path returns false, and the poll gates _stopHiddenActiveStreamPoll on the attach result. --- static/messages.js | 97 +++++++++++++++++-- .../test_hidden_tab_server_initiated_turn.py | 58 ++++++++++- 2 files changed, 148 insertions(+), 7 deletions(-) diff --git a/static/messages.js b/static/messages.js index f4d548458..0d41169e1 100644 --- a/static/messages.js +++ b/static/messages.js @@ -6399,23 +6399,48 @@ let _sessionStreamHiddenSid = null; // on session switch. let _sessionStreamHiddenPollTimer = null; let _sessionStreamHiddenPollSid = null; +// Bounded-retry budget for the hidden poll's "attach returned false → keep +// polling" path (PR #5266 follow-up gate). A never-current pane (multi-pane: +// another session stays on screen) would otherwise poll /api/session/status +// every 6s forever. Cap the consecutive-false retries per (sid, streamId); once +// the budget is exhausted, stop the hidden poll and rely on the normal +// session-switch / loadSession / visible-tab SSE to reattach later. +let _sessionStreamHiddenPollFalseStreamId = null; +let _sessionStreamHiddenPollFalseCount = 0; +const _SESSION_STREAM_HIDDEN_POLL_MAX_FALSE = 20; // ~2 min at the 6s cadence // Attach the existing chat-stream renderer to a server-created stream. Shared // by the `server_turn_started` SSE handler (visible tab) and the hidden-tab // active-stream poll. Idempotent per (sid, streamId): bails if this tab is // already rendering that stream. `recovered` routes through the reconnecting // (replay) path so the renderer rebuilds from the run journal mid-flight. +// +// Returns true when this tab is now responsible for rendering the stream +// (either the renderer was attached, OR a renderer was already attached to +// the same (sid,streamId) — both are "stream is in good hands"). Returns +// false when the attach was NOT consummated — sid not on screen (multi-pane: +// the active pane is a different session) or input invalid or thrown — so +// the hidden-tab poll caller can keep polling and try again on a later tick +// (e.g. once the user switches the pane back to `sid`). Without this +// signal, a poll that fires while another session is in the current pane +// would attach nothing AND stop polling, leaving the turn invisible until +// the next user interaction. function _attachServerInitiatedStream(sid, streamId, recovered) { + let handedOff = false; try { streamId = String(streamId || ''); - if (!streamId) return; + if (!streamId) return false; const isCurrent = (typeof _isSessionCurrentPane === 'function') ? _isSessionCurrentPane(sid) : (S.session && S.session.session_id === sid); - if (!isCurrent) return; - if (S.activeStreamId === streamId) return; + // Multi-pane edge: caller's sid is not the active pane. Don't attach to + // a different pane's UI; tell the poll to keep trying. + if (!isCurrent) return false; + // Already rendering this exact stream — treat as success so the poll + // stops cleanly (renderer owns the stream from here). + if (S.activeStreamId === streamId) return true; const existingLive = (typeof LIVE_STREAMS !== 'undefined') ? LIVE_STREAMS[sid] : null; - if (existingLive && existingLive.streamId === streamId) return; + if (existingLive && existingLive.streamId === streamId) return true; S.busy = true; S.activeStreamId = streamId; if (S.session && S.session.session_id === sid) { @@ -6435,9 +6460,40 @@ function _attachServerInitiatedStream(sid, streamId, recovered) { (S.session && S.session.pending_attachments) || [], recovered ? {reconnecting: true} : {}, ); + // The renderer now owns the stream. Any failure AFTER this point (e.g. a + // post-attach UI refresh throwing) is NOT an attach failure — the stream is + // in good hands, so the caller must not be told to keep polling/retrying. + handedOff = true; } if (typeof renderSessionList === 'function') void renderSessionList(); - } catch (_) {} + return true; + } catch (_) { + try { console.error('hidden-tab server-initiated attach failed', _); } catch (_e) {} + // If the stream was already handed to the renderer, the attach DID succeed; + // a later UI-refresh throw must not be reported as failure (would make the + // poll keep retrying an already-attached stream). + if (handedOff) return true; + // Otherwise the attach threw mid-setup, leaving partial state (S.busy / + // S.activeStreamId / S.session.active_stream_id were set before the DOM + // calls). Clear it so the next hidden-poll tick doesn't see a stale + // activeStreamId, exit early as "already attached", and wedge the turn + // invisible with the composer stuck busy. Only clear when THIS pane/session + // still owns the sid/streamId we were attaching (don't stomp a newer stream + // that a concurrent path may have started). + try { + const stillOurs = (S.session && S.session.session_id === sid) && + (S.activeStreamId === streamId || (S.session && S.session.active_stream_id === streamId)); + if (stillOurs) { + S.busy = false; + S.activeStreamId = null; + if (S.session) S.session.active_stream_id = null; + if (typeof updateSendBtn === 'function') updateSendBtn(); + if (typeof syncTopbar === 'function') syncTopbar(); + if (typeof renderSessionList === 'function') void renderSessionList(); + } + } catch (_e2) {} + return false; + } } // Poll /api/session/status (~6s) for an active stream while the tab is hidden. @@ -6462,7 +6518,33 @@ function _startHiddenActiveStreamPoll(sid) { const streamId = d.active_stream_id; if (streamId && S.activeStreamId !== String(streamId)) { // Server-initiated turn in flight while hidden → attach as replay. - _attachServerInitiatedStream(sid, streamId, true); + // Only stop polling when the attach actually consummated. In the + // multi-pane edge where the active pane is a different session, + // _attachServerInitiatedStream returns false (sid not on screen), so + // we keep polling and re-try on a later tick — e.g. once the user + // switches the active pane back to `sid`, or by then the turn has + // finished and status returns active_stream_id=null naturally. + const streamKey = String(streamId); + // Reset the bounded-retry budget whenever the stream id changes (a new + // server-initiated turn deserves a fresh set of retries). + if (_sessionStreamHiddenPollFalseStreamId !== streamKey) { + _sessionStreamHiddenPollFalseStreamId = streamKey; + _sessionStreamHiddenPollFalseCount = 0; + } + const attached = _attachServerInitiatedStream(sid, streamId, true); + if (attached) { + _stopHiddenActiveStreamPoll(); + } else { + // Bounded give-up: a never-current pane would poll forever otherwise. + // Count consecutive false attaches for this stream id; once the + // budget is spent, stop the hidden poll. A later session-switch / + // loadSession / visible-tab SSE will reattach if the turn is still + // live by then. + _sessionStreamHiddenPollFalseCount += 1; + if (_sessionStreamHiddenPollFalseCount >= _SESSION_STREAM_HIDDEN_POLL_MAX_FALSE) { + _stopHiddenActiveStreamPoll(); + } + } } }) .catch(() => {}); @@ -6477,6 +6559,9 @@ function _startHiddenActiveStreamPoll(sid) { function _stopHiddenActiveStreamPoll() { if (_sessionStreamHiddenPollTimer) { clearInterval(_sessionStreamHiddenPollTimer); _sessionStreamHiddenPollTimer = null; } _sessionStreamHiddenPollSid = null; + // Reset the bounded-retry budget so a fresh poll never inherits a stale count. + _sessionStreamHiddenPollFalseStreamId = null; + _sessionStreamHiddenPollFalseCount = 0; } function _chatStreamActiveForSession(sid) { diff --git a/tests/test_hidden_tab_server_initiated_turn.py b/tests/test_hidden_tab_server_initiated_turn.py index d2e455ae0..cad1e9590 100644 --- a/tests/test_hidden_tab_server_initiated_turn.py +++ b/tests/test_hidden_tab_server_initiated_turn.py @@ -89,7 +89,7 @@ def test_hidden_poll_hits_session_status_and_attaches_as_replay(): """ start = MESSAGES_JS.find("function _startHiddenActiveStreamPoll(sid)") assert start != -1 - body = MESSAGES_JS[start:start + 1400] + body = MESSAGES_JS[start:start + 2400] assert "api/session/status?session_id=" in body assert "d.active_stream_id" in body # attaches as replay (recovered=true) — turn is already mid-flight @@ -127,3 +127,59 @@ def test_hidden_poll_stops_on_session_teardown(): assert stop_idx != -1 block = MESSAGES_JS[stop_idx:stop_idx + 400] assert "_stopHiddenActiveStreamPoll()" in block + + +# ── Multi-pane: attach returns bool; poll only stops on a real attach ────── + +def test_attach_returns_bool_and_bails_false_on_non_current_pane(): + """_attachServerInitiatedStream must signal success/failure so the poll can + decide whether to keep trying. In the multi-pane edge where the active pane + is a DIFFERENT session, it must NOT attach to that pane's UI and must return + false (so the poll keeps retrying for the right pane) — never a bare + `return;` that the caller can't distinguish from success. + """ + fn_idx = MESSAGES_JS.find("function _attachServerInitiatedStream(sid, streamId, recovered)") + assert fn_idx != -1, "_attachServerInitiatedStream signature not found" + body = MESSAGES_JS[fn_idx:fn_idx + 4000] + # Non-current pane bails with an explicit false, not a bare return. + assert "if (!isCurrent) return false;" in body + # Bad/empty args also bail false; success paths return true. + assert "if (!streamId) return false;" in body + assert "return true;" in body + # The catch returns false so a thrown attach keeps the poll alive. + catch_idx = body.find("catch (_)") + assert catch_idx != -1, "function must keep its try/catch" + catch_body = body[catch_idx:] + assert "return false;" in catch_body + # Gate must-fix #1: on a mid-setup throw the catch must CLEAR the partial + # state it set before the DOM calls (S.busy / S.activeStreamId / + # S.session.active_stream_id), guarded to only clear when this pane still owns + # the stream — otherwise the next poll tick sees a stale activeStreamId, exits + # early as "already attached", and wedges the turn invisible + composer busy. + assert "S.activeStreamId = null;" in catch_body + assert "S.busy = false;" in catch_body + # And a post-handoff failure (after attachLiveStream took the stream) must NOT + # be reported as failure — the stream is already in good hands. + assert "handedOff" in body + assert "if (handedOff) return true;" in catch_body + + +def test_poll_stops_only_when_attach_succeeds(): + """The hidden poll must gate _stopHiddenActiveStreamPoll on the attach + return value — stopping unconditionally would, in the multi-pane edge, + cancel the poll while the turn was never attached (renders only on next + interaction). So: capture the bool, stop ONLY when true. + """ + start = MESSAGES_JS.find("function _startHiddenActiveStreamPoll(sid)") + assert start != -1, "_startHiddenActiveStreamPoll signature not found" + # Window 2400: the multi-pane follow-up adds an explanatory comment block + # before the attach call, pushing it past a narrower slice. + body = MESSAGES_JS[start:start + 3200] + assert "const attached = _attachServerInitiatedStream(sid, streamId, true)" in body + # Stop the poll only on a true attach (the false branch keeps polling within + # the bounded-retry budget rather than stopping). + assert "if (attached) {" in body + assert "_stopHiddenActiveStreamPoll();" in body + # The bounded-retry give-up: a never-current pane stops after the budget. + assert "_sessionStreamHiddenPollFalseCount" in body + assert "_SESSION_STREAM_HIDDEN_POLL_MAX_FALSE" in body