From dd543e4175e66216bbf6145841c102be963a0ca3 Mon Sep 17 00:00:00 2001 From: Michael Lam Date: Tue, 12 May 2026 02:52:43 -0700 Subject: [PATCH] fix: purge missing inflight sessions --- static/sessions.js | 16 ++- tests/test_inflight_purge_missing_sessions.py | 124 ++++++++++++++++++ tests/test_issue2066_stale_sidebar_spinner.py | 13 +- 3 files changed, 148 insertions(+), 5 deletions(-) create mode 100644 tests/test_inflight_purge_missing_sessions.py diff --git a/static/sessions.js b/static/sessions.js index 89d7e792..cafab5fa 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -246,7 +246,10 @@ function _isSessionEffectivelyStreaming(s) { function _purgeStaleInflightEntries() { // Clean up INFLIGHT entries for sessions the server confirms are NOT // streaming. This prevents the in-memory cache from growing unbounded - // when streams end abnormally. (#2066) + // when streams end abnormally. (#2066) Additionally, any INFLIGHT entry + // whose session id is no longer present in the current _allSessions list + // (deleted / archived / filtered out) is also removed so that ghost entries + // from deleted sessions do not accumulate. (#2092) if (typeof INFLIGHT !== 'object' || !INFLIGHT) return; const sessionsById = new Map(); if (Array.isArray(_allSessions)) { @@ -255,11 +258,20 @@ function _purgeStaleInflightEntries() { } } for (const sid of Object.keys(INFLIGHT)) { + if (!sessionsById.has(sid)) { + // Session is absent from _allSessions — it was deleted / archived / + // filtered and can never stream again, so drop the entry. + delete INFLIGHT[sid]; + if (typeof clearInflightState === 'function') clearInflightState(sid); + continue; + } const s = sessionsById.get(sid); - if (s && !s.is_streaming) { + if (!s.is_streaming) { + // Session exists but is not streaming — purge it. delete INFLIGHT[sid]; if (typeof clearInflightState === 'function') clearInflightState(sid); } + // Sessions that exist and are still streaming are preserved. } } diff --git a/tests/test_inflight_purge_missing_sessions.py b/tests/test_inflight_purge_missing_sessions.py new file mode 100644 index 00000000..a8dcb077 --- /dev/null +++ b/tests/test_inflight_purge_missing_sessions.py @@ -0,0 +1,124 @@ +# Regression tests for _purgeStaleInflightEntries ghost-entry leak (#2092). +# +# When a session is deleted / archived / filtered out of the sidebar list, +# _allSessions no longer contains it. Previously _purgeStaleInflightEntries() +# only deleted an INFLIGHT entry when the session WAS present and was not +# streaming, leaving ghost entries for absent sessions indefinitely. The fix +# adds an explicit check: if the sid is absent from _allSessions, the entry is +# always removed. +# +# These are source-level / parse-time regression tests using the same pattern +# as test_inflight_stream_reuse.py. They verify the function body contains the +# correct guard logic and would break if the fix regresses. +from pathlib import Path + +REPO_ROOT = Path(__file__).parent.parent +SESSIONS_JS = (REPO_ROOT / 'static' / 'sessions.js').read_text(encoding='utf-8') + + +def _function_body(src: str, name: str) -> str: + marker = f'function {name}(' + start = src.find(marker) + assert start != -1, f'{name}() not found in sessions.js' + # Find the opening { of the function body. After the ')' of the parameter + # list there may be whitespace (space, newline) before '{'. We handle both + # `){` and `) \n{` cases so this works whether or not the source uses a + # newline between the closing paren and the brace. + rparen = src.find(')', start) + assert rparen != -1, f'{name}() closing paren not found' + brace = src.find('{', rparen) + assert brace != -1, f'{name}() body brace not found' + depth = 1 + i = brace + 1 + while i < len(src) and depth: + if src[i] == '{': + depth += 1 + elif src[i] == '}': + depth -= 1 + i += 1 + assert depth == 0, f'{name}() body did not close' + return src[brace + 1:i - 1] + + +def test_purge_removes_entry_when_sid_is_absent_from_all_sessions(): + r'''An INFLIGHT entry whose sid is missing from _allSessions must be removed. + + The original bug: the loop condition was `if (s && !s.is_streaming)`. + When sid was absent, `sessionsById.get(sid)` returned undefined, + the `s &&` guard short-circuited, and no deletion occurred. + The fix adds an explicit `if (!sessionsById.has(sid))` branch before + the streaming check, so missing sessions are always purged. + ''' + body = _function_body(SESSIONS_JS, '_purgeStaleInflightEntries') + + # The function must check whether the sid exists in the sessions map. + assert 'sessionsById.has(sid)' in body, ( + '_purgeStaleInflightEntries() must check sessionsById.has(sid) ' + 'to catch sessions absent from _allSessions' + ) + + # There must be a branch that deletes INFLIGHT[sid] for missing sessions. + # It should appear before the `!s.is_streaming` check so that missing + # sessions are always cleaned regardless of their streaming state. + has_check_pos = body.find('sessionsById.has(sid)') + assert has_check_pos != -1 + + # The deletion for absent sessions must be unconditional (no !s.is_streaming guard). + # Walk forward from the has() check and verify delete appears without a streaming guard. + segment = body[has_check_pos:] + # Find the closing of the outer if block (the next unindented '}' or end of body). + # Simpler: check the first occurrence of 'delete INFLIGHT[sid]' after has() and + # verify the intervening code does NOT contain 'is_streaming' before that delete. + first_delete = segment.find('delete INFLIGHT[sid]') + assert first_delete != -1, 'No delete INFLIGHT[sid] found after sessionsById.has(sid)' + between = segment[:first_delete] + assert 'is_streaming' not in between, ( + 'delete INFLIGHT[sid] for absent sessions must not be guarded by is_streaming' + ) + + +def test_purge_removes_entry_when_sid_present_but_not_streaming(): + r'''An INFLIGHT entry for a session present in _allSessions with + is_streaming:false must also be removed (existing behaviour preserved). + ''' + body = _function_body(SESSIONS_JS, '_purgeStaleInflightEntries') + assert '!s.is_streaming' in body, ( + '_purgeStaleInflightEntries() must still check !s.is_streaming for ' + 'sessions present in _allSessions' + ) + # Verify the delete for the non-streaming case is present. + # The body should contain something like `if (!s.is_streaming) { delete INFLIGHT[sid]; ... }` + ns_pos = body.find('!s.is_streaming') + assert ns_pos != -1 + seg = body[ns_pos:] + delete_in_ns = seg.find('delete INFLIGHT[sid]') + assert delete_in_ns != -1, ( + 'delete INFLIGHT[sid] must follow !s.is_streaming for sessions not streaming' + ) + + +def test_purge_preserves_entry_when_sid_present_and_streaming(): + r'''An INFLIGHT entry for a session present in _allSessions with + is_streaming:true must NOT be deleted. + ''' + body = _function_body(SESSIONS_JS, '_purgeStaleInflightEntries') + + # The non-streaming branch must be an if without an else that deletes. + # If an else block deleted on streaming, the fix would be wrong. + # We verify by checking that the body does NOT contain a pattern like: + # `} else { delete INFLIGHT[sid]; }` immediately after an is_streaming check. + ns_pos = body.find('!s.is_streaming') + assert ns_pos != -1 + # The delete for non-streaming is in the same if block. + # We confirm that there is no unconditional delete outside the two guarded paths. + # Reconstruct the two guarded paths: + # 1. if (!sessionsById.has(sid)) { delete INFLIGHT[sid]; } + # 2. if (!s.is_streaming) { delete INFLIGHT[sid]; } + # After both, there should be no third unguarded delete. + + # Count 'delete INFLIGHT[sid]' — there should be exactly 2 (one per guarded path). + delete_count = body.count('delete INFLIGHT[sid]') + assert delete_count == 2, ( + f'Expected exactly 2 delete INFLIGHT[sid] statements (one per guarded path), ' + f'found {delete_count}. Streaming sessions must not be deleted.' + ) \ No newline at end of file diff --git a/tests/test_issue2066_stale_sidebar_spinner.py b/tests/test_issue2066_stale_sidebar_spinner.py index f1cf82c6..c838dc1c 100644 --- a/tests/test_issue2066_stale_sidebar_spinner.py +++ b/tests/test_issue2066_stale_sidebar_spinner.py @@ -33,7 +33,12 @@ def test_cache_render_purges_stale_non_streaming_inflight_entries(): assert "if (s && s.session_id) sessionsById.set(s.session_id, s);" in purge_block assert "const s = sessionsById.get(sid);" in purge_block assert "_allSessionsById" not in purge_block - assert "if (s && !s.is_streaming)" in purge_block + # Non-streaming sessions that ARE in _allSessions are purged (original #2066 + # semantics). Sessions absent from _allSessions are also purged (adds #2092 + # ghost-entry cleanup); the guard check for !sessionsById.has(sid) must come + # before the non-streaming check for code clarity and correctness. + assert "if (!sessionsById.has(sid))" in purge_block + assert "!s.is_streaming" in purge_block assert "delete INFLIGHT[sid];" in purge_block assert "clearInflightState(sid);" in purge_block assert "_purgeStaleInflightEntries();" in render_block @@ -62,10 +67,12 @@ console.log(JSON.stringify({{inflight: INFLIGHT, cleared}})); result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True) payload = json.loads(result.stdout) + # With #2092, sessions absent from _allSessions (like `unknown-session`) + # are also purged and have clearInflightState called for them. `done-session` + # remains in _allSessions with is_streaming=false so it is still purged too. assert payload == { "inflight": { "running-session": True, - "unknown-session": True, }, - "cleared": ["done-session"], + "cleared": sorted(["unknown-session", "done-session"]), }