fix(interrupt/auth): prevent /stop swallow (incl. Bedrock) and empty-provider credential corruption - #60120
Merged
kshitijk4poor merged 3 commits intoJul 7, 2026
Conversation
Two deep bugs found through systematic analysis of the streaming API call and fallback credential subsystems: 1. Interrupt signal loss (chat_completion_helpers.py): When the worker thread exits before the main thread's poll loop checks the interrupt flag (e.g. _call_anthropic() detects the flag and returns None), the while loop exits normally and the InterruptedError is never raised. /stop is silently swallowed. Fix: re-check _interrupt_requested after the while loop exits. 2. Empty provider bypasses credential guard (agent_runtime_helpers.py): recover_with_credential_pool() guards against cross-provider pool swaps with 'if current_provider and pool_provider and current != pool_provider'. When agent.provider is '' (valid unset state from agent_init.py:326), current_provider is falsy, the guard is skipped, and the pool swaps credentials onto an agent with empty provider. This is the root cause of the 'provider= model=' empty-string error. Fix: only skip the guard when pool_provider is empty (unscoped pool), not when agent provider is empty.
…gger The provider-mismatch guard now checks pool_provider and current_provider != pool_provider. MagicMock.provider returns a truthy child mock by default, which would trigger the guard and skip the pool recovery tests. Set pool.provider='' explicitly.
The salvaged fix added a post-worker _interrupt_requested re-check to the main OpenAI/Anthropic streaming poll loop. The Bedrock Converse poll loop (interruptible_streaming_api_call, api_mode='bedrock_converse') has the same bug class: its worker calls stream_converse_with_callbacks(on_interrupt_check= ...), which breaks out of the event loop on interrupt and returns a PARTIAL response WITHOUT raising (bedrock_adapter.py). The worker sets result[ 'response'] and exits with _interrupt_requested still True, so the in-loop raise never fires and the poll loop returns the partial — silently swallowing /stop on Bedrock exactly as it was on the paths the salvaged commit fixed. Add the identical post-worker re-check before the Bedrock loop's return. The non-streaming loop (interruptible_api_call) is structurally immune: its worker's only early return fires off _request_cancelled, which is set by the main loop immediately before it raises in-loop, so no swallow window exists. Guard test flips _interrupt_requested True mid-stream (after the pre-flight check) and asserts InterruptedError is raised; verified RED without the fix (DID NOT RAISE) and GREEN with it.
kshitijk4poor
enabled auto-merge (rebase)
July 7, 2026 09:22
4 tasks
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
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
Fixes two real bugs, salvaged from #60018 by @isheng-eqi (cherry-picked to preserve authorship), plus a follow-up that completes the interrupt bug class.
Scope note: the original PR branch #60018 was cut from an unrelated feature branch and bundled ~17 undisclosed commits (personality prompt #58774, copilot #59845, kanban, cron, file-tool validation, aux TTL, etc.) touching 18 files, while its body describes only 2. This PR salvages only the two in-scope commits that match the stated fix; the unrelated work belongs to separate PRs and is dropped.
Bug 1: /stop silently swallowed on streaming paths
Root cause: In the interruptible poll loops, the worker thread can exit before the main poll loop observes
_interrupt_requested. The worker's stream callback breaks out and returns a partial response without raising, so the in-loopraise InterruptedErrornever fires and the loop returns the partial —/stopis silently swallowed.Fix (original): post-worker
_interrupt_requestedre-check on the main OpenAI/Anthropic streaming loop.Fix (follow-up, this PR): the Bedrock Converse streaming loop has the identical bug class —
stream_converse_with_callbacks(on_interrupt_check=...)breaks and returns a partialSimpleNamespacewithout raising (bedrock_adapter.py). Added the same post-worker re-check there. The non-streaming loop (interruptible_api_call) is structurally immune and intentionally left unchanged: its only early worker return fires off_request_cancelled, which the main loop sets immediately before it raises in-loop, so no swallow window exists.Bug 2: empty provider bypasses credential guard → corrupted agent state
Root cause:
recover_with_credential_pool()guarded cross-provider pool swaps withif current_provider and pool_provider and current_provider != pool_provider. Whenagent.provider == ""(a valid unset state fromagent_init.py:420provider = provider_name or ""),current_provideris falsy, the guard is skipped, and the pool swapsbase_url/api_keyonto the agent without settingprovider(_swap_credentialnever writesagent.provider), leavingprovider="" model="".Fix:
if pool_provider and current_provider != pool_provider— an empty agent provider is now treated as a mismatch, so recovery is skipped (returnsrecovered=False, logs the mismatch) instead of corrupting the agent. This is the correct trade: recovery can't repair a missing provider, so skipping surfaces the error honestly rather than masking it. No sibling swap-site shares the vulnerable truthiness pattern; the customcustom:<name>normalization still holds when the provider is empty.Tests
tests/agent/test_bedrock_interrupt_post_worker.py(new): flips_interrupt_requestedTrue mid-stream (after the pre-flight check) and assertsInterruptedErroris raised on the Bedrock path; verified RED without the fix (DID NOT RAISE) and GREEN with it. Plus a sanity test that a non-interrupted call returns normally (guard doesn't fire spuriously).tests/agent/test_credential_pool_routing.py,tests/run_agent/test_credential_pool_interrupt.py: updated mocks to setpool.provider=""(MagicMock's truthy child would otherwise mask the new guard).581 passed locally (excluding
test_bedrock_runtime_default_region, a pre-existing environment-sensitive region assertion unrelated to this change — fails identically on cleanmain).Supersedes #60018.
Co-authored-by: isheng ishengeqi@163.com