fix(provider): retry transient response failures safely - #244
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (25)
🚧 Files skipped from review as they are similar to previous changes (20)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds tool-side-effect tracking, prevents retries and context recovery after tool execution, enriches provider errors with retry and request metadata, expands retry classification, adds structured provider logging, and resets terminal streaming state during retry starts. ChangesAssistant runtime and provider error handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Completion
participant ToolExecutor
participant RetryController
Completion->>ToolExecutor: Execute tools
ToolExecutor-->>Completion: Tool execution begins
Completion->>RetryController: Return completion error
RetryController->>RetryController: Check ToolSideEffectsStarted
RetryController-->>Completion: Return original error without retry
sequenceDiagram
participant ProviderHTTP
participant ProviderError
participant RetryController
ProviderHTTP->>ProviderError: Parse status, request ID, and retry headers
ProviderError-->>RetryController: Return structured StatusError
RetryController->>RetryController: Classify provider code and message
RetryController-->>ProviderHTTP: Apply provider or fallback delay
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #244 +/- ##
==========================================
+ Coverage 84.72% 84.78% +0.06%
==========================================
Files 316 316
Lines 29553 29691 +138
==========================================
+ Hits 25039 25174 +135
- Misses 3093 3094 +1
- Partials 1421 1423 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/provider/errors.go (1)
23-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProvider error context-key strings are duplicated as magic literals across the package boundary instead of shared constants.
internal/provider/errors.godefinesproviderRequestIDContextKey/providerResponseIDContextKey(plus pre-existingproviderCodeContextKey/providerTypeContextKey) as unexported constants, sointernal/assistantcannot reference them and instead re-hardcodes the raw strings in two places. If these keys are ever renamed in the provider package, both consumers silently stop matching (no compile error), degrading retry classification and structured logging without any test failure signal.
internal/provider/errors.go#L23-L24: export these context-key constants (and the existing code/type ones) so consumers can reference them directly.internal/assistant/retry.go#L213-L236: replace the"provider_code"/"provider_type"literals inproviderFailureCode/providerErrorContextStringcalls with the exported constants.internal/assistant/runtime_events.go#L88-L94: replace the"provider_request_id"/"provider_response_id"literals with the exported constants.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/provider/errors.go` around lines 23 - 24, Export all provider error context-key constants in internal/provider/errors.go, including the existing code/type keys and request/response ID keys. In internal/assistant/retry.go lines 213-236, replace the provider_code and provider_type literals with the exported constants; in internal/assistant/runtime_events.go lines 88-94, replace the request/response ID literals similarly, using the exported symbols in providerFailureCode and providerErrorContextString calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/assistant/retry.go`:
- Around line 110-117: The provider retry delay can overflow during parsing and
bypass configured retry limits. In internal/assistant/retry.go:110-117, update
providerRetryDelay to cap statusErr.RetryAfter at the configured sane maximum
before returning it; in internal/provider/http.go:87-108, add a fixed ceiling
before converting parsed Retry-After values to time.Duration to prevent
multiplication overflow, and update or remove the comment claiming callers
perform the cap.
In `@internal/assistant/runtime_events.go`:
- Around line 76-98: Update the provider failure logging in the runtime logger
block to replace slog.Any("error", err) with a sanitized slog.String containing
err.Error(). Preserve the existing provider, model, API, attempt, provider_code,
provider_request_id, and provider_response_id attributes.
In `@internal/assistant/runtime_test.go`:
- Line 7: Update the error wrapping in the affected test near the tool execution
assertion to use the existing oops.In("assistant_test").Code(...).Wrapf(...)
pattern instead of fmt.Errorf. Remove the fmt import if it is no longer used,
while preserving the existing error context and message.
---
Nitpick comments:
In `@internal/provider/errors.go`:
- Around line 23-24: Export all provider error context-key constants in
internal/provider/errors.go, including the existing code/type keys and
request/response ID keys. In internal/assistant/retry.go lines 213-236, replace
the provider_code and provider_type literals with the exported constants; in
internal/assistant/runtime_events.go lines 88-94, replace the request/response
ID literals similarly, using the exported symbols in providerFailureCode and
providerErrorContextString calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d0de0d54-dec0-4165-bfcc-8bde6ad1e8dc
📒 Files selected for processing (24)
internal/assistant/client.gointernal/assistant/client_adapter.gointernal/assistant/client_adapter_internal_test.gointernal/assistant/context_build.gointernal/assistant/context_compaction.gointernal/assistant/context_overflow_compaction.gointernal/assistant/context_overflow_compaction_test.gointernal/assistant/export_test.gointernal/assistant/llm_conversion_internal_test.gointernal/assistant/provider_hooks_internal_test.gointernal/assistant/retry.gointernal/assistant/retry_internal_test.gointernal/assistant/retry_test.gointernal/assistant/runtime_events.gointernal/assistant/runtime_model.gointernal/assistant/runtime_test.gointernal/assistant/tool_schema_cache_internal_test.gointernal/provider/client_internal_test.gointernal/provider/errors.gointernal/provider/errors_internal_test.gointernal/provider/http.gointernal/provider/openai_responses_sse.gointernal/terminal/async_events.gointernal/terminal/async_events_internal_test.go
omarluq
force-pushed
the
fix/provider-transient-retries
branch
from
July 29, 2026 20:02
8ad19b1 to
c4e8146
Compare
|
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
1 participant
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.
No description provided.