From 8334767af13151980744558f244daf022a949fea Mon Sep 17 00:00:00 2001 From: Derek Carr Date: Wed, 20 May 2026 11:04:00 -0400 Subject: [PATCH 1/2] refactor(proto): move phase and current_policy_version into SandboxStatus Move phase and current_policy_version from SandboxSpec into SandboxStatus to correctly model mutable runtime state. Update all callers in the gateway server, TUI, and Python SDK to read and write these fields through SandboxStatus accessors. Signed-off-by: Derek Carr --- crates/openshell-cli/src/run.rs | 32 +++-- .../tests/provider_commands_integration.rs | 2 - .../sandbox_create_lifecycle_integration.rs | 64 +++++----- crates/openshell-core/src/metadata.rs | 22 +++- crates/openshell-server/src/compute/mod.rs | 120 +++++++++--------- crates/openshell-server/src/grpc/auth_rpc.rs | 4 +- crates/openshell-server/src/grpc/policy.rs | 83 ++++++------ crates/openshell-server/src/grpc/provider.rs | 10 +- crates/openshell-server/src/grpc/sandbox.rs | 34 ++--- crates/openshell-server/src/grpc/service.rs | 29 ++--- .../openshell-server/src/persistence/tests.rs | 14 +- .../openshell-server/src/service_routing.rs | 2 +- crates/openshell-tui/src/lib.rs | 21 +-- proto/openshell.proto | 14 +- python/openshell/__init__.py | 2 + python/openshell/sandbox.py | 26 +++- 16 files changed, 258 insertions(+), 221 deletions(-) diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index f1a44ad31..c6489e21a 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -1842,11 +1842,11 @@ pub async fn sandbox_create( .into_diagnostic()? .into_inner(); - let mut last_phase = sandbox.phase; + let mut last_phase = sandbox.phase(); let mut last_error_reason = String::new(); let mut last_condition_message = ready_false_condition_message(sandbox.status.as_ref()); // Track whether we have seen a non-Ready phase during the watch. - let mut saw_non_ready = SandboxPhase::try_from(sandbox.phase) != Ok(SandboxPhase::Ready); + let mut saw_non_ready = SandboxPhase::try_from(sandbox.phase()) != Ok(SandboxPhase::Ready); let provision_timeout = Duration::from_secs( std::env::var("OPENSHELL_PROVISION_TIMEOUT") .ok() @@ -1899,8 +1899,8 @@ pub async fn sandbox_create( let evt = item.into_diagnostic()?; match evt.payload { Some(openshell_core::proto::sandbox_stream_event::Payload::Sandbox(s)) => { - let phase = SandboxPhase::try_from(s.phase).unwrap_or(SandboxPhase::Unknown); - last_phase = s.phase; + let phase = SandboxPhase::try_from(s.phase()).unwrap_or(SandboxPhase::Unknown); + last_phase = s.phase(); if let Some(message) = ready_false_condition_message(s.status.as_ref()) { last_condition_message = Some(message); } @@ -2451,7 +2451,7 @@ pub async fn sandbox_get( }; println!(" {} {}", "Id:".dimmed(), id); println!(" {} {}", "Name:".dimmed(), name); - println!(" {} {}", "Phase:".dimmed(), phase_name(sandbox.phase)); + println!(" {} {}", "Phase:".dimmed(), phase_name(sandbox.phase())); println!( " {} {}", "Resource version:".dimmed(), @@ -2535,11 +2535,11 @@ pub async fn sandbox_exec_grpc( .ok_or_else(|| miette::miette!("sandbox not found"))?; // Verify the sandbox is ready before issuing the exec. - if SandboxPhase::try_from(sandbox.phase) != Ok(SandboxPhase::Ready) { + if SandboxPhase::try_from(sandbox.phase()) != Ok(SandboxPhase::Ready) { return Err(miette::miette!( "sandbox '{}' is not ready (phase: {}); wait for it to reach Ready state", name, - phase_name(sandbox.phase) + phase_name(sandbox.phase()) )); } @@ -2747,11 +2747,11 @@ async fn fetch_ready_sandbox_for_forward( .sandbox .ok_or_else(|| miette::miette!("sandbox '{name}' not found"))?; - if SandboxPhase::try_from(sandbox.phase) != Ok(SandboxPhase::Ready) { + if SandboxPhase::try_from(sandbox.phase()) != Ok(SandboxPhase::Ready) { return Err(miette::miette!( "sandbox '{}' is no longer ready (phase: {}); stopping service forward", name, - phase_name(sandbox.phase) + phase_name(sandbox.phase()) )); } @@ -3195,8 +3195,8 @@ pub async fn sandbox_list( // Print rows for sandbox in sandboxes { - let phase = phase_name(sandbox.phase); - let phase_colored = match SandboxPhase::try_from(sandbox.phase) { + let phase = phase_name(sandbox.phase()); + let phase_colored = match SandboxPhase::try_from(sandbox.phase()) { Ok(SandboxPhase::Ready) => phase.green().to_string(), Ok(SandboxPhase::Error) => phase.red().to_string(), Ok(SandboxPhase::Provisioning) => phase.yellow().to_string(), @@ -3224,8 +3224,8 @@ fn sandbox_to_json(sandbox: &Sandbox) -> serde_json::Value { "labels": labels, "resource_version": meta.map_or(0, |m| m.resource_version), "created_at": format_epoch_ms(meta.map_or(0, |m| m.created_at_ms)), - "phase": phase_name(sandbox.phase), - "current_policy_version": sandbox.current_policy_version, + "phase": phase_name(sandbox.phase()), + "current_policy_version": sandbox.current_policy_version(), }) } @@ -7596,8 +7596,6 @@ mod tests { let status = SandboxStatus { sandbox_name: "gpu".to_string(), agent_pod: "gpu-pod".to_string(), - agent_fd: String::new(), - sandbox_fd: String::new(), conditions: vec![SandboxCondition { r#type: "Ready".to_string(), status: "False".to_string(), @@ -7605,6 +7603,7 @@ mod tests { message: "Another GPU sandbox may already be using the available GPU.".to_string(), last_transition_time: String::new(), }], + ..Default::default() }; assert_eq!( @@ -7618,8 +7617,6 @@ mod tests { let status = SandboxStatus { sandbox_name: "gpu".to_string(), agent_pod: "gpu-pod".to_string(), - agent_fd: String::new(), - sandbox_fd: String::new(), conditions: vec![SandboxCondition { r#type: "Scheduled".to_string(), status: "True".to_string(), @@ -7627,6 +7624,7 @@ mod tests { message: "Sandbox scheduled".to_string(), last_transition_time: String::new(), }], + ..Default::default() }; assert!(ready_false_condition_message(Some(&status)).is_none()); diff --git a/crates/openshell-cli/tests/provider_commands_integration.rs b/crates/openshell-cli/tests/provider_commands_integration.rs index 090097a20..02b69dc65 100644 --- a/crates/openshell-cli/tests/provider_commands_integration.rs +++ b/crates/openshell-cli/tests/provider_commands_integration.rs @@ -126,8 +126,6 @@ impl OpenShell for TestOpenShell { }), spec: None, status: None, - phase: 0, - current_policy_version: 0, }), })) } diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index 3ed43b2fc..f51d5e3c9 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -79,18 +79,19 @@ impl OpenShell for TestOpenShell { name }; - Ok(Response::new(SandboxResponse { - sandbox: Some(Sandbox { - metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { - id: format!("id-{sandbox_name}"), - name: sandbox_name, - created_at_ms: 0, - labels: HashMap::new(), - resource_version: 0, - }), - phase: SandboxPhase::Provisioning as i32, - ..Sandbox::default() + let mut sandbox = Sandbox { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: format!("id-{sandbox_name}"), + name: sandbox_name, + created_at_ms: 0, + labels: HashMap::new(), + resource_version: 0, }), + ..Sandbox::default() + }; + sandbox.set_phase(SandboxPhase::Provisioning as i32); + Ok(Response::new(SandboxResponse { + sandbox: Some(sandbox), })) } @@ -99,18 +100,19 @@ impl OpenShell for TestOpenShell { request: tonic::Request, ) -> Result, Status> { let name = request.into_inner().name; - Ok(Response::new(SandboxResponse { - sandbox: Some(Sandbox { - metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { - id: format!("id-{name}"), - name, - created_at_ms: 0, - labels: HashMap::new(), - resource_version: 0, - }), - phase: SandboxPhase::Ready as i32, - ..Sandbox::default() + let mut sandbox = Sandbox { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: format!("id-{name}"), + name, + created_at_ms: 0, + labels: HashMap::new(), + resource_version: 0, }), + ..Sandbox::default() + }; + sandbox.set_phase(SandboxPhase::Ready as i32); + Ok(Response::new(SandboxResponse { + sandbox: Some(sandbox), })) } @@ -351,7 +353,7 @@ impl OpenShell for TestOpenShell { let vm_log_churn_before_ready = self.state.vm_log_churn_before_ready.load(Ordering::SeqCst); tokio::spawn(async move { - let provisioning = Sandbox { + let mut provisioning = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: sandbox_id.clone(), name: sandbox_id.trim_start_matches("id-").to_string(), @@ -359,16 +361,12 @@ impl OpenShell for TestOpenShell { labels: HashMap::new(), resource_version: 0, }), - phase: SandboxPhase::Provisioning as i32, ..Sandbox::default() }; - let error = Sandbox { - phase: SandboxPhase::Error as i32, + provisioning.set_phase(SandboxPhase::Provisioning as i32); + let mut error = Sandbox { status: Some(SandboxStatus { sandbox_name: sandbox_id.trim_start_matches("id-").to_string(), - agent_pod: String::new(), - agent_fd: String::new(), - sandbox_fd: String::new(), conditions: vec![SandboxCondition { r#type: "Ready".to_string(), status: "False".to_string(), @@ -376,13 +374,13 @@ impl OpenShell for TestOpenShell { message: "VM process exited with status 0".to_string(), last_transition_time: String::new(), }], + ..Default::default() }), ..provisioning.clone() }; - let ready = Sandbox { - phase: SandboxPhase::Ready as i32, - ..provisioning.clone() - }; + error.set_phase(SandboxPhase::Error as i32); + let mut ready = provisioning.clone(); + ready.set_phase(SandboxPhase::Ready as i32); let _ = tx .send(Ok(SandboxStreamEvent { diff --git a/crates/openshell-core/src/metadata.rs b/crates/openshell-core/src/metadata.rs index 78533e1e0..af26f73ae 100644 --- a/crates/openshell-core/src/metadata.rs +++ b/crates/openshell-core/src/metadata.rs @@ -6,7 +6,7 @@ //! These traits provide uniform access to `ObjectMeta` fields across all resource types. use crate::proto::{ - InferenceRoute, ObjectForTest, Provider, Sandbox, ServiceEndpoint, SshSession, + InferenceRoute, ObjectForTest, Provider, Sandbox, SandboxStatus, ServiceEndpoint, SshSession, StoredProviderCredentialRefreshState, StoredProviderProfile, }; use std::collections::HashMap; @@ -69,6 +69,26 @@ impl GetResourceVersion for Sandbox { } } +impl Sandbox { + pub fn phase(&self) -> i32 { + self.status.as_ref().map_or(0, |s| s.phase) + } + + pub fn set_phase(&mut self, phase: i32) { + self.status.get_or_insert_with(SandboxStatus::default).phase = phase; + } + + pub fn current_policy_version(&self) -> u32 { + self.status.as_ref().map_or(0, |s| s.current_policy_version) + } + + pub fn set_current_policy_version(&mut self, version: u32) { + self.status + .get_or_insert_with(SandboxStatus::default) + .current_policy_version = version; + } +} + // Implementations for Provider impl ObjectId for Provider { fn object_id(&self) -> &str { diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index 98dc3fd63..6036f8857 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -525,7 +525,7 @@ impl ComputeRuntime { let sandbox = self .store .update_message_cas::(&id, 0, |s| { - s.phase = SandboxPhase::Deleting as i32; + s.set_phase(SandboxPhase::Deleting as i32); }) .await .map_err(|e| { @@ -608,7 +608,7 @@ impl ComputeRuntime { } }; - let phase = SandboxPhase::try_from(sandbox.phase).unwrap_or(SandboxPhase::Unknown); + let phase = SandboxPhase::try_from(sandbox.phase()).unwrap_or(SandboxPhase::Unknown); if !sandbox_phase_should_be_running(phase) { continue; } @@ -682,7 +682,7 @@ impl ComputeRuntime { match self .store .update_message_cas::(&sandbox_id, 0, |s| { - s.phase = SandboxPhase::Error as i32; + s.set_phase(SandboxPhase::Error as i32); let name = s.object_name().to_string(); upsert_ready_condition( &mut s.status, @@ -860,21 +860,25 @@ impl ComputeRuntime { use crate::persistence::WriteCondition; let now_ms = openshell_core::time::now_ms(); - let mut status = incoming.status.as_ref().map(public_status_from_driver); - rewrite_user_facing_conditions(&mut status, None); - let session_connected = self.supervisor_sessions.has_session(&incoming.id); let mut phase = derive_phase(incoming.status.as_ref()); - let sandbox_name = incoming.name.clone(); - if session_connected - && matches!(phase, SandboxPhase::Provisioning | SandboxPhase::Unknown) - { - ensure_supervisor_ready_status(&mut status, &sandbox_name); + + let supervisor_promoted = session_connected + && matches!(phase, SandboxPhase::Provisioning | SandboxPhase::Unknown); + if supervisor_promoted { phase = SandboxPhase::Ready; } - let sandbox = Sandbox { + let mut status = incoming + .status + .as_ref() + .map(|s| public_status_from_driver(s, phase, 0)); + rewrite_user_facing_conditions(&mut status, None); + if supervisor_promoted { + ensure_supervisor_ready_status(&mut status, &sandbox_name); + } + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: incoming.id.clone(), name: sandbox_name, @@ -884,9 +888,8 @@ impl ComputeRuntime { }), spec: None, status, - phase: phase as i32, - current_policy_version: 0, }; + sandbox.set_phase(phase as i32); self.store .put_if( @@ -921,19 +924,25 @@ impl ComputeRuntime { let sandbox = self .store .update_message_cas::(&incoming.id, 0, |sandbox| { - let mut status = incoming.status.as_ref().map(public_status_from_driver); - rewrite_user_facing_conditions(&mut status, sandbox.spec.as_ref()); - let mut phase = derive_phase(incoming.status.as_ref()); - if session_connected - && matches!(phase, SandboxPhase::Provisioning | SandboxPhase::Unknown) - { - ensure_supervisor_ready_status(&mut status, &sandbox_name); + let supervisor_promoted = session_connected + && matches!(phase, SandboxPhase::Provisioning | SandboxPhase::Unknown); + if supervisor_promoted { phase = SandboxPhase::Ready; } + let cpv = sandbox.current_policy_version(); + let mut status = incoming + .status + .as_ref() + .map(|s| public_status_from_driver(s, phase, cpv)); + rewrite_user_facing_conditions(&mut status, sandbox.spec.as_ref()); + if supervisor_promoted { + ensure_supervisor_ready_status(&mut status, &sandbox_name); + } + let old_phase = - SandboxPhase::try_from(sandbox.phase).unwrap_or(SandboxPhase::Unknown); + SandboxPhase::try_from(sandbox.phase()).unwrap_or(SandboxPhase::Unknown); if old_phase != phase { info!( sandbox_id = %incoming.id, @@ -967,9 +976,8 @@ impl ComputeRuntime { if let Some(metadata) = sandbox.metadata.as_mut() { metadata.name.clone_from(&sandbox_name); } - // Note: namespace field removed from public Sandbox API - it remains internal to DriverSandbox sandbox.status = status; - sandbox.phase = phase as i32; + sandbox.set_phase(phase as i32); }) .await .map_err(|e| match e { @@ -1008,7 +1016,7 @@ impl ComputeRuntime { .store .update_message_cas::(sandbox_id, 0, |sandbox| { let current_phase = - SandboxPhase::try_from(sandbox.phase).unwrap_or(SandboxPhase::Unknown); + SandboxPhase::try_from(sandbox.phase()).unwrap_or(SandboxPhase::Unknown); // Skip if sandbox is in terminal state if current_phase == SandboxPhase::Deleting || current_phase == SandboxPhase::Error { @@ -1018,10 +1026,10 @@ impl ComputeRuntime { let sandbox_name = sandbox.object_name().to_string(); if connected { ensure_supervisor_ready_status(&mut sandbox.status, &sandbox_name); - sandbox.phase = SandboxPhase::Ready as i32; + sandbox.set_phase(SandboxPhase::Ready as i32); } else if current_phase == SandboxPhase::Ready { ensure_supervisor_not_ready_status(&mut sandbox.status, &sandbox_name); - sandbox.phase = SandboxPhase::Provisioning as i32; + sandbox.set_phase(SandboxPhase::Provisioning as i32); } }) .await; @@ -1221,10 +1229,7 @@ fn driver_sandbox_from_public(sandbox: &Sandbox) -> DriverSandbox { name: sandbox.object_name().to_string(), namespace: String::new(), // Namespace is set by the driver based on its config spec: sandbox.spec.as_ref().map(driver_sandbox_spec_from_public), - status: sandbox - .status - .as_ref() - .map(|status| driver_status_from_public(status, sandbox.phase)), + status: sandbox.status.as_ref().map(driver_status_from_public), } } @@ -1431,7 +1436,7 @@ fn build_platform_resources_config( } } -fn driver_status_from_public(status: &SandboxStatus, phase: i32) -> DriverSandboxStatus { +fn driver_status_from_public(status: &SandboxStatus) -> DriverSandboxStatus { DriverSandboxStatus { sandbox_name: status.sandbox_name.clone(), instance_id: status.agent_pod.clone(), @@ -1442,7 +1447,7 @@ fn driver_status_from_public(status: &SandboxStatus, phase: i32) -> DriverSandbo .iter() .map(driver_condition_from_public) .collect(), - deleting: SandboxPhase::try_from(phase) == Ok(SandboxPhase::Deleting), + deleting: SandboxPhase::try_from(status.phase) == Ok(SandboxPhase::Deleting), } } @@ -1474,7 +1479,11 @@ fn decode_sandbox_record(record: &ObjectRecord) -> Result { Sandbox::decode(record.payload.as_slice()).map_err(|e| e.to_string()) } -fn public_status_from_driver(status: &DriverSandboxStatus) -> SandboxStatus { +fn public_status_from_driver( + status: &DriverSandboxStatus, + phase: SandboxPhase, + current_policy_version: u32, +) -> SandboxStatus { SandboxStatus { sandbox_name: status.sandbox_name.clone(), agent_pod: status.instance_id.clone(), @@ -1485,6 +1494,8 @@ fn public_status_from_driver(status: &DriverSandboxStatus) -> SandboxStatus { .iter() .map(public_condition_from_driver) .collect(), + phase: phase as i32, + current_policy_version, } } @@ -1523,10 +1534,7 @@ fn upsert_ready_condition( ) { let status = status.get_or_insert_with(|| SandboxStatus { sandbox_name: sandbox_name.to_string(), - agent_pod: String::new(), - agent_fd: String::new(), - sandbox_fd: String::new(), - conditions: Vec::new(), + ..Default::default() }); if let Some(existing) = status @@ -1923,7 +1931,7 @@ mod tests { } fn sandbox_record(id: &str, name: &str, phase: SandboxPhase) -> Sandbox { - Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: id.to_string(), name: name.to_string(), @@ -1931,9 +1939,10 @@ mod tests { labels: HashMap::new(), resource_version: 0, }), - phase: phase as i32, ..Default::default() - } + }; + sandbox.set_phase(phase as i32); + sandbox } fn ssh_session_record(id: &str, sandbox_id: &str) -> SshSession { @@ -2212,8 +2221,6 @@ mod tests { let mut status = Some(SandboxStatus { sandbox_name: "test".to_string(), agent_pod: "test-pod".to_string(), - agent_fd: String::new(), - sandbox_fd: String::new(), conditions: vec![SandboxCondition { r#type: "Ready".to_string(), status: "False".to_string(), @@ -2221,6 +2228,7 @@ mod tests { message: "0/1 nodes are available: 1 Insufficient nvidia.com/gpu.".to_string(), last_transition_time: String::new(), }], + ..Default::default() }); rewrite_user_facing_conditions( @@ -2244,8 +2252,6 @@ mod tests { let mut status = Some(SandboxStatus { sandbox_name: "test".to_string(), agent_pod: "test-pod".to_string(), - agent_fd: String::new(), - sandbox_fd: String::new(), conditions: vec![SandboxCondition { r#type: "Ready".to_string(), status: "False".to_string(), @@ -2253,6 +2259,7 @@ mod tests { message: original.to_string(), last_transition_time: String::new(), }], + ..Default::default() }); rewrite_user_facing_conditions( @@ -2316,7 +2323,7 @@ mod tests { .unwrap() .unwrap(); assert_eq!( - SandboxPhase::try_from(stored.phase).unwrap(), + SandboxPhase::try_from(stored.phase()).unwrap(), SandboxPhase::Ready ); } @@ -2350,7 +2357,7 @@ mod tests { .unwrap() .unwrap(); assert_eq!( - SandboxPhase::try_from(stored.phase).unwrap(), + SandboxPhase::try_from(stored.phase()).unwrap(), SandboxPhase::Ready ); let ready = stored @@ -2383,7 +2390,7 @@ mod tests { .unwrap() .unwrap(); assert_eq!( - SandboxPhase::try_from(stored.phase).unwrap(), + SandboxPhase::try_from(stored.phase()).unwrap(), SandboxPhase::Ready ); } @@ -2394,9 +2401,6 @@ mod tests { let mut sandbox = sandbox_record("sb-1", "sandbox-a", SandboxPhase::Ready); sandbox.status = Some(SandboxStatus { sandbox_name: "sandbox-a".to_string(), - agent_pod: String::new(), - agent_fd: String::new(), - sandbox_fd: String::new(), conditions: vec![SandboxCondition { r#type: "Ready".to_string(), status: "True".to_string(), @@ -2404,7 +2408,9 @@ mod tests { message: "Supervisor session connected".to_string(), last_transition_time: String::new(), }], + ..Default::default() }); + sandbox.set_phase(SandboxPhase::Ready as i32); runtime.store.put_message(&sandbox).await.unwrap(); runtime @@ -2419,7 +2425,7 @@ mod tests { .unwrap() .unwrap(); assert_eq!( - SandboxPhase::try_from(stored.phase).unwrap(), + SandboxPhase::try_from(stored.phase()).unwrap(), SandboxPhase::Provisioning ); let ready = stored @@ -2505,7 +2511,7 @@ mod tests { .unwrap() .unwrap(); assert_eq!( - SandboxPhase::try_from(stored.phase).unwrap(), + SandboxPhase::try_from(stored.phase()).unwrap(), SandboxPhase::Ready ); assert!(stored.spec.as_ref().is_some_and(|spec| spec.gpu)); @@ -2598,7 +2604,7 @@ mod tests { .unwrap() .unwrap(); assert_eq!( - SandboxPhase::try_from(stored.phase).unwrap(), + SandboxPhase::try_from(stored.phase()).unwrap(), SandboxPhase::Ready ); } @@ -2761,7 +2767,7 @@ mod tests { .unwrap() .unwrap(); assert_eq!( - SandboxPhase::try_from(stored.phase).unwrap(), + SandboxPhase::try_from(stored.phase()).unwrap(), SandboxPhase::Error ); let ready = stored @@ -2793,7 +2799,7 @@ mod tests { .unwrap() .unwrap(); assert_eq!( - SandboxPhase::try_from(stored.phase).unwrap(), + SandboxPhase::try_from(stored.phase()).unwrap(), SandboxPhase::Error ); let ready = stored @@ -2820,7 +2826,7 @@ mod tests { .unwrap() .unwrap(); assert_eq!( - SandboxPhase::try_from(stored.phase).unwrap(), + SandboxPhase::try_from(stored.phase()).unwrap(), SandboxPhase::Ready ); } diff --git a/crates/openshell-server/src/grpc/auth_rpc.rs b/crates/openshell-server/src/grpc/auth_rpc.rs index 8e98b1824..88c771bed 100644 --- a/crates/openshell-server/src/grpc/auth_rpc.rs +++ b/crates/openshell-server/src/grpc/auth_rpc.rs @@ -193,7 +193,7 @@ mod tests { } async fn insert_sandbox(state: &Arc, sandbox_id: &str) { - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(ObjectMeta { id: sandbox_id.to_string(), name: sandbox_id.to_string(), @@ -205,9 +205,9 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Ready as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Ready as i32); state.store.put_message(&sandbox).await.unwrap(); } diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index bdc96d862..2abc670fd 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -1166,7 +1166,7 @@ pub(super) async fn handle_get_sandbox_policy_status( .ok_or_else(|| Status::not_found("sandbox not found"))?; ( sandbox.object_id().to_string(), - sandbox.current_policy_version, + sandbox.current_policy_version(), ) }; @@ -1295,7 +1295,7 @@ pub(super) async fn handle_report_policy_status( state .store .update_message_cas::(&req.sandbox_id, 0, |sandbox| { - sandbox.current_policy_version = version_to_set; + sandbox.set_current_policy_version(version_to_set); }) .await .map_err(|e| super::persistence_error_to_status(e, "update current_policy_version"))?; @@ -3037,7 +3037,7 @@ mod tests { // Two sandboxes; the caller is principal of A, the request body // references B. for (id, name) in [("sb-a", "sandbox-a"), ("sb-b", "sandbox-b")] { - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: id.to_string(), name: name.to_string(), @@ -3049,9 +3049,9 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Provisioning as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Provisioning as i32); state.store.put_message(&sandbox).await.unwrap(); } let req = with_sandbox( @@ -3070,7 +3070,7 @@ mod tests { async fn same_sandbox_get_sandbox_config_allowed() { use openshell_core::proto::{SandboxPhase, SandboxSpec}; let state = test_server_state().await; - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-self".to_string(), name: "self".to_string(), @@ -3082,9 +3082,9 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Provisioning as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Provisioning as i32); state.store.put_message(&sandbox).await.unwrap(); let req = with_sandbox( Request::new(GetSandboxConfigRequest { @@ -3102,7 +3102,7 @@ mod tests { use openshell_core::proto::{SandboxPhase, SandboxSpec}; let state = test_server_state().await; for (id, name) in [("sb-a", "sandbox-a"), ("sb-b", "sandbox-b")] { - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: id.to_string(), name: name.to_string(), @@ -3114,9 +3114,9 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Provisioning as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Provisioning as i32); state.store.put_message(&sandbox).await.unwrap(); } let req = with_sandbox( @@ -3137,7 +3137,7 @@ mod tests { use openshell_core::proto::{SandboxPhase, SandboxSpec}; let state = test_server_state().await; for (id, name) in [("sb-a", "sandbox-a"), ("sb-b", "sandbox-b")] { - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: id.to_string(), name: name.to_string(), @@ -3149,9 +3149,9 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Provisioning as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Provisioning as i32); state.store.put_message(&sandbox).await.unwrap(); } let req = with_sandbox( @@ -3224,7 +3224,7 @@ mod tests { // RBAC was the user gate; the IDOR guard must NOT trip for users. use openshell_core::proto::{SandboxPhase, SandboxSpec}; let state = test_server_state().await; - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-x".to_string(), name: "x".to_string(), @@ -3236,9 +3236,9 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Provisioning as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Provisioning as i32); state.store.put_message(&sandbox).await.unwrap(); let req = with_user(Request::new(GetSandboxConfigRequest { sandbox_id: "sb-x".to_string(), @@ -3291,7 +3291,7 @@ mod tests { let store = test_store().await; - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-no-policy".to_string(), name: "no-policy-sandbox".to_string(), @@ -3303,9 +3303,9 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Provisioning as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Provisioning as i32); store.put_message(&sandbox).await.unwrap(); let loaded = store @@ -3360,7 +3360,7 @@ mod tests { ) -> Sandbox { use openshell_core::proto::{SandboxPhase, SandboxSpec}; - Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: id.to_string(), name: name.to_string(), @@ -3373,9 +3373,10 @@ mod tests { providers, ..Default::default() }), - phase: SandboxPhase::Ready as i32, ..Default::default() - } + }; + sandbox.set_phase(SandboxPhase::Ready as i32); + sandbox } async fn enable_providers_v2(state: &Arc) { @@ -4246,7 +4247,7 @@ mod tests { .collect(), ..Default::default() }; - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-global-profile".to_string(), name: "global-profile-sandbox".to_string(), @@ -4259,9 +4260,9 @@ mod tests { providers: vec!["work-github".to_string()], ..Default::default() }), - phase: SandboxPhase::Ready as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Ready as i32); state.store.put_message(&sandbox).await.unwrap(); let global_policy = SandboxPolicy { @@ -4335,7 +4336,7 @@ mod tests { let store = test_store().await; - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-backfill".to_string(), name: "backfill-sandbox".to_string(), @@ -4347,9 +4348,9 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Provisioning as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Provisioning as i32); store.put_message(&sandbox).await.unwrap(); let new_policy = ProtoSandboxPolicy { @@ -4397,7 +4398,7 @@ mod tests { }; let state = test_server_state().await; - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-draft-flow".to_string(), name: "draft-flow".to_string(), @@ -4409,9 +4410,9 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Ready as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Ready as i32); state.store.put_message(&sandbox).await.unwrap(); let sandbox_name = sandbox.object_name().to_string(); @@ -4608,7 +4609,7 @@ mod tests { let state = test_server_state().await; let sandbox_name = "agent-feedback-loop".to_string(); - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-feedback".to_string(), name: sandbox_name.clone(), @@ -4620,9 +4621,9 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Ready as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Ready as i32); state.store.put_message(&sandbox).await.unwrap(); let proposed_rule = NetworkPolicyRule { @@ -4706,7 +4707,7 @@ mod tests { let state = test_server_state().await; let sandbox_name = "redraft-loop".to_string(); - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-redraft".to_string(), name: sandbox_name.clone(), @@ -4718,9 +4719,9 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Ready as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Ready as i32); state.store.put_message(&sandbox).await.unwrap(); // Two proposals with the same host|port|binary (so the mechanistic @@ -4820,7 +4821,7 @@ mod tests { let state = test_server_state().await; let sandbox_name = "mechanistic-dedup".to_string(); - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-mech-dedup".to_string(), name: sandbox_name.clone(), @@ -4832,9 +4833,9 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Ready as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Ready as i32); state.store.put_message(&sandbox).await.unwrap(); let proposed_rule = NetworkPolicyRule { @@ -4920,7 +4921,7 @@ mod tests { let state = test_server_state().await; let sandbox_name = "undo-clears-reason".to_string(); - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-undo-clears".to_string(), name: sandbox_name.clone(), @@ -4932,9 +4933,9 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Ready as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Ready as i32); state.store.put_message(&sandbox).await.unwrap(); let proposed_rule = NetworkPolicyRule { @@ -5026,7 +5027,7 @@ mod tests { use openshell_core::proto::{NetworkBinary, NetworkEndpoint, SandboxPhase, SandboxSpec}; let state = test_server_state().await; - let sandbox_a = Sandbox { + let mut sandbox_a = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-draft-owner".to_string(), name: "draft-owner".to_string(), @@ -5038,10 +5039,10 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Ready as i32, ..Default::default() }; - let sandbox_b = Sandbox { + sandbox_a.set_phase(SandboxPhase::Ready as i32); + let mut sandbox_b = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-draft-other".to_string(), name: "draft-other".to_string(), @@ -5053,9 +5054,9 @@ mod tests { policy: None, ..Default::default() }), - phase: SandboxPhase::Ready as i32, ..Default::default() }; + sandbox_b.set_phase(SandboxPhase::Ready as i32); state.store.put_message(&sandbox_a).await.unwrap(); state.store.put_message(&sandbox_b).await.unwrap(); @@ -6449,7 +6450,7 @@ mod tests { // Create a sandbox WITHOUT a policy (spec.policy = None) // This simulates a sandbox before the supervisor has discovered and synced a policy - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-1".to_string(), name: "test-sandbox".to_string(), @@ -6462,9 +6463,9 @@ mod tests { providers: Vec::new(), ..Default::default() }), - phase: SandboxPhase::Provisioning as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Provisioning as i32); state.store.put_message(&sandbox).await.unwrap(); // Fetch the sandbox to get its current resource_version @@ -6524,7 +6525,7 @@ mod tests { let state = test_server_state().await; // Create a sandbox WITHOUT a policy - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-1".to_string(), name: "test-sandbox".to_string(), @@ -6537,9 +6538,9 @@ mod tests { providers: Vec::new(), ..Default::default() }), - phase: SandboxPhase::Provisioning as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Provisioning as i32); state.store.put_message(&sandbox).await.unwrap(); // Get current version @@ -6605,7 +6606,7 @@ mod tests { let state = Arc::new(test_server_state().await); // Create a sandbox WITHOUT a policy - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sb-1".to_string(), name: "test-sandbox".to_string(), @@ -6618,9 +6619,9 @@ mod tests { providers: Vec::new(), ..Default::default() }), - phase: SandboxPhase::Provisioning as i32, ..Default::default() }; + sandbox.set_phase(SandboxPhase::Provisioning as i32); state.store.put_message(&sandbox).await.unwrap(); // All three clients fetch the sandbox and see the same version diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index 3ddaae037..c58c0c9b7 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -3623,7 +3623,7 @@ mod tests { .await .unwrap(); - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sandbox-001".to_string(), name: "test-sandbox".to_string(), @@ -3636,9 +3636,8 @@ mod tests { ..SandboxSpec::default() }), status: None, - phase: SandboxPhase::Ready as i32, - ..Default::default() }; + sandbox.set_phase(SandboxPhase::Ready as i32); store.put_message(&sandbox).await.unwrap(); let loaded = store @@ -3660,7 +3659,7 @@ mod tests { let store = test_store().await; - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: "sandbox-002".to_string(), name: "empty-sandbox".to_string(), @@ -3670,9 +3669,8 @@ mod tests { }), spec: Some(SandboxSpec::default()), status: None, - phase: SandboxPhase::Ready as i32, - ..Default::default() }; + sandbox.set_phase(SandboxPhase::Ready as i32); store.put_message(&sandbox).await.unwrap(); let loaded = store diff --git a/crates/openshell-server/src/grpc/sandbox.rs b/crates/openshell-server/src/grpc/sandbox.rs index 1855972d7..8156a1005 100644 --- a/crates/openshell-server/src/grpc/sandbox.rs +++ b/crates/openshell-server/src/grpc/sandbox.rs @@ -107,7 +107,7 @@ pub(super) async fn handle_create_sandbox( let now_ms = current_time_ms(); - let sandbox = Sandbox { + let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { id: id.clone(), name: name.clone(), @@ -117,9 +117,8 @@ pub(super) async fn handle_create_sandbox( }), spec: Some(spec), status: None, - phase: SandboxPhase::Provisioning as i32, - current_policy_version: 0, }; + sandbox.set_phase(SandboxPhase::Provisioning as i32); // Ensure metadata is valid (defense in depth - should always be true for server-constructed metadata) super::validation::validate_object_metadata(sandbox.metadata.as_ref(), "sandbox")?; @@ -560,7 +559,7 @@ pub(super) async fn handle_watch_sandbox( if stop_on_terminal { let phase = - SandboxPhase::try_from(sandbox.phase).unwrap_or(SandboxPhase::Unknown); + SandboxPhase::try_from(sandbox.phase()).unwrap_or(SandboxPhase::Unknown); if phase == SandboxPhase::Ready { return; } @@ -630,7 +629,7 @@ pub(super) async fn handle_watch_sandbox( return; } if stop_on_terminal { - let phase = SandboxPhase::try_from(sandbox.phase).unwrap_or(SandboxPhase::Unknown); + let phase = SandboxPhase::try_from(sandbox.phase()).unwrap_or(SandboxPhase::Unknown); if phase == SandboxPhase::Ready { return; } @@ -733,7 +732,7 @@ pub(super) async fn handle_exec_sandbox( .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))? .ok_or_else(|| Status::not_found("sandbox not found"))?; - if SandboxPhase::try_from(sandbox.phase).ok() != Some(SandboxPhase::Ready) { + if SandboxPhase::try_from(sandbox.phase()).ok() != Some(SandboxPhase::Ready) { return Err(Status::failed_precondition("sandbox is not ready")); } @@ -847,7 +846,7 @@ pub(super) async fn handle_forward_tcp( .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))? .ok_or_else(|| Status::not_found("sandbox not found"))?; - if SandboxPhase::try_from(sandbox.phase).ok() != Some(SandboxPhase::Ready) { + if SandboxPhase::try_from(sandbox.phase()).ok() != Some(SandboxPhase::Ready) { return Err(Status::failed_precondition("sandbox is not ready")); } @@ -1176,7 +1175,7 @@ pub(super) async fn handle_exec_sandbox_interactive( .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))? .ok_or_else(|| Status::not_found("sandbox not found"))?; - if SandboxPhase::try_from(sandbox.phase).ok() != Some(SandboxPhase::Ready) { + if SandboxPhase::try_from(sandbox.phase()).ok() != Some(SandboxPhase::Ready) { return Err(Status::failed_precondition("sandbox is not ready")); } @@ -1249,7 +1248,7 @@ pub(super) async fn handle_create_ssh_session( .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))? .ok_or_else(|| Status::not_found("sandbox not found"))?; - if SandboxPhase::try_from(sandbox.phase).ok() != Some(SandboxPhase::Ready) { + if SandboxPhase::try_from(sandbox.phase()).ok() != Some(SandboxPhase::Ready) { return Err(Status::failed_precondition("sandbox is not ready")); } @@ -2091,7 +2090,7 @@ mod tests { } fn test_sandbox(name: &str, providers: Vec) -> Sandbox { - Sandbox { + let mut sandbox = Sandbox { metadata: Some(ObjectMeta { id: format!("sandbox-{name}"), name: name.to_string(), @@ -2105,10 +2104,11 @@ mod tests { providers, ..Default::default() }), - phase: SandboxPhase::Ready as i32, - current_policy_version: 7, ..Default::default() - } + }; + sandbox.set_phase(SandboxPhase::Ready as i32); + sandbox.set_current_policy_version(7); + sandbox } #[tokio::test] @@ -2144,11 +2144,11 @@ mod tests { .await .unwrap() .unwrap(); + assert_eq!(sandbox.phase(), SandboxPhase::Ready as i32); + assert_eq!(sandbox.current_policy_version(), 7); let spec = sandbox.spec.unwrap(); assert_eq!(spec.providers, vec!["work-github"]); assert_eq!(spec.log_level, "debug"); - assert_eq!(sandbox.phase, SandboxPhase::Ready as i32); - assert_eq!(sandbox.current_policy_version, 7); } #[tokio::test] @@ -2429,7 +2429,7 @@ mod tests { async fn interactive_exec_rejects_sandbox_not_ready() { let state = test_server_state().await; let mut sandbox = test_sandbox("not-ready", Vec::new()); - sandbox.phase = SandboxPhase::Provisioning as i32; + sandbox.set_phase(SandboxPhase::Provisioning as i32); state.store.put_message(&sandbox).await.unwrap(); let stored = state @@ -2439,7 +2439,7 @@ mod tests { .unwrap() .unwrap(); assert_ne!( - SandboxPhase::try_from(stored.phase).ok(), + SandboxPhase::try_from(stored.phase()).ok(), Some(SandboxPhase::Ready) ); } diff --git a/crates/openshell-server/src/grpc/service.rs b/crates/openshell-server/src/grpc/service.rs index 4d73f2279..a8144b4c7 100644 --- a/crates/openshell-server/src/grpc/service.rs +++ b/crates/openshell-server/src/grpc/service.rs @@ -279,22 +279,19 @@ mod tests { use openshell_core::proto::SandboxPhase; async fn seed_sandbox(state: &Arc, name: &str) { - state - .store - .put_message(&Sandbox { - metadata: Some(ObjectMeta { - id: format!("sandbox-{name}"), - name: name.to_string(), - created_at_ms: 1_000, - labels: HashMap::new(), - resource_version: 0, - }), - spec: Some(openshell_core::proto::SandboxSpec::default()), - phase: SandboxPhase::Ready as i32, - ..Default::default() - }) - .await - .unwrap(); + let mut sandbox = Sandbox { + metadata: Some(ObjectMeta { + id: format!("sandbox-{name}"), + name: name.to_string(), + created_at_ms: 1_000, + labels: HashMap::new(), + resource_version: 0, + }), + spec: Some(openshell_core::proto::SandboxSpec::default()), + ..Default::default() + }; + sandbox.set_phase(SandboxPhase::Ready as i32); + state.store.put_message(&sandbox).await.unwrap(); } #[test] diff --git a/crates/openshell-server/src/persistence/tests.rs b/crates/openshell-server/src/persistence/tests.rs index 123feb862..bd14c39f9 100644 --- a/crates/openshell-server/src/persistence/tests.rs +++ b/crates/openshell-server/src/persistence/tests.rs @@ -1252,8 +1252,6 @@ async fn cas_update_message_cas_succeeds() { }), spec: None, status: None, - phase: 0, - current_policy_version: 0, }; store.put_message(&sandbox).await.unwrap(); @@ -1261,14 +1259,14 @@ async fn cas_update_message_cas_succeeds() { // Update using CAS with expected_version = 0 (use current version) let updated = store .update_message_cas::("test-id", 0, |s| { - s.phase = 2; // Set to Ready - s.current_policy_version = 42; + s.set_phase(2); // Set to Ready + s.set_current_policy_version(42); }) .await .unwrap(); - assert_eq!(updated.phase, 2); - assert_eq!(updated.current_policy_version, 42); + assert_eq!(updated.phase(), 2); + assert_eq!(updated.current_policy_version(), 42); assert_eq!( updated.metadata.as_ref().map_or(0, |m| m.resource_version), 2 @@ -1293,8 +1291,6 @@ async fn cas_update_message_cas_conflicts_on_concurrent_updates() { }), spec: None, status: None, - phase: 0, - current_policy_version: 0, }; store.put_message(&sandbox).await.unwrap(); @@ -1308,7 +1304,7 @@ async fn cas_update_message_cas_conflicts_on_concurrent_updates() { let handle = tokio::spawn(async move { store .update_message_cas::("test-id", 1, |s| { - s.current_policy_version = i; + s.set_current_policy_version(i); }) .await }); diff --git a/crates/openshell-server/src/service_routing.rs b/crates/openshell-server/src/service_routing.rs index 4b99a8ef7..7ebd6dba9 100644 --- a/crates/openshell-server/src/service_routing.rs +++ b/crates/openshell-server/src/service_routing.rs @@ -264,7 +264,7 @@ async fn proxy_to_endpoint( return Err(route_err); } }; - if SandboxPhase::try_from(sandbox.phase).ok() != Some(SandboxPhase::Ready) { + if SandboxPhase::try_from(sandbox.phase()).ok() != Some(SandboxPhase::Ready) { let err = ServiceRouteError::sandbox_not_ready(); emit_service_http_failure( &state, diff --git a/crates/openshell-tui/src/lib.rs b/crates/openshell-tui/src/lib.rs index 1969715ce..22201dbb6 100644 --- a/crates/openshell-tui/src/lib.rs +++ b/crates/openshell-tui/src/lib.rs @@ -20,6 +20,7 @@ use crossterm::terminal::{ use miette::{IntoDiagnostic, Result}; use openshell_core::auth::EdgeAuthInterceptor; use openshell_core::metadata::{ObjectId, ObjectLabels, ObjectName}; +use openshell_core::proto::SandboxPhase; use openshell_core::proto::open_shell_client::OpenShellClient; use ratatui::Terminal; use ratatui::backend::CrosstermBackend; @@ -1418,10 +1419,10 @@ fn spawn_create_sandbox(app: &mut App, tx: mpsc::UnboundedSender) { if let Ok(resp) = client.get_sandbox(req).await && let Some(sandbox) = resp.into_inner().sandbox { - if sandbox.phase == 2 { + if sandbox.phase() == SandboxPhase::Ready as i32 { break sandbox.object_id().to_string(); } - if sandbox.phase == 3 { + if sandbox.phase() == SandboxPhase::Error as i32 { let _ = tx.send(Event::CreateResult(Err( "sandbox entered error state".to_string() ))); @@ -2274,7 +2275,7 @@ async fn refresh_sandboxes(app: &mut App) { .iter() .map(|s| s.object_name().to_string()) .collect(); - app.sandbox_phases = sandboxes.iter().map(|s| phase_label(s.phase)).collect(); + app.sandbox_phases = sandboxes.iter().map(|s| phase_label(s.phase())).collect(); app.sandbox_images = sandboxes .iter() .map(|s| { @@ -2304,8 +2305,10 @@ async fn refresh_sandboxes(app: &mut App) { }) .collect(); - app.sandbox_policy_versions = - sandboxes.iter().map(|s| s.current_policy_version).collect(); + app.sandbox_policy_versions = sandboxes + .iter() + .map(openshell_core::proto::Sandbox::current_policy_version) + .collect(); // Build NOTES column from active port forwards. let forwards = openshell_core::forward::list_forwards().unwrap_or_default(); @@ -2428,10 +2431,10 @@ async fn refresh_sandbox_draft_counts(app: &mut App) { fn phase_label(phase: i32) -> String { match phase { - 1 => "Provisioning", - 2 => "Ready", - 3 => "Error", - 4 => "Deleting", + x if x == SandboxPhase::Provisioning as i32 => "Provisioning", + x if x == SandboxPhase::Ready as i32 => "Ready", + x if x == SandboxPhase::Error as i32 => "Error", + x if x == SandboxPhase::Deleting as i32 => "Deleting", _ => "Unknown", } .to_string() diff --git a/proto/openshell.proto b/proto/openshell.proto index 90d1594f7..02dbbe283 100644 --- a/proto/openshell.proto +++ b/proto/openshell.proto @@ -298,10 +298,9 @@ message Sandbox { SandboxSpec spec = 2; // Latest user-facing observed status derived by the gateway. SandboxStatus status = 3; - // Gateway-derived lifecycle summary. - SandboxPhase phase = 4; - // Currently active policy version (updated when sandbox reports loaded). - uint32 current_policy_version = 5; + + reserved 4, 5; + reserved "phase", "current_policy_version"; } // Desired sandbox configuration provided through the public API. @@ -352,8 +351,7 @@ message SandboxTemplate { // User-facing sandbox status derived by the gateway from compute-driver observations. // -// Lifecycle summary is exposed separately as `Sandbox.phase`. Public status does -// not embed driver-only flags such as `deleting`. +// Public status does not embed driver-only flags such as `deleting`. message SandboxStatus { // Compute-platform sandbox object name. string sandbox_name = 1; @@ -365,6 +363,10 @@ message SandboxStatus { string sandbox_fd = 4; // Latest user-facing readiness and lifecycle conditions. repeated SandboxCondition conditions = 5; + // Gateway-derived lifecycle summary. + SandboxPhase phase = 6; + // Currently active policy version (updated when sandbox reports loaded). + uint32 current_policy_version = 7; } // User-facing sandbox condition derived from driver-native conditions. diff --git a/python/openshell/__init__.py b/python/openshell/__init__.py index 27f8aaae9..94a391d4e 100644 --- a/python/openshell/__init__.py +++ b/python/openshell/__init__.py @@ -15,6 +15,7 @@ SandboxError, SandboxRef, SandboxSession, + SandboxStatusRef, TlsConfig, ) @@ -35,6 +36,7 @@ "SandboxError", "SandboxRef", "SandboxSession", + "SandboxStatusRef", "TlsConfig", "__version__", ] diff --git a/python/openshell/sandbox.py b/python/openshell/sandbox.py index 11cdbd98b..80a2f674d 100644 --- a/python/openshell/sandbox.py +++ b/python/openshell/sandbox.py @@ -34,11 +34,25 @@ class TlsConfig: key_path: pathlib.Path +@dataclass(frozen=True) +class SandboxStatusRef: + phase: int + current_policy_version: int + + @dataclass(frozen=True) class SandboxRef: id: str name: str - phase: int + status: SandboxStatusRef + + @property + def phase(self) -> int: + return self.status.phase + + @property + def current_policy_version(self) -> int: + return self.status.current_policy_version @dataclass(frozen=True) @@ -261,9 +275,9 @@ def wait_ready( deadline = time.time() + timeout_seconds while time.time() < deadline: sandbox = self.get(sandbox_name) - if sandbox.phase == openshell_pb2.SANDBOX_PHASE_READY: + if sandbox.status.phase == openshell_pb2.SANDBOX_PHASE_READY: return sandbox - if sandbox.phase == openshell_pb2.SANDBOX_PHASE_ERROR: + if sandbox.status.phase == openshell_pb2.SANDBOX_PHASE_ERROR: raise SandboxError(f"sandbox {sandbox_name} entered error phase") time.sleep(1) raise SandboxError(f"sandbox {sandbox_name} was not ready within timeout") @@ -585,10 +599,14 @@ def _serialize_python_callable( def _sandbox_ref(sandbox: openshell_pb2.Sandbox) -> SandboxRef: + status = sandbox.status if sandbox.HasField("status") else None return SandboxRef( id=sandbox.metadata.id if sandbox.metadata else "", name=sandbox.metadata.name if sandbox.metadata else "", - phase=sandbox.phase, + status=SandboxStatusRef( + phase=status.phase if status else 0, + current_policy_version=status.current_policy_version if status else 0, + ), ) From 122942e77426bf9cf0908a53d5f626cb13c9a3e4 Mon Sep 17 00:00:00 2001 From: Derek Carr Date: Tue, 26 May 2026 17:48:54 -0400 Subject: [PATCH 2/2] fix(server): preserve sandbox status on statusless driver updates When a driver update arrives without a status payload (e.g. before Kubernetes populates the status subresource), preserve the stored phase, conditions, and current policy version instead of resetting them. Adds a regression test covering the edge case. Signed-off-by: Derek Carr --- crates/openshell-server/src/compute/mod.rs | 75 ++++++++++++++++++++-- 1 file changed, 71 insertions(+), 4 deletions(-) diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index 6036f8857..0a79596e9 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -924,7 +924,12 @@ impl ComputeRuntime { let sandbox = self .store .update_message_cas::(&incoming.id, 0, |sandbox| { - let mut phase = derive_phase(incoming.status.as_ref()); + let old_phase = + SandboxPhase::try_from(sandbox.phase()).unwrap_or(SandboxPhase::Unknown); + let mut phase = incoming + .status + .as_ref() + .map_or(old_phase, |status| derive_phase(Some(status))); let supervisor_promoted = session_connected && matches!(phase, SandboxPhase::Provisioning | SandboxPhase::Unknown); if supervisor_promoted { @@ -935,14 +940,19 @@ impl ComputeRuntime { let mut status = incoming .status .as_ref() - .map(|s| public_status_from_driver(s, phase, cpv)); + .map(|s| public_status_from_driver(s, phase, cpv)) + .or_else(|| sandbox.status.clone()); rewrite_user_facing_conditions(&mut status, sandbox.spec.as_ref()); if supervisor_promoted { ensure_supervisor_ready_status(&mut status, &sandbox_name); } - let old_phase = - SandboxPhase::try_from(sandbox.phase()).unwrap_or(SandboxPhase::Unknown); + if let Some(s) = status.as_mut() + && s.sandbox_name.is_empty() + { + s.sandbox_name.clone_from(&sandbox_name); + } + if old_phase != phase { info!( sandbox_id = %incoming.id, @@ -978,6 +988,7 @@ impl ComputeRuntime { } sandbox.status = status; sandbox.set_phase(phase as i32); + sandbox.set_current_policy_version(cpv); }) .await .map_err(|e| match e { @@ -2328,6 +2339,62 @@ mod tests { ); } + #[tokio::test] + async fn apply_sandbox_update_without_status_preserves_existing_status() { + let runtime = test_runtime(Arc::new(TestDriver::default())).await; + let mut sandbox = sandbox_record("sb-1", "sandbox-a", SandboxPhase::Ready); + sandbox.status = Some(SandboxStatus { + sandbox_name: "sandbox-a".to_string(), + conditions: vec![SandboxCondition { + r#type: "Ready".to_string(), + status: "True".to_string(), + reason: "DependenciesReady".to_string(), + message: "Pod is Ready".to_string(), + last_transition_time: String::new(), + }], + current_policy_version: 7, + ..Default::default() + }); + sandbox.set_phase(SandboxPhase::Ready as i32); + runtime.store.put_message(&sandbox).await.unwrap(); + + runtime + .apply_sandbox_update(DriverSandbox { + id: "sb-1".to_string(), + name: "sandbox-a".to_string(), + namespace: "default".to_string(), + spec: None, + status: None, + }) + .await + .unwrap(); + + let stored = runtime + .store + .get_message::("sb-1") + .await + .unwrap() + .unwrap(); + assert_eq!( + SandboxPhase::try_from(stored.phase()).unwrap(), + SandboxPhase::Ready + ); + assert_eq!(stored.current_policy_version(), 7); + let ready = stored + .status + .as_ref() + .and_then(|status| { + status + .conditions + .iter() + .find(|condition| condition.r#type == "Ready") + }) + .unwrap(); + assert_eq!(ready.status, "True"); + assert_eq!(ready.reason, "DependenciesReady"); + assert_eq!(ready.message, "Pod is Ready"); + } + #[tokio::test] async fn apply_sandbox_update_promotes_connected_supervisor_session_to_ready() { let runtime = test_runtime(Arc::new(TestDriver::default())).await;