fix(config): address durable operation review findings

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

Author: pimlock

crates/openshell-cli/src/main.rs

@@ -2844,7 +2844,7 @@ async fn run_async() -> Result<()> {
                         .await?;
                     } else {
                         let name = resolve_sandbox_name(name, &ctx.name, &cli.workspace)?;
-                        run::sandbox_policy_set(
+                        let exit_code = run::sandbox_policy_set(
                             &ctx.endpoint,
                             &name,
                             &policy,
@@ -2854,6 +2854,9 @@ async fn run_async() -> Result<()> {
                             &tls,
                         )
                         .await?;
+                        if exit_code != 0 {
+                            std::process::exit(exit_code);
+                        }
                     }
                 }
                 PolicyCommands::Update {
@@ -2872,7 +2875,7 @@ async fn run_async() -> Result<()> {
                     timeout,
                 } => {
                     let name = resolve_sandbox_name(name, &ctx.name, &cli.workspace)?;
-                    run::sandbox_policy_update(
+                    let exit_code = run::sandbox_policy_update(
                         &ctx.endpoint,
                         &name,
                         &add_endpoints,
@@ -2891,6 +2894,9 @@ async fn run_async() -> Result<()> {
                         &tls,
                     )
                     .await?;
+                    if exit_code != 0 {
+                        std::process::exit(exit_code);
+                    }
                 }
                 PolicyCommands::Get {
                     name,

crates/openshell-cli/src/run.rs

@@ -73,6 +73,30 @@ use std::time::{Duration, Instant};
 use tonic::{Code, Status};
 
 const PROVISIONAL_CONTAINER_EXIT_RECONCILIATION_TIMEOUT: Duration = Duration::from_secs(5);
+const POLICY_WAIT_TIMEOUT_EXIT_CODE: i32 = 124;
+
+fn report_policy_wait_timeout(status: &Status) -> Option<i32> {
+    if status.code() != Code::DeadlineExceeded {
+        return None;
+    }
+    let operation_id = status
+        .metadata()
+        .get("operation-id")
+        .and_then(|value| value.to_str().ok());
+    if let Some(operation_id) = operation_id {
+        eprintln!(
+            "{} Timeout waiting for policy update operation {}; update remains committed",
+            "✗".red().bold(),
+            operation_id
+        );
+    } else {
+        eprintln!(
+            "{} Timeout waiting for policy update; update remains committed",
+            "✗".red().bold()
+        );
+    }
+    Some(POLICY_WAIT_TIMEOUT_EXIT_CODE)
+}
 
 fn report_config_update_operation(
     operation: Option<&ConfigUpdateOperation>,
@@ -4976,7 +5000,7 @@ pub async fn sandbox_policy_set(
     timeout_secs: u64,
     workspace: &str,
     tls: &TlsOptions,
-) -> Result<()> {
+) -> Result<i32> {
     let policy = load_sandbox_policy(Some(policy_path))?
         .ok_or_else(|| miette::miette!("No policy loaded from {policy_path}"))?;
 
@@ -4997,7 +5021,7 @@ pub async fn sandbox_policy_set(
         .and_then(|r| r.into_inner().revision)
         .map_or(0, |r| r.version);
 
-    let response = client
+    let response = match client
         .update_config(UpdateConfigRequest {
             sandbox: name.to_string(),
             workspace_scope: Some(openshell_core::proto::workspace_selector(
@@ -5016,7 +5040,16 @@ pub async fn sandbox_policy_set(
             ..Default::default()
         })
         .await
-        .into_diagnostic()?;
+    {
+        Ok(response) => response,
+        Err(status) if wait => {
+            if let Some(exit_code) = report_policy_wait_timeout(&status) {
+                return Ok(exit_code);
+            }
+            return Err(status).into_diagnostic();
+        }
+        Err(status) => return Err(status).into_diagnostic(),
+    };
 
     let resp = response.into_inner();
 
@@ -5027,7 +5060,7 @@ pub async fn sandbox_policy_set(
             resp.version,
             &resp.policy_hash[..12]
         );
-        return Ok(());
+        return Ok(0);
     }
 
     eprintln!(
@@ -5038,10 +5071,11 @@ pub async fn sandbox_policy_set(
     );
 
     if !wait {
-        return Ok(());
+        return Ok(0);
     }
 
-    report_config_update_operation(resp.operation.as_ref(), resp.version)
+    report_config_update_operation(resp.operation.as_ref(), resp.version)?;
+    Ok(0)
 }
 
 /// Preview or atomically submit explicitly scoped incremental policy operations.
@@ -5063,7 +5097,7 @@ pub async fn sandbox_policy_update(
     timeout_secs: u64,
     workspace: &str,
     tls: &TlsOptions,
-) -> Result<()> {
+) -> Result<i32> {
     if dry_run && wait {
         return Err(miette!("--wait cannot be combined with --dry-run"));
     }
@@ -5112,12 +5146,12 @@ pub async fn sandbox_policy_update(
         );
         print_policy_merge_warnings(&merged.warnings);
         print_sandbox_policy(&merged.policy);
-        return Ok(());
+        return Ok(0);
     }
 
     let current_version = current.version;
     let current_hash = current.policy_hash.clone();
-    let response = client
+    let response = match client
         .update_config(UpdateConfigRequest {
             sandbox: name.to_string(),
             workspace_scope: Some(openshell_core::proto::workspace_selector(
@@ -5136,8 +5170,16 @@ pub async fn sandbox_policy_update(
             ..Default::default()
         })
         .await
-        .into_diagnostic()?
-        .into_inner();
+    {
+        Ok(response) => response.into_inner(),
+        Err(status) if wait => {
+            if let Some(exit_code) = report_policy_wait_timeout(&status) {
+                return Ok(exit_code);
+            }
+            return Err(status).into_diagnostic();
+        }
+        Err(status) => return Err(status).into_diagnostic(),
+    };
 
     print_policy_merge_warnings(&merged.warnings);
 
@@ -5148,7 +5190,7 @@ pub async fn sandbox_policy_update(
             response.version,
             short_hash(&response.policy_hash)
         );
-        return Ok(());
+        return Ok(0);
     }
 
     eprintln!(
@@ -5159,10 +5201,11 @@ pub async fn sandbox_policy_update(
     );
 
     if !wait {
-        return Ok(());
+        return Ok(0);
     }
 
-    report_config_update_operation(response.operation.as_ref(), response.version)
+    report_config_update_operation(response.operation.as_ref(), response.version)?;
+    Ok(0)
 }
 
 pub async fn sandbox_policy_get(

crates/openshell-cli/tests/sandbox_name_fallback_integration.rs

@@ -28,6 +28,7 @@ use openshell_core::proto::{
     ServiceStatus, SupervisorMessage, UpdateProviderRequest, WatchSandboxRequest,
 };
 use std::sync::Arc;
+use std::sync::atomic::{AtomicBool, Ordering};
 use tempfile::TempDir;
 use tokio::net::TcpListener;
 use tokio::sync::{Mutex, mpsc};
@@ -41,6 +42,7 @@ use tonic::{Response, Status};
 #[derive(Clone, Default)]
 struct SandboxState {
     last_get_name: Arc<Mutex<Option<String>>>,
+    timeout_config_updates: Arc<AtomicBool>,
 }
 
 #[derive(Clone, Default)]
@@ -488,6 +490,13 @@ impl OpenShell for TestOpenShell {
         &self,
         _request: tonic::Request<openshell_core::proto::UpdateConfigRequest>,
     ) -> Result<Response<openshell_core::proto::UpdateConfigResponse>, Status> {
+        if self.state.timeout_config_updates.load(Ordering::Relaxed) {
+            let mut status = Status::deadline_exceeded("update remains pending");
+            status
+                .metadata_mut()
+                .insert("operation-id", "operation-timeout-123".parse().unwrap());
+            return Err(status);
+        }
         Err(Status::unimplemented("not implemented in test"))
     }
 
@@ -497,7 +506,7 @@ impl OpenShell for TestOpenShell {
     ) -> Result<Response<GetSandboxPolicyStatusResponse>, Status> {
         let req = request.into_inner();
         assert_eq!(req.sandbox, "my-sandbox");
-        assert_eq!(req.version, 3);
+        assert!(matches!(req.version, 0 | 3));
         assert!(!req.global);
 
         let policy = SandboxPolicy {
@@ -729,7 +738,7 @@ struct TestServer {
     endpoint: String,
     tls: TlsOptions,
     openshell: TestOpenShell,
-    _dir: TempDir,
+    dir: TempDir,
 }
 
 async fn run_server() -> TestServer {
@@ -776,7 +785,7 @@ async fn run_server() -> TestServer {
         endpoint,
         tls,
         openshell,
-        _dir: dir,
+        dir,
     }
 }
 
@@ -1016,3 +1025,65 @@ async fn explicit_name_takes_precedence_over_persisted() {
         "explicit name should be used, not the persisted one"
     );
 }
+
+#[tokio::test(flavor = "multi_thread")]
+async fn policy_wait_timeouts_exit_124_for_set_and_update() {
+    let ts = run_server().await;
+    ts.openshell
+        .state
+        .timeout_config_updates
+        .store(true, Ordering::Relaxed);
+
+    let config_dir = tempfile::tempdir().unwrap();
+    let mtls_dir = config_dir
+        .path()
+        .join("openshell/gateways/timeout-test/mtls");
+    std::fs::create_dir_all(&mtls_dir).unwrap();
+    for name in ["ca.crt", "tls.crt", "tls.key"] {
+        std::fs::copy(ts.dir.path().join(name), mtls_dir.join(name)).unwrap();
+    }
+    let policy_dir = tempfile::tempdir().unwrap();
+    let policy_path = policy_dir.path().join("policy.yaml");
+    std::fs::write(&policy_path, "version: 1\n").unwrap();
+
+    let common = [
+        "--gateway",
+        "timeout-test",
+        "--gateway-endpoint",
+        ts.endpoint.as_str(),
+        "policy",
+    ];
+    let commands = [
+        vec![
+            "set",
+            "my-sandbox",
+            "--policy",
+            policy_path.to_str().unwrap(),
+            "--wait",
+            "--timeout",
+            "1",
+        ],
+        vec![
+            "update",
+            "my-sandbox",
+            "--add-endpoint",
+            "api.example.com:443",
+            "--wait",
+            "--timeout",
+            "1",
+        ],
+    ];
+
+    for args in commands {
+        let output = std::process::Command::new(env!("CARGO_BIN_EXE_openshell"))
+            .args(common)
+            .args(args)
+            .env("XDG_CONFIG_HOME", config_dir.path())
+            .output()
+            .unwrap();
+        assert_eq!(output.status.code(), Some(124), "{output:?}");
+        let stderr = String::from_utf8_lossy(&output.stderr);
+        assert!(stderr.contains("operation-timeout-123"), "{stderr}");
+        assert!(stderr.contains("remains committed"), "{stderr}");
+    }
+}