cleveragents/cleveractors-core
0
0
Fork 0
Code Issues 17 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

feat(agents): add LLM agent retry mechanisms with exponential backoff #70

Merged
CoreRasurae merged 2 commits from feature/m2-llm-agent-retry-mechanisms into master 2026-07-03 15:21:51 +00:00
Conversation 5 Commits 2 Files Changed 11
+934 -17
CoreRasurae commented 2026-07-02 20:25:28 +00:00
Member
Copy Link
Copy Source

Implements configurable retry mechanisms for LLM agent communications with exponential backoff (ADR-2032).

  • Adds max_retries (default 7) and max_retry_time (default 60s) config fields to LLM agents
  • Both guards support -1 sentinel to disable that guard
  • All five chat_model.ainvoke() calls wrapped via _retry_ainvoke()
  • Pruning pass ainvoke wrapped via call_with_retry()
  • BDD tests cover all termination scenarios

Closes #69

Implements configurable retry mechanisms for LLM agent communications with exponential backoff (ADR-2032). - Adds `max_retries` (default 7) and `max_retry_time` (default 60s) config fields to LLM agents - Both guards support -1 sentinel to disable that guard - All five chat_model.ainvoke() calls wrapped via _retry_ainvoke() - Pruning pass ainvoke wrapped via call_with_retry() - BDD tests cover all termination scenarios Closes #69
CoreRasurae added 3 commits 2026-07-02 20:25:28 +00:00
feat(agents): add LLM agent retry mechanisms with exponential backoff f3e414b453 
Introduce configurable retry mechanisms for all LLM agent communications
(main agent and pruning agent) with exponential backoff.

- New config fields: max_retries (default 7), max_retry_time (default 60s)
  -1 sentinel disables the corresponding guard (infinite retries/time)
- Exponential backoff: initial 0.5s, doubled each retry
- Termination: whichever comes first (max_retries or max_retry_time),
  with -1 disabling the respective check
- Counter and accumulated wait time reset on success
- Timeout ExecutionError includes URL, actor graph name, and retry stats
- Uniform scope: applies to both main agent and pruning agent

ISSUES CLOSED: #69
feat(agents): add LLM agent retry mechanisms with exponential backoff dd4bf340e5 
Implement configurable retry mechanisms (ADR-2032) for all LLM agent
communications (main agent and pruning agent) with exponential backoff.

- New retry utility (cleveractors.agents.retry): call_with_retry with
  max_retries (default 7) and max_retry_time (default 60s), -1 sentinel
  disables the corresponding guard
- Exponential backoff: initial 0.5s, doubled each retry (D-3)
- Termination: whichever is reached first (max_retries or max_retry_time),
  with -1 disabling the respective check (D-2)
- Retry counter and accumulated wait time reset on success (D-4)
- Timeout raises ExecutionError(kind="timeout") with URL, actor graph
  name, retry count, and accumulated wait time in the message (D-7)
- Same client/connection reused across retries, no re-establishment (D-6)
- Uniform scope: applies to both main agent and pruning agent (D-5)
- Graph name threaded through state metadata and ContextVar for error
  reporting
- BDD tests (7 scenarios) for retry behaviour including -1 sentinel cases

ISSUES CLOSED: #69
feat(agents): add LLM agent retry mechanisms with exponential backoff
CI / lint (pull_request) Successful in 50s
Details
CI / typecheck (pull_request) Successful in 56s
Details
CI / security (pull_request) Successful in 55s
Details
CI / quality (pull_request) Successful in 38s
Details
CI / integration_tests (pull_request) Successful in 1m14s
Details
CI / build (pull_request) Successful in 39s
Details
CI / unit_tests (pull_request) Has started running
Details
CI / coverage (pull_request) Has been cancelled
Details
CI / status-check (pull_request) Has been cancelled
Details
a62d1318ff 
- Added CHANGELOG.md entry under ## [Unreleased] → ### Added
- Documents the retry feature implementation (issue #69, ADR-2032)
CoreRasurae added the
Type
Feature
 label 2026-07-02 21:12:11 +00:00
CoreRasurae added this to the v2.1.0 milestone 2026-07-02 21:12:15 +00:00
CoreRasurae added 1 commit 2026-07-02 21:49:29 +00:00
fix(agents): eagerly init chat model in _retry_ainvoke to preserve ConfigurationError
CI / lint (pull_request) Has been cancelled
Details
CI / typecheck (pull_request) Has been cancelled
Details
CI / security (pull_request) Has been cancelled
Details
CI / quality (pull_request) Has been cancelled
Details
CI / unit_tests (pull_request) Has been cancelled
Details
CI / integration_tests (pull_request) Has been cancelled
Details
CI / coverage (pull_request) Has been cancelled
Details
CI / build (pull_request) Has been cancelled
Details
CI / status-check (pull_request) Has been cancelled
Details
9090483194 
Previously, ConfigurationError from credential validation was raised
inside call_with_retry, where it was caught by the generic except
Exception handler and retried (then wrapped as ExecutionError). This
broke the contract that process_message raises ConfigurationError for
missing credentials.

By eagerly accessing self.chat_model before entering the retry loop,
credential validation failures propagate directly out of _retry_ainvoke
and reach the except ConfigurationError handler in process_message,
preserving the original exception type and message.
CoreRasurae force-pushed feature/m2-llm-agent-retry-mechanisms from 9090483194 to 023977a3c9 2026-07-02 21:54:02 +00:00 Compare
CoreRasurae added 1 commit 2026-07-02 22:36:44 +00:00
test(agents): disable retry in error-path tests to avoid unnecessary delays
CI / lint (pull_request) Successful in 45s
Details
CI / typecheck (pull_request) Successful in 1m4s
Details
CI / security (pull_request) Successful in 1m2s
Details
CI / quality (pull_request) Successful in 44s
Details
CI / unit_tests (pull_request) Successful in 7m44s
Details
CI / integration_tests (pull_request) Successful in 1m20s
Details
CI / build (pull_request) Successful in 46s
Details
CI / coverage (pull_request) Successful in 7m42s
Details
CI / status-check (pull_request) Successful in 4s
Details
3d10550d85 
Pre-existing tests that inject error-raising mock chat models (raising
LangChainException or RuntimeError) were triggering the full
exponential-backoff retry loop (~63.5s per test). With 6 such tests,
this added minutes to the suite.

Set max_retries=0 in each error-path step definition so the retry loop
fails fast on the first attempt, preserving the test contract while
avoiding unnecessary sleep delays.
Graa requested changes 2026-07-02 22:49:31 +00:00
Graa left a comment
First-time contributor
Copy Link
Copy Source

Requesting changes for one correctness issue in the retry termination logic.

call_with_retry() only checks max_retry_time after 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, with max_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 default max_retry_time=60 is 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 nox was unavailable in my environment, so I could not independently run nox -s unit_tests -- features/llm_agent_retry.feature.

Requesting changes for one correctness issue in the retry termination logic. `call_with_retry()` only checks `max_retry_time` after 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, with `max_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 default `max_retry_time=60` is 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 `nox` was unavailable in my environment, so I could not independently run `nox -s unit_tests -- features/llm_agent_retry.feature`.
hurui200320 requested changes 2026-07-03 06:07:57 +00:00
Dismissed
hurui200320 left a comment
Member
Copy Link
Copy Source

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. The call_with_retry() loop checks the max_retry_time budget 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_retry allows one extra LLM attempt past the max_retry_time cap (ADR-2032 D-2 violation)

  • File: src/cleveractors/agents/retry.py, lines 72–115

  • Problem: The termination check is performed inside the except Exception as e: arm, after coro_factory() has already been awaited. The flow is: sleep → next call → on failure check budget. This means a sleep that pushes accumulated_wait past max_retry_time is 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):

    • After 6 sleeps the cumulative wait is 0.5 + 1 + 2 + 4 + 8 + 16 = 31.5 s, retry_count = 6.
    • The loop checks 31.5 >= 60 → False, then sleeps another 32 s, making accumulated_wait = 63.5 s (already past the 60 s cap).
    • The 7th retry (the 8th attempt) is then made; only on its failure does the loop decide to stop.
    • With max_retry_time=0.75, the same logic produces: sleep 0.5, attempt, sleep 1.0, attempt, then attempt a third time with accumulated_wait = 1.5 s before finally timing out.
    • The new feature scenarios in features/llm_agent_retry.feature only assert the final kind/reason and 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, if max_retry_time >= 0 and accumulated_wait + wait_time > max_retry_time (or >=, per ADR wording), set should_stop = True, stop_reason = "max_retry_time_exceeded", and break without sleeping or making the next attempt. Roughly:

    if max_retry_time >= 0 and accumulated_wait + wait_time > max_retry_time:
    should_stop = True
    stop_reason = "max_retry_time_exceeded"

if should_stop:
    break

retry_count += 1
await asyncio.sleep(wait_time)
accumulated_wait += wait_time
wait_time *= 2.0

    (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_parts is named with a misleading underscore prefix

  • File: src/cleveractors/agents/retry.py, line 118
  • Problem: The variable is a regular local that is read a few lines later (error_msg = " ".join(_build_timeout_msg_parts)). The leading underscore conventionally signals "private / unused", which is misleading here.
  • Recommendation: Rename to msg_parts (or timeout_msg_parts).

2. kind="timeout" set unconditionally in except LangChainException arm of process_message

  • File: src/cleveractors/agents/llm.py, lines 1432–1434
  • Problem: This handler now sets kind="timeout" for any LangChainException. Per ADR-2032 D-7, kind="timeout" is reserved for retry-termination errors that also carry a reason of "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 "every kind='timeout' error has a reason". In practice all ainvoke() calls go through call_with_retry(), so this arm rarely fires; the field is effectively dead-code-but-still-set. Consider whether the LangChain arm should leave kind="" (as it did before the diff) and reserve kind="timeout" strictly for the retry exhaustion path.
  • Recommendation: Drop the kind="timeout" from this handler, or document the rationale explicitly. The new except Exception as e block (line 1458–1463) already handles the retry-exhaustion case correctly via the isinstance(e, ExecutionError) branch.

3. Unrelated rename in actor_result_token_counting_steps.py

  • File: features/steps/actor_result_token_counting_steps.py, line 1890
  • Problem: The agent name was 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.
  • Recommendation: If kept, mention it in the commit body so reviewers know it's intentional; otherwise, drop it.

Nits

  • File: src/cleveractors/agents/llm.py, line 615 — prune_url = _get_provider_url(prune_model, self._credentials) is computed inside the existing broad try: of _run_pruning_pass but before any ainvoke. That's fine, but note that if build_chat_model() (called just above on line 572) raises, prune_url is unused. Cosmetic only.
  • File: 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_time config fields, wraps all five ainvoke() sites plus the pruning-pass call, preserves ExecutionError.kind/reason through the wrap-and-rethrow chain, plumbs the graph name via a ContextVar, 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 in call_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.

## 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. The `call_with_retry()` loop checks the `max_retry_time` budget 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_retry` allows one extra LLM attempt past the `max_retry_time` cap (ADR-2032 D-2 violation)** - **File:** `src/cleveractors/agents/retry.py`, lines 72–115 - **Problem:** The termination check is performed inside the `except Exception as e:` arm, **after** `coro_factory()` has already been awaited. The flow is: *sleep → next call → on failure check budget*. This means a sleep that pushes `accumulated_wait` past `max_retry_time` is 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`): - After 6 sleeps the cumulative wait is `0.5 + 1 + 2 + 4 + 8 + 16 = 31.5 s`, `retry_count = 6`. - The loop checks `31.5 >= 60` → False, then sleeps another `32 s`, making `accumulated_wait = 63.5 s` (already past the 60 s cap). - The 7th retry (the 8th attempt) is then made; only on its failure does the loop decide to stop. - With `max_retry_time=0.75`, the same logic produces: sleep 0.5, attempt, sleep 1.0, attempt, then attempt a third time with `accumulated_wait = 1.5 s` before finally timing out. - The new feature scenarios in `features/llm_agent_retry.feature` only assert the final `kind`/`reason` and 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, if `max_retry_time >= 0 and accumulated_wait + wait_time > max_retry_time` (or `>=`, per ADR wording), set `should_stop = True`, `stop_reason = "max_retry_time_exceeded"`, and break without sleeping or making the next attempt. Roughly: ```python if max_retry_time >= 0 and accumulated_wait + wait_time > max_retry_time: should_stop = True stop_reason = "max_retry_time_exceeded" if should_stop: break retry_count += 1 await asyncio.sleep(wait_time) accumulated_wait += wait_time wait_time *= 2.0 ``` (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_parts` is named with a misleading underscore prefix** - **File:** `src/cleveractors/agents/retry.py`, line 118 - **Problem:** The variable is a regular local that is read a few lines later (`error_msg = " ".join(_build_timeout_msg_parts)`). The leading underscore conventionally signals "private / unused", which is misleading here. - **Recommendation:** Rename to `msg_parts` (or `timeout_msg_parts`). **2. `kind="timeout"` set unconditionally in `except LangChainException` arm of `process_message`** - **File:** `src/cleveractors/agents/llm.py`, lines 1432–1434 - **Problem:** This handler now sets `kind="timeout"` for any `LangChainException`. Per ADR-2032 D-7, `kind="timeout"` is reserved for retry-termination errors that also carry a `reason` of `"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 "every `kind='timeout'` error has a `reason`". In practice all `ainvoke()` calls go through `call_with_retry()`, so this arm rarely fires; the field is effectively dead-code-but-still-set. Consider whether the LangChain arm should leave `kind=""` (as it did before the diff) and reserve `kind="timeout"` strictly for the retry exhaustion path. - **Recommendation:** Drop the `kind="timeout"` from this handler, or document the rationale explicitly. The new `except Exception as e` block (line 1458–1463) already handles the retry-exhaustion case correctly via the `isinstance(e, ExecutionError)` branch. **3. Unrelated rename in `actor_result_token_counting_steps.py`** - **File:** `features/steps/actor_result_token_counting_steps.py`, line 1890 - **Problem:** The agent `name` was 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. - **Recommendation:** If kept, mention it in the commit body so reviewers know it's intentional; otherwise, drop it. ### Nits - **File:** `src/cleveractors/agents/llm.py`, line 615 — `prune_url = _get_provider_url(prune_model, self._credentials)` is computed inside the existing broad `try:` of `_run_pruning_pass` but before any ainvoke. That's fine, but note that if `build_chat_model()` (called just above on line 572) raises, `prune_url` is unused. Cosmetic only. - **File:** `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_time` config fields, wraps all five `ainvoke()` sites plus the pruning-pass call, preserves `ExecutionError.kind`/`reason` through the wrap-and-rethrow chain, plumbs the graph name via a `ContextVar`, 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 in `call_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.
CoreRasurae force-pushed feature/m2-llm-agent-retry-mechanisms from 3d10550d85 to 7c36f36900 2026-07-03 10:49:52 +00:00 Compare
CoreRasurae force-pushed feature/m2-llm-agent-retry-mechanisms from 7c36f36900 to b310089f9d 2026-07-03 11:34:41 +00:00 Compare
CoreRasurae force-pushed feature/m2-llm-agent-retry-mechanisms from b310089f9d to 585772fcc0 2026-07-03 11:43:27 +00:00 Compare
CoreRasurae force-pushed feature/m2-llm-agent-retry-mechanisms from 585772fcc0 to af54f8c740 2026-07-03 11:58:44 +00:00 Compare
CoreRasurae force-pushed feature/m2-llm-agent-retry-mechanisms from af54f8c740 to d85e9ce229 2026-07-03 12:29:42 +00:00 Compare
hurui200320 requested changes 2026-07-03 12:41:20 +00:00
Dismissed
hurui200320 left a comment
Member
Copy Link
Copy Source

PR Review: !70 (Ticket #69)

Verdict: Request Changes

The author force-pushed a rebase of the branch (new SHAs cebe96a and d85e9ce replace the previous 32e9263 and af54f8c), but git diff of 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 in features/steps/llm_agent_retry_steps.py inside the raise 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_time is the binding constraint

  • File: src/cleveractors/agents/retry.py, lines 93–147 (call_with_retry)
  • Problem: Unchanged from the previous review. The pre-sleep check (lines 124–131) lives inside the post-failure except Exception arm, so its accumulated_wait + wait_time > max_retry_time comparison is only evaluated after the next coro_factory() has already been awaited. Because the initial wait_time is _INITIAL_RETRY_DELAY = 0.5, the check evaluates to False whenever max_retry_time ≥ 0.5 (since 0 + 0.5 > max_retry_time ⇒ False). The very next attempt is then issued, and only when it fails does the post-sleep accumulated_wait >= max_retry_time check 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 the 31.5 + 32 > 60 clause, but small-but-nonzero time caps like 0.75, 0.5, or 1.0 still allow one extra coro_factory() call — which is exactly the scenario Graa originally flagged in their review (commit 3d10550d) 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."
  • Recommendation: Restructure the loop so the stop decision happens before await coro_factory() is invoked, and tighten the pre-attempt comparison to >= (matching D-2's "reaches or exceeds" wording):
while True:
    # Decide BEFORE attempting whether any further attempt fits the budget.
    stop_reasons: list[str] = []
    if max_retries >= 0 and retry_count >= max_retries:
        stop_reasons.append("max_retries_exceeded")
    if max_retry_time >= 0 and accumulated_wait >= max_retry_time:
        stop_reasons.append("max_retry_time_exceeded")
    if stop_reasons:
        stop_reason = stop_reasons[0]
        break

    try:
        result = await coro_factory()
        retry_count = 0
        accumulated_wait = 0.0
        return result
    except ExecutionError:
        raise
    except Exception as e:
        if not _is_http_comms_error(e):
            raise
        last_exception = e
        # ... logging ...
        retry_count += 1
        # Sleep, but cap the next iteration by checking the projected total.
        if (
            max_retry_time >= 0
            and accumulated_wait + wait_time >= max_retry_time
        ):
            stop_reason = "max_retry_time_exceeded"
            accumulated_wait += wait_time
            break
        await asyncio.sleep(wait_time)
        accumulated_wait += wait_time
        wait_time *= 2.0

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_url

  • File: src/cleveractors/agents/llm.py, lines 469–473
  • Problem: Unchanged. _retry_ainvoke reads self._provider_url (which reads self._chat_model directly, bypassing lazy init) before _ = self.chat_model triggers _ensure_chat_model(). On a brand-new agent, self._chat_model is None and credentials without base_url cause _get_provider_url to short-circuit to "". The first retry-timeout ExecutionError therefore omits the URL segment of the message. All subsequent calls work correctly.
  • Recommendation: Swap the two lines so the warm-up happens first:
_ = self.chat_model  # Force lazy init first
provider_url = self._provider_url  # Now _chat_model is non-None

2. _get_provider_url only recognises OpenAI-style URL attribute names

  • File: src/cleveractors/agents/retry.py, lines 52–60
  • Problem: Unchanged. The fallback loop checks ("base_url", "openai_api_base") only. ChatAnthropic exposes anthropic_api_url; ChatGoogleGenerativeAI exposes neither. For non-OpenAI providers whose credentials dict lacks base_url, the URL is empty even after lazy init.
  • Recommendation: Extend the attribute list, or duck-type str(model).lower() for an api_url/api_base substring. Alternatively, document base_url in credentials as the recommended source of truth.

3. astream() path in stream_message() is not protected by retry

  • File: src/cleveractors/agents/llm.py, line 1828
  • Problem: Unchanged. The no-tools streaming path calls self.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)", but astream() (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.
  • Recommendation: Either wrap astream(...) in call_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 -1

  • File: src/cleveractors/agents/llm.py, lines 327–328; src/cleveractors/agents/retry.py, lines 115, 119, 128, 154, 156
  • Problem: Unchanged. Both max_retries=-2 and max_retry_time=-0.5 are treated identically to -1 (disabled). The issue acceptance criteria specify "Set to -1 to allow infinite retries"; the ADR defines -1 as the canonical sentinel. A user typo'ing -7 would silently get infinite retries rather than a ConfigurationError.
  • Recommendation: Validate in LLMAgent.__init__:
if self._max_retries < -1:
    raise ConfigurationError(
        f"max_retries must be -1 or a non-negative integer, "
        f"got {self._max_retries!r}"
    )
if self._max_retry_time < -1:
    raise ConfigurationError(
        f"max_retry_time must be -1 or a non-negative number, "
        f"got {self._max_retry_time!r}"
    )

5. Unrelated cosmetic rename carried over from previous review

  • File: features/steps/actor_result_token_counting_steps.py, line 1890
  • Problem: Unchanged. Agent name renamed from "shared_agent" to "test_agent" in the second commit (d85e9ce). Nothing in this PR or the issue depends on the prior name.
  • Recommendation: Mention it in the commit body or drop it.

Nits

  • File: src/cleveractors/agents/llm.py, line 692 — prune_url = _get_provider_url(prune_model, self._credentials) is computed inside the broad try: block but before any ainvoke. Cosmetic.
  • File: src/cleveractors/langgraph/nodes.py, line 360 — current_graph_name.set(graph_name) returns a Token that is discarded. The ContextVar value persists between nodes in the same asyncio.Task and is never reset. Cosmetic.
  • File: 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.
  • File: src/cleveractors/agents/retry.py, lines 39–48 — httpx.ConnectError, httpx.TimeoutException, and httpx.RemoteProtocolError are all subclasses of httpx.HTTPError, so the explicit isinstance calls before the final catch-all are redundant. Cosmetic.

Summary

The force-push was a rebase/cleanup, not a fix: git diff between the previous HEAD (af54f8c) and the new HEAD (d85e9ce) on the implementation source is empty. The only delta is a cosmetic line-wrap in features/steps/llm_agent_retry_steps.py. None of the findings from the previous review — most importantly the pre-sleep check that still permits an extra coro_factory() call when max_retry_time is 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_parts rename to msg_parts is clean, and the kind="timeout" leak into the LangChainException arm 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.

## PR Review: !70 (Ticket #69) ### Verdict: **Request Changes** The author force-pushed a rebase of the branch (new SHAs `cebe96a` and `d85e9ce` replace the previous `32e9263` and `af54f8c`), but `git diff` of 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 in `features/steps/llm_agent_retry_steps.py` inside the `raise 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_time` is the binding constraint** - **File:** `src/cleveractors/agents/retry.py`, lines 93–147 (`call_with_retry`) - **Problem:** Unchanged from the previous review. The pre-sleep check (lines 124–131) lives inside the post-failure `except Exception` arm, so its `accumulated_wait + wait_time > max_retry_time` comparison is only evaluated *after* the next `coro_factory()` has already been awaited. Because the initial `wait_time` is `_INITIAL_RETRY_DELAY = 0.5`, the check evaluates to `False` whenever `max_retry_time ≥ 0.5` (since `0 + 0.5 > max_retry_time` ⇒ `False`). The very next attempt is then issued, and only when it fails does the post-sleep `accumulated_wait >= max_retry_time` check 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 the `31.5 + 32 > 60` clause, but small-but-nonzero time caps like `0.75`, `0.5`, or `1.0` still allow one extra `coro_factory()` call — which is exactly the scenario Graa originally flagged in their review (commit `3d10550d`) 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."* - **Recommendation:** Restructure the loop so the stop decision happens *before* `await coro_factory()` is invoked, and tighten the pre-attempt comparison to `>=` (matching D-2's "reaches or exceeds" wording): ```python while True: # Decide BEFORE attempting whether any further attempt fits the budget. stop_reasons: list[str] = [] if max_retries >= 0 and retry_count >= max_retries: stop_reasons.append("max_retries_exceeded") if max_retry_time >= 0 and accumulated_wait >= max_retry_time: stop_reasons.append("max_retry_time_exceeded") if stop_reasons: stop_reason = stop_reasons[0] break try: result = await coro_factory() retry_count = 0 accumulated_wait = 0.0 return result except ExecutionError: raise except Exception as e: if not _is_http_comms_error(e): raise last_exception = e # ... logging ... retry_count += 1 # Sleep, but cap the next iteration by checking the projected total. if ( max_retry_time >= 0 and accumulated_wait + wait_time >= max_retry_time ): stop_reason = "max_retry_time_exceeded" accumulated_wait += wait_time break await asyncio.sleep(wait_time) accumulated_wait += wait_time wait_time *= 2.0 ``` 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_url`** - **File:** `src/cleveractors/agents/llm.py`, lines 469–473 - **Problem:** Unchanged. `_retry_ainvoke` reads `self._provider_url` (which reads `self._chat_model` directly, bypassing lazy init) *before* `_ = self.chat_model` triggers `_ensure_chat_model()`. On a brand-new agent, `self._chat_model is None` and credentials without `base_url` cause `_get_provider_url` to short-circuit to `""`. The first retry-timeout `ExecutionError` therefore omits the URL segment of the message. All subsequent calls work correctly. - **Recommendation:** Swap the two lines so the warm-up happens first: ```python _ = self.chat_model # Force lazy init first provider_url = self._provider_url # Now _chat_model is non-None ``` **2. `_get_provider_url` only recognises OpenAI-style URL attribute names** - **File:** `src/cleveractors/agents/retry.py`, lines 52–60 - **Problem:** Unchanged. The fallback loop checks `("base_url", "openai_api_base")` only. `ChatAnthropic` exposes `anthropic_api_url`; `ChatGoogleGenerativeAI` exposes neither. For non-OpenAI providers whose credentials dict lacks `base_url`, the URL is empty even after lazy init. - **Recommendation:** Extend the attribute list, or duck-type `str(model).lower()` for an `api_url`/`api_base` substring. Alternatively, document `base_url` in credentials as the recommended source of truth. **3. `astream()` path in `stream_message()` is not protected by retry** - **File:** `src/cleveractors/agents/llm.py`, line 1828 - **Problem:** Unchanged. The no-tools streaming path calls `self.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)"*, but `astream()` (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. - **Recommendation:** Either wrap `astream(...)` in `call_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 `-1`** - **File:** `src/cleveractors/agents/llm.py`, lines 327–328; `src/cleveractors/agents/retry.py`, lines 115, 119, 128, 154, 156 - **Problem:** Unchanged. Both `max_retries=-2` and `max_retry_time=-0.5` are treated identically to `-1` (disabled). The issue acceptance criteria specify *"Set to `-1` to allow infinite retries"*; the ADR defines `-1` as the canonical sentinel. A user typo'ing `-7` would silently get infinite retries rather than a `ConfigurationError`. - **Recommendation:** Validate in `LLMAgent.__init__`: ```python if self._max_retries < -1: raise ConfigurationError( f"max_retries must be -1 or a non-negative integer, " f"got {self._max_retries!r}" ) if self._max_retry_time < -1: raise ConfigurationError( f"max_retry_time must be -1 or a non-negative number, " f"got {self._max_retry_time!r}" ) ``` **5. Unrelated cosmetic rename carried over from previous review** - **File:** `features/steps/actor_result_token_counting_steps.py`, line 1890 - **Problem:** Unchanged. Agent name renamed from `"shared_agent"` to `"test_agent"` in the second commit (`d85e9ce`). Nothing in this PR or the issue depends on the prior name. - **Recommendation:** Mention it in the commit body or drop it. ### Nits - **File:** `src/cleveractors/agents/llm.py`, line 692 — `prune_url = _get_provider_url(prune_model, self._credentials)` is computed inside the broad `try:` block but before any `ainvoke`. Cosmetic. - **File:** `src/cleveractors/langgraph/nodes.py`, line 360 — `current_graph_name.set(graph_name)` returns a `Token` that is discarded. The ContextVar value persists between nodes in the same `asyncio.Task` and is never reset. Cosmetic. - **File:** `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. - **File:** `src/cleveractors/agents/retry.py`, lines 39–48 — `httpx.ConnectError`, `httpx.TimeoutException`, and `httpx.RemoteProtocolError` are all subclasses of `httpx.HTTPError`, so the explicit `isinstance` calls before the final catch-all are redundant. Cosmetic. ### Summary The force-push was a rebase/cleanup, not a fix: `git diff` between the previous HEAD (`af54f8c`) and the new HEAD (`d85e9ce`) on the implementation source is empty. The only delta is a cosmetic line-wrap in `features/steps/llm_agent_retry_steps.py`. None of the findings from the previous review — most importantly the pre-sleep check that still permits an extra `coro_factory()` call when `max_retry_time` is 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_parts` rename to `msg_parts` is clean, and the `kind="timeout"` leak into the `LangChainException` arm 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.
CoreRasurae force-pushed feature/m2-llm-agent-retry-mechanisms from d85e9ce229 to 77fd52e8b3 2026-07-03 13:59:44 +00:00 Compare
hurui200320 approved these changes 2026-07-03 14:07:24 +00:00
Dismissed
hurui200320 left a comment
Member
Copy Link
Copy Source

PR 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-135 and prevents the next coro_factory() call when the upcoming sleep would push the accumulated wait past the cap — both for small caps like 0.75 (terminates after 2 calls) and the default case (terminates after 7 calls, 31.5s). The previously-raised minor items (misleading underscore prefix on msg_parts, kind="timeout" leak into the LangChainException arm, 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"

  • File: src/cleveractors/agents/llm.py, lines 327-328
  • Problem: The spec specifies -1 as the canonical sentinel for unbounded guards, but LLMAgent.__init__ does not validate the range. A user who fat-fingers max_retries=-7 or max_retry_time=-0.5 gets the same "disabled" behavior as -1 with 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.
  • Recommendation: Add explicit range checks in LLMAgent.__init__: 
    if self._max_retries < -1:
    raise ConfigurationError(
        f"max_retries must be -1 or a non-negative integer, "
        f"got {self._max_retries!r}"
    )
if self._max_retry_time < -1:
    raise ConfigurationError(
        f"max_retry_time must be -1 or a non-negative number, "
        f"got {self._max_retry_time!r}"
    )

2. No-tools streaming path (astream()) is not wrapped in retry

  • File: src/cleveractors/agents/llm.py, line 1828
  • Problem: process_message() wraps all five ainvoke() call sites and the pruning pass, and the tools-configured streaming path goes through _execute_tool_loop() (which uses ainvoke() internally, so it is protected). The no-tools streaming path in stream_message() calls self.chat_model.astream(lc_messages) directly with no retry wrapping. ADR-2032 D-5 says "every ainvoke() or equivalent call made by LLMAgent to the configured LLM provider when processing a message" — astream() is functionally an LLM communication, so this is a scope gap.
  • Recommendation: Either wrap astream(...) in call_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_name ContextVar is set but never reset

  • File: src/cleveractors/langgraph/nodes.py, line 360
  • Problem: current_graph_name.set(graph_name) returns a Token that is discarded, and the value persists for the lifetime of the current asyncio.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.
  • Recommendation: Reset the ContextVar in a finally block (or use the returned Token with current_graph_name.reset(token) to restore the prior value when the node finishes).

Nits

  • File: src/cleveractors/agents/retry.py, lines 39-48 — httpx.ConnectError, httpx.TimeoutException, and httpx.RemoteProtocolError are all subclasses of httpx.HTTPError, so the explicit isinstance checks before the final httpx.HTTPError catch-all are redundant. Cosmetic.
  • File: 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.
  • File: src/cleveractors/agents/llm.py, line 692 — prune_url = _get_provider_url(prune_model, self._credentials) is computed inside the broad try: block of _run_pruning_pass but before any ainvoke; if build_chat_model() (called just above) raises, the work is wasted. Cosmetic.

Summary

The PR correctly adds max_retries / max_retry_time configuration fields with -1 sentinel support, wraps all five ainvoke() sites in process_message() and the pruning-pass call, preserves ExecutionError.kind/reason through the wrap-and-rethrow chain, plumbs the graph name through a ContextVar for descriptive timeout errors, and matches the documented error message shape. The integration points in langgraph/nodes.py and langgraph/pure_graph.py are minimal and targeted. The pre-sleep termination check at retry.py:132-135 correctly 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.

## PR 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-135` and prevents the next `coro_factory()` call when the upcoming sleep would push the accumulated wait past the cap — both for small caps like `0.75` (terminates after 2 calls) and the default case (terminates after 7 calls, 31.5s). The previously-raised minor items (misleading underscore prefix on `msg_parts`, `kind="timeout"` leak into the `LangChainException` arm, 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"** - **File:** `src/cleveractors/agents/llm.py`, lines 327-328 - **Problem:** The spec specifies `-1` as the canonical sentinel for unbounded guards, but `LLMAgent.__init__` does not validate the range. A user who fat-fingers `max_retries=-7` or `max_retry_time=-0.5` gets the same "disabled" behavior as `-1` with 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. - **Recommendation:** Add explicit range checks in `LLMAgent.__init__`: ```python if self._max_retries < -1: raise ConfigurationError( f"max_retries must be -1 or a non-negative integer, " f"got {self._max_retries!r}" ) if self._max_retry_time < -1: raise ConfigurationError( f"max_retry_time must be -1 or a non-negative number, " f"got {self._max_retry_time!r}" ) ``` **2. No-tools streaming path (`astream()`) is not wrapped in retry** - **File:** `src/cleveractors/agents/llm.py`, line 1828 - **Problem:** `process_message()` wraps all five `ainvoke()` call sites and the pruning pass, and the tools-configured streaming path goes through `_execute_tool_loop()` (which uses `ainvoke()` internally, so it is protected). The no-tools streaming path in `stream_message()` calls `self.chat_model.astream(lc_messages)` directly with no retry wrapping. ADR-2032 D-5 says "every `ainvoke()` or equivalent call made by `LLMAgent` to the configured LLM provider when processing a message" — `astream()` is functionally an LLM communication, so this is a scope gap. - **Recommendation:** Either wrap `astream(...)` in `call_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_name` ContextVar is set but never reset** - **File:** `src/cleveractors/langgraph/nodes.py`, line 360 - **Problem:** `current_graph_name.set(graph_name)` returns a `Token` that is discarded, and the value persists for the lifetime of the current `asyncio.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. - **Recommendation:** Reset the ContextVar in a `finally` block (or use the returned `Token` with `current_graph_name.reset(token)` to restore the prior value when the node finishes). ### Nits - **File:** `src/cleveractors/agents/retry.py`, lines 39-48 — `httpx.ConnectError`, `httpx.TimeoutException`, and `httpx.RemoteProtocolError` are all subclasses of `httpx.HTTPError`, so the explicit `isinstance` checks before the final `httpx.HTTPError` catch-all are redundant. Cosmetic. - **File:** `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. - **File:** `src/cleveractors/agents/llm.py`, line 692 — `prune_url = _get_provider_url(prune_model, self._credentials)` is computed inside the broad `try:` block of `_run_pruning_pass` but before any `ainvoke`; if `build_chat_model()` (called just above) raises, the work is wasted. Cosmetic. ### Summary The PR correctly adds `max_retries` / `max_retry_time` configuration fields with `-1` sentinel support, wraps all five `ainvoke()` sites in `process_message()` and the pruning-pass call, preserves `ExecutionError.kind`/`reason` through the wrap-and-rethrow chain, plumbs the graph name through a `ContextVar` for descriptive timeout errors, and matches the documented error message shape. The integration points in `langgraph/nodes.py` and `langgraph/pure_graph.py` are minimal and targeted. The pre-sleep termination check at `retry.py:132-135` correctly 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.
docs/adr/ADR-2032-llm-agent-retry-mechanisms.md Outdated
@@ -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"): LLM
hurui200320 commented 2026-07-03 14:07:24 +00:00
Member
Copy Link
Copy Source

Nit: 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.

Nit: 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.
src/cleveractors/agents/llm.py Outdated
@@ -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))
hurui200320 commented 2026-07-03 14:07:24 +00:00
Member
Copy Link
Copy Source

Minor: the spec specifies -1 as the canonical sentinel for unbounded guards, but this code does not validate the range. A user who fat-fingers max_retries=-7 or max_retry_time=-0.5 gets the same "disabled" behavior as -1 with no warning. Consider adding explicit range checks:

if self._max_retries < -1:
    raise ConfigurationError(
        f"max_retries must be -1 or a non-negative integer, "
        f"got {self._max_retries!r}"
    )
if self._max_retry_time < -1:
    raise ConfigurationError(
        f"max_retry_time must be -1 or a non-negative number, "
        f"got {self._max_retry_time!r}"
    )
Minor: the spec specifies `-1` as the canonical sentinel for unbounded guards, but this code does not validate the range. A user who fat-fingers `max_retries=-7` or `max_retry_time=-0.5` gets the same "disabled" behavior as `-1` with no warning. Consider adding explicit range checks: ```python if self._max_retries < -1: raise ConfigurationError( f"max_retries must be -1 or a non-negative integer, " f"got {self._max_retries!r}" ) if self._max_retry_time < -1: raise ConfigurationError( f"max_retry_time must be -1 or a non-negative number, " f"got {self._max_retry_time!r}" ) ```
src/cleveractors/agents/llm.py Outdated
@@ -653,3 +690,3 @@
)
prune_messages = [_SM(content=system_content)] + user_msgs
prune_response = await prune_model.ainvoke(prune_messages)
prune_url = _get_provider_url(prune_model, self._credentials)
hurui200320 commented 2026-07-03 14:07:24 +00:00
Member
Copy Link
Copy Source

Nit: prune_url is computed inside the broad try: block of _run_pruning_pass but before any ainvoke; if build_chat_model() (called just above) raises, the work is wasted. Cosmetic.

Nit: `prune_url` is computed inside the broad `try:` block of `_run_pruning_pass` but before any `ainvoke`; if `build_chat_model()` (called just above) raises, the work is wasted. Cosmetic.
src/cleveractors/agents/llm.py Outdated
hurui200320 commented 2026-07-03 14:07:24 +00:00
Member
Copy Link
Copy Source

Minor: the no-tools streaming path calls astream() directly with no retry wrapping. ADR-2032 D-5 says "every ainvoke() or equivalent call" — astream() is functionally an LLM communication, so this is a scope gap. Either wrap astream(...) in call_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.

Minor: the no-tools streaming path calls `astream()` directly with no retry wrapping. ADR-2032 D-5 says "every `ainvoke()` or equivalent call" — `astream()` is functionally an LLM communication, so this is a scope gap. Either wrap `astream(...)` in `call_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.
src/cleveractors/agents/retry.py Outdated
@@ -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):
hurui200320 commented 2026-07-03 14:07:24 +00:00
Member
Copy Link
Copy Source

Nit: httpx.ConnectError, httpx.TimeoutException, and httpx.RemoteProtocolError are all subclasses of httpx.HTTPError, so the explicit isinstance checks before the final httpx.HTTPError catch-all are redundant. Cosmetic.

Nit: `httpx.ConnectError`, `httpx.TimeoutException`, and `httpx.RemoteProtocolError` are all subclasses of `httpx.HTTPError`, so the explicit `isinstance` checks before the final `httpx.HTTPError` catch-all are redundant. Cosmetic.
src/cleveractors/langgraph/nodes.py Outdated
@@ -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)
hurui200320 commented 2026-07-03 14:07:24 +00:00
Member
Copy Link
Copy Source

Minor: current_graph_name.set(graph_name) returns a Token that is discarded, and the value persists for the lifetime of the current asyncio.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 a finally block (or use current_graph_name.reset(token) to restore the prior value when the node finishes).

Minor: `current_graph_name.set(graph_name)` returns a `Token` that is discarded, and the value persists for the lifetime of the current `asyncio.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 a `finally` block (or use `current_graph_name.reset(token)` to restore the prior value when the node finishes).
CoreRasurae force-pushed feature/m2-llm-agent-retry-mechanisms from 77fd52e8b3 to fd72b4b77f 2026-07-03 14:20:52 +00:00 Compare
CoreRasurae force-pushed feature/m2-llm-agent-retry-mechanisms from fd72b4b77f to 064a6ce0c2 2026-07-03 14:42:53 +00:00 Compare
hurui200320 approved these changes 2026-07-03 14:52:03 +00:00
hurui200320 left a comment
Member
Copy Link
Copy Source

PR 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-135 and prevents the next coro_factory() call when the upcoming sleep would push the accumulated wait past the cap. All five ainvoke() call sites in process_message() plus the pruning pass are wrapped, the current_graph_name ContextVar is properly reset in a finally block (src/cleveractors/langgraph/nodes.py:414-416), and the no-tools stream_message() path now wraps the initial astream() connection. Sentinel validation for < -1 values raises ConfigurationError. 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_message drops kind/reason when wrapping an ExecutionError (asymmetric with process_message)

  • File: src/cleveractors/agents/llm.py, lines 1978-1999 (stream_message's except Exception arm)

  • Problem: process_message() preserves the inner ExecutionError's kind/reason on wrap (lines 1610-1615):

    if isinstance(e, ExecutionError):
    raise ExecutionError(
        f"LLM processing failed: {_err_msg}",
        kind=e.kind,
        reason=e.reason,
    ) from None

    stream_message() does not mirror this preservation. Its except Exception as e arm unconditionally does:

    raise ExecutionError(f"LLM streaming failed: {_err_msg}") from None

    Consequently, when the streaming path triggers a retry timeout — either through _execute_tool_loop() (re-raises _tle.cause, which is the retry ExecutionError) or through _start_stream() — the outer wrap discards kind="timeout" and reason="max_retries_exceeded" / "max_retry_time_exceeded". The caller sees kind="", defeating programmatic error classification and contradicting the CHANGELOG claim that "ExecutionError.kind/reason are preserved through the exception chain".

  • Recommendation: Mirror the process_message pattern in stream_message (and the LangChainException arm can be left as-is, since LangChainException is not an ExecutionError):

    if isinstance(e, ExecutionError):
    raise ExecutionError(
        f"LLM streaming failed: {_err_msg}",
        kind=e.kind,
        reason=e.reason,
    ) from None

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_url placement in _run_pruning_pass) — those were implicitly accepted as "won't fix in this PR" by the most recent APPROVED review from hurui200320 (review #9721), so they are not re-raised here per the scope-control rule.

Summary

The PR correctly adds max_retries / max_retry_time configuration fields with -1 sentinel support, wraps all five ainvoke() sites in process_message() and the pruning-pass call, plumbs the graph name through a ContextVar for descriptive timeout errors, and matches the documented error message shape. The integration points in langgraph/nodes.py and langgraph/pure_graph.py are minimal and targeted. The pre-sleep termination check at retry.py:132-135 correctly 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 like 0.5 terminate after the first failed attempt without a redundant coro_factory() call. The default max_retries=7, max_retry_time=60 interaction 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 asymmetric kind/reason preservation between process_message and stream_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.

## PR 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-135` and prevents the next `coro_factory()` call when the upcoming sleep would push the accumulated wait past the cap. All five `ainvoke()` call sites in `process_message()` plus the pruning pass are wrapped, the `current_graph_name` `ContextVar` is properly reset in a `finally` block (`src/cleveractors/langgraph/nodes.py:414-416`), and the no-tools `stream_message()` path now wraps the initial `astream()` connection. Sentinel validation for `< -1` values raises `ConfigurationError`. 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_message` drops `kind`/`reason` when wrapping an `ExecutionError` (asymmetric with `process_message`)** - **File:** `src/cleveractors/agents/llm.py`, lines 1978-1999 (`stream_message`'s `except Exception` arm) - **Problem:** `process_message()` preserves the inner `ExecutionError`'s `kind`/`reason` on wrap (lines 1610-1615): ```python if isinstance(e, ExecutionError): raise ExecutionError( f"LLM processing failed: {_err_msg}", kind=e.kind, reason=e.reason, ) from None ``` `stream_message()` does not mirror this preservation. Its `except Exception as e` arm unconditionally does: ```python raise ExecutionError(f"LLM streaming failed: {_err_msg}") from None ``` Consequently, when the streaming path triggers a retry timeout — either through `_execute_tool_loop()` (re-raises `_tle.cause`, which is the retry `ExecutionError`) or through `_start_stream()` — the outer wrap discards `kind="timeout"` and `reason="max_retries_exceeded"` / `"max_retry_time_exceeded"`. The caller sees `kind=""`, defeating programmatic error classification and contradicting the CHANGELOG claim that "*`ExecutionError.kind`/`reason` are preserved through the exception chain*". - **Recommendation:** Mirror the `process_message` pattern in `stream_message` (and the `LangChainException` arm can be left as-is, since `LangChainException` is not an `ExecutionError`): ```python if isinstance(e, ExecutionError): raise ExecutionError( f"LLM streaming failed: {_err_msg}", kind=e.kind, reason=e.reason, ) from None ``` ### 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_url` placement in `_run_pruning_pass`) — those were implicitly accepted as "won't fix in this PR" by the most recent APPROVED review from `hurui200320` (review #9721), so they are not re-raised here per the scope-control rule. ### Summary The PR correctly adds `max_retries` / `max_retry_time` configuration fields with `-1` sentinel support, wraps all five `ainvoke()` sites in `process_message()` and the pruning-pass call, plumbs the graph name through a `ContextVar` for descriptive timeout errors, and matches the documented error message shape. The integration points in `langgraph/nodes.py` and `langgraph/pure_graph.py` are minimal and targeted. The pre-sleep termination check at `retry.py:132-135` correctly 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 like `0.5` terminate after the first failed attempt without a redundant `coro_factory()` call. The default `max_retries=7, max_retry_time=60` interaction 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 asymmetric `kind`/`reason` preservation between `process_message` and `stream_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.
CoreRasurae force-pushed feature/m2-llm-agent-retry-mechanisms from 064a6ce0c2 to 5e4c5ac8f9 2026-07-03 14:53:25 +00:00 Compare
CoreRasurae force-pushed feature/m2-llm-agent-retry-mechanisms from 5e4c5ac8f9 to 52451b2550 2026-07-03 15:00:38 +00:00 Compare
CoreRasurae merged commit 52451b2550 into master 2026-07-03 15:21:51 +00:00
CoreRasurae deleted branch feature/m2-llm-agent-retry-mechanisms 2026-07-03 15:21:51 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Graa
hurui200320
Labels
Clear labels
auto/blocked-by-deps
PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.
 auto/ci-timeout
Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.
 auto/claimed-implementer
Currently being processed by an implementer worker.
 auto/claimed-merge
Currently being processed by the merge driver.
 auto/claimed-reviewer
Currently being processed by a reviewer worker.
 auto/driver-down
Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.
 auto/invariant-violation
Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.
 auto/last-attempt-tier-0
In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.
 auto/last-attempt-tier-1
In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.
 auto/last-attempt-tier-2
In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.
 auto/last-attempt-tier-min
In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.
 Automation Tracking
Tracking issues used by the AI Automation system for agents to communicate and report.
 auto/needs-conflict-resolution
Rebase conflict needs LLM conflict-resolver.
 auto/needs-implementer
Failing CI needs implementer attention.
 auto/postmortem
Documenting a driver incident or rollback.
 auto/ready-to-merge
Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.
 auto/restart-throttled
Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.
 auto/revert
Revert PR backing out an invariant violation. Fast-tracked through the merge driver.
 auto/sentinel
Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.
 auto/stale-inactivity
No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.
 auto/unstable
Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.
 Blocked
A ticket in a blocked state and unable to complete until some other task is completed first.
Bounty
$100
A bounty of $100 for any open-source contributor who provides a MR that solves this issue
Bounty
$1000
A bounty of $1000 for any open-source contributor who provides a MR that solves this issue
Bounty
$10000
A bounty of $10000 for any open-source contributor who provides a MR that solves this issue
Bounty
$20
A bounty of $20 for any open-source contributor who provides a MR that solves this issue
Bounty
$2000
A bounty of $2000 for any open-source contributor who provides a MR that solves this issue
Bounty
$250
A bounty of $250 for any open-source contributor who provides a MR that solves this issue
Bounty
$50
A bounty of $50 for any open-source contributor who provides a MR that solves this issue
Bounty
$500
A bounty of $500 for any open-source contributor who provides a MR that solves this issue
Bounty
$5000
A bounty of $5000 for any open-source contributor who provides a MR that solves this issue
Bounty
$750
A bounty of $750 for any open-source contributor who provides a MR that solves this issue
MoSCoW
Could have
Could have feature in order to satisfy the epic/legendary.
MoSCoW
Must have
Must have feature in order to satisfy the epic/legendary.
MoSCoW
Should have
Should have feature in order to satisfy the epic/legendary.
Needs Feedback
There are questions in the ticket that can not be completed until the project owner provides clarity.
Points
1
1 man-hours worth of work for an expert with no learning curve.
Points
13
13 man-hours worth of work for an expert with no learning curve.
Points
2
2 man-hours worth of work for an expert with no learning curve.
Points
21
21 man-hours worth of work for an expert with no learning curve.
Points
3
3 man-hours worth of work for an expert with no learning curve.
Points
34
34 man-hours worth of work for an expert with no learning curve.
Points
5
5 man-hours worth of work for an expert with no learning curve.
Points
55
55 man-hours worth of work for an expert with no learning curve.
Points
8
8 man-hours worth of work for an expert with no learning curve.
Points
88
88 man-hours worth of work for an expert with no learning curve.
Priority
Backlog
This ticket has backlogged priority and is not to be worked on yet
Priority
CI Blocker
Critical priority issue that blocks CI/CD pipeline and prevents PR merges
Priority
Critical
The priority is critical
Priority
High
The priority is high
Priority
Low
The priority is low
Priority
Medium
The priority is medium
Signed-off: Owner
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Signed-off: Scrum Master
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Signed-off: Tech Lead
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Spike
A ticket for learning a tool or technology that is needed to be able to do future planning and design.
State
Completed
The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.
State
Duplicate
A ticket that represents the same content as an existing ticket.
State
In Progress
A ticket that is actively being developed.
State
In Review
A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.
State
Paused
This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.
State
Unverified
All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.
State
Verified
The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.
State
Wont Do
This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.
Type
Automation
Any edits or discussion about the AI automated coding system.
Type
Bug
Something that doesnt work as intended.
Type
Discussion
Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.
Type
Documentation
An error or improvement needed in the documentation.
Type
Epic
Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.
Type
Feature
Some new functionality not present.
Type
Legendary
A type of Epic which will contain other Epics.
Type
Refactor
A code change that restructures existing code without changing its external behavior.
Type
Support
Someone needs help using the project.
Type
Task
A generic task that doesnt fit into the other type categories.
Type
Testing
Work exclusively focusing on fixing or expanding testing.
No Label
Type
Feature
Milestone
No items
No Milestone v2.1.0
Projects
Clear projects
No project
Assignees
Clear assignees
aditya (Aditya Chhabra) aleenaumair (Aleena Umair) brent.edwards (Brent Edwards) CoreRasurae (Luis Mendes) drew (Drew Morris) eugen.thaci (Eugen Thaci) freemo (Jeffrey Phillips Freeman) HAL9000 (HAL 9000) HAL9001 (HAL9001) hamza.khyari (Hamza Khyari) hurui200320 (Rui Hu) justin.morris khird (Kyle Hird) org.cleveragents
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
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?