Hare/yoi
1
1
Fork 0
Code Actions Packages Releases Activity

fix: reject ambiguous feature service providers

Browse Source
This commit is contained in:
Hare
2026-08-26 16:01:06 +09:00
parent 048007a042
commit aaa9c7987c
3 changed files with 37 additions and 167 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/worker/src/controller.rs
+1 -1
View File
@@ -874,7 +874,7 @@ where
let mut feature_registry = FeatureRegistryBuilder::new(); let mut feature_registry = FeatureRegistryBuilder::new();
if feature_config.sub_worker.enabled { if feature_config.sub_worker.enabled {
feature_registry.add_fallback_service_provider( feature_registry.add_module(
crate::feature::builtin::manage_worker::sub_worker_control_feature( crate::feature::builtin::manage_worker::sub_worker_control_feature(
worker.workspace_client_handle(), worker.workspace_client_handle(),
spawned_registry.clone(), spawned_registry.clone(),
crates/worker/src/feature.rs
+20 -145
View File
@@ -1555,34 +1555,16 @@ pub enum FeaturePlanError {
features: Vec<FeatureId>, features: Vec<FeatureId>,
services: Vec<ServiceId>, services: Vec<ServiceId>,
}, },
#[error(
"fallback service provider `{feature}` must contribute exactly one service and no other feature surface"
)]
InvalidFallbackProvider { feature: FeatureId },
}
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum ServiceProviderPreference {
Primary,
Fallback,
}
struct FeatureRegistryContribution {
module: Arc<dyn FeatureModule>,
provider_preference: ServiceProviderPreference,
} }
/// Builder/installer for enabled feature modules. /// Builder/installer for enabled feature modules.
pub struct FeatureRegistryBuilder { pub struct FeatureRegistryBuilder {
contributions: Vec<FeatureRegistryContribution>, modules: Vec<Arc<dyn FeatureModule>>,
} }
fn build_feature_install_plan( fn build_feature_install_plan(
descriptors: &[FeatureDescriptor], descriptors: &[FeatureDescriptor],
preferences: &[ServiceProviderPreference],
) -> Result<FeatureInstallPlan, FeaturePlanError> { ) -> Result<FeatureInstallPlan, FeaturePlanError> {
debug_assert_eq!(descriptors.len(), preferences.len());
let mut feature_ids = BTreeSet::new(); let mut feature_ids = BTreeSet::new();
for descriptor in descriptors { for descriptor in descriptors {
if !feature_ids.insert(descriptor.id.clone()) { if !feature_ids.insert(descriptor.id.clone()) {
@@ -1595,19 +1577,6 @@ fn build_feature_install_plan(
let mut provider_candidates: BTreeMap<ServiceId, Vec<(usize, ServiceDeclaration)>> = let mut provider_candidates: BTreeMap<ServiceId, Vec<(usize, ServiceDeclaration)>> =
BTreeMap::new(); BTreeMap::new();
for (index, descriptor) in descriptors.iter().enumerate() { for (index, descriptor) in descriptors.iter().enumerate() {
if preferences[index] == ServiceProviderPreference::Fallback
&& (descriptor.provides_services.len() != 1
|| !descriptor.tools.is_empty()
|| !descriptor.hooks.is_empty()
|| !descriptor.instructions.is_empty()
|| !descriptor.background_tasks.is_empty()
|| !descriptor.requires_services.is_empty()
|| !descriptor.protocol_providers.is_empty())
{
return Err(FeaturePlanError::InvalidFallbackProvider {
feature: descriptor.id.clone(),
});
}
for service in &descriptor.provides_services { for service in &descriptor.provides_services {
provider_candidates provider_candidates
.entry(service.id.clone()) .entry(service.id.clone())
@@ -1617,52 +1586,23 @@ fn build_feature_install_plan(
} }
let mut selected_providers = BTreeMap::new(); let mut selected_providers = BTreeMap::new();
let mut selected_fallbacks = BTreeSet::new();
for (service, candidates) in provider_candidates { for (service, candidates) in provider_candidates {
let primary = candidates if candidates.len() > 1 {
.iter() let mut providers = candidates
.filter(|(index, _)| preferences[*index] == ServiceProviderPreference::Primary)
.collect::<Vec<_>>();
let selected = if primary.len() == 1 {
primary[0]
} else if primary.len() > 1 {
let mut providers = primary
.iter() .iter()
.map(|(index, _)| descriptors[*index].id.clone()) .map(|(index, _)| descriptors[*index].id.clone())
.collect::<Vec<_>>(); .collect::<Vec<_>>();
providers.sort(); providers.sort();
return Err(FeaturePlanError::AmbiguousServiceProvider { service, providers }); return Err(FeaturePlanError::AmbiguousServiceProvider { service, providers });
} else { }
let fallback = candidates let (provider_index, declaration) = candidates
.iter() .into_iter()
.filter(|(index, _)| preferences[*index] == ServiceProviderPreference::Fallback) .next()
.collect::<Vec<_>>(); .expect("provider candidates are non-empty");
if fallback.len() > 1 { selected_providers.insert(service, (provider_index, declaration));
let mut providers = fallback
.iter()
.map(|(index, _)| descriptors[*index].id.clone())
.collect::<Vec<_>>();
providers.sort();
return Err(FeaturePlanError::AmbiguousServiceProvider { service, providers });
}
let Some(selected) = fallback.first() else {
continue;
};
selected_fallbacks.insert(selected.0);
*selected
};
selected_providers.insert(service.clone(), (selected.0, selected.1.clone()));
} }
let active = descriptors let active = (0..descriptors.len()).collect::<BTreeSet<_>>();
.iter()
.enumerate()
.filter_map(|(index, _)| {
(preferences[index] == ServiceProviderPreference::Primary
|| selected_fallbacks.contains(&index))
.then_some(index)
})
.collect::<BTreeSet<_>>();
let mut adjacency = vec![BTreeSet::new(); descriptors.len()]; let mut adjacency = vec![BTreeSet::new(); descriptors.len()];
let mut indegree = vec![0usize; descriptors.len()]; let mut indegree = vec![0usize; descriptors.len()];
let mut edge_services: BTreeMap<(usize, usize), BTreeSet<ServiceId>> = BTreeMap::new(); let mut edge_services: BTreeMap<(usize, usize), BTreeSet<ServiceId>> = BTreeMap::new();
@@ -1815,7 +1755,7 @@ fn find_service_dependency_cycle(
impl Default for FeatureRegistryBuilder { impl Default for FeatureRegistryBuilder {
fn default() -> Self { fn default() -> Self {
Self { Self {
contributions: Vec::new(), modules: Vec::new(),
} }
} }
} }
@@ -1829,23 +1769,7 @@ impl FeatureRegistryBuilder {
where where
M: FeatureModule + 'static, M: FeatureModule + 'static,
{ {
self.contributions.push(FeatureRegistryContribution { self.modules.push(Arc::new(module));
module: Arc::new(module),
provider_preference: ServiceProviderPreference::Primary,
});
self
}
/// Register a service-only provider that is selected only when no primary
/// contribution provides its service.
pub fn add_fallback_service_provider<M>(&mut self, module: M) -> &mut Self
where
M: FeatureModule + 'static,
{
self.contributions.push(FeatureRegistryContribution {
module: Arc::new(module),
provider_preference: ServiceProviderPreference::Fallback,
});
self self
} }
@@ -1858,24 +1782,18 @@ impl FeatureRegistryBuilder {
} }
pub fn is_empty(&self) -> bool { pub fn is_empty(&self) -> bool {
self.contributions.is_empty() self.modules.is_empty()
} }
pub fn descriptors(&self) -> Vec<FeatureDescriptor> { pub fn descriptors(&self) -> Vec<FeatureDescriptor> {
self.contributions self.modules
.iter() .iter()
.map(|contribution| contribution.module.descriptor()) .map(|module| module.descriptor())
.collect() .collect()
} }
pub fn plan(&self) -> Result<FeatureInstallPlan, FeaturePlanError> { pub fn plan(&self) -> Result<FeatureInstallPlan, FeaturePlanError> {
let descriptors = self.descriptors(); build_feature_install_plan(&self.descriptors())
let preferences = self
.contributions
.iter()
.map(|contribution| contribution.provider_preference)
.collect::<Vec<_>>();
build_feature_install_plan(&descriptors, &preferences)
} }
/// Install modules into the existing Engine tool path and hook builder. /// Install modules into the existing Engine tool path and hook builder.
@@ -1919,16 +1837,11 @@ impl FeatureRegistryBuilder {
mut installed_tool_names: HashMap<String, FeatureId>, mut installed_tool_names: HashMap<String, FeatureId>,
) -> FeatureRegistryInstallReport { ) -> FeatureRegistryInstallReport {
let descriptors: Vec<_> = self let descriptors: Vec<_> = self
.contributions .modules
.iter() .iter()
.map(|contribution| contribution.module.descriptor()) .map(|module| module.descriptor())
.collect(); .collect();
let preferences = self let plan = match build_feature_install_plan(&descriptors) {
.contributions
.iter()
.map(|contribution| contribution.provider_preference)
.collect::<Vec<_>>();
let plan = match build_feature_install_plan(&descriptors, &preferences) {
Ok(plan) => plan, Ok(plan) => plan,
Err(error) => { Err(error) => {
let message = error.to_string(); let message = error.to_string();
@@ -1956,11 +1869,7 @@ impl FeatureRegistryBuilder {
}; };
let mut service_registry = FeatureServiceRegistry::default(); let mut service_registry = FeatureServiceRegistry::default();
let mut reports = Vec::with_capacity(plan.ordered_indices.len()); let mut reports = Vec::with_capacity(plan.ordered_indices.len());
let mut modules = self let mut modules = self.modules.into_iter().map(Some).collect::<Vec<_>>();
.contributions
.into_iter()
.map(|contribution| Some(contribution.module))
.collect::<Vec<_>>();
let ordered_modules = plan let ordered_modules = plan
.ordered_indices .ordered_indices
.into_iter() .into_iter()
@@ -2384,40 +2293,6 @@ mod tests {
); );
} }
#[test]
fn primary_provider_supersedes_explicit_fallback_candidate() {
let primary = PlannedServiceFeature::new(
FeatureDescriptor::builtin("primary", "Primary")
.with_provided_service(provided_service("control", "1")),
);
let fallback = PlannedServiceFeature::new(
FeatureDescriptor::builtin("fallback", "Fallback")
.with_provided_service(provided_service("control", "1")),
);
let fallback_calls = Arc::clone(&fallback.install_calls);
let mut builder = FeatureRegistryBuilder::new().with_module(primary);
builder.add_fallback_service_provider(fallback);
let plan = builder
.plan()
.expect("primary plus fallback is unambiguous");
assert_eq!(
plan.service_providers()[&ServiceId::builtin("control")].provider,
FeatureId::builtin("primary")
);
assert!(
!plan
.ordered_features()
.contains(&FeatureId::builtin("fallback"))
);
let mut hook_builder = HookRegistryBuilder::default();
let mut pending_tools = Vec::new();
let report = builder.install_into_pending(&mut pending_tools, &mut hook_builder);
assert!(!report.has_errors());
assert_eq!(fallback_calls.load(Ordering::SeqCst), 0);
}
#[test] #[test]
fn plan_failure_has_no_install_side_effects_and_reports_identities() { fn plan_failure_has_no_install_side_effects_and_reports_identities() {
let first = PlannedServiceFeature::new( let first = PlannedServiceFeature::new(
crates/worker/src/feature/builtin/manage_worker.rs
+16 -21
View File
@@ -1093,7 +1093,7 @@ mod tests {
} }
#[tokio::test] #[tokio::test]
async fn worker_provider_supersedes_sub_worker_fallback_without_duplicate_service() { async fn worker_and_sub_worker_providers_are_ambiguous_before_install() {
let runtime_base = tempfile::tempdir().unwrap(); let runtime_base = tempfile::tempdir().unwrap();
let runtime_dir = Arc::new( let runtime_dir = Arc::new(
crate::runtime::dir::RuntimeDir::create(runtime_base.path(), "feature-plan") crate::runtime::dir::RuntimeDir::create(runtime_base.path(), "feature-plan")
@@ -1103,34 +1103,29 @@ mod tests {
let registry = SpawnedWorkerRegistry::new(runtime_dir); let registry = SpawnedWorkerRegistry::new(runtime_dir);
let client: Arc<dyn WorkspaceClient> = Arc::new(RecordingWorkspaceClient::default()); let client: Arc<dyn WorkspaceClient> = Arc::new(RecordingWorkspaceClient::default());
let mut builder = crate::feature::FeatureRegistryBuilder::new(); let mut builder = crate::feature::FeatureRegistryBuilder::new();
builder.add_fallback_service_provider(sub_worker_control_feature( builder.add_module(sub_worker_control_feature(client.clone(), registry.clone()));
client.clone(),
registry.clone(),
));
builder.add_module(manage_worker_feature(client, Some(registry), true)); builder.add_module(manage_worker_feature(client, Some(registry), true));
let plan = builder
.plan()
.expect("primary worker provider must supersede fallback");
let provider = &plan.service_providers()[&ServiceId::builtin(WORKER_CONTROL_SERVICE_ID)];
assert_eq!( assert_eq!(
provider.provider, builder.plan(),
crate::feature::FeatureId::builtin("worker") Err(crate::feature::FeaturePlanError::AmbiguousServiceProvider {
); service: ServiceId::builtin(WORKER_CONTROL_SERVICE_ID),
assert!( providers: vec![
!plan crate::feature::FeatureId::builtin("sub-worker-control"),
.ordered_features() crate::feature::FeatureId::builtin("worker"),
.contains(&crate::feature::FeatureId::builtin("sub-worker-control")) ],
})
); );
let mut hooks = crate::HookRegistryBuilder::default(); let mut hooks = crate::HookRegistryBuilder::default();
let mut pending_tools = Vec::new(); let mut pending_tools = Vec::new();
let report = builder.install_into_pending(&mut pending_tools, &mut hooks); let report = builder.install_into_pending(&mut pending_tools, &mut hooks);
assert!(!report.has_errors(), "{}", report.error_message()); assert!(matches!(
assert_eq!( report.plan_error,
report.services.providers()[&ServiceId::builtin(WORKER_CONTROL_SERVICE_ID)].feature_id, Some(crate::feature::FeaturePlanError::AmbiguousServiceProvider { .. })
crate::feature::FeatureId::builtin("worker") ));
); assert!(pending_tools.is_empty());
assert!(report.services.providers().is_empty());
} }
#[test] #[test]