diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index 93a41ebf90..7995b59f11 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -272,6 +272,18 @@ _API_CALL_MODULES = frozenset({ "chat_completion_helpers", }) +# Maximum total outer-loop exceptions tolerated within one user turn before +# the loop gives up (#92450). The turn budget is unlimited by default +# (``max_iterations = sys.maxsize``), so the historical "near the limit" +# guard no longer stops a turn whose outer loop keeps raising: permanent +# failures spun at ~64 retries/s, pegged a core, and overwrote the rotated +# agent.log history (days of diagnostic context) within minutes. The inner +# retry/fallback machinery owns transient API recovery and terminates on its +# own; only exceptions that ESCAPE it reach this bound, so the cap can be +# small. Still scaled down by a tiny explicit ``max_iterations`` so a +# manually bounded budget keeps governing. +_MAX_OUTER_LOOP_ERRORS = 8 + def _is_interpreter_shutdown_error(exc: Exception) -> bool: """Check if *exc* is a fatal interpreter-shutdown failure. @@ -1918,6 +1930,8 @@ def run_conversation( failed = False codex_ack_continuations = 0 length_continue_retries = 0 + # Total outer-loop exceptions this turn (#92450) — see _MAX_OUTER_LOOP_ERRORS. + _outer_error_count = 0 truncated_tool_call_retries = 0 truncated_response_parts: List[str] = [] compression_attempts = 0 @@ -8353,6 +8367,11 @@ def run_conversation( break except Exception as e: + # Count every escaped exception against the per-turn bound before + # classification — permanent failures must terminate even when the + # turn budget is unlimited (#92450). + _outer_error_count += 1 + # Phase-aware error classification. The huge outer try/except spans # both the actual API request and all local post-processing of the # returned assistant message. Deterministic local bugs (e.g. @@ -8478,14 +8497,23 @@ def run_conversation( # If we're near the limit, break to avoid infinite loops. # Local processing errors are deterministic — stop immediately - # rather than retrying until the budget is exhausted. + # rather than retrying until the budget is exhausted. Repeated + # outer-loop errors stop after a small per-turn cap: with + # max_iterations now unlimited by default, a permanent failure + # would otherwise spin forever and overwrite the rotated log + # history within minutes (#92450). + _outer_error_cap = min(_MAX_OUTER_LOOP_ERRORS, max(1, agent.max_iterations)) if ( _is_local_processing_error or api_call_count >= agent.max_iterations - 1 + or _outer_error_count >= _outer_error_cap ): if _is_local_processing_error: _turn_exit_reason = f"local_processing_error({error_msg[:80]})" final_response = f"I apologize, but I encountered an error while processing the model response: {error_msg}" + elif _outer_error_count >= _outer_error_cap: + _turn_exit_reason = f"repeated_outer_errors({error_msg[:80]})" + final_response = f"I apologize, but I encountered repeated errors: {error_msg}" else: _turn_exit_reason = f"error_near_max_iterations({error_msg[:80]})" final_response = f"I apologize, but I encountered repeated errors: {error_msg}" diff --git a/run_agent.py b/run_agent.py index 5da066ab18..831a14748f 100644 --- a/run_agent.py +++ b/run_agent.py @@ -3914,6 +3914,13 @@ class AIAgent: + "an error occurred near the iteration limit before a final " "answer. Check the tool output above, then send `continue`." ) + if reason.startswith("repeated_outer_errors"): + return ( + prefix + + "the turn kept failing with repeated errors and was stopped " + "early instead of retrying forever. Check the errors above, " + "then send `continue` to retry." + ) if reason == "pending_tool_result": return ( prefix diff --git a/tests/run_agent/test_92450_outer_error_retry_bound.py b/tests/run_agent/test_92450_outer_error_retry_bound.py new file mode 100644 index 0000000000..240a56d0d0 --- /dev/null +++ b/tests/run_agent/test_92450_outer_error_retry_bound.py @@ -0,0 +1,203 @@ +"""Tests for issue #92450 — outer-loop error retries must be bounded even +when the turn budget (``max_iterations``) is unlimited. + +Before the fix, the outer ``except`` in ``run_conversation`` only left the +loop on a local-processing error (#66267) or when +``api_call_count >= agent.max_iterations - 1``. With the default budget now +unlimited (``sys.maxsize``), a permanent failure that escaped the inner +retry/fallback machinery spun forever (~64 retries/s measured in the issue), +pegged a core, and overwrote days of rotated agent.log history within +minutes. + +Injection points reflect the real escape path: exceptions raised INSIDE the +inner retry loop (transport errors from ``create()``, normalization, +fallbacks, ...) never reach the outer handler — that machinery recovers or +terminates on its own. The outer handler sees failures from the final +response assembly, e.g. ``_build_assistant_message`` (a chat-completion- +helpers callable, so such crashes classify as RETRYABLE, never local — +exactly the spin shape reported in the issue). + +The fix adds a small per-turn cap on total outer-loop exceptions +(``_MAX_OUTER_LOOP_ERRORS``, scaled down by a tiny explicit +``max_iterations``). These tests pin the behavior contract: + +* repeated escaping errors must terminate the turn within the cap; +* a turn that recovers after early escaping failures must complete normally; +* local-processing errors still exit immediately (#66267 unchanged); +* a finite ``max_iterations`` still exhausts via the original near-limit path. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + + +@pytest.fixture() +def loop_agent(): + """AIAgent with a mocked OpenAI client (mirrors test_run_agent's fixture) + so we can stage responses on ``.chat.completions.create``.""" + from run_agent import AIAgent + from tests.run_agent.test_run_agent import _mock_response + + with ( + patch("run_agent.get_tool_definitions", return_value=[]), + patch("run_agent.check_toolset_requirements", return_value={}), + patch("run_agent.OpenAI"), + ): + agent = AIAgent( + api_key="test-key-1234567890", + base_url="https://openrouter.ai/api/v1", + quiet_mode=True, + skip_context_files=True, + skip_memory=True, + ) + agent.client = MagicMock() + agent._cached_system_prompt = "You are helpful." + agent._use_prompt_caching = False + agent.tool_delay = 0 + agent.compression_enabled = False + agent.save_trajectories = False + # Every API call itself SUCCEEDS — #92450's spin happens after the + # response arrives, when final-response assembly keeps crashing. + agent.client.chat.completions.create.side_effect = ( + lambda *a, **k: _mock_response(content="ok", finish_reason="stop") + ) + return agent + + +class _PermanentError(Exception): + """An error type no recovery path classifies as retryable-local.""" + + +def _make_local_frame_raiser(): + """Compile a raiser whose frame filename lives in a local-processing + module, so the production traceback classifier (#66267) sees it as a + deterministic local bug — without mocking away the classifier itself.""" + namespace = {} + code = compile( + "def _raise(exc):\n raise exc\n", + filename="/agent/agent_runtime_helpers.py", + mode="exec", + ) + exec(code, namespace) + return namespace["_raise"] + + +class TestOuterErrorRetryBound: + def test_repeated_escaping_errors_are_bounded(self, loop_agent): + """A permanent final-assembly failure that escapes every retry layer + must end the turn within the per-turn error cap instead of spinning + forever under the unlimited default budget.""" + boom = _PermanentError("permanent assembly contract violation") + + with ( + patch.object( + loop_agent, "_build_assistant_message", side_effect=boom + ), + patch.object(loop_agent, "_persist_session"), + patch.object(loop_agent, "_save_trajectory"), + patch.object(loop_agent, "_cleanup_task_resources"), + ): + result = loop_agent.run_conversation("hello") + + # The bound path returns an apology as final_response with + # failed=False (same contract as the legacy near-limit exit), so the + # meaningful assertions are: bounded call count + dedicated exit + # reason + apology text. + assert result["api_calls"] <= 8, ( + "The loop must give up within the per-turn error bound instead " + "of retrying an unlimited-budget failure forever." + ) + assert result["turn_exit_reason"].startswith("repeated_outer_errors"), ( + f"unexpected exit reason: {result['turn_exit_reason']}" + ) + assert "repeated errors" in (result["final_response"] or "") + + def test_recovery_after_failures_completes_normally(self, loop_agent): + """Early escaping failures followed by success must NOT trip the new + bound — the turn completes and the reply is delivered.""" + good_msg = { + "role": "assistant", + "content": "Recovered fine.", + "finish_reason": "stop", + } + + with ( + patch.object( + loop_agent, + "_build_assistant_message", + side_effect=[ + _PermanentError("transient assembly hiccup"), + _PermanentError("transient assembly hiccup"), + dict(good_msg), + ], + ), + patch.object(loop_agent, "_persist_session"), + patch.object(loop_agent, "_save_trajectory"), + patch.object(loop_agent, "_cleanup_task_resources"), + ): + result = loop_agent.run_conversation("hello") + + # final_response comes from the raw provider content ("ok"), so the + # recovery contract is: clean text_response exit exactly on the 3rd + # call, marked completed — proving the two earlier escapes did not + # trip the new bound. + assert result["completed"] is True, ( + f"turn should recover: exit={result.get('turn_exit_reason')}" + ) + assert result["failed"] is False + assert result["api_calls"] == 3 + assert result["turn_exit_reason"].startswith("text_response"), ( + f"unexpected exit reason: {result['turn_exit_reason']}" + ) + + def test_local_processing_error_still_exits_immediately(self, loop_agent): + """#66267 regression guard: a deterministic local bug exits on first + occurrence — the new counter must not delay or change that exit.""" + raiser = _make_local_frame_raiser() + boom = TypeError("list content fed into a str regex helper") + + with ( + patch.object( + loop_agent, + "_strip_think_blocks", + side_effect=lambda text: raiser(boom), + ), + patch.object(loop_agent, "_persist_session"), + patch.object(loop_agent, "_save_trajectory"), + patch.object(loop_agent, "_cleanup_task_resources"), + ): + result = loop_agent.run_conversation("hello") + + assert result["turn_exit_reason"].startswith("local_processing_error"), ( + f"unexpected exit reason: {result['turn_exit_reason']}" + ) + assert result["api_calls"] == 1, ( + "A local processing error must stop immediately on first " + "occurrence, not consume retries." + ) + + def test_finite_budget_still_governs_near_limit_exit(self, loop_agent): + """With a small explicit max_iterations the ORIGINAL near-limit path + fires (unchanged reason string), not the new repeated-error path.""" + loop_agent.max_iterations = 3 + boom = _PermanentError("permanent assembly failure") + + with ( + patch.object( + loop_agent, "_build_assistant_message", side_effect=boom + ), + patch.object(loop_agent, "_persist_session"), + patch.object(loop_agent, "_save_trajectory"), + patch.object(loop_agent, "_cleanup_task_resources"), + ): + result = loop_agent.run_conversation("hello") + + # Budget of 3 → the legacy guard fires on error #2 (api_call_count 2 + # >= 3 - 1); the new cap would fire at error #3, so the legacy path + # must win here. + assert result["turn_exit_reason"].startswith( + "error_near_max_iterations" + ), f"legacy near-limit path must govern: {result['turn_exit_reason']}"