fix(#6473): retry provider update on concurrent modification errors - #6474
fix(#6473): retry provider update on concurrent modification errors#6474fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
When multiple fullsend run invocations ensure the same provider concurrently, openshell returns a resource_version conflict: "provider was modified concurrently". The updateProvider function now retries with jittered exponential backoff (100ms/200ms/400ms, 3 attempts) on these specific errors. Each retry re-invokes openshell, which re-reads the current resource_version, resolving the conflict. Only concurrent modification errors are retried — other update failures return immediately. The jitter (up to 50% of the backoff interval) reduces collision probability when multiple writers retry at similar times. Closes #6473
|
🤖 Finished Review · ✅ Success · Started 1:21 AM UTC · Completed 1:36 AM UTC Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsLow
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| return fmt.Errorf("provider update %q failed: %w (output: %s)", name, err, outStr) | ||
| lastErr = fmt.Errorf("provider update %q failed: %w (output: %s)", name, err, outStr) | ||
|
|
||
| if !isConcurrentModificationErr(lastErr) { |
There was a problem hiding this comment.
[low] edge-case
isConcurrentModificationErr is checked after secret redaction, so if a secret value is a substring of the openshell error phrase "provider was modified concurrently" (e.g., the secret is literally "provider" or "modified"), the redaction corrupts the match string and retries silently stop triggering. In practice this is unlikely because credential values are tokens/keys, not common English words, but the ordering creates a latent correctness gap.
Suggested fix: Check the raw output for the concurrent-modification pattern before redacting secrets, e.g.: isConcurrent := strings.Contains(strings.ToLower(string(out)), "provider was modified concurrently"), then redact and build lastErr, then branch on isConcurrent.
| // select picks up ctx.Done() during the backoff sleep. The update | ||
| // retry base backoff is 100ms, so 80ms is enough for one update | ||
| // attempt to run but not enough for the backoff to complete. | ||
| ctx, cancel := context.WithTimeout(context.Background(), 80*time.Millisecond) |
There was a problem hiding this comment.
[low] test-flakiness
TestEnsureProvider_ConcurrentModification_ContextCancelled uses a hard-coded 80ms context timeout, relying on two shell-process spawns (create + update) completing within that window so the context expires during the backoff sleep rather than during process execution. Under heavy CI load, process spawning can exceed 80ms. If the context expires during process execution and before the marker file is written, the assertion assert.Len(t, entries, 1) could fail (0 entries instead of 1).
Suggested fix: Increase the timeout to a more conservative value (e.g., 500ms) that still expires well before the first backoff (100-150ms) completes but gives ample time for process spawning. Alternatively, use a cancellable context and cancel it explicitly after observing the first marker file.
fullsend-ai-review
Bot
added
the
ready-for-merge
Summary
Add retry-with-jittered-exponential-backoff to the provider update path to handle optimistic concurrency conflicts. When multiple
fullsend runinvocations ensure the same provider concurrently, openshell returns aresource_versionconflict error. TheupdateProviderfunction now retries up to 3 times (100ms/200ms/400ms with up to 50% jitter) on these specific errors, re-invoking openshell which re-reads the current resource version.Related Issue
Fixes #6473
Changes
isConcurrentModificationErr()to detect the "provider was modified concurrently" error pattern from openshell stderrupdateRetries(3) andupdateRetryBaseBackoff(100ms) constants for update-specific retry configurationupdateProvider()to retry with jittered exponential backoff on concurrent modification errors only — other failures return immediatelyTesting
go test -race ./internal/sandbox/...)updateProvider,isConcurrentModificationErr)go vetcleanCloses #6473
Post-script verification
agent/6473-provider-update-retry)25066fa47de491c5a8326a08c5ab506d55caec22..HEAD)