mirror of
https://github.com/nesquena/hermes-webui.git
synced 2026-07-12 18:50:35 +00:00
fix(webui): hidden-tab poll keeps retrying when attach defers in multi-pane (#5172 follow-up)
#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.
This commit is contained in:
+91
-6
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user