fix(EVO-2167): retry the AI Processor call on transient failures#4
Open
pastoriniMatheus wants to merge 1 commit into
Open
fix(EVO-2167): retry the AI Processor call on transient failures#4pastoriniMatheus wants to merge 1 commit into
pastoriniMatheus wants to merge 1 commit into
Conversation
The bot-runtime made a single call to the AI Processor: any non-200 (401/5xx) or network error aborted the pipeline and the customer's message was dropped with no reply and no retry (only AI_CALL_TIMEOUT_SECONDS existed). A momentary blip — deploy, restart, DB hiccup — meant a permanently lost answer. - ai_adapter.go: extract a single attempt into doOnce() and wrap Call() in a retry loop with exponential backoff + jitter. Retryable: network errors and 429/500/502/503/504. NOT retried: 4xx (permanent), per-attempt timeout, pipeline cancellation. Body is built once and reused per attempt. - config.go: AI_CALL_MAX_RETRIES (default 2) and AI_CALL_RETRY_BASE_MS (default 200). - main.go: wire the new config into NewAIAdapter. - tests: 503->200 retry succeeds (2 calls); persistent 500 exhausts retries (1+2 calls); 400 not retried (1 call); network error then success (2 calls); maxRetries=0 disables retry. Existing tests updated to the new signature (0 retries). Complements EVO-2166: the processor now returns 503 (not a silent 401) on infra errors, so this retry covers the transient auth/infra case. Root cause of the incident is EVO-2141 (pool_pre_ping, already merged); this is defense in depth. Note: test/e2e/e2e_test.go was already incompatible with NewAIAdapter on develop (pre-existing, unrelated) and is left as-is; repo CI is docker-only (no go test lane).
Reviewer's GuideImplements configurable retry logic with exponential backoff and jitter for AI Processor calls, wiring it through configuration and main, and adding focused tests to validate retry and non-retry scenarios while preserving existing single-call behavior when retries are disabled. Sequence diagram for AIAdapter Call retry on transient failuressequenceDiagram
actor BotRuntime
participant AIAdapter
participant AIProcessor
BotRuntime->>AIAdapter: Call(ctx, A2ARequest)
loop attempts = maxRetries + 1
AIAdapter->>AIProcessor: client.Do(httpReq)
alt [network error]
AIAdapter-->>AIAdapter: doOnce(ctx, url, body, req, start) returns (nil, true, err)
opt [more retries]
AIAdapter-->>AIAdapter: backoffDelay(attempt)
end
else [status 5xx/429]
AIProcessor-->>AIAdapter: HTTP 5xx/429
AIAdapter-->>AIAdapter: isRetryableStatus(StatusCode) = true
opt [more retries]
AIAdapter-->>AIAdapter: backoffDelay(attempt)
end
else [status 4xx/timeout/cancel/decode error]
AIProcessor-->>AIAdapter: HTTP 4xx or other error
AIAdapter-->>BotRuntime: error (no retry)
AIAdapter-->>AIAdapter: break
else [HTTP 200]
AIProcessor-->>AIAdapter: HTTP 200
AIAdapter-->>AIAdapter: doOnce(...) returns NormalizedResponse
AIAdapter-->>BotRuntime: NormalizedResponse
AIAdapter-->>AIAdapter: break
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The jitter implementation in backoffDelay uses the global math/rand without seeding, so every process will produce the same retry timing pattern; consider seeding once at startup or using a per-adapter rand.Source to get more realistic jitter.
- In the retry loop, using time.After directly in the select allocates a new timer on every iteration even if the context is already cancelled; consider using time.NewTimer with a defer timer.Stop() or checking ctx.Done() before creating the timer to avoid unnecessary allocations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The jitter implementation in backoffDelay uses the global math/rand without seeding, so every process will produce the same retry timing pattern; consider seeding once at startup or using a per-adapter rand.Source to get more realistic jitter.
- In the retry loop, using time.After directly in the select allocates a new timer on every iteration even if the context is already cancelled; consider using time.NewTimer with a defer timer.Stop() or checking ctx.Done() before creating the timer to avoid unnecessary allocations.
## Individual Comments
### Comment 1
<location path="pkg/ai/service/ai_adapter.go" line_range="227-228" />
<code_context>
+ if d <= 0 || d > maxBackoff {
+ d = maxBackoff
+ }
+ // Keep at least half the delay, add up to half more as jitter.
+ jitter := time.Duration(rand.Int63n(int64(d/2) + 1))
+ return d/2 + jitter
}
</code_context>
<issue_to_address>
**suggestion:** Consider seeding or isolating the RNG source for jittered backoff
Using the package-level `math/rand` default source means the jitter sequence is deterministic (seeded to 1) unless something else seeds it, so all processes will see the same jitter pattern after startup. For more effective jitter, seed the RNG once at startup or use a dedicated `rand.Source`/`rand.Rand` for backoff (e.g., on `aiAdapter`).
Suggested implementation:
```golang
import (
"math/rand"
// existing imports...
)
// backoffDelay returns the exponential backoff for the given attempt (1-based)
// with up to 50% jitter, capped at maxBackoff.
func (a *aiAdapter) backoffDelay(attempt int) time.Duration {
d := a.retryBaseDelay << (attempt - 1) // base * 2^(attempt-1)
if d <= 0 || d > maxBackoff {
d = maxBackoff
}
// Lazily initialize a jitter RNG specific to this adapter instance to
// avoid using the global math/rand source and to get per-process jitter.
if a.rng == nil {
a.rng = rand.New(rand.NewSource(time.Now().UnixNano()))
}
// Keep at least half the delay, add up to half more as jitter.
maxJitter := d / 2
jitter := time.Duration(a.rng.Int63n(int64(maxJitter) + 1))
return d/2 + jitter
```
```golang
type aiAdapter struct {
timeoutSecs int
maxRetries int
retryBaseDelay time.Duration
client *http.Client
// rng is a per-adapter RNG used for jittered backoff.
rng *rand.Rand
}
```
1. The import edit above assumes a simple import block; you may need to merge the `math/rand` import into the existing `import (...)` block and remove the placeholder comment `// existing imports...`.
2. If you already have a constructor or initializer for `aiAdapter`, you could prefer initializing `rng` there instead of lazily inside `backoffDelay`, e.g. `rng: rand.New(rand.NewSource(time.Now().UnixNano())),` to avoid the `nil` check on every call.
3. If your project uses a central randomness source (for testability), you may want to inject a `rand.Source` or `*rand.Rand` into `aiAdapter` instead of creating it with `time.Now().UnixNano()` directly.
</issue_to_address>
### Comment 2
<location path="pkg/ai/service/ai_adapter_retry_test.go" line_range="97-58" />
<code_context>
+ }
+}
+
+func TestCall_RetriesOnNetworkErrorThenSucceeds(t *testing.T) {
+ var calls int32
+ server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ if atomic.AddInt32(&calls, 1) == 1 {
+ panic(http.ErrAbortHandler) // abruptly close the connection -> client network error
+ }
+ writeOK(w)
+ }))
+ defer server.Close()
+
+ adapter := aiService.NewAIAdapter(30, 2, 1)
+ resp, err := adapter.Call(context.Background(), retryReq(server.URL))
+ if err != nil {
+ t.Fatalf("expected success after network retry, got error: %v", err)
+ }
+ if resp.Content != "ok" {
+ t.Errorf("content = %q, want ok", resp.Content)
+ }
+ if got := atomic.LoadInt32(&calls); got != 2 {
+ t.Errorf("server calls = %d, want 2", got)
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test to ensure retries stop when the context is cancelled during backoff
Current tests cover retries on HTTP and network errors, but not how retries interact with `ctx.Done()` during backoff. Please add a test that triggers an initial transient failure with `maxRetries > 0`, cancels the context during the backoff sleep, and verifies that `Call` returns `ErrPipelineCancelled` and does not issue additional HTTP requests. This will ensure cancellation is honored even when a retry is pending.
Suggested implementation:
```golang
if got := atomic.LoadInt32(&calls); got != 1 {
t.Errorf("server calls = %d, want 1 (400 is permanent, no retry)", got)
}
}
func TestCall_StopsRetryWhenContextCancelledDuringBackoff(t *testing.T) {
var calls int32
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
atomic.AddInt32(&calls, 1)
// Always return a transient error to trigger retry/backoff.
w.WriteHeader(http.StatusServiceUnavailable)
}))
defer server.Close()
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
adapter := aiService.NewAIAdapter(30, 3, 1)
// Cancel the context while the adapter is in its backoff period before retrying.
go func() {
time.Sleep(10 * time.Millisecond)
cancel()
}()
resp, err := adapter.Call(ctx, retryReq(server.URL))
if !errors.Is(err, aiService.ErrPipelineCancelled) {
t.Fatalf("expected ErrPipelineCancelled, got: %v", err)
}
if resp != nil {
t.Fatalf("expected no response on cancellation, got: %#v", resp)
}
if got := atomic.LoadInt32(&calls); got != 1 {
t.Errorf("server calls = %d, want 1 (no additional calls after cancellation)", got)
}
}
func TestCall_RetriesOn503ThenSucceeds(t *testing.T) {
```
To compile, ensure the test file's import block includes the required packages if they are not already present:
1. Add `time` and `errors` to the imports, e.g.:
<<<<<<< SEARCH
import (
=======
import (
"errors"
"time"
>>>>>>> REPLACE
(merge these into the existing import list alongside `context`, `net/http/httptest`, `sync/atomic`, etc., without duplicating any imports.)
This new test assumes `ErrPipelineCancelled` is exported from the `aiService` package and is the error returned when the context is canceled. If a different sentinel error is used in your codebase, replace `aiService.ErrPipelineCancelled` with the correct value.
</issue_to_address>
### Comment 3
<location path="pkg/ai/service/ai_adapter_test.go" line_range="88" />
<code_context>
defer server.Close()
- adapter := aiService.NewAIAdapter(30)
+ adapter := aiService.NewAIAdapter(30, 0, 0)
resp, err := adapter.Call(context.Background(), &aiModel.A2ARequest{
OutgoingURL: server.URL + "/api/v1/a2a/agent-123",
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a small constructor-focused test to cover normalization of maxRetries and retryBaseMs
The new `NewAIAdapter` behavior that normalizes negative `maxRetries` to 0 and `retryBaseMs <= 0` to 200 isn’t currently covered by tests, since all callers use valid, non-zero values. Please add a focused unit test (e.g. `TestNewAIAdapter_NormalizesConfig`) that constructs adapters with `maxRetries = -1` and `retryBaseMs = 0` (or negative) and asserts that `maxRetries` and `retryBaseDelay` are normalized as expected. This will help prevent regressions in the defensive config handling.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+227
to
+228
| // Keep at least half the delay, add up to half more as jitter. | ||
| jitter := time.Duration(rand.Int63n(int64(d/2) + 1)) |
There was a problem hiding this comment.
suggestion: Consider seeding or isolating the RNG source for jittered backoff
Using the package-level math/rand default source means the jitter sequence is deterministic (seeded to 1) unless something else seeds it, so all processes will see the same jitter pattern after startup. For more effective jitter, seed the RNG once at startup or use a dedicated rand.Source/rand.Rand for backoff (e.g., on aiAdapter).
Suggested implementation:
import (
"math/rand"
// existing imports...
)
// backoffDelay returns the exponential backoff for the given attempt (1-based)
// with up to 50% jitter, capped at maxBackoff.
func (a *aiAdapter) backoffDelay(attempt int) time.Duration {
d := a.retryBaseDelay << (attempt - 1) // base * 2^(attempt-1)
if d <= 0 || d > maxBackoff {
d = maxBackoff
}
// Lazily initialize a jitter RNG specific to this adapter instance to
// avoid using the global math/rand source and to get per-process jitter.
if a.rng == nil {
a.rng = rand.New(rand.NewSource(time.Now().UnixNano()))
}
// Keep at least half the delay, add up to half more as jitter.
maxJitter := d / 2
jitter := time.Duration(a.rng.Int63n(int64(maxJitter) + 1))
return d/2 + jittertype aiAdapter struct {
timeoutSecs int
maxRetries int
retryBaseDelay time.Duration
client *http.Client
// rng is a per-adapter RNG used for jittered backoff.
rng *rand.Rand
}- The import edit above assumes a simple import block; you may need to merge the
math/randimport into the existingimport (...)block and remove the placeholder comment// existing imports.... - If you already have a constructor or initializer for
aiAdapter, you could prefer initializingrngthere instead of lazily insidebackoffDelay, e.g.rng: rand.New(rand.NewSource(time.Now().UnixNano())),to avoid thenilcheck on every call. - If your project uses a central randomness source (for testability), you may want to inject a
rand.Sourceor*rand.RandintoaiAdapterinstead of creating it withtime.Now().UnixNano()directly.
| if resp.Content != "ok" { | ||
| t.Errorf("content = %q, want ok", resp.Content) | ||
| } | ||
| if got := atomic.LoadInt32(&calls); got != 2 { |
There was a problem hiding this comment.
suggestion (testing): Add a test to ensure retries stop when the context is cancelled during backoff
Current tests cover retries on HTTP and network errors, but not how retries interact with ctx.Done() during backoff. Please add a test that triggers an initial transient failure with maxRetries > 0, cancels the context during the backoff sleep, and verifies that Call returns ErrPipelineCancelled and does not issue additional HTTP requests. This will ensure cancellation is honored even when a retry is pending.
Suggested implementation:
if got := atomic.LoadInt32(&calls); got != 1 {
t.Errorf("server calls = %d, want 1 (400 is permanent, no retry)", got)
}
}
func TestCall_StopsRetryWhenContextCancelledDuringBackoff(t *testing.T) {
var calls int32
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
atomic.AddInt32(&calls, 1)
// Always return a transient error to trigger retry/backoff.
w.WriteHeader(http.StatusServiceUnavailable)
}))
defer server.Close()
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
adapter := aiService.NewAIAdapter(30, 3, 1)
// Cancel the context while the adapter is in its backoff period before retrying.
go func() {
time.Sleep(10 * time.Millisecond)
cancel()
}()
resp, err := adapter.Call(ctx, retryReq(server.URL))
if !errors.Is(err, aiService.ErrPipelineCancelled) {
t.Fatalf("expected ErrPipelineCancelled, got: %v", err)
}
if resp != nil {
t.Fatalf("expected no response on cancellation, got: %#v", resp)
}
if got := atomic.LoadInt32(&calls); got != 1 {
t.Errorf("server calls = %d, want 1 (no additional calls after cancellation)", got)
}
}
func TestCall_RetriesOn503ThenSucceeds(t *testing.T) {To compile, ensure the test file's import block includes the required packages if they are not already present:
- Add
timeanderrorsto the imports, e.g.:
<<<<<<< SEARCH
import (
import (
"errors"
"time"
REPLACE
(merge these into the existing import list alongside context, net/http/httptest, sync/atomic, etc., without duplicating any imports.)
This new test assumes ErrPipelineCancelled is exported from the aiService package and is the error returned when the context is canceled. If a different sentinel error is used in your codebase, replace aiService.ErrPipelineCancelled with the correct value.
| defer server.Close() | ||
|
|
||
| adapter := aiService.NewAIAdapter(30) | ||
| adapter := aiService.NewAIAdapter(30, 0, 0) |
There was a problem hiding this comment.
suggestion (testing): Consider a small constructor-focused test to cover normalization of maxRetries and retryBaseMs
The new NewAIAdapter behavior that normalizes negative maxRetries to 0 and retryBaseMs <= 0 to 200 isn’t currently covered by tests, since all callers use valid, non-zero values. Please add a focused unit test (e.g. TestNewAIAdapter_NormalizesConfig) that constructs adapters with maxRetries = -1 and retryBaseMs = 0 (or negative) and asserts that maxRetries and retryBaseDelay are normalized as expected. This will help prevent regressions in the defensive config handling.
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.
EVO-2167 — retry na chamada ao AI Processor em falha transitória
Root cause
O
bot-runtimefazia uma única chamada ao AI Processor (pkg/ai/service/ai_adapter.go): qualquer status ≠ 200 (401/5xx) ou erro de rede abortava o pipeline e a mensagem do cliente era descartada, sem resposta e sem reenvio — só existiaAI_CALL_TIMEOUT_SECONDS. Um blip momentâneo (deploy, restart, hiccup de DB) = resposta perdida em definitivo.Correção
ai_adapter.godoOnce()e envolveCall()num loop de retry com backoff exponencial + jitter (cap 5s). Retenta: erros de rede e429/500/502/503/504. Não retenta:4xx(permanente), timeout por tentativa, cancelamento do pipeline. Body construído uma vez e reusadoconfig.goAI_CALL_MAX_RETRIES(default 2) eAI_CALL_RETRY_BASE_MS(default 200)main.goNewAIAdapterLog
pipeline.ai.retrypor tentativa (observabilidade).Sinergia com EVO-2166
Com o EVO-2166, o processor passa a devolver 503 (não um 401 silencioso) em erro de infra — então este retry cobre o caso transitório de auth/infra sem reenviar 401 genuíno (chave inválida). Causa-raiz do incidente é a EVO-2141 (pool_pre_ping, já mergeada); isto é defesa em profundidade.
Testes (
ai_adapter_retry_test.go, httptest contando requisições)maxRetries=0: desabilita retry (1 chamada)Testes existentes atualizados para a nova assinatura (
0retries = comportamento single-call). Resultado local:go build ./...✅ ·go vet ./pkg/ai/... ./internal/config/...✅ ·go test ./pkg/ai/... ./internal/config/...ok (3.5s) ✅.Relacionado
Summary by Sourcery
Add configurable retry logic with exponential backoff for AI Processor calls to avoid dropping customer replies on transient failures.
New Features:
Enhancements:
Tests: