diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 4a6f32f1aa..7af4c6437f 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -2541,14 +2541,23 @@ async fn validate_provider_composition_for_existing_sandboxes( .expect("catalog was inserted for sandbox workspace"); let base_policy = current_base_policy_for_sandbox(state.store.as_ref(), &sandbox).await?; - let provider_layers = provider_policy_context_with_catalog( + // A provider deleted under a sandbox leaves a dangling reference. The + // sandbox already fails closed, so warn and validate the rest. + let (records, missing) = super::provider::load_existing_provider_environment_records( state.store.as_ref(), - catalog, &workspace, provider_names, ) - .await? - .layers; + .await?; + if !missing.is_empty() { + warn!( + workspace = %workspace, + sandbox = %sandbox.object_name(), + providers = %missing.join(","), + "sandbox references providers that no longer exist" + ); + } + let provider_layers = provider_policy_context_from_records(catalog, &records).layers; validate_candidate_effective_policy(&base_policy, &provider_layers).map_err(|error| { Status::failed_precondition(format!( "cannot activate provider policy composition: sandbox '{}/{}' has an invalid effective policy: {}", @@ -22582,6 +22591,98 @@ mod tests { assert!(error.message().contains("invalid effective policy")); } + async fn install_dangling_provider_sandbox(state: &Arc) { + state + .store + .put_message(&test_sandbox( + "sandbox-dangling", + "sandbox-dangling", + test_policy_with_rule("base", "api.example.com"), + vec!["deleted-provider".to_string()], + )) + .await + .unwrap(); + } + + #[tokio::test] + async fn startup_preflight_tolerates_dangling_provider_reference() { + let state = test_server_state().await; + install_dangling_provider_sandbox(&state).await; + + validate_provider_composition_startup_preflight(&state) + .await + .expect("a dangling provider reference must not block startup"); + } + + #[tokio::test] + async fn startup_preflight_still_rejects_invalid_policy_beside_dangling_reference() { + let state = test_server_state().await; + install_dangling_provider_sandbox(&state).await; + // Sorts after `sandbox-dangling`, so the dangling sandbox is checked first. + install_ambiguous_provider_binding(&state, "z-beside-dangling").await; + + let error = validate_provider_composition_startup_preflight(&state) + .await + .expect_err("an invalid composed policy must stay fatal"); + + assert_eq!(error.code(), Code::FailedPrecondition); + assert!(error.message().contains("sandbox-z-beside-dangling")); + } + + #[tokio::test] + async fn startup_preflight_validates_providers_beside_dangling_reference_on_one_sandbox() { + let state = test_server_state().await; + install_ambiguous_provider_binding(&state, "mixed").await; + state + .store + .put_message(&test_sandbox( + "sandbox-mixed", + "sandbox-mixed", + test_policy_with_rule("base", "api.example.com"), + vec!["deleted-provider".to_string(), "provider-mixed".to_string()], + )) + .await + .unwrap(); + + let error = validate_provider_composition_startup_preflight(&state) + .await + .expect_err("a dangling reference must not skip the sandbox's other providers"); + + assert_eq!(error.code(), Code::FailedPrecondition); + assert!(error.message().contains("sandbox-mixed")); + } + + #[tokio::test] + async fn deleting_global_policy_tolerates_dangling_provider_reference() { + let state = test_server_state().await; + install_dangling_provider_sandbox(&state).await; + + handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + global: true, + policy: Some(test_policy_with_rule("global", "global.example.com")), + ..Default::default() + })), + ) + .await + .expect("global policy should suppress provider composition"); + handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + global: true, + setting_key: POLICY_SETTING_KEY.to_string(), + delete_setting: true, + ..Default::default() + })), + ) + .await + .expect("a dangling provider reference must not block global policy deletion"); + + let settings = load_global_settings(state.store.as_ref()).await.unwrap(); + assert!(!settings.settings.contains_key(POLICY_SETTING_KEY)); + } + #[test] fn merge_effective_settings_global_overrides_sandbox_key() { let global = StoredSettings { diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index e82bb9a4d7..8abcbe037e 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -1027,31 +1027,85 @@ pub(super) async fn resolve_provider_environment_with_credentials( resolve_provider_environment_from_records_with_credentials(catalog, &records, credentials).await } +/// Loads one provider; `Ok(None)` means it does not exist. +async fn load_provider_environment_record( + store: &Store, + workspace: &str, + name: &str, +) -> Result, Status> { + let Some(record) = store + .get_by_name(Provider::object_type(), workspace, name) + .await + .map_err(|e| Status::internal(format!("failed to fetch provider '{name}': {e}")))? + else { + return Ok(None); + }; + let provider = Provider::decode(record.payload.as_slice()) + .map_err(|e| Status::internal(format!("failed to decode provider '{name}': {e}")))?; + let refresh_states = + crate::provider_refresh::list_refresh_states_for_provider(store, &record.id).await?; + Ok(Some(ProviderEnvironmentRecord { + name: name.to_string(), + object_id: record.id, + resource_version: record.resource_version, + provider, + refresh_states, + })) +} + +/// Loads providers concurrently, in `provider_names` order; a missing +/// provider is `None`. Store errors short-circuit. +async fn load_optional_provider_environment_records( + store: &Store, + workspace: &str, + provider_names: &[String], +) -> Result>, Status> { + // Providers are independent, so load them concurrently. + futures::future::try_join_all( + provider_names + .iter() + .map(|name| load_provider_environment_record(store, workspace, name)), + ) + .await +} + pub(super) async fn load_provider_environment_records( store: &Store, workspace: &str, provider_names: &[String], ) -> Result, Status> { - // Providers are independent, so load them concurrently. - futures::future::try_join_all(provider_names.iter().map(|name| async move { - let record = store - .get_by_name(Provider::object_type(), workspace, name) - .await - .map_err(|e| Status::internal(format!("failed to fetch provider '{name}': {e}")))? - .ok_or_else(|| Status::failed_precondition(format!("provider '{name}' not found")))?; - let provider = Provider::decode(record.payload.as_slice()) - .map_err(|e| Status::internal(format!("failed to decode provider '{name}': {e}")))?; - let refresh_states = - crate::provider_refresh::list_refresh_states_for_provider(store, &record.id).await?; - Ok(ProviderEnvironmentRecord { - name: name.clone(), - object_id: record.id, - resource_version: record.resource_version, - provider, - refresh_states, + load_optional_provider_environment_records(store, workspace, provider_names) + .await? + .into_iter() + .zip(provider_names) + .map(|(record, name)| { + record + .ok_or_else(|| Status::failed_precondition(format!("provider '{name}' not found"))) }) - })) - .await + .collect() +} + +/// Like [`load_provider_environment_records`], but returns the names of +/// missing providers instead of failing. +pub(super) async fn load_existing_provider_environment_records<'a>( + store: &Store, + workspace: &str, + provider_names: &'a [String], +) -> Result<(Vec, Vec<&'a str>), Status> { + let mut records = Vec::with_capacity(provider_names.len()); + let mut missing = Vec::new(); + for (record, name) in + load_optional_provider_environment_records(store, workspace, provider_names) + .await? + .into_iter() + .zip(provider_names) + { + match record { + Some(record) => records.push(record), + None => missing.push(name.as_str()), + } + } + Ok((records, missing)) } #[cfg(test)]