fix(anthropic): abort watchdog TLS sockets from a stranger thread, close from the worker (#67142) - #67210
Conversation
…ose from the worker (NousResearch#67142) The direct-Anthropic stale/interrupt watchdog closed `_anthropic_client` from the outer poll thread — a stranger thread relative to the worker driving `_anthropic_client.messages.stream(...)`. That raced the worker's still-live SSL BIO, and when the kernel recycled the just-freed TLS socket FD into an unrelated `open()` (e.g. `cron/executions.db`) a pending 24-byte TLS application-data record was flushed into that file's header, corrupting a SQLite database — the same shape NousResearch#29507 fixed for the OpenAI path, but on `api_mode == "anthropic_messages"`. Extend the NousResearch#29507 owner-thread contract to the shared Anthropic client: - Add `_abort_anthropic_client()` (companion to `_abort_request_openai_client`): a stranger-thread abort that only `shutdown(SHUT_RDWR)`s the client's pool sockets, never releasing the FD. - Both the non-streaming and streaming loops stamp the worker's thread id and route every Anthropic teardown through an owner-aware helper. From a stranger thread it aborts sockets (unblocking the worker) and defers the close+rebuild; the worker completes the close+rebuild from its own thread (in its retry cleanup, or a finally drain on interrupt), where FD release is safe. This preserves the NousResearch#28161 no-hang behavior (the abort still unblocks a stuck stream promptly and the dead pool is still rebuilt) while ensuring the TLS FD is only released by the thread that owns it.
…7142) Add tests/run_agent/test_67142_anthropic_watchdog_fd_ownership.py covering: - `_abort_anthropic_client` shuts pool sockets down with `SHUT_RDWR` and never closes the socket or the client (FD stays owned by the worker). - The streaming stale-stream and interrupt watchdogs abort from the stranger (poll) thread and never call `close()` there; the worker performs the close()+rebuild from its own thread. - The non-streaming stale watchdog holds the same contract. Update the two existing regressions that unblocked the worker by mocking `_anthropic_client.close()` from the detector — they now unblock via the `_abort_anthropic_client` abort path, matching the new ownership contract while still asserting no OpenAI-primary replace and no 15-minute hang.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for extending the #29507 ownership discipline to the shared Anthropic path. The premise is confirmed on current main: the outer streaming stale and interrupt watchdogs still call the shared-client close/rebuild path at agent/chat_completion_helpers.py:3591-3595 and 3625-3629 (remote main 614dc194).
Problems
- The deferred finalizer closes
agent._anthropic_clientby current attribute lookup (agent/chat_completion_helpers.py:609and2519in PR head), not the instance aborted by the watchdog. The stale non-streaming path returns after onlyt.join(timeout=2.0)(agent/chat_completion_helpers.py:958-972), so a late worker can close/rebuild a client that a subsequent request has begun using. Bind teardown to the call-owned client, or prevent a new shared-client request until the owning worker has drained. - The added suite has non-streaming stale coverage but no non-streaming interrupt case (
tests/run_agent/test_67142_anthropic_watchdog_fd_ownership.py:267-312), although the PR changes that branch too.
Suggested changes
- Add a delayed-worker regression that demonstrates a late finalizer cannot touch a later request's client.
- Add the missing non-streaming interrupt ownership regression.
Automated hermes-sweeper review.
| return | ||
| _anthropic_teardown["rebuild_pending"] = False | ||
| try: | ||
| agent._anthropic_client.close() |
There was a problem hiding this comment.
This must retain the specific client that the watchdog aborted. The stale path returns after a two-second join, so a late worker finalizer can otherwise close whatever agent._anthropic_client a later request has installed or is using.
| @pytest.mark.filterwarnings( | ||
| "ignore::pytest.PytestUnhandledThreadExceptionWarning" | ||
| ) | ||
| def test_stale_call_aborts_from_stranger_thread_and_closes_from_worker( |
There was a problem hiding this comment.
Please add the non-streaming interrupt case as well: this suite covers non-streaming stale handling, but the PR also changes the non-streaming interrupt watchdog branch.
|
Closing as superseded by #67238 (merged), which already fixes #67142 (same class: Anthropic stale/interrupt watchdog closing the shared client from a stranger thread → TLS FD recycled into SQLite). #67238 takes the request-local client approach (salvaging #51688) rather than abort-from-stranger-thread / close-from-worker. Please reopen only if you've reproduced a remaining corruption path on current |
Summary
Fixes #67142 — a direct-Anthropic cron request reproduced the same 24-byte TLS/SQLite FD-reuse corruption shape as #29507, on
api_mode == "anthropic_messages".The outer stale/interrupt watchdog runs on the poll thread — a stranger thread relative to the worker driving
_anthropic_client.messages.stream(...). Onorigin/mainall four outer Anthropic watchdog branches called_anthropic_client.close()from that stranger thread:agent/chat_completion_helpers.pynon-streaming stale watchdog + interruptagent/chat_completion_helpers.pystreaming stale watchdog + interruptClosing the client there races the worker's still-live SSL
BIO. When the kernel recycles the just-freed TLS socket FD into an unrelatedopen()(e.g.cron/executions.db), a pending 24-byte TLS application-data record is flushed into that file, clobbering SQLite header bytes 5..28 and yieldingsqlite3.DatabaseError: file is not a database. Becausecreate_execution()runs before executor dispatch, every subsequent due cron on that profile then fails at the tick boundary.This extends the #29507 owner-thread contract to the shared Anthropic client:
_abort_anthropic_client()(companion to_abort_request_openai_client) — a stranger-thread abort that onlyshutdown(SHUT_RDWR)s the client's httpx pool sockets and never releases the FD.close()+_rebuild_anthropic_client()from its own thread (in its retry cleanup, or afinallydrain on interrupt), where FD release is safe.Both properties requested in the issue hold:
The two in-worker retry-cleanup closes remain worker-owned.
Test plan
scripts/run_tests.sh tests/run_agent/test_67142_anthropic_watchdog_fd_ownership.py— new suite proving the ownership contract (socket-level abort, streaming stale + interrupt, non-streaming stale).scripts/run_tests.sh tests/run_agent/test_28161_anthropic_stream_pool_cleanup.py tests/run_agent/test_stream_stale_circuit_breaker.py tests/run_agent/test_streaming.py— updated regressions still assert no OpenAI-primary replace and no 15-minute hang, now via the abort path.test_tls_fd_recycle_corruption,test_cascading_interrupt_6600,test_stream_interrupt_retry,test_non_stream_stale_timeout,test_bedrock_interrupt_post_worker,test_cron_inline_api_call_62151,test_stream_single_writer_65991,test_partial_stream_finish_reason,test_stream_stale_breaker_reset,test_moa_streaming,test_create_openai_client_reuse— all green.Infographic