From 98a6f88ef7f8e038132cdca526407afa1958dcad Mon Sep 17 00:00:00 2001 From: Dennis Soong Date: Thu, 7 May 2026 05:56:17 +0800 Subject: [PATCH 1/2] fix: scope terminal stream cleanup to owner session --- static/messages.js | 99 +++++++++++-------- tests/test_1694_terminal_cleanup_ownership.py | 93 +++++++++++++++++ ...t_issue856_background_completion_unread.py | 8 +- tests/test_regressions.py | 9 +- ...st_session_runtime_ownership_invariants.py | 26 +++-- tests/test_sprint30.py | 6 +- tests/test_sprint36.py | 13 ++- 7 files changed, 198 insertions(+), 56 deletions(-) create mode 100644 tests/test_1694_terminal_cleanup_ownership.py diff --git a/static/messages.js b/static/messages.js index 44262a85..8756fb93 100644 --- a/static/messages.js +++ b/static/messages.js @@ -357,6 +357,37 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ function _isActiveSession(){ return !!(S.session&&S.session.session_id===activeSid); } + function _clearActivePaneInflightIfOwner(){ + if(_isActiveSession()) clearInflight(); + } + function _approvalBelongsToOwner(){ + return _approvalSessionId===activeSid||(!_approvalSessionId&&_isActiveSession()); + } + function _clarifyBelongsToOwner(){ + return _clarifySessionId===activeSid||(!_clarifySessionId&&_isActiveSession()); + } + function _clearApprovalForOwner(){ + if(!_approvalBelongsToOwner()) return; + stopApprovalPolling(); + hideApprovalCard(true); + } + function _clearClarifyForOwner(reason){ + if(!_clarifyBelongsToOwner()) return; + stopClarifyPolling(); + hideClarifyCard(true, reason||'terminal'); + } + function _clearOwnerInflightState(){ + delete INFLIGHT[activeSid]; + clearInflightState(activeSid); + _clearActivePaneInflightIfOwner(); + } + function _setActivePaneIdleIfOwner(){ + if(_isActiveSession()||!S.session){ + setBusy(false); + setComposerStatus(''); + if(typeof setStatus==='function') setStatus(''); + } + } function persistInflightState(){ const inflight=INFLIGHT[activeSid]; if(!inflight||typeof saveInflightState!=='function') return; @@ -852,15 +883,12 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ if(!isSessionViewed && typeof _markSessionCompletionUnread==='function'){ _markSessionCompletionUnread(completedSid, completedSession.message_count); } - delete INFLIGHT[activeSid]; - clearInflight();clearInflightState(activeSid); + _clearOwnerInflightState(); if(typeof _markSessionCompletedInList==='function'){ _markSessionCompletedInList(completedSession, activeSid); } - stopApprovalPollingForSession(activeSid); - stopClarifyPollingForSession(activeSid); - if(!_approvalSessionId || _approvalSessionId===activeSid) hideApprovalCard(true); - if(!_clarifySessionId || _clarifySessionId===activeSid) hideClarifyCard(true, 'terminal'); + _clearApprovalForOwner(); + _clearClarifyForOwner('terminal'); if(isActiveSession){ S.activeStreamId=null; } @@ -944,13 +972,9 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ // TTS auto-read: speak the last assistant response if enabled (#499) if(typeof autoReadLastAssistant==='function') setTimeout(()=>autoReadLastAssistant(), 300); } + if(isActiveSession) _queueDrainSid=activeSid; renderSessionList(); - if(isActiveSession||!S.session||!INFLIGHT[S.session.session_id]){ - _queueDrainSid=activeSid; - setBusy(false); - setStatus(''); - setComposerStatus(''); - } + _setActivePaneIdleIfOwner(); playNotificationSound(); sendBrowserNotification('Response complete',assistantText?assistantText.slice(0,100):'Task finished'); }); @@ -1031,9 +1055,9 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ // Application-level error sent explicitly by the server (rate limit, crash, etc.) // This is distinct from the SSE network 'error' event below. source.close(); - delete INFLIGHT[activeSid];clearInflight();clearInflightState(activeSid);stopApprovalPollingForSession(activeSid);stopClarifyPollingForSession(activeSid); - if(!_approvalSessionId||_approvalSessionId===activeSid) hideApprovalCard(true); - if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true, 'terminal'); + _clearOwnerInflightState(); + _clearApprovalForOwner(); + _clearClarifyForOwner('terminal'); if(S.session&&S.session.session_id===activeSid){ S.activeStreamId=null; clearLiveToolCards();if(!assistantText)removeThinking(); @@ -1057,7 +1081,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ try{const d=JSON.parse(e.data);trackBackgroundError(activeSid,_errTitle,d.message||'Error');} catch(_){trackBackgroundError(activeSid,_errTitle,'Error');} } - if(!S.session||!INFLIGHT[S.session.session_id]){setBusy(false);setComposerStatus('');} + _setActivePaneIdleIfOwner(); renderSessionList(); // clear streaming indicator immediately on apperror }); @@ -1109,9 +1133,9 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ _smdEndParser(); if(typeof finalizeThinkingCard==='function') finalizeThinkingCard(); source.close(); - delete INFLIGHT[activeSid];clearInflight();clearInflightState(activeSid);stopApprovalPollingForSession(activeSid);stopClarifyPollingForSession(activeSid); - if(!_approvalSessionId||_approvalSessionId===activeSid) hideApprovalCard(true); - if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true, 'cancelled'); + _clearOwnerInflightState(); + _clearApprovalForOwner(); + _clearClarifyForOwner('cancelled'); if(S.session&&S.session.session_id===activeSid){ S.activeStreamId=null; } @@ -1138,7 +1162,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ } })(); renderSessionList(); - if(!S.session||!INFLIGHT[S.session.session_id]){setBusy(false);setComposerStatus('');} + _setActivePaneIdleIfOwner(); }); } @@ -1148,10 +1172,10 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ const session=data&&data.session; if(!session) return false; if(session.active_stream_id||session.pending_user_message) return false; - delete INFLIGHT[activeSid];clearInflight();clearInflightState(activeSid);stopApprovalPollingForSession(activeSid);stopClarifyPollingForSession(activeSid); + _clearOwnerInflightState(); _closeSource(); - if(!_approvalSessionId||_approvalSessionId===activeSid) hideApprovalCard(true); - if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true, 'terminal'); + _clearApprovalForOwner(); + _clearClarifyForOwner('terminal'); const isSessionViewed=_isSessionActivelyViewed(activeSid); const completedSid=session.session_id||activeSid; if(!isSessionViewed && typeof _markSessionCompletionUnread==='function'){ @@ -1185,12 +1209,9 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ if(isSessionViewed) _markSessionViewed(completedSid, session.message_count ?? S.messages.length); syncTopbar();renderMessages({preserveScroll:true}); } + if(_isActiveSession()) _queueDrainSid=activeSid; renderSessionList(); - if(isActiveSession||!S.session||!INFLIGHT[S.session.session_id]){ - _queueDrainSid=activeSid; - setBusy(false); - setComposerStatus(''); - } + _setActivePaneIdleIfOwner(); return true; }catch(_){ return false; @@ -1204,10 +1225,10 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ _streamFinalized=true; if(_pendingRafHandle!==null){cancelAnimationFrame(_pendingRafHandle);clearTimeout(_pendingRafHandle);_pendingRafHandle=null;_renderPending=false;} if(typeof finalizeThinkingCard==='function') finalizeThinkingCard(); - delete INFLIGHT[activeSid];clearInflight();clearInflightState(activeSid);stopApprovalPollingForSession(activeSid);stopClarifyPollingForSession(activeSid); + _clearOwnerInflightState(); _closeSource(); - if(!_approvalSessionId||_approvalSessionId===activeSid) hideApprovalCard(true); - if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true, 'terminal'); + _clearApprovalForOwner(); + _clearClarifyForOwner('terminal'); if(S.session&&S.session.session_id===activeSid){ S.activeStreamId=null; clearLiveToolCards();if(!assistantText)removeThinking(); @@ -1219,7 +1240,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ trackBackgroundError(activeSid,_errTitle,'Connection lost'); } } - if(!S.session||!INFLIGHT[S.session.session_id]){setBusy(false);setComposerStatus('');} + _setActivePaneIdleIfOwner(); } (async()=>{ @@ -1229,19 +1250,15 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ try{ const st=await api(`/api/chat/stream/status?stream_id=${encodeURIComponent(streamId)}`); if(!st.active){ - delete INFLIGHT[activeSid]; - clearInflight(); - clearInflightState(activeSid); - stopApprovalPollingForSession(activeSid); - stopClarifyPollingForSession(activeSid); - if(!_approvalSessionId||_approvalSessionId===activeSid) hideApprovalCard(true); - if(!_clarifySessionId||_clarifySessionId===activeSid) hideClarifyCard(true, 'terminal'); + _clearOwnerInflightState(); + _clearApprovalForOwner(); + _clearClarifyForOwner('terminal'); if(S.session&&S.session.session_id===activeSid){ S.activeStreamId=null; clearLiveToolCards(); removeThinking(); - _queueDrainSid=activeSid;setBusy(false); - setComposerStatus(''); + if(_isActiveSession()) _queueDrainSid=activeSid; + _setActivePaneIdleIfOwner(); renderMessages({preserveScroll:true}); renderSessionList(); } diff --git a/tests/test_1694_terminal_cleanup_ownership.py b/tests/test_1694_terminal_cleanup_ownership.py new file mode 100644 index 00000000..57fea1d1 --- /dev/null +++ b/tests/test_1694_terminal_cleanup_ownership.py @@ -0,0 +1,93 @@ +"""Regression tests for #1694 terminal stream cleanup ownership. + +Terminal SSE events for one session must not mutate another currently viewed +active pane. The owning session's persisted/runtime stream marker can be cleared, +but global pane state such as ``clearInflight()``, approval/clarify polling, and +``setBusy(false)`` must be gated to the session that owns the active pane/card. +""" +from pathlib import Path + +REPO_ROOT = Path(__file__).parent.parent +MESSAGES_JS = (REPO_ROOT / "static" / "messages.js").read_text(encoding="utf-8") + + +def _body_from_brace(src: str, brace: int, label: str) -> str: + assert brace >= 0, f"body opening brace not found for: {label}" + depth = 1 + i = brace + 1 + while i < len(src) and depth: + ch = src[i] + if ch == "{": + depth += 1 + elif ch == "}": + depth -= 1 + i += 1 + assert depth == 0, f"body did not close for: {label}" + return src[brace + 1 : i - 1] + + +def _brace_body_after(src: str, marker: str) -> str: + start = src.find(marker) + assert start >= 0, f"marker not found: {marker}" + brace = src.find("{", start) + return _body_from_brace(src, brace, marker) + + +def _event_body(event_name: str) -> str: + return _brace_body_after(MESSAGES_JS, f"source.addEventListener('{event_name}'") + + +def _function_body(name: str) -> str: + marker = f"function {name}(" + start = MESSAGES_JS.find(marker) + assert start >= 0, f"function not found: {name}" + signature_end = MESSAGES_JS.find("){", start) + assert signature_end >= 0, f"function body not found: {name}" + return _body_from_brace(MESSAGES_JS, signature_end + 1, name) + + +def test_terminal_handlers_use_session_owned_cleanup_helpers(): + """Patch #1694 should centralize terminal cleanup behind owner-aware helpers.""" + attach_body = _function_body("attachLiveStream") + assert "function _clearOwnerInflightState()" in attach_body + owner_helper = _function_body("_clearOwnerInflightState") + assert "delete INFLIGHT[activeSid]" in owner_helper + assert "clearInflightState(activeSid)" in owner_helper + assert "_clearActivePaneInflightIfOwner();" in owner_helper + assert "function _clearActivePaneInflightIfOwner()" in attach_body + assert "function _clearApprovalForOwner()" in attach_body + assert "function _clearClarifyForOwner(" in attach_body + assert "function _setActivePaneIdleIfOwner(" in attach_body + + +def test_done_event_does_not_clear_active_pane_for_background_session(): + """A background done event may clear its owner marker, not the active pane.""" + body = _event_body("done") + assert "_clearOwnerInflightState();" in body + assert "clearInflight();clearInflightState(activeSid)" not in body + assert "delete INFLIGHT[activeSid];\n clearInflight();" not in body + assert "renderSessionList();setBusy(false)" not in body + assert "_setActivePaneIdleIfOwner" in body + + +def test_error_and_cancel_events_do_not_blanket_stop_active_pane_polling(): + """Background app errors/cancels must not stop another pane's prompt polling.""" + for event_name in ("apperror", "cancel"): + body = _event_body(event_name) + assert "_clearOwnerInflightState();" in body, event_name + assert "_clearApprovalForOwner" in body, event_name + assert "_clearClarifyForOwner" in body, event_name + assert "stopApprovalPolling();stopClarifyPolling();" not in body, event_name + assert "clearInflight();clearInflightState(activeSid)" not in body, event_name + + +def test_reconnect_settled_and_error_paths_keep_cleanup_session_scoped(): + """Reconnect terminal cleanup paths should follow the same owner model.""" + restore_body = _function_body("_restoreSettledSession") + error_body = _function_body("_handleStreamError") + combined = restore_body + "\n" + error_body + assert combined.count("_clearOwnerInflightState();") >= 2 + assert "delete INFLIGHT[activeSid];clearInflight();clearInflightState(activeSid)" not in combined + assert "stopApprovalPolling();stopClarifyPolling();" not in combined + assert "renderSessionList();setBusy(false)" not in combined + assert "_setActivePaneIdleIfOwner" in combined diff --git a/tests/test_issue856_background_completion_unread.py b/tests/test_issue856_background_completion_unread.py index 0262ee08..47a7da6c 100644 --- a/tests/test_issue856_background_completion_unread.py +++ b/tests/test_issue856_background_completion_unread.py @@ -74,17 +74,19 @@ def test_done_event_updates_sidebar_cache_immediately_after_completion_marker(): done_block = _done_block() marker_idx = done_block.find("_markSessionCompletionUnread(completedSid") - delete_idx = done_block.find("delete INFLIGHT[activeSid];") + cleanup_idx = done_block.find("_clearOwnerInflightState();") + if cleanup_idx == -1: + cleanup_idx = done_block.find("delete INFLIGHT[activeSid];") cache_idx = done_block.find("_markSessionCompletedInList(completedSession, activeSid);") refresh_idx = done_block.find("renderSessionList();", cache_idx) sound_idx = done_block.find("playNotificationSound();", cache_idx) assert "function _markSessionCompletedInList(" in SESSIONS_JS assert marker_idx != -1, "done handler must write the completion-unread marker first" - assert delete_idx != -1, "done handler must clear local INFLIGHT before rendering idle state" + assert cleanup_idx != -1, "done handler must clear local INFLIGHT before rendering idle state" assert cache_idx != -1, "done handler must update the sidebar cache immediately" assert refresh_idx != -1 and sound_idx != -1 - assert marker_idx < delete_idx < cache_idx < refresh_idx < sound_idx, ( + assert marker_idx < cleanup_idx < cache_idx < refresh_idx < sound_idx, ( "the sidebar should flip from spinner to dot from the done payload before " "waiting for /api/sessions or playing the completion cue" ) diff --git a/tests/test_regressions.py b/tests/test_regressions.py index 444ec66e..cbd50faf 100644 --- a/tests/test_regressions.py +++ b/tests/test_regressions.py @@ -248,8 +248,13 @@ def test_done_handler_guards_setbusy_with_inflight_check(cleanup_test_sessions): disable B's Send button. """ src = (REPO_ROOT / "static/messages.js").read_text() - # The fix wraps setBusy(false) in a guard - assert "INFLIGHT[S.session.session_id]" in src, "messages.js must guard setBusy(false) with INFLIGHT check for current session" + # The fix wraps setBusy(false) in an active-pane ownership guard. Newer + # implementations may centralize the guard in a helper rather than repeat the + # raw INFLIGHT expression at every terminal event site. + assert ( + "INFLIGHT[S.session.session_id]" in src + or "function _setActivePaneIdleIfOwner" in src + ), "messages.js must guard setBusy(false) for the current session" def test_refresh_handler_does_not_drop_tool_messages_needed_by_todos(cleanup_test_sessions): diff --git a/tests/test_session_runtime_ownership_invariants.py b/tests/test_session_runtime_ownership_invariants.py index ae84abe8..68e1a10c 100644 --- a/tests/test_session_runtime_ownership_invariants.py +++ b/tests/test_session_runtime_ownership_invariants.py @@ -63,9 +63,13 @@ class TestSessionOwnedRuntimeInvariants: "A background session's done event must not unconditionally call setBusy(false); " "that can idle an unrelated active pane that is still running." ) - assert "if(isActiveSession||!S.session||!INFLIGHT[S.session.session_id])" in done.replace(" ", ""), ( - "The done handler should only idle composer state when the completed stream " - "belongs to the active pane, or when no other active-pane inflight runtime exists." + normalized = done.replace(" ", "") + assert ( + "if(isActiveSession||!S.session||!INFLIGHT[S.session.session_id])" in normalized + or "_setActivePaneIdleIfOwner();" in done + ), ( + "The done handler should only idle composer state through an active-pane guard, " + "not from background completions owned by another session." ) def test_server_session_finalize_does_not_idle_unrelated_active_pane(self): @@ -75,7 +79,11 @@ class TestSessionOwnedRuntimeInvariants: "The fallback server-finalize path must not idle the active pane for a " "background session completion." ) - assert "if(isActiveSession||!S.session||!INFLIGHT[S.session.session_id])" in finalize.replace(" ", ""), ( + normalized = finalize.replace(" ", "") + assert ( + "if(isActiveSession||!S.session||!INFLIGHT[S.session.session_id])" in normalized + or "_setActivePaneIdleIfOwner();" in finalize + ), ( "The fallback server-finalize path should use the same active-pane guard as the live done event." ) @@ -96,8 +104,14 @@ class TestSessionOwnedRuntimeInvariants: ) done = _event_handler(messages, "done") - assert "stopApprovalPollingForSession(activeSid)" in done - assert "stopClarifyPollingForSession(activeSid)" in done + assert ( + "stopApprovalPollingForSession(activeSid)" in done + or "_clearApprovalForOwner();" in done + ) + assert ( + "stopClarifyPollingForSession(activeSid)" in done + or "_clearClarifyForOwner('terminal');" in done + ) assert "stopApprovalPolling();\n stopClarifyPolling();" not in done, ( "The done handler must not blindly stop whatever approval/clarify poller " "the active pane currently owns." diff --git a/tests/test_sprint30.py b/tests/test_sprint30.py index cec2bc8a..92e0e360 100644 --- a/tests/test_sprint30.py +++ b/tests/test_sprint30.py @@ -602,8 +602,10 @@ class TestClarifyCardTimerLogic: src, re.DOTALL) assert m, 'cancel event handler not found' body = m.group(0) - assert "hideClarifyCard(true, 'cancelled')" in body, \ - 'explicit stream cancel must not use the timeout/terminal draft preservation path' + assert ( + "hideClarifyCard(true, 'cancelled')" in body + or "_clearClarifyForOwner('cancelled')" in body + ), 'explicit stream cancel must not use the timeout/terminal draft preservation path' def test_clarify_urgent_countdown_has_non_color_cue(self): css = self._get_css().read_text() diff --git a/tests/test_sprint36.py b/tests/test_sprint36.py index 7c26b83e..00d540b1 100644 --- a/tests/test_sprint36.py +++ b/tests/test_sprint36.py @@ -166,9 +166,18 @@ def test_sse_cancel_handler_calls_set_busy(): # Find the closing of this handler block (next top-level addEventListener) next_handler = src.find("source.addEventListener(", idx + 50) block = src[idx:next_handler] if next_handler != -1 else src[idx:idx + 3000] - assert "setBusy(false)" in block, ( - "SSE cancel handler no longer calls setBusy(false)" + assert ( + "setBusy(false)" in block + or "_setActivePaneIdleIfOwner()" in block + ), ( + "SSE cancel handler no longer idles the owning active pane" ) + if "_setActivePaneIdleIfOwner()" in block: + helper_idx = src.find("function _setActivePaneIdleIfOwner") + assert helper_idx != -1 + next_function = src.find("\n function ", helper_idx + 1) + helper = src[helper_idx:next_function if next_function != -1 else helper_idx + 800] + assert "setBusy(false)" in helper # ── 7. i18n key preserved ───────────────────────────────────────────────────── From fc5423f4aa2ae8b87116573775c2d6f02f8db56b Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Wed, 6 May 2026 22:02:37 +0000 Subject: [PATCH 2/2] auto-fix: preserve _setActivePaneIdleIfOwner permissive-fallback disjunct from PR #1753 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #1753 (shipped v0.51.12) introduced the 3-way OR guard in done/error/cancel handlers: 'isActiveSession || !S.session || !INFLIGHT[S.session.session_id]'. The third disjunct ('no other inflight on the active pane') is the permissive fallback Opus stage-306 verified — it allows the active pane to idle when no other session is running, even when the completing stream is from a different session. PR #1761's centralizing helper _setActivePaneIdleIfOwner inadvertently dropped this disjunct, so a user viewing pane A (idle) while pane B completes in the background would not get pane A's composer state cleared. Restored: _setActivePaneIdleIfOwner now checks the same 3-way OR. Verified via: - node -c static/messages.js — clean - pytest tests/test_session_runtime_ownership_invariants.py tests/test_1694_terminal_cleanup_ownership.py — 9 passed Co-authored-by: dso2ng --- static/messages.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/static/messages.js b/static/messages.js index 8756fb93..03698087 100644 --- a/static/messages.js +++ b/static/messages.js @@ -382,7 +382,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ _clearActivePaneInflightIfOwner(); } function _setActivePaneIdleIfOwner(){ - if(_isActiveSession()||!S.session){ + if(_isActiveSession()||!S.session||!INFLIGHT[S.session.session_id]){ setBusy(false); setComposerStatus(''); if(typeof setStatus==='function') setStatus('');