fix(model): honour backoffMultiplier in retry strategy - #3089
fix(model): honour backoffMultiplier in retry strategy#3089wangzhen8866 wants to merge 2 commits into
Conversation
|
|
ExecutionConfig.backoffMultiplier was a dead field: the inline retry chains in ModelUtils, ToolExecutor and EmbeddingUtils used Retry.backoff(...) whose built-in multiplier defaults to 2, silently ignoring any user-supplied value. Introduce RetrySpecs.build(ExecutionConfig) as the single place that turns an ExecutionConfig into a RetryBackoffSpec, delegating to Reactor RetryBackoffSpec.multiplier(double) so the configured multiplier finally takes effect (defaulting to 2.0 for backward compatibility). Refactor the three drifted inline copies to use it and align the ModelTimeoutRetryTest helper with production. Add RetrySpecsTest with deterministic field-level assertions that lock the multiplier propagation and default fallbacks.
aab0a91 to
4d97435
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Confirmed the root cause: backoffMultiplier was a dead knob — all three inline chains used Retry.backoff(...) without .multiplier(...), so Reactor's hard-coded 2.0 always won. Centralising into RetrySpecs.build(ExecutionConfig) is the right shape (it also collapses three copies of the null-default logic that had already drifted), and RetrySpecsTest asserts the spec fields directly instead of sleeping on wall-clock backoff, which is exactly how this should be pinned down. Both findings below are non-blocking.
Findings
- [Info]
RetrySpecs.java:104—getMaxAttempts() - 1LNPEs on a nullmaxAttempts; the precondition is documented but unenforced on a now-public helper - [Info]
ModelUtils.java:116— reading Reactor'smultiplier/minBackofffields for logging ties three call sites to an undocumented surface
Compatibility note (please call out in the release note)
Behaviour for configurations that set backoffMultiplier != 2.0 changes on upgrade — previously ignored, now honoured (this is of course the point, but it is a user-visible timing change for anyone who had e.g. 1.0 to get a fixed delay). ExecutionConfig.Builder#backoffMultiplier already rejects < 1.0, so Reactor's own multiplier() validation can't be tripped by a config built through the normal path. MODEL_DEFAULTS uses 2.0, so default behaviour is bit-for-bit unchanged.
Checklist notes
- Scope:
StudioClient(agentscope-extensions-studio) still builds its ownRetry.backoff(...)— out of scope here since it is not driven byExecutionConfig, but worth remembering if that knob is ever added there. - Tests:
ModelTimeoutRetryTestnow mirrors production throughRetrySpecsinstead of duplicating the inline code — good, that removes the drift that let the bug survive in the first place. - CLA signed, CI green (ubuntu + windows build, license, module sync, codecov patch).
Automated review by github-manager-bot
| Double multiplier = config.getBackoffMultiplier(); | ||
| double effectiveMultiplier = multiplier != null ? multiplier : DEFAULT_MULTIPLIER; | ||
|
|
||
| return Retry.backoff(config.getMaxAttempts() - 1L, initialBackoff) |
There was a problem hiding this comment.
[Info] config.getMaxAttempts() - 1L unboxes Integer, so calling build() with a maxAttempts == null config throws a bare NPE instead of the documented precondition failing loudly. The Javadoc puts the guard on the caller, but this class is now public API in io.agentscope.core.model, so someone will call it directly. Consider Objects.requireNonNull(config.getMaxAttempts(), "maxAttempts must be set before building a retry spec") (or return Retry.max(0) for the disabled case) so the contract is enforced rather than assumed.
| LOG.debug( | ||
| "Applied retry config: maxAttempts={}, initialBackoff={} for model: {}", | ||
| "Applied retry config: maxAttempts={}, multiplier={}, initialBackoff={}" | ||
| + " for model: {}", | ||
| maxAttempts, | ||
| initialBackoff, | ||
| retrySpec.multiplier, |
There was a problem hiding this comment.
[Info] retrySpec.multiplier / retrySpec.minBackoff are Reactor's public fields on RetryBackoffSpec, not a documented accessor API — they are convenient today, but they are the kind of internal surface that gets renamed on a reactor-core minor bump (and this same pattern is repeated in ToolExecutor and EmbeddingUtils, so one upgrade breaks three call sites). Since the effective values are already computed in RetrySpecs.build, a cheap alternative is to return them from a small holder (or just log execConfig.getBackoffMultiplier() plus the resolved default) and keep the Reactor dependency behind the helper.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Consolidates the three duplicated inline Retry.backoff(...) chains into a single RetrySpecs.build(ExecutionConfig) helper and, in doing so, fixes a real bug: backoffMultiplier was exposed on ExecutionConfig but never applied. The direction is right and the new RetrySpecsTest covers the knob directly. My main blocker is that the logging changes read package-private fields of Reactor's RetryBackoffSpec (multiplier, minBackoff) from io.agentscope.core.*, which should not compile; please confirm the build status.
CLA: signed ✅
Findings:
- [Critical]
ModelUtils.java:113—retrySpec.multiplier/retrySpec.minBackoffare not public onRetryBackoffSpec; same pattern inToolExecutor.javaandEmbeddingUtils.java, and inRetrySpecsTest. - [Warning]
RetrySpecs.java:88—getMaxAttempts()is unboxed without a null/range guard, despite the javadoc pushing that responsibility onto callers. - [Info]
RetrySpecs.java:46— worth documenting as a user-visible behaviour change: anyone who had setbackoffMultipliernow gets different retry timings than before.
Automated review by github-manager-bot
| responseFlux = responseFlux.retryWhen(retrySpec); | ||
| LOG.debug( | ||
| "Applied retry config: maxAttempts={}, initialBackoff={} for model: {}", | ||
| "Applied retry config: maxAttempts={}, multiplier={}, initialBackoff={}" |
There was a problem hiding this comment.
Compile risk: retrySpec.multiplier and retrySpec.minBackoff are package-private fields of Reactor's RetryBackoffSpec (they live in reactor.util.retry), so reading them from io.agentscope.core.model should fail with "multiplier has private access in RetryBackoffSpec". The same pattern is repeated in ToolExecutor.java and EmbeddingUtils.java, and RetrySpecsTest also asserts on those fields. Could you confirm the exact reactor-core version you built against, and if it does not compile, log the values computed locally in RetrySpecs.build() instead (e.g. expose a small describe(config) helper or return a record holding spec + effective multiplier/backoff)? That keeps the debug log informative without depending on Reactor internals.
| public static RetryBackoffSpec build(ExecutionConfig config) { | ||
| Duration initialBackoff = config.getInitialBackoff(); | ||
| if (initialBackoff == null) { | ||
| initialBackoff = DEFAULT_INITIAL_BACKOFF; |
There was a problem hiding this comment.
config.getMaxAttempts() - 1L dereferences maxAttempts without a null check, and a caller passing maxAttempts <= 1 would build Retry.backoff(0, ...) / a negative bound. The javadoc documents the guard as the caller's responsibility, which is fine, but three production call sites now rely on it. Consider failing fast here (Objects.requireNonNull(config.getMaxAttempts(), ...), plus if (maxAttempts < 2) return Retry.max(0)-style no-op, or simply reject with IllegalArgumentException) so a future call site cannot produce an opaque NPE inside a reactive chain.
| * <li>{@code maxBackoff} unset → 10 seconds | ||
| * <li>{@code retryOn} unset → retry all errors | ||
| * <li>{@code backoffMultiplier} unset → {@code 2.0} (Reactor default) | ||
| * </ul> |
There was a problem hiding this comment.
Good deduplication — this is the right shape for the fix. One behavioural note worth calling out explicitly for reviewers: Retry.backoff(long, Duration) sets firstFixDelay, so with a configured maxAttempts the effective backoff sequence for existing users stays initialBackoff * multiplier^n; since multiplier was previously always the implicit 2.0, users who had set backoffMultiplier will now see different (larger or smaller) real delays. That is the intent of the PR, but flagging it as a behaviour change in the PR description / changelog would help, since it affects model call retry timing for everyone who configured the knob.
|
Correction after checking CI: The two remaining points still stand and are the only ones I would like addressed:
Overall: LGTM once the null-guard question is settled. Automated review follow-up by github-manager-bot |
3 participants
AgentScope-Java Version
2.0.3-SNAPSHOT (based on
main, post v2.0.1 GA)Description
Background
ExecutionConfig.backoffMultiplierwas a dead field: it was exposed as afirst-class, user-configurable knob (documented in the builder Javadoc and
MODEL_DEFAULTS), but none of the three inline retry chains actually consumed it.All three used
Retry.backoff(maxAttempts - 1, initialBackoff)..., whose built-inexponential factor is hard-coded to
2by Reactor. As a result, any user-suppliedbackoffMultiplier(1.5)/3.0was silently ignored — the retry cadence wasalways 2× regardless of configuration. Even the E2E test passing
2.0vs1.5could not detect the difference.
Root cause
Retry.backoff(...)returns aRetryBackoffSpecwhosemultiplierdefaults to2.0; the inline chains never called.multiplier(...), soExecutionConfig.getBackoffMultiplier()had no consumer.Changes
RetrySpecs.build(ExecutionConfig)— single place that turns anExecutionConfiginto aRetryBackoffSpec, delegating to Reactor's nativeRetryBackoffSpec.multiplier(double)so the configured multiplier finallytakes effect. Defaults to
2.0when unset (backward compatible). Alsocentralises the
initialBackoff/maxBackoff/retryOnnull-fallbacks thathad previously drifted out of sync across three call sites.
RetrySpecs.build(...):ModelUtils.applyTimeoutAndRetry(model API calls)ToolExecutor.applyRetry(tool calls)EmbeddingUtils.applyTimeoutAndRetry(embedding API calls)Each keeps its own
doBeforeRetrylogging; debug log now also prints theeffective
multiplierfor observability.ExecutionConfig.getBackoffMultiplier()and bothapplyTimeoutAndRetrymethod docs now state thatmultiplieris honoured viaRetrySpecs(null → default2.0).RetrySpecsTest(5 cases) — deterministic, field-level assertions onRetryBackoffSpec.multiplier / minBackoff / maxBackoff / maxAttempts / errorFilter / jitterFactor, locking the multiplier propagation and defaultfallbacks. No real-time sleeps, no jitter randomness.
ModelTimeoutRetryTestprivate helper migrated toRetrySpecs.buildso thetest mirror stays in lock-step with production.
Why low-risk / easy to review
backoffMultiplieris unset,the default
2.0matches Reactor's previous implicit behaviour exactly.RetryBackoffSpec.multiplier(...)ratherthan a hand-rolled
Retry.fromcompanion_publisher — zero custom retry logic.How to test
mvn -pl agentscope-core test -Dtest=RetrySpecsTest→ 5/5mvn -pl agentscope-core test -Dtest=ModelTimeoutRetryTest→ 7/7mvn -pl agentscope-extensions/agentscope-extensions-rag/agentscope-extensions-rag-simple test -Dtest=EmbeddingUtilsTest→ 9/9mvn -pl agentscope-core spotless:check→ clean (499 files)Checklist
mvn spotless:applymvn testfor affected modules)