Repository navigation
fix(auth): clear recovered provider error state on real success paths - #372
diegosouzapw merged 5 commits into
Conversation
Clear errorCode, lastErrorType, and lastErrorSource when an account recovers so provider state returns to a fully clean active status. Add a focused regression test for recovered-account cleanup.
Pass errorCode, lastErrorType, and lastErrorSource through the runtime credentials object so clearAccountError can clear stale provider error metadata after a real successful request. Also update the regression test to use getProviderCredentials, matching the production call path.
Refine the recovered-account regression test to match the real observed state: an account can remain active while still carrying stale refresh-failure metadata. This verifies that getProviderCredentials surfaces those fields and that clearAccountError clears them through the real runtime path.
Clear recovered provider error metadata after successful credentialed requests in non-chat API routes as well. Add route-level regression tests covering a Response-based success path and a result-object success path.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where provider accounts could retain outdated error metadata even after successfully recovering from previous failures. By enriching the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a bug where stale error metadata persisted for provider accounts even after successful requests. The fix is well-implemented by enhancing getProviderCredentials to return the full error state and then clearing this state in the relevant API routes upon success. The inclusion of comprehensive regression tests for both the account-level cleanup and route-level success paths is commendable and significantly increases confidence in the fix. My feedback focuses on two areas for potential improvement: enhancing type safety in the authentication service to move away from any types, and abstracting the duplicated success-handling logic from the API routes to improve maintainability.
| const response = await handleAudioSpeech({ body, credentials }); | ||
| if (response?.ok) { | ||
| await clearRecoveredProviderState(credentials); | ||
| } | ||
| return response; |
There was a problem hiding this comment.
This pattern of awaiting the handler, checking for success, and then clearing the provider state is repeated across multiple route files (e.g., audio/transcriptions, moderations, rerank). To reduce code duplication and improve maintainability, you could consider abstracting this logic into a higher-order function or a wrapper.
For example, you could create a helper in auth.ts:
// In sse/services/auth.ts
export async function handleRequestAndClearState(
handlerPromise: Promise<Response>,
credentials: any
): Promise<Response> {
const response = await handlerPromise;
if (response?.ok) {
await clearRecoveredProviderState(credentials);
}
return response;
}Then, the route handler could be simplified to:
// In route.ts
return handleRequestAndClearState(
handleAudioSpeech({ body, credentials }),
credentials
);A similar helper could be created for the routes that check result.success.
There was a problem hiding this comment.
Good maintainability point. I am intentionally keeping this PR scoped to the recovery bug fix itself rather than introducing a new wrapper abstraction across multiple route shapes. These handlers do not currently return a single common success shape (Response vs { success, data }), so abstracting this further would broaden the change beyond the targeted fix.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| @@ -126,6 +131,7 @@ export async function POST(request) { | |||
| const result = await handleEmbedding({ body, credentials, log }); | |||
|
|
|||
| if (result.success) { | |||
There was a problem hiding this comment.
WARNING: Inconsistent response handling pattern. This route checks result.success while other routes in this PR (speech, transcriptions, moderations, rerank) check response?.ok. Consider standardizing the approach for maintainability.
There was a problem hiding this comment.
I am leaving this as-is intentionally. The routes touched here already sit on top of different handler contracts: some return a structured { success, data } result, while others return a Response. This PR only adds recovered-state cleanup on the existing success signal for each route. Standardizing those handler return shapes would be a broader refactor than this bug-fix PR.
|
|
||
| return handleModeration({ body: { ...body, model }, credentials }); | ||
| const response = await handleModeration({ body: { ...body, model }, credentials }); | ||
| if (response?.ok) { |
There was a problem hiding this comment.
WARNING: Inconsistent response handling pattern. This route checks response?.ok while other routes in this PR (embeddings, images, music, videos) check result.success. Consider standardizing the approach for maintainability.
There was a problem hiding this comment.
Same reasoning here: this PR is using each route's existing success contract rather than trying to normalize handler return types across unrelated endpoints. I agree consistency would be nicer long-term, but I do not want to widen this fix into a cross-handler refactor.
| return_documents: body.return_documents, | ||
| credentials, | ||
| }); | ||
| if (response?.ok) { |
There was a problem hiding this comment.
WARNING: Inconsistent response handling pattern. This route checks response?.ok while other routes in this PR (embeddings, images, music, videos) check result.success. Consider standardizing the approach for maintainability.
There was a problem hiding this comment.
Same reasoning here as on the other route comments: I am intentionally keying recovery cleanup off each route's current success contract (response.ok vs result.success) and not changing upstream handler return conventions inside this PR. That would be a broader maintainability refactor rather than part of the recovery-state bug fix itself.
Code Review SummaryStatus: 4 Issues Found (Unchanged) | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Other Observations (not in diff)No new issues found in this PR review. The changes implement a new Files Reviewed (14 files)
|
Tighten the helper signatures added for recovered provider cleanup. This removes the new any-typed recovery parameters called out in review without broadening the PR into unrelated auth typing work.
bug fixes (PRs #373, #371, #372, #369 by @kfiramar): - fix(db): provider_connections.group column migration for existing DBs - fix(i18n): replace missing deleteConnection key with delete in tooltip - fix(auth): clear stale error metadata on genuine provider recovery - fix(startup): unify env loading across npm/electron startup paths code quality improvements (per kilo-code-bot review): - docs: document result.success vs response.ok patterns in auth.ts - refactor: normalize overridePath?.trim() in electron/main.js - docs: explain preferredEnv merge order intent
…ider-error-state Thanks @kfiramar! 🎉 Critical fix — stale error metadata on recovered provider accounts was preventing valid accounts from being selected properly after recovery. Improvement added on top: documented the two valid success-check patterns (`result.success` for open-sse handlers vs `response?.ok` for fetch-based handlers) to address the kilo-code-bot review warning — both patterns are correct by design, now explicitly documented. 5 commits total, 2 test files (+168 lines of coverage). Merged!
bug fixes (PRs cheaper-inference#373, cheaper-inference#371, cheaper-inference#372, cheaper-inference#369 by @kfiramar): - fix(db): provider_connections.group column migration for existing DBs - fix(i18n): replace missing deleteConnection key with delete in tooltip - fix(auth): clear stale error metadata on genuine provider recovery - fix(startup): unify env loading across npm/electron startup paths code quality improvements (per kilo-code-bot review): - docs: document result.success vs response.ok patterns in auth.ts - refactor: normalize overridePath?.trim() in electron/main.js - docs: explain preferredEnv merge order intent
…ider-error-state Thanks @kfiramar! 🎉 Critical fix — stale error metadata on recovered provider accounts was preventing valid accounts from being selected properly after recovery. Improvement added on top: documented the two valid success-check patterns (`result.success` for open-sse handlers vs `response?.ok` for fetch-based handlers) to address the kilo-code-bot review warning — both patterns are correct by design, now explicitly documented. 5 commits total, 2 test files (+168 lines of coverage). Merged!
bug fixes (PRs cheaper-inference#373, cheaper-inference#371, cheaper-inference#372, cheaper-inference#369 by @kfiramar): - fix(db): provider_connections.group column migration for existing DBs - fix(i18n): replace missing deleteConnection key with delete in tooltip - fix(auth): clear stale error metadata on genuine provider recovery - fix(startup): unify env loading across npm/electron startup paths code quality improvements (per kilo-code-bot review): - docs: document result.success vs response.ok patterns in auth.ts - refactor: normalize overridePath?.trim() in electron/main.js - docs: explain preferredEnv merge order intent
…ider-error-state Thanks @kfiramar! 🎉 Critical fix — stale error metadata on recovered provider accounts was preventing valid accounts from being selected properly after recovery. Improvement added on top: documented the two valid success-check patterns (`result.success` for open-sse handlers vs `response?.ok` for fetch-based handlers) to address the kilo-code-bot review warning — both patterns are correct by design, now explicitly documented. 5 commits total, 2 test files (+168 lines of coverage). Merged!
bug fixes (PRs cheaper-inference#373, cheaper-inference#371, cheaper-inference#372, cheaper-inference#369 by @kfiramar): - fix(db): provider_connections.group column migration for existing DBs - fix(i18n): replace missing deleteConnection key with delete in tooltip - fix(auth): clear stale error metadata on genuine provider recovery - fix(startup): unify env loading across npm/electron startup paths code quality improvements (per kilo-code-bot review): - docs: document result.success vs response.ok patterns in auth.ts - refactor: normalize overridePath?.trim() in electron/main.js - docs: explain preferredEnv merge order intent
Summary
errorCode,lastErrorType, andlastErrorSourcefromgetProviderCredentials()so recovery cleanup can act on the real stored stateProblem
Recovered provider accounts could remain marked with stale failure metadata even after a successful request.
In practice, that meant a connection could be back to
testStatus=activewhile still retaining fields such as:error_code=refresh_failedlast_error_type=token_refresh_failedlast_error_source=oauthThat left the stored provider state inconsistent with the provider's actual recovered condition.
Root Cause
There were two gaps.
First,
clearAccountError()was being asked to decide whether cleanup was needed based on the current credentials object, butgetProviderCredentials()was not returning all of the relevant persisted error metadata. In the real runtime path, the cleanup check therefore had incomplete information.Second, chat-style routes already had success callbacks in the handler stack, but multiple other provider-backed success routes did not clear recovered error state at all. A successful embeddings, moderation, image, audio, rerank, music, or video request could therefore leave stale failure markers behind indefinitely.
Why This Fix
This PR keeps the fix scoped to actual recovery semantics:
Notably, cache hits are intentionally unchanged. A semantic-cache hit does not prove the provider recovered, so it should not clear provider failure state.
Proof
I verified the bug and the fix at three levels.
Data-path proof
On the pre-fix code path,
getProviderCredentials()did not includeerrorCode,lastErrorType, orlastErrorSource, so the recovery cleanup path could not reliably clear the real stored metadata.Runtime proof against real stored state
I verified a local provider row with:
test_status=activeThen I exercised a non-cached successful provider request and confirmed the patched branch clears the stale fields afterward.
I re-checked the
src/app/api/v1surface and confirmed there are no remaining provider-backed routes usinggetProviderCredentials()that still omit recovery clearing after a genuine success. The patched branch covers both success styles already present in the codebase:result.successresponse.okVerification
node --import ./node_modules/tsx/dist/esm/index.mjs --test tests/unit/auth-clear-account-error.test.mjs tests/unit/auth-clear-provider-routes.test.mjsPORT=20132 npm run buildsrc/app/api/v1routes to confirm the success-path coverage is complete and limited to genuine provider success