Conversation
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
Overall: clean, well-scoped configurability —
Test gap worth one small addition: an upper-bound case with |
|
Fixed. Bounded in the schema, and clamped in Looking for the same shape turned up two more, so
Your 2 goes away with the clamp in One I haven't fixed: |
RETRY_MAX_RETRIES and the backoff constants in session/retry.ts are compiled in, so a turn is abandoned after 5 attempts (~68s) with no way to change it. That default is right for the infinite-loop reports it was added for, and wrong for providers whose transient errors outlive it. Add experimental.retry to the config schema and thread it into SessionRetry.policy. Every field falls back to the existing constant, so the schedule is byte-for-byte unchanged when the key is absent. maxRetries of -1 retries for as long as the error stays retryable. Expose backoffFactor alongside maxRetries: with the factor left at 2, a raised attempt count is mostly dead time, since an error carrying headers but no retry-after is not clamped by RETRY_MAX_DELAY_NO_HEADERS at all. Refs anomalyco#43596 Assisted-by: Claude Opus 5
Making the constants configurable opened three ways to produce a delay the
scheduler cannot honour. Duration.millis turns every one of them into an
immediate retry, which is the failure mode the attempt cap was added for.
1. maxDelayMs above 2^31-1. That value was the constant precisely because
setTimeout overflows past it:
delay() -> 4294967296
TimeoutOverflowWarning: does not fit into a 32-bit signed integer
slept 2ms, expected ~49 days
Bounded in the schema, and normalized in resolve() as well, since the
plugin config hook mutates the loaded config without revalidation.
2. NaN. A large backoffFactor or jitterFactor overflows the exponential to
Infinity, and Infinity * 0 jitter is NaN. Reachable at attempt 1 with a
jitterFactor the schema accepts, and at attempt ~320 with backoffFactor
10 once maxRetries is -1.
3. Negative. A malformed retry-after already produced a negative delay; the
HTTP-date branch guards for it, the two numeric branches did not. Bounded
attempts kept this cheap, -1 does not.
cap() is the single choke point every return path in delay() goes through,
so the finite and non-negative guards live there. Default schedules are
unchanged, pinned by the existing and new tests.
Assisted-by: Claude Opus 5
b870c14 to
4a34561
Compare
anomalyco#43596 now also asks for the opposite of what motivated this: turns with side effects need the processor to never replay a failed provider request. Zero already did that, since the policy returns done on attempt 1, but nothing pinned it. Assisted-by: Claude Opus 5.5
4a34561 to
c575723
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Regenerate public SDK/OpenAPI artifacts and normalize invalid maxDelayMs values.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Adds configurable retry policy settings under experimental.retry, preserving default behavior and adding test coverage.
Changes:
- Configures retry limits, delays, backoff, jitter, and caps.
- Passes retry configuration into session processing.
- Adds schema definitions and retry behavior tests.
| File | Description |
|---|---|
packages/opencode/test/session/retry.test.ts |
Tests default and customized retry behavior. |
packages/opencode/src/session/retry.ts |
Implements configurable retry limits and delays. |
packages/opencode/src/session/processor.ts |
Passes retry configuration into session execution. |
packages/core/src/v1/config/config.ts |
Defines the experimental.retry schema. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| function resolve(tuning?: Tuning) { | ||
| return { | ||
| maxRetries: tuning?.maxRetries ?? RETRY_MAX_RETRIES, | ||
| initialDelayMs: tuning?.initialDelayMs ?? RETRY_INITIAL_DELAY, | ||
| backoffFactor: tuning?.backoffFactor ?? RETRY_BACKOFF_FACTOR, | ||
| jitterFactor: tuning?.jitterFactor ?? RETRY_JITTER_FACTOR, | ||
| // Normalized here so policy() and delay() read the same ceiling. Config | ||
| // validation also bounds this, but a plugin config hook mutates the loaded | ||
| // config without revalidation, so it cannot be the only guard. | ||
| maxDelayMs: Math.min(tuning?.maxDelayMs ?? RETRY_MAX_DELAY, RETRY_MAX_DELAY), | ||
| maxDelayNoHeadersMs: tuning?.maxDelayNoHeadersMs ?? RETRY_MAX_DELAY_NO_HEADERS, | ||
| } | ||
| } |
There was a problem hiding this comment.
Right, and it's the argument I'd made in the comment just above it: the plugin hook skips revalidation, so an upper bound alone wasn't enough. Fixed in resolve() rather than for maxDelayMs alone, since every field has the same exposure. Each one now falls back to its default unless it passes the same rule as the schema. That also catches maxRetries: NaN, which the old check read as unlimited. Tests for both.
On regenerating the SDK and openapi.json: generate.yml does that on every push to dev, so I've left the generated files alone.
…'s ceiling The plugin config hook mutates the loaded config without revalidation, so schema bounds cannot be the only guard. resolve() only enforced an upper bound on maxDelayMs: a hook setting it to -1 or NaN made cap() return a negative or NaN delay, which Duration.millis turns into an immediate retry (Copilot review). The same exposure applied to every other field, and one was worse: a NaN maxRetries failed the `>= 0` test and read as unlimited. Each field now falls back to its default unless it passes the same rule as the schema. Generated SDK and openapi.json are left to generate.yml, which regenerates them on every push to dev. Assisted-by: Claude Opus 5.5
|
I just built a similar change to support max retry duration (effectively same as |
Issue for this PR
Closes #43596
Type of change
What does this PR do?
The retry constants in
session/retry.tsare compiled in, so a turn is dropped after 5 attempts and there is no way to change that. This adds an optionalexperimental.retryblock and passes it intoSessionRetry.policy. Each field falls back to the constant it replaces, so nothing moves unless you set one.I exposed
backoffFactoras well asmaxRetries, because the attempt count alone barely helps. With no response headers the delay is already clamped at 30s from attempt 5 on, so more attempts is mostly more waiting. With headers but noretry-afterit isn't clamped at all (#33728), so attempt 10 waits ~17 min.retry-afterhandling is untouched.maxRetries: -1retries while the error stays retryable, which is the long quota window case in the issue. That does hand back the unbounded retry the cap was added to stop (#41848). It's opt-in and off by default, but say the word and I'll drop it and keep only the finite knobs.maxRetries: 0is the other end: a failed request is never replayed. The issue has since asked for exactly that, for turns with side effects.Tuned values can't produce a delay the scheduler mishandles:
cap()keeps every delay finite, non-negative and under thesetTimeoutlimit, sinceDuration.millisturns NaN and negatives into an immediate retry. Details in the review thread below.How did you verify your code works?
Rebased on
devon 2026-09-23.bun test test/session/(424 pass),bun test test/config(230 pass),bun turbo typecheckon core and opencode.Eleven new tests in
retry.test.ts. The one that matters: absent and empty tuning both still produce[2000, 4000, 8000, 16000, 30000, 30000], so the default schedule is unchanged.I also ran
script/schema.tsto look at the generated schema. That's why the two float fields useSchema.Finiteand notSchema.Number, which was emitting"NaN"and"Infinity"string enums into the public schema.Screenshots / recordings
Not a UI change.
Checklist