fix(ai): bound retries on persistent rate-limit to prevent infinite loop (H1)#1235
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…oop (H1) The main AI loop's RateLimitError handler did `iteration -= 1; sleep(5); continue`, pinning the loop counter so a *persistent* 429 (exhausted quota, billing block, provider throttle) spun forever — bounded only by the 3h K8s Job deadline — holding a worker slot. `call_llm` already retries 429 with exponential backoff (~2/4/8s) before re-raising, so the outer sleep(5) was redundant double-waiting. Fix: stop decrementing `iteration` (let it advance) and add a bounded consecutive-429 guard that hard-aborts with a clear Error after 4 in a row, resetting on any successful call. Removes the redundant outer sleep (backoff is preserved inside call_llm). AuthenticationError / connectivity handling is left intact. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NNjPggRSVZ2xnLb7ZxWP5H
419540b to
4745758
Compare
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.
Finding: H1 (High) — persistent rate-limit causes an effectively infinite loop.
Root cause
In
secator/tasks/ai.py, the main loop's exception handler did, forlitellm.RateLimitError:Decrementing
iterationpins the loop counter, so a persistent 429 (exhausted quota, billing block, provider throttle) never advances towardmax_iterationsand spins forever — bounded only by the 3h K8s Job deadline — holding a worker slot the whole time.call_llminsecator/ai/utils.pyalready retries 429 (up to 3×, ~2/4/8s exponential backoff) before re-raising, so the outersleep(5)was redundant double-waiting.Fix
secator/tasks/ai.py, rate-limit handler region only:iteration -= 1so a 429 lets the iteration advance.rate_limit_streak, mirroring the existingempty_streakpattern): hard-abort with a clearErrorafter 4 consecutive rate-limit failures, then_save_history()+ return. The streak resets to 0 on any successfulcall_llm, so transient throttling is forgiven; only a persistent 429 trips the abort. This bounds termination atmin(4, max_iterations)independent ofmax_iterations(which is user-settable and can be raised by modes / query extensions).sleep(5); backoff is preserved insidecall_llm. Dropped the now-orphanedfrom time import sleepimport.AuthenticationError/APIConnectionErrorhandling left untouched.secator/ai/utils.pycall_llmretry/backoff already provides the spacing between attempts (2/4/8s) and was left as-is — no change needed.Validation
python3 -m py_compile secator/tasks/ai.py secator/ai/utils.py→ OK.tests/unit/test_ai_tokens.py::TestAiRateLimitTermination::test_persistent_rate_limit_aborts_boundedreuses the existing_loop_patchesharness, patchescall_llmto raiseRateLimitErroron every call withmax_iterations=50, drives the real_run_loop, and asserts the loop callscall_llmexactly 4 times (the consecutive-429 cap, far below the 50-iteration budget) and ends with a rate-limit abortError— i.e. it provably terminates instead of looping unbounded. Passes.test_ai_loop.pyguardrail/e2e tests andtest_ai_tokens.py::test_loop_sums_token_usage) are unchanged with vs. without this patch (verified by stash/compare).Extra issues surfaced
Noted in adjacent code; not fixed here (separate PRs / other lanes):
ai-resiliency(not introduced by this PR, flagged for triage): intests/unit/test_ai_loop.py, 9 guardrail/e2e tests fail withAction denied: shell command not approved(e.g.TestGuardrailsAutoMode::test_allowed_command_passes,TestMainLoopAutoE2E::test_multi_turn_auto_loop), andtests/unit/test_ai_tokens.py::TestAiTokenAccountingEndToEnd::test_loop_sums_token_usageasserts 600 but gets 100. These look like guardrail/loop drift, owned by other regions.empty_streak) hard-stops after 3 withreturn(no saved abort Error type distinction) — consistent with this fix's pattern, but worth confirming it's the intended UX.call_llmretryable set (utils.py) lumpsBadRequestErrorand genericAPIErrorinto the same retry/backoff path as transient 429s — a permanently malformed request will still burn 3 backoff attempts (~6s) before surfacing. Candidate to split non-retryable client errors out.query_extensions/ mode configs raisemax_iterationsat runtime — independent of this fix (the new guard is mode-independent), but a reminder that the iteration budget is not a tight upper bound on wall-clock by itself.🤖 Generated with Claude Code
https://claude.ai/code/session_01NNjPggRSVZ2xnLb7ZxWP5H