Skip to content

fix(llm): cap combined retry budget across transient and schema failures#762

Open
bh462007 wants to merge 1 commit into
Coder-s-OG-s:mainfrom
bh462007:fix/llm-router-retry-budget
Open

fix(llm): cap combined retry budget across transient and schema failures#762
bh462007 wants to merge 1 commit into
Coder-s-OG-s:mainfrom
bh462007:fix/llm-router-retry-budget

Conversation

@bh462007

Copy link
Copy Markdown
Contributor

Fixes #735

What was wrong

llmCall() maintained two separate retry counters for each provider:

  • retryCount for transient failures (max MAX_RETRIES = 2)
  • schemaRetryCount for schema/JSON failures (max 1)

Although the loop checked both limits, each error path only considered its own counter before retrying. If a provider alternated between transient and schema failures, it could make 4 total attempts before falling back, exceeding the intended overall retry budget.

What changed

Added a totalAttempts counter with a limit of MAX_TOTAL_ATTEMPTS = MAX_RETRIES + 1 (3). Each retry now checks both its individual limit and the overall attempt limit.

This only affects mixed failure sequences. Existing behavior remains unchanged for:

  • Transient-only failures (still up to 3 attempts)
  • Schema-only failures (still up to 2 attempts)

I intentionally kept the existing retry counters instead of replacing them with a single shared counter, since that would have changed the schema-only retry behavior and broken an existing test.

Testing

  • Added a test covering the reported sequence: transient → schema → transient → schema. It confirms retries now stop after 3 total attempts instead of 4.
  • Verified that all 8 existing tests in router.test.ts continue to pass without changes, confirming no regressions for transient-only, schema-only, or timeout scenarios.
  • Ran npx tsc --noEmit with no new TypeScript errors.

Note: I couldn't run the full test suite through the normal npm test path because tests/setup.ts is configured as a global setupFiles entry in vitest.config.ts and imports @testing-library/jest-dom/vitest, but the required Testing Library packages aren't listed in package.json. This prevents every test in the repository from running. To verify this change, I temporarily commented out that single config entry, ran the tests, then restored it before committing (git diff was clean afterward). I'm happy to open a separate issue for the test setup if that's helpful.

Scope

  • src/lib/llm/router.ts
  • src/lib/llm/router.test.ts

Copilot AI review requested due to automatic review settings July 19, 2026 16:03
@vercel

vercel Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

@bh462007 is attempting to deploy a commit to the codersogs-3057's projects Team on Vercel.

A member of the Team first needs to authorize it.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@pavsoss
pavsoss self-requested a review July 19, 2026 21:42

@pavsoss pavsoss left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @bh462007 the fix looks solid, but this branch contains unmerged commit from #761 , please rebase the branch for this PR on top of the main branch to isolate the LLM router change
Also it's recommended to first fetch changes from the upstream and then origin a new branch (for the new issues) on upstream/main

@pavsoss pavsoss added the Needs author reply Author need to reply label Jul 19, 2026
@jakharmonika364

Copy link
Copy Markdown
Collaborator

this branch have conflict with the main, please fix it

@bh462007
bh462007 force-pushed the fix/llm-router-retry-budget branch from ee33b4a to e0819e1 Compare July 24, 2026 10:03
@bh462007

Copy link
Copy Markdown
Contributor Author

Rebased my branch onto upstream/main using git rebase --onto upstream/main 7c4790a ee33b4a to remove the duplicate journey-progress commit, since it had already been merged upstream in #761 (5f36773). This left only the router fix in the branch.

I also added the upstream remote so I can base future branches directly on the latest upstream/main instead of my fork. Thanks for pointing that out!

@pavsoss pavsoss left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: LLM provider retry budget can be exceeded when transient and schema failures interleave

4 participants