fix(models): retry transient backend failures with backoff; actionable @embed errors (#1594) - #1770
Draft
heskew wants to merge 3 commits into
Draft
fix(models): retry transient backend failures with backoff; actionable @embed errors (#1594)#1770heskew wants to merge 3 commits into
heskew wants to merge 3 commits into
Conversation
…@embed errors (#1594) Retry half: shared fetchWithRetry in backendHelpers — bounded same-backend retries for HTTP 408/429/5xx (incl. 529) and transient network rejections, honoring Retry-After (capped at 30s; longer server demands surface the response instead), jittered exponential backoff (500ms base, doubling, 10s cap), never on abort. The composed caller-signal/requestTimeoutMs budget spans all attempts, so requestTimeoutMs keeps its meaning as the overall per-call deadline and backoff sleeps abort promptly. Wired into the ollama/openai/anthropic #post paths; bedrock plumbs the same maxRetries knob to the AWS SDK's maxAttempts instead (the SDK owns its retry loop). New per-entry config: maxRetries (default 2, 0 disables), retryBackoffMs (fetch backends only). Same-backend retries run before Models.ts multi-candidate failover, answering the layering question consolidated from #1595. Error half: @embed's rethrown error now includes the configured model name, and — only for ServerError subclasses carrying upstreamStatus, i.e. our own backend classes' deliberately sanitized, length-capped HTTP-error messages — the backend message verbatim. A bare upstreamStatus on a non-ServerError is not proof of sanitization and stays behind safe identifiers (class name, status), preserving the #1593 posture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BdbdepRFuyUWoEmNN1pSVQ
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements bounded retry logic with jittered exponential backoff and Retry-After header support across several model backends (Anthropic, Bedrock, Ollama, and OpenAI), and refines embedding error surfacing to prevent sensitive data leaks. The feedback suggests improving robustness by explicitly checking for trimmed empty strings before numeric coercion of the Retry-After header, and wrapping property accesses on untrusted error objects in try/catch blocks to prevent potential crashes from optional chaining.
Contributor
|
Reviewed; no blockers found. |
Number(' ') === 0, so a whitespace header would sleep 0ms instead of
falling back to the computed backoff. Gemini review catch.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BdbdepRFuyUWoEmNN1pSVQ
…iew) From the HEG step-10 domain review: an unclamped maxRetries with no requestTimeoutMs/caller signal could wedge the embed/write path for minutes (and on the cache-from-source path that time is spent under a per-record store lock) — clamp configured values at MAX_CONFIG_RETRIES. Also fixes requestTimeoutMs doc drift (it is the overall budget spanning all attempts, not per-request) and documents that a retried generate is not exactly-once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BdbdepRFuyUWoEmNN1pSVQ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1594 (consolidates #1595, closed as duplicate).
Two resilience/DX gaps in the
@embed/ model-backend path, both found while dogfooding bulk embedding: a single transient provider error failed the whole write (no retry anywhere in the stack), and the surfaced error gave no hint of provider, model, or cause.Retry with backoff (all model callers benefit:
@embed,scope.models, the/v1gateway)New shared
fetchWithRetryinresources/models/backendHelpers.ts, wired into the#postpath of the ollama, openai, and anthropic backends:408/429/5xx(includes Anthropic's529overloaded) and transient network rejections. Deterministic client errors (400/401/404/…) are never retried.Retry-After(delta-seconds or HTTP-date) is honored up to a 30s cap — a server demanding a longer wait gets its response surfaced immediately rather than retried early (impolite) or held open (stalls the write).retryBackoffMsbase (default 500ms), doubling per attempt, 10s per-sleep cap, jitter factor in [0.5, 1).requestTimeoutMskeeps its documented meaning as the overall per-call deadline — retries fit inside the budget rather than extending it. Aborts (caller or timeout) cancel a pending backoff sleep immediately and are never retried.maxRetries(default 2,0disables) andretryBackoffMs. Malformed values fall back to defaults.bedrock goes through the AWS SDK, which owns its retry loop — the same
maxRetriesknob is plumbed to the SDK'smaxAttempts(maxRetries + 1) instead of double-wrapping. Default matches the SDK's own (3 attempts). NoretryBackoffMsthere; the SDK's retry strategy manages delays.Design decisions (answering the questions consolidated from #1595)
Modelsdispatch — so every caller benefits and per-backend semantics (SDK-managed vs fetch) stay local.Models.tsmulti-candidate failover proceeds to the next candidate exactly as today.@embedbulk writes: out of scope; separate design if pursued.Actionable
@embederror surface#1593 already added the error class name + upstream HTTP status to the sanitized rethrow. This completes it:
The configured model name is now always included.
The backend's message is surfaced verbatim — but only when the error is a
ServerErrorsubclass carryingupstreamStatus, which is exactly our backend classes' HTTP-error path whose messages are deliberately sanitized and length-capped (no request content, capped upstream text). Example:Failed to compute embedding for attribute "embedding" (model "default") [OpenAIBackendError] (backend HTTP 404): OpenAI /embeddings returned HTTP 404: models/default is not foundA bare
upstreamStatuson a non-ServerErroris not proof of sanitization (custom embedder errors can carry anything): those keep the @embed / models.embed: logical model name is forwarded as the wire model id (404 when they differ), and the error is unhelpful #1593 posture — safe identifiers only,— see server log for details. There's a regression test pinning this.The
ServerErrorcheck is lazily required in the failure path only, mirroring the existinggetLoggerpattern (hdbError imports the logger; fail-closed to the sanitized message if the require fails).Tests
backendHelpers.test.js: 30+ new cases —resolveRetryConfigvalidation, retriable-status matrix,Retry-Afterparsing (delta/date/garbage/past), jitter bounds + cap,abortableSleepabort semantics, andfetchWithRetryscenarios (retry-then-success,Retry-Afterhonored/over-cap, non-retriable passthrough, exhaustion,maxRetries: 0, network-error retry/exhaustion, abort-never-retried, abort-during-backoff).openai/index.test.js: retry through the real#post(429 → success),maxRetries: 0surfacesupstreamStatus, non-retriable 400 single-attempt.bedrock/index.test.js:maxAttemptsplumbing (default → 3,maxRetries: 0→ 1).embedHook.test.js: verbatim surface for real backend-shaped errors (message + model + status + class), spoofedupstreamStatuson a plainErrorstays sanitized.All 249 tests across the six affected suites pass; oxlint and prettier clean; build shows only the known pre-existing TS errors in unrelated files.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BdbdepRFuyUWoEmNN1pSVQ
Review notes (HEG step 10 — cross-model review, 2026-07-17)
Harper-domain pass (deep-review + adjudication): no blockers. Direction-fit verified: aligns with the bounded-backoff precedents (
DatabaseTransactioncommit retry, #1371 index-backfill retry), preserves thecomposeSignal/requestTimeoutMscontract, extends the #1593 sanitized-error posture deliberately (fail-closed gate; provider text capped at 500 chars), and is orthogonal to the uWS/h2c/h3 server-side networking work (outbound fetch only).Open items for the human reviewer (found by the review, not resolved in-PR):
Table.tsholds the per-record store lock whileembedBefore()runs; that path'ssourceContextneed not carry a signal, and with norequestTimeoutMsconfigured the retry budget can stretch the hold (worst case ~60s+ under a rate-limited provider honoringRetry-After). Pre-existing exposure — the network call already ran under that lock unbounded — but retries amplify the worst case. In-PR mitigations:maxRetriesis now clamped at 10, and settingrequestTimeoutMsper model entry bounds it fully. A structural fix (bounding total embed time on the lock-held refresh path inTable.ts) is out of this PR's scope — flag if you want it here or as a follow-up.maxRetries × backofflatency before a clear misconfiguration surfaces. Deliberate: distinguishing transient from permanent rejection shapes across undici versions is fragile, and the cost is bounded (~1.5s at defaults).Docs decision (HEG step 16):
maxRetries/retryBackoffMsare user-facing model-entry config, but models config has no docs pages yet anywhere — documentation lands wholesale with the #510 docs arc, so no companion docs PR here.Caveats: both outside-model review legs failed this run (codex CLI init error; agy degraded then timed out) — this was a domain-pass-only review. Re-run the outside legs if you want second-model corroboration before merge.