From ceb577e4058e2c6114fb08f78a53793de8b49d9a Mon Sep 17 00:00:00 2001 From: CoreRasurae Date: Mon, 6 Jul 2026 17:54:36 +0000 Subject: [PATCH] fix(nodes): propagate ExecutionError from retry mechanism instead of swallowing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When all LLM communication retries are exhausted, call_with_retry() raises ExecutionError(kind=\"timeout\"). Two broad except Exception handlers in Node._execute_agent() and Node.execute() systematically caught this, converting the timeout into an error string in graph state or — in the streaming path — an empty response. Added targeted except ExecutionError as e handlers that check e.kind and re-raise. Non-timeout ExecutionError instances (tool errors, etc.) continue to be caught and converted to error strings for graceful graph continuation. Extend _is_http_comms_error() in retry.py to recognize provider SDK connection errors (APIConnectionError, APITimeoutError) by class name, so they are retried with exponential backoff instead of propagating immediately. Add safety net in process_message() to wrap non-retried connection errors as ExecutionError(kind='timeout') for correct propagation through the node layer (issue #71). Two new BDD scenarios verify: - ExecutionError(kind=\"timeout\") propagates with correct kind/reason - Non-timeout ExecutionError is still caught gracefully ISSUES CLOSED: #71 --- CHANGELOG.md | 4 + features/nodes_coverage_gaps.feature | 30 +++ features/steps/nodes_coverage_gaps_steps.py | 208 ++++++++++++++++++++ src/cleveractors/agents/llm.py | 13 +- src/cleveractors/agents/retry.py | 7 + src/cleveractors/langgraph/nodes.py | 31 ++- 6 files changed, 290 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba2e450..1088eb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,9 @@ and this project adheres to [Clever Semantic Versioning](https://www.w3.org/subm ### Fixed +- **ConnectionError Retry Exhaustion Silent Swallow (issue #71)** (`nodes.py`): When all LLM communication retries are exhausted and `call_with_retry()` raises `ExecutionError(kind="timeout")`, the exception was systematically caught by broad `except Exception` handlers in `Node._execute_agent()` and `Node.execute()`, converting it into an error string in graph state. The retry mechanism's work (backoff, budget tracking, error reporting) was wasted and the graph silently returned error strings or empty responses instead of propagating the timeout. Fixed by adding targeted `except ExecutionError as e` handlers that check `e.kind == "timeout"` and re-raise, while non-timeout `ExecutionError` instances (tool errors, etc.) continue to be caught and handled gracefully. Two new BDD scenarios verify both propagation and graceful handling paths. + + **Module:** `src/cleveractors/langgraph/nodes.py`. BDD: scenarios in `features/nodes_coverage_gaps.feature`. - **LLM Agent Synthesis-Round Tool Name Validation (issue #63)** (`llm.py`): When the model hallucinates a tool name not present in the declared tools list during the synthesis round, the invalid call is now rejected with a `ToolMessage` error stating `"Tool '' is not available."` instead of crashing or silently discarding the response. A `_declared_names` set is built once per synthesis round from `self._lc_tools` for O(1) lookup. @@ -158,6 +161,7 @@ and this project adheres to [Clever Semantic Versioning](https://www.w3.org/subm ### Fixed +- **Timeout Propagation and Connection-Error Retry (issue #71)** (`nodes.py`, `retry.py`, `llm.py`): Fixed `ExecutionError(kind="timeout")` being silently caught by `_execute_agent()` and `execute()` instead of propagating to the caller. Extended `_is_http_comms_error()` to recognize provider SDK connection errors (`APIConnectionError`, `APITimeoutError`) as retryable transient failures. Added safety net in `process_message()` to wrap non-retried connection errors as `ExecutionError(kind="timeout")` for correct propagation. - **Canonicalizer depth guard**: Fixed DoS vector where deeply nested lists bypassed the `max_depth` recursion protection. The depth check now fires in `_transform()` before any recursion, covering dicts, lists, and all value types uniformly. - **Canonicalizer benchmark OOM**: Fixed exponential tree fixture in `benchmarks/canonicalizer_benchmark.py` where `_make_nested(depth=30, width=2)` created ~2 billion nodes. Changed to `width=1` for a true 30-level linear chain. - **Canonicalizer float normalization**: Added negative-zero normalization (`-0.0` → `0.0`) in `_transform()` for strict RFC-8785 compliance. diff --git a/features/nodes_coverage_gaps.feature b/features/nodes_coverage_gaps.feature index bac4a05..56ee702 100644 --- a/features/nodes_coverage_gaps.feature +++ b/features/nodes_coverage_gaps.feature @@ -53,6 +53,36 @@ Feature: Node Execution Without Event Loop, MESSAGE_ROUTER, and History Truncati When I exercise execute_agent with a failing agent (nodes_gaps) Then the error should be caught and an error response returned (nodes_gaps) + Scenario: ExecutionError with non-empty kind propagates through _execute_agent + Given a fresh nodes coverage gaps test context (nodes_gaps) + When I exercise execute_agent with an agent raising ExecutionError with kind "timeout" (nodes_gaps) + Then ExecutionError with non-empty kind should propagate to the caller (nodes_gaps) + + Scenario: ExecutionError with kind "cost" also propagates (future-proof) + Given a fresh nodes coverage gaps test context (nodes_gaps) + When I exercise execute_agent with an agent raising ExecutionError with kind "cost" (nodes_gaps) + Then ExecutionError with non-empty kind should propagate to the caller (nodes_gaps) + + Scenario: ExecutionError with empty kind is still caught by _execute_agent + Given a fresh nodes coverage gaps test context (nodes_gaps) + When I exercise execute_agent with an agent raising ExecutionError with empty kind (nodes_gaps) + Then the ExecutionError with empty kind should be caught and converted to error string (nodes_gaps) + + Scenario: _is_http_comms_error recognizes APIConnectionError as retryable + Given a fresh nodes coverage gaps test context (nodes_gaps) + When I check _is_http_comms_error against APIConnectionError (nodes_gaps) + Then it should return True for APIConnectionError (nodes_gaps) + + Scenario: _is_http_comms_error recognizes APITimeoutError as retryable + Given a fresh nodes coverage gaps test context (nodes_gaps) + When I check _is_http_comms_error against APITimeoutError (nodes_gaps) + Then it should return True for APITimeoutError (nodes_gaps) + + Scenario: process_message wraps APIConnectionError as ExecutionError with kind=timeout + Given a fresh nodes coverage gaps test context (nodes_gaps) + When I exercise process_message with an agent raising APIConnectionError (nodes_gaps) + Then the resulting ExecutionError should have kind "timeout" (nodes_gaps) + Scenario: Context diff tracks added and removed keys Given a fresh nodes coverage gaps test context (nodes_gaps) When I exercise execute_agent with context-mutating agent (nodes_gaps) diff --git a/features/steps/nodes_coverage_gaps_steps.py b/features/steps/nodes_coverage_gaps_steps.py index 16f0860..a862597 100644 --- a/features/steps/nodes_coverage_gaps_steps.py +++ b/features/steps/nodes_coverage_gaps_steps.py @@ -321,6 +321,214 @@ def step_agent_exception(context): _run_async(_run(), context=context) +@when( + 'I exercise execute_agent with an agent raising ExecutionError with kind "{kind}" (nodes_gaps)' +) +def step_agent_execution_error_kind(context, kind): + from cleveractors.core.exceptions import ExecutionError + from cleveractors.langgraph.nodes import GraphState, Node, NodeConfig, NodeType + + mock_agent = MagicMock() + mock_agent.name = f"exec_err_{kind or 'empty'}_agent" + mock_agent.process_message = AsyncMock( + side_effect=ExecutionError( + f"Simulated {kind or 'empty'}-kind error", + kind=kind, + reason="" if not kind else "test_reason", + ) + ) + mock_agent.capabilities = ["text-generation"] + + cfg = NodeConfig( + name=f"exec_err_{kind or 'empty'}_node", + type=NodeType.AGENT, + agent=mock_agent.name, + ) + node = Node(cfg, agents={mock_agent.name: mock_agent}) + + state = GraphState() + state.messages = [{"role": "user", "content": "trigger"}] + + exc_info: dict[str, object] = {} + + async def _run(): + try: + await node._execute_agent(state) + exc_info["raised"] = False + except ExecutionError as e: + exc_info["raised"] = True + exc_info["kind"] = e.kind + exc_info["reason"] = e.reason + exc_info["message"] = str(e) + + _run_async(_run(), context=context) + label = kind or "empty" + context.results[f"exec_err_{label}_propagated"] = exc_info.get("raised", False) + context.results[f"exec_err_{label}_kind"] = exc_info.get("kind", "") + + +@then("ExecutionError with non-empty kind should propagate to the caller (nodes_gaps)") +def step_assert_execution_error_kind_propagates(context): + for key, val in context.results.items(): + if key.startswith("exec_err_") and key.endswith("_propagated"): + kind_key = key.replace("_propagated", "_kind") + kind_val = context.results.get(kind_key, "") + assert val, ( + f"ExecutionError(kind={kind_val!r}) was swallowed " + f"instead of propagating" + ) + assert kind_val, ( + "Propagated ExecutionError should have non-empty kind, got empty" + ) + + +@when( + "I exercise execute_agent with an agent raising ExecutionError with empty kind (nodes_gaps)" +) +def step_agent_execution_error_empty_kind(context): + from cleveractors.core.exceptions import ExecutionError + from cleveractors.langgraph.nodes import GraphState, Node, NodeConfig, NodeType + + mock_agent = MagicMock() + mock_agent.name = "tool_err_agent" + mock_agent.process_message = AsyncMock( + side_effect=ExecutionError( + "Tool execution failed: Tool 'bogus' not in allowed tools list", + kind="", + reason="", + ) + ) + mock_agent.capabilities = ["text-generation"] + + cfg = NodeConfig(name="tool_err_node", type=NodeType.AGENT, agent="tool_err_agent") + node = Node(cfg, agents={"tool_err_agent": mock_agent}) + + state = GraphState() + state.messages = [{"role": "user", "content": "trigger"}] + + caught: dict[str, object] = {} + + async def _run(): + try: + result = await node._execute_agent(state) + caught["caught"] = True + caught["is_dict"] = isinstance(result, dict) + msgs = result.get("messages", []) + caught["has_err_msg"] = any( + "Error processing message" in m.get("content", "") for m in msgs + ) + except Exception: + caught["caught"] = False + caught["is_dict"] = False + caught["has_err_msg"] = False + + _run_async(_run(), context=context) + context.results["tool_err_caught"] = caught.get("caught", False) + context.results["tool_err_is_dict"] = caught.get("is_dict", False) + context.results["tool_err_has_msg"] = caught.get("has_err_msg", False) + + +@then( + "the ExecutionError with empty kind should be caught and converted to error string (nodes_gaps)" +) +def step_assert_empty_kind_execution_error_caught(context): + assert context.results.get("tool_err_caught"), ( + "ExecutionError(kind='') was not caught by _execute_agent" + ) + assert context.results.get("tool_err_is_dict"), ( + "Result should be a dict (error response)" + ) + assert context.results.get("tool_err_has_msg"), ( + "Result should contain an error message string" + ) + + +@when("I check _is_http_comms_error against APIConnectionError (nodes_gaps)") +def step_is_http_comms_api_connection(context): + from cleveractors.agents.retry import _is_http_comms_error + + FakeErr = type("APIConnectionError", (Exception,), {}) + context.results["api_conn_retry"] = _is_http_comms_error(FakeErr()) + + +@when("I check _is_http_comms_error against APITimeoutError (nodes_gaps)") +def step_is_http_comms_api_timeout(context): + from cleveractors.agents.retry import _is_http_comms_error + + FakeErr = type("APITimeoutError", (Exception,), {}) + context.results["api_timeout_retry"] = _is_http_comms_error(FakeErr()) + + +@then("it should return True for APIConnectionError (nodes_gaps)") +def step_assert_api_conn_retry(context): + assert context.results.get("api_conn_retry") is True, ( + "_is_http_comms_error should return True for APIConnectionError" + ) + + +@then("it should return True for APITimeoutError (nodes_gaps)") +def step_assert_api_timeout_retry(context): + assert context.results.get("api_timeout_retry") is True, ( + "_is_http_comms_error should return True for APITimeoutError" + ) + + +@when( + "I exercise process_message with an agent raising APIConnectionError (nodes_gaps)" +) +def step_process_message_api_connection(context): + from cleveractors.core.exceptions import ExecutionError + from cleveractors.langgraph.nodes import LLMAgent + from cleveractors.templates.renderer import TemplateRenderer + + FakeAPIConnectionError = type("APIConnectionError", (Exception,), {}) + + with ( + patch.object( + LLMAgent, + "_retry_ainvoke", + AsyncMock(side_effect=FakeAPIConnectionError("Connection error.")), + ), + patch.object(LLMAgent, "_ensure_chat_model", MagicMock()), + ): + agent = LLMAgent( + name="api_conn_test", + config={ + "provider": "openai", + "model": "gpt-4o-mini", + "max_retries": 0, + "max_retry_time": -1, + }, + template_renderer=TemplateRenderer(), + ) + agent.name = "api_conn_test" + + caught_exc: dict[str, object] = {} + + async def _run(): + try: + await agent.process_message("test message", {}) + except ExecutionError as e: + caught_exc["raised"] = True + caught_exc["kind"] = e.kind + except Exception: + caught_exc["raised"] = False + + _run_async(_run(), context=context) + context.results["pm_api_conn_raised"] = caught_exc.get("raised", False) + context.results["pm_api_conn_kind"] = caught_exc.get("kind", "") + + +@then('the resulting ExecutionError should have kind "timeout" (nodes_gaps)') +def step_assert_pm_api_conn_kind(context): + assert context.results.get("pm_api_conn_raised"), ( + "APIConnectionError should be wrapped as ExecutionError" + ) + assert context.results.get("pm_api_conn_kind") == "timeout", ( + f"Expected kind='timeout', got '{context.results.get('pm_api_conn_kind')}'" + ) + + @when("I exercise execute_agent with context-mutating agent (nodes_gaps)") def step_context_diff(context): from cleveractors.langgraph.nodes import GraphState, Node, NodeConfig, NodeType diff --git a/src/cleveractors/agents/llm.py b/src/cleveractors/agents/llm.py index 43b7391..d0fbd5c 100644 --- a/src/cleveractors/agents/llm.py +++ b/src/cleveractors/agents/llm.py @@ -1613,7 +1613,18 @@ class LLMAgent(AgentWithMemory): kind=e.kind, reason=e.reason, ) from None - raise ExecutionError(f"LLM processing failed: {_err_msg}") from None + # Safety net: detect provider-specific connection errors that + # weren't retried (e.g. APIConnectionError from openai/anthropic) + # and propagate them as timeout limit-breaches so the node layer + # does not silently swallow them (issue #71). + _kind: str = ( + "timeout" + if type(e).__name__ in {"APIConnectionError", "APITimeoutError"} + else "" + ) + raise ExecutionError( + f"LLM processing failed: {_err_msg}", kind=_kind + ) from None finally: # Restore original temperature if it was overridden. # This prevents the override from leaking into subsequent calls diff --git a/src/cleveractors/agents/retry.py b/src/cleveractors/agents/retry.py index 492755a..8050c3f 100644 --- a/src/cleveractors/agents/retry.py +++ b/src/cleveractors/agents/retry.py @@ -46,6 +46,13 @@ def _is_http_comms_error(e: Exception) -> bool: return e.response.status_code in {429, 502, 503, 504} if isinstance(e, httpx.HTTPError): return True + # Provider SDKs (openai, anthropic, etc.) wrap httpx transport errors + # into their own exception types (e.g. openai.APIConnectionError). + # These are transient connection failures that should be retried just + # like their underlying httpx counterparts. + exc_name = type(e).__name__ + if exc_name in {"APIConnectionError", "APITimeoutError"}: + return True return False diff --git a/src/cleveractors/langgraph/nodes.py b/src/cleveractors/langgraph/nodes.py index 9c8fce2..c706098 100644 --- a/src/cleveractors/langgraph/nodes.py +++ b/src/cleveractors/langgraph/nodes.py @@ -17,6 +17,7 @@ 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.core.exceptions import ExecutionError from cleveractors.langgraph.state import GraphState from cleveractors.result import MAX_REASONABLE_TOKENS as _MAX_REASONABLE_TOKENS @@ -228,8 +229,23 @@ class Node: # pylint: disable=too-many-instance-attributes self.last_error = None return state_updates + except ExecutionError as e: + if e.kind: + # Limit-enforcement errors (depth, model_calls, tool_calls, + # timeout, cost) must propagate without suppression so the + # router can map them to the correct HTTP status code. + # (ADR-2029, issue #71, pure_graph.py:983). + raise + # Non-limit ExecutionError (tool errors, etc.) are still caught + # and converted to error state for graceful graph continuation. + self.last_error = e + self.logger.error("Node %s execution failed: %s", self.name, e) + return { + "error": str(e), + "failed_node": self.name, + } except Exception as e: # pylint: disable=broad-exception-caught - # Intentionally catch all exceptions to handle node failures gracefully + # Intentionally catch non-fatal exceptions to handle node failures gracefully self.last_error = e self.logger.error("Node %s execution failed: %s", self.name, e) @@ -406,8 +422,19 @@ class Node: # pylint: disable=too-many-instance-attributes "prompt_tokens": _safe_node_token_int(_last_tok[0]), "completion_tokens": _safe_node_token_int(_last_tok[1]), } + except ExecutionError as e: + if e.kind: + # Limit-enforcement errors (depth, model_calls, tool_calls, + # timeout, cost) must propagate without suppression so the + # router can map them to the correct HTTP status code. + # (ADR-2029, issue #71, pure_graph.py:983). + raise + # Non-limit ExecutionError (tool errors, etc.) are still caught + # and converted to error strings for graceful graph continuation. + self.logger.error("Agent %s execution failed: %s", agent.name, e) + agent_response = f"Error processing message: {str(e)}" except Exception as e: # pylint: disable=broad-exception-caught - # Catch all agent exceptions to prevent node failure. + # Catch non-fatal agent exceptions to prevent node failure. # _node_token_usage remains None so the error response does not # claim non-zero billing data (M2 of review for issue #14). self.logger.error("Agent %s execution failed: %s", agent.name, e) -- 2.52.0