feat(agents): add LLM agent retry mechanisms with exponential backoff #70
No reviewers
Labels
No labels
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Blocks
#69 LLM Agent Communication Retry Mechanisms with Exponential Backoff
cleveragents/cleveractors-core
Reference
cleveragents/cleveractors-core!70
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/m2-llm-agent-retry-mechanisms"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements configurable retry mechanisms for LLM agent communications with exponential backoff (ADR-2032).
max_retries(default 7) andmax_retry_time(default 60s) config fields to LLM agentsCloses #69
9090483194023977a3c9Requesting changes for one correctness issue in the retry termination logic.
call_with_retry()only checksmax_retry_timeafter a failed LLM call. That means the helper can sleep past the configured retry-time budget and then make one more provider request before it notices the limit. For example, withmax_retry_time=0.75, the current sequence is: initial failure, sleep 0.5s, retry failure, sleep 1.0s, third provider call, then timeout with accumulated wait 1.5s. ADR-2032 D-2 says that once accumulated wait time reaches/exceeds the cap, the library must not make another attempt. With the defaults, this also means the code can wait 63.5s and perform the 7th retry even though the defaultmax_retry_time=60is documented to stop before that final 32s wait/retry.Please check the next backoff interval against the remaining retry-time budget before sleeping and reattempting, then add a Behave scenario that asserts the exact call count / accumulated wait boundary. The existing scenarios only check the final timeout reason, so they miss this extra request.
CI note: the pull request checks were still pending while I reviewed, and local
noxwas unavailable in my environment, so I could not independently runnox -s unit_tests -- features/llm_agent_retry.feature.PR Review: !70 (Ticket #69)
Verdict: Request Changes
The implementation is largely sound and follows the ADR-2032 design closely, but there is one major correctness issue that was already raised by the previous reviewer (
Graa) and remains unresolved. Thecall_with_retry()loop checks themax_retry_timebudget only after the next LLM call has already happened, which violates ADR-2032 D-2's explicit requirement that "When a limit is reached, the library MUST NOT make another attempt." With the defaults (max_retries=7,max_retry_time=60), the loop still waits 63.5 s and performs the 7th retry after the 60 s cap has already been exceeded.Critical Issues
None
Major Issues
1.
call_with_retryallows one extra LLM attempt past themax_retry_timecap (ADR-2032 D-2 violation)File:
src/cleveractors/agents/retry.py, lines 72–115Problem: The termination check is performed inside the
except Exception as e:arm, aftercoro_factory()has already been awaited. The flow is: sleep → next call → on failure check budget. This means a sleep that pushesaccumulated_waitpastmax_retry_timeis still executed, and the attempt following that sleep is also performed. This contradicts ADR-2032 D-2 verbatim: "When a limit is reached, the library MUST NOT make another attempt."Concrete trace with defaults (
max_retries=7,max_retry_time=60.0,_INITIAL_RETRY_DELAY=0.5):0.5 + 1 + 2 + 4 + 8 + 16 = 31.5 s,retry_count = 6.31.5 >= 60→ False, then sleeps another32 s, makingaccumulated_wait = 63.5 s(already past the 60 s cap).max_retry_time=0.75, the same logic produces: sleep 0.5, attempt, sleep 1.0, attempt, then attempt a third time withaccumulated_wait = 1.5 sbefore finally timing out.features/llm_agent_retry.featureonly assert the finalkind/reasonand never the call count or accumulated wait, so this defect passes those scenarios.Recommendation: Add a pre-sleep check. Before the
await asyncio.sleep(wait_time)call, ifmax_retry_time >= 0 and accumulated_wait + wait_time > max_retry_time(or>=, per ADR wording), setshould_stop = True,stop_reason = "max_retry_time_exceeded", and break without sleeping or making the next attempt. Roughly:(Per the maintainers' instruction, the new Behave scenario requested by the previous reviewer to lock in the call-count boundary is out of scope for this review.)
Minor Issues
1.
_build_timeout_msg_partsis named with a misleading underscore prefixsrc/cleveractors/agents/retry.py, line 118error_msg = " ".join(_build_timeout_msg_parts)). The leading underscore conventionally signals "private / unused", which is misleading here.msg_parts(ortimeout_msg_parts).2.
kind="timeout"set unconditionally inexcept LangChainExceptionarm ofprocess_messagesrc/cleveractors/agents/llm.py, lines 1432–1434kind="timeout"for anyLangChainException. Per ADR-2032 D-7,kind="timeout"is reserved for retry-termination errors that also carry areasonof"max_retries_exceeded"or"max_retry_time_exceeded". Setting it for a LangChain error that never reached the retry terminator is semantically misleading and breaks the invariant "everykind='timeout'error has areason". In practice allainvoke()calls go throughcall_with_retry(), so this arm rarely fires; the field is effectively dead-code-but-still-set. Consider whether the LangChain arm should leavekind=""(as it did before the diff) and reservekind="timeout"strictly for the retry exhaustion path.kind="timeout"from this handler, or document the rationale explicitly. The newexcept Exception as eblock (line 1458–1463) already handles the retry-exhaustion case correctly via theisinstance(e, ExecutionError)branch.3. Unrelated rename in
actor_result_token_counting_steps.pyfeatures/steps/actor_result_token_counting_steps.py, line 1890namewas changed from"shared_agent"to"test_agent". Nothing in this PR (or in the issue) depends on the previous name; the rename appears to be a no-op driven by a desire for less misleading naming. It's not harmful, but it is an unrelated drive-by edit inside what should be a focused retry-mechanism PR.Nits
src/cleveractors/agents/llm.py, line 615 —prune_url = _get_provider_url(prune_model, self._credentials)is computed inside the existing broadtry:of_run_pruning_passbut before any ainvoke. That's fine, but note that ifbuild_chat_model()(called just above on line 572) raises,prune_urlis unused. Cosmetic only.docs/adr/ADR-2032-llm-agent-retry-mechanisms.md, D-7 example — the message example mentions(max_retry_time was disabled). The implementation also appends(max_retries was disabled)when applicable. The ADR could be updated to reflect both clauses; purely documentation polish.Summary
The PR correctly adds
max_retries/max_retry_timeconfig fields, wraps all fiveainvoke()sites plus the pruning-pass call, preservesExecutionError.kind/reasonthrough the wrap-and-rethrow chain, plumbs the graph name via aContextVar, and matches the documented error message shape. The integration points (nodes.py,pure_graph.py) are minimal and targeted. The major defect is the previous reviewer's outstanding concern: the time-budget check is performed after the next attempt rather than before the next sleep, allowing the loop to overshoot the documented cap by one full backoff interval. Once that pre-sleep check is added (a small, well-scoped change incall_with_retry), the implementation will satisfy ADR-2032 D-2 and can be merged. The minor items are non-blocking but worth addressing for code-quality hygiene.3d10550d857c36f369007c36f36900b310089f9db310089f9d585772fcc0585772fcc0af54f8c740af54f8c740d85e9ce229PR Review: !70 (Ticket #69)
Verdict: Request Changes
The author force-pushed a rebase of the branch (new SHAs
cebe96aandd85e9cereplace the previous32e9263andaf54f8c), butgit diffof the implementation source (src/cleveractors/agents/retry.py,src/cleveractors/agents/llm.py,src/cleveractors/langgraph/nodes.py,src/cleveractors/langgraph/pure_graph.py) against the prior commit is empty. The only delta is a cosmetic line-wrap infeatures/steps/llm_agent_retry_steps.pyinside theraise httpx.ConnectError(...)body — no semantic change. None of the findings from the previous review have been addressed.Critical Issues
None
Major Issues
1. Pre-sleep check is never reached when
max_retry_timeis the binding constraintsrc/cleveractors/agents/retry.py, lines 93–147 (call_with_retry)except Exceptionarm, so itsaccumulated_wait + wait_time > max_retry_timecomparison is only evaluated after the nextcoro_factory()has already been awaited. Because the initialwait_timeis_INITIAL_RETRY_DELAY = 0.5, the check evaluates toFalsewhenevermax_retry_time ≥ 0.5(since0 + 0.5 > max_retry_time⇒False). The very next attempt is then issued, and only when it fails does the post-sleepaccumulated_wait >= max_retry_timecheck at line 119 actually stop the loop. The default-config case (max_retries=7, max_retry_time=60) happens to work because the 7th iteration triggers the31.5 + 32 > 60clause, but small-but-nonzero time caps like0.75,0.5, or1.0still allow one extracoro_factory()call — which is exactly the scenario Graa originally flagged in their review (commit3d10550d) and which the new commit does not address. ADR-2032 D-2 states verbatim: "When a limit is reached, the library MUST NOT make another attempt."await coro_factory()is invoked, and tighten the pre-attempt comparison to>=(matching D-2's "reaches or exceeds" wording):With this shape, the loop never invokes
coro_factory()once it knows the budget cannot accommodate another attempt — D-2's "MUST NOT make another attempt" is enforced rather than implied.Minor Issues
1. Provider URL is empty on the first failed retry when credentials lack
base_urlsrc/cleveractors/agents/llm.py, lines 469–473_retry_ainvokereadsself._provider_url(which readsself._chat_modeldirectly, bypassing lazy init) before_ = self.chat_modeltriggers_ensure_chat_model(). On a brand-new agent,self._chat_model is Noneand credentials withoutbase_urlcause_get_provider_urlto short-circuit to"". The first retry-timeoutExecutionErrortherefore omits the URL segment of the message. All subsequent calls work correctly.2.
_get_provider_urlonly recognises OpenAI-style URL attribute namessrc/cleveractors/agents/retry.py, lines 52–60("base_url", "openai_api_base")only.ChatAnthropicexposesanthropic_api_url;ChatGoogleGenerativeAIexposes neither. For non-OpenAI providers whose credentials dict lacksbase_url, the URL is empty even after lazy init.str(model).lower()for anapi_url/api_basesubstring. Alternatively, documentbase_urlin credentials as the recommended source of truth.3.
astream()path instream_message()is not protected by retrysrc/cleveractors/agents/llm.py, line 1828self.chat_model.astream(lc_messages)directly with no retry wrapping. The issue description says "configurable retry mechanisms for all LLM agent communications (main agent and pruning agent)", butastream()(no-tools path) is functionally an LLM communication. The tools-configured streaming path is covered via_execute_tool_loop(which uses_retry_ainvoke), so the inconsistency is specifically the no-tools streaming branch.astream(...)incall_with_retry(treating the async iterator factory as the unit of retry), or update ADR-2032 D-5 to formally exclude the no-tools streaming path.4. Negative sentinel validation: any value < 0 disables the guard, not just
-1src/cleveractors/agents/llm.py, lines 327–328;src/cleveractors/agents/retry.py, lines 115, 119, 128, 154, 156max_retries=-2andmax_retry_time=-0.5are treated identically to-1(disabled). The issue acceptance criteria specify "Set to-1to allow infinite retries"; the ADR defines-1as the canonical sentinel. A user typo'ing-7would silently get infinite retries rather than aConfigurationError.LLMAgent.__init__:5. Unrelated cosmetic rename carried over from previous review
features/steps/actor_result_token_counting_steps.py, line 1890"shared_agent"to"test_agent"in the second commit (d85e9ce). Nothing in this PR or the issue depends on the prior name.Nits
src/cleveractors/agents/llm.py, line 692 —prune_url = _get_provider_url(prune_model, self._credentials)is computed inside the broadtry:block but before anyainvoke. Cosmetic.src/cleveractors/langgraph/nodes.py, line 360 —current_graph_name.set(graph_name)returns aTokenthat is discarded. The ContextVar value persists between nodes in the sameasyncio.Taskand is never reset. Cosmetic.docs/adr/ADR-2032-llm-agent-retry-mechanisms.md, D-7 — The example shows(max_retry_time was disabled)but the implementation also appends(max_retries was disabled)symmetrically. Pure documentation polish.src/cleveractors/agents/retry.py, lines 39–48 —httpx.ConnectError,httpx.TimeoutException, andhttpx.RemoteProtocolErrorare all subclasses ofhttpx.HTTPError, so the explicitisinstancecalls before the final catch-all are redundant. Cosmetic.Summary
The force-push was a rebase/cleanup, not a fix:
git diffbetween the previous HEAD (af54f8c) and the new HEAD (d85e9ce) on the implementation source is empty. The only delta is a cosmetic line-wrap infeatures/steps/llm_agent_retry_steps.py. None of the findings from the previous review — most importantly the pre-sleep check that still permits an extracoro_factory()call whenmax_retry_timeis small — have been addressed. The implementation is otherwise sound: Graa's pre-sleep fix is in place for the default-config case, the_build_timeout_msg_partsrename tomsg_partsis clean, and thekind="timeout"leak into theLangChainExceptionarm has been removed. Once the loop is restructured so the stop decision happens before the next attempt (and the pre-attempt comparison is tightened to>=), the implementation will fully satisfy ADR-2032 D-2.d85e9ce22977fd52e8b3PR Review: !70 (Ticket #69)
Verdict: Approve
The implementation correctly satisfies all acceptance criteria in issue #69 and the eight design decisions in ADR-2032. The pre-sleep termination check that was flagged in the two prior review cycles is already in place at
src/cleveractors/agents/retry.py:132-135and prevents the nextcoro_factory()call when the upcoming sleep would push the accumulated wait past the cap — both for small caps like0.75(terminates after 2 calls) and the default case (terminates after 7 calls, 31.5s). The previously-raised minor items (misleading underscore prefix onmsg_parts,kind="timeout"leak into theLangChainExceptionarm, provider-URL ordering, Anthropic URL attribute) are also all addressed. Only minor hygiene items remain, none of which block the merge.Critical Issues
None
Major Issues
None
Minor Issues
1. Sentinel validation accepts any negative value as "disabled"
src/cleveractors/agents/llm.py, lines 327-328-1as the canonical sentinel for unbounded guards, butLLMAgent.__init__does not validate the range. A user who fat-fingersmax_retries=-7ormax_retry_time=-0.5gets the same "disabled" behavior as-1with no warning. The error message will also fail to show "(max_retries was disabled)" because the check is== -1(retry.py:158), so the message is internally inconsistent with the runtime semantics.LLMAgent.__init__:2. No-tools streaming path (
astream()) is not wrapped in retrysrc/cleveractors/agents/llm.py, line 1828process_message()wraps all fiveainvoke()call sites and the pruning pass, and the tools-configured streaming path goes through_execute_tool_loop()(which usesainvoke()internally, so it is protected). The no-tools streaming path instream_message()callsself.chat_model.astream(lc_messages)directly with no retry wrapping. ADR-2032 D-5 says "everyainvoke()or equivalent call made byLLMAgentto the configured LLM provider when processing a message" —astream()is functionally an LLM communication, so this is a scope gap.astream(...)incall_with_retry(treating the async-iterator factory as the unit of retry), or update ADR-2032 D-5 to formally exclude the no-tools streaming path. Worth a quick decision from the author.3.
current_graph_nameContextVar is set but never resetsrc/cleveractors/langgraph/nodes.py, line 360current_graph_name.set(graph_name)returns aTokenthat is discarded, and the value persists for the lifetime of the currentasyncio.Task. If a graph sets the name and a subsequent node in the same task does not (e.g. a non-graph context reusing the same task), a stale graph name will leak into retry error messages.finallyblock (or use the returnedTokenwithcurrent_graph_name.reset(token)to restore the prior value when the node finishes).Nits
src/cleveractors/agents/retry.py, lines 39-48 —httpx.ConnectError,httpx.TimeoutException, andhttpx.RemoteProtocolErrorare all subclasses ofhttpx.HTTPError, so the explicitisinstancechecks before the finalhttpx.HTTPErrorcatch-all are redundant. Cosmetic.docs/adr/ADR-2032-llm-agent-retry-mechanisms.md, D-7 example — the example message shows only(max_retry_time was disabled), but the implementation also appends(max_retries was disabled)symmetrically. Pure documentation polish.src/cleveractors/agents/llm.py, line 692 —prune_url = _get_provider_url(prune_model, self._credentials)is computed inside the broadtry:block of_run_pruning_passbut before anyainvoke; ifbuild_chat_model()(called just above) raises, the work is wasted. Cosmetic.Summary
The PR correctly adds
max_retries/max_retry_timeconfiguration fields with-1sentinel support, wraps all fiveainvoke()sites inprocess_message()and the pruning-pass call, preservesExecutionError.kind/reasonthrough the wrap-and-rethrow chain, plumbs the graph name through aContextVarfor descriptive timeout errors, and matches the documented error message shape. The integration points inlanggraph/nodes.pyandlanggraph/pure_graph.pyare minimal and targeted. The pre-sleep termination check atretry.py:132-135correctly satisfies ADR-2032 D-2's "MUST NOT make another attempt" requirement, and the default-config case terminates after 7 calls / 31.5s (before the 7th sleep of 32s would push past the 60s cap). The remaining items are non-blocking validation/scope polish that can be addressed in a follow-up.@ -0,0 +187,4 @@MUST indicate that the corresponding guard was disabled. For example, if`max_retries` was reached and `max_retry_time == -1`:> ExecutionError(kind="timeout", reason="max_retries_exceeded"): LLMNit: the D-7 example message shows only
(max_retry_time was disabled), but the implementation also appends(max_retries was disabled)symmetrically. Pure documentation polish.@ -323,2 +324,4 @@self._pruning_threshold: int = _raw_threshold if _raw_threshold else 512# Retry mechanism (§4.4.10 / ADR-2032 D-1).self._max_retries: int = int(config.get("max_retries", 7))Minor: the spec specifies
-1as the canonical sentinel for unbounded guards, but this code does not validate the range. A user who fat-fingersmax_retries=-7ormax_retry_time=-0.5gets the same "disabled" behavior as-1with no warning. Consider adding explicit range checks:@ -653,3 +690,3 @@)prune_messages = [_SM(content=system_content)] + user_msgsprune_response = await prune_model.ainvoke(prune_messages)prune_url = _get_provider_url(prune_model, self._credentials)Nit:
prune_urlis computed inside the broadtry:block of_run_pruning_passbut before anyainvoke; ifbuild_chat_model()(called just above) raises, the work is wasted. Cosmetic.Minor: the no-tools streaming path calls
astream()directly with no retry wrapping. ADR-2032 D-5 says "everyainvoke()or equivalent call" —astream()is functionally an LLM communication, so this is a scope gap. Either wrapastream(...)incall_with_retry(treating the async-iterator factory as the unit of retry), or update ADR-2032 D-5 to formally exclude the no-tools streaming path.@ -0,0 +36,4 @@Non-HTTP errors (e.g. ``BadRequestError``, ``AuthenticationError``,``ValueError``) are raised immediately so the caller fails fast."""if isinstance(e, httpx.TimeoutException):Nit:
httpx.ConnectError,httpx.TimeoutException, andhttpx.RemoteProtocolErrorare all subclasses ofhttpx.HTTPError, so the explicitisinstancechecks before the finalhttpx.HTTPErrorcatch-all are redundant. Cosmetic.@ -356,1 +357,4 @@last_token_usage_var.set((0, 0))graph_name = state.metadata.get("graph_name", "")if graph_name:current_graph_name.set(graph_name)Minor:
current_graph_name.set(graph_name)returns aTokenthat is discarded, and the value persists for the lifetime of the currentasyncio.Task. If a subsequent node in the same task does not set the name, a stale graph name will leak into retry error messages. Consider resetting the ContextVar in afinallyblock (or usecurrent_graph_name.reset(token)to restore the prior value when the node finishes).77fd52e8b3fd72b4b77ffd72b4b77f064a6ce0c2PR Review: cleveragents/cleveractors-core!70 (Ticket #69)
Verdict: Approve
The implementation correctly satisfies all ten acceptance criteria from issue #69 and the eight design decisions in ADR-2032. The pre-sleep termination check that was flagged in earlier review cycles is now in place at
src/cleveractors/agents/retry.py:132-135and prevents the nextcoro_factory()call when the upcoming sleep would push the accumulated wait past the cap. All fiveainvoke()call sites inprocess_message()plus the pruning pass are wrapped, thecurrent_graph_nameContextVaris properly reset in afinallyblock (src/cleveractors/langgraph/nodes.py:414-416), and the no-toolsstream_message()path now wraps the initialastream()connection. Sentinel validation for< -1values raisesConfigurationError. The remaining item below is a small behavioral asymmetry that does not block the merge.Critical Issues
None
Major Issues
None
Minor Issues
1.
stream_messagedropskind/reasonwhen wrapping anExecutionError(asymmetric withprocess_message)File:
src/cleveractors/agents/llm.py, lines 1978-1999 (stream_message'sexcept Exceptionarm)Problem:
process_message()preserves the innerExecutionError'skind/reasonon wrap (lines 1610-1615):stream_message()does not mirror this preservation. Itsexcept Exception as earm unconditionally does:Consequently, when the streaming path triggers a retry timeout — either through
_execute_tool_loop()(re-raises_tle.cause, which is the retryExecutionError) or through_start_stream()— the outer wrap discardskind="timeout"andreason="max_retries_exceeded"/"max_retry_time_exceeded". The caller seeskind="", defeating programmatic error classification and contradicting the CHANGELOG claim that "ExecutionError.kind/reasonare preserved through the exception chain".Recommendation: Mirror the
process_messagepattern instream_message(and theLangChainExceptionarm can be left as-is, sinceLangChainExceptionis not anExecutionError):Nits
None additional beyond the cosmetic items raised in earlier reviews (httpx isinstance redundancy in
retry.py:39-48, ADR D-7 example wording,prune_urlplacement in_run_pruning_pass) — those were implicitly accepted as "won't fix in this PR" by the most recent APPROVED review fromhurui200320(review #9721), so they are not re-raised here per the scope-control rule.Summary
The PR correctly adds
max_retries/max_retry_timeconfiguration fields with-1sentinel support, wraps all fiveainvoke()sites inprocess_message()and the pruning-pass call, plumbs the graph name through aContextVarfor descriptive timeout errors, and matches the documented error message shape. The integration points inlanggraph/nodes.pyandlanggraph/pure_graph.pyare minimal and targeted. The pre-sleep termination check atretry.py:132-135correctly satisfies ADR-2032 D-2's "MUST NOT make another attempt" requirement — the default-config case terminates after 6 retries / 31.5s of accumulated wait (before the 7th 32s sleep would push past the 60s cap), and small caps like0.5terminate after the first failed attempt without a redundantcoro_factory()call. The defaultmax_retries=7, max_retry_time=60interaction is explicitly acknowledged in the ADR's "Negative / Risks" section. The only real issue I found that the previous reviewers did not catch is the asymmetrickind/reasonpreservation betweenprocess_messageandstream_message, which is a Minor (non-blocking) concern — recommend addressing in this PR or a quick follow-up to align with the CHANGELOG's contract.064a6ce0c25e4c5ac8f95e4c5ac8f952451b2550