Skip to content

fix(workflow): treat RetryConfig(max_attempts=0) as no retries#6294

Open
Osamaali313 wants to merge 3 commits into
google:mainfrom
Osamaali313:fix-retry-max-attempts-zero
Open

fix(workflow): treat RetryConfig(max_attempts=0) as no retries#6294
Osamaali313 wants to merge 3 commits into
google:mainfrom
Osamaali313:fix-retry-max-attempts-zero

Conversation

@Osamaali313

Copy link
Copy Markdown

Fixes #6293

Problem

RetryConfig.max_attempts is documented as "Maximum number of attempts, including the original request. If 0 or 1, it means no retries. If not specified, default to 5." and defaults to None. But _should_retry_node resolved the default with a falsy-coalescing or:

# src/google/adk/workflow/utils/_retry_utils.py:35
max_attempts = retry_config.max_attempts or 5

0 is falsy, so RetryConfig(max_attempts=0) — documented as "no retries" — is silently replaced with 5, running 4 retries instead of none. max_attempts=1 happens to work (1 or 5 == 1); only the documented 0 case is broken. The intended "unset" sentinel is None, which must still map to 5.

Workflow and RetryConfig are public API (both in __all__, no @experimental gate), and this drives every workflow node's retry loop via _NodeRunner._attempt_retry.

Fix

max_attempts = (
    retry_config.max_attempts if retry_config.max_attempts is not None else 5
)

Tests

Added TestShouldRetryNode to tests/unittests/workflow/utils/test_retry_utils.py (the file previously covered only _get_retry_delay), asserting:

  • max_attempts of 0 and 1 disable retries
  • max_attempts=3 retries until the limit
  • unset (None) defaults to 5

Before the fix, the max_attempts=0 case fails; after, it passes. The full tests/unittests/workflow/ suite passes (619 passed, no regressions).

RetryConfig.max_attempts documents "If 0 or 1, it means no retries. If
not specified, default to 5." and defaults to None. _should_retry_node
resolved the default with `retry_config.max_attempts or 5`, which treats
an explicit 0 as falsy and replaces it with 5, so a node configured for
no retries ran up to 5 attempts. Use an explicit None check so 0 and 1
disable retries while the unset (None) case still defaults to 5.
@rohityan rohityan added the workflow [Component] This issue is related to ADKworkflow label Jul 6, 2026
Wrap the long _should_retry_node assertions to satisfy the repo's pyink
(line-length 80) pre-commit hook. No behavior change.
@Osamaali313

Copy link
Copy Markdown
Author

Pushed a small formatting commit — pyink (line-length 80) wanted the long _should_retry_node assertions in test_retry_utils.py wrapped; I ran pyink with the repo config and it's now clean (pyink --check passes, tests still green: 10 passed).

Heads up on the other red check: the Mypy failure isn't from this PR's diff — the "new" errors it lists are all in src/google/adk/models/lite_llm.py (_finish_reason_to_error_message arg-type), which this PR doesn't touch. Both baseline and PR report 3375 errors; it looks like the baseline-diff surfaced a pre-existing lite_llm.py typing issue after the main merge. Happy to help if you'd like, but it's outside this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

workflow [Component] This issue is related to ADKworkflow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RetryConfig(max_attempts=0) runs 5 attempts instead of disabling retries

2 participants