From 402cb7b48f47569b476c1c654b736153a16b95aa Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Sun, 23 Aug 2026 10:33:39 -0400 Subject: [PATCH] fix(sandbox): retry provider updates rejected as modified concurrently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EnsureProvider already retries the "unsupported provider type or profile" error from a concurrent ImportProfile delete+reimport, but not the error that follows it: when several fullsend runs start on one gateway 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 outright before the agent started. This hit the fullsend-ai/agents functional tests on every PR that changes profiles/fullsend-vertex-ai.yaml (eval parallelism 4): one triage case died in ~1s with that error in three of four runs of agents#965, and on main — where the profile is unchanged — never. Treat the conflict as the same transient class and retry the create+update cycle with the existing backoff; the update is idempotent. openshell wraps the message across lines with a box-drawing gutter, so it is matched with a regexp across the wrap rather than as a contiguous substring — the first attempt at a substring match did not match the real output. Assisted-by: Claude (code) Signed-off-by: Wayne Sun --- internal/sandbox/sandbox.go | 46 +++++++++++++++++++-------- internal/sandbox/sandbox_test.go | 54 ++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 13 deletions(-) diff --git a/internal/sandbox/sandbox.go b/internal/sandbox/sandbox.go index 03ca39db2a..9a1a8430c7 100644 --- a/internal/sandbox/sandbox.go +++ b/internal/sandbox/sandbox.go @@ -13,6 +13,7 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "sort" "strings" "syscall" @@ -52,8 +53,11 @@ const ( retryMaxBackoff = 15 * time.Second // providerRetries is the number of times EnsureProvider retries on - // transient "unsupported provider type or profile" errors caused by - // concurrent ImportProfile processes deleting and reimporting profiles. + // transient errors caused by concurrent fullsend runs sharing a + // gateway: "unsupported provider type or profile" (a concurrent + // ImportProfile deleting and reimporting a changed profile) and the + // gateway's optimistic-concurrency rejection of a provider update + // ("provider was modified concurrently"). providerRetries = 3 providerRetryBackoff = 500 * time.Millisecond ) @@ -309,9 +313,12 @@ var reservedCredentialKeys = map[string]bool{ // into the child process environment, where openshell reads them directly. // See https://docs.nvidia.com/openshell/latest/sandboxes/manage-providers#bare-key-form // -// Transient "unsupported provider type or profile" errors are retried with -// short backoff. These occur when a concurrent ImportProfile process -// temporarily removes a profile during its delete+reimport cycle. +// Transient errors from concurrent runs on the same gateway are retried with +// short backoff: "unsupported provider type or profile" occurs when a +// concurrent ImportProfile process temporarily removes a profile during its +// delete+reimport cycle, and "provider was modified concurrently" when two +// runs update the same provider at once (the gateway rejects the stale +// resource_version; the update is idempotent, so retrying is safe). func EnsureProvider(ctx context.Context, name, providerType string, credentials, config map[string]string, fromURL bool) error { if fromURL { for k := range credentials { @@ -329,9 +336,8 @@ func EnsureProvider(ctx context.Context, name, providerType string, credentials, if lastErr == nil { return nil } - // Retry only on "unsupported provider type or profile" — this is - // the transient error from the ImportProfile race. - if !isUnsupportedProviderErr(lastErr) { + // Retry only on the transient concurrency errors. + if !isTransientProviderErr(lastErr) { return lastErr } if attempt < providerRetries-1 { @@ -345,17 +351,31 @@ func EnsureProvider(ctx context.Context, name, providerType string, credentials, return fmt.Errorf("retries exhausted after %d attempts: %w", providerRetries, lastErr) } -// isUnsupportedProviderErr reports whether err contains the openshell -// error message indicating a missing or not-yet-imported profile. +// isTransientProviderErr reports whether err is one of the openshell +// errors produced by concurrent runs racing on the same gateway: a missing +// or not-yet-reimported profile, or an optimistic-concurrency rejection of +// a provider update ("provider was modified concurrently (current +// resource_version: N)"). // // NOTE: This matches literal text from the openshell CLI's stderr output. // If openshell changes its error wording in a future version, this check // will silently stop matching and retries will no longer trigger. Update -// the substring if the upstream message changes. -func isUnsupportedProviderErr(err error) bool { - return err != nil && strings.Contains(strings.ToLower(err.Error()), "unsupported provider type or profile") +// the substrings if the upstream messages change. +func isTransientProviderErr(err error) bool { + if err == nil { + return false + } + msg := strings.ToLower(err.Error()) + return strings.Contains(msg, "unsupported provider type or profile") || + providerModifiedConcurrentlyRe.MatchString(msg) } +// providerModifiedConcurrentlyRe matches openshell's optimistic-concurrency +// error. The CLI wraps the message across lines with a box-drawing gutter +// (`"provider was modified` / `│ concurrently (current resource_version: 2)"`), +// so the two words are matched across any non-word characters. +var providerModifiedConcurrentlyRe = regexp.MustCompile(`provider was modified\W+concurrently`) + // tryCreateProvider performs a single attempt to create (or update) a // provider via openshell. Extracted from EnsureProvider to support retry. func tryCreateProvider(ctx context.Context, name string, args, extraEnv, secrets []string, credentials, config map[string]string, fromURL bool) error { diff --git a/internal/sandbox/sandbox_test.go b/internal/sandbox/sandbox_test.go index 01c657c631..71d20de2e4 100644 --- a/internal/sandbox/sandbox_test.go +++ b/internal/sandbox/sandbox_test.go @@ -1617,6 +1617,60 @@ exit 0 assert.Len(t, entries, 3, "should have made 3 attempts") } +// TestEnsureProvider_RetriesConcurrentUpdateConflict verifies that when the +// provider already exists and the gateway rejects the update because another +// run modified it concurrently, EnsureProvider retries the create+update +// cycle instead of failing the run. +func TestEnsureProvider_RetriesConcurrentUpdateConflict(t *testing.T) { + dir := t.TempDir() + markerDir := filepath.Join(dir, "markers") + require.NoError(t, os.MkdirAll(markerDir, 0o755)) + + // Fake openshell: create always reports the provider exists; update + // fails with the optimistic-concurrency error until the 3rd attempt. + script := fmt.Sprintf(`#!/bin/sh +if [ "$2" = "create" ]; then + echo "Error: × code: 'Some entity that we attempted to create already exists', message: \"provider already exists\"" >&2 + exit 1 +fi +if [ "$2" = "update" ]; then + echo x > "%s/attempt.$$" + count=0 + for f in "%s"/attempt.*; do + [ -e "$f" ] && count=$((count + 1)) + done + if [ "$count" -lt 3 ]; then + echo "Error: × code: 'The operation was aborted', message: \"provider was modified" >&2 + echo " │ concurrently (current resource_version: 2)\"" >&2 + exit 1 + fi + exit 0 +fi +exit 0 +`, markerDir, markerDir) + fakePath := filepath.Join(dir, "openshell") + require.NoError(t, os.WriteFile(fakePath, []byte(script), 0o755)) + t.Setenv("PATH", dir) + + err := EnsureProvider(context.Background(), "vertex-ai", "vertex-ai", nil, nil, false) + assert.NoError(t, err, "should succeed after retrying the conflicting update") + + entries, readErr := os.ReadDir(markerDir) + require.NoError(t, readErr) + assert.Len(t, entries, 3, "should have retried the update 3 times") +} + +func TestIsTransientProviderErr(t *testing.T) { + t.Parallel() + wrapped := "provider update \"vertex-ai\" failed: exit status 1 (output: Error: × code: 'The operation was aborted', message: \"provider was modified\n │ concurrently (current resource_version: 2)\"\n)" + assert.False(t, isTransientProviderErr(nil)) + assert.True(t, isTransientProviderErr(fmt.Errorf("x: unsupported provider type or profile: p"))) + assert.True(t, isTransientProviderErr(errors.New(wrapped)), "must match across the CLI's line wrap") + assert.True(t, isTransientProviderErr(errors.New("provider was modified concurrently"))) + assert.False(t, isTransientProviderErr(fmt.Errorf("status: PermissionDenied"))) + assert.False(t, isTransientProviderErr(fmt.Errorf("provider was modified by an operator"))) +} + // TestEnsureProvider_NoRetryOnOtherErrors verifies that non-transient // errors are not retried. func TestEnsureProvider_NoRetryOnOtherErrors(t *testing.T) {