fix(esc): mid-stream cancellation for OpenAI-compatible + Minimax providers - #145
Merged
Merged
Conversation
…viders PR #144 added abort-signal-aware streaming for ``AnthropicProvider``, but ``OpenAICompatibleProvider`` and ``MinimaxProvider`` got only the pre-call fast-path. Users running through LiteLLM / GLM / OpenAI / DeepSeek — the most common "OpenAI-compatible proxy → Claude" stack — still saw ESC wait the full model latency before the post-API abort check fired. Same 20+ second symptom from before #144. Port the response-close listener pattern from #144's AnthropicProvider: * Register a listener on the abort signal that calls ``stream.response.close()`` to close the underlying HTTP socket. Closes interrupt the SDK's blocking next-chunk read so the iterator raises immediately, even when the model is in a multi-second gap between chunks (extended thinking, tool_use generation). * For OpenAI-compatible providers, additionally add an in-loop ``if abort_signal.aborted: break`` check at the top of each ``for chunk in stream`` iteration. Covers the case where chunks arrive back-to-back fast enough that the listener's close lands one iteration late, or where the SDK has already prefetched chunks past the close point. * Signal-state-authoritative exception translation in the ``except Exception`` block — different SDK versions raise different exception classes when the response is closed mid-read, so the signal is the only stable abort indicator. * Register-then-recheck ordering closes the sub-microsecond race where ``_fire`` can snapshot the listener list and silently drop a freshly-appended listener. * ``finally`` block detaches the listener so long-lived controllers (the REPL engine's, reused across many turns) don't accumulate dead listeners. Minimax wraps the anthropic SDK against its compatible endpoint, so it gets the AnthropicProvider treatment (no in-loop check — the ``with client.messages.stream(...) as stream:`` pattern only exposes ``text_stream``, not a generic iterator). Ten regression tests pin the contract: * ``test_openai_compat_abort_signal.py`` (6 tests) — pre-abort fast-path with leaf-level ``assert_not_called()``, mid-stream close via response.close + timing bound, **load-bearing** in-loop check (asserts ``on_text_chunk`` saw only "first" not "second" — mutation-verified by deleting the in-loop check and watching the test fail), normal-completion regression check, ``abort_signal=None`` legacy parity, listener detachment. * ``test_minimax_abort_signal.py`` (4 tests) — same shape as AnthropicProvider, with ``_ensure_client`` as the fast-path sentinel. Three-way duplication of the close-listener pattern (Anthropic, Minimax, OpenAI-compat) is acknowledged. Extracting a shared helper is left as a follow-up — the three providers' surrounding contexts differ enough (Anthropic has the watchdog + non-streaming fallback, OpenAI-compat has bare-iterator semantics, Minimax has the ``with``-block + ``get_final_message``) that a premature extraction would either grow the helper to a 4-knob API or leak abstraction. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5 tasks
5 tasks
singlaamitesh
pushed a commit
to singlaamitesh/clawcodex
that referenced
this pull request
Jul 7, 2026
…l-openai-compatible fix(esc): mid-stream cancellation for OpenAI-compatible + Minimax providers
singlaamitesh
pushed a commit
to singlaamitesh/clawcodex
that referenced
this pull request
Jul 7, 2026
…ncel PRs agentforce314#144 and agentforce314#145 added abort-signal-aware streaming for three providers (Anthropic, Minimax, OpenAI-compatible). The pattern across all three is structurally identical — pre-call fast-path, register- then-recheck listener closing the stream's response on abort, signal- state-authoritative exception translation, listener detachment, post- stream recheck — but the bookkeeping was inlined and triplicated. Adding a fourth provider would have meant a fourth copy. Extract the pattern into ``src/providers/_stream_abort.py``: * ``StreamAbortGuard(abort_signal)`` — one per call. ``abort_signal=None`` makes every method a no-op so providers can use it unconditionally. * ``raise_if_pre_aborted()`` — pre-call fast-path raising ``AbortError``. * ``raise_if_post_aborted()`` — same check at a different boundary. * ``aborted`` property — cheap in-loop check (OpenAI-compat path). * ``reraise_if_aborted(original_exc)`` — translate SDK exceptions to ``AbortError`` via ``raise ... from``; no-op if signal didn't fire. * ``with guard.attach(stream):`` — register a listener that closes ``stream.response`` on abort; detach in ``__exit__``. ``__exit__`` ALSO closes the response if ``signal.aborted`` is True at exit, as a safety net for the race window where ``AbortSignal._fire`` snapshots the listener list, the consumer thread observes ``aborted=True``, breaks out, detaches the listener, and the snapshot's iteration then runs against an empty list — that path would otherwise leak the underlying httpx response open. The close is idempotent on httpx (``if not self.is_closed`` guard) so the double-fire path is safe. Provider line counts: anthropic_provider.py -88 lines minimax_provider.py -73 lines openai_compatible.py -212 lines _stream_abort.py +211 (mostly docstring) Net: providers shrink from 374 lines of abort bookkeeping to 110 (SDK-specific iteration shape only). The provider-level tests from agentforce314#144 and agentforce314#145 (15 tests across three files) pass unmodified, proving behavior preservation end-to-end. Seventeen new unit tests in ``tests/test_stream_abort_guard.py`` pin the helper's contract directly: pre/post fast-paths, ``aborted`` property semantics, ``attach`` lifecycle (register, detach, race- recovery via register-then-recheck), close-failure tolerance, no- response-attribute graceful degradation, ``reraise_if_aborted`` translation + cause chaining + no-op, and — critically — the ``__exit__`` close-on-abort safety net. The safety-net test mutation-verified: removing the ``__exit__`` close branch makes the test fail with the exact expected assertion. The OpenAI-compat in-loop test gains a ``stream.response.close.called`` assertion to pin the close at the provider level too. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
singlaamitesh
pushed a commit
to singlaamitesh/clawcodex
that referenced
this pull request
Jul 7, 2026
PRs agentforce314#145 / agentforce314#146 added mid-stream ESC cancellation for OpenAI-compatible providers via a ``stream.response.close()`` listener. That works for direct connections, but a user on LiteLLM-proxied Claude Opus 4.7 reported it still hangs indefinitely — ``response.close()`` from the listener thread doesn't actually terminate the SDK's blocking socket read in that configuration, so the iterator keeps draining bytes and the agent loop's cancel boundary never sees the abort. Two earlier attempts confirmed the diagnosis: * Adding ``signal.pthread_kill(main_thread, SIGINT)`` to the listener DID unblock the iteration but propagated ``KeyboardInterrupt`` uncaught past every layer of exception handling, exiting the entire ``python -m src.cli`` process. The signal hits the main thread at the next bytecode boundary, which is rarely inside our narrow ``except KeyboardInterrupt`` window. * The TypeScript reference at ``typescript/src/services/api/openaiShim.ts`` works because JavaScript's native ``fetch + AbortSignal`` integration makes ``reader.read()`` reject with ``AbortError`` when the signal trips. Python's sync ``httpx`` has no equivalent. Fix: decouple the main thread's response time from the SDK's cooperation. The SDK iteration runs on a daemon worker thread that pushes chunks into a ``queue.Queue``. The main thread polls with a 100 ms timeout and re-checks ``guard.aborted`` between ticks. On abort, raise ``AbortError`` immediately and orphan the worker — it dies when the underlying connection eventually closes. The cost is some wasted bandwidth on the orphaned read; the benefit is that ESC unwinds within ~100 ms regardless of LiteLLM / httpx / chunked- transfer behavior. Specifics: * Worker pushes each chunk into the queue, then a ``_DONE`` sentinel in a ``finally`` so the main thread can break the loop on either normal exhaustion or worker exception. * Worker catches ``BaseException`` so ``KeyboardInterrupt`` / ``SystemExit`` from the worker thread are routed back to the main thread for re-raise rather than silently dropped. * Main-thread loop checks abort in the ``Queue.Empty`` branch (bounds ESC latency at one tick) AND after processing each chunk (preserves the chunk we just received, matching the in-loop-check semantics pinned by the existing chunk-list regression test). * Abort sites call ``guard.raise_if_post_aborted()`` instead of hardcoding ``AbortError("user_interrupt")`` so a non-default abort reason on the signal (future ``"rate_limit_backoff"`` etc.) is preserved. * Updated method docstring explains the worker indirection and the Python-vs-JS-fetch contrast so the next engineer touching this doesn't "simplify" the queue away. Two new regression tests: * ``test_abort_unwinds_promptly_even_when_iterator_never_returns`` uses a ``_StuckStream`` whose ``__iter__`` blocks on a never-set Event. Without the worker+queue, the main thread would block on ``next(stream)`` forever; mutation-verified by reverting to the pre-fix code and watching pytest hang past an 8-second gtimeout. * ``test_normal_completion_still_captures_final_usage`` pins the drain invariant: the main thread must process the final empty-choices/populated-usage chunk before breaking on ``_DONE``, otherwise OpenAI's ``stream_options.include_usage=True`` token counts would silently regress. Anthropic and Minimax providers are unchanged. They use the ``anthropic`` SDK whose ``client.messages.stream(...)`` works reliably against direct connections, and no user has reported the LiteLLM-style proxy buffering against them. If that changes, the same worker+queue pattern ports cleanly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
1 participant
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
AnthropicProvider, butOpenAICompatibleProviderandMinimaxProvidergot only the pre-call fast-path. Users on LiteLLM (OpenAI-compatible) → Claude — the user who reported the lingering 10s ESC pause is on this exact stack — still saw ESC wait the full model latency before the outer query loop's abort check fired.User-reported symptom
After #144 merged, a user testing with LiteLLM → Anthropic Claude Opus 4.7 reported ESC still took ~10s during the model's "Thinking…" phase. The call path went through
openai_compatible.py(LiteLLM exposes an OpenAI-compatible API), notanthropic_provider.py, so #144's listener never registered. This PR closes that gap.Changes
src/providers/openai_compatible.py— two defenses inchat_stream_response:abort_signalviaadd_listener(..., once=True). Callsstream.response.close()to close the underlying httpx socket. The SDK's blocking next-chunk read raises immediately. Handles the user's exact case (long gap between chunks during extended thinking / tool_use generation).if abort_signal.aborted: breakat the top of eachfor chunk in stream:iteration. Catches the SDK-prefetched-chunks case where the listener's close lands one iteration late.except Exceptionblock.finallyblock detaches the listener so long-lived controllers don't accumulate listeners.src/providers/minimax_provider.py— Minimax uses the anthropic SDK against its compatible endpoint, so it gets the AnthropicProvider treatment (response-close listener; no in-loop check needed because thewith ... as stream:only exposestext_stream).tests/test_openai_compat_abort_signal.py(new, 6 tests):client.chat.completions.create(leaf-levelassert_not_called())stream.response.close()was calledon_text_chunkcallback, assertsseen == ["first"]not["first", "second"]. Mutation-tested by deleting the in-loop check and watching the test fail with the exact expected message.abort_signal=Nonelegacy paritytests/test_minimax_abort_signal.py(new, 4 tests): same shape as the Anthropic tests, with_ensure_clientas the fast-path sentinel.Test plan
Follow-up (deferred)
Three-way duplication of the response-close-listener pattern across
AnthropicProvider,MinimaxProvider, andOpenAICompatibleProvider. The three contexts differ enough (Anthropic has the watchdog + non-streaming fallback, OpenAI-compat has bare iterator + in-loop check, Minimax haswith-block +get_final_message) that premature extraction would either grow the helper to a 4-knob API or leak abstraction. Will file as a separate refactor PR.🤖 Generated with Claude Code