Repository navigation
fix(models): retry transient request failures in VercelModel and GoogleModel - #5663
Conversation
|
@strandly-the-agent review pls |
|
Issue: This PR adds a new public config option — Suggestion: Per
|
|
Assessment: Approve Well-scoped, well-reasoned fix for the flaky Review notes
Nicely done — the error-classification reasoning and the CI/e2e evidence in the description made this easy to review. |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Verdict: approve — sound fix, real tests. One non-blocking nit on where maxRetries is validated.
I re-derived the two dependency claims the fix rests on, and both hold:
- ai-sdk only converts a fetch
TypeErrorinto a retryableAPICallErrorwhenerror.cause != null(@ai-sdk/provider-utilshandleFetchError). Chromium'sFailed to fetchhas nocause, so it reached you raw —isFetchNetworkErroris load-bearing in the browser, not belt-and-braces. @google/genai'sgenerateContentStreamonly awaits the request on the AFC-disabled branch; Strands lands there because it passes plainfunctionDeclarations(shouldDisableAfcreturns true with noCallableTool). So the rejection really is pre-response andwithRequestRetrycan see it. Worth a one-line comment, since the AFC branch returns the generator eagerly and would make the retry a silent no-op.
Nit (inline): a bad maxRetries is caught only on the first stream(), and in VercelModel it comes back as ModelError: Language model stream error: maxRetries must be an integer >= 0 (got -1).
✅ Verified, questions, appendix
Verified at 00d5136 (base aba77371a)
| what | result |
|---|---|
npx vitest run --project unit-node src/models/__tests__/{model,vercel,google}.test.ts src/retry |
318 passed, no type errors |
npx vitest run --project unit-browser src/models/__tests__/{model,vercel,google}.test.ts |
285 passed (chromium) |
full --project unit-node |
5759 passed / 2 failed / 6 skipped — both failures are src/telemetry/__tests__/config.test.ts asserting service.name === 'strands-agents' while my sandbox sets OTEL_RESOURCE_ATTRIBUTES. Sandbox artifact, not yours. |
eslint on the 9 changed files |
clean |
Behaviour I checked by reading, not by trusting the description:
- No
maxRetriescollision.LanguageModelCallSettingsisOmit<LanguageModelV3CallOptions, 'prompt'\|'tools'\|'toolChoice'>, andLanguageModelV3CallOptionshas nomaxRetries(@ai-sdk/provider), so the new field is genuinely new and nothing was previously forwarded under that name. The two "does not forward maxRetries" tests pin it. - Shared
REQUEST_RETRY_BACKOFFis safe to share across concurrent calls.ExponentialBackoffholds only readonly config andnextDelayis pure (backoff-strategy.ts), so there is no cross-request state. - 429 really does reach the agent strategy.
classifyErrormapsstatusCode === 429→ModelThrottledError, andDefaultModelRetryStrategy.isRetryableretries onlyModelThrottledError— so nothing is double-retried, and a 5xx that exhausts the in-provider budget is not retried again at the agent level. - Cancellation contract survives. Aborting during a backoff makes
withRequestRetrythrow the last transient error, not a cancel error — butagent.ts:2563-2567calls_throwIfCancelled()in the catch before rethrowing, so the caller still getsCancelledError. Fine as-is. - Tests prove behaviour. The Node and browser retry tests reject a mock once and compare the collected events to a clean run (
toEqual(expected)), which is what actually rules out duplicated stream events;maxRetries: 0, exhausted budget, 429, and abort-during-backoff are each pinned by call counts rather than by mock echoes. - No new public surface leaked.
withRequestRetry/isFetchNetworkErrorare@internaland absent fromsrc/index.ts;sleepmoved intobackoff-strategy.tsbutsrc/retry/index.tsuses explicit named exports, so it did not become public by accident. - Repo conventions. No site/ docs in this PR is consistent with
AGENTS.md("a branch that touches several sub-projects is almost always several PRs") and you called the follow-up out in the description.
Questions (both non-blocking)
- Retries are invisible above the provider.
AfterModelCallEvent.attemptCountdoesn't count them and there's no span or metric, so a turn that silently absorbed 1–6s looks like one slow model call. Debug log only, intentional for now? - Python Gemini parity.
strands-py/src/strands/models/gemini.pyhas no retry at all and I found no tracking issue for it. Want me to file the parity issue so the follow-up you named doesn't get lost?
Appendix — non-blocking (3)
api/needs-reviewstill isn't applied (already raised above, so just one data point):.github/workflows/api-review-label.ymlonly fails whenapi/needs-reviewis present withoutapi/review-complete, so the green check on a PR with neither label is vacuous — it isn't evidence the gate was cleared. For what it's worth the shape has precedent:BedrockModelOptionsalready carries a top-levelrequestTimeoutthat shadows aclientConfigsetting.- The two
maxRetriesTSDocs read as one knob with two scopes (Vercel: network + 408/409/5xx; Google: network only). The Google one says "whose connection fails", which is accurate but easy to skim past — a clause atstrands-ts/src/models/google/types.ts:57saying an HTTP error response is never retried would close it, and the doc-site follow-up is a good place for the comparison. startedAt/elapsedMsinwithRequestRetryare dead weight today —ExponentialBackoff.nextDelayreads onlyattempt. Fine as theBackoffContextcontract; noting it so it doesn't read as an implemented time budget.
Evidence: vitest-and-lint.log, repro-invalid-maxretries.test.ts, repro-invalid-maxretries.log.
|
Follow-up to my review above: the API bar-raise and adversarial passes are done — nothing new, verdict stays approve. Two things worth having on the thread:
Adversarial repros (local HTTP server + real
|
| scenario | Vercel (Bedrock) | |
|---|---|---|
| dead port, default | 3 fetches, then ModelError after ~4s |
3 fetches, then TypeError: fetch failed after ~5s |
dead port, maxRetries: 0 |
1 fetch | 1 fetch |
| socket reset before headers | retried (2 requests) | retried (2 requests) |
| socket reset after headers / mid-body | not retried — TypeError: terminated isn't in the message set, and modelMessageStartEvent had already been emitted, so no duplicate events |
not retried |
| request body across attempts | byte-identical | byte-identical |
agent.cancel() / cancelSignal during backoff |
stopReason: cancelled, 1 fetch |
same |
| 503, 503, 200 | recovers in-provider; agent strategy sees 1 attempt | — |
| 503 ×3 | ModelError after 3 fetches; agent strategy does not retry it again |
— |
| 429 always | 1 fetch per attempt; agent strategy retries ×3 — no double retry | — |
genai AFC path (a CallableTool in config.tools) |
— | retry does not fire (generateContentStream returns the generator eagerly). Not reachable through builtInTools?: Tool[] without a cast, so just confirming the comment suggestion above. |
⚪ API nit (non-blocking): the two maxRetries TSDocs (strands-ts/src/models/vercel.ts:98-105, strands-ts/src/models/google/types.ts:56-64) don't state the cost of the default. The PR body does ("~6s longer"); the place users read doesn't. One sentence — e.g. "Retries back off ~1–2s, then ~2–4s" — would do it.
|
Re-review at Assessment: Comment (not Approve yet). A correction to my previous review: I said the old Moving retries into the agent strategy is the better design: hooks and Review notes
|
Human Overview
Description
integ-browserkeeps failing withModelError: Language model stream error: Failed to fetchin VercelModel (Bedrock) tests. It hit 3 of the last ~13 full TS integ runs, including the main release run 40931625672. The cause is a short network drop between the runner's Chromium and Bedrock. Bedrock, Anthropic and OpenAI survive these drops because their vendor SDKs retry connection failures. VercelModel callsdoStream()directly (the AI SDK only retries insidestreamText), and@google/genaionly retries when you opt in. So for these two providers a dropped connection surfaces as a plainModelError, which the agent's retry strategy ignores.VercelModel and GoogleModel now throw a new
ModelNetworkErrorfor fetch network failures, both when connecting and mid-stream.DefaultModelRetryStrategyretries it alongsideModelThrottledError. All retrying stays in the agent'sretryStrategy, where hooks andModelRouterfallback can see it, instead of a second loop inside the provider. HTTP error responses are classified as before; a Vercel 5xx is still aModelError.Public API Changes
New exported
ModelNetworkError extends ModelError, retried by the default strategy:Network failures from these two providers are now retried with the default budget (6 attempts, exponential backoff from 4s) before they surface. The error message is unchanged. To opt out, override
isRetryable, or passretryStrategy: nullto disable retries.Related Issues
None. Found while investigating the flaky
integ-browserfailures linked above.Documentation PR
Not included yet. Two TypeScript docs statements need updating in a follow-up site PR.
agents/retry-strategies.mdxsays the default retriesModelThrottledErroronly. The error-classification bullet inmodel-providers/vercel.mdxdoesn't list the new error.Type of Change
Bug fix
Testing
Ran the full TS unit suites in Node and the browser, plus lint, type-check, format and complexity. Ran the
integ-browserVercelModel (Bedrock) interventions suite locally: 52/52 pass. End to end against real Bedrock (script below), afetchthat throwsTypeError('Failed to fetch')on the model call after a denied tool reproduces the CI failure. The default strategy retries once and the agent finishes withendTurn. WithretryStrategy: nullthe run fails withModelNetworkError. GoogleModel's classification is covered by unit tests only; I didn't run it against the live Gemini API.hatch run prepare(n/a, TypeScript-only)Additional Details
Extended context (optional)
Earlier revision. The first commit on this branch added a provider-level retry loop with a
maxRetriesoption. Review pointed out that it duplicatedDefaultModelRetryStrategy, so this revision replaces it with classification only.Naming. It's
ModelNetworkError, notNetworkError, becauseDOMExceptionalready usesNetworkErroras a name (and Firefox's fetch failure message starts with it). It also follows the SDK'sModel…Errorconvention. It covers network failures only. Folding retryable 5xx responses in would make the name wrong, so those keep today's classification.Mid-stream failures. These are now retried too. One CI run dropped the connection mid-stream with Chromium's
TypeError: network error. The agent re-runs the whole model call, and events already streamed have already been emitted. That's the same as a mid-streamModelThrottledErrortoday.GoogleModel. The error
ifladder in the stream catch is now a lookup table keyed onGoogleErrorType, so adding an error type is a compile error until it gets a mapping. This also keepsstream()'s complexity from rising. Google's existing 503UNAVAILABLE→ModelThrottledErrormapping is unchanged.Why not genai's own
retryOptions. With it set, genai turns non-OK responses into generic errors beforethrowErrorIfNotOKruns. That would hide throttling and context-overflow detection fromclassifyGoogleError.CI evidence.
Failed to fetchshowed up in 0 of ~25 runs before 2026-10-29 01:21Z and in 3 of ~13 since then. Each failure hit a different deny test. In each case a sibling test running at the same time also failed its first attempt and passed on vitest's immediate retry, which points to a network blip. One failing branch's merge-base had passed many earlier runs, so no SDK commit caused it.Follow-ups. Python parity: Python's retry strategy also only retries
ModelThrottledException, and its Gemini provider has the same gap. The same run'sagent.test.ts > AnthropicModel > handles tool invocationbrowser timeout is unrelated.E2E script (repo root
.context/, run withnpx tsxfromstrands-ts/):Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.