diff --git a/.github/workflows/conversation-lifecycle.yml b/.github/workflows/conversation-lifecycle.yml index 6f49c3c8..24ede84b 100644 --- a/.github/workflows/conversation-lifecycle.yml +++ b/.github/workflows/conversation-lifecycle.yml @@ -12,6 +12,7 @@ on: - 'api/**.py' - 'server.py' - 'tests/browser_conversation_lifecycle.py' + - 'tests/sidebar_terminal_barrier_server.py' - 'tests/browser_historical_transcript_hydration.py' - 'tests/browser_reconnect_scene_redraw.py' - 'requirements*.txt' @@ -24,6 +25,7 @@ on: - 'api/**.py' - 'server.py' - 'tests/browser_conversation_lifecycle.py' + - 'tests/sidebar_terminal_barrier_server.py' - 'tests/browser_historical_transcript_hydration.py' - 'tests/browser_reconnect_scene_redraw.py' - 'requirements*.txt' @@ -86,6 +88,14 @@ jobs: LIFECYCLE_TEST_BITE: settle-worklog-frame-proof run: python tests/browser_conversation_lifecycle.py + - name: Recover a missing terminal frame + if: matrix.scenario == 'normal' + env: + LIFECYCLE_ARTIFACT_DIR: lifecycle-artifacts/missing-terminal + LIFECYCLE_SCENARIO: normal + LIFECYCLE_TEST_BITE: sidebar-idle-missing-terminal + run: python tests/browser_conversation_lifecycle.py + - name: Upload failure artifacts if: failure() uses: actions/upload-artifact@v4 diff --git a/docs/pr-media/6469-sidebar-handoff/README.md b/docs/pr-media/6469-sidebar-handoff/README.md new file mode 100644 index 00000000..d458093d --- /dev/null +++ b/docs/pr-media/6469-sidebar-handoff/README.md @@ -0,0 +1,91 @@ +# Sidebar idle / terminal handoff evidence + +## Failure and scope + +The independent `/api/sessions` response can report idle before the chat SSE's +terminal frame reaches the page. Three sidebar paths were retiring chat state: +idle reconciliation, INFLIGHT purging, and optimistic-row retirement. The idle +path clears the Worklog and active stream ID; the subsequent `done` is rejected +as stale, so no settled Anchor scene is persisted. + +A real-HTTP barrier reproduces this on both published PR head +`75c8ff8d2a991f22389e3e4263ee03fb8d8f67a0` and frozen upstream +`b09fded7b8c98b549a49aaaece3721015337d307`. Applying only the `sessions.js` +product fix to the frozen upstream also passes the identical oracle: this is a +shared lifecycle defect, not evidence that artifact replay introduced it. The +historical hosted run lacks this instrumentation, so its exact interleaving +cannot be proved retrospectively; its empty-surface/no-persistence signature +matches the controlled reproduction. + +## Reproduction + +```sh +LIFECYCLE_SCENARIO=normal LIFECYCLE_TEST_BITE=settle-worklog-frame-proof \ + .venv/bin/python tests/browser_conversation_lifecycle.py +./scripts/test.sh tests/test_sidebar_terminal_handoff.py -q +``` + +The existing frame proof now uses a test-only server wrapper to hold the actual +`done` HTTP write. The real worker finishes; the real browser receives an idle +sidebar response through `renderSessionList()` while its native EventSource +remains open. The test verifies pane, INFLIGHT and registry ownership, then +releases `done` and retains all existing painted-frame, final-answer, semantic +activity and hard-reload assertions. The fixture always releases/cleans up and +has a bounded wait. Normal production server startup is unchanged. + +No renderer, terminal payload or persistence response is fabricated. Runtime +events come from the existing deterministic localhost Gateway fixture, not a +live model/provider. All state and files are disposable. These are browser +viewport checks, not physical-device acceptance. + +## Before / after + +Each screenshot is taken after the idle sidebar response, before the terminal +HTTP frame is released. The exact published source fails at both widths. The +patched source preserves three live activity rows and then passes completion +and hard reload. `results.json` records the predicate results without machine +paths or credentials. + +| Viewport | Before | After | +| --- | --- | --- | +| 1440 x 900 | [Empty surface](before-1440.png) | [Owned handoff retained](after-1440.png) | +| 390 x 844 | [Empty surface](before-390.png) | [Owned handoff retained](after-390.png) | + +The fix only defers these sidebar cleanups for the current pane's exact OPEN +chat transport. Missing, CONNECTING, CLOSED and mismatched sources still take +the existing stale recovery path. + +## Missing terminal frame: bounded recovery + +The OPEN-only guard at `85f1c2d34b62b0ade85f51ed2cf8c1cf56f5ab9e` could wait +forever when the terminal frame never arrived. The follow-up keeps the live +handoff for 1.5 seconds, then probes the exact stream through +`/api/chat/stream/status` with an eight-second timeout and no retries. +Repeated sidebar hints cannot restart the deadline or create concurrent +requests. `active:true` keeps the OPEN owner intact even when the persisted +session fields already look idle; only explicit runtime inactivity permits the +canonical session restore. A failed or malformed runtime probe uses the +existing interrupted-stream path rather than claiming successful settlement. +Normal terminal delivery, transport replacement/close, navigation, and a newer +optimistic turn invalidate the old request. Already-running stream-end recovery +remains the sole recovery owner. + +```sh +LIFECYCLE_SCENARIO=normal LIFECYCLE_TEST_BITE=sidebar-idle-missing-terminal \ + .venv/bin/python tests/browser_conversation_lifecycle.py +./scripts/test.sh tests/test_sidebar_idle_stream_recovery.py -q +``` + +The new browser row keeps the real terminal HTTP barrier closed until cleanup; +the client must recover without receiving `done` or `stream_end`. Published +`85f1c2d34b` fails the 11-second completion bound with `S.busy` still true. +The patched implementation restores the final answer and passes the unchanged +painted-frame, semantic activity, persistence and hard-reload checks before the +barrier is released. The cloud workflow runs both delayed- and missing-terminal +rows. No live provider or production state is used. + +A production Gateway regression also blocks post-turn goal evaluation after +the success transcript has already cleared `active_stream_id` and pending +fields. During that interval the worker and exact entry in `STREAMS` remain +live and no `done` event has been emitted, proving why persisted session flags +cannot authorize sidebar settlement on their own. diff --git a/docs/pr-media/6469-sidebar-handoff/after-1440.png b/docs/pr-media/6469-sidebar-handoff/after-1440.png new file mode 100644 index 00000000..e28d30be Binary files /dev/null and b/docs/pr-media/6469-sidebar-handoff/after-1440.png differ diff --git a/docs/pr-media/6469-sidebar-handoff/after-390.png b/docs/pr-media/6469-sidebar-handoff/after-390.png new file mode 100644 index 00000000..f876ed24 Binary files /dev/null and b/docs/pr-media/6469-sidebar-handoff/after-390.png differ diff --git a/docs/pr-media/6469-sidebar-handoff/before-1440.png b/docs/pr-media/6469-sidebar-handoff/before-1440.png new file mode 100644 index 00000000..87953ff1 Binary files /dev/null and b/docs/pr-media/6469-sidebar-handoff/before-1440.png differ diff --git a/docs/pr-media/6469-sidebar-handoff/before-390.png b/docs/pr-media/6469-sidebar-handoff/before-390.png new file mode 100644 index 00000000..20c745cb Binary files /dev/null and b/docs/pr-media/6469-sidebar-handoff/before-390.png differ diff --git a/docs/pr-media/6469-sidebar-handoff/results.json b/docs/pr-media/6469-sidebar-handoff/results.json new file mode 100644 index 00000000..c1564591 --- /dev/null +++ b/docs/pr-media/6469-sidebar-handoff/results.json @@ -0,0 +1,66 @@ +[ + { + "mode": "before", + "viewport": [ + 1440, + 900 + ], + "exit": 1, + "observation": { + "serverIdle": true, + "paneRetained": false, + "inflightRetained": false, + "sourceRetained": true, + "registryRetained": true, + "liveRows": 0 + } + }, + { + "mode": "before", + "viewport": [ + 390, + 844 + ], + "exit": 1, + "observation": { + "serverIdle": true, + "paneRetained": false, + "inflightRetained": false, + "sourceRetained": true, + "registryRetained": true, + "liveRows": 0 + } + }, + { + "mode": "after", + "viewport": [ + 1440, + 900 + ], + "exit": 0, + "observation": { + "serverIdle": true, + "paneRetained": true, + "inflightRetained": true, + "sourceRetained": true, + "registryRetained": true, + "liveRows": 3 + } + }, + { + "mode": "after", + "viewport": [ + 390, + 844 + ], + "exit": 0, + "observation": { + "serverIdle": true, + "paneRetained": true, + "inflightRetained": true, + "sourceRetained": true, + "registryRetained": true, + "liveRows": 3 + } + } +] diff --git a/docs/rfcs/live-to-final-assistant-replies.md b/docs/rfcs/live-to-final-assistant-replies.md index 6564390e..60e762c6 100644 --- a/docs/rfcs/live-to-final-assistant-replies.md +++ b/docs/rfcs/live-to-final-assistant-replies.md @@ -470,6 +470,19 @@ Expected behavior: - A session row's running indicator should reflect a real active run or a clearly restorable state, not stale persisted metadata alone. +- An idle sidebar response does not prove that the browser has consumed the + terminal chat frame. While the current pane still owns its exact OPEN chat + transport, list reconciliation and cache purging must preserve that handoff. + Missing, connecting, closed, or mismatched transports keep the normal stale + state recovery path; a cached stream ID or busy flag is insufficient. + OPEN is not proof that a terminal frame will arrive: an idle hint must also + schedule bounded recovery without erasing the live scene. Before reading a + settled session snapshot, that recovery must check the exact stream's runtime + status. A runtime-active stream retains ownership even when persisted + `active_stream_id` and pending fields are already clear; only explicit + runtime inactivity permits canonical snapshot settlement. Probe failure is a + degraded/interrupted state, not evidence of successful completion. A newer + turn, replacement source, or received terminal event invalidates the request. - Background completion, cancellation, or failure should be represented without stealing the visible pane from the user. - Session switching should not erase pending live context, in-flight snapshots, diff --git a/static/messages.js b/static/messages.js index dac891d6..45cc458f 100644 --- a/static/messages.js +++ b/static/messages.js @@ -2089,6 +2089,7 @@ function closeLiveStream(sessionId, streamId, source){ if(!live) return; if(streamId&&live.streamId!==streamId) return; if(source&&live.source!==source) return; + if(typeof live.cancelIdleRecovery==='function') live.cancelIdleRecovery(); // Snapshot the current live-turn DOM BEFORE tearing the stream down. The // per-event snapshot (snapshotLiveTurn) only fires on content/tool_complete // SSE events, so switching away during a quiet window (mid tool-exec, silent @@ -5745,12 +5746,82 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ return true; } + function _bindSidebarIdleRecovery(live){ + let pending=null; + live.cancelIdleRecovery=()=>{ + if(pending&&pending.timer) clearTimeout(pending.timer); + pending=null; + }; + live.recoverFromSidebarIdle=()=>{ + if(pending||_streamFinalized||_terminalStateReached||_pendingStreamEndRecovery) return; + const request={inflight:INFLIGHT[activeSid],timer:null}; + const isCurrent=()=>pending===request&&!_streamFinalized&&!_terminalStateReached&& + LIVE_STREAMS[activeSid]===live&&live.source.readyState===1&& + S.session&&S.session.session_id===activeSid&&S.activeStreamId===streamId&& + INFLIGHT[activeSid]===request.inflight&& + !(typeof _sendInProgress!=='undefined'&&_sendInProgress&&activeSid===_sendInProgressSid); + pending=request; + // Give the independently delivered terminal frame a short handoff window, + // then use canonical session recovery even if the transport stays OPEN. + // One ticket spans both timer and request; list refreshes cannot extend it. + request.timer=setTimeout(async()=>{ + request.timer=null; + if(!isCurrent()){ + if(pending===request) live.cancelIdleRecovery(); + return; + } + try{ + let runtimeStatus=null; + try{ + runtimeStatus=await api( + `/api/chat/stream/status?stream_id=${encodeURIComponent(streamId)}`, + {timeoutMs:8000,retries:0,timeoutToast:false} + ); + }catch(_){ + // Runtime ownership is authoritative here. If the probe itself + // fails, do not infer completion from already-cleared session + // fields; surface the existing interrupted-stream path instead. + if(isCurrent()) _handleStreamError(live.source); + return; + } + if(!isCurrent()) return; + if(!runtimeStatus||typeof runtimeStatus.active!=='boolean'){ + _handleStreamError(live.source); + return; + } + // Gateway success writeback clears persisted active/pending fields + // before post-turn goal evaluation and terminal event emission. + // STREAMS-backed status therefore owns this decision: an exact active + // runtime must keep its OPEN browser handoff even if the sidebar row + // already looks idle. + if(runtimeStatus.active) return; + const status=await _restoreSettledSession(live.source,{ + status:true, + isCurrent, + requestOptions:{timeoutMs:8000,retries:0,timeoutToast:false}, + preserveVisibleOnShorterTerminalSnapshot:true, + }); + // A stale sidebar response is not permission to terminate a worker + // which the authoritative session snapshot still reports as active. + if(isCurrent()&&status!=='restored'&&status!=='active') _handleStreamError(live.source); + }finally{ + if(pending===request) live.cancelIdleRecovery(); + } + },1500); + }; + for(const event of ['done','cancel','apperror','stream_end','error']){ + live.source.addEventListener(event,live.cancelIdleRecovery); + } + } + function _wireSSE(source){ const existingLive=LIVE_STREAMS[activeSid]; if(existingLive&&existingLive.source&&existingLive.source!==source){ + if(typeof existingLive.cancelIdleRecovery==='function') existingLive.cancelIdleRecovery(); try{if(existingLive.source.readyState!==2)existingLive.source.close();}catch(_){ } } LIVE_STREAMS[activeSid]={streamId,source}; + _bindSidebarIdleRecovery(LIVE_STREAMS[activeSid]); // Note on #631 Bug B: the original PR description stated the server // "replays buffered token events" on reconnect, and proposed resetting @@ -7024,13 +7095,16 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ async function _restoreSettledSession(source, options=null){ const returnStatus=!!(options&&options.status); + const isCurrent=options&&typeof options.isCurrent==='function'?options.isCurrent:null; + if(isCurrent&&!isCurrent()) return returnStatus?'stale':false; const preserveVisibleOnShorterTerminalSnapshot=!!(options&&options.preserveVisibleOnShorterTerminalSnapshot); if(_isActiveSession() && S.activeStreamId!==streamId){ _closeSource(source); return returnStatus?'stale':false; } try{ - const data=await api(`/api/session?session_id=${encodeURIComponent(activeSid)}`); + const data=await api(`/api/session?session_id=${encodeURIComponent(activeSid)}`,options&&options.requestOptions||{}); + if(isCurrent&&!isCurrent()) return returnStatus?'stale':false; // Opus #2852 race-fix: if a late `done` event ran the finalize path while // we were awaiting the network roundtrip, bail out — done already settled. if(_streamFinalized) return returnStatus?'restored':true; diff --git a/static/sessions.js b/static/sessions.js index dd6a6368..f748f45a 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -852,6 +852,14 @@ function _reconcileActiveSessionIdleStateFromList(serverRows) { const serverRow=serverRows.find(s=>s&&s.session_id===sid); if (!serverRow) return false; if (!_isServerIdleSessionRow(serverRow)) return false; + // Sidebar idle metadata can beat the terminal frame on the independent chat + // SSE. Let its exact OPEN transport finish the Anchor handoff; orphaned or + // disconnected streams still use the existing idle recovery below. + if (_hasOwnedOpenLiveStream(sid)) { + const live=LIVE_STREAMS[sid]; + if(typeof live.recoverFromSidebarIdle==='function') live.recoverFromSidebarIdle(); + return false; + } let changed=false; if (S.busy) { S.busy=false; changed=true; } if (S.activeStreamId) { S.activeStreamId=null; changed=true; } @@ -940,6 +948,8 @@ function _purgeStaleInflightEntries() { if (typeof _sendInProgress !== 'undefined' && _sendInProgress && sid === _sendInProgressSid) { continue; } + // The sidebar render must not purge what the idle reconciler preserved. + if (_hasOwnedOpenLiveStream(sid)) continue; if (!sessionsById.has(sid)) { const knownSource = sourceById ? sourceById.get(sid) : null; if (currentSidebarSource && (!knownSource || knownSource !== currentSidebarSource)) { @@ -961,6 +971,14 @@ function _purgeStaleInflightEntries() { } } +function _hasOwnedOpenLiveStream(sid) { + if (typeof S === 'undefined' || !S || !S.session || S.session.session_id !== sid || !S.activeStreamId) return false; + const live = typeof LIVE_STREAMS === 'object' && LIVE_STREAMS ? LIVE_STREAMS[sid] : null; + // readyState 1 is EventSource.OPEN. A cached stream ID or busy flag alone + // is not ownership and must never disable recovery for a stuck indicator. + return Boolean(live && live.streamId === S.activeStreamId && live.source && live.source.readyState === 1); +} + function _rememberSessionListSource(s, sid = null, allowScopeFallback = true) { const resolvedSid = sid || (s && s.session_id); if (!resolvedSid) return; @@ -5278,7 +5296,8 @@ function _shouldKeepLocalOnlyOptimisticSessionRow(local){ function _dropStaleOptimisticSessionRow(sid){ if(!sid) return; if(typeof _rememberSessionListSource==='function') _rememberSessionListSource(null, sid, false); - if(INFLIGHT&&INFLIGHT[sid]){ + // Retiring sidebar optimism must not retire the independent chat owner. + if(INFLIGHT&&INFLIGHT[sid]&&!_hasOwnedOpenLiveStream(sid)){ delete INFLIGHT[sid]; if(typeof clearInflightState==='function') clearInflightState(sid); } diff --git a/tests/browser_conversation_lifecycle.py b/tests/browser_conversation_lifecycle.py index cc9c8972..34c02521 100644 --- a/tests/browser_conversation_lifecycle.py +++ b/tests/browser_conversation_lifecycle.py @@ -39,6 +39,7 @@ SCENARIO = os.environ.get("LIFECYCLE_SCENARIO", "normal").strip() or "normal" TOOL_NAME = "read_file" TOOL_ID = "lifecycle-tool-1" TEST_BITE = os.environ.get("LIFECYCLE_TEST_BITE", "").strip() +_FRAME_PROOF_BITES = {"settle-worklog-frame-proof", "sidebar-idle-missing-terminal"} GATEWAY_ACTIVITY_TIMEOUT = 60.0 ANCHOR_SCENE_PERSIST_TIMEOUT = 60.0 ANCHOR_SCENE_PROJECTION_TIMEOUT = 10_000 @@ -267,8 +268,17 @@ def _start_webui_server(repo_root: Path, env: dict, artifact_dir: Path): suffix = "" if attempts == 1 else f"-attempt-{attempt + 1}" log_path = artifact_dir / f"server{suffix}.log" log = log_path.open("w", encoding="utf-8") + command = [sys.executable, str(repo_root / "server.py")] + barrier_dir = env.get("HERMES_TEST_TERMINAL_BARRIER_DIR") + if barrier_dir: + command = [ + sys.executable, + str(repo_root / "tests" / "sidebar_terminal_barrier_server.py"), + str(repo_root), + barrier_dir, + ] proc = subprocess.Popen( - [sys.executable, str(repo_root / "server.py")], + command, cwd=repo_root, env=run_env, stdout=log, @@ -286,6 +296,50 @@ def _start_webui_server(repo_root: Path, env: dict, artifact_dir: Path): raise RuntimeError(f"WebUI server did not become healthy{detail}") +def _exercise_sidebar_terminal_handoff(page, barrier_dir: Path, artifact_dir: Path): + """Order the real idle-list response before the real terminal SSE write.""" + deadline = time.monotonic() + 10 + while not (barrier_dir / "ready").exists(): + if time.monotonic() >= deadline: + raise AssertionError("terminal HTTP barrier was not reached") + page.wait_for_timeout(10) + observation = page.evaluate( + """async () => { + const sid = S.session.session_id; + const stream = S.activeStreamId; + const inflight = INFLIGHT[sid]; + const source = LIVE_STREAMS[sid] && LIVE_STREAMS[sid].source; + const registries = window._liveAnchorRegistries; + const registry = registries && registries.get(stream); + await renderSessionList(); + await new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(resolve))); + const live = document.querySelector('#liveAssistantTurn'); + return { + serverIdle: _allSessions.some(row => row.session_id === sid && _isServerIdleSessionRow(row)), + paneRetained: S.busy === true && S.activeStreamId === stream, + inflightRetained: !!inflight && INFLIGHT[sid] === inflight, + sourceRetained: !!source && source.readyState === EventSource.OPEN && + LIVE_STREAMS[sid] && LIVE_STREAMS[sid].source === source, + registryRetained: !!registry && registries.get(stream) === registry, + liveRows: live ? live.querySelectorAll('[data-anchor-scene-row="1"]').length : 0, + }; + }""" + ) + (artifact_dir / "sidebar-before-terminal.json").write_text( + json.dumps(observation, indent=2), encoding="utf-8" + ) + page.screenshot(path=str(artifact_dir / "sidebar-before-terminal.png"), full_page=True) + for invariant in ("serverIdle", "paneRetained", "inflightRetained", "sourceRetained", "registryRetained"): + assert observation[invariant] is True, { + "message": "idle sidebar response revoked the live terminal handoff", + "invariant": invariant, + "observation": observation, + } + assert observation["liveRows"] > 0, observation + if TEST_BITE != "sidebar-idle-missing-terminal": + (barrier_dir / "release").touch() + + class DeterministicGateway: """A localhost-only Gateway Runs server with test-controlled phase gates.""" @@ -733,11 +787,12 @@ def main() -> int: "drop-anchor-persistence", "drop-terminal-anchor-row", "settle-worklog-frame-proof", + "sidebar-idle-missing-terminal", }: raise ValueError( f"Unsupported LIFECYCLE_TEST_BITE {TEST_BITE!r}; " "expected one of '', 'drop-anchor-persistence', " - "'drop-terminal-anchor-row', 'settle-worklog-frame-proof'" + "'drop-terminal-anchor-row', 'settle-worklog-frame-proof', 'sidebar-idle-missing-terminal'" ) if TEST_BITE == "drop-terminal-anchor-row" and scenario != "terminal-error": raise ValueError( @@ -782,6 +837,12 @@ def main() -> int: "NO_PROXY": "127.0.0.1,localhost", "no_proxy": "127.0.0.1,localhost", }) + env.pop("HERMES_TEST_TERMINAL_BARRIER_DIR", None) + terminal_barrier_dir = None + if TEST_BITE in _FRAME_PROOF_BITES and scenario == "normal": + terminal_barrier_dir = state_dir / "terminal-barrier" + terminal_barrier_dir.mkdir() + env["HERMES_TEST_TERMINAL_BARRIER_DIR"] = str(terminal_barrier_dir) proc = None log = None log_path = None @@ -873,7 +934,7 @@ def main() -> int: # must never expose an expanded empty Worklog shell or an activity-less # gap. Sample after rAF callbacks for a frame have drained; DOM mutations # within one task or earlier rAF callbacks are not user-visible frames. - if TEST_BITE == "settle-worklog-frame-proof": + if TEST_BITE in _FRAME_PROOF_BITES: page.evaluate( """() => { const samples = []; @@ -941,12 +1002,17 @@ def main() -> int: timeout=10000, ) gateway.release_terminal.set() + if terminal_barrier_dir is not None: + _exercise_sidebar_terminal_handoff(page, terminal_barrier_dir, artifact_dir) page.wait_for_function( """text => typeof S !== 'undefined' && S.busy === false && !S.activeStreamId && ((document.querySelector('#msgInner') || {}).innerText || '').includes(text)""", arg=FINAL_TEXT, - timeout=15000, + timeout=11000 if TEST_BITE == "sidebar-idle-missing-terminal" else 15000, ) + if TEST_BITE == "sidebar-idle-missing-terminal": + assert not (terminal_barrier_dir / "release").exists(), "terminal frame was unexpectedly released" + print("OK missing terminal: canonical snapshot settled the still-OPEN transport before terminal delivery") else: gateway.release_terminal.set() page.wait_for_function( @@ -958,7 +1024,7 @@ def main() -> int: ) session_id = page.evaluate("S.session && S.session.session_id") assert session_id, "active session id missing after settlement" - if TEST_BITE == "settle-worklog-frame-proof": + if TEST_BITE in _FRAME_PROOF_BITES: page.wait_for_timeout(100) settle_proof = page.evaluate( "window.__lifecycleSettleFrameProof && window.__lifecycleSettleFrameProof.stop()" @@ -1176,6 +1242,8 @@ def main() -> int: exit_code = 1 return 1 finally: + if terminal_barrier_dir is not None: + (terminal_barrier_dir / "release").touch() gateway.close() if browser is not None: browser.close() diff --git a/tests/sidebar_terminal_barrier_server.py b/tests/sidebar_terminal_barrier_server.py new file mode 100644 index 00000000..9c9dc137 --- /dev/null +++ b/tests/sidebar_terminal_barrier_server.py @@ -0,0 +1,40 @@ +"""Test-only server for the public settle-frame gate's cross-channel race. + +Hold the actual HTTP terminal frame after the worker has finished. The browser +keeps its native, open EventSource while /api/sessions independently reports idle. +No browser state, renderer, persistence result, or terminal payload is mocked. +""" + +from pathlib import Path +import sys +import time + + +def main(): + if len(sys.argv) != 3: + raise SystemExit("expected repository root and isolated barrier directory") + repo_root, barrier_dir = (Path(value) for value in sys.argv[1:]) + sys.path.insert(0, str(repo_root)) + import server + from api import routes + + ready = barrier_dir / "ready" + release = barrier_dir / "release" + original = routes._sse_with_id + + def terminal_barrier(handler, event, data, event_id=None): + if event == "done" and handler.path.startswith("/api/chat/stream?"): + ready.touch() + deadline = time.monotonic() + 15 + while not release.exists(): + if time.monotonic() >= deadline: + raise TimeoutError("test did not release the terminal HTTP frame") + time.sleep(0.005) + return original(handler, event, data, event_id) + + routes._sse_with_id = terminal_barrier + server.main() + + +if __name__ == "__main__": + main() diff --git a/tests/test_issue2454_active_session_spinner.py b/tests/test_issue2454_active_session_spinner.py index e9872996..a6cdb00d 100644 --- a/tests/test_issue2454_active_session_spinner.py +++ b/tests/test_issue2454_active_session_spinner.py @@ -183,6 +183,7 @@ def test_optimistic_merge_preserves_server_running_state_when_local_cache_is_sta optimistic_body = _function_body(SESSIONS_SRC, "function _isOptimisticFirstTurnSessionRow(") keep_body = _function_body(SESSIONS_SRC, "function _shouldKeepLocalOnlyOptimisticSessionRow(") drop_body = _function_body(SESSIONS_SRC, "function _dropStaleOptimisticSessionRow(") + owner_body = _function_body(SESSIONS_SRC, "function _hasOwnedOpenLiveStream(") merge_body = _function_body(SESSIONS_SRC, "function _mergeOptimisticFirstTurnSessions(") script = f""" @@ -206,6 +207,7 @@ function _isServerIdleSessionRow(s) {{{helper_body}}} function _isSessionLocallyStreaming(s) {{{local_body}}} function _isOptimisticFirstTurnSessionRow(s) {{{optimistic_body}}} function _shouldKeepLocalOnlyOptimisticSessionRow(local) {{{keep_body}}} +function _hasOwnedOpenLiveStream(sid) {{{owner_body}}} function _dropStaleOptimisticSessionRow(sid) {{{drop_body}}} function _mergeOptimisticFirstTurnSessions(fetchedSessions) {{{merge_body}}} const merged = _mergeOptimisticFirstTurnSessions([{{ diff --git a/tests/test_issue4766_sidebar_source_pushdown.py b/tests/test_issue4766_sidebar_source_pushdown.py index c1031174..ec238733 100644 --- a/tests/test_issue4766_sidebar_source_pushdown.py +++ b/tests/test_issue4766_sidebar_source_pushdown.py @@ -536,7 +536,7 @@ def test_source_filtered_cache_preserves_hidden_bucket_runtime_state(): remember_source_fn = _extract_function(src, "_rememberSessionListSource") remember_streaming_fn = _extract_function(src, "_rememberRenderedStreamingState") remember_snapshot_fn = _extract_function(src, "_rememberRenderedSessionSnapshot") - purge_fn = _extract_function(src, "_purgeStaleInflightEntries") + purge_fn = _extract_function(src, "_hasOwnedOpenLiveStream") + "\n" + _extract_function(src, "_purgeStaleInflightEntries") mark_fn = _extract_function(src, "_markPollingCompletionUnreadTransitions") script = f""" global._allSessions = [{{ @@ -654,7 +654,7 @@ def test_session_list_response_omits_bucket_counts_when_missing(monkeypatch): @pytest.mark.skipif(NODE is None, reason="node not on PATH") def test_scope_mismatch_error_path_respects_sidebar_source(): src = SESSIONS_JS.read_text(encoding="utf-8") - purge_fn = _extract_function(src, "_purgeStaleInflightEntries") + purge_fn = _extract_function(src, "_hasOwnedOpenLiveStream") + "\n" + _extract_function(src, "_purgeStaleInflightEntries") clear_fn = _extract_function(src, "_clearSessionSourceTabCounts") requested_source_fn = _extract_function(src, "_requestedSessionSidebarSource") exclude_hidden_fn = _extract_function(src, "_sessionListExcludeHiddenEnabled") diff --git a/tests/test_sidebar_idle_stream_recovery.py b/tests/test_sidebar_idle_stream_recovery.py new file mode 100644 index 00000000..7de907fa --- /dev/null +++ b/tests/test_sidebar_idle_stream_recovery.py @@ -0,0 +1,173 @@ +"""A sidebar-idle hint must retain the live handoff without waiting forever. + +Drive the production scheduling helper with a fake clock and a deferred snapshot +boundary. The browser lifecycle gate separately covers the real HTTP restore. +""" + +import json +from pathlib import Path +import shutil +import subprocess + +import pytest + +from tests.test_issue2454_active_session_spinner import _function_body + +ROOT = Path(__file__).resolve().parents[1] + + +def _run(case): + sessions = (ROOT / "static/sessions.js").read_text(encoding="utf-8") + messages = (ROOT / "static/messages.js").read_text(encoding="utf-8") + functions = "\n".join( + f"function {name}({params}) {{" + _function_body(sessions, f"function {name}(") + "}" + for name, params in [ + ("_isServerIdleSessionRow", "s"), + ("_hasOwnedOpenLiveStream", "sid"), + ("_reconcileActiveSessionIdleStateFromList", "serverRows"), + ] + ) + if "function _bindSidebarIdleRecovery(" in messages: + functions += "\nfunction _bindSidebarIdleRecovery(live) {" + _function_body( + messages, "function _bindSidebarIdleRecovery(" + ) + "}" + script = r""" +const assert=require('assert'); +const activeSid='current',streamId='turn-1'; +let _terminalStateReached=false,_streamFinalized=false; +let _pendingStreamEndRecovery=CASE==='stream-end-pending'; +const S={session:{session_id:activeSid},busy:true,activeStreamId:streamId}; +const INFLIGHT={current:{streamId}},original=INFLIGHT.current; +const timers=[],listeners={},calls=[],statusCalls=[],failures=[]; +function setTimeout(fn,delay){const t={fn,delay,cancelled:false};timers.push(t);return t;} +function clearTimeout(t){if(t)t.cancelled=true;} +const source={readyState:1,addEventListener(name,fn){(listeners[name]??=[]).push(fn);}}; +const live={streamId,source},LIVE_STREAMS={current:live}; +let resolveSnapshot; +async function api(path,options){ + statusCalls.push({path,options}); + if(CASE==='probe-error') throw new Error('status unavailable'); + return {active:CASE==='runtime-active'}; +} +async function _restoreSettledSession(source,options){ + calls.push({source,options}); + return new Promise(resolve=>{resolveSnapshot=resolve;}); +} +function _handleStreamError(s){failures.push(s);S.busy=false;S.activeStreamId=null;} +FUNCTIONS +if(typeof _bindSidebarIdleRecovery==='function')_bindSidebarIdleRecovery(live); +const idle=[{session_id:activeSid,is_streaming:false,active_stream_id:null}]; +(async()=>{ + _reconcileActiveSessionIdleStateFromList(idle); + _reconcileActiveSessionIdleStateFromList(idle); + if(CASE==='stream-end-pending'){assert.strictEqual(timers.length,0,'existing stream-end recovery must remain the sole owner');console.log('ok');return;} + assert.strictEqual(timers.length,1,'an OPEN transport must schedule one bounded snapshot recovery'); + assert.ok(timers[0].delay>0&&timers[0].delay<=2000); + assert.strictEqual(S.busy,true,'the first idle hint must not destroy the live handoff'); + const mode=CASE; + if(mode==='cancel-rearm'){ + live.cancelIdleRecovery(); + _reconcileActiveSessionIdleStateFromList(idle); + assert.strictEqual(timers.length,2); + await timers[0].fn(); + assert.strictEqual(timers[1].cancelled,false,'a queued old callback cannot cancel a replacement ticket'); + const replacement=timers[1].fn(); + await Promise.resolve(); + assert.strictEqual(calls.length,1); + resolveSnapshot('active');await replacement; + console.log('ok');return; + } + if(mode==='cancel-before'){ + live.cancelIdleRecovery(); + assert.strictEqual(timers[0].cancelled,true); + console.log('ok');return; + } + if(mode==='replace-before')LIVE_STREAMS.current={streamId,source:{readyState:1}}; + const running=timers[0].fn(); + if(mode==='replace-before'){ + await running;assert.strictEqual(calls.length,0);console.log('ok');return; + } + await Promise.resolve(); + assert.strictEqual(statusCalls.length,1); + assert.ok(statusCalls[0].path.includes('/api/chat/stream/status?stream_id=turn-1')); + assert.deepStrictEqual(statusCalls[0].options,{timeoutMs:8000,retries:0,timeoutToast:false}); + if(mode==='runtime-active'){ + await running; + assert.strictEqual(calls.length,0,'active runtime must not fall through to session restore'); + assert.strictEqual(failures.length,0); + assert.strictEqual(S.busy,true); + assert.strictEqual(INFLIGHT.current,original); + console.log('ok');return; + } + if(mode==='probe-error'){ + await running; + assert.strictEqual(calls.length,0,'failed runtime probe must not trust idle session flags'); + assert.strictEqual(failures.length,1); + console.log('ok');return; + } + assert.strictEqual(calls.length,1); + _reconcileActiveSessionIdleStateFromList(idle); + assert.strictEqual(timers.length,1,'repeated idle hints must not restart a pending deadline'); + const owns=calls[0].options.isCurrent; + assert.strictEqual(owns(),true); + assert.deepStrictEqual(calls[0].options.requestOptions,{timeoutMs:8000,retries:0,timeoutToast:false}); + if(mode==='replace-during')LIVE_STREAMS.current={streamId,source:{readyState:1}}; + if(mode==='new-turn')INFLIGHT.current={streamId:null}; + if(mode==='navigate')S.session={session_id:'other'}; + if(mode==='done'){ + for(const fn of listeners.done||[])fn(); + _streamFinalized=true;S.busy=false;S.activeStreamId=null; + } + if(['replace-during','new-turn','navigate','done'].includes(mode))assert.strictEqual(owns(),false); + resolveSnapshot(mode==='active'?'active':mode==='restored'?'restored':'error'); + await running; + assert.strictEqual(failures.length,mode==='error'?1:0); + assert.strictEqual(owns(),false,'settled requests must lose authority'); + if(mode==='active'){ + assert.strictEqual(S.busy,true);assert.strictEqual(INFLIGHT.current,original); + } + console.log('ok'); +})().catch(e=>{console.error(e.stack||e);process.exit(1);}); +""".replace("FUNCTIONS", functions).replace("CASE", json.dumps(case)) + node = shutil.which("node") + if not node: + pytest.skip("node required") + result = subprocess.run([node, "-e", script], capture_output=True, text=True, timeout=20) + assert result.returncode == 0, result.stdout + result.stderr + + +@pytest.mark.parametrize("case", [ + "error", "active", "restored", "runtime-active", "probe-error", "cancel-before", "replace-before", + "replace-during", "new-turn", "navigate", "done", "stream-end-pending", "cancel-rearm", +]) +def test_sidebar_idle_open_stream_has_bounded_owner_safe_recovery(case): + _run(case) + + +def test_snapshot_restore_rechecks_request_owner_after_await(): + source = (ROOT / "static/messages.js").read_text(encoding="utf-8") + body = _function_body(source, "async function _restoreSettledSession(") + script = r""" +const assert=require('assert'); +const activeSid='current',streamId='turn-1'; +let current=true,resolveSnapshot; +const S={session:{session_id:activeSid},activeStreamId:streamId}; +let _streamFinalized=false; +function _isActiveSession(){return true;} +function _closeSource(){throw Error('stale restore must not close a replacement');} +function api(){return new Promise(resolve=>{resolveSnapshot=resolve;});} +async function _restoreSettledSession(source,options=null){BODY} +(async()=>{ + const pending=_restoreSettledSession({}, {status:true,isCurrent:()=>current}); + current=false; + resolveSnapshot({session:{session_id:activeSid,active_stream_id:'newer-turn',messages:[]}}); + assert.strictEqual(await pending,'stale','request authority must be rechecked before using the response'); + assert.strictEqual(S.activeStreamId,streamId); + console.log('ok'); +})().catch(e=>{console.error(e.stack||e);process.exit(1);}); +""".replace("BODY", body) + node = shutil.which("node") + if not node: + pytest.skip("node required") + result = subprocess.run([node, "-e", script], capture_output=True, text=True, timeout=20) + assert result.returncode == 0, result.stdout + result.stderr diff --git a/tests/test_sidebar_terminal_handoff.py b/tests/test_sidebar_terminal_handoff.py new file mode 100644 index 00000000..506df8c6 --- /dev/null +++ b/tests/test_sidebar_terminal_handoff.py @@ -0,0 +1,96 @@ +"""Sidebar metadata must not retire a still-open, exact chat transport. + +The live SSE and /api/sessions responses are independently scheduled. An idle +sidebar row can arrive before the terminal frame carrying the Anchor scene. +""" + +import json +from pathlib import Path +import shutil +import subprocess + +import pytest + +from tests.test_issue2454_active_session_spinner import _function_body + +ROOT = Path(__file__).resolve().parents[1] + + +def _run_case(operation, transport, *, absent=False, close_after=False): + source = (ROOT / "static/sessions.js").read_text(encoding="utf-8") + names = ["_isServerIdleSessionRow", "_reconcileActiveSessionIdleStateFromList", "_purgeStaleInflightEntries", "_dropStaleOptimisticSessionRow"] + if "function _hasOwnedOpenLiveStream(" in source: + names.append("_hasOwnedOpenLiveStream") + parameters = { + "_isServerIdleSessionRow": "s", + "_reconcileActiveSessionIdleStateFromList": "serverRows", + "_hasOwnedOpenLiveStream": "sid", + "_dropStaleOptimisticSessionRow": "sid", + } + functions = "\n".join( + f"function {name}({parameters.get(name, '')}) {{" + + _function_body(source, f"function {name}(") + "}" + for name in names + ) + script = """ +const S={session:{session_id:'current',active_stream_id:'turn-1'},busy:true,activeStreamId:'turn-1'}; +const INFLIGHT={current:{streamId:'turn-1',messages:[{role:'assistant',content:'work so far'}]}}; +const original=INFLIGHT.current; +const LIVE_STREAMS={current:TRANSPORT}; +const _sessionStreamingById=new Map(); +const calls=[]; +const idle={session_id:'current',is_streaming:false,active_stream_id:null,pending_user_message:null}; +const _allSessions=ABSENT?[]:[idle]; +function clearInflightState(){calls.push('clear');} +function _forgetObservedStreamingSession(){} +function hideApprovalCard(){} +function hideLiveRunStatus(){} +function clearLiveToolCards(){calls.push('clear-dom');} +function updateSendBtn(){} +function _scheduleActiveSessionIdleReload(){calls.push('reload');} +FUNCTIONS +function run(){if(OPERATION==='idle')_reconcileActiveSessionIdleStateFromList([idle]);else if(OPERATION==='optimistic')_dropStaleOptimisticSessionRow('current');else _purgeStaleInflightEntries();} +run(); +const beforeClose={busy:S.busy,stream:S.activeStreamId,retained:INFLIGHT.current===original,calls:[...calls]}; +if(CLOSE_AFTER){LIVE_STREAMS.current.source.readyState=2;run();} +console.log(JSON.stringify({beforeClose,afterClose:{retained:!!INFLIGHT.current,busy:S.busy}})); +""" + script = script.replace("TRANSPORT", json.dumps(transport)).replace("ABSENT", json.dumps(absent)) + script = script.replace("FUNCTIONS", functions).replace("OPERATION", json.dumps(operation)) + script = script.replace("CLOSE_AFTER", json.dumps(close_after)) + node = shutil.which("node") + if not node: + pytest.skip("node required") + result = subprocess.run([node, "-e", script], text=True, capture_output=True, timeout=20) + assert result.returncode == 0, result.stderr + return json.loads(result.stdout) + + +@pytest.mark.parametrize("operation", ["idle", "purge", "optimistic"]) +def test_current_open_transport_keeps_terminal_handoff_and_recovers_after_close(operation): + result = _run_case(operation, {"streamId": "turn-1", "source": {"readyState": 1}}, close_after=True) + assert result["beforeClose"] == {"busy": True, "stream": "turn-1", "retained": True, "calls": []} + assert result["afterClose"]["retained"] is False + if operation == "idle": + assert result["afterClose"]["busy"] is False + + +@pytest.mark.parametrize("operation", ["idle", "purge", "optimistic"]) +@pytest.mark.parametrize("transport", [ + None, + {"streamId": "turn-1", "source": None}, + {"streamId": "turn-1", "source": {"readyState": 0}}, + {"streamId": "turn-1", "source": {"readyState": 2}}, + {"streamId": "older-turn", "source": {"readyState": 1}}, +]) +def test_missing_closed_connecting_or_wrong_transport_does_not_block_idle_recovery(operation, transport): + result = _run_case(operation, transport) + assert result["beforeClose"]["retained"] is False + if operation == "idle": + assert result["beforeClose"]["busy"] is False + assert "reload" in result["beforeClose"]["calls"] + + +def test_current_open_transport_survives_a_filtered_sidebar_list(): + result = _run_case("purge", {"streamId": "turn-1", "source": {"readyState": 1}}, absent=True) + assert result["beforeClose"]["retained"] is True diff --git a/tests/test_webui_gateway_chat_backend.py b/tests/test_webui_gateway_chat_backend.py index 0e7d2830..226fee6c 100644 --- a/tests/test_webui_gateway_chat_backend.py +++ b/tests/test_webui_gateway_chat_backend.py @@ -4,6 +4,7 @@ from email.message import Message import json from pathlib import Path import re +import threading import time import urllib.error @@ -858,6 +859,104 @@ def test_gateway_chat_worker_emits_goal_continue_for_goal_related_turn(tmp_path, assert saved.messages[-1]["content"] == "goal reply" +def test_gateway_goal_eval_keeps_runtime_active_after_success_writeback_before_done( + tmp_path, monkeypatch +): + """Persisted idle flags can precede terminal emission while STREAMS is still active.""" + session_dir = tmp_path / "sessions" + session_dir.mkdir() + monkeypatch.setattr(models, "SESSION_DIR", session_dir) + monkeypatch.setattr(models, "SESSION_INDEX_FILE", session_dir / "_index.json") + monkeypatch.setattr(models, "SESSIONS", OrderedDict()) + + class FakeResponse: + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def __iter__(self): + yield b'data: {"choices":[{"delta":{"content":"goal reply"}}]}\n\n' + yield b"data: [DONE]\n\n" + + monkeypatch.setenv("HERMES_WEBUI_GATEWAY_BASE_URL", "http://gateway.local") + monkeypatch.setattr( + gateway_chat.urllib.request, "urlopen", lambda req, timeout=0: FakeResponse() + ) + + from api import goals as webui_goals + + goal_entered = threading.Event() + release_goal = threading.Event() + observed = {} + + monkeypatch.setattr(webui_goals, "has_active_goal", lambda *args, **kwargs: True) + + def blocked_goal_eval(*args, **kwargs): + saved = models.get_session(s.session_id) + observed["active_stream_id"] = saved.active_stream_id + observed["pending_user_message"] = saved.pending_user_message + observed["stream_registered"] = stream_id in STREAMS + goal_entered.set() + assert release_goal.wait(timeout=10) + return {"should_continue": False, "message": "Goal checked"} + + monkeypatch.setattr(webui_goals, "evaluate_goal_after_turn", blocked_goal_eval) + + s = new_session() + stream_id = "stream-gateway-goal-runtime-window" + s.active_stream_id = stream_id + s.pending_user_message = "finish it" + s.pending_attachments = [] + s.pending_started_at = 123 + s.save() + channel = create_stream_channel() + subscriber = channel.subscribe() + STREAMS[stream_id] = channel + + worker = threading.Thread( + target=gateway_chat._run_gateway_chat_streaming, + args=( + s.session_id, + "finish it", + "test-model", + str(tmp_path), + stream_id, + [], + ), + kwargs={"goal_related": True}, + daemon=True, + ) + worker.start() + assert goal_entered.wait(timeout=10), "worker did not reach post-writeback goal evaluation" + + # This is the exact authority split the browser recovery must respect: + # persisted session metadata already looks idle, while the runtime stream + # (and therefore /api/chat/stream/status) is still active and can emit + # goal / goal_continue / done / stream_end. + assert observed == { + "active_stream_id": None, + "pending_user_message": None, + "stream_registered": True, + } + assert stream_id in STREAMS + assert worker.is_alive() + assert not any(item[0] == "done" for item in list(subscriber.queue)) + + release_goal.set() + worker.join(timeout=10) + assert not worker.is_alive() + events = [] + while not subscriber.empty(): + events.append(subscriber.get_nowait()) + names = [item[0] for item in events] + assert "goal" in names + assert "done" in names + assert "stream_end" in names + assert names.index("goal") < names.index("done") < names.index("stream_end") + + def test_gateway_chat_worker_skips_goal_judge_for_non_goal_turn(tmp_path, monkeypatch): session_dir = tmp_path / "sessions" session_dir.mkdir()