fix(agent): avoid closing shared Anthropic client on interrupt - #51688
fix(agent): avoid closing shared Anthropic client on interrupt#51688raymondyan-zhijie wants to merge 2 commits into
Conversation
Anthropic Messages uses a shared SDK client, unlike the request-local OpenAI-wire clients protected by the existing NousResearch#29507 owner-thread close discipline. The stale/interrupt poll loop was still closing and rebuilding that shared client from a non-owner thread, which can release TLS file descriptors while a worker thread is still unwinding the SSL BIO. If SQLite reuses that descriptor, pending TLS bytes can clobber a database header. Skip shared Anthropic client close/rebuild from non-streaming stale, non-streaming interrupt, and streaming interrupt paths. The in-flight SDK call is left to its timeout or stream context cleanup while the existing OpenAI request-local close path remains unchanged. Add regression coverage for the Anthropic-compatible paths so future interrupt/stale changes do not reintroduce shared client close/rebuild. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Anthropic Messages calls previously used the shared SDK client for the in-flight request. Avoiding shared-client close from the poll thread fixed the FD recycle risk, but left stale and interrupted Anthropic requests running until SDK timeout. Create per-request Anthropic clients for non-streaming and streaming calls, using the same owner-thread close discipline as OpenAI-wire requests. Stranger threads only abort sockets; the owning worker performs the SDK close in its finally block. Streaming Anthropic interrupts now raise before get_final_message() so cancelled streams do not drain to completion. Extend the cascading-interrupt regression tests to assert that the shared client is not closed, request-local clients are aborted/closed, and interrupted streams do not call get_final_message(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the shared-client ownership issue. The premise is still present on current main: non-streaming stale/interrupt paths close agent._anthropic_client at agent/chat_completion_helpers.py:717-719 and :759-761, and streaming interrupt does so at :3109-3111.
Problems
- Current
mainhas three additional shared-Anthropic cleanup sites that this PR does not cover: mid-tool retry atagent/chat_completion_helpers.py:2834-2837, ordinary retry at:2894-2897, and stale-stream cleanup at:3080-3083. All callclose()and_rebuild_anthropic_client()on the shared client. - The non-streaming dispatch was extracted after this PR's base (
agent/chat_completion_helpers.py:449-457, commit5c5dd6b7e), so this needs a current-structure transplant rather than a direct application.
Suggested changes
- Carry the request-local Anthropic ownership discipline through all six current cleanup/interrupt paths and add regression cases for the three streaming retry/stale paths.
Automated hermes-sweeper review.
| exc, | ||
| ) | ||
|
|
||
| def _create_request_anthropic_client(self, *, reason: str) -> Any: |
There was a problem hiding this comment.
When salvaging this request-local client helper onto current main, route every current shared-Anthropic cleanup site through it. Current main still closes/rebuilds the shared client in streaming mid-tool retry, retry, and stale cleanup (agent/chat_completion_helpers.py:2836, 2896, 3082) in addition to the paths changed here.
|
Carried this forward in #67238 for #67142 (the direct-Anthropic
I also pushed the invariant a bit further — the shared Your commits are preserved via |
|
Landed via #67238 (merged) — your commits preserved via |
…tchdog never corrupts SQLite (NousResearch#67142) Direct-Anthropic requests used a single shared _anthropic_client, and the stale/interrupt watchdog closed + rebuilt it from the poll (stranger) thread at four sites (non-streaming stale/interrupt, streaming stale/interrupt). Closing a client whose TLS socket a worker thread was still reading released the FD from a stranger thread; the kernel recycled it under a live SSL BIO, which then wrote a 24-byte TLS record into an unrelated SQLite header (cron/executions.db), bricking every cron on the profile. Same shape as the OpenAI-only NousResearch#29507 fix, but the Anthropic path never got the owner-thread contract. Extend the NousResearch#29507 ownership contract to Anthropic: build a per-request client (_create_request_anthropic_client), register it with the request-client holder tagged by kind, and route _close_request_client_once by kind — a stranger thread only shuts the request client's sockets down (_abort_request_anthropic_client), while the owning worker performs the SDK close (_close_request_anthropic_client). The shared _anthropic_client is now never closed from inside a request (streaming or non-streaming), including the worker retry-cleanup sites, since each attempt builds a fresh request client. The NousResearch#28161 no-hang guarantee is preserved: the poll-thread socket abort unblocks the worker immediately. Salvages the approach from NousResearch#51688 (@raymondyan-zhijie), reimplemented onto current main (non-streaming dispatch was refactored into _dispatch_nonstreaming_api_request; streaming grew _cancel_current_stream_attempt and worker retry-cleanup sites). Tests updated to the request-local mechanism (incl. replacing a banned source-reading test with a behavior test) plus new regression coverage proving the watchdog aborts the request client and never touches the shared client. Co-authored-by: raymondyan-zhijie <32435458+raymondyan-zhijie@users.noreply.github.com>
…owner-thread-abort fix(agent): request-local Anthropic clients so the stale/interrupt watchdog never corrupts SQLite (NousResearch#67142, supersedes NousResearch#51688)
…tchdog never corrupts SQLite (NousResearch#67142) Direct-Anthropic requests used a single shared _anthropic_client, and the stale/interrupt watchdog closed + rebuilt it from the poll (stranger) thread at four sites (non-streaming stale/interrupt, streaming stale/interrupt). Closing a client whose TLS socket a worker thread was still reading released the FD from a stranger thread; the kernel recycled it under a live SSL BIO, which then wrote a 24-byte TLS record into an unrelated SQLite header (cron/executions.db), bricking every cron on the profile. Same shape as the OpenAI-only NousResearch#29507 fix, but the Anthropic path never got the owner-thread contract. Extend the NousResearch#29507 ownership contract to Anthropic: build a per-request client (_create_request_anthropic_client), register it with the request-client holder tagged by kind, and route _close_request_client_once by kind — a stranger thread only shuts the request client's sockets down (_abort_request_anthropic_client), while the owning worker performs the SDK close (_close_request_anthropic_client). The shared _anthropic_client is now never closed from inside a request (streaming or non-streaming), including the worker retry-cleanup sites, since each attempt builds a fresh request client. The NousResearch#28161 no-hang guarantee is preserved: the poll-thread socket abort unblocks the worker immediately. Salvages the approach from NousResearch#51688 (@raymondyan-zhijie), reimplemented onto current main (non-streaming dispatch was refactored into _dispatch_nonstreaming_api_request; streaming grew _cancel_current_stream_attempt and worker retry-cleanup sites). Tests updated to the request-local mechanism (incl. replacing a banned source-reading test with a behavior test) plus new regression coverage proving the watchdog aborts the request client and never touches the shared client. Co-authored-by: raymondyan-zhijie <32435458+raymondyan-zhijie@users.noreply.github.com>
…owner-thread-abort fix(agent): request-local Anthropic clients so the stale/interrupt watchdog never corrupts SQLite (NousResearch#67142, supersedes NousResearch#51688)
…tchdog never corrupts SQLite (NousResearch#67142) Direct-Anthropic requests used a single shared _anthropic_client, and the stale/interrupt watchdog closed + rebuilt it from the poll (stranger) thread at four sites (non-streaming stale/interrupt, streaming stale/interrupt). Closing a client whose TLS socket a worker thread was still reading released the FD from a stranger thread; the kernel recycled it under a live SSL BIO, which then wrote a 24-byte TLS record into an unrelated SQLite header (cron/executions.db), bricking every cron on the profile. Same shape as the OpenAI-only NousResearch#29507 fix, but the Anthropic path never got the owner-thread contract. Extend the NousResearch#29507 ownership contract to Anthropic: build a per-request client (_create_request_anthropic_client), register it with the request-client holder tagged by kind, and route _close_request_client_once by kind — a stranger thread only shuts the request client's sockets down (_abort_request_anthropic_client), while the owning worker performs the SDK close (_close_request_anthropic_client). The shared _anthropic_client is now never closed from inside a request (streaming or non-streaming), including the worker retry-cleanup sites, since each attempt builds a fresh request client. The NousResearch#28161 no-hang guarantee is preserved: the poll-thread socket abort unblocks the worker immediately. Salvages the approach from NousResearch#51688 (@raymondyan-zhijie), reimplemented onto current main (non-streaming dispatch was refactored into _dispatch_nonstreaming_api_request; streaming grew _cancel_current_stream_attempt and worker retry-cleanup sites). Tests updated to the request-local mechanism (incl. replacing a banned source-reading test with a behavior test) plus new regression coverage proving the watchdog aborts the request client and never touches the shared client. Co-authored-by: raymondyan-zhijie <32435458+raymondyan-zhijie@users.noreply.github.com>
…owner-thread-abort fix(agent): request-local Anthropic clients so the stale/interrupt watchdog never corrupts SQLite (NousResearch#67142, supersedes NousResearch#51688)
Anthropic Messages uses a shared SDK client, unlike the request-local OpenAI-wire clients protected by the existing #29507 owner-thread close discipline. The stale/interrupt poll loop was still closing and rebuilding that shared client from a non-owner thread, which can release TLS file descriptors while a worker thread is still unwinding the SSL BIO. If SQLite reuses that descriptor, pending TLS bytes can clobber a database header.
Skip shared Anthropic client close/rebuild from non-streaming stale, non-streaming interrupt, and streaming interrupt paths. The in-flight SDK call is left to its timeout or stream context cleanup while the existing OpenAI request-local close path remains unchanged.
Add regression coverage for the Anthropic-compatible paths so future interrupt/stale changes do not reintroduce shared client close/rebuild.
What does this PR do?
Related Issue
Fixes #
Type of Change
Changes Made
How to Test
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AFor New Skills
hermes --toolsets skills -q "Use the X skill to do Y"Screenshots / Logs