From c991f360211100303f005ca0584bd9c2e2c36aa0 Mon Sep 17 00:00:00 2001 From: Michael Lam Date: Fri, 15 May 2026 13:30:46 -0700 Subject: [PATCH] fix: clean session attachment and stream recovery leftovers --- api/routes.py | 6 ++++++ api/upload.py | 13 +++++++++---- static/messages.js | 1 + tests/test_sprint1.py | 22 +++++++++++++++++++++- tests/test_streaming_race_fix.py | 11 +++++++++++ 5 files changed, 48 insertions(+), 5 deletions(-) diff --git a/api/routes.py b/api/routes.py index d64ba1f7..2fcc82d4 100644 --- a/api/routes.py +++ b/api/routes.py @@ -4628,6 +4628,12 @@ def handle_post(handler, parsed) -> bool: p.with_suffix('.json.bak').unlink(missing_ok=True) except Exception: logger.debug("Failed to unlink session file %s", p) + try: + from api.upload import _session_attachment_dir + + shutil.rmtree(_session_attachment_dir(sid), ignore_errors=True) + except Exception: + logger.debug("Failed to clean attachment dir for deleted session %s", sid) # Prune the per-session agent lock so deleted sessions don't leak # Lock entries in SESSION_AGENT_LOCKS forever. with SESSION_AGENT_LOCKS_LOCK: diff --git a/api/upload.py b/api/upload.py index 15091969..d85e2f13 100644 --- a/api/upload.py +++ b/api/upload.py @@ -76,10 +76,7 @@ def _attachment_root() -> Path: def _upload_destination(session_id: str, safe_name: str) -> Path: - root = _attachment_root() - dest_dir = (root / _re.sub(r'[^\w.\-]', '_', str(session_id or 'session'))[:120]).resolve() - if not dest_dir.is_relative_to(root): - raise ValueError('Invalid attachment directory') + dest_dir = _session_attachment_dir(session_id) dest_dir.mkdir(parents=True, exist_ok=True) dest = (dest_dir / safe_name).resolve() if not dest.is_relative_to(dest_dir): @@ -87,6 +84,14 @@ def _upload_destination(session_id: str, safe_name: str) -> Path: return dest +def _session_attachment_dir(session_id: str, *, root: Path | None = None) -> Path: + root = (root or _attachment_root()).resolve() + dest_dir = (root / _re.sub(r'[^\w.\-]', '_', str(session_id or 'session'))[:120]).resolve() + if not dest_dir.is_relative_to(root): + raise ValueError('Invalid attachment directory') + return dest_dir + + def handle_upload(handler): import traceback as _tb try: diff --git a/static/messages.js b/static/messages.js index e060486c..1a21d48a 100644 --- a/static/messages.js +++ b/static/messages.js @@ -601,6 +601,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){ function _reattachOrRestoreAfterDeferredStreamError(){ if(_terminalStateReached||_streamFinalized) return; + if((S.session&&S.session.session_id)!==activeSid) return; (async()=>{ try{ if(streamId){ diff --git a/tests/test_sprint1.py b/tests/test_sprint1.py index 12dd9619..9216818c 100644 --- a/tests/test_sprint1.py +++ b/tests/test_sprint1.py @@ -179,6 +179,25 @@ def test_session_delete(): assert e.code in (404, 500), f"Expected 404 or 500, got {e.code}" +def test_session_delete_removes_attachment_inbox(cleanup_test_sessions): + """Deleting a session also removes its chat attachment inbox directory.""" + sid, _ = make_session_tracked(cleanup_test_sessions) + + result, status = post_multipart("/api/upload", {"session_id": sid}, { + "file": ("delete-me.txt", b"temporary attachment") + }) + assert status == 200, f"Upload failed {status}: {result}" + attachment_dir = pathlib.Path(result["path"]).parent + assert attachment_dir.name == sid + assert attachment_dir.exists() + + delete_result, delete_status = post("/api/session/delete", {"session_id": sid}) + + assert delete_status == 200 + assert delete_result.get("ok") is True + assert not attachment_dir.exists() + + def test_session_delete_nonexistent(): """Deleting a nonexistent session should return ok:True (idempotent).""" result, status = post("/api/session/delete", {"session_id": "doesnotexist"}) @@ -321,7 +340,7 @@ def test_upload_text_file(cleanup_test_sessions): def test_upload_respects_attachment_dir_env(monkeypatch, tmp_path): """HERMES_WEBUI_ATTACHMENT_DIR routes chat uploads to a per-session inbox.""" - from api.upload import _upload_destination + from api.upload import _session_attachment_dir, _upload_destination inbox = tmp_path / "attachment-inbox" monkeypatch.setenv("HERMES_WEBUI_ATTACHMENT_DIR", str(inbox)) @@ -330,6 +349,7 @@ def test_upload_respects_attachment_dir_env(monkeypatch, tmp_path): assert dest == inbox.resolve() / "session-123" / "notes.md" assert dest.parent.exists() + assert _session_attachment_dir("session-123") == inbox.resolve() / "session-123" def test_upload_too_large(cleanup_test_sessions): diff --git a/tests/test_streaming_race_fix.py b/tests/test_streaming_race_fix.py index 925eb220..0780b0c0 100644 --- a/tests/test_streaming_race_fix.py +++ b/tests/test_streaming_race_fix.py @@ -173,3 +173,14 @@ class TestReconnectAccumulatorPreservation: assert 'cancelAnimationFrame' in fn, ( "_handleStreamError must cancel any pending rAF before renderMessages() runs" ) + + def test_deferred_stream_recovery_bails_after_session_switch(self): + """Deferred hidden-tab recovery must not reattach an old stream after + the user has switched to a different session in the same tab.""" + src = read('static/messages.js') + m = re.search(r'function _reattachOrRestoreAfterDeferredStreamError\(\)\{.*?\n \}', src, re.DOTALL) + assert m, "_reattachOrRestoreAfterDeferredStreamError not found" + fn = m.group(0) + assert 'S.session&&S.session.session_id' in fn + assert '!==activeSid' in fn + assert fn.index('!==activeSid') < fn.index('api(`/api/chat/stream/status?stream_id=')