diff --git a/tests/config.rs b/tests/config.rs index 7d495b9..b50fdbc 100644 --- a/tests/config.rs +++ b/tests/config.rs @@ -1,8 +1,8 @@ mod helpers; use helpers::{ - exec_pike, get_picodata_table, init_plugin_workspace, run_cluster, CmdArguments, PLUGIN_DIR, - TESTS_DIR, + exec_pike, get_picodata_table, init_plugin_workspace, run_cluster, Cluster, CmdArguments, + PLUGIN_DIR, TESTS_DIR, }; use rstest::rstest; use std::{ @@ -56,7 +56,7 @@ fn test_config_apply(#[case] params_builder: ApplyParamsBuilder) { let _cluster_handle = run_cluster( Duration::from_secs(120), TOTAL_INSTANCES, - CmdArguments::default(), + &CmdArguments::default(), ) .unwrap(); @@ -86,7 +86,7 @@ fn test_corrupted_config() { let _cluster_handle = run_cluster( Duration::from_secs(120), TOTAL_INSTANCES, - CmdArguments::default(), + &CmdArguments::default(), ) .unwrap(); @@ -175,6 +175,7 @@ fn test_workspace_config_apply() { // Run cluster and check successful plugin installation run(params).unwrap(); + let _cluster = Cluster::manage(&workspace_path); let start = Instant::now(); let mut is_cluster_valid = false; @@ -261,8 +262,6 @@ fn test_workspace_config_apply() { } assert!(is_cluster_valid, "Failed to apply config for one plugin"); - - exec_pike(["stop", "--plugin-path", "workspace_plugin"]); } #[test] diff --git a/tests/enter.rs b/tests/enter.rs index fa731cf..cbc2646 100644 --- a/tests/enter.rs +++ b/tests/enter.rs @@ -13,7 +13,7 @@ fn test_enter_instance() { let _cluster_handle = run_cluster( Duration::from_secs(120), TOTAL_INSTANCES, - CmdArguments::default(), + &CmdArguments::default(), ) .unwrap(); diff --git a/tests/helpers/mod.rs b/tests/helpers/mod.rs index 275b3cb..1fac4d7 100644 --- a/tests/helpers/mod.rs +++ b/tests/helpers/mod.rs @@ -21,6 +21,8 @@ use std::{ use tar::Archive; use toml_edit::{DocumentMut, Item}; +use pike::cluster::StopParamsBuilder; + pub const TESTS_DIR: &str = "./tests/tmp/"; pub const PLUGIN_NAME: &str = "test-plugin"; pub const PLUGIN_DIR: &str = concat!(TESTS_DIR, PLUGIN_NAME); @@ -44,49 +46,138 @@ pub struct CmdArguments { pub run_args: Vec, pub build_args: Vec, pub plugin_args: Vec, - pub stop_args: Vec, } +/// RAII guard that guarantees a running picodata cluster is stopped once the +/// value goes out of scope — both on a normal return and while unwinding from a +/// failed assertion. +/// +/// Tying cleanup to a guard is what makes the integration tests safe: +/// +/// * **No leaked daemons.** Clusters started with `daemon(true)` keep running +/// independently of the returned [`pike::cluster::PicodataInstance`] handles +/// (their `Drop` is a no-op in daemon mode). Holding a `Cluster` guard means a +/// panicking test can no longer leave picodata processes behind to collide +/// with the next test over ports and sockets. +/// +/// * **No panic-in-panic aborts.** [`Cluster::drop`] never panics: it stops the +/// cluster through the library `pike::cluster::stop` entry point and only logs +/// failures. A `Drop` that panicked while a test was already unwinding would +/// abort the whole test process — the spurious "segfault"-looking crash we are +/// eliminating here. pub struct Cluster { + /// Path to the plugin project whose cluster this guard owns. + plugin_path: PathBuf, + /// Data directory (relative to `plugin_path`) that holds `cluster/`. + data_dir: PathBuf, + /// Foreground `cargo-pike run` process to reap, when the cluster was started + /// through a subprocess (see [`run_cluster`]). `None` for clusters started + /// in-process via `pike::cluster::run`. run_handler: Option, - pub cmd_args: CmdArguments, } -impl Drop for Cluster { - fn drop(&mut self) { - let mut args = vec!["stop", "--plugin-path", PLUGIN_NAME]; - args.extend(self.cmd_args.stop_args.iter().map(String::as_str)); - exec_pike(args); +impl Cluster { + /// Take ownership of cleanup for a cluster started in-process (e.g. via + /// `pike::cluster::run`) under `plugin_path`, using the default `./tmp` data + /// directory. + /// + /// Call this right after the cluster is started so that any later panic + /// still triggers cleanup: + /// + /// ```ignore + /// run(params).unwrap(); + /// let _cluster = Cluster::manage(plugin_path); + /// // ... assertions that may panic ... + /// ``` + pub fn manage(plugin_path: impl Into) -> Cluster { + Cluster::manage_with_data_dir(plugin_path, "./tmp") + } - if let Some(ref mut run_handler) = self.run_handler { - run_handler.wait().unwrap(); + /// Like [`Cluster::manage`], but with an explicit data directory. + pub fn manage_with_data_dir( + plugin_path: impl Into, + data_dir: impl Into, + ) -> Cluster { + Cluster { + plugin_path: plugin_path.into(), + data_dir: data_dir.into(), + run_handler: None, } } -} -impl Cluster { - fn new(run_params: CmdArguments) -> Cluster { - info!("cleaning artefacts from previous run"); + fn set_run_handler(&mut self, handler: Child) { + self.run_handler = Some(handler); + } - match fs::remove_file(Path::new(TESTS_DIR).join("instance.log")) { - Ok(()) => info!("Clearing logs."), - Err(e) if e.kind() == ErrorKind::NotFound => { - info!("instance.log not found, skipping cleanup"); + /// Stop the cluster without ever panicking. Errors are logged instead of + /// propagated, so this is safe to call from `Drop` during unwinding. + /// + /// Stopping is idempotent: instances without an active admin socket are + /// skipped, so re-stopping an already stopped cluster is a no-op. + fn stop_quietly(&self) { + let params = match StopParamsBuilder::default() + .plugin_path(self.plugin_path.clone()) + .data_dir(self.data_dir.clone()) + .build() + { + Ok(params) => params, + Err(e) => { + eprintln!("[cluster guard] failed to build stop params: {e:#}"); + return; } - Err(e) => panic!("failed to delete instance.log: {e}"), - } + }; - Cluster { - run_handler: None, - cmd_args: run_params, + // A cluster that never came up (missing data dir) or was already + // stopped is a perfectly fine state to land in here — just log it. + // + // `pike::cluster::stop` is expected to only ever return errors, but it + // still holds a couple of internal `unwrap`/`assert!`s. Run it inside + // `catch_unwind` so that even a latent library panic cannot turn this + // cleanup — which may execute while a test is already unwinding — into a + // panic-in-panic process abort. + let stop_outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + pike::cluster::stop(¶ms) + })); + match stop_outcome { + Ok(Ok(())) => {} + Ok(Err(e)) => eprintln!( + "[cluster guard] could not stop cluster at {}: {e:#}", + self.plugin_path.display() + ), + Err(_) => eprintln!( + "[cluster guard] stopping cluster at {} panicked; ignored during cleanup", + self.plugin_path.display() + ), } } +} - fn set_run_handler(&mut self, handler: Child) { - self.run_handler = Some(handler); +impl Drop for Cluster { + fn drop(&mut self) { + // Stop the instances first: a foreground `cargo-pike run` exits on its + // own once the picodata children we just killed are gone. + self.stop_quietly(); + + if let Some(mut run_handler) = self.run_handler.take() { + // Never unwrap while (possibly) unwinding. `kill` is a safety net in + // case the runner is still alive; both calls are allowed to fail. + let _ = run_handler.kill(); + let _ = run_handler.wait(); + } } } +/// Extract the `--data-dir` value from `pike run` arguments, defaulting to the +/// `./tmp` directory that pike's CLI uses when the flag is absent (mirrors the +/// `default_value` of `pike run`/`pike stop`). +fn data_dir_from_args(run_args: &[String]) -> PathBuf { + run_args + .iter() + .position(|arg| arg == "--data-dir") + .and_then(|index| run_args.get(index + 1)) + .map_or_else(|| PathBuf::from("./tmp"), PathBuf::from) +} + pub struct TestPluginInitParams where A: AsRef + std::fmt::Debug, @@ -324,28 +415,35 @@ pub fn build_plugin(build_type: &BuildType, new_version: &str, plugin_path: &Pat pub fn run_cluster( timeout: Duration, total_instances: i32, - cmd_args: CmdArguments, + cmd_args: &CmdArguments, ) -> Result { - // Set up cleanup function - let mut cluster_handle = Cluster::new(cmd_args); + info!("cleaning artefacts from previous run"); + match fs::remove_file(Path::new(TESTS_DIR).join("instance.log")) { + Ok(()) => info!("Clearing logs."), + Err(e) if e.kind() == ErrorKind::NotFound => { + info!("instance.log not found, skipping cleanup"); + } + Err(e) => panic!("failed to delete instance.log: {e}"), + } - // Create plugin from template - let mut args = cluster_handle - .cmd_args - .plugin_args - .iter() - .map(String::as_str); + // Data directory (relative to the plugin dir) the cluster will live in. + let data_dir = data_dir_from_args(&cmd_args.run_args); + // Acquire the RAII guard up-front: from here on, every early return or panic + // stops the cluster and reaps the runner through `Cluster::drop`. + let mut cluster_handle = Cluster::manage_with_data_dir(PLUGIN_DIR, &data_dir); + + // Create plugin from template init_plugin_with_args(TestPluginInitParams { name: "test-plugin".to_string(), - init_args: args.collect(), + init_args: cmd_args.plugin_args.iter().map(String::as_str).collect(), ..Default::default() }); // Build the plugin Command::new("cargo") .arg("build") - .args(&cluster_handle.cmd_args.build_args) + .args(&cmd_args.build_args) .current_dir(PLUGIN_DIR) .output()?; @@ -354,7 +452,7 @@ pub fn run_cluster( let run_handler = Command::new(format!("{root_dir}/target/debug/cargo-pike")) .arg("pike") .arg("run") - .args(&cluster_handle.cmd_args.run_args) + .args(&cmd_args.run_args) .current_dir(PLUGIN_DIR) .spawn() .unwrap(); @@ -364,20 +462,9 @@ pub fn run_cluster( // Run in the loop until we get info about successful plugin installation loop { - // Get path to data dir from cmd_args - let cur_run_args = &cluster_handle.cmd_args.run_args; - let mut data_dir_path = Path::new("tmp"); - if let Some(index) = cur_run_args.iter().position(|x| x == "--data-dir") { - if index + 1 < cur_run_args.len() { - data_dir_path = Path::new(&cur_run_args[index + 1]); - } - } // Check if cluster set up correctly - let mut picodata_admin = await_picodata_admin( - Duration::from_secs(60), - Path::new(PLUGIN_DIR), - data_dir_path, - )?; + let mut picodata_admin = + await_picodata_admin(Duration::from_secs(60), Path::new(PLUGIN_DIR), &data_dir)?; let stdout = picodata_admin .stdout .take() diff --git a/tests/pre_install_sql.rs b/tests/pre_install_sql.rs index 32f6ce5..bde44f8 100644 --- a/tests/pre_install_sql.rs +++ b/tests/pre_install_sql.rs @@ -1,6 +1,6 @@ mod helpers; -use helpers::{get_picodata_table, init_plugin, PLUGIN_DIR, PLUGIN_NAME}; +use helpers::{get_picodata_table, init_plugin, Cluster, PLUGIN_DIR, PLUGIN_NAME}; use std::{ collections::BTreeMap, path::Path, @@ -49,6 +49,7 @@ fn test_pre_install_sql_execution() { .unwrap(); let _instances = run(params).expect("Cluster run failed"); + let _cluster = Cluster::manage(plugin_path); let start = Instant::now(); let mut check_passed = false; @@ -68,14 +69,6 @@ fn test_pre_install_sql_execution() { std::thread::sleep(Duration::from_secs(1)); } - pike::cluster::stop( - &pike::cluster::StopParamsBuilder::default() - .plugin_path(plugin_path.to_path_buf()) - .build() - .unwrap(), - ) - .unwrap(); - assert!( check_passed, "Pre-install SQL scripts were not executed or data is missing" diff --git a/tests/run.rs b/tests/run.rs index 52d58a9..a696e91 100644 --- a/tests/run.rs +++ b/tests/run.rs @@ -4,7 +4,9 @@ use helpers::{ build_plugin, cleanup_dir, exec_pike, exec_pike_in, get_picodata_table, init_plugin, init_plugin_with_args, init_plugin_workspace, run_cluster, wait_cluster_start_completed, }; -use helpers::{CmdArguments, TestPluginInitParams, LIB_EXT, PLUGIN_DIR, PLUGIN_NAME, TESTS_DIR}; +use helpers::{ + Cluster, CmdArguments, TestPluginInitParams, LIB_EXT, PLUGIN_DIR, PLUGIN_NAME, TESTS_DIR, +}; use pike::cluster::{run, MigrationContextVar, Plugin, RunParamsBuilder, Service, Tier, Topology}; use std::collections::BTreeMap; use std::fs::OpenOptions; @@ -61,7 +63,7 @@ fn test_cluster_setup_debug() { let _cluster_handle = run_cluster( Duration::from_secs(120), TOTAL_INSTANCES, - CmdArguments::default(), + &CmdArguments::default(), ) .unwrap(); } @@ -73,15 +75,11 @@ fn test_cluster_setup_release() { .iter() .map(|&s| s.into()) .collect(), - stop_args: ["--data-dir", "new_data_dir"] - .iter() - .map(|&s| s.into()) - .collect(), ..Default::default() }; let _cluster_handle = - run_cluster(Duration::from_secs(120), TOTAL_INSTANCES, run_params).unwrap(); + run_cluster(Duration::from_secs(120), TOTAL_INSTANCES, &run_params).unwrap(); } // Using as much command line arguments in this test as we can @@ -107,11 +105,10 @@ fn test_cluster_daemon_and_arguments() { .map(|&s| s.into()) .collect(), plugin_args: vec!["--workspace".to_string()], - ..Default::default() }; let _cluster_handle = - run_cluster(Duration::from_secs(120), TOTAL_INSTANCES, run_params).unwrap(); + run_cluster(Duration::from_secs(120), TOTAL_INSTANCES, &run_params).unwrap(); // Validate each instances's PID for entry in fs::read_dir(Path::new(PLUGIN_DIR).join("tmp").join("cluster")).unwrap() { @@ -173,6 +170,8 @@ fn test_topology_struct_run() { .unwrap(); run(params).unwrap(); + // RAII cleanup: stops the cluster on scope exit, including a panic below. + let _cluster = Cluster::manage(plugin_path); let start = Instant::now(); let mut cluster_started = false; @@ -188,8 +187,6 @@ fn test_topology_struct_run() { } } - exec_pike(["stop", "--plugin-path", PLUGIN_NAME]); - assert!(cluster_started); } @@ -237,6 +234,10 @@ fn test_multiple_run_attempt_are_idempotent() { .build() .unwrap(); + // The guard is acquired before the first run; the cluster persists across + // idempotent re-runs and is stopped once when the test ends or panics. + let _cluster = Cluster::manage(plugin_path); + // Execute pike run twice to ensure sequential runs // are idempotent. for _ in 0..1 { @@ -247,8 +248,6 @@ fn test_multiple_run_attempt_are_idempotent() { true })); } - - exec_pike(["stop", "--plugin-path", PLUGIN_NAME]); } #[test] @@ -328,6 +327,7 @@ fn test_topology_struct_one_tier() { .unwrap(); run(params).unwrap(); + let _cluster = Cluster::manage(plugin_path); let start = Instant::now(); let mut cluster_started = false; @@ -343,8 +343,6 @@ fn test_topology_struct_one_tier() { } } - exec_pike(["stop", "--plugin-path", PLUGIN_NAME]); - assert!(cluster_started); } @@ -375,6 +373,7 @@ fn test_topology_struct_run_no_plugin() { .unwrap(); run(params).unwrap(); + let _cluster = Cluster::manage(plugin_path); let start = Instant::now(); let mut cluster_started = false; @@ -389,8 +388,6 @@ fn test_topology_struct_run_no_plugin() { } } - exec_pike(["stop", "--plugin-path", PLUGIN_NAME]); - assert!(cluster_started); } @@ -440,6 +437,8 @@ fn test_picodata_instance_interaction() { .unwrap(); let pico_instances = run(params).unwrap(); + let _cluster = Cluster::manage(plugin_path); + let properties = pico_instances.first().unwrap().properties(); let data_dir = properties.data_dir.to_str().unwrap(); @@ -454,8 +453,6 @@ fn test_picodata_instance_interaction() { Path::new(data_dir).join("audit.log").to_str().unwrap(), "./tests/tmp/test-plugin/./tmp/cluster/i1/audit.log" ); - - exec_pike(["stop", "--plugin-path", PLUGIN_NAME]); } #[test] @@ -503,6 +500,7 @@ fn test_quickstart_pipeline() { // Run cluster and check successful plugin installation run(params).unwrap(); + let _cluster = Cluster::manage(&quickstart_path); let start = Instant::now(); let mut cluster_started = false; @@ -591,6 +589,7 @@ fn test_workspace_pipeline() { // Run cluster and check successful plugin installation run(params).unwrap(); + let _cluster = Cluster::manage(&workspace_path); let start = Instant::now(); let mut cluster_started = false; @@ -681,6 +680,7 @@ fn test_run_without_plugin_directory() { .unwrap(); run(params).unwrap(); + let _cluster = Cluster::manage(&plugin_dir); let start = Instant::now(); let mut cluster_started = false; @@ -697,8 +697,6 @@ fn test_run_without_plugin_directory() { thread::sleep(Duration::from_secs(1)); } - exec_pike(["stop", "--plugin-path", "test_run_without_plugin_directory"]); - assert!(cluster_started); } @@ -713,7 +711,7 @@ fn test_run_with_several_tiers() { ..Default::default() }; - let _cluster_handle = run_cluster(Duration::from_secs(120), 6, run_params).unwrap(); + let _cluster_handle = run_cluster(Duration::from_secs(120), 6, &run_params).unwrap(); let start = Instant::now(); let mut cluster_started = false; @@ -841,6 +839,7 @@ fn run_with_external_plugin_directory() { .unwrap(); run(params).unwrap(); + let _cluster = Cluster::manage(our_plugin_path); let cluster_started = wait_cluster_start_completed(our_plugin_path, |state| { assert_eq!(state.pico_instance.matches("Online").count(), 8); @@ -848,8 +847,6 @@ fn run_with_external_plugin_directory() { true }); - exec_pike(["stop", "--plugin-path", PLUGIN_NAME]); - assert!(cluster_started); } @@ -892,6 +889,7 @@ fn run_with_external_plugin_archive() { .unwrap(); run(params).unwrap(); + let _cluster = Cluster::manage(Path::new("./tests/tmp/test-plugin")); let cluster_started = wait_cluster_start_completed(Path::new("./tests/tmp/test-plugin"), |state| { @@ -900,8 +898,6 @@ fn run_with_external_plugin_archive() { true }); - exec_pike(["stop", "--plugin-path", PLUGIN_NAME]); - assert!(cluster_started); } @@ -939,6 +935,7 @@ fn run_with_external_plugin_project() { .unwrap(); run(params).unwrap(); + let _cluster = Cluster::manage(our_plugin_path); let cluster_started = wait_cluster_start_completed(our_plugin_path, |state| { assert_eq!(state.pico_instance.matches("Online").count(), 8); @@ -946,8 +943,6 @@ fn run_with_external_plugin_project() { true }); - exec_pike(["stop", "--plugin-path", PLUGIN_NAME]); - assert!(cluster_started); } @@ -987,6 +982,7 @@ fn run_with_external_plugin_workspace() { let params = make_ext_run_params(plugin_path, plugins).build().unwrap(); run(params).unwrap(); + let _cluster = Cluster::manage(plugin_path); let cluster_started = wait_cluster_start_completed(plugin_path, |state| { assert_eq!(state.pico_instance.matches("Online").count(), 8); @@ -994,8 +990,6 @@ fn run_with_external_plugin_workspace() { true }); - exec_pike(["stop", "--plugin-path", PLUGIN_NAME]); - assert!(cluster_started); } @@ -1008,7 +1002,7 @@ fn run_specific_instance() { let _cluster_handle = run_cluster( Duration::from_secs(120), TOTAL_INSTANCES, - CmdArguments::default(), + &CmdArguments::default(), ) .unwrap(); @@ -1090,8 +1084,6 @@ fn run_specific_instance() { true }); - exec_pike(["stop", "--plugin-path", PLUGIN_NAME]); - assert!(cluster_started); } @@ -1105,7 +1097,7 @@ fn revive_terminated_instances() { let _cluster_handle = run_cluster( Duration::from_secs(120), TOTAL_INSTANCES, - CmdArguments::default(), + &CmdArguments::default(), ) .expect("Failed to run the cluster"); @@ -1138,7 +1130,6 @@ fn revive_terminated_instances() { }); assert!(cluster_started); - exec_pike(["stop", "--plugin-path", PLUGIN_NAME]); } #[test] @@ -1200,6 +1191,8 @@ fn run_with_env_variables() { .unwrap(); let pico_instances = run(params).unwrap(); + let _cluster = Cluster::manage(plugin_path); + let properties = pico_instances.first().unwrap().properties(); assert_eq!(properties.bin_port, &3301); @@ -1212,8 +1205,6 @@ fn run_with_env_variables() { properties.data_dir.to_str().unwrap(), "./tests/tmp/test-plugin/./tmp/cluster/i1" ); - - exec_pike(["stop", "--plugin-path", PLUGIN_NAME]); } #[test] @@ -1222,7 +1213,7 @@ fn run_with_wait_vshard_discovery() { let _cluster_handle = run_cluster( Duration::from_secs(360), TOTAL_INSTANCES, - CmdArguments { + &CmdArguments { run_args: vec![ "--wait-vshard-discovery".to_string(), "--wait-vshard-discovery-timeout".to_string(), diff --git a/tests/stop.rs b/tests/stop.rs index a2518eb..28da649 100644 --- a/tests/stop.rs +++ b/tests/stop.rs @@ -48,7 +48,7 @@ fn test_pike_stop_default() { let _cluster_handle = run_cluster( CLUSTER_START_TIMEOUT, TOTAL_INSTANCES, - CmdArguments::default(), + &CmdArguments::default(), ) .unwrap(); @@ -64,7 +64,7 @@ fn test_pike_stop_daemon_cluster() { run_args: ["--daemon"].iter().map(|&s| s.into()).collect(), ..Default::default() }; - let _cluster_handle = run_cluster(CLUSTER_START_TIMEOUT, TOTAL_INSTANCES, cmd_args) + let _cluster_handle = run_cluster(CLUSTER_START_TIMEOUT, TOTAL_INSTANCES, &cmd_args) .expect("Failed to start cluster"); // Stop picodata cluster @@ -78,7 +78,7 @@ fn test_pike_stop_sigterm_with_timeout() { let _cluster_handle = run_cluster( CLUSTER_START_TIMEOUT, TOTAL_INSTANCES, - CmdArguments::default(), + &CmdArguments::default(), ) .expect("Failed to start cluster"); @@ -104,7 +104,7 @@ fn test_pike_stop_of_specific_instance() { let _cluster_handle = run_cluster( Duration::from_secs(120), TOTAL_INSTANCES, - CmdArguments::default(), + &CmdArguments::default(), ) .unwrap();