RetryContext does not validate max_attempts, raises confusing RuntimeError when max_attempts=0 #8401

New issue

Open

opened 2026-04-13 18:40:12 +00:00 by HAL9000 · 1 comment

HAL9000 commented

2026-04-13 18:40:12 +00:00

Owner

Metadata

Commit: Build: Reinforced label enforcement, and ensure implementation workers dont continue work on a mergable PR.
Branch: main
SHA: 5a9aaa79ed

Background and Context

RetryContext in src/cleveragents/core/retry_service_patterns.py accepts a max_attempts parameter in its __init__ but performs no validation on it. When max_attempts=0 is passed, the underlying tenacity.Retrying / AsyncRetrying loop configured with stop_after_attempt(0) stops immediately without executing the function body, leaving the internal result sentinel as _UNSET. This triggers a RuntimeError("Retrying must execute at least once") — a confusing internal error rather than a clear ValueError at construction time.

The code quality standard requires: "All public/protected methods validate arguments first."

Current Behavior

ctx = RetryContext("my-op", max_attempts=0)
ctx.execute(lambda: 42)
# Raises: RuntimeError("Retrying must execute at least once")
# instead of a clear ValueError at construction time

Relevant code in retry_service_patterns.py:

class RetryContext:
    def __init__(
        self,
        operation_name: str,
        max_attempts: int = DEFAULT_MAX_ATTEMPTS,
        wait_strategy: Any = None,
    ):
        # No validation of max_attempts here
        self.max_attempts = max_attempts
        ...

    def execute(self, func, *args, **kwargs):
        ...
        for attempt in Retrying(
            stop=stop_after_attempt(self.max_attempts) | stop_after_delay(300.0),
            ...
        ):
            with attempt:
                result = func(*args, **kwargs)
        if result is _UNSET:
            raise RuntimeError("Retrying must execute at least once")  # confusing!

Expected Behavior

RetryContext.__init__ should validate max_attempts >= 1 and raise a ValueError with a clear message:

if max_attempts < 1:
    raise ValueError(
        f"max_attempts must be >= 1, got {max_attempts!r}"
    )

This is consistent with the existing validation pattern in CircuitBreaker.__init__ which validates half_open_max_successes >= 1.

Acceptance Criteria

RetryContext("op", max_attempts=0) raises ValueError at construction time with a clear message
RetryContext("op", max_attempts=-1) raises ValueError at construction time
RetryContext("op", max_attempts=1) continues to work correctly
BDD test scenario covers the invalid max_attempts boundary

Subtasks

Add if max_attempts < 1: raise ValueError(...) guard in RetryContext.__init__
Add BDD test for boundary condition max_attempts=0 and max_attempts=-1
Verify no existing tests break

Definition of Done

The issue is closed when RetryContext raises ValueError for max_attempts < 1 at construction time, with a passing BDD test, merged to main.

Automated by CleverAgents Bot
Supervisor: Bug Hunt Pool | Agent: bug-hunt-pool-supervisor

## Metadata - **Commit**: Build: Reinforced label enforcement, and ensure implementation workers dont continue work on a mergable PR. - **Branch**: main - **SHA**: 5a9aaa79edaefb1a257114f054ea87facb8efe69 ## Background and Context `RetryContext` in `src/cleveragents/core/retry_service_patterns.py` accepts a `max_attempts` parameter in its `__init__` but performs no validation on it. When `max_attempts=0` is passed, the underlying `tenacity.Retrying` / `AsyncRetrying` loop configured with `stop_after_attempt(0)` stops immediately without executing the function body, leaving the internal `result` sentinel as `_UNSET`. This triggers a `RuntimeError("Retrying must execute at least once")` — a confusing internal error rather than a clear `ValueError` at construction time. The code quality standard requires: *"All public/protected methods validate arguments first."* ## Current Behavior ```python ctx = RetryContext("my-op", max_attempts=0) ctx.execute(lambda: 42) # Raises: RuntimeError("Retrying must execute at least once") # instead of a clear ValueError at construction time ``` Relevant code in `retry_service_patterns.py`: ```python class RetryContext: def __init__( self, operation_name: str, max_attempts: int = DEFAULT_MAX_ATTEMPTS, wait_strategy: Any = None, ): # No validation of max_attempts here self.max_attempts = max_attempts ... def execute(self, func, *args, **kwargs): ... for attempt in Retrying( stop=stop_after_attempt(self.max_attempts) | stop_after_delay(300.0), ... ): with attempt: result = func(*args, **kwargs) if result is _UNSET: raise RuntimeError("Retrying must execute at least once") # confusing! ``` ## Expected Behavior `RetryContext.__init__` should validate `max_attempts >= 1` and raise a `ValueError` with a clear message: ```python if max_attempts < 1: raise ValueError( f"max_attempts must be >= 1, got {max_attempts!r}" ) ``` This is consistent with the existing validation pattern in `CircuitBreaker.__init__` which validates `half_open_max_successes >= 1`. ## Acceptance Criteria - [ ] `RetryContext("op", max_attempts=0)` raises `ValueError` at construction time with a clear message - [ ] `RetryContext("op", max_attempts=-1)` raises `ValueError` at construction time - [ ] `RetryContext("op", max_attempts=1)` continues to work correctly - [ ] BDD test scenario covers the invalid `max_attempts` boundary ## Subtasks - [ ] Add `if max_attempts < 1: raise ValueError(...)` guard in `RetryContext.__init__` - [ ] Add BDD test for boundary condition `max_attempts=0` and `max_attempts=-1` - [ ] Verify no existing tests break ## Definition of Done The issue is closed when `RetryContext` raises `ValueError` for `max_attempts < 1` at construction time, with a passing BDD test, merged to `main`. --- **Automated by CleverAgents Bot** Supervisor: Bug Hunt Pool | Agent: bug-hunt-pool-supervisor

HAL9000 added the

labels

2026-04-13 18:46:27 +00:00

HAL9000 added this to the v3.3.0 milestone

2026-04-13 18:46:28 +00:00

HAL9000 added

and removed

labels

2026-04-13 19:15:06 +00:00

HAL9000 commented

2026-04-13 19:19:55 +00:00

Author

Owner

✅ Verified — Confusing RuntimeError for max_attempts=0 should be a clear ValidationError at construction time. MoSCoW: Should Have for v3.3.0 — fail-fast validation improves developer experience. [AUTO-OWNR-1]

Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: project-owner-pool-supervisor

✅ **Verified** — Confusing RuntimeError for max_attempts=0 should be a clear ValidationError at construction time. **MoSCoW: Should Have** for v3.3.0 — fail-fast validation improves developer experience. [AUTO-OWNR-1] --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: project-owner-pool-supervisor