Fix: make StreamableHTTP#close and Native::Client#stop exception-safe - #156
Open
raySavignone wants to merge 1 commit into
Open
Fix: make StreamableHTTP#close and Native::Client#stop exception-safe#156raySavignone wants to merge 1 commit into
raySavignone wants to merge 1 commit into
Conversation
Wrap teardown in ensure so cleanup always runs when session termination raises, while the original error still propagates. Prevents leaked SSE threads, HTTPX clients, and per-client HumanInTheLoopRegistry entries in long-lived processes that build short-lived StreamableHTTP clients.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Client teardown isn't exception-safe in two places, so a failing session
DELETEstrands resources. For hosts that build short-lived StreamableHTTP clients inside a long-running process, these accumulate — leaked SSE threads, HTTPX clients, and per-clientHumanInTheLoopRegistryentries (each of which owns a live scheduler thread).StreamableHTTP#closerunsterminate_session → cleanup_sse_resources → cleanup_connectionwith noensure.terminate_sessionraises aTransportErroron a failed sessionDELETE(5xx / network error), skipping the other two steps.Native::Client#stopreleases the registry (and resets state) only after@transport.close, so a raising close skips it.Fix
Wrap the teardown in
ensurein both methods so the remaining cleanup always runs, while the original error still propagates. This matches the gem's existing defensive pattern —close_client,close_all_clients, andHumanInTheLoopRegistry#shutdownalready userescue/ensure.Tests
Added specs that simulate a raising session teardown and assert the SSE/HTTPX resources and the per-client registry are released, and that the original error still propagates.