fix(sandbox): retry provider updates rejected as modified concurrently

[waynesun09]

Summary

EnsureProvider retries the transient unsupported provider type or profile error from a concurrent ImportProfile delete+reimport, but not the error that follows it on a shared gateway: when several fullsend runs start right after a profile changed, each re-imports the profile and then updates the provider, and the gateway rejects all but one update with provider was modified concurrently (current resource_version: N). The losing run failed before the agent started. This retries that conflict with the existing backoff (the update is idempotent).

Related Issue

No tracker issue. Observed in the fullsend-ai/agents functional tests (eval parallelism: 4): one triage case died in ~1 s at $0.00 with this error in three of four runs of fullsend-ai/agents#965 (which changes profiles/fullsend-vertex-ai.yaml), including a merge-queue attempt, and never on main where the profile is unchanged. Any PR touching a shared profile can hit it.

Changes

• internal/sandbox/sandbox.go: isUnsupportedProviderErr → isTransientProviderErr, which also matches the optimistic-concurrency rejection; the existing retry loop (3 attempts, 500 ms backoff) now covers both. openshell wraps the message across lines with a box-drawing gutter ("provider was modified / │ concurrently (…)), so the match is a regexp across the wrap (provider was modified\W+concurrently) — a contiguous substring does not match the real output (the new test reproduces the wrapped form).
• internal/sandbox/sandbox_test.go: TestEnsureProvider_RetriesConcurrentUpdateConflict (fake openshell: create → "already exists", update → conflict twice, then success; asserts 3 update attempts and overall success) and TestIsTransientProviderErr (wrapped and unwrapped forms, negatives).

Testing

• make lint equivalent on the changed files: gofmt, go vet, pre-commit — clean (no golangci-lint on this host; CI build/test cover it)
• Tests added/updated for new or modified logic — go test ./internal/sandbox/ passes
• After merge: re-run the agents functional tests on agents#965 (they build fullsend from main) — the triage race should no longer fail the job

Checklist

• PR title follows Conventional Commits (correct type, ! for breaking changes)
• Commits are signed off (DCO) — human and human-directed agent sessions only
• I wrote this contribution myself and can explain all changes in it

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:35 PM UTC · Completed 2:48 PM UTC

Commit: 402cb7b · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Retry EnsureProvider when provider update is rejected due to concurrent modification

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Expand EnsureProvider retry logic to treat gateway concurrency rejections as transient.
• Match openshell’s wrapped “provider was modified concurrently” output via regexp.
• Add targeted tests covering conflict retries and transient-error classification.

Diagram

graph TD
  A["EnsureProvider"] --> B["tryCreateProvider"] --> C{{"openshell CLI"}}
  C -->|"error output"| F["isTransientProviderErr"] --> G{"Transient?"}
  G -->|"yes"| H["Backoff (500ms) + retry (max 3)"] --> B
  G -->|"no"| X["Return error"]
  C -->|"success"| S["Return nil"]

  subgraph Legend
    direction LR
    _fn["Function"] ~~~ _ext{{"External CLI"}} ~~~ _dec{"Decision"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Prefer structured error codes (if available) over stderr text matching

• ➕ More robust to wording/formatting changes in openshell output
• ➕ Allows precise branching (e.g., retry only on specific codes)
• ➖ Requires openshell support (e.g., JSON output) or changes to invocation/parsing
• ➖ More implementation complexity than a simple regexp

2. Add jitter / exponential backoff for provider retry loop

• ➕ Reduces thundering-herd retries when many runs start simultaneously
• ➕ Better behavior on busy/shared gateways
• ➖ Slightly longer worst-case startup time in tests/CI
• ➖ More tuning/complexity than current fixed short backoff

Recommendation: Current approach is appropriate given the constraints: EnsureProvider’s update is idempotent, the retry loop already exists, and the new regexp handles the real (wrapped) CLI output. If openshell exposes stable machine-readable error codes in the future, migrating from stderr substring/regex matching to code-based matching would further reduce brittleness.

Files changed (2) +87 / -13

Bug fix (1) +33 / -13

sandbox.goRetry on optimistic-concurrency provider update conflicts +33/-13

Retry on optimistic-concurrency provider update conflicts

• Renames and expands the transient-error classifier to include the gateway’s "provider was modified concurrently" rejection in addition to the existing "unsupported provider type or profile" case. Uses a regexp to match openshell’s wrapped stderr formatting and gates the existing 3-attempt retry loop on this broader transient classification.

internal/sandbox/sandbox.go

Tests (1) +54 / -0

sandbox_test.goAdd tests for concurrent update conflict retry and transient matcher +54/-0

Add tests for concurrent update conflict retry and transient matcher

• Adds a fake-openshell test that simulates an existing provider and two consecutive optimistic-concurrency update failures before succeeding, asserting EnsureProvider retries and eventually succeeds. Adds unit tests for isTransientProviderErr covering wrapped/unwrapped conflict messages and negative cases.

internal/sandbox/sandbox_test.go

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can turn these tips off under Display preferences

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Looks good to me

[waynesun09]

Merge-queue E2E failed twice (runs 32889823311, 32894999360), both with harness "fork-pr-sync" dispatch count did not settle … still pending after 12m0s, on two different fixture pools. Root cause is not this PR (diff is internal/sandbox/* only; own E2E on 402cb7b is green): since agents#861 merged at 17:37Z the review pre-script unshallows the ~1.6 GB e2e fixture repos on every run — 71 s to 18 min observed — see fullsend-ai/agents#1032 for the log evidence and fix proposal. Re-enqueuing once more since ~90% of post-merge E2E runs still pass; if it fails a third time, hold until the agents fix lands.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 9:17 PM UTC · Completed 9:26 PM UTC

Commit: 402cb7b · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.59

[fullsend-ai-retro]

Retro: PR #6523 — fix(sandbox): retry provider updates rejected as modified concurrently

Outcome: Workflow went well. The review agent correctly approved a clean, well-tested human-authored bug fix. The human reviewer (rh-hemartin) independently reached the same conclusion. No rework was needed. The PR merged on its third merge-queue attempt after two unrelated E2E failures.

Timeline

1. 2026-08-23 14:33 UTC — waynesun09 opens the PR, adding retry logic for optimistic-concurrency rejections in EnsureProvider.
2. 2026-08-23 14:34–14:48 UTC — Review agent run 32645818290 dispatches 4 sub-agents (correctness, style-conventions, intent-coherence, docs-currency) via the pr-review orchestrator. 3 of 4 sub-agents fail on first attempt because claude-sonnet-4-5@20250929 is unavailable on Vertex AI; the orchestrator retries all three on Opus. A single low-severity finding (missing linked issue) is downgraded to informational by the challenger. Verdict: approve. Cost: $3.98, duration: ~14 min.
3. 2026-08-23 14:35–14:38 UTC — Qodo and Codecov report clean results (zero issues, 100% patch coverage).
4. 2026-08-25 09:32 UTC — Human reviewer rh-hemartin approves with no comments.
5. 2026-08-25 19:29–20:40 UTC — Two merge-queue attempts fail (32889823311, 32894999360) on the fork-pr-sync behaviour test timing out after 12 minutes. Root cause: agents#861 introduced unbounded git fetch --unshallow on 1.6 GB fixture repos, exceeding the settle window.
6. 2026-08-25 21:01–21:15 UTC — Third merge-queue attempt succeeds. PR merged.

Assessment

• Review quality: The review agent's approval was correct. The correctness sub-agent explicitly analyzed the regex and confirmed it handles openshell's box-drawing line wraps. The challenger appropriately downgraded the missing-issue finding. Zero inline comments was a reasonable outcome for this clean change.
• Rework rate: Zero iterations. The human author got it right the first time.
• Token cost: $3.98 is reasonable for a 4-dimension review, though model fallback failures added unnecessary cost and ~3 min latency.
• Autonomy readiness: Agent and human reviews aligned perfectly — both approved without inline comments. This is a positive autonomy signal for well-structured, focused human-authored bug fixes with comprehensive tests.

Evidence for existing issues (no new proposals needed)

• #1771 (model fallback for sub-agents): Still occurring as of 2026-08-23 — 3 of 4 sub-agents failed on claude-sonnet-4-5@20250929 unavailability, adding cost and latency.
• fullsend-ai/agents#1032 (pre-review unshallow breaks E2E settle window): Caused 2 of 3 merge-queue attempts to fail on this PR, adding ~2 hours to time-to-merge.

No novel improvement opportunities were identified. The workflow functioned as designed, and both known issues are already tracked with fixes in progress.