From 047dfa40a54ad5d3957ab76cdaf013edd2b4b50e Mon Sep 17 00:00:00 2001 From: James Drake Date: Sat, 19 Sep 2026 00:29:07 -0500 Subject: [PATCH] fix(routes): never mask the launch error during cleanup _cleanup_chat_start_launch_failure runs inside the launch-failure handler, but the session re-resolve only caught KeyError. An unreadable or undeserializable session store raises something else, and that escaped the helper, replacing the worker-launch error the caller is about to report and aborting the reset after the registries were already cleared. Contain the resolution-and-reset so the helper cannot raise. The KeyError branch keeps its meaning (deleted session, return without saving); anything else is logged at debug like the existing save() failure path. --- api/routes.py | 52 ++++++++++++------- .../test_issue6869_gateway_launch_failure.py | 34 +++++++++++- 2 files changed, 65 insertions(+), 21 deletions(-) diff --git a/api/routes.py b/api/routes.py index da9537b3..e8054150 100644 --- a/api/routes.py +++ b/api/routes.py @@ -22824,26 +22824,38 @@ def _cleanup_chat_start_launch_failure(session, stream_id: str) -> None: # could resurrect a session deleted while the launch was failing. Same # pattern as the #1533 race fix (routes.py:3077) and the anchor-scene # write guard (routes.py:5140). - with _get_session_agent_lock(session.session_id): - try: - canonical = get_session(session.session_id) - except KeyError: - return # session deleted while the thread launch was failing - if getattr(canonical, "active_stream_id", None) != stream_id: - return # a successor turn already owns the session - canonical.active_stream_id = None - canonical.pending_user_message = None - canonical.pending_attachments = [] - canonical.pending_started_at = None - canonical.pending_user_source = None - try: - canonical.save() - except Exception: - logger.debug( - "Failed to persist chat-start cleanup after worker launch failure for %s", - stream_id, - exc_info=True, - ) + # + # This runs while the original launch failure is being handled, so it must + # never raise. Lock acquisition and session resolution can fail on their own + # (I/O, deserialization), and an escaping error here would mask the launch + # failure the caller is about to report while leaving the reset half done. + try: + with _get_session_agent_lock(session.session_id): + try: + canonical = get_session(session.session_id) + except KeyError: + return # session deleted while the thread launch was failing + if getattr(canonical, "active_stream_id", None) != stream_id: + return # a successor turn already owns the session + canonical.active_stream_id = None + canonical.pending_user_message = None + canonical.pending_attachments = [] + canonical.pending_started_at = None + canonical.pending_user_source = None + try: + canonical.save() + except Exception: + logger.debug( + "Failed to persist chat-start cleanup after worker launch failure for %s", + stream_id, + exc_info=True, + ) + except Exception: + logger.debug( + "Failed to reset session state after worker launch failure for %s", + stream_id, + exc_info=True, + ) def _is_hidden_empty_session(s) -> bool: diff --git a/tests/test_issue6869_gateway_launch_failure.py b/tests/test_issue6869_gateway_launch_failure.py index d4ea0ea0..01563067 100644 --- a/tests/test_issue6869_gateway_launch_failure.py +++ b/tests/test_issue6869_gateway_launch_failure.py @@ -182,4 +182,36 @@ def test_gateway_launch_failure_cleanup_does_not_resurrect_deleted_session(monke assert "old-stream" not in config.STREAMS assert stale.active_stream_id == "old-stream" assert stale.pending_user_message == "pending prompt" - assert saved == [], "cleanup must not save a deleted session" \ No newline at end of file + assert saved == [], "cleanup must not save a deleted session" + + +def test_gateway_launch_failure_cleanup_never_masks_the_launch_error(monkeypatch): + """A resolution failure during cleanup must not replace the launch error. + + Cleanup runs inside the launch-failure handler, so an error it fails to + contain is reported instead of the real launch failure. Resolution can fail + with something other than KeyError when the session store is unreadable or + cannot be deserialized. + """ + saved = [] + + def failing_resolver(sid, metadata_only=False): + raise OSError("session store unreadable") + + monkeypatch.setattr(routes, "get_session", failing_resolver) + stale = _make_session( + "session-launch-resolution-error", + active_stream_id="old-stream", + pending_user_message="pending prompt", + save=lambda: saved.append("stale"), + ) + _register_failed_stream(stale.session_id, "old-stream") + + # Must not raise: the caller is already handling the launch failure. + routes._cleanup_chat_start_launch_failure(stale, "old-stream") + + # The registry half still completed, and nothing was saved. + assert config.session_writeback_owner(stale.session_id) is None + assert "old-stream" not in config.STREAM_SESSION_OWNERS + assert "old-stream" not in config.STREAMS + assert saved == [] \ No newline at end of file