From ca2153ec54ef600abbc6eef2101939324b6a154d Mon Sep 17 00:00:00 2001 From: Frank Song Date: Sun, 20 Sep 2026 18:11:37 +0800 Subject: [PATCH] fix: bound sidebar idle recovery for missing terminal events (cherry picked from commit 1be0e114b32eefe14f94837ed3cf78aadc14ca9b) --- .github/workflows/conversation-lifecycle.yml | 10 ++ docs/pr-media/6469-sidebar-handoff/README.md | 30 +++- docs/rfcs/live-to-final-assistant-replies.md | 4 + static/messages.js | 52 ++++++- static/sessions.js | 6 +- tests/browser_conversation_lifecycle.py | 18 ++- tests/test_sidebar_idle_stream_recovery.py | 149 +++++++++++++++++++ 7 files changed, 258 insertions(+), 11 deletions(-) create mode 100644 tests/test_sidebar_idle_stream_recovery.py 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 index c7df4cbc..341e6a5f 100644 --- a/docs/pr-media/6469-sidebar-handoff/README.md +++ b/docs/pr-media/6469-sidebar-handoff/README.md @@ -53,6 +53,30 @@ paths or credentials. 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. It does not add a separate recovery timer or -claim to redesign transport liveness; prolonged network failure remains the -responsibility of the existing stream error/recovery machinery. +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 uses the existing canonical session restore with +an eight-second request timeout and no retries. Repeated sidebar hints cannot +restart the deadline or create concurrent requests. An active server snapshot +preserves the run; a failed or timed-out request uses existing interrupted-stream +recovery rather than claiming success. 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. diff --git a/docs/rfcs/live-to-final-assistant-replies.md b/docs/rfcs/live-to-final-assistant-replies.md index 4094906c..10599623 100644 --- a/docs/rfcs/live-to-final-assistant-replies.md +++ b/docs/rfcs/live-to-final-assistant-replies.md @@ -475,6 +475,10 @@ Expected behavior: 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 canonical snapshot recovery without erasing the live scene. + A newer turn, replacement source, or received terminal event invalidates that + request; a still-active server snapshot is not permission to terminate work. - 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..9423fb55 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,58 @@ 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{ + 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 +7071,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 401c37e6..f748f45a 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -855,7 +855,11 @@ function _reconcileActiveSessionIdleStateFromList(serverRows) { // 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)) return false; + 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; } diff --git a/tests/browser_conversation_lifecycle.py b/tests/browser_conversation_lifecycle.py index 59dcb473..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 @@ -335,7 +336,8 @@ def _exercise_sidebar_terminal_handoff(page, barrier_dir: Path, artifact_dir: Pa "observation": observation, } assert observation["liveRows"] > 0, observation - (barrier_dir / "release").touch() + if TEST_BITE != "sidebar-idle-missing-terminal": + (barrier_dir / "release").touch() class DeterministicGateway: @@ -785,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( @@ -836,7 +839,7 @@ def main() -> int: }) env.pop("HERMES_TEST_TERMINAL_BARRIER_DIR", None) terminal_barrier_dir = None - if TEST_BITE == "settle-worklog-frame-proof" and scenario == "normal": + 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) @@ -931,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 = []; @@ -1005,8 +1008,11 @@ def main() -> int: """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( @@ -1018,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()" diff --git a/tests/test_sidebar_idle_stream_recovery.py b/tests/test_sidebar_idle_stream_recovery.py new file mode 100644 index 00000000..eea5abd0 --- /dev/null +++ b/tests/test_sidebar_idle_stream_recovery.py @@ -0,0 +1,149 @@ +"""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=[],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 _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(); + 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; + } + 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", "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