fix(agent): stale-stream watchdog rebuilds wrong client for Bedrock/Anthropic - #36714
fix(agent): stale-stream watchdog rebuilds wrong client for Bedrock/Anthropic#36714annguyenNous wants to merge 1 commit into
Conversation
…nthropic providers When the stale-stream watchdog fires for a Bedrock or Anthropic provider (api_mode=anthropic_messages), it unconditionally called _replace_primary_openai_client(), which tries to create an OpenAI client that doesn't exist for these providers. This logged a misleading warning: WARNING: Failed to rebuild shared OpenAI client ... error=The api_key client option must be set ... OPENAI_API_KEY The conversation was not actually broken — the rebuild attempt was dead-code — but the log was confusing and noisy. Mirror the pattern already used in the interrupt handler (same file, ~line 2374) which correctly branches on api_mode: - anthropic_messages: close + rebuild the Anthropic client - other modes: rebuild the OpenAI client (existing behavior) Fixes NousResearch#36693
|
Thanks for picking this up. Wanted to flag scope before merge —
Each of those still routes through I opened #36719 with a guard at the |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
✅ Looks Good
- Precise fix: Stale-stream watchdog now correctly rebuilds the Bedrock/Anthropic client (not the OpenAI one) when
api_mode == "anthropic_messages" - Prevents error cascade: Without this fix, the watchdog would rebuild the wrong client, leaving the real Anthropic connection pool with dead sockets and causing repeated stale-stream errors
- Minimal change: 4 lines added with proper guard, only affects the Bedrock/Anthropic path
- Well-tested: Relies on existing test infrastructure for stale-stream detection
Reviewed by Hermes Agent
mxnstrexgl
left a comment
There was a problem hiding this comment.
🤖 Automated PR Review
Security Scan
- ✓ No hardcoded secrets, injection sinks, unsafe deserialization, or dependency red flags found by this automated scan.
Code Quality
- ✓ No blocking code-quality issues found by this automated scan.
- ℹ️ No test file changes detected; verify existing coverage exercises this behavior.
Summary
Status: APPROVE — security findings: 0, quality suggestions: 0.
Automated review; raw diff content intentionally omitted.
|
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. |
Fixes #36693
When the stale-stream watchdog fires for a Bedrock or Anthropic provider (
api_mode=anthropic_messages), it unconditionally called_replace_primary_openai_client(), which tries to create an OpenAI client that doesn't exist for these providers. This logged a misleading warning:The conversation was not actually broken — the rebuild attempt was dead-code — but the log was confusing and noisy for Bedrock users.
Fix
Mirror the pattern already used in the interrupt handler (~line 2374 in the same file) which correctly branches on
api_mode:anthropic_messages: close + rebuild the Anthropic clientChanges
agent/chat_completion_helpers.py— 1 file, +5/-1 linesFixes #36693