Skip to content

feat: add retry_after and delay_multiplier to LLMError for generalized backoff - #24

Merged
Brian Krabach (bkrabach) merged 5 commits into
mainfrom
feat/retry-backoff-generalization
Feb 28, 2026
Merged

feat: add retry_after and delay_multiplier to LLMError for generalized backoff#24
Brian Krabach (bkrabach) merged 5 commits into
mainfrom
feat/retry-backoff-generalization

Conversation

@bkrabach

@bkrabach Brian Krabach (bkrabach) commented Feb 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

Promotes retry_after from RateLimitError to the LLMError base class and adds a new delay_multiplier attribute. This enables any error type to carry server-directed wait hints and error-severity backoff scaling.

Motivation

Anthropic's overloaded_error (HTTP 529) is a transient error where the server is too busy. Previously, retry_after was scoped to RateLimitError only, meaning overloaded errors couldn't carry server timing hints and couldn't signal that longer backoff is appropriate. This change makes both capabilities available to any error type.

Changes

llm_errors.py

  • Add retry_after: float | None = None to LLMError.__init__ — server-directed wait hint
  • Add delay_multiplier: float = 1.0 to LLMError.__init__ — error-severity scaling factor
  • RateLimitError now forwards retry_after via super().__init__() instead of defining its own
  • ProviderUnavailableError accepts both new kwargs
  • __repr__ includes both fields when non-default

utils/retry.py

  • delay_multiplier applied after max_delay cap (can exceed cap — intentional)
  • isinstance(e, RateLimitError) guard removed — retry_after honored on any LLMError
  • Order of operations: base exponential → cap → multiplier → server hint → jitter
  • honor_retry_after docstring generalized

Backward Compatibility

Fully backward compatible:

  • retry_after defaults to None — no behavior change unless explicitly set
  • delay_multiplier defaults to 1.0 — no behavior change unless explicitly set
  • Existing RateLimitError construction with retry_after=X still works identically

Testing

  • 12 new tests in test_llm_errors.py for new attributes, repr, inheritance
  • 6 new tests in test_retry.py for multiplier math, generalized retry_after, combined behavior
  • All 302 existing tests continue to pass

@bkrabach @microsoft-amplifier
feat: add retry_after and delay_multiplier to LLMError base class
5dad10a 
Add retry_after (float | None) and delay_multiplier (float) as keyword-only
parameters on LLMError.__init__, with defaults None and 1.0 respectively.

- Update LLMError docstring to document new attributes
- Update LLMError.__repr__ to show retry_after when not None, delay_multiplier when != 1.0
- RateLimitError now forwards retry_after via super().__init__ instead of local assignment
- ProviderUnavailableError accepts and forwards both new kwargs
- All changes backward compatible (default values preserve existing behavior)
- 12 new tests: TestRetryAfterOnBaseClass (7) + TestReprWithNewFields (5)

🤖 Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach
feat: generalize delay_multiplier and retry_after in retry_with_backoff
891ca96 
- Apply error's delay_multiplier after max_delay cap (can exceed max_delay)
- Generalize retry_after from RateLimitError-only to any LLMError
- Update honor_retry_after docstring to reflect generic behavior
- Order: base_delay → cap → multiplier → retry_after floor → jitter

Tests: TestDelayMultiplier (3), TestRetryAfterOnAnyError (3)
@bkrabach @microsoft-amplifier
fix: update stale RetryConfig class docstring to match generalized re…
04b2188 
…try_after

🤖 Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach @microsoft-amplifier
style: move inline asyncio import to top-level in test_retry.py
07d77c1 
Moved import asyncio from inside the test_honor_retry_after_false_ignores_retry_after
test body to the top-level imports for consistency with the rest of the file.

Code quality review suggestion.

🤖 Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach @microsoft-amplifier
test: patch asyncio.sleep in slow retry_after test for faster CI
1679042 
Patched asyncio.sleep in test_retry_after_can_exceed_max_delay to avoid
a ~5 second real sleep during CI. The test now uses unittest.mock.patch on
asyncio.sleep (same pattern as the nearby test_retry_after_wins_over_multiplied_delay)
while still asserting the correct delay value via the on_retry callback.
Simplified the test to use AsyncMock side_effect instead of a manual closure.

Tests now run in 2.5s instead of ~7.5s.

🤖 Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach Brian Krabach (bkrabach) mentioned this pull request Feb 28, 2026
Merged
@bkrabach
Brian Krabach (bkrabach) merged commit 75a0542 into main Feb 28, 2026
1 check passed
Brian Krabach (bkrabach) added a commit that referenced this pull request Mar 7, 2026
@bkrabach @microsoft-amplifier
feat: add retry_after and delay_multiplier to LLMError for generalize…
1fb2481 
…d backoff (#24)

* feat: add retry_after and delay_multiplier to LLMError base class

Add retry_after (float | None) and delay_multiplier (float) as keyword-only
parameters on LLMError.__init__, with defaults None and 1.0 respectively.

- Update LLMError docstring to document new attributes
- Update LLMError.__repr__ to show retry_after when not None, delay_multiplier when != 1.0
- RateLimitError now forwards retry_after via super().__init__ instead of local assignment
- ProviderUnavailableError accepts and forwards both new kwargs
- All changes backward compatible (default values preserve existing behavior)
- 12 new tests: TestRetryAfterOnBaseClass (7) + TestReprWithNewFields (5)

🤖 Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

* feat: generalize delay_multiplier and retry_after in retry_with_backoff

- Apply error's delay_multiplier after max_delay cap (can exceed max_delay)
- Generalize retry_after from RateLimitError-only to any LLMError
- Update honor_retry_after docstring to reflect generic behavior
- Order: base_delay → cap → multiplier → retry_after floor → jitter

Tests: TestDelayMultiplier (3), TestRetryAfterOnAnyError (3)

* fix: update stale RetryConfig class docstring to match generalized retry_after

🤖 Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

* style: move inline asyncio import to top-level in test_retry.py

Moved import asyncio from inside the test_honor_retry_after_false_ignores_retry_after
test body to the top-level imports for consistency with the rest of the file.

Code quality review suggestion.

🤖 Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

* test: patch asyncio.sleep in slow retry_after test for faster CI

Patched asyncio.sleep in test_retry_after_can_exceed_max_delay to avoid
a ~5 second real sleep during CI. The test now uses unittest.mock.patch on
asyncio.sleep (same pattern as the nearby test_retry_after_wins_over_multiplied_delay)
while still asserting the correct delay value via the on_retry callback.
Simplified the test to use AsyncMock side_effect instead of a manual closure.

Tests now run in 2.5s instead of ~7.5s.

🤖 Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

---------

Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Ken Chau (kenotron-ms) added a commit that referenced this pull request May 8, 2026
@kenotron-ms @microsoft-amplifier
fix(core): @field_serializer(when_used='always') on Usage/SessionStat…
1652bab 
…us.cost_usd — revives PR #73

Revives the approach from amplifier-core PR #73 (closed without merge 2026-05-07T18:15:32Z)
with one critical refinement: use when_used='always' instead of default mode='json'-only.

PROBLEM:
M2 cost-tracking introduced cost_usd: Decimal | None on Usage and SessionStatus. Without
serializer help, plain model_dump() (called by every orchestrator: loop-streaming line 472,505,
loop-basic 361, loop-events 307,337) emits Decimal values into events, which crash json.dumps()
at every downstream JSONL write site (hooks-logging, hooks-backup, hook-shell, context-persistent,
foundation/serialization.py). A "patch every consumer" approach doesn't scale to N orchestrators
or future hook modules.

FIX:
Move conversion to the model itself using @field_serializer("cost_usd", when_used="always"):
- model_dump() and model_dump(mode='json') both return JSON-safe strings for cost_usd
- Direct attribute access still returns Decimal for in-memory math
- None survives as None (preserves None ≠ Decimal("0") invariant for "unknown" vs "free")
- Plus _json_default(obj) in models.py + default= in ToolResult.get_serialized_output()
  for non-Pydantic paths where tool outputs may carry Decimal

WHY "when_used='always'" MATTERS:
PR #73 used default field_serializer behavior, which fires only on mode='json'. But every
orchestrator calls plain model_dump(). Verification today (Pydantic 2.13.4) confirmed that
without when_used='always', Decimal values still leak into events on the plain-dump path.

VERIFIED BEHAVIORS (10 tests):
✓ model_dump() returns string for cost_usd
✓ json.dumps(usage.model_dump()) doesn't crash
✓ Direct usage.cost_usd access still returns Decimal
✓ Existing field_validator rejecting float still fires
✓ extra="allow" still works for provider-specific fields
✓ None survives as None (not "None")
✓ Round-trip model_validate(model_dump()) rebuilds with Decimal
✓ Precision preserved across 0.0000123, 12345.6789, 0, 1E-9
✓ Plain model_dump() and mode='json' produce identical output
✓ int coerces to Decimal then serializes to string

NEW TESTS:
- tests/test_cost_usd_serialization.py: 25 tests (all pass)
- 129 adjacent tests still pass (test_message_models, test_session, test_tool_result_autopop,
  test_interfaces, test_generated_equivalence)

VERSION BUMP:
1.5.0 → 1.5.1 (PATCH, per release-mandate)
- pyproject.toml
- crates/amplifier-core/Cargo.toml
- bindings/python/Cargo.toml
- python/amplifier_core/__init__.py

E2E SMOKE TEST: PASSED (run by user before push, per release-mandate Pre-Merge Gate)

FILES CHANGED:
- python/amplifier_core/message_models.py: +field_serializer on Usage.cost_usd
- python/amplifier_core/models.py: +field_serializer on SessionStatus.cost_usd + _json_default + default= in ToolResult
- tests/test_cost_usd_serialization.py: new, 234 lines
- Version files: all bumped to 1.5.1
- Cargo.lock: regenerated by Rust toolchain

UNBLOCKS M2/M3 milestone (microsoft-amplifier/amplifier-support#225):
Once landed, these 11 PRs can drop per-consumer Decimal sanitizer code:
- foundation #190: drop Decimal→str branch from serialization.py
- hooks-logging #5: close as superseded (model_dump(mode='json') becomes redundant)
- provider PRs: anthropic #52, openai #32, azure-openai #24, gemini #28, vllm #22, ollama #14, chat-completions #5
- consumer PRs: app-cli #171, hooks-streaming-ui #11 (unchanged)

Plus 3 modules no longer need follow-up PRs:
- amplifier-module-hooks-backup (automatic safety)
- amplifier-module-hook-shell (automatic safety)
- amplifier-module-context-persistent (automatic safety)

Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@kenotron-ms Ken Chau (kenotron-ms) mentioned this pull request May 8, 2026
Merged
Salil Das (sadlilas) pushed a commit that referenced this pull request May 8, 2026
@kenotron-ms @microsoft-amplifier
fix: Decimal JSON serialization for Usage.cost_usd — required for M2 …
91fa469 
…cost stamping (#73)

* fix(core): @field_serializer(when_used='always') on Usage/SessionStatus.cost_usd — revives PR #73

Revives the approach from amplifier-core PR #73 (closed without merge 2026-05-07T18:15:32Z)
with one critical refinement: use when_used='always' instead of default mode='json'-only.

PROBLEM:
M2 cost-tracking introduced cost_usd: Decimal | None on Usage and SessionStatus. Without
serializer help, plain model_dump() (called by every orchestrator: loop-streaming line 472,505,
loop-basic 361, loop-events 307,337) emits Decimal values into events, which crash json.dumps()
at every downstream JSONL write site (hooks-logging, hooks-backup, hook-shell, context-persistent,
foundation/serialization.py). A "patch every consumer" approach doesn't scale to N orchestrators
or future hook modules.

FIX:
Move conversion to the model itself using @field_serializer("cost_usd", when_used="always"):
- model_dump() and model_dump(mode='json') both return JSON-safe strings for cost_usd
- Direct attribute access still returns Decimal for in-memory math
- None survives as None (preserves None ≠ Decimal("0") invariant for "unknown" vs "free")
- Plus _json_default(obj) in models.py + default= in ToolResult.get_serialized_output()
  for non-Pydantic paths where tool outputs may carry Decimal

WHY "when_used='always'" MATTERS:
PR #73 used default field_serializer behavior, which fires only on mode='json'. But every
orchestrator calls plain model_dump(). Verification today (Pydantic 2.13.4) confirmed that
without when_used='always', Decimal values still leak into events on the plain-dump path.

VERIFIED BEHAVIORS (10 tests):
✓ model_dump() returns string for cost_usd
✓ json.dumps(usage.model_dump()) doesn't crash
✓ Direct usage.cost_usd access still returns Decimal
✓ Existing field_validator rejecting float still fires
✓ extra="allow" still works for provider-specific fields
✓ None survives as None (not "None")
✓ Round-trip model_validate(model_dump()) rebuilds with Decimal
✓ Precision preserved across 0.0000123, 12345.6789, 0, 1E-9
✓ Plain model_dump() and mode='json' produce identical output
✓ int coerces to Decimal then serializes to string

NEW TESTS:
- tests/test_cost_usd_serialization.py: 25 tests (all pass)
- 129 adjacent tests still pass (test_message_models, test_session, test_tool_result_autopop,
  test_interfaces, test_generated_equivalence)

VERSION BUMP:
1.5.0 → 1.5.1 (PATCH, per release-mandate)
- pyproject.toml
- crates/amplifier-core/Cargo.toml
- bindings/python/Cargo.toml
- python/amplifier_core/__init__.py

E2E SMOKE TEST: PASSED (run by user before push, per release-mandate Pre-Merge Gate)

FILES CHANGED:
- python/amplifier_core/message_models.py: +field_serializer on Usage.cost_usd
- python/amplifier_core/models.py: +field_serializer on SessionStatus.cost_usd + _json_default + default= in ToolResult
- tests/test_cost_usd_serialization.py: new, 234 lines
- Version files: all bumped to 1.5.1
- Cargo.lock: regenerated by Rust toolchain

UNBLOCKS M2/M3 milestone (microsoft-amplifier/amplifier-support#225):
Once landed, these 11 PRs can drop per-consumer Decimal sanitizer code:
- foundation #190: drop Decimal→str branch from serialization.py
- hooks-logging #5: close as superseded (model_dump(mode='json') becomes redundant)
- provider PRs: anthropic #52, openai #32, azure-openai #24, gemini #28, vllm #22, ollama #14, chat-completions #5
- consumer PRs: app-cli #171, hooks-streaming-ui #11 (unchanged)

Plus 3 modules no longer need follow-up PRs:
- amplifier-module-hooks-backup (automatic safety)
- amplifier-module-hook-shell (automatic safety)
- amplifier-module-context-persistent (automatic safety)

Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

* test: update test_model_dump_includes_cost_usd_as_decimal for new contract

The test was written for the OLD contract (model_dump() returns Decimal).
With @field_serializer(when_used='always') on Usage.cost_usd, plain
model_dump() now returns string — same as mode='json'. Renamed and
updated assertion to match. Added a "direct attribute access still
returns Decimal" assertion to make the bidirectional contract explicit.

Self-correction: I missed sweeping bindings/python/tests/ in the
original PR's verification pass; only ran tests/. The CI catch is
exactly what the release-mandate Pre-Merge Gate is for.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

* test: drop bug-history references; lead docstrings with the contract

Test docstrings shouldn't read like a changelog. A reader landing on
these tests cares about what cost_usd actually is, not which milestone
introduced it or which PR-number-of-the-week it was last fixed in.

- Module docstring opens with "cost_usd is Decimal in memory ... JSON-safe
  string when serialized" and the None ≠ Decimal("0") invariant
- Class docstrings: "Decimal in memory, string on the wire"
- Per-test docstrings describe what the test verifies, not why history
  forced it (no @field_serializer, no _json_default, no PR #73, no M2)
- Also: regenerate uv.lock for the 1.5.0 → 1.5.1 version bump that was
  missed when bumping the other version files manually

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

---------

Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@bkrabach