Skip to content

fix(anthropic): skip primary OpenAI client swap on stale stream retry to prevent 15-min hang - #33855

Closed
sweetcornna wants to merge 1 commit into
NousResearch:mainfrom
sweetcornna:fix/28161-anthropic-stream-stale-replace-hang
Closed

fix(anthropic): skip primary OpenAI client swap on stale stream retry to prevent 15-min hang#33855
sweetcornna wants to merge 1 commit into
NousResearch:mainfrom
sweetcornna:fix/28161-anthropic-stream-stale-replace-hang

Conversation

@sweetcornna

@sweetcornna sweetcornna commented May 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • On api_mode == "anthropic_messages", stream cleanup paths now close and rebuild the Anthropic client instead of swapping the primary OpenAI client.
  • Covers the mid-tool retry cleanup, generic stream retry cleanup, and stale-stream cleanup paths from Anthropic streaming: stale/retry paths call _replace_primary_openai_client, causing 15-min hang on stuck streams #28161.
  • Tracks Anthropic tool_use names from content_block_start so text + tool-use + transient disconnect enters the mid-tool retry path instead of returning a partial-text stub.
  • Rebased onto upstream/main at d0e017bac; latest merge-tree check against b1af653bf remains clean.

Review notes

TDD / verification

  • RED: .venv\Scripts\python.exe -m pytest tests\run_agent\test_streaming.py::TestAnthropicStreamCallbacks::test_anthropic_tool_use_start_marks_partial_tool_for_mid_tool_retry -q --timeout-method=thread failed on current upstream/main; the call returned the partial-stream stub instead of retrying through to the final Anthropic tool-use message.
  • Intermediate check: applying only the head tool-name patch still failed (rebuild_calls == []), proving the Anthropic cleanup parent change was required too.
  • GREEN targeted: .venv\Scripts\python.exe -m pytest tests\run_agent\test_streaming.py::TestAnthropicStreamCallbacks::test_anthropic_stream_parser_valueerror_retries_before_delivery tests\run_agent\test_streaming.py::TestAnthropicStreamCallbacks::test_anthropic_stale_stream_does_not_swap_openai_client tests\run_agent\test_streaming.py::TestAnthropicStreamCallbacks::test_anthropic_tool_use_start_marks_partial_tool_for_mid_tool_retry -q --timeout-method=thread -> 3 passed, 1 existing Discord audioop deprecation warning.
  • Full focused file: .venv\Scripts\python.exe -m pytest tests\run_agent\test_streaming.py -q --timeout-method=thread -> 38 passed, 1 existing Discord audioop deprecation warning.
  • .venv\Scripts\ruff.exe check agent\chat_completion_helpers.py tests\run_agent\test_streaming.py -> passed.
  • .venv\Scripts\python.exe -m py_compile agent\chat_completion_helpers.py tests\run_agent\test_streaming.py -> passed.
  • git diff --check -> passed.
  • git merge-tree --write-tree upstream/main HEAD -> clean (2602ff6df085a817f4ac99d10aa8dff726f74cce).

@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint provider/anthropic Anthropic native Messages API labels May 28, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competing with #28240 — both fix the same three streaming-cleanup sites in chat_completion_helpers.py that unconditionally call _replace_primary_openai_client() for Anthropic api_mode (issue #28161). Maintainer should pick one.

@sweetcornna

sweetcornna commented May 31, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, triage. Comparison with #28240 for dedupe:

Both PRs make the same core fix — on the Anthropic stream-cleanup path, rebuild the Anthropic client (_anthropic_client.close() + _rebuild_anthropic_client()) instead of swapping in the primary OpenAI client (#28161). Credit to #28240 (drift-handler) for raising it first.

What this PR adds: the api_mode == "anthropic_messages" branch at all three cleanup sites in agent/chat_completion_helpers.py that previously called _replace_primary_openai_client() unconditionally (stream_mid_tool_retry_pool_cleanup, stream_retry_pool_cleanup, stale_stream_pool_cleanup), mirroring the existing _interrupt_requested branch — plus regression tests in tests/run_agent/test_streaming.py (the retry test flips mock_replace 1→0 + asserts rebuild, and a new test_anthropic_stale_stream_does_not_swap_openai_client).

#28240 makes the equivalent fix without a test. Either works for me — if maintainers prefer the minimal one, the test here can be cherry-picked. Happy to consolidate whichever is easier to review.

@sweetcornna
sweetcornna force-pushed the fix/28161-anthropic-stream-stale-replace-hang branch from 50d9ead to 3b33e9f Compare June 1, 2026 03:13
@sweetcornna

Copy link
Copy Markdown
Contributor Author

CI note — the red checks are inherited from main, not introduced by this PR.

This branch was just rebased onto the latest main to clear a merge conflict. The failing test (N) jobs reproduce on main itself and live entirely in files this PR does not touch:

  • tests/hermes_cli/test_model_catalog.py::TestManifestMatchesInRepoLists::test_in_repo_lists_match_manifestwebsite/static/api/model-catalog.json is out of sync with the in-repo model lists after a8526a415 ("bump minimax to minimax-m3"), which edited hermes_cli/models.py without regenerating the catalog. Fix on main: python scripts/build_model_catalog.py && git add website/static/api/model-catalog.json.
  • tests/hermes_cli/test_gui_command.py::test_gui_installs_packages_and_launches_desktop_app and ::test_gui_forwards_desktop_environment_overridesStopIteration, also failing on main independent of this PR.

This PR only changes agent/chat_completion_helpers.py (Anthropic-path client rebuild) and tests/run_agent/test_streaming.py, both of which pass.

Happy to rebase again once main is green.

@sweetcornna
sweetcornna force-pushed the fix/28161-anthropic-stream-stale-replace-hang branch from 06e9450 to e85ee15 Compare June 11, 2026 14:11
@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jun 21, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closed in favor of #53926, now merged to main (commit a0b9663).

This was a popular bug — 12 independent PRs fixed the same issue (#28161): the three stream-cleanup paths in the streaming code rebuilt the OpenAI primary client unconditionally, which on Anthropic-native sessions both failed (no OPENAI_API_KEY) and left the wedged stream open, causing the ~15-minute hang.

We salvaged @EloquentBrush0x's #28240 (the earliest dedicated fix for this issue) onto current main, resolved the conflict against the newer request-client cleanup helper, repointed the bug-encoding regression test, and added coverage for the two reachable Anthropic cleanup sites. Verified live: stale stream torn down in <1s instead of ~900s, Anthropic client closed+rebuilt, OpenAI rebuild never called on the Anthropic path.

Thank you for the fix — closing as a duplicate of the merged work. Credit to everyone who reported and fixed this.

#53926

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P1 High — major feature broken, no workaround provider/anthropic Anthropic native Messages API sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants