fix: close errored stream response in tool loop + correct cache-hit icon re-sync#15
Merged
Merged
Conversation
…con re-sync Two verified residual bugs: 1. Resource leak in _stream_one_round (native tool-call streaming path): when _retryable_request raises HTTPError, it does not close the streaming response on its final raise. _stream_response handles this by caching the body and closing exc.response, but _stream_one_round's HTTPError handler did not, leaking the pooled connection on every failed tool-stream round. Mirror _stream_response's cleanup. 2. Premature icon re-sync stop in pipes() cache-hit path: the guard used len(_icons_synced) < len(_models_cache), but _icons_synced also accumulates orphan model IDs from _sync_orphan_db_icons(), so its size can exceed the catalog count and stop re-syncing before all catalog models are confirmed (leaving some icons on the OWUI default until the 5-min cache TTL forces a full refresh). Check whether any catalog model is still unsynced instead of comparing set sizes. Adds focused regression tests for both. 939 -> 944 tests, all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HuonivE5ofaGiFVUy2Z39E
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
2 participants
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.
Summary
Code audit found two genuine correctness defects (both low severity).
1. Resource leak — errored streaming response not closed in the native-tool loop
_retryable_requestdoes not close the streaming response on its final raise. The plain-stream path_stream_responsecompensates by caching the body and callingexc.response.close(), but the native-tool streaming path_stream_one_roundonly didyield self._format_http_error(exc)— leaking the pooled connection on every failed tool-stream round. Fix mirrors_stream_response's cleanup (cache.content, thenclose()).2. Premature icon re-sync stop on cache hits
The cache-hit re-sync guard compared
len(self._icons_synced) < len(self._models_cache). But_icons_syncedalso accumulates orphan model IDs from_sync_orphan_db_icons()(IDs not in the catalog), so the set size can exceed the catalog count and the comparison becomesFalse— stopping re-sync while some catalog models are still unsynced, leaving their icons on the OWUI default placeholder until the 5-minute cache TTL forces a refresh. Fix checks whether any catalog model id is missing from_icons_synced(ignoring theerrorsentinel) instead of comparing sizes.Tests
Standalone
python test_pipe.py: 944/944 pass (+5 new regression tests, 0 regressions).🤖 Generated with Claude Code
https://claude.ai/code/session_01HuonivE5ofaGiFVUy2Z39E
Generated by Claude Code