diff --git a/CHANGELOG.md b/CHANGELOG.md index a6c63d5..ba2e450 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,10 @@ and this project adheres to [Clever Semantic Versioning](https://www.w3.org/subm - **Tool-Calling Capability Discovery (issue #59)** (`llm.py`): `get_capabilities()` now returns `"tool-calling"` when tools are configured on the agent. `get_metadata()` includes `tools_configured` (boolean) and `tool_count` (integer) for introspection. +- **LLM Agent Retry Mechanisms with Exponential Backoff (issue #69)** (`retry.py`, `llm.py`, `nodes.py`): `call_with_retry()` provides exponential backoff (`base * 2^attempt`) with two independent termination guards (`max_retries`/`max_retry_time`, -1 disables). Both guards share a single accumulated-wait counter; whichever limit is reached first terminates the loop. A pre-call budget check ensures no attempt is made once the accumulated wait reaches the cap (ADR-2032 D-2). Sentinel values below `-1` are rejected with `ConfigurationError`. All `chat_model.ainvoke()` calls in `process_message()` and the pruning-pass `ainvoke` are wrapped. The no-tools `stream_message()` path wraps the initial `astream()` connection in per-chunk granular retry. The graph name flows through a ContextVar for descriptive timeout errors, with the ContextVar reset after each node execution to prevent stale name leakage. `ExecutionError.kind`/`reason` are preserved through the exception chain. + + **ADR:** `docs/adr/ADR-2032-llm-agent-retry-mechanisms.md` documents seven design decisions including config fields, termination semantics, exponential backoff, and timeout error format. + ### Fixed diff --git a/docs/adr/ADR-2032-llm-agent-retry-mechanisms.md b/docs/adr/ADR-2032-llm-agent-retry-mechanisms.md new file mode 100644 index 0000000..76c37fe --- /dev/null +++ b/docs/adr/ADR-2032-llm-agent-retry-mechanisms.md @@ -0,0 +1,372 @@ +# ADR-2032: LLM Agent Communication Retry Mechanisms — Specification Extensions + +**Status:** accepted + +**Date:** 2026-07-02 + +**Author:** Luis Mendes (CoreRasurae) + +**Issue:** #69 — Introduce LLM agent communication retry mechanisms with exponential backoff + +--- + +## Context + +The current LLM agent implementation (`LLMAgent` and the pruning pass in the +tool-call loop) makes a single `ainvoke()` call to the configured LLM provider +(e.g. an `openai_compatible` server). When that call fails due to transient +network errors, server overload, or rate-limiting, the failure propagates +immediately to the caller with no retry logic. + +In production deployments against self-hosted or third-party LLM servers, +transient failures are common. A single failure should not abort the entire +actor-graph execution when a brief retry window would allow the call to succeed. + +The Actor Configuration Standard (§4.4) defines the LLM agent configuration +schema. It currently has no fields for controlling communication retry behaviour. +The multi-turn tool-call loop (ADR-2031) introduced the pruning pass, which +makes a separate LLM call that also lacks retry logic. + +This ADR documents the specification extensions required to support configurable +retry mechanisms for all LLM agent communications (both main-agent calls and +pruning-agent calls). + +--- + +## Decision + +### D-1: New LLM Agent Configuration Fields (`max_retries`, `max_retry_time`) + +**What:** Two new optional configuration fields added to the LLM agent schema +in §4.4. + +| Field | Type | Required | Default | Description | +|-------|------|----------|---------|-------------| +| `max_retries` | integer | No | `7` | Maximum number of retry attempts permitted for a single LLM communication. When the retry count reaches this value, no further attempts are made. Set to `-1` to allow infinite retries (bounded only by `max_retry_time` if set to a non-negative value). | +| `max_retry_time` | number | No | `60.0` | Maximum accumulated wall-clock time (in seconds) permitted for retries of a single LLM communication. When the total accumulated wait time reaches or exceeds this value, no further attempts are made. Set to `-1` to allow infinite retry time (bounded only by `max_retries` if set to a non-negative value). | + +**Spec relationship:** §4.4 currently defines the LLM agent configuration +fields. These two new fields extend the field set without removing or altering +any existing mandated parameter. Per §1.3, compliant implementations MAY accept +additional agent configuration fields. + +Both fields default to sensible values (`7` retries, `60` seconds), ensuring +backward compatibility: when no new config keys are provided, the retry +mechanism is active with safe defaults. + +### D-2: Retry Loop Termination — Whichever Comes First + +**What:** The retry loop terminates when whichever of the two limits is +reached first: + +1. The retry attempt count reaches `max_retries` (the initial attempt is not + counted as a retry; only subsequent attempts after a failure count toward + this limit). When `max_retries == -1` this limit is disabled. +2. The accumulated wall-clock wait time reaches or exceeds `max_retry_time` + seconds. Accumulated time is measured as the sum of all sleep intervals + between retries, not the total elapsed wall time of the entire operation. + When `max_retry_time == -1` this limit is disabled. + +When a limit is reached, the library MUST NOT make another attempt. The last +encountered error is then propagated to the caller as an `ExecutionError` +with `kind="timeout"`. Setting both `max_retries` and `max_retry_time` to +`-1` creates an infinite retry loop and is strongly discouraged. + +**Spec relationship:** This defines the retry-loop semantics. The specification +does not currently define any retry behaviour, so this is an implementation +extension consistent with §1.3. + +### D-3: Exponential Backoff + +**What:** The wait time between retry attempts follows an exponential backoff +strategy: + +- **Initial wait time:** `0.5` seconds (hard-coded, not user-configurable). +- **Doubling:** After each failed attempt, the wait time is doubled. + - Attempt 1 (first retry): wait `0.5` s + - Attempt 2 (second retry): wait `1.0` s + - Attempt 3 (third retry): wait `2.0` s + - Attempt 4 (fourth retry): wait `4.0` s + - Attempt 5 (fifth retry): wait `8.0` s + - Attempt 6 (sixth retry): wait `16.0` s + - Attempt 7 (seventh retry): wait `32.0` s +- **No jitter:** The initial implementation does not add jitter to the backoff + interval. This may be introduced in a future ADR if thundering-herd patterns + are observed. +- **No cap override:** The doubling continues regardless of `max_retry_time`; + the `max_retry_time` check governs termination independently of the backoff + schedule. + +The implementation MUST sleep (via `asyncio.sleep()` in asynchronous contexts) +for the computed wait time between attempts. + +**Spec relationship:** The backoff schedule is an implementation detail of the +retry mechanism. It is not user-configurable and does not appear in the +configuration document. The `0.5` s initial interval and doubling factor are +chosen to give a reasonable number of retries before the wait times become +impractically long. + +### D-4: Retry State — Counter and Accumulated Time + +**What:** The retry loop maintains two pieces of runtime state: + +1. **Retry counter:** An integer count of retry attempts made so far for the + current LLM communication. Reset to `0` at the start of each new LLM call. + Incremented by `1` after each failed attempt before computing the next + backoff interval. +2. **Accumulated wait time:** A floating-point total of all sleep intervals + (in seconds) that have elapsed during retries for the current LLM + communication. Reset to `0.0` at the start of each new LLM call. Updated + after each sleep completes. + +On successful communication, both the retry counter and the accumulated wait +time MUST be reset to their initial values (`0` and `0.0` respectively). + +**Spec relationship:** Runtime state internal to the retry loop. Not exposed +in the configuration schema. + +### D-5: Retry Scope — Main Agent and Pruning Agent + +**What:** The retry mechanism applies uniformly to all LLM communications, +specifically: + +1. **Main agent calls:** Every `ainvoke()` or equivalent call made by + `LLMAgent` to the configured LLM provider when processing a message. +2. **Pruning agent calls:** Every LLM call made by the pruning pass + (ADR-2031 D-3 step 4) when `allow_tool_output_pruning` is enabled. + +The same `max_retries` and `max_retry_time` configuration values are used for +both the main agent and the pruning agent. There is no separate retry +configuration for the pruning pass. + +**Spec relationship:** The pruning pass (ADR-2031) makes a stateless, +single-turn LLM call. This ADR extends that call with the same retry +mechanism as the main agent call, ensuring uniform resilience across all LLM +communications in the system. + +### D-6: No Re-Establishment of Communication + +**What:** The retry loop does **not** tear down and re-establish the +communication channel (e.g. HTTP connection, WebSocket, client object) between +attempts. The same chat model instance and client configuration are reused +for all retry attempts. Retries operate at the request/response level only. + +**Spec relationship:** This is an implementation constraint. The LangChain +`ChatModel` instance (or equivalent provider client) is created once and +reused across retries. Configuration parameters such as `base_url`, +`api_key`, model name, and temperature are fixed for the entire retry sequence. + +### D-7: Timeout Error Reporting + +**What:** When the retry loop terminates due to a termination condition, the +library MUST raise an `ExecutionError` with: + +- `kind="timeout"` +- `reason` set to one of: + - `"max_retries_exceeded"` — when `max_retries >= 0` was reached. + - `"max_retry_time_exceeded"` — when `max_retry_time >= 0` was reached. + - `"max_retries_exceeded"` — when both conditions were met simultaneously. +- The error message MUST include: + - The URL of the LLM provider server being contacted (e.g. the `base_url` + or endpoint URL). + - The actor graph name or identifier that was being executed when the + timeout occurred. + - The retry count and accumulated wait time at the point of termination. + +Example error message format (finite termination): + +> ExecutionError(kind="timeout", reason="max_retries_exceeded"): LLM +> communication timed out after 7 retries (accumulated wait time: 63.5s) +> while contacting 'https://llm.example.com/v1/chat/completions' for actor +> graph 'research-assistant'. + +This ensures that operators can identify which remote endpoint and which actor +graph are experiencing connectivity issues. + +When one of the two guards is set to `-1` (infinite), the error message +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 +> communication timed out after 7 retries (max_retry_time was disabled) +> while contacting 'https://llm.example.com/v1/chat/completions' for actor +> graph 'research-assistant'. + +Symmetrically, if `max_retry_time` was exceeded and `max_retries == -1`: + +> ExecutionError(kind="timeout", reason="max_retry_time_exceeded"): LLM +> communication timed out after 7 retries (max_retries was disabled) +> while contacting 'https://llm.example.com/v1/chat/completions' for actor +> graph 'research-assistant'. + +**Spec relationship:** `ExecutionError` is defined in ADR-2029 with a `kind` +field supporting the `"timeout"` value. This ADR adds two new `reason` +sub-codes (`"max_retries_exceeded"` and `"max_retry_time_exceeded"`) and +specifies the error message format. + +### D-8: Retry Scope — Only HTTP Communication Errors + +**What:** The retry loop retries only exceptions that represent transient +HTTP communication or network errors. All other exception types (e.g. +`BadRequestError`, `AuthenticationError`, `ValueError`, provider-level +validation errors) propagate immediately without retry. + +Specifically, the following exception types trigger a retry: + +- `httpx.ConnectError` — connection refused, DNS resolution failure. +- `httpx.TimeoutException` (and subclasses `ReadTimeout`, + `WriteTimeout`, `PoolTimeout`) — request-level timeout. +- `httpx.RemoteProtocolError` — connection dropped mid-request. +- `httpx.HTTPStatusError` with status codes `429` (rate limited), + `502` (bad gateway), `503` (service unavailable), or + `504` (gateway timeout) — server-side transient errors. +- Any other `httpx.HTTPError` — catch-all for transport-level errors + not covered above. + +All other exceptions (including provider-specific validation errors, +authentication failures, and programming errors) are re-raised immediately. + +**Spec relationship:** The ADR-2032 D-5 (Retry Scope) originally stated +that retries apply to "all LLM communications". This decision refines +that scope to exclude non-transient errors, ensuring that the caller +fails fast on configuration or logic errors while still retrying +transient network failures. + +--- + +## Consequences + +### Positive + +- **Increased resilience:** Transient LLM server failures (network blips, + rate-limit 429s, 503s) are automatically retried, reducing spurious + actor-graph failures. +- **Configurable behaviour:** Users can tune `max_retries` and + `max_retry_time` to match their deployment's reliability characteristics, + including setting either or both to `-1` for unbounded retries. +- **Deterministic termination:** When both guards are finite, the "whichever + comes first" model guarantees that retries never exceed either the attempt + budget or the time budget. +- **Backward compatibility:** All new configuration fields are optional with + sensible defaults (`max_retries=7`, `max_retry_time=60`). Existing YAML + configurations continue to work unchanged. +- **No spec violations:** Every extension is justified by §1.3 (extensibility + clause) or falls within implementation-quality territory. +- **Uniform retry scope:** Both main-agent and pruning-agent calls benefit + from the same retry logic, ensuring no silent failure path. +- **Observable timeouts:** Error messages include the target URL and actor + graph name, enabling rapid diagnosis of connectivity issues. + +### Negative / Risks + +- **Increased latency on failure:** A sequence of 7 retries with exponential + backoff has a total wait time of `0.5 + 1 + 2 + 4 + 8 + 16 + 32 = 63.5` + seconds, which exceeds the default `max_retry_time` of 60 seconds. In + practice, the `max_retry_time` (60 s) will usually terminate retries before + the 7th retry's 32 s wait completes. Users who want all 7 retries must set + `max_retry_time` to at least `63.5` or set it to `-1` for unbounded time. +- **Infinite loop risk:** Setting both `max_retries = -1` and + `max_retry_time = -1` creates an infinite retry loop. This configuration is + strongly discouraged; documentation MUST warn against it. +- **Duplicate request processing:** If the server processes the request but + the TCP connection drops before the response reaches the client, a retry may + cause duplicate processing. This is a general concern with retry-at-most-once + semantics. Callers should ensure idempotency where possible. +- **No jitter:** In deployments with many agents hitting the same LLM server, + synchronised backoff may cause thundering-herd patterns on recovery. Jitter + may be added in a follow-up ADR if observed in production. +- **Connection reuse:** Reusing the same client across retries means that a + persistently broken connection will fail on every retry. This is acceptable + because the retry loop terminates quickly via `max_retries` or + `max_retry_time`. + +### Follow-up Required + +- **ADR-2032 compliance validation:** Add BDD scenarios verifying: + - Retry loop terminates at `max_retries` (exact count). + - Retry loop terminates at `max_retry_time` (accumulated wait). + - Exponential backoff doubles correctly. + - Counter and accumulated time reset on success. + - Timeout error includes URL and actor graph name. + - Pruning agent retries use the same configuration. +- **Integration test:** Add a Robot Framework test that simulates transient + LLM server failures and verifies the retry mechanism with a mock server. +- **Specification update:** After battle-testing, graduate the new + configuration fields into §4.4 of the Actor Configuration Standard via a + subsequent ADR. + +--- + +## Alternatives Considered + +### A-1: No retry mechanism (status quo) + +**Rejected because:** A single transient failure aborts the entire actor graph. +In production environments with self-hosted LLM servers, transient failures are +common enough that the status quo causes unacceptable reliability. Users would +need to implement their own retry wrappers, leading to inconsistent behaviour +across deployments. + +### A-2: Fixed-interval retries (no backoff) + +**Rejected because:** Without backoff, retries would pound the failing server +at a constant rate, potentially worsening the outage. Exponential backoff gives +the server time to recover while still attempting retries promptly after the +initial failure. + +### A-3: Jittered exponential backoff + +**Rejected because:** Jitter adds implementation complexity and is primarily +beneficial in large-scale distributed deployments with many concurrent clients. +For a single-agent or small-scale deployment, the benefit does not justify the +complexity. Jitter may be added in a follow-up ADR if thundering-herd patterns +are observed. + +### A-4: Separate retry configuration for pruning pass + +**Rejected because:** The pruning pass (ADR-2031) is a subordinate LLM call +within the main agent's tool-call loop. Having separate retry configs for the +pruning pass would add schema complexity without clear benefit. The same +`max_retries` and `max_retry_time` values are appropriate for both. + +### A-5: Re-establish communication channel on each retry + +**Rejected because:** Tearing down and recreating the HTTP client, WebSocket, +or LangChain `ChatModel` instance on each retry adds overhead and risks +introducing new failure modes (e.g. credential reload, DNS re-resolution). +Reusing the same client is simpler and sufficient for transient failures. + +### A-6: Use `max_elapsed_time` instead of accumulated wait time + +**Rejected because:** Using total elapsed wall time (including the LLM call +duration itself) would make the timeout dependent on the server's response +time, which can vary widely. Accumulated wait time only measures the time +spent waiting between retries, giving a consistent and predictable timeout +budget regardless of server response latency. + +### A-7: Single `max_retries` without time budget + +**Rejected because:** Without a time budget, a long-running sequence of +retries (e.g. 7 retries with 32 s last wait = 63.5 s total) could hold up +the actor graph for an unacceptable duration. The `max_retry_time` cap +ensures the system responds within a bounded time. + +### A-8: No infinite sentinel (`-1`) support — always require finite limits + +**Rejected because:** Some deployments may have external timeout enforcement +(e.g. a load balancer or reverse proxy that terminates long-running requests) +and prefer only a retry-count limit without a redundant time budget, or vice +versa. The `-1` sentinel provides this flexibility while keeping the schema +simple. The risk of infinite looping is mitigated by documentation warnings +and the sensible defaults (`max_retries=7`, `max_retry_time=60`). + +### A-9: Retry all exception types indiscriminately + +**Rejected because:** Retrying non-transient errors (authentication +failures, invalid requests, programming errors, provider-side validation +rejections) wastes time and delays failure reporting. A `BadRequestError` +caused by an oversized payload will fail on every retry; the user should +see the error immediately rather than waiting for the retry budget to +exhaust. The D-8 approach distinguishes between transient network errors +(worth retrying) and deterministic logic/configuration errors (fail fast), +giving the best of both worlds. diff --git a/features/llm_agent_retry.feature b/features/llm_agent_retry.feature new file mode 100644 index 0000000..57e319d --- /dev/null +++ b/features/llm_agent_retry.feature @@ -0,0 +1,58 @@ +Feature: LLM Agent Communication Retry Mechanisms (ADR-2032) + As a developer using CleverActors + I want LLM agent communications to be retried with exponential backoff + So that transient LLM server failures are handled gracefully + + Background: + Given retry: LLM agent system is initialized + And retry: I have a mock chat model that can be configured to fail + + Scenario: Retry succeeds after transient failures + Given retry: I have an LLM agent configured with max_retries 3 and max_retry_time 60.0 + When retry: I configure the mock to fail 2 times then succeed + And retry: I process a message through the agent + Then retry: the message should be processed successfully + And retry: the response should be "Final response after retry" + + Scenario: max_retries exceeded raises timeout error + Given retry: I have an LLM agent configured with max_retries 2 and max_retry_time 60.0 + When retry: I configure the mock to always fail + And retry: I process a message through the agent + Then retry: an ExecutionError should be raised with kind "timeout" + And retry: the error reason should be "max_retries_exceeded" + + Scenario: max_retry_time exceeded raises timeout error + Given retry: I have an LLM agent configured with max_retries 100 and max_retry_time 0.5 + When retry: I configure the mock to always fail + And retry: I process a message through the agent + Then retry: an ExecutionError should be raised with kind "timeout" + And retry: the error reason should be "max_retry_time_exceeded" + + Scenario: max_retries for infinite retries bounded by time + Given retry: I have an LLM agent configured with max_retries -1 and max_retry_time 0.5 + When retry: I configure the mock to always fail + And retry: I process a message through the agent + Then retry: an ExecutionError should be raised with kind "timeout" + And retry: the error reason should be "max_retry_time_exceeded" + + Scenario: max_retry_time for infinite time bounded by count + Given retry: I have an LLM agent configured with max_retries 3 and max_retry_time -1 + When retry: I configure the mock to always fail + And retry: I process a message through the agent + Then retry: an ExecutionError should be raised with kind "timeout" + And retry: the error reason should be "max_retries_exceeded" + + Scenario: Success resets retry counter + Given retry: I have an LLM agent configured with max_retries 3 and max_retry_time 60.0 + When retry: I configure the mock to fail 1 times then succeed + And retry: I process a message through the agent + Then retry: the message should be processed successfully + And retry: the response should be "Final response after retry" + + Scenario: Timeout error includes provider URL + Given retry: I have an LLM agent configured with max_retries 1 and max_retry_time 60.0 + And retry: the agent has a base URL "https://test-llm.example.com/v1" + When retry: I configure the mock to always fail + And retry: I process a message through the agent + Then retry: the error message should contain "https://test-llm.example.com/v1" + And retry: the error message should contain the agent name diff --git a/features/steps/actor_result_token_counting_steps.py b/features/steps/actor_result_token_counting_steps.py index e7a1037..3a4e931 100644 --- a/features/steps/actor_result_token_counting_steps.py +++ b/features/steps/actor_result_token_counting_steps.py @@ -1887,13 +1887,14 @@ def step_artc_single_llm_agent_parallel(context: Context) -> None: }, ): agent = LLMAgent( - name="shared_agent", + name="test_agent", config={ "provider": "openai", "model": "gpt-4", "system_prompt": "You are helpful.", "temperature": 0.7, "max_tokens": 100, + "max_retries": 0, }, template_renderer=renderer, ) diff --git a/features/steps/credential_llm_error_steps.py b/features/steps/credential_llm_error_steps.py index bce42d4..d1c76e6 100644 --- a/features/steps/credential_llm_error_steps.py +++ b/features/steps/credential_llm_error_steps.py @@ -137,6 +137,7 @@ def step_inject_mock_raising_langchain_exception(context: Any) -> None: """ from cleveractors.agents.llm import LangChainException + context.agent._max_retries = 0 model = MagicMock() model.ainvoke = AsyncMock(side_effect=LangChainException("Simulated LC error")) model.temperature = 0.7 diff --git a/features/steps/llm_agent_retry_steps.py b/features/steps/llm_agent_retry_steps.py new file mode 100644 index 0000000..fc1257c --- /dev/null +++ b/features/steps/llm_agent_retry_steps.py @@ -0,0 +1,189 @@ +"""Step definitions for LLM agent retry mechanism tests (ADR-2032).""" + +from __future__ import annotations + +import asyncio +from unittest.mock import AsyncMock, Mock, PropertyMock, patch + +import httpx +from behave import given, then, when +from behave.runner import Context + +from cleveractors.agents.llm import LLMAgent +from cleveractors.core.exceptions import ExecutionError +from cleveractors.templates.renderer import TemplateRenderer + +# Negligible retry delay for fast tests (0.5ms vs production 0.5s). +# The exponential doubling is still validated: 0.0005 → 0.001 → 0.002 → ... +_TEST_RETRY_DELAY = 0.0005 + + +def _make_mock_model() -> Mock: + """Create a basic mock chat model.""" + model = Mock() + model.ainvoke = AsyncMock() + model.temperature = 0.7 + return model + + +@given("retry: LLM agent system is initialized") +def step_retry_system_init(context: Context) -> None: + """Initialize the test environment.""" + context.template_renderer = Mock(spec=TemplateRenderer) + context.template_renderer.render_string.return_value = "mocked system prompt" + context.fail_count = 0 + context.max_fails = 0 + context.always_fail = False + context.exception = None + context.result = None + context.mock_model = _make_mock_model() + # Patch initial retry delay to negligible value so tests run fast. + # The exponential doubling (0.0005 → 0.001 → 0.002 → ...) is preserved. + context._retry_delay_patch = patch( + "cleveractors.agents.retry._INITIAL_RETRY_DELAY", _TEST_RETRY_DELAY + ) + context._retry_delay_patch.start() + context._active_patches.append(context._retry_delay_patch) + + +@given("retry: I have a mock chat model that can be configured to fail") +def step_retry_have_mock_model(context: Context) -> None: + """Ensure mock model is ready.""" + if not hasattr(context, "mock_model"): + context.mock_model = _make_mock_model() + + +@given( + "retry: I have an LLM agent configured with max_retries {max_retries:d} and max_retry_time {max_retry_time:g}" +) +def step_retry_agent_with_config( + context: Context, max_retries: int, max_retry_time: float +) -> None: + """Create an LLM agent with specific retry configuration.""" + config = { + "name": "retry_test_agent", + "provider": "openai", + "api_key": "test-key", + "max_retries": max_retries, + "max_retry_time": max_retry_time, + } + agent = LLMAgent("retry_test_agent", config, context.template_renderer) + # Inject mock model to bypass lazy init + agent.chat_model = context.mock_model + context.agent = agent + + +@given('retry: the agent has a base URL "{url}"') +def step_retry_agent_base_url(context: Context, url: str) -> None: + """Set a base URL on the mock chat model for error message testing.""" + type(context.mock_model).base_url = PropertyMock(return_value=url) + + +@when("retry: I configure the mock to fail {count:d} times then succeed") +def step_retry_configure_fail_then_succeed(context: Context, count: int) -> None: + """Configure mock to fail count times then return a success response.""" + context.fail_count = 0 + context.max_fails = count + context.always_fail = False + + async def _mock_ainvoke(*args: object, **kwargs: object) -> object: + await asyncio.sleep(0.001) + if context.fail_count < context.max_fails: + context.fail_count += 1 + raise httpx.ConnectError( + f"Simulated transient failure #{context.fail_count}" + ) + response = Mock() + response.content = "Final response after retry" + response.usage_metadata = {"input_tokens": 10, "output_tokens": 5} + type(response).response_metadata = PropertyMock(return_value={}) + return response + + context.mock_model.ainvoke = AsyncMock(side_effect=_mock_ainvoke) + + +@when("retry: I configure the mock to always fail") +def step_retry_configure_always_fail(context: Context) -> None: + """Configure mock to always raise an error.""" + context.always_fail = True + context.fail_count = 0 + context.max_fails = 0 + + async def _mock_ainvoke(*args: object, **kwargs: object) -> object: + await asyncio.sleep(0.001) + context.fail_count += 1 + raise httpx.ConnectError(f"Simulated failure #{context.fail_count}") + + context.mock_model.ainvoke = AsyncMock(side_effect=_mock_ainvoke) + + +@when("retry: I process a message through the agent") +def step_retry_process_message(context: Context) -> None: + """Process a message and capture any exception.""" + try: + context.result = asyncio.run(context.agent.process_message("Test message")) + context.exception = None + except ExecutionError as e: + context.exception = e + context.result = None + except Exception as e: + context.exception = e + context.result = None + + +@then("retry: the message should be processed successfully") +def step_retry_message_success(context: Context) -> None: + """Assert no exception was raised during processing.""" + assert context.exception is None, ( + f"Expected no exception, got {type(context.exception).__name__}: {context.exception}" + ) + assert context.result is not None, "Expected a result but got None" + + +@then('retry: the response should be "{expected_text}"') +def step_retry_response_equals(context: Context, expected_text: str) -> None: + """Assert the response matches the expected text.""" + assert context.result == expected_text, ( + f"Expected response {expected_text!r}, got {context.result!r}" + ) + + +@then('retry: an ExecutionError should be raised with kind "{kind}"') +def step_retry_error_kind(context: Context, kind: str) -> None: + """Assert an ExecutionError was raised with the given kind.""" + assert context.exception is not None, "Expected an exception but none was raised" + assert isinstance(context.exception, ExecutionError), ( + f"Expected ExecutionError, got {type(context.exception).__name__}" + ) + assert context.exception.kind == kind, ( + f"Expected kind={kind!r}, got {context.exception.kind!r}" + ) + + +@then('retry: the error reason should be "{reason}"') +def step_retry_error_reason(context: Context, reason: str) -> None: + """Assert the error reason matches.""" + assert context.exception is not None, "Expected an exception but none was raised" + assert context.exception.reason == reason, ( + f"Expected reason={reason!r}, got {context.exception.reason!r}" + ) + + +@then('retry: the error message should contain "{text}"') +def step_retry_error_contains(context: Context, text: str) -> None: + """Assert the error message contains the given text.""" + assert context.exception is not None, "Expected an exception but none was raised" + assert text in str(context.exception), ( + f"Expected error message to contain {text!r}, got {context.exception!r}" + ) + + +@then("retry: the error message should contain the agent name") +def step_retry_error_contains_agent_name(context: Context) -> None: + """Assert the error message contains the agent's name.""" + assert context.exception is not None, "Expected an exception but none was raised" + agent_name = context.agent.name + assert agent_name in str(context.exception), ( + f"Expected error message to contain agent name {agent_name!r}, " + f"got {context.exception!r}" + ) diff --git a/features/steps/llm_agent_steps.py b/features/steps/llm_agent_steps.py index 9fa3c34..ed0853d 100644 --- a/features/steps/llm_agent_steps.py +++ b/features/steps/llm_agent_steps.py @@ -500,6 +500,8 @@ def step_process_message_openai_error(context): """Process message with OpenAI API error.""" context.test_message = "Test message" + # Disable retry — test expects fast failure on first ainvoke error + context.llm_agent._max_retries = 0 # Replace the chat model with a mock that raises an error context.llm_agent.chat_model = create_mock_chat_model(error_message="API Error") try: @@ -529,6 +531,8 @@ def step_process_message_anthropic_error(context): """Process message with Anthropic API error.""" context.test_message = "Test message" + # Disable retry — test expects fast failure on first ainvoke error + context.llm_agent._max_retries = 0 # Replace the chat model with a mock that raises an error context.llm_agent.chat_model = create_mock_chat_model(error_message="Unauthorized") try: @@ -558,6 +562,8 @@ def step_process_message_google_error(context): """Process message with Google API error.""" context.test_message = "Test message" + # Disable retry — test expects fast failure on first ainvoke error + context.llm_agent._max_retries = 0 # Replace the chat model with a mock that raises an error context.llm_agent.chat_model = create_mock_chat_model(error_message="Forbidden") try: @@ -612,6 +618,8 @@ def step_exception_during_processing(context): """Simulate exception during message processing.""" context.test_message = "Test message" + # Disable retry — test expects fast failure on first ainvoke error + context.llm_agent._max_retries = 0 # Replace the chat model with a mock that raises an error context.llm_agent.chat_model = create_mock_chat_model( error_message="Test exception" diff --git a/src/cleveractors/agents/llm.py b/src/cleveractors/agents/llm.py index 259f65d..43b7391 100644 --- a/src/cleveractors/agents/llm.py +++ b/src/cleveractors/agents/llm.py @@ -39,6 +39,7 @@ from cleveractors.agents.base import AgentWithMemory from cleveractors.agents.llm_client import build_chat_model from cleveractors.agents.llm_imports import populate_langchain_globals from cleveractors.agents.llm_tools import normalize_tool_entry as _normalize_tool_entry +from cleveractors.agents.retry import _get_provider_url, call_with_retry from cleveractors.core.exceptions import ( AgentCreationError, ConfigurationError, @@ -322,6 +323,20 @@ class LLMAgent(AgentWithMemory): ) 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)) + if self._max_retries < -1: + raise ConfigurationError( + f"max_retries must be -1 or a non-negative integer, " + f"got {self._max_retries!r}" + ) + self._max_retry_time: float = float(config.get("max_retry_time", 60.0)) + 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}" + ) + # pruning_tool_filter: list of tool names eligible for pruning (§4.4.8 D-2). # Pruning only applies when the invoked tool name appears in this list. _raw_filter = config.get("pruning_tool_filter") @@ -446,6 +461,38 @@ class LLMAgent(AgentWithMemory): } return default_models.get(self.provider.lower(), DEFAULT_MODEL) + @property + def _provider_url(self) -> str: + """Return the provider URL for error messages (ADR-2032 D-7).""" + return _get_provider_url(self._chat_model, self._credentials) + + async def _retry_ainvoke( + self, + messages: list[Any], + **kwargs: Any, + ) -> Any: + """Invoke the chat model with retry logic (ADR-2032). + + Wraps ``self.chat_model.ainvoke(messages, **kwargs)`` with the + exponential-backoff retry loop configured via ``max_retries`` and + ``max_retry_time``. + """ + # Eagerly initialize the model so that ConfigurationError from + # credential validation fails fast, outside the retry loop. + _ = self.chat_model + provider_url = self._provider_url + + async def _invoke() -> Any: + return await self.chat_model.ainvoke(messages, **kwargs) + + return await call_with_retry( + coro_factory=_invoke, + max_retries=self._max_retries, + max_retry_time=self._max_retry_time, + agent_name=self.name, + provider_url=provider_url, + ) + def _get_model_context_window(self) -> int: """Return the model's advertised context window size in tokens. @@ -652,7 +699,18 @@ class LLMAgent(AgentWithMemory): ) ) 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) + + async def _prune_invoke() -> Any: + return await prune_model.ainvoke(prune_messages) + + prune_response = await call_with_retry( + coro_factory=_prune_invoke, + max_retries=self._max_retries, + max_retry_time=self._max_retry_time, + agent_name=f"{self.name}:prune({tool_name})", + provider_url=prune_url, + ) except Exception: logger.warning( "Agent %s: pruning pass LLM call failed for tool %r; using raw output", @@ -809,9 +867,7 @@ class LLMAgent(AgentWithMemory): "call to complete your answer, do it now." ) messages.append(HumanMessage(content=_synthesis_text)) - response = await self.chat_model.ainvoke( - messages, **invoke_kwargs - ) + response = await self._retry_ainvoke(messages, **invoke_kwargs) _any_invocation_made = True _bp, _bc, _ = self._extract_token_counts(response) _accumulated_prompt += _bp @@ -955,7 +1011,7 @@ class LLMAgent(AgentWithMemory): tool_call_id=call_id, ) ) - response = await self.chat_model.ainvoke(messages) + response = await self._retry_ainvoke(messages) _any_invocation_made = True _bp3, _bc3, _ = self._extract_token_counts(response) _accumulated_prompt += _bp3 @@ -964,7 +1020,7 @@ class LLMAgent(AgentWithMemory): break # ── Regular ainvoke ──────────────────────────────────────── - response = await self.chat_model.ainvoke(messages, **invoke_kwargs) + response = await self._retry_ainvoke(messages, **invoke_kwargs) _any_invocation_made = True _mp, _mc, _ = self._extract_token_counts(response) _accumulated_prompt += _mp @@ -1126,7 +1182,7 @@ class LLMAgent(AgentWithMemory): ) ) ) - response = await self.chat_model.ainvoke(messages, tools=self._lc_tools) + response = await self._retry_ainvoke(messages, tools=self._lc_tools) _any_invocation_made = True _sp, _sc, _ = self._extract_token_counts(response) _accumulated_prompt += _sp @@ -1218,7 +1274,7 @@ class LLMAgent(AgentWithMemory): tool_call_id=call_id, ) ) - response = await self.chat_model.ainvoke(messages) + response = await self._retry_ainvoke(messages) _any_invocation_made = True _sfp, _sfc, _ = self._extract_token_counts(response) _accumulated_prompt += _sfp @@ -1461,12 +1517,11 @@ class LLMAgent(AgentWithMemory): _completion_tokens = _loop_result.accumulated_completion else: # No tools: single plain ainvoke() — no tool dispatch or synthesis. - _no_tools_resp = await self.chat_model.ainvoke(messages) + _no_tools_resp = await self._retry_ainvoke(messages) _pt, _ct, _ = self._extract_token_counts(_no_tools_resp) _prompt_tokens = _pt _completion_tokens = _ct response_text = str(_no_tools_resp.content) - # Capture token counts immediately after ainvoke() succeeds. # Setting the sentinel variables here marks the "ainvoke succeeded" # boundary: any exception raised after this point is a post-ainvoke @@ -1550,6 +1605,14 @@ class LLMAgent(AgentWithMemory): type(e).__name__, _err_msg, ) + # ADR-2032: preserve kind and reason when the inner error is + # already an ExecutionError (e.g. timeout from retry mechanism). + if isinstance(e, ExecutionError): + raise ExecutionError( + f"LLM processing failed: {_err_msg}", + kind=e.kind, + reason=e.reason, + ) from None raise ExecutionError(f"LLM processing failed: {_err_msg}") from None finally: # Restore original temperature if it was overridden. @@ -1763,6 +1826,28 @@ class LLMAgent(AgentWithMemory): return # generator done; finally block still runs # ── No-tools path: real token-by-token astream() ───────────── + # Wrap the initial stream connection (first chunk) in retry so + # transient LLM provider failures are retried at chunk granularity + # (ADR-2032 D-5 / issue #70 review). Once the first chunk + # arrives the stream is established; remaining chunks stream + # without retry. + + async def _start_stream() -> tuple[Any | None, Any | None]: + stream = self.chat_model.astream(lc_messages) + try: + first_chunk = await stream.__anext__() + except StopAsyncIteration: + return None, None + return first_chunk, stream + + first_chunk, remaining_stream = await call_with_retry( + coro_factory=_start_stream, + max_retries=self._max_retries, + max_retry_time=self._max_retry_time, + agent_name=self.name, + provider_url=self._provider_url, + ) + # Stream tokens — accumulate full response for memory update only # when memory_enabled is True. Accumulating unconditionally would # grow agent_response_parts to 100K+ entries for long responses @@ -1770,16 +1855,21 @@ class LLMAgent(AgentWithMemory): # consumed (m1 fix). # Use a list accumulator and join at the end to avoid O(n²) string # allocations from repeated += on immutable Python strings. - last_chunk: Any = None + last_chunk: Any = first_chunk agent_response_parts: list[str] = [] - async for chunk in self.chat_model.astream(lc_messages): - last_chunk = chunk - # Guard against metadata-only chunks where content is None: - # str(None) would yield the literal string "None" to the caller. - token = str(chunk.content) if chunk.content is not None else "" + if first_chunk is not None: + token = ( + str(first_chunk.content) if first_chunk.content is not None else "" + ) if _memory_enabled: agent_response_parts.append(token) yield token + async for chunk in remaining_stream: + last_chunk = chunk + token = str(chunk.content) if chunk.content is not None else "" + if _memory_enabled: + agent_response_parts.append(token) + yield token # Extract token counts from the final chunk. # Three-tier fallback chain (AC3, mirrors process_message()): @@ -1906,6 +1996,15 @@ class LLMAgent(AgentWithMemory): type(e).__name__, _err_msg, ) + # ADR-2032: preserve kind and reason when the inner error is + # already an ExecutionError (e.g. timeout from retry mechanism). + # Mirrors process_message() pattern (symmetric behavior). + if isinstance(e, ExecutionError): + raise ExecutionError( + f"LLM streaming failed: {_err_msg}", + kind=e.kind, + reason=e.reason, + ) from None raise ExecutionError(f"LLM streaming failed: {_err_msg}") from None finally: # Restore original temperature if it was overridden. diff --git a/src/cleveractors/agents/retry.py b/src/cleveractors/agents/retry.py new file mode 100644 index 0000000..492755a --- /dev/null +++ b/src/cleveractors/agents/retry.py @@ -0,0 +1,174 @@ +"""LLM agent communication retry mechanisms with exponential backoff (ADR-2032). + +This module provides the retry wrapper used by LLMAgent for all LLM +communications (main agent calls and pruning agent calls). The retry +loop employs exponential backoff (initial 0.5s, doubled each retry) and +terminates when whichever of ``max_retries`` or ``max_retry_time`` is reached +first. Setting either value to ``-1`` disables that guard. +""" + +from __future__ import annotations + +import asyncio +import contextvars +import logging +from typing import Any, Callable + +import httpx + +from cleveractors.core.exceptions import ExecutionError + +logger = logging.getLogger(__name__) + +# Per-task actor graph name. Set by Node._execute_agent() before calling +# agent.process_message() so the retry wrapper can include the graph name +# in timeout error messages. +current_graph_name: contextvars.ContextVar[str] = contextvars.ContextVar( + "current_graph_name", default="" +) + +_INITIAL_RETRY_DELAY: float = 0.5 + + +def _is_http_comms_error(e: Exception) -> bool: + """Return True when *e* is a transient HTTP/network error worth retrying. + + Non-HTTP errors (e.g. ``BadRequestError``, ``AuthenticationError``, + ``ValueError``) are raised immediately so the caller fails fast. + """ + if isinstance(e, httpx.TimeoutException): + return True + if isinstance(e, httpx.ConnectError): + return True + if isinstance(e, httpx.RemoteProtocolError): + return True + if isinstance(e, httpx.HTTPStatusError): + return e.response.status_code in {429, 502, 503, 504} + if isinstance(e, httpx.HTTPError): + return True + return False + + +def _get_provider_url(chat_model: Any, credentials: dict[str, str] | None) -> str: + """Extract the provider URL from a chat model instance or credentials.""" + if credentials and credentials.get("base_url"): + return credentials["base_url"] + for attr in ("base_url", "openai_api_base", "anthropic_api_url"): + url = getattr(chat_model, attr, None) + if url: + return str(url) + return "" + + +async def call_with_retry( + coro_factory: Callable[[], Any], + max_retries: int, + max_retry_time: float, + agent_name: str, + provider_url: str, +) -> Any: + """Execute a coroutine with exponential-backoff retry (ADR-2032). + + Args: + coro_factory: A zero-argument callable that returns an awaitable. + Called once per attempt. The same client/connection is reused + across retries (D-6: no re-establishment). + max_retries: Maximum retry attempts (``-1`` for unlimited). + max_retry_time: Maximum accumulated wait time in seconds + (``-1`` for unlimited). + agent_name: Agent name for error messages and logging. + provider_url: Provider URL for error messages. + + Returns: + The return value of the successful coroutine invocation. + + Raises: + ExecutionError: With ``kind="timeout"`` when all retries are exhausted. + """ + retry_count: int = 0 + accumulated_wait: float = 0.0 + wait_time: float = _INITIAL_RETRY_DELAY + last_exception: Exception | None = None + + while True: + # Pre-call budget check for max_retry_time only (accumulated wait): + # if accumulated_wait already meets or exceeds the cap, do not make + # another attempt (ADR-2032 D-2: "the library MUST NOT make another + # attempt" once a limit is reached). max_retries is checked in the + # post-failure block so the initial attempt is always permitted. + if max_retry_time >= 0 and accumulated_wait >= max_retry_time: + stop_reason = "max_retry_time_exceeded" + 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 + logger.warning( + "Agent %s: LLM call failed (attempt %d): %s", + agent_name, + retry_count + 1, + e, + ) + + should_stop = False + stop_reason = "" + + if max_retries >= 0 and retry_count >= max_retries: + should_stop = True + stop_reason = "max_retries_exceeded" + + # Pre-sleep check: if the next backoff interval would push the + # accumulated wait time past max_retry_time, stop before sleeping + # and never make the following attempt (ADR-2032 D-2: "the + # library MUST NOT make another attempt" once the cap is hit). + if not should_stop and max_retry_time >= 0: + if accumulated_wait + wait_time >= max_retry_time: + should_stop = True + stop_reason = "max_retry_time_exceeded" + + if should_stop: + break + + retry_count += 1 + logger.info( + "Agent %s: retrying in %.1fs (attempt %d, accumulated wait %.1fs)", + agent_name, + wait_time, + retry_count, + accumulated_wait + wait_time, + ) + + await asyncio.sleep(wait_time) + accumulated_wait += wait_time + wait_time *= 2.0 + + graph_name = current_graph_name.get() + msg_parts: list[str] = [ + f"LLM communication timed out after {retry_count} retries", + f"(accumulated wait time: {accumulated_wait:.1f}s)", + ] + if max_retries == -1: + msg_parts.append("(max_retries was disabled)") + if max_retry_time == -1: + msg_parts.append("(max_retry_time was disabled)") + if provider_url: + msg_parts.append(f"while contacting '{provider_url}'") + if graph_name: + msg_parts.append(f"for actor graph '{graph_name}'") + elif agent_name: + msg_parts.append(f"for agent '{agent_name}'") + + error_msg = " ".join(msg_parts) + logger.error("Agent %s: %s", agent_name, error_msg) + + raise ExecutionError( + error_msg, kind="timeout", reason=stop_reason + ) from last_exception diff --git a/src/cleveractors/langgraph/nodes.py b/src/cleveractors/langgraph/nodes.py index 5bee7c2..9c8fce2 100644 --- a/src/cleveractors/langgraph/nodes.py +++ b/src/cleveractors/langgraph/nodes.py @@ -5,6 +5,7 @@ Node definitions for LangGraph integration. from __future__ import annotations import asyncio +import contextvars import logging from collections.abc import AsyncGenerator from copy import deepcopy @@ -14,6 +15,7 @@ from typing import Any, List, Optional, cast from cleveractors.agents.base import Agent from cleveractors.agents.llm import LLMAgent, last_token_usage_var +from cleveractors.agents.retry import current_graph_name from cleveractors.agents.tool import ToolAgent from cleveractors.langgraph.state import GraphState from cleveractors.result import MAX_REASONABLE_TOKENS as _MAX_REASONABLE_TOKENS @@ -341,6 +343,7 @@ class Node: # pylint: disable=too-many-instance-attributes # leaves _node_token_usage as None so the synthetic error response # does not claim non-zero billing data. _node_token_usage: dict[str, Any] | None = None + _graph_name_token: contextvars.Token[str] | None = None try: # Reset the ContextVar before calling process_message() so that # stale values from a previous node in the same asyncio Task @@ -354,6 +357,9 @@ class Node: # pylint: disable=too-many-instance-attributes # of the context at creation time, so this reset in one Task does # not affect sibling Tasks. last_token_usage_var.set((0, 0)) + graph_name = state.metadata.get("graph_name", "") + if graph_name: + _graph_name_token = current_graph_name.set(graph_name) agent_response = await agent.process_message(agent_input, context) # Log response info for debugging truncation issues if agent_response: @@ -406,6 +412,9 @@ class Node: # pylint: disable=too-many-instance-attributes # claim non-zero billing data (M2 of review for issue #14). self.logger.error("Agent %s execution failed: %s", agent.name, e) agent_response = f"Error processing message: {str(e)}" + finally: + if _graph_name_token is not None: + current_graph_name.reset(_graph_name_token) # Return state updates, including any context changes made by the agent state_updates: dict[str, Any] = { diff --git a/src/cleveractors/langgraph/pure_graph.py b/src/cleveractors/langgraph/pure_graph.py index c222aba..282f078 100644 --- a/src/cleveractors/langgraph/pure_graph.py +++ b/src/cleveractors/langgraph/pure_graph.py @@ -787,6 +787,8 @@ class PureLangGraph: # Store the current message being processed in metadata so nodes can access it # This is important for tool agents that need to process routed messages state.metadata["current_message"] = message + # Pass graph name for retry error reporting (ADR-2032 D-7). + state.metadata["graph_name"] = self.name # Execute node with current state try: