fix(cli): retry provider attachment on a stale resource version

Attach and detach send the resource version the command itself just
read, so a concurrent status write, such as a supervisor configuration
result, could fail the command with "sandbox was modified by another
operation". Retry those conflicts with a fresh read, up to five times.
Mutations whose save is uncertain still never retry.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>

Author: pimlock

crates/openshell-cli/src/commands/provider.rs

@@ -129,6 +129,10 @@ pub async fn sandbox_provider_list(
     Ok(())
 }
 
+/// Attempts for an attach or detach whose resource version went stale between
+/// the command's read and its write.
+const PROVIDER_ATTACHMENT_CAS_ATTEMPTS: u32 = 5;
+
 /// Save a provider attachment and optionally wait for its installed authority.
 pub async fn sandbox_provider_attach(
     server: &str,
@@ -141,45 +145,53 @@ pub async fn sandbox_provider_attach(
     readiness.validate()?;
     let mut client = grpc_client(server, tls).await?;
 
-    // Fetch current sandbox to get resource_version for CAS
-    let sandbox = client
-        .get_sandbox(GetSandboxRequest {
-            name: (name).to_string(),
-            workspace_scope: Some(openshell_core::proto::workspace_selector(
-                (workspace).to_string(),
-            )),
-        })
-        .await
-        .map_err(|status| miette!("provider attachment lookup failed ({})", status.code()))?
-        .into_inner()
-        .sandbox
-        .ok_or_else(|| miette::miette!("sandbox not found"))?;
+    // The resource version only guards this command's own read, so a
+    // conflict from a concurrent status write is retried with a fresh read.
+    let mut attempt = 1;
+    let (sandbox, response) = loop {
+        let sandbox = client
+            .get_sandbox(GetSandboxRequest {
+                name: (name).to_string(),
+                workspace_scope: Some(openshell_core::proto::workspace_selector(
+                    (workspace).to_string(),
+                )),
+            })
+            .await
+            .map_err(|status| miette!("provider attachment lookup failed ({})", status.code()))?
+            .into_inner()
+            .sandbox
+            .ok_or_else(|| miette::miette!("sandbox not found"))?;
 
-    let resource_version = sandbox.metadata.as_ref().map_or(0, |m| m.resource_version);
+        let resource_version = sandbox.metadata.as_ref().map_or(0, |m| m.resource_version);
 
-    let response = match client
-        .attach_sandbox_provider(AttachSandboxProviderRequest {
-            request_id: String::new(),
-            sandbox: (name).to_string(),
-            workspace_scope: Some(openshell_core::proto::workspace_selector(
-                (workspace).to_string(),
-            )),
-            provider: provider.to_string(),
-            expected_resource_version: resource_version,
-        })
-        .await
-    {
-        Ok(response) => response.into_inner(),
-        // Explicit post-save uncertainty takes precedence over a generic retry hint.
-        Err(status)
-            if status.code() == Code::Aborted && !provider_mutation_is_uncertain(&status) =>
+        match client
+            .attach_sandbox_provider(AttachSandboxProviderRequest {
+                request_id: String::new(),
+                sandbox: (name).to_string(),
+                workspace_scope: Some(openshell_core::proto::workspace_selector(
+                    (workspace).to_string(),
+                )),
+                provider: provider.to_string(),
+                expected_resource_version: resource_version,
+            })
+            .await
         {
-            return Err(miette::miette!(
-                "Failed to attach provider: sandbox was modified by another operation.\n\
-                 Please retry the command."
-            ));
+            Ok(response) => break (sandbox, response.into_inner()),
+            // Explicit post-save uncertainty takes precedence over a generic retry hint.
+            Err(status)
+                if status.code() == Code::Aborted && !provider_mutation_is_uncertain(&status) =>
+            {
+                if attempt < PROVIDER_ATTACHMENT_CAS_ATTEMPTS {
+                    attempt += 1;
+                    continue;
+                }
+                return Err(miette::miette!(
+                    "Failed to attach provider: sandbox was modified by another operation.\n\
+                     Please retry the command."
+                ));
+            }
+            Err(error) => return Err(provider_mutation_error(&error, "attachment")),
         }
-        Err(error) => return Err(provider_mutation_error(&error, "attachment")),
     };
 
     let receipt = response.receipt.ok_or_else(|| {
@@ -216,45 +228,53 @@ pub async fn sandbox_provider_detach(
     readiness.validate()?;
     let mut client = grpc_client(server, tls).await?;
 
-    // Fetch current sandbox to get resource_version for CAS
-    let sandbox = client
-        .get_sandbox(GetSandboxRequest {
-            name: (name).to_string(),
-            workspace_scope: Some(openshell_core::proto::workspace_selector(
-                (workspace).to_string(),
-            )),
-        })
-        .await
-        .map_err(|status| miette!("provider detachment lookup failed ({})", status.code()))?
-        .into_inner()
-        .sandbox
-        .ok_or_else(|| miette::miette!("sandbox not found"))?;
+    // The resource version only guards this command's own read, so a
+    // conflict from a concurrent status write is retried with a fresh read.
+    let mut attempt = 1;
+    let (sandbox, response) = loop {
+        let sandbox = client
+            .get_sandbox(GetSandboxRequest {
+                name: (name).to_string(),
+                workspace_scope: Some(openshell_core::proto::workspace_selector(
+                    (workspace).to_string(),
+                )),
+            })
+            .await
+            .map_err(|status| miette!("provider detachment lookup failed ({})", status.code()))?
+            .into_inner()
+            .sandbox
+            .ok_or_else(|| miette::miette!("sandbox not found"))?;
 
-    let resource_version = sandbox.metadata.as_ref().map_or(0, |m| m.resource_version);
+        let resource_version = sandbox.metadata.as_ref().map_or(0, |m| m.resource_version);
 
-    let response = match client
-        .detach_sandbox_provider(DetachSandboxProviderRequest {
-            request_id: String::new(),
-            sandbox: (name).to_string(),
-            workspace_scope: Some(openshell_core::proto::workspace_selector(
-                (workspace).to_string(),
-            )),
-            provider: provider.to_string(),
-            expected_resource_version: resource_version,
-        })
-        .await
-    {
-        Ok(response) => response.into_inner(),
-        // Explicit post-save uncertainty takes precedence over a generic retry hint.
-        Err(status)
-            if status.code() == Code::Aborted && !provider_mutation_is_uncertain(&status) =>
+        match client
+            .detach_sandbox_provider(DetachSandboxProviderRequest {
+                request_id: String::new(),
+                sandbox: (name).to_string(),
+                workspace_scope: Some(openshell_core::proto::workspace_selector(
+                    (workspace).to_string(),
+                )),
+                provider: provider.to_string(),
+                expected_resource_version: resource_version,
+            })
+            .await
         {
-            return Err(miette::miette!(
-                "Failed to detach provider: sandbox was modified by another operation.\n\
-                 Please retry the command."
-            ));
+            Ok(response) => break (sandbox, response.into_inner()),
+            // Explicit post-save uncertainty takes precedence over a generic retry hint.
+            Err(status)
+                if status.code() == Code::Aborted && !provider_mutation_is_uncertain(&status) =>
+            {
+                if attempt < PROVIDER_ATTACHMENT_CAS_ATTEMPTS {
+                    attempt += 1;
+                    continue;
+                }
+                return Err(miette::miette!(
+                    "Failed to detach provider: sandbox was modified by another operation.\n\
+                     Please retry the command."
+                ));
+            }
+            Err(error) => return Err(provider_mutation_error(&error, "detachment")),
         }
-        Err(error) => return Err(provider_mutation_error(&error, "detachment")),
     };
 
     let receipt = response.receipt.ok_or_else(|| miette!("gateway did not return a provider receipt; saved detachment cannot establish revocation"))?;

crates/openshell-cli/tests/provider_commands_integration.rs

@@ -110,6 +110,8 @@ struct ProviderState {
     readiness_sequence: Arc<AtomicU64>,
     corrupt_mutation_receipt: Arc<Mutex<Option<ReceiptCorruption>>>,
     fail_mutation_after_save: Arc<Mutex<Option<Status>>>,
+    /// Plain resource-version conflicts to return before an attachment saves.
+    sandbox_provider_conflicts: Arc<AtomicU64>,
     global_settings: Arc<Mutex<HashMap<String, SettingValue>>>,
 }
 
@@ -428,6 +430,18 @@ impl OpenShell for TestOpenShell {
                 sandbox_name: sandbox_name.clone(),
                 provider: request.provider.clone(),
             });
+        if self
+            .state
+            .sandbox_provider_conflicts
+            .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |left| {
+                left.checked_sub(1)
+            })
+            .is_ok()
+        {
+            return Err(Status::aborted(
+                "sandbox was modified concurrently (current resource_version: 2)",
+            ));
+        }
         if !self
             .state
             .providers
@@ -490,6 +504,18 @@ impl OpenShell for TestOpenShell {
                 sandbox_name: sandbox_name.clone(),
                 provider: request.provider.clone(),
             });
+        if self
+            .state
+            .sandbox_provider_conflicts
+            .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |left| {
+                left.checked_sub(1)
+            })
+            .is_ok()
+        {
+            return Err(Status::aborted(
+                "sandbox was modified concurrently (current resource_version: 2)",
+            ));
+        }
         let mut sandbox_providers = self.state.sandbox_providers.lock().await;
         let providers = sandbox_providers.entry(sandbox_name.clone()).or_default();
         let before_len = providers.len();
@@ -6035,3 +6061,95 @@ async fn provider_create_from_gcloud_adc_missing_client_secret() {
         "no provider must be created when ADC validation fails"
     );
 }
+
+#[tokio::test]
+async fn provider_attachment_retries_resource_version_conflicts() {
+    let server = run_server().await;
+    seed_readiness_provider(&server).await;
+    for action in ["attach", "detach"] {
+        let sandbox_name = "conflicted";
+        server.state.sandbox_providers.lock().await.insert(
+            sandbox_name.to_string(),
+            if action == "attach" {
+                Vec::new()
+            } else {
+                vec![READINESS_PROVIDER.to_string()]
+            },
+        );
+        server.state.sandbox_provider_requests.lock().await.clear();
+        server
+            .state
+            .sandbox_provider_conflicts
+            .store(2, Ordering::SeqCst);
+        let output = run_readiness_cli(
+            &server,
+            &[
+                "sandbox",
+                "provider",
+                action,
+                sandbox_name,
+                READINESS_PROVIDER,
+            ],
+        )
+        .await;
+        assert!(
+            output.status.success(),
+            "{action}: {}",
+            String::from_utf8_lossy(&output.stderr)
+        );
+        assert_eq!(
+            server.state.sandbox_provider_requests.lock().await.len(),
+            3,
+            "{action} retried each conflict once"
+        );
+        assert_eq!(
+            server
+                .state
+                .sandbox_providers
+                .lock()
+                .await
+                .get(sandbox_name)
+                .unwrap()
+                .contains(&READINESS_PROVIDER.to_string()),
+            action == "attach"
+        );
+    }
+}
+
+#[tokio::test]
+async fn provider_attachment_reports_persistent_conflicts() {
+    let server = run_server().await;
+    seed_readiness_provider(&server).await;
+    server
+        .state
+        .sandbox_providers
+        .lock()
+        .await
+        .insert("contended".to_string(), Vec::new());
+    server
+        .state
+        .sandbox_provider_conflicts
+        .store(u64::MAX, Ordering::SeqCst);
+    let output = run_readiness_cli(
+        &server,
+        &[
+            "sandbox",
+            "provider",
+            "attach",
+            "contended",
+            READINESS_PROVIDER,
+        ],
+    )
+    .await;
+    assert!(!output.status.success());
+    assert!(
+        String::from_utf8_lossy(&output.stderr).contains("Please retry the command"),
+        "{}",
+        String::from_utf8_lossy(&output.stderr)
+    );
+    assert_eq!(server.state.sandbox_provider_requests.lock().await.len(), 5);
+    server
+        .state
+        .sandbox_provider_conflicts
+        .store(0, Ordering::SeqCst);
+}