feat(retry): add pluggable backoff strategies with jitter to Python [replica of harness-sdk#4960] by poshinchen · Pull Request #17 · poshinchen/sdk-python

poshinchen · 2026-10-09T01:18:56Z

Replica of https://github.com/strands-agents/harness-sdk/pull/4960 (URL in backticks to avoid a cross-reference on the original PR) for reviewer evaluation. Branches mirror the original PR's base and head commits.

The review below was produced by the optimized Strandly reviewer (variant v3-reason, optimization run aeb3ee, agent model Opus 4.8) during validation on PRs it never saw in training. Compare with the human and Strandly reviews on the original PR.

Port the TypeScript backoff library to strands-py as strands.retry: ConstantBackoff, LinearBackoff and ExponentialBackoff with none/full/equal/ decorrelated jitter, plus RetryDecision and an overridable ModelRetryStrategy.compute_retry_decision. ModelRetryStrategy stays the concrete default and keeps its seconds-based constructor and attempt budget semantics. AfterModelCallEvent now reports attempt_count. Refs strands-agents#3151

poshinchen

Strandly v3-reason (optimized) review

Assessment: comment

Assessment: Comment

Well-structured port of pluggable backoff/jitter to Python with thoughtful handling of backward compatibility (seconds-based args, is_retryable, budget semantics preserved from main). The deprecation re-export in _retry.py and the thorough docstrings are commendable. A few items warrant verification before merge.

Review Categories

API design: New public abstractions (BackoffStrategy, backoff classes, RetryDecision) require formal API bar-raising sign-off; confirm the api/needs-review label.
Correctness: Verify sync/async dispatch of compute_retry_decision and attempt-budget accounting when subclasses short-circuit before super().
Robustness: random global state for jitter and silent degradation of inverted max_delay < base_delay ranges in decorrelated jitter.
Coupling: Router still reaches into private retry-strategy internals; a public reset seam would be safer.
Testing: Prefer full-event equality over per-field assertions; add coverage for an async compute_retry_decision override.

This cleanly closes the SDK parity gap with TypeScript while respecting Python's existing retry semantics.

File-level comments:

strands-py/src/strands/retry/__init__.py: Important

Issue: This is a new public API surface (ExponentialBackoff, LinearBackoff, ConstantBackoff, BackoffStrategy, BackoffContext, JitterKind, RetryDecision) exported from strands and strands.retry. Per the API bar-raising process, new customer-facing abstractions require API review.
Suggestion: Please confirm the PR carries the api/needs-review (or api/review-complete) label. The description includes signatures and examples, which is good; a designated API reviewer should still sign off given the new extensible protocol.

poshinchen · 2026-10-09T01:18:58Z

+    if kind == "equal":
+        return raw / 2 + random.random() * (raw / 2)
+    if last_delay is None:
+        return random.random() * raw


Important

Issue: Jitter uses the random module, whose global state is shared process-wide and not thread-safe for concurrent next_delay calls; it also can't be seeded deterministically per-strategy for reproducible tests.
Suggestion: Please consider using a per-instance random.Random() instance (optionally seedable) so jitter is isolated from global random state and testable without monkeypatching the module.

poshinchen · 2026-10-09T01:18:58Z

+
+class ConstantBackoff:
+    """Constant backoff: returns the same delay for every retry."""
+


Suggestion

Issue: In the decorrelated branch, when max_delay < base_delay the max(base_delay, ...) guard forces upper == base_delay, so the jitter range collapses to zero and the delay is always base_delay — silently ignoring the inverted config.
Suggestion: Consider validating max_delay >= base_delay in __init__ (for Linear/Exponential) and raising ValueError rather than silently degrading, so misconfiguration surfaces early.

poshinchen · 2026-10-09T01:18:58Z

+        return RetryDecision(retry=True, delay=delay)
+
+    def register_hooks(self, registry: HookRegistry, **kwargs: Any) -> None:
+        """Register callbacks for AfterModelCallEvent and AfterInvocationEvent.


Important

Issue: compute_retry_decision mutates state (self._current_attempt += 1, self._first_failure_at) before computing the delay. A subclass override that calls super().compute_retry_decision(event) conditionally (as shown in the PR docs' TimeoutRetryStrategy) can leave _current_attempt out of sync when it returns its own decision without calling super.
Suggestion: Please verify the attempt-budget accounting stays correct across subclass overrides that short-circuit before super(), and document that overrides which retry independently should not rely on the base counter.

poshinchen · 2026-10-09T01:18:58Z

+
+        Returns:
+            True if the exception should trigger a retry, False otherwise.
+        """


Important

Issue: compute_retry_decision is declared to return RetryDecision | Awaitable[RetryDecision] (sync or async). The dispatch logic that awaits the result when it's a coroutine is in the truncated _handle_after_model_call portion and not visible here.
Suggestion: Please verify the caller uses inspect.isawaitable() (note inspect is imported) to await async overrides, and that a sync override returning a plain RetryDecision is never mistakenly awaited. A test covering an async def compute_retry_decision override would guard this contract.

poshinchen · 2026-10-09T01:18:58Z

+        Args:
+            base_delay: Base delay in seconds. Defaults to 1.
+            max_delay: Upper bound in seconds applied before jitter. Defaults to 30.
+            jitter: Jitter mode. Defaults to ``"full"``.


Suggestion

Issue: The default jitter for LinearBackoff/ExponentialBackoff is "full", but ModelRetryStrategy's implicit default backoff uses jitter="none". This divergence is intentional (per the description) but easy to miss for users who construct a backoff explicitly expecting it to match the old no-jitter default.
Suggestion: Consider calling out this difference in the ModelRetryStrategy.backoff docstring/Args so the behavior change (no jitter by default vs. full jitter when composing a backoff) is explicit.

poshinchen · 2026-10-09T01:18:58Z

            # ModelRetryStrategy exposes no public seam for a budget reset yet, so a rename in
            # _retry.py breaks this.
            event.agent._retry_strategy._reset_retry_state()
+            event._restart_attempts()


Suggestion

Issue: The router comment (visible on line 388-390 context) notes ModelRetryStrategy exposes no public seam for a budget reset yet, so a rename in _retry.py breaks this, and _advance reaches into event.agent._retry_strategy._reset_retry_state() plus the new event._restart_attempts().
Suggestion: Please consider exposing a small public/protected reset hook on the retry strategy to remove this fragile private-attribute coupling; it will reduce the chance a future refactor silently breaks fallback budget resets.

poshinchen · 2026-10-09T01:18:58Z

+    result = await agent.invoke_async("test prompt")
+
+    assert result.message["content"][0]["text"] == "Success"
+    assert mock_sleep.sleep_calls == [1, 2]


Suggestion

Issue: The test asserts only mock_sleep.sleep_calls and the result text, but AfterModelCallEvent now carries attempt_count; the hook-budget test (test_hook_requested_retries_do_not_consume_retry_budget) checks attempt_counts separately rather than asserting the full event shape.
Suggestion: Where feasible, assert the full AfterModelCallEvent equality (as test_event_loop.py now does with attempt_count=) so regressions in other event fields aren't silently missed. Per-field assertions can hide unexpected field changes.

poshinchen · 2026-10-09T01:18:58Z

    """Event emitted when the event loop is throttled due to rate limiting."""

-    def __init__(self, delay: int) -> None:
+    def __init__(self, delay: float) -> None:


Suggestion

Issue: EventLoopThrottleEvent.__init__ signature changed from delay: int to delay: float. This is a widening of a public-ish event constructor; existing callers passing ints still work, but any type-checked downstream subclass/override would see a signature change.
Suggestion: Please verify this event is not part of a documented stable contract that would consider the parameter type a breaking change, and note the widening in the changelog if applicable.

poshinchen · 2026-10-09T01:18:58Z

    stop_response: ModelStopResponse | None = None
    exception: Exception | None = None
    retry: bool = False
+    attempt_count: int = 1


Suggestion

Issue: attempt_count has a mutable-sounding sibling _restart_attempt_count set via object.__setattr__, implying this event is frozen; attempt_count itself is a public read field but _can_write only permits retry.
Suggestion: Please confirm hooks reading attempt_count cannot accidentally mutate it (good, since _can_write restricts writes), and that the 1-based semantics plus reset-on-candidate-switch behavior documented here are covered by the router test asserting [1,2,3,1,2,3].

github-actions · 2026-10-09T01:22:21Z

Documentation Preview Failed

The documentation deployment encountered an error. Please check the deployment logs for more details.

poshinchen

Strandly v4-reason (optimized) review — run af1746, Opus 4.8, upgraded harness (read_file + suggestion-aware reward)

Assessment: approve

Assessment: Approve

Well-structured port of pluggable backoff/jitter to Python. The separation of pure delay math (BackoffStrategy) from retry policy (ModelRetryStrategy) is clean, backward compatibility is preserved (legacy _retry.py re-export, unchanged defaults/semantics), and the mutually-exclusive backoff vs initial_delay/max_delay guard is a good call. Documentation and tests are updated alongside the code.

Review Categories

API design: Public surface is coherent and matches the documented examples; override points are clearly described.
Validation: Numeric backoff args (delay/base_delay/max_delay/multiplier) aren't validated at construction while RetryDecision validates the final delay — an inconsistency worth closing.
Maintainability: compute_retry_decision carries non-obvious side effects for subclasses, and the router↔strategy↔event coupling for budget/attempt resets remains fragile.
Testing: Good coverage; a couple of behaviors (event attempt-count reset) would benefit from direct unit tests.

None of the comments are blocking — nice cross-SDK consistency work.

poshinchen · 2026-10-09T20:03:28Z

+        synchronous or ``async``; the base implementation is synchronous.
+
+        Args:
+            event: The AfterModelCallEvent for the failed model call.


Important
Issue: compute_retry_decision is a documented public override point, yet it mutates instance state (_current_attempt, _first_failure_at, _last_delay) as a side effect. A subclass that returns early for its own cases without calling super() silently skips the budget increment, and one that calls super() for the fallthrough gets the increment — behavior that's easy to get wrong and hard to spot.
Suggestion: Consider documenting explicitly in the docstring that calling super().compute_retry_decision() is what advances the attempt budget, and that subclasses handling a case themselves bypass it. A brief note here prevents subtle budget bugs in overrides.

poshinchen · 2026-10-09T20:03:28Z

+    def next_delay(self, context: BackoffContext) -> float:
+        """Return the configured delay.
+
+        Args:


Important
Issue: Constructors validate jitter but not the numeric delay args. A negative or non-finite base_delay/max_delay/delay/multiplier silently flows through to asyncio.sleep, where RetryDecision would reject the same negative delay. This is an inconsistent contract: the final sleep value is validated downstream only in some paths.
Suggestion: Validate delay/base_delay/max_delay (and multiplier) are non-negative and finite at construction time, mirroring _validate_jitter, so misconfiguration fails fast at the call site rather than mid-retry.

poshinchen · 2026-10-09T20:03:28Z

+    if kind == "equal":
+        return raw / 2 + random.random() * (raw / 2)
+    if last_delay is None:
+        return random.random() * raw


Suggestion
Issue: In decorrelated mode, the raw argument (the capped exponential/linear value) is computed by the caller but completely ignored — the delay is derived solely from last_delay/base_delay. This is correct per the standard decorrelated-jitter algorithm, but a reader may be surprised that the exponential growth path is discarded here.
Suggestion: A one-line comment noting that decorrelated jitter intentionally ignores raw and derives the delay from last_delay would save future readers the double-take.

poshinchen · 2026-10-09T20:03:28Z

+    result = await agent.invoke_async("test prompt")
+
+    assert result.message["content"][0]["text"] == "Success"
+    assert mock_sleep.sleep_calls == [1, 2]


Suggestion
Issue: test_hook_requested_retries_do_not_consume_retry_budget asserts the key behavior via two separate per-field checks (sleep_calls and attempt_counts), which is fine, but the test name and the PR's headline guarantee ("hook retries don't consume budget") would be clearer if it also asserted the throttle strategy still had its full budget available — i.e. that it reached max_attempts=3 on throttles after the two ValueError retries.
Suggestion: The current sleep_calls == [1, 2] indirectly proves this; consider a short comment tying those two sleeps to the two throttle retries so the intent survives future edits.

poshinchen · 2026-10-09T20:03:28Z

            # ModelRetryStrategy exposes no public seam for a budget reset yet, so a rename in
            # _retry.py breaks this.
            event.agent._retry_strategy._reset_retry_state()
+            event._restart_attempts()


Suggestion
Issue: The router comment (above this line, retained from main) warns that ModelRetryStrategy exposes no public seam for a budget reset yet, so a rename in _retry.py breaks this. This PR adds a second coupling: event._restart_attempts() plus the private _reset_retry_state() reach into internals across module boundaries.
Suggestion: Since this PR touches both sides, consider whether a small internal protocol (e.g. a _reset_for_candidate_switch() seam that encapsulates both the budget reset and attempt restart) would reduce the fragile cross-module coupling the existing comment already flags.

poshinchen · 2026-10-09T20:03:28Z

+
+    def _next_attempt_count(self) -> int:
+        """Attempt count for the model call that follows a retry of this one."""
+        return 1 if self._restart_attempt_count else self.attempt_count + 1


Suggestion
Issue: _next_attempt_count / _restart_attempts encode the "reset on candidate switch" rule, but attempt_count is a public, documented field customers read in hooks. The reset semantics ("resets to 1 when model routing switches candidates") are only discoverable from the docstring, not from the event's behavior.
Suggestion: Consider adding a focused unit test on AfterModelCallEvent._next_attempt_count() covering both the normal increment and the post-_restart_attempts() reset, so the contract is pinned independently of the router integration test.

poshinchen · 2026-10-09T20:03:32Z

Live Strandly's actual comments on the original PR 4960 (copied for comparison; mentions escaped and links de-linked so nothing is pinged or cross-referenced)

Review (COMMENTED):

No blockers from me. Every finding from my strands-agents#4398 review is fixed here (budget no longer drained by other hooks' retries, backoff=+initial_delay raises, bad jitter raises at construction, dead _calculate_delay gone). The one thing I'd still fix before merge is a docs inconsistency this PR creates: hooks.mdx "Model Call Retry" now has the TS tab saying no manual counter is needed while the Python tab beside it still hand-rolls one.

Not approving solo because BackoffStrategy/RetryDecision/compute_retry_decision are new public abstractions and the PR has no api/needs-review label (the bot already said this; +1).
🟡 docs: hooks.mdx Python example contradicts the TS tab after this PR

retry-strategies.mdx:266 sends readers to hooks.md#model-call-retry. That page's Python tab (site/src/content/docs/user-guide/sdk/agents/hooks.mdx:668-693) still keeps self.retry_count, resets it on BeforeInvocationEvent, and sleeps 2 ** self.retry_count; the TS tab in the same <Tabs> block (hooks.ts:208-209) says "attemptCount is 1-indexed and includes the attempt that just failed, so no manual counter is needed". Same rewrite you already did in retry-strategies.mdx:239-251, one file over:
import asyncio
import logging
from strands.hooks import HookProvider, HookRegistry, AfterModelCallEvent

logger = logging.getLogger(__name__)

class RetryOnServiceUnavailable(HookProvider):
    """Retry model calls when ServiceUnavailable errors occur."""

    def __init__(self, max_retries: int = 3):
        self.max_retries = max_retries

    def register_hooks(self, registry: HookRegistry) -> None:
        registry.add_callback(AfterModelCallEvent, self.handle_retry)

    async def handle_retry(self, event: AfterModelCallEvent) -> None:
        # attempt_count is 1-based and includes the attempt that just failed,
        # so no manual counter or per-invocation reset is needed.
        if (
            event.exception
            and "ServiceUnavailable" in str(event.exception)
            and event.attempt_count <= self.max_retries
        ):
            logger.info("ServiceUnavailable encountered, retrying model call")
            await asyncio.sleep(2 ** event.attempt_count)
            event.retry = True
Questions (non-blocking)

BackoffContext.attempt is the strategy's own retry count (model_retry_strategy.py:132), not event.attempt_count as in TS. That's the documented divergence and I think the right call — but retry-strategies.mdx:110 just says "the 1-based attempt"; one sentence saying it excludes other hooks' retries would stop a TS-ported backoff from surprising someone.

compute_retry_decision overrides that return their own RetryDecision(retry=True) without super() have no max_attempts bound (the budget lives inside the base method, :118-125). Verified: max_attempts=2 retried 10 TimeoutErrors. "Full control" is the intent, so fine — worth one line in the docstring?

BackoffContext.elapsed is populated (:133) but nothing shipped reads it. Forward-compat with TS elapsedMs, or wait until something uses it?

event_loop.py:53, agent/agent.py:36, agent/__init__.py:12 still import via event_loop._retry, whose docstring now calls itself the legacy path. Intentional (the import-cycle note in the PR body), or should core import ..retry and keep the shim for users only?

✅ Verified at ea611a0

pytest tests/strands/{retry,agent,event_loop,models/routing,hooks} → 1247 passed; ruff check / ruff format --check clean on the touched files. The full unit suite couldn't complete in my sandbox (infra timeouts), so I'm relying on CI for the rest.

Default ladder unchanged vs main: [4, 8, 16, 32, 64] through a real Agent + mocked model; event_loop_throttled_delay still emitted (now float).

Budget semantics: hook-requested retries interleaved with throttles → strategy sleeps [1, 2], budget untouched by the hook's retries (the feat: add pluggable retry backoff to Python strands-agents/harness-sdk#4398 🔴).

attempt_count at all three AfterModelCallEvent sites (event_loop.py:553/644/674): exception path [1,2,3], success-path hook retry [1,2,3], BeforeModelCallEvent.cancel path [1,2]; router switch A→B → [1,2,3,1,2,3] with a fresh budget on B; the restart applies to the next event only.

Jitter math vs strands-ts/src/retry/backoff-strategy.ts: 10k-sample bounds (full ⊂ [0,raw], equal ⊂ [raw/2,raw], decorrelated ⊂ [base, min(max,3·last)]) → 0 violations; max_delay < base_delay guard identical to TS.

RetryDecision rejects None/NaN/±inf/negative with ValueError chained on the original ModelThrottledException (not masked).

Import-cycle check, fresh interpreter each: import strands, from strands.event_loop._retry import ModelRetryStrategy, import strands.retry (+9 variants) all OK; shim class is the strands.retry class.

Shared instance across two concurrent agents interleaves state — identical on main, so not a regression; docs now say don't share (retry-strategies.mdx:210).

Appendix — non-blocking (4)

⚪ router.py:391-392 comment "a rename in _retry.py breaks this" is stale — the class lives in strands/retry/model_retry_strategy.py.

⚪ model_retry_strategy.py:203 or decision.delay is None is unreachable: RetryDecision.__post_init__ already rejects retry=True, delay=None.

⚪ EventLoopThrottleEvent (:206) is now emitted for any strategy-decided retry, including non-throttle ones from a compute_retry_decision override. Cosmetic; streaming/events.mdx already hedges.

⚪ _DEFAULT_INITIAL_DELAY/_DEFAULT_MAX_DELAY (:23-24) duplicate event_loop.INITIAL_DELAY/MAX_DELAY; Agent() passes the latter explicitly so no drift today.

jambrubu and others added 2 commits October 6, 2026 16:25

Merge branch 'main' into agent-tasks/3151-retry-backoff

ea611a0

poshinchen commented Oct 9, 2026

View reviewed changes

poshinchen had a problem deploying to auto-approve October 9, 2026 01:19 — with GitHub Actions Failure

poshinchen commented Oct 9, 2026

View reviewed changes


		class ConstantBackoff:
		"""Constant backoff: returns the same delay for every retry."""

Conversation

poshinchen commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poshinchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 9, 2026

Documentation Preview Failed

Uh oh!

poshinchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poshinchen commented Oct 9, 2026

Uh oh!

This branch had an error being deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

poshinchen commented Oct 9, 2026 •

edited

Loading