fix(core): a non-positive retry budget no longer skips the wrapped call

[DABH]

Description

nat.utils.exception_handlers.automatic_retries is the retry layer that provider and client plugin packages build on. The provider-authoring guide (docs/source/extend/custom-components/adding-an-llm-provider.md) tells plugin authors to add RetryMixin to their provider config, and the first-party client packages wire RetryMixin.num_retries straight into patch_with_retry so every public client method retries on retryable errors (for example packages/nvidia_nat_langchain/src/nat/plugins/langchain/llm.py) — the pattern third-party client packages follow.

All four wrapper variants produced by _retry_decorator (plain sync, coroutine, sync generator, and async generator) iterate for attempt in range(retries). When the budget is 0 or negative, the range is empty and the wrapped callable is never invoked at all:

• the sync and coroutine wrappers fall through and return None,
• the generator wrappers complete without yielding anything.

No exception is raised and nothing is logged, so the failure mode is a silent no-op. Nothing validates the budget upstream either: RetryMixin.num_retries is an unconstrained int that first-party and third-party provider packages pass straight into patch_with_retry(retries=...). A user who sets num_retries: 0 in a workflow configuration (a natural way to express "do not retry") turns every patched LLM, embedder, or memory client method into a call that never reaches the provider and quietly returns None or an empty stream. That is very hard to debug from inside a plugin package, and it is a correctness trap for any plugin author who exposes the retry configuration to end users.

This change makes the wrapped callable always execute at least once:

• Normalize the budget once in _retry_decorator (total_attempts = max(1, retries)) and use it consistently in all four wrapper variants, both for the attempt loop and for the last-attempt re-raise check.
• Log the normalization once at DEBUG level. It happens in the decorator factory, which runs once per patch_with_retry call rather than per patched method or per invocation, so the adjustment is observable without adding log noise.
• Document the semantics in the module docstring and in the _retry_decorator and patch_with_retry docstrings: the budget counts total attempts (the initial call plus any retries), and values below 1 behave as 1.
• Behavior for budgets >= 1 is unchanged; the existing tests that pin attempt counts on persistent failure pass unmodified, and a new boundary case pins budget 1 (exactly one attempt on success and on failure), the point where the normalization becomes a no-op.
• Add tests covering budgets of 0, -1, and the boundary budget of 1 for all four wrapper variants (real result returned, exactly one invocation), failure propagation after a single attempt, and the patch_with_retry public path. The eleven non-positive-budget cases fail against the previous implementation; the budget-1 cases pass against both implementations.

Clamping rather than raising is deliberate: num_retries has never been constrained, so configurations with a non-positive budget load and run today (however incorrectly), and raising would turn them into hard startup failures — normalizing to a single attempt instead gives num_retries: 0 the semantics its author plainly intended (call once, never retry).

No tracking issue exists for this yet; happy to file one if the team prefers.

Testing

• uv run --project packages/nvidia_nat_core -- pytest packages/nvidia_nat_core/tests/nat/utils/test_retry_wrapper.py -q — 46 passed (30 pre-existing tests unchanged, 16 new).
• Verified the new tests capture the bug: with the previous implementation restored, the same command reports 11 failed, 35 passed — exactly the non-positive-budget cases fail, while the budget-1 boundary cases pass on both implementations, as intended.
• uv run pre-commit run yapf --files packages/nvidia_nat_core/src/nat/utils/exception_handlers/automatic_retries.py packages/nvidia_nat_core/tests/nat/utils/test_retry_wrapper.py — Passed.
• uv run pre-commit run ruff-check --files packages/nvidia_nat_core/src/nat/utils/exception_handlers/automatic_retries.py packages/nvidia_nat_core/tests/nat/utils/test_retry_wrapper.py — Passed.
• uv run python ci/scripts/copyright.py --verify-apache-v2 — passed.

By Submitting this PR I confirm:

• I am familiar with the Contributing Guidelines.
• We require that all contributors "sign-off" on their commits. This certifies that the contribution is your original work, or you have rights to submit it under the same license, or a compatible license.
  • Any contribution which contains commits that are not Signed-Off will not be accepted.
• When the PR is ready for review, new or existing tests cover these changes.
• When the PR is ready for review, the documentation is up to date with these changes.

Summary by CodeRabbit

• Bug Fixes

  • Retry settings below one now consistently execute the operation once across synchronous, asynchronous, and generator-based operations.
  • Failures from the single allowed attempt are surfaced correctly.
  • Retry budgets are interpreted as total attempts for more predictable behavior.

• Documentation

  • Clarified retry attempt-budget behavior.

• Tests

  • Added coverage for zero, negative, and one-attempt settings across supported execution types.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 6e819877-27de-492b-88af-7441a17f3707

📥 Commits

Reviewing files that changed from the base of the PR and between f0c5a49 and f07fde4.

📒 Files selected for processing (1)

• packages/nvidia_nat_core/src/nat/utils/exception_handlers/automatic_retries.py

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/nvidia_nat_core/src/nat/utils/exception_handlers/automatic_retries.py

---

Walkthrough

The retry decorator now treats retries as a total-attempt budget. Values below one normalize to one attempt across synchronous, asynchronous, generator, and patched-method wrappers. Tests cover successful and failing minimal-budget cases.

Changes

Retry budget behavior

Layer / File(s)	Summary
Normalize retry budgets packages/nvidia_nat_core/src/nat/utils/exception_handlers/automatic_retries.py	The retry implementation normalizes budgets below one and uses the normalized value for all wrapper attempt loops and termination checks. Documentation describes the total-attempt behavior.
Validate minimal retry budgets packages/nvidia_nat_core/tests/nat/utils/test_retry_wrapper.py	Tests verify that budgets of 0, -1, and 1 execute successful callables and generators once, propagate first-attempt failures, and invoke patched methods once.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise, descriptive, within 72 characters, and clearly describes the retry-budget fix.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

packages/nvidia_nat_core/src/nat/utils/exception_handlers/automatic_retries.py (1)

447-453: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the required Google-style docstring.

patch_with_retry is a public function. Replace the NumPy-style Retry budget section with a Google-style Args: section. Format parameter names as code entities.

As per coding guidelines, “Provide Google-style docstrings for every public module, class, function and CLI command” and “Surround code entities with backticks.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/nvidia_nat_core/src/nat/utils/exception_handlers/automatic_retries.py`
around lines 447 - 453, Update the public function `patch_with_retry` docstring
by replacing the NumPy-style “Retry budget” section with a Google-style `Args:`
section. Document `retries` there and surround the parameter name with
backticks, while preserving the existing description of attempts and minimum
behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@packages/nvidia_nat_core/src/nat/utils/exception_handlers/automatic_retries.py`:
- Around line 447-453: Update the public function `patch_with_retry` docstring
by replacing the NumPy-style “Retry budget” section with a Google-style `Args:`
section. Document `retries` there and surround the parameter name with
backticks, while preserving the existing description of attempts and minimum
behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 06ab08a6-50a7-4447-bda1-5e9728ac02e8

📥 Commits

Reviewing files that changed from the base of the PR and between baf355b and f0c5a49.

📒 Files selected for processing (2)

• packages/nvidia_nat_core/src/nat/utils/exception_handlers/automatic_retries.py
• packages/nvidia_nat_core/tests/nat/utils/test_retry_wrapper.py

[chuenchen309]

Checked this against develop. The bug is real, the fix is right, and the reachability argument is stronger than the description makes it — worth adding, because it's the part a reviewer will want before merging.

automatic_retries.py imports only the standard library, so I loaded the file straight off develop and drove all four wrapper shapes rather than transcribing anything:

  --- retries=3  (control) ---
    sync      -> 'sync-result'      generator -> ['gen-item']
    coroutine -> 'coro-result'      async gen -> ['agen-item']
    wrapped callable actually invoked: ['sync', 'coro', 'gen', 'agen']

  --- retries=0 ---
    sync      -> None               generator -> []
    coroutine -> None               async gen -> []
    wrapped callable actually invoked: NEVER

-1 behaves the same. So it isn't only that the call is skipped — the sync and coroutine wrappers hand the caller None and the two generator wrappers hand back an empty sequence, both of which look like a legitimate result rather than a failure.

Why a non-positive budget is reachable. RetryMixin.num_retries is Field(default=5, ...) with no bound — no ge, no gt:

num_retries= 5  -> ACCEPTED        num_retries= 0  -> ACCEPTED
num_retries= 1  -> ACCEPTED        num_retries=-1  -> ACCEPTED

So num_retries: 0 in a workflow YAML validates cleanly, and it's the obvious way for someone to write "don't retry this provider" — do_auto_retry: false is the intended switch, but nothing points you there or rejects the other spelling. The user gets an LLM client whose methods silently return None.

The fix doesn't change the normal path. Counting invocations of an always-failing callable, before and after the patch:

              develop        with #2146
retries=1     1 time(s)      1 time(s)
retries=3     3 time(s)      3 time(s)
retries=5     5 time(s)      5 time(s)

Identical, which confirms the docstring addition ("counts total attempts: the initial call plus any retries") is documenting the existing semantics rather than redefining them — worth stating explicitly, since "normalize to at least 1 attempt" could otherwise read like a behaviour change to the budget.

One detail worth calling out because it's easy to miss when reviewing: changing attempt == retries - 1 to attempt == total_attempts - 1 isn't cosmetic. Left alone with retries=0, that comparison is against -1 and never matches, so the last-attempt branch would stop raising. Both had to move together, and they did in all four wrappers.

Nothing blocking from me. The only thing I'd consider is whether RetryMixin.num_retries should also carry ge=1, so the config surface rejects the value instead of the decorator quietly repairing it — but that's a separate call about config validation, and this PR is the right fix for the layer it's touching.

AI-assisted review; the outputs above come from executing the module as fetched from develop, and from applying this PR's change to it, not from reading. I have no stake in this PR.