From 1ed3a28e09724eae20ff8ed886ff1cf641858ef3 Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Fri, 16 Oct 2026 21:56:54 +0100 Subject: [PATCH 1/2] fix(server): warn on dangling provider refs at startup Signed-off-by: Eric Curtin --- crates/openshell-server/src/grpc/policy.rs | 70 +++++++++++++++++--- crates/openshell-server/src/grpc/provider.rs | 44 +++++++----- 2 files changed, 91 insertions(+), 23 deletions(-) diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index e9c22bef6f..2655e9eca6 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -2502,14 +2502,31 @@ 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( - state.store.as_ref(), - catalog, - &workspace, - provider_names, - ) - .await? - .layers; + // A provider deleted under a sandbox leaves a dangling reference. The + // sandbox already fails closed, so warn and validate the rest. + let mut records = Vec::with_capacity(provider_names.len()); + let mut missing = Vec::new(); + for name in provider_names { + match super::provider::load_provider_environment_record( + state.store.as_ref(), + &workspace, + name, + ) + .await? + { + Some(record) => records.push(record), + None => missing.push(name.as_str()), + } + } + 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: {}", @@ -21887,6 +21904,43 @@ 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; + install_ambiguous_provider_binding(&state, "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-beside-dangling")); + } + #[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 883f897d15..6f7ec6729d 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -1033,6 +1033,32 @@ pub(super) async fn resolve_provider_environment_with_credentials( .await } +/// Loads one provider; `Ok(None)` means it does not exist. +pub(super) 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, + })) +} + pub(super) async fn load_provider_environment_records( store: &Store, workspace: &str, @@ -1040,22 +1066,10 @@ pub(super) async fn load_provider_environment_records( ) -> Result, Status> { let mut records = Vec::with_capacity(provider_names.len()); for name in provider_names { - let record = store - .get_by_name(Provider::object_type(), workspace, name) - .await - .map_err(|e| Status::internal(format!("failed to fetch provider '{name}': {e}")))? + let record = load_provider_environment_record(store, workspace, name) + .await? .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?; - records.push(ProviderEnvironmentRecord { - name: name.clone(), - object_id: record.id, - resource_version: record.resource_version, - provider, - refresh_states, - }); + records.push(record); } Ok(records) } From 8c3231e65e79d68c5d1771ec7d9afee3dfe2098c Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Sun, 18 Oct 2026 21:29:37 +0100 Subject: [PATCH 2/2] fix(server): load providers concurrently at startup Signed-off-by: Eric Curtin --- crates/openshell-server/src/grpc/policy.rs | 79 ++++++++++++++++---- crates/openshell-server/src/grpc/provider.rs | 55 ++++++++++++-- 2 files changed, 111 insertions(+), 23 deletions(-) diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index dae9d2b41b..7af4c6437f 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -2543,20 +2543,12 @@ async fn validate_provider_composition_for_existing_sandboxes( current_base_policy_for_sandbox(state.store.as_ref(), &sandbox).await?; // A provider deleted under a sandbox leaves a dangling reference. The // sandbox already fails closed, so warn and validate the rest. - let mut records = Vec::with_capacity(provider_names.len()); - let mut missing = Vec::new(); - for name in provider_names { - match super::provider::load_provider_environment_record( - state.store.as_ref(), - &workspace, - name, - ) - .await? - { - Some(record) => records.push(record), - None => missing.push(name.as_str()), - } - } + let (records, missing) = super::provider::load_existing_provider_environment_records( + state.store.as_ref(), + &workspace, + provider_names, + ) + .await?; if !missing.is_empty() { warn!( workspace = %workspace, @@ -22626,14 +22618,69 @@ mod tests { async fn startup_preflight_still_rejects_invalid_policy_beside_dangling_reference() { let state = test_server_state().await; install_dangling_provider_sandbox(&state).await; - install_ambiguous_provider_binding(&state, "beside-dangling").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-beside-dangling")); + 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] diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index f9879b9c4b..8abcbe037e 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -1028,7 +1028,7 @@ pub(super) async fn resolve_provider_environment_with_credentials( } /// Loads one provider; `Ok(None)` means it does not exist. -pub(super) async fn load_provider_environment_record( +async fn load_provider_environment_record( store: &Store, workspace: &str, name: &str, @@ -1053,18 +1053,59 @@ pub(super) async fn load_provider_environment_record( })) } +/// 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 { - load_provider_environment_record(store, workspace, name) + 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"))) + }) + .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? - .ok_or_else(|| Status::failed_precondition(format!("provider '{name}' not found"))) - })) - .await + .into_iter() + .zip(provider_names) + { + match record { + Some(record) => records.push(record), + None => missing.push(name.as_str()), + } + } + Ok((records, missing)) } #[cfg(test)]