From f2284f68b45264fd5166b76a4ffed1e6c549d4ec Mon Sep 17 00:00:00 2001
From: jlanson <jlanson@mitre.org>
Date: Wed, 4 Dec 2024 11:39:35 -0500
Subject: [PATCH] fix: graceful closing of other plugins on initialization
 failure

Now that we parse all default expressions from plugins, we discovered
that the `activity` plugin had a malformed expression, and when the
parsing failed and `hc` exited, other plugin processes were left
orphaned. This commit makes sure we use up the JoinSet that performs
plugin initialization, thus trying to initialize all plugins instead of
stopping as soon as the first error occurs. By doing so, we prevent
orphaned processes and report all errors which makes for a better user
experience.

Signed-off-by: jlanson <jlanson@mitre.org>
---
 hipcheck/src/plugin/mod.rs   | 29 ++++++++++++++++++++++++-----
 hipcheck/src/plugin/types.rs | 17 +++++++++++++----
 plugins/activity/src/main.rs |  2 +-
 3 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/hipcheck/src/plugin/mod.rs b/hipcheck/src/plugin/mod.rs
index b7507e75..e152b41b 100644
--- a/hipcheck/src/plugin/mod.rs
+++ b/hipcheck/src/plugin/mod.rs
@@ -8,9 +8,9 @@ mod plugin_manifest;
 mod retrieval;
 mod types;
 
-use crate::error::Result;
 pub use crate::plugin::{get_plugin_key, manager::*, plugin_id::PluginId, types::*};
 use crate::policy_exprs::Expr;
+use crate::{error::Result, hc_error};
 pub use arch::{get_current_arch, try_set_arch, Arch};
 pub use download_manifest::{ArchiveFormat, DownloadManifest, HashAlgorithm, HashWithDigest};
 use hipcheck_common::types::{Query, QueryDirection};
@@ -19,7 +19,7 @@ pub use plugin_manifest::{
 };
 pub use retrieval::retrieve_plugins;
 use serde_json::Value;
-use std::collections::HashMap;
+use std::{collections::HashMap, ops::Not};
 use tokio::sync::Mutex;
 
 pub async fn initialize_plugins(
@@ -34,12 +34,31 @@ pub async fn initialize_plugins(
 		set.spawn(p.initialize(c));
 	}
 
-	let mut out: Vec<PluginTransport> = vec![];
+	let mut inited: Vec<PluginTransport> = vec![];
+	let mut failures: Vec<_> = vec![];
+
 	while let Some(res) = set.join_next().await {
-		out.push(res??);
+		// @Todo - what is the cleanup if the tokio func fails?
+		let init_res = res?;
+
+		// Instead of immediately erroring, we need to finish
+		// initializing all plugins so they shut down properly
+		match init_res {
+			Ok(pt) => inited.push(pt),
+			Err(e) => failures.push(e),
+		};
 	}
 
-	Ok(out)
+	if failures.is_empty().not() {
+		let mut err = "Failures occurred during plugin initialization:".to_owned();
+		for x in failures {
+			err += "\n";
+			err += x.to_string().as_str();
+		}
+		Err(hc_error!("{}", err))
+	} else {
+		Ok(inited)
+	}
 }
 
 #[derive(Debug)]
diff --git a/hipcheck/src/plugin/types.rs b/hipcheck/src/plugin/types.rs
index f0536f99..ccf250d2 100644
--- a/hipcheck/src/plugin/types.rs
+++ b/hipcheck/src/plugin/types.rs
@@ -288,10 +288,19 @@ impl PluginContext {
 
 		self.set_configuration(&config).await?.as_result()?;
 
-		let opt_default_policy_expr = self
-			.get_default_policy_expression()
-			.await?
-			.map(|s| std_parse(s.as_str()))
+		let opt_str = self.get_default_policy_expression().await?;
+		// This is where we turn the `std_parse` error into a user-facing message
+		let opt_default_policy_expr = opt_str
+			.map(|s| {
+				std_parse(s.as_str()).map_err(|e| {
+					hc_error!(
+						"Plugin '{}' has bad default policy expression '{}': {}",
+						self.plugin.name,
+						s,
+						e
+					)
+				})
+			})
 			.transpose()?;
 
 		let opt_explain_default_query = self.explain_default_query().await?;
diff --git a/plugins/activity/src/main.rs b/plugins/activity/src/main.rs
index 83e0050a..11baa04e 100644
--- a/plugins/activity/src/main.rs
+++ b/plugins/activity/src/main.rs
@@ -76,7 +76,7 @@ impl Plugin for ActivityPlugin {
 			return Err(Error::UnspecifiedQueryState);
 		};
 
-		Ok(format!("lte $ P{}w", conf.weeks.unwrap_or(71)))
+		Ok(format!("(lte $ P{}w)", conf.weeks.unwrap_or(71)))
 	}
 
 	fn explain_default_query(&self) -> Result<Option<String>> {