From 19080f73b1fe786b263814d3b5730ec91c8d2935 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Wed, 10 Jun 2026 01:12:48 -0700 Subject: [PATCH] =?UTF-8?q?Release=20v0.51.348=20=E2=80=94=20Release=20LL?= =?UTF-8?q?=20(Phase=200=20hotfix:=20timeout=20regression=20+=20data-loss?= =?UTF-8?q?=20+=20leaks)=20(#3917)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * stage v0.51.348: Phase 0 hotfix — approval/clarify timeout regression (#3913), queue/draft durability (#3906), settings auto-reopen (#3909), kanban FD leak (#3904) * stage v0.51.348: re-anchor 4 SSE frontend tests to poll-only design (#3913); apply Opus SHOULD-FIX — immediate first poll tick so pending approval/clarify cards show instantly --------- Co-authored-by: nesquena-hermes --- CHANGELOG.md | 12 ++ api/kanban_bridge.py | 17 ++- static/boot.js | 8 +- static/messages.js | 130 +++++------------- static/panels.js | 14 +- static/sessions.js | 42 +++--- static/ui.js | 58 ++++++-- tests/test_approval_sse.py | 82 ++++++----- tests/test_clarify_sse.py | 46 +++++-- tests/test_issue660.py | 93 +++++++++---- tests/test_issue_new_chat_draft_restore.py | 16 +++ ...st_session_runtime_ownership_invariants.py | 13 +- tests/test_subpath_frontend_routes.py | 8 +- 13 files changed, 315 insertions(+), 224 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bc8b685..86f28391 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,18 @@ ## [Unreleased] +## [v0.51.348] — 2026-06-10 — Release LL (Phase 0 hotfix: timeout regression + data-loss + leaks) + +### Fixed + +- **"Request timed out" during approval/clarify turns no longer fires from browser connection-pool exhaustion.** v0.51.340 added `/api/session/stream` as a 6th persistent SSE `EventSource`. Browsers cap HTTP/1.1 at 6 connections per origin, so with all 6 long-lived streams open the next `fetch()` — including the approval/clarify POST itself — queued indefinitely and surfaced as a timeout toast even though the server responded normally. Approval and clarify prompts now use HTTP polling (approval 1.5 s, clarify 3 s) via the existing fallback-poll helpers, freeing 2 connection slots. (#3913, fixes #3807 / #3748 / #3014) +- **Queued follow-up messages and draft-only sessions are more durable across refresh and tab restore.** Session queues now mirror to both `sessionStorage` and `localStorage`, restore through one shared helper (falling back to the durable copy when `sessionStorage` is missing after a tab/process restore), clear stale queue state from both layers, and keep zero-message sessions when they still own unsent composer draft text or files. (#3906, #3108) +- **Settings panel no longer auto-reopens while you are on the Chat panel.** `switchSettingsSection()` force-mutated the current panel back to `settings` instead of just remembering the section for the next time settings is opened; visible on iPad/touch. (#3909) + +### Performance + +- **Kanban API requests no longer leak a file descriptor each.** `api/kanban_bridge.py::_conn()` returned a raw connection used as a `with` block, but sqlite3's context manager only scopes the transaction and never closes the FD. On a long-lived server these accumulated, pinning stale WAL snapshots and starving checkpoints for gateway/CLI workers sharing the board DB. `_conn()` now uses the closing context manager. (#3904) + ## [v0.51.347] — 2026-06-09 — Release LK (streaming & render reliability cluster) ### Fixed diff --git a/api/kanban_bridge.py b/api/kanban_bridge.py index 45fd0168..166da10c 100644 --- a/api/kanban_bridge.py +++ b/api/kanban_bridge.py @@ -79,9 +79,24 @@ def _normalise_board_or_raise(raw): def _conn(board=None): - """Initialize the kanban DB for the given board slug and return a context-managed sqlite connection.""" + """Initialize the kanban DB for the given board slug and return a context manager + that yields a sqlite connection and CLOSES it on exit. + + Must be ``kb.connect_closing`` — a raw ``kb.connect()`` connection used as + ``with _conn(...) as conn:`` only gets sqlite3's transaction-scope context + manager, which never closes the file descriptor. In this long-lived server + that leaks one FD per request and pins stale WAL snapshots (FDs to deleted + ``-wal``/``-shm`` files), which starves SQLite checkpoints on the shared + kanban DB and aggravates probe⇄checkpoint contention for every process. + """ kb = _kb() kb.init_db(board=board) + closing = getattr(kb, "connect_closing", None) + if closing is not None: + return closing(board=board) + # Older kanban_db builds (and lightweight test doubles) without + # connect_closing: fall back to the raw connection; sqlite3's own + # context manager at least scopes the transaction. return kb.connect(board=board) diff --git a/static/boot.js b/static/boot.js index bb2c4709..771dba80 100644 --- a/static/boot.js +++ b/static/boot.js @@ -2044,7 +2044,13 @@ function applyBotName(){ S.session.active_stream_id || S.session.pending_user_message ); - if(S.session && (S.session.message_count||0) === 0 && !_restoredInFlight){ + const _restoredDraft = (S.session && S.session.composer_draft) || {}; + const _restoredDraftText = String(_restoredDraft.text||'').trim(); + const _restoredDraftFiles = Array.isArray(_restoredDraft.files) + ? _restoredDraft.files.filter(Boolean) + : []; + const _restoredHasDraft = !!(_restoredDraftText || _restoredDraftFiles.length); + if(S.session && (S.session.message_count||0) === 0 && !_restoredInFlight && !_restoredHasDraft){ S.session=null; S.messages=[]; S._bootReady=true; // Restore panel pref before syncing so the workspace panel stays visible diff --git a/static/messages.js b/static/messages.js index 3828c5b5..69e5a3d7 100644 --- a/static/messages.js +++ b/static/messages.js @@ -4136,44 +4136,19 @@ async function respondApproval(choice) { function startApprovalPolling(sid) { stopApprovalPolling(); _approvalPollingSessionId = sid || null; - // ── SSE (preferred): long-lived connection, server pushes instantly ── - try { - const es = new EventSource(new URL('api/approval/stream?session_id=' + encodeURIComponent(sid), document.baseURI || location.href).href); - let _fallbackActive = false; - es.addEventListener('initial', e => { - const d = JSON.parse(e.data); - if (d.pending) { showApprovalForSession(sid, d.pending, d.pending_count || 1); } - else { _clearApprovalPendingForSession(sid); _hideApprovalCardIfOwner(sid); } - }); - - es.addEventListener('approval', e => { - const d = JSON.parse(e.data); - if (d.pending) { showApprovalForSession(sid, d.pending, d.pending_count || 1); } - else { _clearApprovalPendingForSession(sid); _hideApprovalCardIfOwner(sid); } - }); - - es.onerror = () => { - // SSE failed — fall back to HTTP polling (3s interval) - if (_fallbackActive) return; - _fallbackActive = true; - try { es.close(); } catch(_){} - _startApprovalFallbackPoll(sid); - }; - - // If the session changes or stops being busy, close the SSE. - // We detect this via a periodic check (cheap — no network request). - _approvalSSEHealthTimer = setInterval(() => { - if (!S.busy || !S.session || S.session.session_id !== sid) { - stopApprovalPolling(); _hideApprovalCardIfOwner(sid, true); - } - }, 5000); - - _approvalEventSource = es; - } catch(_e) { - // EventSource constructor failed — use polling directly - _startApprovalFallbackPoll(sid); - } + // Use HTTP polling instead of SSE to avoid browser connection pool exhaustion. + // Browsers limit to 6 concurrent HTTP connections per origin over HTTP/1.1. + // With 6 persistent SSE streams (sessions/events, gateway/stream, + // session/stream, approval/stream, clarify/stream, chat/stream), the pool + // fills and all fetch() requests queue indefinitely. The server responds + // normally (curl works), but the browser has no available sockets. + // + // This was introduced in v0.51.340 when /api/session/stream was added as + // the 6th persistent SSE connection. Until we multiplex streams or serve + // SSE from a separate origin, use HTTP polling to free 2 connection slots. + // (1.5-second interval, acceptable tradeoff) + _startApprovalFallbackPoll(sid); } let _approvalEventSource = null; @@ -4181,7 +4156,10 @@ let _approvalSSEHealthTimer = null; let _approvalPollingSessionId = null; function _startApprovalFallbackPoll(sid) { - _approvalPollTimer = setInterval(async () => { + // Run one tick immediately so a session already blocked on a pending approval + // shows its card instantly (the removed SSE 'initial' event used to do this); + // then poll on the 1500ms cadence. (#3913 SHOULD-FIX) + const _tick = async () => { if (!S.busy || !S.session || S.session.session_id !== sid) { stopApprovalPolling(); _hideApprovalCardIfOwner(sid, true); return; } @@ -4193,7 +4171,9 @@ function _startApprovalFallbackPoll(sid) { else { _clearApprovalPendingForSession(sid); _hideApprovalCardIfOwner(sid); } } catch(e) { /* ignore poll errors */ } finally { _approvalFallbackPollInFlight = false; } - }, 1500); // matches the v0.50.247 polling cadence so degraded-mode users see the same responsiveness + }; + _approvalPollTimer = setInterval(_tick, 1500); // matches the v0.50.247 polling cadence so degraded-mode users see the same responsiveness + _tick(); } function stopApprovalPollingForSession(sid) { @@ -4879,64 +4859,26 @@ function startClarifyPolling(sid) { _clarifyPollingSessionId = sid || null; _clarifyMissingEndpointWarned = false; - // SSE primary path: long-lived connection pushes events instantly. - try { - _clarifyEventSource = new EventSource(new URL('api/clarify/stream?session_id=' + encodeURIComponent(sid), document.baseURI || location.href).href); - } catch(e) { - _startClarifyFallbackPoll(sid); - return; - } - - _clarifyEventSource.addEventListener('initial', function(ev) { - try { - var d = JSON.parse(ev.data); - if (d.pending) { showClarifyForSession(sid, d.pending); } - else { _clearClarifyPendingForSession(sid); _hideClarifyCardIfOwner(sid, false, 'expired'); } - } catch(e) {} - }); - - _clarifyEventSource.addEventListener('clarify', function(ev) { - try { - var d = JSON.parse(ev.data); - if (d.pending) { showClarifyForSession(sid, d.pending); } - else { _clearClarifyPendingForSession(sid); _hideClarifyCardIfOwner(sid, false, 'expired'); } - } catch(e) {} - }); - - _clarifyEventSource.onerror = function() { - if (_clarifyEventSource) { try { _clarifyEventSource.close(); } catch(_){} _clarifyEventSource = null; } - if (_clarifyHealthTimer) { clearInterval(_clarifyHealthTimer); _clarifyHealthTimer = null; } - _startClarifyFallbackPoll(sid); - }; - - // Stale-detector: track last event timestamp; only reconnect if no event - // (initial or clarify) has arrived in 60s. The server sends a keepalive - // comment line every 30s but EventSource silently consumes those; we only - // bump lastEventAt on actual application events. With no real events for - // 60s on a long-lived clarify connection the server is effectively silent - // and a reconnect is the safe move. + // Use HTTP polling instead of SSE to avoid browser connection pool exhaustion. + // Browsers limit to 6 concurrent HTTP connections per origin over HTTP/1.1. + // With 6 persistent SSE streams (sessions/events, gateway/stream, + // session/stream, approval/stream, clarify/stream, chat/stream), the pool + // fills and all fetch() requests queue indefinitely. The server responds + // normally (curl works), but the browser has no available sockets. // - // Without the lastEventAt gate the original PR force-reconnected every 60s - // regardless of activity, which churned one TCP/SSE setup per minute per - // active session. (Opus pre-release review of v0.50.249.) - let _lastClarifyEventAt = Date.now(); - const _markClarifyEvent = () => { _lastClarifyEventAt = Date.now(); }; - _clarifyEventSource.addEventListener('initial', _markClarifyEvent); - _clarifyEventSource.addEventListener('clarify', _markClarifyEvent); - _clarifyHealthTimer = setInterval(function() { - if (Date.now() - _lastClarifyEventAt < 60000) return; - if (_clarifyEventSource) { - try { _clarifyEventSource.close(); } catch(_){} - _clarifyEventSource = null; - } - clearInterval(_clarifyHealthTimer); _clarifyHealthTimer = null; - startClarifyPolling(sid); - }, 60000); + // This was introduced in v0.51.340 when /api/session/stream was added as + // the 6th persistent SSE connection. Until we multiplex streams or serve + // SSE from a separate origin, use HTTP polling to free 2 connection slots. + // (3-second interval, acceptable tradeoff) + _startClarifyFallbackPoll(sid); } function _startClarifyFallbackPoll(sid) { _clarifyPollingSessionId = sid || null; - _clarifyFallbackTimer = setInterval(async () => { + // Run one tick immediately so a session already blocked on a pending clarify + // shows its card instantly (the removed SSE 'initial' event used to do this); + // then poll on the 3000ms cadence. (#3913 SHOULD-FIX) + const _tick = async () => { if (!S.session || S.session.session_id !== sid) { stopClarifyPolling(); _hideClarifyCardIfOwner(sid, true, 'session'); return; } @@ -4959,7 +4901,9 @@ function _startClarifyFallbackPoll(sid) { } finally { _clarifyFallbackPollInFlight = false; } - }, 3000); + }; + _clarifyFallbackTimer = setInterval(_tick, 3000); + _tick(); } function stopClarifyPollingForSession(sid) { diff --git a/static/panels.js b/static/panels.js index d4073b60..ecf35493 100644 --- a/static/panels.js +++ b/static/panels.js @@ -6066,15 +6066,13 @@ function _toggleTabVisibilityChip(panel){ } function switchSettingsSection(name){ - // If the main content is not showing settings, switch back first + // If the main content is not showing settings, just remember the section + // without force-switching the panel. The section will be applied when the + // user next opens settings via switchPanel(). (#appearance-auto-reopen) if (_currentPanel !== 'settings') { - _currentPanel = 'settings'; - var mainEl = document.querySelector('main.main'); - if (mainEl) { - ['settings','skills','memory','tasks','kanban','workspaces','profiles','insights','logs','plugin'].forEach(function(p) { - mainEl.classList.toggle('showing-' + p, p === 'settings'); - }); - } + _currentSettingsSection = name; + _settingsSection = name; + return; } let section=(name==='appearance'||name==='preferences'||name==='providers'||name==='plugins'||name==='system'||name==='help')?name:'conversation'; // Deep-linking to the Plugins pane when the tab is hidden (no plugins diff --git a/static/sessions.js b/static/sessions.js index 36409307..17883cac 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -1110,34 +1110,29 @@ async function loadSession(sid){ // Stale? A newer loadSession() call has already started (#1060). if (_loadingSessionId !== sid) return; - // Restore any queued message that survived page refresh via sessionStorage. + // Restore any queued message that survived page refresh or tab restore. if(typeof queueSessionMessage==='function'){ try{ - const _storedQ=sessionStorage.getItem('hermes-queue-'+sid); - if(_storedQ){ - const _entries=JSON.parse(_storedQ); - if(Array.isArray(_entries)&&_entries.length){ - const _lastMsg=S.messages.slice().reverse() - .find(m=>m&&m.role==='assistant'); - const _lastAsst=_lastMsg?(_lastMsg.timestamp||_lastMsg._ts||0)*1000:0; - const _fresh=_entries.filter(e=>!e._queued_at||e._queued_at>_lastAsst); - if(_fresh.length){ - const _first=_fresh[0]; - const _msg=$&&$('msg'); - if(_msg&&_first.text&&!_msg.value){ - _msg.value=_first.text||''; - if(typeof autoResize==='function') autoResize(); - if(typeof showToast==='function') showToast((_fresh.length>1?`${_fresh.length} queued messages restored (showing first)`:'Queued message restored')+' — review and send when ready'); - } - sessionStorage.removeItem('hermes-queue-'+sid); - } else { - sessionStorage.removeItem('hermes-queue-'+sid); + const _entries=typeof _readPersistedSessionQueue==='function' + ? _readPersistedSessionQueue(sid) + : []; + if(Array.isArray(_entries)&&_entries.length){ + const _lastMsg=S.messages.slice().reverse() + .find(m=>m&&m.role==='assistant'); + const _lastAsst=_lastMsg?(_lastMsg.timestamp||_lastMsg._ts||0)*1000:0; + const _fresh=_entries.filter(e=>!e._queued_at||e._queued_at>_lastAsst); + if(_fresh.length){ + const _first=_fresh[0]; + const _msg=$&&$('msg'); + if(_msg&&_first.text&&!_msg.value){ + _msg.value=_first.text||''; + if(typeof autoResize==='function') autoResize(); + if(typeof showToast==='function') showToast((_fresh.length>1?`${_fresh.length} queued messages restored (showing first)`:'Queued message restored')+' — review and send when ready'); } - } else { - sessionStorage.removeItem('hermes-queue-'+sid); } + if(typeof _clearPersistedSessionQueue==='function') _clearPersistedSessionQueue(sid); } - }catch(_){sessionStorage.removeItem('hermes-queue-'+sid);} + }catch(_){if(typeof _clearPersistedSessionQueue==='function') _clearPersistedSessionQueue(sid);} } // Reconstruct tool calls from message metadata, or fall back to session-level summary. @@ -5840,6 +5835,7 @@ async function deleteSession(sid, beforeDelete=null){ return false; } const response=deleteResult&&deleteResult.response; + if(typeof _clearPersistedSessionQueue==='function') _clearPersistedSessionQueue(sid); if(!optimisticRendered){ _pendingSessionReflowPositions=reflowPositions; _optimisticallyRemoveSessionFromList(sid); diff --git a/static/ui.js b/static/ui.js index c30b71a8..378e2016 100644 --- a/static/ui.js +++ b/static/ui.js @@ -163,14 +163,52 @@ function _getSessionQueue(sid, create=false){ if(!SESSION_QUEUES[sid]&&create) SESSION_QUEUES[sid]=[]; return SESSION_QUEUES[sid]||[]; } +function _queueStorageKey(sid){ + return 'hermes-queue-'+sid; +} +function _clearPersistedSessionQueue(sid){ + if(!sid) return; + const key=_queueStorageKey(sid); + try{sessionStorage.removeItem(key);}catch(_){} + try{localStorage.removeItem(key);}catch(_){} +} +function _persistSessionQueueStorage(sid, queue){ + if(!sid) return; + const q=Array.isArray(queue)?queue:[]; + if(!q.length){_clearPersistedSessionQueue(sid);return;} + const key=_queueStorageKey(sid); + let payload='[]'; + try{payload=JSON.stringify(q);}catch(_){return;} + try{sessionStorage.setItem(key,payload);}catch(_){} + try{localStorage.setItem(key,payload);}catch(_){} +} +function _readPersistedSessionQueue(sid){ + if(!sid) return []; + const key=_queueStorageKey(sid); + const read=(store)=>{ + try{ + const raw=store&&store.getItem?store.getItem(key):null; + if(!raw) return null; + const parsed=JSON.parse(raw); + return Array.isArray(parsed)?parsed:null; + }catch(_){return null;} + }; + const sessionValue=read(sessionStorage); + if(sessionValue&&sessionValue.length) return sessionValue; + const localValue=read(localStorage); + if(localValue&&localValue.length){ + try{sessionStorage.setItem(key,JSON.stringify(localValue));}catch(_){} + return localValue; + } + return []; +} function queueSessionMessage(sid, payload){ if(!sid||!payload) return 0; const q=_getSessionQueue(sid,true); // Stamp created_at so the restore path can detect stale entries (agent already responded) const entry={...payload, _queued_at: Date.now()}; q.push(entry); - // Persist to sessionStorage so the queue survives page refresh - try{ sessionStorage.setItem('hermes-queue-'+sid, JSON.stringify(q)); }catch(_){} + _persistSessionQueueStorage(sid,q); return q.length; } function shiftQueuedSessionMessage(sid){ @@ -179,9 +217,9 @@ function shiftQueuedSessionMessage(sid){ const next=q.shift(); if(!q.length){ delete SESSION_QUEUES[sid]; - try{ sessionStorage.removeItem('hermes-queue-'+sid); }catch(_){} + _clearPersistedSessionQueue(sid); } else { - try{ sessionStorage.setItem('hermes-queue-'+sid, JSON.stringify(q)); }catch(_){} + _persistSessionQueueStorage(sid,q); } return next; } @@ -4204,8 +4242,8 @@ function _renderQueueChips(sid){ function _saveAndRefresh(){ const liveQ=_getSessionQueue(sid,false); - if(!liveQ.length){delete SESSION_QUEUES[sid];try{sessionStorage.removeItem('hermes-queue-'+sid);}catch(_){}} - else{SESSION_QUEUES[sid]=[...liveQ];try{sessionStorage.setItem('hermes-queue-'+sid,JSON.stringify(liveQ));}catch(_){}} + if(!liveQ.length){delete SESSION_QUEUES[sid];_clearPersistedSessionQueue(sid);} + else{SESSION_QUEUES[sid]=[...liveQ];_persistSessionQueueStorage(sid,liveQ);} delete _queueRenderKeys[sid]; updateQueueBadge(sid); } @@ -4232,7 +4270,7 @@ function _renderQueueChips(sid){ const firstFiles=(snapshot.find(e=>e&&Array.isArray(e.files)&&e.files.length)||{files:[]}).files; liveQ.length=0;liveQ.push({text:combined,files:firstFiles,model:first.model||'',model_provider:first.model_provider||null,_queued_at:Date.now()}); SESSION_QUEUES[sid]=liveQ; - try{sessionStorage.setItem('hermes-queue-'+sid,JSON.stringify(liveQ));}catch(_){} + _persistSessionQueueStorage(sid,liveQ); delete _queueRenderKeys[sid]; updateQueueBadge(sid); }; @@ -4312,7 +4350,7 @@ function _renderQueueChips(sid){ const idx=_entryTs!=null?liveQ.findIndex(e=>e&&e._queued_at===_entryTs):i; if(idx!==-1){ liveQ[idx]={...liveQ[idx],text:newText}; - try{sessionStorage.setItem('hermes-queue-'+sid,JSON.stringify(liveQ));}catch(_){} + _persistSessionQueueStorage(sid,liveQ); delete _queueRenderKeys[sid]; updateQueueBadge(sid); } @@ -4351,8 +4389,8 @@ function _renderQueueChips(sid){ const liveQ=_getSessionQueue(sid,false); const idx=_entryTs!=null?liveQ.findIndex(e=>e&&e._queued_at===_entryTs):i; if(idx!==-1) liveQ.splice(idx,1); - if(!liveQ.length){delete SESSION_QUEUES[sid];try{sessionStorage.removeItem('hermes-queue-'+sid);}catch(_){}} - else{SESSION_QUEUES[sid]=[...liveQ];try{sessionStorage.setItem('hermes-queue-'+sid,JSON.stringify(liveQ));}catch(_){}} + if(!liveQ.length){delete SESSION_QUEUES[sid];_clearPersistedSessionQueue(sid);} + else{SESSION_QUEUES[sid]=[...liveQ];_persistSessionQueueStorage(sid,liveQ);} delete _queueRenderKeys[sid]; updateQueueBadge(sid); }; diff --git a/tests/test_approval_sse.py b/tests/test_approval_sse.py index fae8cf0f..b9343c3d 100644 --- a/tests/test_approval_sse.py +++ b/tests/test_approval_sse.py @@ -148,55 +148,53 @@ class TestSSEStaticAnalysis: class TestFrontendSSEImplementation: - """Verify the frontend JavaScript SSE implementation.""" + """Verify the frontend approval prompt transport. - def test_eventsource_used(self): - """Frontend must use EventSource for SSE connection.""" - assert "new EventSource(" in MESSAGES_JS, \ - "startApprovalPolling must create an EventSource for SSE" + As of #3913 the frontend no longer opens an approval-stream EventSource: + six persistent SSE connections exhausted the browser's 6-per-origin + HTTP/1.1 pool, hanging the approval POST itself ("Request timed out"). + ``startApprovalPolling`` now routes straight to the HTTP fallback poller. + The backend SSE route remains for compatibility (its tests are above); + these assertions pin the poll-only frontend so the regression can't return. + """ - def test_sse_url_matches_backend(self): - """Frontend SSE URL must match backend approval stream route.""" - assert "api/approval/stream" in MESSAGES_JS, \ - "EventSource must connect to the approval stream endpoint" + def _approval_polling_body(self): + start = MESSAGES_JS.index("function startApprovalPolling(") + end = MESSAGES_JS.index("\nfunction ", start + 1) + return MESSAGES_JS[start:end] + + def test_frontend_does_not_open_approval_stream(self): + """startApprovalPolling must NOT create an approval-stream EventSource (#3913).""" + body = self._approval_polling_body() + assert "api/approval/stream" not in body, \ + "Frontend must not open the approval-stream EventSource (browser conn-pool exhaustion, #3913)" + assert "new EventSource(" not in body, \ + "startApprovalPolling must not construct an EventSource — it polls over HTTP now" + + def test_routes_directly_to_fallback_poll(self): + """startApprovalPolling must call _startApprovalFallbackPoll directly.""" + body = self._approval_polling_body() + assert "_startApprovalFallbackPoll(sid)" in body, \ + "startApprovalPolling must route to the HTTP fallback poller" + + def test_fallback_poll_hits_pending_endpoint(self): + """The fallback poller must GET the approval/pending endpoint relative to the mount.""" + assert 'api("/api/approval/pending?session_id="' in MESSAGES_JS, \ + "Fallback poll must query /api/approval/pending" assert "EventSource('/api/approval/stream" not in MESSAGES_JS, \ - "EventSource URL must stay relative for subpath mounts" - - def test_initial_event_listener(self): - """Frontend must listen for 'initial' SSE events.""" - assert "'initial'" in MESSAGES_JS or '"initial"' in MESSAGES_JS, \ - "Frontend must addEventListener for 'initial' SSE events" - - def test_approval_event_listener(self): - """Frontend must listen for 'approval' SSE events.""" - assert "'approval'" in MESSAGES_JS or '"approval"' in MESSAGES_JS, \ - "Frontend must addEventListener for 'approval' SSE events" - - def test_onerror_fallback_to_polling(self): - """onerror must fall back to HTTP polling.""" - assert "_startApprovalFallbackPoll" in MESSAGES_JS, \ - "SSE onerror handler must call _startApprovalFallbackPoll" + "No root-absolute approval EventSource may remain (subpath-mount safety)" def test_fallback_poll_interval(self): - """Fallback polling interval must match v0.50.247's 1500ms cadence.""" + """Approval fallback polling interval must keep the 1500ms cadence.""" assert "1500" in MESSAGES_JS, \ - "Fallback polling interval must be 1500ms to match degraded-mode parity with v0.50.247" + "Approval fallback polling interval must be 1500ms (degraded-mode parity with v0.50.247)" - def test_fallback_closes_eventsource(self): - """onerror handler must close the EventSource before falling back.""" - # The onerror handler should call es.close() - assert "es.close()" in MESSAGES_JS, \ - "onerror handler must close the EventSource before falling back" - - def test_stop_closes_eventsource(self): - """stopApprovalPolling must close EventSource.""" - assert "_approvalEventSource.close()" in MESSAGES_JS, \ - "stopApprovalPolling must close _approvalEventSource" - - def test_health_timer_cleanup(self): - """stopApprovalPolling must clear the SSE health timer.""" - assert "_approvalSSEHealthTimer" in MESSAGES_JS, \ - "SSE health timer must be tracked and cleared in stopApprovalPolling" + def test_stop_defensively_closes_any_eventsource(self): + """stopApprovalPolling must still defensively close a lingering EventSource handle.""" + # The _approvalEventSource var stays declared (always null now) and the + # null-guarded close() remains so any future re-introduction stays safe. + assert "_approvalEventSource" in MESSAGES_JS, \ + "stopApprovalPolling must keep the defensive _approvalEventSource cleanup" # ═══════════════════════════════════════════════════════════════════════════════ diff --git a/tests/test_clarify_sse.py b/tests/test_clarify_sse.py index 8f106c4f..31f775cf 100644 --- a/tests/test_clarify_sse.py +++ b/tests/test_clarify_sse.py @@ -73,35 +73,55 @@ class TestClarifySSERoutesCode: class TestClarifySSEFrontendCode: + """Frontend clarify transport. + + As of #3913 the frontend no longer opens a clarify-stream EventSource + (it was one of six persistent SSE streams that exhausted the browser's + 6-per-origin HTTP/1.1 pool). ``startClarifyPolling`` now routes straight + to the HTTP fallback poller. The backend SSE route stays for compatibility + (tests above); these pin the poll-only frontend against regression. + """ + @pytest.fixture(autouse=True) def _load_js(self): self.js = _read(_MESSAGES) - def test_uses_event_source(self): - assert "new EventSource" in self.js - assert "api/clarify/stream" in self.js - assert "EventSource('/api/clarify/stream" not in self.js + def _clarify_polling_body(self): + start = self.js.index("function startClarifyPolling(") + end = self.js.index("\nfunction ", start + 1) + return self.js[start:end] - def test_frontend_listens_initial_event(self): - assert "'initial'" in self.js or '"initial"' in self.js + def test_does_not_open_clarify_stream(self): + body = self._clarify_polling_body() + assert "api/clarify/stream" not in body, \ + "Frontend must not open the clarify-stream EventSource (conn-pool exhaustion, #3913)" + assert "new EventSource" not in body, \ + "startClarifyPolling must not construct an EventSource — it polls over HTTP now" - def test_frontend_listens_clarify_event(self): - assert "'clarify'" in self.js or '"clarify"' in self.js + def test_routes_directly_to_fallback_poll(self): + body = self._clarify_polling_body() + assert "_startClarifyFallbackPoll(sid)" in body, \ + "startClarifyPolling must route to the HTTP fallback poller" def test_frontend_has_fallback_poll(self): - assert "_startClarifyFallbackPoll" in self.js or "clarifyFallbackTimer" in self.js + assert "_startClarifyFallbackPoll" in self.js or "_clarifyFallbackTimer" in self.js + + def test_fallback_poll_hits_pending_endpoint(self): + assert 'api("/api/clarify/pending?session_id="' in self.js, \ + "Clarify fallback poll must query /api/clarify/pending" + assert "EventSource('/api/clarify/stream" not in self.js, \ + "No root-absolute clarify EventSource may remain (subpath-mount safety)" def test_frontend_fallback_interval_3s(self): # Fallback poll interval should be 3000ms assert "3000" in self.js - def test_frontend_stop_closes_event_source(self): + def test_frontend_stop_defensively_closes_event_source(self): + # _clarifyEventSource stays declared (always null now) with a + # null-guarded close() so any future re-introduction stays safe. assert "_clarifyEventSource" in self.js assert ".close()" in self.js - def test_frontend_has_health_timer(self): - assert "_clarifyHealthTimer" in self.js - # ══════════════════════════════════════════════════════════════════════════════ # 2. Unit tests — import clarify module directly diff --git a/tests/test_issue660.py b/tests/test_issue660.py index b26a5c04..a0df483f 100644 --- a/tests/test_issue660.py +++ b/tests/test_issue660.py @@ -1,8 +1,9 @@ """ -Tests for #660: session queue persistence across page refresh. +Tests for session queue persistence across page refresh and tab restore. -The queue is stored to sessionStorage when entries are added/removed, -and restored from sessionStorage on session load when the agent is idle. +#660 introduced sessionStorage persistence. #3108 hardens it by mirroring queue +state to localStorage and restoring from the durable copy when sessionStorage is +missing after browser tab/process restore. """ import pathlib @@ -14,35 +15,62 @@ sess_src = SESSIONS_JS.read_text(encoding='utf-8') class TestQueuePersistence: - """queueSessionMessage persists to sessionStorage.""" + """queueSessionMessage persists through the shared dual-storage helper.""" - def test_queue_writes_to_session_storage(self): - """queueSessionMessage must write to sessionStorage after enqueueing.""" - assert "sessionStorage.setItem('hermes-queue-'+sid" in ui_src + def test_queue_storage_helpers_exist(self): + """Queue persistence must be centralized so write/delete paths stay symmetric.""" + assert "function _queueStorageKey(sid)" in ui_src + assert "function _persistSessionQueueStorage(sid, queue)" in ui_src + assert "function _readPersistedSessionQueue(sid)" in ui_src + assert "function _clearPersistedSessionQueue(sid)" in ui_src + + def test_queue_writes_to_session_and_local_storage(self): + """queueSessionMessage must mirror queue state to sessionStorage and localStorage.""" + helper_start = ui_src.find("function _persistSessionQueueStorage(sid, queue)") + helper_end = ui_src.find("function _readPersistedSessionQueue(sid)", helper_start) + assert helper_start != -1 and helper_end != -1, "_persistSessionQueueStorage helper not found" + helper = ui_src[helper_start:helper_end] + assert "sessionStorage.setItem(key,payload)" in helper + assert "localStorage.setItem(key,payload)" in helper def test_queue_stamps_queued_at_timestamp(self): """Each queue entry must have a _queued_at timestamp for stale-entry detection.""" assert '_queued_at' in ui_src - def test_shift_removes_from_session_storage(self): - """shiftQueuedSessionMessage must remove/update sessionStorage on dequeue.""" - assert "sessionStorage.removeItem('hermes-queue-'+sid)" in ui_src + def test_shift_uses_shared_persist_and_clear_helpers(self): + """shiftQueuedSessionMessage must update/remove both storage layers through helpers.""" + start = ui_src.find("function shiftQueuedSessionMessage(sid)") + end = ui_src.find("function getQueuedSessionCount(sid)", start) + assert start != -1 and end != -1, "shiftQueuedSessionMessage block not found" + body = ui_src[start:end] + assert "_clearPersistedSessionQueue(sid)" in body + assert "_persistSessionQueueStorage(sid,q)" in body - def test_shift_updates_session_storage_when_items_remain(self): - """When queue still has items after shift, sessionStorage is updated (not removed).""" - # After shift: if queue still has items, update storage with remaining - assert "sessionStorage.setItem('hermes-queue-'+sid, JSON.stringify(q))" in ui_src - # Counts: should appear in both add and update paths (2 occurrences minimum) - count = ui_src.count("sessionStorage.setItem('hermes-queue-'+sid") - assert count >= 2, f"Expected >=2 sessionStorage.setItem calls, found {count}" + def test_queue_card_edit_paths_use_shared_helpers(self): + """Queue edit/combine/delete paths must not leave localStorage stale.""" + assert "_saveAndRefresh()" in ui_src + assert "_persistSessionQueueStorage(sid,liveQ)" in ui_src + assert "_clearPersistedSessionQueue(sid)" in ui_src class TestQueueRestore: - """Queue is restored from sessionStorage on session load when agent is idle.""" + """Queue is restored from the shared storage helper on idle session load.""" - def test_restore_reads_session_storage(self): - """sessions.js must read from sessionStorage in the idle-session load path.""" - assert "sessionStorage.getItem('hermes-queue-'+sid)" in sess_src + def test_restore_reads_shared_helper(self): + """sessions.js must use the shared helper so localStorage fallback is reachable.""" + assert "_readPersistedSessionQueue(sid)" in sess_src + + def test_read_helper_falls_back_to_local_storage(self): + """The helper must fall back to localStorage and re-mirror sessionStorage.""" + start = ui_src.find("function _readPersistedSessionQueue(sid)") + end = ui_src.find("function queueSessionMessage(sid", start) + assert start != -1 and end != -1, "_readPersistedSessionQueue block not found" + body = ui_src[start:end] + assert "const sessionValue=read(sessionStorage)" in body + assert "if(sessionValue&&sessionValue.length) return sessionValue;" in body + assert "const localValue=read(localStorage)" in body + assert "if(localValue&&localValue.length)" in body + assert "sessionStorage.setItem(key,JSON.stringify(localValue))" in body def test_restore_uses_timestamp_guard(self): """Stale entries (created before last assistant response) must be dropped.""" @@ -58,19 +86,30 @@ class TestQueueRestore: assert "_msg.value=_first.text" in sess_src def test_restore_clears_stale_storage(self): - """On timestamp mismatch, stale sessionStorage entry is removed.""" - assert "sessionStorage.removeItem('hermes-queue-'+sid)" in sess_src + """On timestamp mismatch, stale queue state is removed from both storage layers.""" + assert "_clearPersistedSessionQueue(sid)" in sess_src def test_restore_wrapped_in_try_catch(self): - """sessionStorage access must be wrapped in try/catch (private browsing may block it).""" - # The restore block must have a catch that clears the bad key - assert "catch(_){sessionStorage.removeItem" in sess_src + """Storage access must be wrapped in try/catch (private browsing may block it).""" + assert "catch(_){if(typeof _clearPersistedSessionQueue==='function') _clearPersistedSessionQueue(sid);}" in sess_src + + def test_delete_session_clears_persisted_queue_after_success(self): + """Deleting a session must clear localStorage-backed queue state after the API succeeds.""" + start = sess_src.find("async function deleteSession(sid, beforeDelete=null)") + end = sess_src.find("// ── Project helpers", start) + assert start != -1 and end != -1, "deleteSession block not found" + body = sess_src[start:end] + clear_pos = body.find("if(typeof _clearPersistedSessionQueue==='function') _clearPersistedSessionQueue(sid);") + error_pos = body.find("if(deleteResult&&deleteResult.error){") + success_pos = body.find("const response=deleteResult&&deleteResult.response;") + assert error_pos != -1 and success_pos != -1 and clear_pos != -1 + assert success_pos < clear_pos, "queue cleanup should run only after delete success" def test_active_session_not_restored_as_draft(self): """When agent is active (INFLIGHT), queue restore must NOT run.""" # The restore block must be inside the else branch (idle path), not the INFLIGHT branch inflight_pos = sess_src.find("if(INFLIGHT[sid]){") - restore_pos = sess_src.find("sessionStorage.getItem('hermes-queue-'") + restore_pos = sess_src.find("_readPersistedSessionQueue(sid)") else_pos = sess_src.find("}else{", inflight_pos) assert restore_pos > else_pos, \ "Queue restore must be inside the else (idle) branch, not the INFLIGHT branch" diff --git a/tests/test_issue_new_chat_draft_restore.py b/tests/test_issue_new_chat_draft_restore.py index ea72c8f9..e6443620 100644 --- a/tests/test_issue_new_chat_draft_restore.py +++ b/tests/test_issue_new_chat_draft_restore.py @@ -110,3 +110,19 @@ def test_clear_composer_draft_forgets_same_new_chat_candidate(): assert "_clearRememberedNewChatDraftSession(sid);" in body, ( "sending a draft must stop New Chat from restoring that now-cleared candidate" ) + + +def test_boot_restore_preserves_zero_message_session_with_composer_draft(): + """A hard refresh should not discard a zero-message session that owns unsent draft text/files.""" + marker = "const _restoredInFlight = S.session && (" + start = BOOT_JS.find(marker) + end = BOOT_JS.find("// Restore the panel from localStorage", start) + assert start != -1 and end != -1, "boot restored-session cleanup block not found" + body = BOOT_JS[start:end] + assert "const _restoredDraft = (S.session && S.session.composer_draft) || {};" in body + assert "const _restoredDraftText = String(_restoredDraft.text||'').trim();" in body + assert "const _restoredDraftFiles = Array.isArray(_restoredDraft.files)" in body + assert "const _restoredHasDraft = !!(_restoredDraftText || _restoredDraftFiles.length);" in body + assert "&& !_restoredInFlight && !_restoredHasDraft" in body, ( + "zero-message restored sessions should only be dropped when they have no draft" + ) diff --git a/tests/test_session_runtime_ownership_invariants.py b/tests/test_session_runtime_ownership_invariants.py index 152d79b1..87816e52 100644 --- a/tests/test_session_runtime_ownership_invariants.py +++ b/tests/test_session_runtime_ownership_invariants.py @@ -125,12 +125,15 @@ class TestSessionOwnedRuntimeInvariants: assert "_clarifyPollingSessionId = sid || null" in fallback, ( "Any clarify fallback poller should retain its owner session id." ) - onerror_idx = start_clarify.index("_clarifyEventSource.onerror") - onerror_body = start_clarify[onerror_idx:start_clarify.index("};", onerror_idx)] - assert "stopClarifyPolling();" not in onerror_body, ( - "SSE fallback must not clear _clarifyPollingSessionId before starting the fallback poller." + # As of #3913 the clarify transport is poll-only (no SSE onerror path): + # startClarifyPolling must route directly to the owner-keyed fallback + # poller, and must not clear the polling session id before doing so. + assert "_startClarifyFallbackPoll(sid)" in start_clarify, ( + "startClarifyPolling must hand off to the owner-keyed fallback poller." + ) + assert "_clarifyEventSource.onerror" not in start_clarify, ( + "SSE onerror path was removed in #3913 — startClarifyPolling polls over HTTP now." ) - assert "_startClarifyFallbackPoll(sid)" in onerror_body def test_live_stream_transport_and_inflight_state_remain_session_keyed(self): messages = read("static/messages.js") diff --git a/tests/test_subpath_frontend_routes.py b/tests/test_subpath_frontend_routes.py index c04e8914..123e7df4 100644 --- a/tests/test_subpath_frontend_routes.py +++ b/tests/test_subpath_frontend_routes.py @@ -50,9 +50,15 @@ def test_direct_frontend_event_sources_are_relative_to_current_mount(): src = read("static/messages.js") assert "EventSource('/api/" not in src assert 'EventSource("/api/' not in src - for endpoint in ("api/approval/stream", "api/clarify/stream", "api/chat/stream"): + # #3913 removed the approval/clarify-stream EventSources (they were 2 of the + # 6 persistent SSE streams exhausting the browser conn pool). The remaining + # long-lived EventSources must still resolve relative to the current mount. + for endpoint in ("api/chat/stream", "api/session/stream"): assert endpoint in src assert "new URL(" in src + # The approval/clarify HTTP poll endpoints must also stay mount-relative. + for endpoint in ("api/approval/pending", "api/clarify/pending"): + assert endpoint in src def test_static_vendor_import_is_relative_to_current_mount():