From b80ebf61fb4773718476ddf9e84585df97fb9213 Mon Sep 17 00:00:00 2001 From: sdairs Date: Mon, 24 Aug 2026 21:02:38 +0100 Subject: [PATCH 01/16] Make local server metadata atomic --- crates/clickhousectl/src/error.rs | 36 + crates/clickhousectl/src/local/docker.rs | 16 +- crates/clickhousectl/src/local/mod.rs | 83 +- crates/clickhousectl/src/local/output.rs | 56 ++ crates/clickhousectl/src/local/postgres.rs | 82 +- crates/clickhousectl/src/local/server.rs | 753 +++++++++++++++--- .../src/version_manager/install.rs | 10 +- .../tests/local_server_metadata_test.rs | 145 ++++ 8 files changed, 965 insertions(+), 216 deletions(-) create mode 100644 crates/clickhousectl/tests/local_server_metadata_test.rs diff --git a/crates/clickhousectl/src/error.rs b/crates/clickhousectl/src/error.rs index edec1ed8..de74f462 100644 --- a/crates/clickhousectl/src/error.rs +++ b/crates/clickhousectl/src/error.rs @@ -132,6 +132,42 @@ pub enum Error { #[error("Server '{0}' not found")] ServerNotFound(String), + #[error( + "Could not read metadata for server '{name}' at .clickhouse/servers/{name}.json: {source}. Check that the file is readable and retry." + )] + ServerMetadataRead { + name: String, + #[source] + source: std::io::Error, + }, + + #[error( + "Permission denied accessing metadata for server '{name}' at .clickhouse/servers/{name}.json. Restore access to the file and its parent directory, then retry." + )] + ServerMetadataPermission { + name: String, + #[source] + source: std::io::Error, + }, + + #[error( + "Metadata for server '{name}' at .clickhouse/servers/{name}.json is invalid: {source}. Repair or remove the metadata file, then retry." + )] + ServerMetadataParse { + name: String, + #[source] + source: serde_json::Error, + }, + + #[error( + "Could not update metadata for server '{name}' at .clickhouse/servers/{name}.json: {source}. Check write access to .clickhouse/servers and retry." + )] + ServerMetadataWrite { + name: String, + #[source] + source: std::io::Error, + }, + #[error( "No server name was provided and multiple non-default ClickHouse servers exist. Pass a name or run `clickhousectl local server stop-all`; use `clickhousectl local server list` to see available servers." )] diff --git a/crates/clickhousectl/src/local/docker.rs b/crates/clickhousectl/src/local/docker.rs index e84eb950..99b12619 100644 --- a/crates/clickhousectl/src/local/docker.rs +++ b/crates/clickhousectl/src/local/docker.rs @@ -921,13 +921,12 @@ pub fn start_existing_blocking(id: &str) -> Result<()> { /// Safe to call multiple times in one CLI invocation. When Docker isn't /// reachable, `connect()` fails fast (no socket → immediate error, no I/O /// timeout) and we return silently. -pub fn recover_project_postgres_blocking(project_cwd: &str) { +pub fn recover_project_postgres_blocking(project_cwd: &str) -> Result<()> { use crate::local::server::{ - Engine, ServerInfo, ensure_pg_data_dir, pg_instance_key, save_server_info, - server_meta_path_for_recovery, + Engine, ServerInfo, ensure_pg_data_dir, pg_instance_key, save_recovered_server_info, }; let cwd_owned = project_cwd.to_string(); - let _ = block_on(async move { + block_on(async move { let docker = match connect().await { Ok(d) => d, Err(_) => return Ok::<(), Error>(()), @@ -938,10 +937,7 @@ pub fn recover_project_postgres_blocking(project_cwd: &str) { }; for c in containers { let key = pg_instance_key(&c.user_name, &c.major); - if server_meta_path_for_recovery(&key).exists() { - continue; - } - let _ = ensure_pg_data_dir(&c.user_name, &c.major); + ensure_pg_data_dir(&c.user_name, &c.major)?; let info = ServerInfo { name: key, pid: 0, @@ -953,10 +949,10 @@ pub fn recover_project_postgres_blocking(project_cwd: &str) { engine: Engine::Postgres, container_id: Some(c.container_id.clone()), }; - let _ = save_server_info(&info); + save_recovered_server_info(&info, false)?; } Ok(()) - }); + }) } #[cfg(test)] diff --git a/crates/clickhousectl/src/local/mod.rs b/crates/clickhousectl/src/local/mod.rs index ea71f298..318aa4f2 100644 --- a/crates/clickhousectl/src/local/mod.rs +++ b/crates/clickhousectl/src/local/mod.rs @@ -194,8 +194,8 @@ fn remove(version: &str, force: bool, json: bool) -> Result<()> { // Recover orphaned servers so we detect a running process even when its // metadata file is missing, then refuse to pull the binary out from under // a server running on this version. - server::recover_current_project_servers(); - let in_use: Vec = server::list_running_servers() + server::recover_current_project_servers()?; + let in_use: Vec = server::list_running_servers()? .into_iter() .filter(|i| i.version == version) .map(|i| i.name) @@ -271,19 +271,15 @@ fn run_client( (h, p, v) } else { let server_name = name.as_deref().unwrap_or("default"); - let entries = server::list_all_servers(); - let entry = entries - .iter() - .find(|e| e.name == server_name) + server::recover_current_project_servers()?; + let lock = server::ServerLock::acquire(server_name)?; + let info = lock + .load_info()? .ok_or_else(|| Error::ServerNotFound(server_name.to_string()))?; - if !entry.running { + if !lock.is_running()? { return Err(Error::ServerNotRunning(server_name.to_string())); } - let info = entry - .info - .as_ref() - .ok_or_else(|| Error::ServerNotRunning(server_name.to_string()))?; - ("localhost".to_string(), info.tcp_port, info.version.clone()) + ("localhost".to_string(), info.tcp_port, info.version) }; let binary = paths::binary_path(&version)?; @@ -351,12 +347,12 @@ async fn start_server( // Recover any orphaned servers so name resolution and collision checks // see processes that lost their metadata files. - server::recover_current_project_servers(); + server::recover_current_project_servers()?; // Resolve server name and check for collisions before any downloads let server_name = server::resolve_name(name.as_deref())?; - if name.is_some() && server::is_server_running(&server_name) { + if name.is_some() && server::is_server_running(&server_name)? { return Err(Error::ServerAlreadyRunning(server_name)); } @@ -395,7 +391,7 @@ async fn start_server( } // Show running server count - let running = server::running_server_count(); + let running = server::running_server_count()?; if !json && running > 0 { eprintln!( "Note: {} server{} already running (use `clickhousectl local server list` to see them)", @@ -441,6 +437,10 @@ async fn start_server( let mut cmd = Command::new(&binary); cmd.arg("server"); + let server_lock = server::ServerLock::acquire(&server_name)?; + if server_lock.is_running()? { + return Err(Error::ServerAlreadyRunning(server_name)); + } server::ensure_server_data_dir(&server_name)?; let data_dir = server::server_data_dir(&server_name); @@ -487,7 +487,12 @@ async fn start_server( engine: server::Engine::Clickhouse, container_id: None, }; - server::save_server_info(&info)?; + if let Err(error) = server_lock.save_info(&info) { + let _ = child.kill(); + let _ = child.wait(); + return Err(error); + } + drop(server_lock); if no_wait { server::check_spawn_health(&mut child, &server_name, &log_path).await?; @@ -527,7 +532,12 @@ async fn start_server( engine: server::Engine::Clickhouse, container_id: None, }; - server::save_server_info(&info)?; + if let Err(error) = server_lock.save_info(&info) { + let _ = child.kill(); + let _ = child.wait(); + return Err(error); + } + drop(server_lock); eprintln!( "Server '{}' running (PID: {}, HTTP: {}, TCP: {})", @@ -565,18 +575,14 @@ fn dotenv_server( json: bool, ) -> Result<()> { let server_name = name.unwrap_or("default"); - let entries = server::list_all_servers(); - let entry = entries - .iter() - .find(|e| e.name == server_name) + server::recover_current_project_servers()?; + let lock = server::ServerLock::acquire(server_name)?; + let info = lock + .load_info()? .ok_or_else(|| Error::ServerNotFound(server_name.to_string()))?; - if !entry.running { + if !lock.is_running()? { return Err(Error::ServerNotRunning(server_name.to_string())); } - let info = entry - .info - .as_ref() - .ok_or_else(|| Error::ServerNotRunning(server_name.to_string()))?; // Only write vars we actually know from server metadata. // User, password, and database are only included when explicitly provided. @@ -770,7 +776,7 @@ async fn run_server_commands(command: ServerCommands, json: bool) -> Result<()> fn stop_server_local(name: Option, json: bool) -> Result<()> { let name = match name { Some(name) => name, - None => match select_omitted_stop_name(&server::list_clickhouse_server_names())? { + None => match select_omitted_stop_name(&server::list_clickhouse_server_names()?)? { Some(name) => name, None => { output::print_output(&output::ServerStopNoopOutput::no_servers(), json); @@ -782,17 +788,15 @@ fn stop_server_local(name: Option, json: bool) -> Result<()> { // Explicit names do not enumerate first, so recover orphaned processes // before checking the selected identity. - server::recover_current_project_servers(); + server::recover_current_project_servers()?; + let lock = server::ServerLock::acquire(&name)?; - match classify_stop( - server::is_server_running(&name), - server::server_data_dir(&name).exists(), - ) { + match classify_stop(lock.is_running()?, server::server_data_dir(&name).exists()) { StopOutcome::Stop => { if !json { println!("Stopping server '{}'...", name); } - server::kill_server(&name)?; + lock.kill()?; let out = output::ServerStopOutput { name, already_stopped: false, @@ -830,7 +834,7 @@ fn remove_server_local(name: Option, json: bool) -> Result<()> { Some(name) => name, None if server::server_data_dir("default").exists() => "default".to_string(), None => { - let has_custom = server::list_clickhouse_server_names() + let has_custom = server::list_clickhouse_server_names()? .iter() .any(|name| name != "default"); return Err(if has_custom { @@ -844,9 +848,10 @@ fn remove_server_local(name: Option, json: bool) -> Result<()> { // Recover orphaned servers so we correctly detect a running process even // when its metadata file is missing. - server::recover_current_project_servers(); + server::recover_current_project_servers()?; + let lock = server::ServerLock::acquire(&name)?; - if server::is_server_running(&name) { + if lock.is_running()? { return Err(Error::ServerRunningCannotRemove(name)); } let data_dir = server::server_data_dir(&name); @@ -856,7 +861,7 @@ fn remove_server_local(name: Option, json: bool) -> Result<()> { // Remove the whole server directory (parent of data/). let server_dir = data_dir.parent().unwrap(); std::fs::remove_dir_all(server_dir)?; - server::remove_server_info(&name); + lock.remove_info()?; let out = output::ServerRemoveOutput { name }; output::print_output(&out, json); Ok(()) @@ -883,7 +888,7 @@ fn classify_stop(running: bool, exists_on_disk: bool) -> StopOutcome { } fn list_servers_local(json: bool) -> Result<()> { - let entries = server::list_all_servers(); + let entries = server::list_all_servers()?; let running_count = entries.iter().filter(|e| e.running).count(); let total = entries.len(); @@ -1026,7 +1031,7 @@ fn stop_server_global(name: &str, project: Option<&str>, json: bool) -> Result<( } fn stop_all_servers_local(json: bool) -> Result<()> { - let servers = server::list_running_servers(); + let servers = server::list_running_servers()?; let out = stop_servers(&servers, json, server::kill_server); if json { output::print_output(&out, json); diff --git a/crates/clickhousectl/src/local/output.rs b/crates/clickhousectl/src/local/output.rs index 4a5b717f..f0ba903e 100644 --- a/crates/clickhousectl/src/local/output.rs +++ b/crates/clickhousectl/src/local/output.rs @@ -15,6 +15,10 @@ pub enum LocalErrorCode { ServerNotFound, ServerNotRunning, ServerRunning, + ServerMetadataRead, + ServerMetadataPermission, + ServerMetadataInvalid, + ServerMetadataWrite, InvalidVersion, VersionUnavailable, PortInUse, @@ -60,6 +64,26 @@ impl LocalErrorOutput { error.to_string(), "clickhousectl local server list", ), + Error::ServerMetadataRead { .. } => ( + LocalErrorCode::ServerMetadataRead, + error.to_string(), + "clickhousectl local server list", + ), + Error::ServerMetadataPermission { .. } => ( + LocalErrorCode::ServerMetadataPermission, + error.to_string(), + "clickhousectl local server list", + ), + Error::ServerMetadataParse { .. } => ( + LocalErrorCode::ServerMetadataInvalid, + error.to_string(), + "clickhousectl local server list", + ), + Error::ServerMetadataWrite { .. } => ( + LocalErrorCode::ServerMetadataWrite, + error.to_string(), + "clickhousectl local server list", + ), Error::InvalidVersion(_) => ( LocalErrorCode::InvalidVersion, error.to_string(), @@ -772,6 +796,38 @@ mod tests { LocalErrorCode::ServerRunning, "clickhousectl local server list", ), + ( + Error::ServerMetadataRead { + name: "default".into(), + source: std::io::Error::other("read failed"), + }, + LocalErrorCode::ServerMetadataRead, + "clickhousectl local server list", + ), + ( + Error::ServerMetadataPermission { + name: "default".into(), + source: std::io::Error::from(std::io::ErrorKind::PermissionDenied), + }, + LocalErrorCode::ServerMetadataPermission, + "clickhousectl local server list", + ), + ( + Error::ServerMetadataParse { + name: "default".into(), + source: serde_json::from_str::("{").unwrap_err(), + }, + LocalErrorCode::ServerMetadataInvalid, + "clickhousectl local server list", + ), + ( + Error::ServerMetadataWrite { + name: "default".into(), + source: std::io::Error::other("write failed"), + }, + LocalErrorCode::ServerMetadataWrite, + "clickhousectl local server list", + ), ( Error::InvalidVersion("invalid version".into()), LocalErrorCode::InvalidVersion, diff --git a/crates/clickhousectl/src/local/postgres.rs b/crates/clickhousectl/src/local/postgres.rs index ac74d792..fe0a9cf0 100644 --- a/crates/clickhousectl/src/local/postgres.rs +++ b/crates/clickhousectl/src/local/postgres.rs @@ -106,13 +106,13 @@ async fn start( password_from_env, } = preflight_start_options(name.as_deref(), version.as_deref(), port, extra_env)?; - server::recover_current_project_servers(); + server::recover_current_project_servers()?; // User-facing name (no version suffix). Defaults to "default" when no // postgres "default" is currently running. let user_name = match name.as_deref() { Some(n) => n.to_string(), - None => default_pg_name(), + None => default_pg_name()?, }; // If `--version` is omitted but there's already exactly one instance for @@ -123,7 +123,7 @@ async fn start( let (tag, major) = match version.as_deref() { Some(v) => (v.to_string(), pg_major_from_tag(v)), None => { - let existing = server::find_pg_instances(&user_name); + let existing = server::find_pg_instances(&user_name)?; match existing.len() { 0 => ( DEFAULT_PG_TAG.to_string(), @@ -154,6 +154,7 @@ async fn start( // Disk identifier — uniquely scopes (name, major) so two majors of the // same name never share container/data/metadata. let key = server::pg_instance_key(&user_name, &major); + let metadata_lock = server::ServerLock::acquire(&key)?; let docker = docker::connect().await?; @@ -163,12 +164,12 @@ async fn start( .unwrap_or_default(); // Resume path: an instance for this exact (name, major) already exists. - if let Some(prior) = server::load_info(&key) { + if let Some(prior) = metadata_lock.load_info()? { let cid = prior.container_id.as_deref().unwrap_or(""); let container_present = !cid.is_empty() && docker.inspect_container(cid, None).await.is_ok(); if container_present { - if server::is_server_running(&key) { + if metadata_lock.is_running()? { return Err(Error::ServerAlreadyRunning(user_name)); } if !json @@ -184,7 +185,7 @@ async fn start( user_name, user_name ); } - return resume_existing(&docker, prior, json).await; + return resume_existing(&docker, prior, &metadata_lock, json).await; } // Metadata orphaned — container removed externally. Force explicit // cleanup to avoid silently re-initing against potentially-stale data. @@ -247,16 +248,15 @@ async fn start( engine: Engine::Postgres, container_id: Some(container_id.clone()), }; - let rollback = FreshStartRollback { container_id: container_id.clone(), - metadata_path: server::server_meta_path_for_recovery(&key), + metadata_path: metadata_lock.metadata_path(), instance_dir, remove_fresh_data_on_failure, }; finish_fresh_start(&docker, rollback, async { docker::start_container(&docker, &container_id).await?; - server::save_server_info(&info)?; + metadata_lock.save_info(&info)?; wait_for_postgres_ready( &docker, &container_id, @@ -388,8 +388,8 @@ async fn rollback_fresh_start( /// Default user-facing name when `--name` is omitted: `"default"` if no /// postgres "default" is running, otherwise a random adjective-noun. -fn default_pg_name() -> String { - let any_default_running = server::find_pg_instances("default").iter().any(|i| { +fn default_pg_name() -> Result { + let any_default_running = server::find_pg_instances("default")?.iter().any(|i| { i.container_id .as_deref() .map(docker::is_container_running_blocking) @@ -398,9 +398,9 @@ fn default_pg_name() -> String { if any_default_running { // Fall back to the existing random-name generator, which checks // metadata file uniqueness across engines. - server::resolve_name(None).unwrap_or_else(|_| "default".into()) + server::resolve_name(None) } else { - "default".into() + Ok("default".into()) } } @@ -412,11 +412,11 @@ fn resolve_pg_target(user_name: &str, version: Option<&str>) -> Result Err(Error::ServerNotFound(user_name.to_string())), 1 => Ok(instances.into_iter().next().unwrap()), @@ -434,7 +434,12 @@ fn resolve_pg_target(user_name: &str, version: Option<&str>) -> Result Result<()> { +async fn resume_existing( + docker: &bollard::Docker, + prior: ServerInfo, + metadata_lock: &server::ServerLock, + json: bool, +) -> Result<()> { let container_id = prior.container_id.clone().expect("checked by caller"); let display_name = user_name_from_key(&prior.name).to_string(); @@ -459,7 +464,7 @@ async fn resume_existing(docker: &bollard::Docker, prior: ServerInfo, json: bool started_at: server::now_timestamp(), ..prior }; - server::save_server_info(&info)?; + metadata_lock.save_info(&info)?; let out = output::PostgresStartOutput { name: display_name, @@ -699,7 +704,7 @@ fn generate_password() -> String { async fn stop(name: &str, version: Option<&str>, json: bool) -> Result<()> { server::validate_server_name(name)?; - server::recover_current_project_servers(); + server::recover_current_project_servers()?; let target = resolve_pg_target(name, version)?; if !json { let display = format!("{} ({})", user_name_from_key(&target.name), target.version); @@ -715,8 +720,8 @@ async fn stop(name: &str, version: Option<&str>, json: bool) -> Result<()> { } async fn stop_all(json: bool) -> Result<()> { - server::recover_current_project_servers(); - let servers: Vec<_> = server::list_running_servers() + server::recover_current_project_servers()?; + let servers: Vec<_> = server::list_running_servers()? .into_iter() .filter(|s| s.engine == Engine::Postgres) .collect(); @@ -736,11 +741,16 @@ async fn stop_all(json: bool) -> Result<()> { fn remove(name: &str, version: Option<&str>, json: bool) -> Result<()> { server::validate_server_name(name)?; - server::recover_current_project_servers(); + server::recover_current_project_servers()?; - let target = resolve_pg_target(name, version)?; + let mut target = resolve_pg_target(name, version)?; let key = target.name.clone(); - if server::is_server_running(&key) { + let metadata_lock = server::ServerLock::acquire(&key)?; + target = metadata_lock + .load_info()? + .filter(|info| info.engine == Engine::Postgres) + .ok_or_else(|| Error::ServerNotFound(name.to_string()))?; + if metadata_lock.is_running()? { return Err(Error::ServerAlreadyRunning(name.to_string())); } @@ -755,7 +765,7 @@ fn remove(name: &str, version: Option<&str>, json: bool) -> Result<()> { // privileged container in that case. let pg_dir = server::servers_dir_join(&key); docker::remove_host_dir_blocking(&pg_dir)?; - server::remove_server_info(&key); + metadata_lock.remove_info()?; let out = output::ServerRemoveOutput { name: name.to_string(), }; @@ -789,12 +799,18 @@ async fn client( ); } - server::recover_current_project_servers(); + server::recover_current_project_servers()?; let server_name = name.as_deref().unwrap_or("default"); - let info = resolve_pg_target(server_name, version.as_deref())?; - if !server::is_server_running(&info.name) { + let target = resolve_pg_target(server_name, version.as_deref())?; + let metadata_lock = server::ServerLock::acquire(&target.name)?; + let info = metadata_lock + .load_info()? + .filter(|info| info.engine == Engine::Postgres) + .ok_or_else(|| Error::ServerNotFound(server_name.to_string()))?; + if !metadata_lock.is_running()? { return Err(Error::ServerNotRunning(server_name.to_string())); } + drop(metadata_lock); let docker = docker::connect().await?; let container_id = info @@ -909,12 +925,18 @@ fn exec_host_psql( } fn dotenv(name: Option<&str>, version: Option<&str>, use_local: bool, json: bool) -> Result<()> { - server::recover_current_project_servers(); + server::recover_current_project_servers()?; let server_name = name.unwrap_or("default"); - let info = resolve_pg_target(server_name, version)?; - if !server::is_server_running(&info.name) { + let target = resolve_pg_target(server_name, version)?; + let metadata_lock = server::ServerLock::acquire(&target.name)?; + let info = metadata_lock + .load_info()? + .filter(|info| info.engine == Engine::Postgres) + .ok_or_else(|| Error::ServerNotFound(server_name.to_string()))?; + if !metadata_lock.is_running()? { return Err(Error::ServerNotRunning(server_name.to_string())); } + drop(metadata_lock); // Read user/password/db from the container env so we always emit accurate creds. let (user, password, database) = docker::block_on(read_pg_env_for_dotenv( diff --git a/crates/clickhousectl/src/local/server.rs b/crates/clickhousectl/src/local/server.rs index 024022ed..5833bf43 100644 --- a/crates/clickhousectl/src/local/server.rs +++ b/crates/clickhousectl/src/local/server.rs @@ -4,6 +4,9 @@ use crate::local::discovery; use crate::local::docker; use serde::{Deserialize, Serialize}; use std::collections::BTreeSet; +use std::fs::{File, OpenOptions}; +use std::io::Write; +use std::os::fd::AsRawFd; use std::path::{Path, PathBuf}; use std::time::Duration; @@ -105,10 +108,8 @@ fn server_meta_path(name: &str) -> PathBuf { servers_dir().join(format!("{}.json", name)) } -/// Public alias used by `docker.rs` during orphan recovery — keeps the path -/// computation in one place. -pub fn server_meta_path_for_recovery(name: &str) -> PathBuf { - server_meta_path(name) +fn server_lock_path(name: &str) -> PathBuf { + servers_dir().join(".locks").join(format!("{}.lock", name)) } /// Data directory for a ClickHouse server: .clickhouse/servers//data/. @@ -168,39 +169,231 @@ pub fn ensure_pg_data_dir(name: &str, major: &str) -> Result<()> { Ok(()) } -/// Save server info to a metadata file. -pub fn save_server_info(info: &ServerInfo) -> Result<()> { - let dir = servers_dir(); - std::fs::create_dir_all(&dir)?; - let path = server_meta_path(&info.name); - let json = serde_json::to_string_pretty(info)?; - std::fs::write(path, json)?; - Ok(()) +struct FileLock { + file: File, } -/// Remove a server's metadata file. -pub fn remove_server_info(name: &str) { - let _ = std::fs::remove_file(server_meta_path(name)); +impl FileLock { + fn acquire(path: &Path, name: &str) -> Result { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).map_err(|source| metadata_write_error(name, source))?; + } + let file = OpenOptions::new() + .create(true) + .read(true) + .write(true) + .truncate(false) + .open(path) + .map_err(|source| metadata_access_error(name, source))?; + + loop { + // SAFETY: `file` owns this descriptor for the lifetime of the lock. + let result = unsafe { libc::flock(file.as_raw_fd(), libc::LOCK_EX) }; + if result == 0 { + return Ok(Self { file }); + } + let source = std::io::Error::last_os_error(); + if source.kind() != std::io::ErrorKind::Interrupted { + return Err(metadata_access_error(name, source)); + } + } + } } -/// Mark a ClickHouse server as stopped without discarding its metadata. -/// -/// The PID match avoids overwriting metadata when a newer process was already -/// recorded before this transition began. -pub fn mark_server_stopped(name: &str, pid: u32) -> Result<()> { - let Some(mut info) = load_info(name) else { - return Ok(()); - }; - if info.engine == Engine::Clickhouse && info.pid == pid { - info.pid = 0; - info.version.clear(); - info.http_port = 0; - info.tcp_port = 0; - save_server_info(&info)?; +impl Drop for FileLock { + fn drop(&mut self) { + // SAFETY: `self.file` remains open until after `drop` returns. + unsafe { + libc::flock(self.file.as_raw_fd(), libc::LOCK_UN); + } + } +} + +struct TemporaryMetadata { + path: PathBuf, + committed: bool, +} + +impl Drop for TemporaryMetadata { + fn drop(&mut self) { + if !self.committed { + let _ = std::fs::remove_file(&self.path); + } } +} + +fn metadata_access_error(name: &str, source: std::io::Error) -> Error { + if source.kind() == std::io::ErrorKind::PermissionDenied { + Error::ServerMetadataPermission { + name: name.to_string(), + source, + } + } else { + Error::ServerMetadataRead { + name: name.to_string(), + source, + } + } +} + +fn metadata_write_error(name: &str, source: std::io::Error) -> Error { + if source.kind() == std::io::ErrorKind::PermissionDenied { + Error::ServerMetadataPermission { + name: name.to_string(), + source, + } + } else { + Error::ServerMetadataWrite { + name: name.to_string(), + source, + } + } +} + +fn read_server_info(name: &str) -> Result> { + let content = match std::fs::read(server_meta_path(name)) { + Ok(content) => content, + Err(source) if source.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(source) => return Err(metadata_access_error(name, source)), + }; + serde_json::from_slice(&content) + .map(Some) + .map_err(|source| Error::ServerMetadataParse { + name: name.to_string(), + source, + }) +} + +fn write_server_info(name: &str, info: &ServerInfo) -> Result<()> { + let path = server_meta_path(name); + let file_name = path + .file_name() + .expect("server metadata path has a file name") + .to_string_lossy(); + let temp_path = path.with_file_name(format!( + ".{file_name}.{}.tmp", + uuid::Uuid::new_v4().simple() + )); + let mut temporary = TemporaryMetadata { + path: temp_path.clone(), + committed: false, + }; + let mut file = OpenOptions::new() + .create_new(true) + .write(true) + .open(&temp_path) + .map_err(|source| metadata_write_error(name, source))?; + let json = serde_json::to_vec_pretty(info)?; + file.write_all(&json) + .map_err(|source| metadata_write_error(name, source))?; + file.flush() + .map_err(|source| metadata_write_error(name, source))?; + file.sync_all() + .map_err(|source| metadata_write_error(name, source))?; + pause_before_metadata_rename_for_test(); + std::fs::rename(&temp_path, &path).map_err(|source| metadata_write_error(name, source))?; + temporary.committed = true; Ok(()) } +/// The per-server cross-process lifecycle lock. Callers that perform an +/// external lifecycle action keep this guard through the corresponding +/// metadata update so another invocation cannot act on an obsolete snapshot. +pub struct ServerLock { + name: String, + _file: FileLock, +} + +impl ServerLock { + pub fn acquire(name: &str) -> Result { + validate_server_name(name)?; + ensure_servers_dir()?; + Ok(Self { + name: name.to_string(), + _file: FileLock::acquire(&server_lock_path(name), name)?, + }) + } + + pub fn load_info(&self) -> Result> { + read_server_info(&self.name) + } + + pub fn metadata_path(&self) -> PathBuf { + server_meta_path(&self.name) + } + + pub fn load_running_info(&self) -> Result> { + Ok(self.load_info()?.filter(is_alive)) + } + + pub fn is_running(&self) -> Result { + Ok(self.load_running_info()?.is_some()) + } + + pub fn save_info(&self, info: &ServerInfo) -> Result<()> { + write_server_info(&self.name, info) + } + + pub fn remove_info(&self) -> Result<()> { + match std::fs::remove_file(server_meta_path(&self.name)) { + Ok(()) => Ok(()), + Err(source) if source.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(source) => Err(metadata_write_error(&self.name, source)), + } + } + + pub fn mark_stopped(&self, pid: u32) -> Result<()> { + let Some(mut info) = self.load_info()? else { + return Ok(()); + }; + if info.engine == Engine::Clickhouse && info.pid == pid { + set_stopped(&mut info); + self.save_info(&info)?; + } + Ok(()) + } + + pub fn kill(&self) -> Result<()> { + let info = self + .load_running_info()? + .ok_or_else(|| Error::ServerNotRunning(self.name.clone()))?; + match info.engine { + Engine::Clickhouse => { + kill_process(info.pid)?; + self.mark_stopped(info.pid)?; + } + Engine::Postgres => { + let id = info.container_id.as_deref().ok_or_else(|| { + Error::DockerError(format!( + "Postgres server '{}' has no container_id in metadata", + self.name + )) + })?; + docker::stop_blocking(id)?; + } + } + Ok(()) + } +} + +fn set_stopped(info: &mut ServerInfo) { + info.pid = 0; + info.version.clear(); + info.http_port = 0; + info.tcp_port = 0; +} + +/// Save server info with an atomic replacement under the lifecycle lock. +#[cfg(test)] +pub fn save_server_info(info: &ServerInfo) -> Result<()> { + ServerLock::acquire(&info.name)?.save_info(info) +} + +/// Mark a ClickHouse server as stopped if the recorded PID still matches. +pub fn mark_server_stopped(name: &str, pid: u32) -> Result<()> { + ServerLock::acquire(name)?.mark_stopped(pid) +} + /// Engine-aware liveness check. fn is_alive(info: &ServerInfo) -> bool { match info.engine { @@ -212,22 +405,20 @@ fn is_alive(info: &ServerInfo) -> bool { } } -/// Load server metadata regardless of liveness. Returns None if no metadata -/// file exists or it can't be parsed. `name` is the disk identifier — for -/// ClickHouse this is just the user-facing name, for Postgres it's -/// `-pg` (use `pg_instance_key`). -pub fn load_info(name: &str) -> Option { - let content = std::fs::read_to_string(server_meta_path(name)).ok()?; - serde_json::from_str(&content).ok() +/// Load server metadata regardless of liveness. A missing file is `None`; +/// access and parse failures remain actionable errors. +pub fn load_info(name: &str) -> Result> { + ServerLock::acquire(name)?.load_info() } /// Find every Postgres instance whose user-facing name is `name`. Returns /// one entry per major version that has a metadata file on disk. -pub fn find_pg_instances(name: &str) -> Vec { +pub fn find_pg_instances(name: &str) -> Result> { let prefix = format!("{}-pg", name); let dir = match std::fs::read_dir(servers_dir()) { Ok(d) => d, - Err(_) => return Vec::new(), + Err(source) if source.kind() == std::io::ErrorKind::NotFound => return Ok(Vec::new()), + Err(source) => return Err(source.into()), }; let mut out = Vec::new(); for entry in dir.flatten() { @@ -248,22 +439,21 @@ pub fn find_pg_instances(name: &str) -> Vec { if major.is_empty() || !major.chars().all(|c| c.is_ascii_digit()) { continue; } - if let Some(info) = load_info(stem) + if let Some(info) = load_info(stem)? && info.engine == Engine::Postgres { out.push(info); } } - out + Ok(out) } /// Load server metadata only if the underlying process/container is alive. /// Does not update stale metadata. `list_all_servers` is the single place that /// marks ClickHouse entries stopped when their PID is gone, so callers like /// `is_server_running` and `resolve_name` can read metadata without side effects. -fn load_running_info(name: &str) -> Option { - let info = load_info(name)?; - if is_alive(&info) { Some(info) } else { None } +pub fn load_running_info(name: &str) -> Result> { + ServerLock::acquire(name)?.load_running_info() } /// List all known servers (both running and stopped). @@ -272,19 +462,20 @@ fn load_running_info(name: &str) -> Option { /// entry — for ClickHouse the disk id is the user-facing name; for Postgres /// it's `-pg`. Also runs process/container discovery so /// orphaned instances reappear. -pub fn list_all_servers() -> Vec { - recover_current_project_servers(); +pub fn list_all_servers() -> Result> { + recover_current_project_servers()?; let dir = servers_dir(); let mut entries = Vec::new(); let dir_entries = match std::fs::read_dir(&dir) { Ok(e) => e, - Err(_) => return entries, + Err(source) if source.kind() == std::io::ErrorKind::NotFound => return Ok(entries), + Err(source) => return Err(source.into()), }; - for entry in dir_entries.flatten() { - let path = entry.path(); + for entry in dir_entries { + let entry = entry?; let fname = match entry.file_name().into_string() { Ok(s) => s, Err(_) => continue, @@ -293,56 +484,48 @@ pub fn list_all_servers() -> Vec { Some(s) => s, None => continue, }; - if !path.is_file() { + let Some((info, running)) = normalize_server_info(stem)? else { continue; - } - - let mut info = load_info(stem); - let mut running = match &info { - Some(i) => is_alive(i), - None => false, }; - // A dead ClickHouse PID can result from a crash or a global stop. Keep - // the metadata and normalize it to the same stopped sentinel Postgres - // uses so the instance remains discoverable. - if let Some(i) = &info - && !running - && i.engine == Engine::Clickhouse - && i.pid != 0 - { - let stale_pid = i.pid; - let _ = mark_server_stopped(stem, stale_pid); - // Reload because a concurrent restart may have replaced the stale - // PID before `mark_server_stopped` acquired the latest metadata. - info = load_info(stem); - running = info.as_ref().is_some_and(is_alive); - } - entries.push(ServerEntry { name: stem.to_string(), running, - info, + info: Some(info), }); } entries.sort_by(|a, b| b.running.cmp(&a.running).then(a.name.cmp(&b.name))); - entries + Ok(entries) +} + +fn normalize_server_info(name: &str) -> Result> { + let lock = ServerLock::acquire(name)?; + let Some(mut info) = lock.load_info()? else { + return Ok(None); + }; + let running = is_alive(&info); + if !running && info.engine == Engine::Clickhouse && info.pid != 0 { + pause_during_stale_normalization_for_test(); + set_stopped(&mut info); + lock.save_info(&info)?; + } + Ok(Some((info, running))) } /// List only currently running servers. -pub fn list_running_servers() -> Vec { - list_all_servers() +pub fn list_running_servers() -> Result> { + Ok(list_all_servers()? .into_iter() .filter(|e| e.running) .filter_map(|e| e.info) - .collect() + .collect()) } /// List known ClickHouse server identities, including stopped data directories /// retained from older versions that may not have metadata. -pub fn list_clickhouse_server_names() -> Vec { - let entries = list_all_servers(); +pub fn list_clickhouse_server_names() -> Result> { + let entries = list_all_servers()?; let mut clickhouse_names = BTreeSet::new(); let mut postgres_keys = BTreeSet::new(); @@ -372,17 +555,17 @@ pub fn list_clickhouse_server_names() -> Vec { } } - clickhouse_names.into_iter().collect() + Ok(clickhouse_names.into_iter().collect()) } /// Check if a named server is currently running. -pub fn is_server_running(name: &str) -> bool { - load_running_info(name).is_some() +pub fn is_server_running(name: &str) -> Result { + Ok(load_running_info(name)?.is_some()) } /// Count running servers. -pub fn running_server_count() -> usize { - list_running_servers().len() +pub fn running_server_count() -> Result { + Ok(list_running_servers()?.len()) } fn is_process_alive(pid: u32) -> bool { @@ -438,25 +621,7 @@ fn kill_process(pid: u32) -> Result<()> { /// the metadata file so a subsequent `start` resumes the same container /// (preserving the password and any other PGDATA-encoded settings). pub fn kill_server(name: &str) -> Result<()> { - let info = load_running_info(name).ok_or_else(|| Error::ServerNotRunning(name.to_string()))?; - - match info.engine { - Engine::Clickhouse => { - kill_process(info.pid)?; - mark_server_stopped(name, info.pid)?; - } - Engine::Postgres => { - let id = info.container_id.as_deref().ok_or_else(|| { - Error::DockerError(format!( - "Postgres server '{}' has no container_id in metadata", - name - )) - })?; - docker::stop_blocking(id)?; - // Metadata + container preserved so `start` can resume. - } - } - Ok(()) + ServerLock::acquire(name)?.kill() } /// Resolve the server name: use provided name, "default" if none and no default running, @@ -469,8 +634,8 @@ pub fn resolve_name(name: Option<&str>) -> Result { Ok(n.to_string()) } None => { - if is_server_running("default") { - Ok(generate_random_name()) + if is_server_running("default")? { + generate_random_name() } else { Ok("default".to_string()) } @@ -478,7 +643,7 @@ pub fn resolve_name(name: Option<&str>) -> Result { } } -fn generate_random_name() -> String { +fn generate_random_name() -> Result { let seed = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .unwrap_or_default() @@ -488,15 +653,15 @@ fn generate_random_name() -> String { let noun = NOUNS[((mixed / ADJECTIVES.len() as u128) % NOUNS.len() as u128) as usize]; let tag = format!("{}-{}", adj, noun); - if is_server_running(&tag) { + if is_server_running(&tag)? { for i in 2..100 { let candidate = format!("{}-{}", tag, i); - if !is_server_running(&candidate) { - return candidate; + if !is_server_running(&candidate)? { + return Ok(candidate); } } } - tag + Ok(tag) } /// Wait a moment after spawn and check if the child exited immediately. @@ -507,7 +672,7 @@ pub async fn check_spawn_health( ) -> Result<()> { tokio::time::sleep(SPAWN_HEALTH_DELAY).await; if let Some(status) = child.try_wait().map_err(|e| Error::Exec(e.to_string()))? { - let _ = mark_server_stopped(name, child.id()); + mark_server_stopped(name, child.id())?; return Err(Error::StartupExit(format!( "Server '{}' exited immediately after starting ({}). See server log: {}", name, @@ -518,22 +683,29 @@ pub async fn check_spawn_health( Ok(()) } -async fn stop_starting_child( - child: &mut std::process::Child, - name: &str, -) -> std::result::Result<(), String> { +async fn stop_starting_child(child: &mut std::process::Child, name: &str) -> Result<()> { let pid = child.id(); - if child.try_wait().map_err(|e| e.to_string())?.is_none() + if child + .try_wait() + .map_err(|error| Error::Exec(error.to_string()))? + .is_none() && let Err(signal_error) = send_signal(pid, libc::SIGTERM) - && child.try_wait().map_err(|e| e.to_string())?.is_none() + && child + .try_wait() + .map_err(|error| Error::Exec(error.to_string()))? + .is_none() { - return Err(signal_error.to_string()); + return Err(signal_error); } let deadline = tokio::time::Instant::now() + STARTUP_SHUTDOWN_TIMEOUT; loop { - if child.try_wait().map_err(|e| e.to_string())?.is_some() { - return mark_server_stopped(name, pid).map_err(|e| e.to_string()); + if child + .try_wait() + .map_err(|error| Error::Exec(error.to_string()))? + .is_some() + { + return mark_server_stopped(name, pid); } if tokio::time::Instant::now() >= deadline { break; @@ -541,9 +713,13 @@ async fn stop_starting_child( tokio::time::sleep(STARTUP_POLL_INTERVAL).await; } - child.kill().map_err(|e| e.to_string())?; - child.wait().map_err(|e| e.to_string())?; - mark_server_stopped(name, pid).map_err(|e| e.to_string()) + child + .kill() + .map_err(|error| Error::Exec(error.to_string()))?; + child + .wait() + .map_err(|error| Error::Exec(error.to_string()))?; + mark_server_stopped(name, pid) } /// Wait until ClickHouse responds to HTTP health checks and accepts TCP connections. @@ -566,7 +742,7 @@ pub async fn wait_for_server_ready( loop { if let Some(status) = child.try_wait().map_err(|e| Error::Exec(e.to_string()))? { - let _ = mark_server_stopped(name, child.id()); + mark_server_stopped(name, child.id())?; return Err(Error::StartupExit(format!( "Server '{}' exited before becoming ready on HTTP port {} and TCP port {} ({}). \ See server log: {}", @@ -604,6 +780,17 @@ pub async fn wait_for_server_ready( let pid = child.id(); let cleanup = match stop_starting_child(child, name).await { Ok(()) => " and was stopped".to_string(), + Err(error) + if matches!( + &error, + Error::ServerMetadataRead { .. } + | Error::ServerMetadataPermission { .. } + | Error::ServerMetadataParse { .. } + | Error::ServerMetadataWrite { .. } + ) => + { + return Err(error); + } Err(error) => format!("; failed to stop PID {}: {}", pid, error), }; return Err(Error::StartupTimeout(format!( @@ -712,10 +899,10 @@ pub fn now_timestamp() -> String { /// `.clickhouse/servers//data/` path. If a process is found that has no /// metadata file, a new `ServerInfo` is saved so it appears in `server list` /// and can be managed normally. -pub fn recover_current_project_servers() { +pub fn recover_current_project_servers() -> Result<()> { let current_dir = match std::env::current_dir().and_then(|p| p.canonicalize()) { Ok(p) => p.display().to_string(), - Err(_) => return, + Err(source) => return Err(source.into()), }; let processes = discovery::discover_clickhouse_processes(); @@ -730,11 +917,6 @@ pub fn recover_current_project_servers() { continue; } - // Only recover if we don't already have metadata for this server - if load_running_info(&proc.server_name).is_some() { - continue; - } - let info = ServerInfo { name: proc.server_name, pid: proc.pid, @@ -746,11 +928,25 @@ pub fn recover_current_project_servers() { engine: Engine::Clickhouse, container_id: None, }; - let _ = save_server_info(&info); + save_recovered_server_info(&info, true)?; } // Also recover orphaned Postgres containers belonging to this project. - docker::recover_project_postgres_blocking(¤t_dir); + docker::recover_project_postgres_blocking(¤t_dir)?; + Ok(()) +} + +/// Install recovered metadata without racing a normal lifecycle write. ClickHouse +/// discovery may replace a stale stopped record; Postgres recovery only fills a +/// genuinely absent record because stopped containers are still valid state. +pub fn save_recovered_server_info(info: &ServerInfo, replace_stale: bool) -> Result<()> { + let lock = ServerLock::acquire(&info.name)?; + if let Some(existing) = lock.load_info()? + && (!replace_stale || is_alive(&existing)) + { + return Ok(()); + } + lock.save_info(info) } /// A server entry for global listing — always running (discovered via process inspection). @@ -795,9 +991,302 @@ pub fn kill_server_by_pid(pid: u32) -> Result<()> { kill_process(pid) } +#[cfg(test)] +fn wait_for_test_release(marker_var: &str, release_var: &str) { + let (Ok(marker), Ok(release)) = (std::env::var(marker_var), std::env::var(release_var)) else { + return; + }; + std::fs::write(marker, b"ready").expect("write metadata test marker"); + let release = PathBuf::from(release); + let deadline = std::time::Instant::now() + Duration::from_secs(10); + while !release.exists() { + assert!( + std::time::Instant::now() < deadline, + "timed out waiting for metadata test release" + ); + std::thread::sleep(Duration::from_millis(5)); + } +} + +#[cfg(test)] +fn pause_before_metadata_rename_for_test() { + wait_for_test_release( + "CHCTL_TEST_METADATA_RENAME_READY", + "CHCTL_TEST_METADATA_RENAME_RELEASE", + ); +} + +#[cfg(not(test))] +fn pause_before_metadata_rename_for_test() {} + +#[cfg(test)] +fn pause_during_stale_normalization_for_test() { + wait_for_test_release( + "CHCTL_TEST_STALE_NORMALIZE_READY", + "CHCTL_TEST_STALE_NORMALIZE_RELEASE", + ); +} + +#[cfg(not(test))] +fn pause_during_stale_normalization_for_test() {} + #[cfg(test)] mod tests { use super::*; + use std::process::{Child, Command, Stdio}; + use std::time::Instant; + + const WRITE_HELPER: &str = "local::server::tests::metadata_write_subprocess"; + const NORMALIZE_HELPER: &str = "local::server::tests::metadata_normalize_subprocess"; + const CONCURRENCY_HELPER: &str = "local::server::tests::metadata_concurrency_subprocess"; + + fn test_info(name: &str, pid: u32, version: &str) -> ServerInfo { + ServerInfo { + name: name.to_string(), + pid, + version: version.to_string(), + http_port: 8123, + tcp_port: 9000, + started_at: "1700000000".to_string(), + cwd: "/tmp/project".to_string(), + engine: Engine::Clickhouse, + container_id: None, + } + } + + fn metadata_path(project: &Path) -> PathBuf { + project.join(".clickhouse/servers/default.json") + } + + fn write_initial_metadata(project: &Path, info: &ServerInfo) { + let path = metadata_path(project); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(path, serde_json::to_vec_pretty(info).unwrap()).unwrap(); + } + + fn helper(test_name: &str, project: &Path) -> Command { + let mut command = Command::new(std::env::current_exe().expect("locate test binary")); + command + .args(["--exact", test_name, "--nocapture"]) + .env("CHCTL_TEST_METADATA_PROJECT", project) + .stdout(Stdio::null()) + .stderr(Stdio::null()); + command + } + + fn wait_for_path(path: &Path) { + let deadline = Instant::now() + Duration::from_secs(5); + while !path.exists() { + assert!( + Instant::now() < deadline, + "timed out waiting for {}", + path.display() + ); + std::thread::sleep(Duration::from_millis(5)); + } + } + + fn wait_success(child: &mut Child) { + let status = child.wait().expect("wait for metadata helper"); + assert!(status.success(), "metadata helper failed with {status}"); + } + + fn enter_helper_project() -> Option { + let project = std::env::var_os("CHCTL_TEST_METADATA_PROJECT")?; + let project = PathBuf::from(project); + std::env::set_current_dir(&project).unwrap(); + Some(project) + } + + #[test] + fn metadata_write_subprocess() { + if enter_helper_project().is_none() { + return; + } + let pid = std::env::var("CHCTL_TEST_METADATA_PID") + .unwrap() + .parse() + .unwrap(); + let version = std::env::var("CHCTL_TEST_METADATA_VERSION").unwrap(); + if let Some(attempt) = std::env::var_os("CHCTL_TEST_METADATA_ATTEMPT") { + std::fs::write(attempt, b"attempt").unwrap(); + } + save_server_info(&test_info("default", pid, &version)).unwrap(); + } + + #[test] + fn metadata_normalize_subprocess() { + if enter_helper_project().is_none() { + return; + } + normalize_server_info("default").unwrap(); + } + + #[test] + fn metadata_concurrency_subprocess() { + let Some(_) = enter_helper_project() else { + return; + }; + let pid = std::env::var("CHCTL_TEST_METADATA_PID") + .unwrap() + .parse() + .unwrap(); + match std::env::var("CHCTL_TEST_METADATA_OPERATION") + .unwrap() + .as_str() + { + "lifecycle" => { + let lock = ServerLock::acquire("default").unwrap(); + wait_for_test_release("CHCTL_TEST_LIFECYCLE_READY", "CHCTL_TEST_LIFECYCLE_RELEASE"); + lock.save_info(&test_info("default", pid, "26.8.1.1")) + .unwrap(); + } + "client" => { + std::fs::write( + std::env::var_os("CHCTL_TEST_METADATA_ATTEMPT").unwrap(), + b"attempt", + ) + .unwrap(); + let state = if load_running_info("default").unwrap().is_some() { + b"running".as_slice() + } else { + b"stopped".as_slice() + }; + std::fs::write(std::env::var_os("CHCTL_TEST_CLIENT_RESULT").unwrap(), state) + .unwrap(); + } + "stop" => { + std::fs::write( + std::env::var_os("CHCTL_TEST_METADATA_ATTEMPT").unwrap(), + b"attempt", + ) + .unwrap(); + kill_server("default").unwrap(); + } + operation => panic!("unknown metadata test operation {operation}"), + } + } + + #[test] + fn interrupted_atomic_write_keeps_the_previous_document() { + let project = tempfile::tempdir().unwrap(); + write_initial_metadata(project.path(), &test_info("default", 0, "old")); + let ready = project.path().join("rename-ready"); + let release = project.path().join("never-release"); + let mut writer = helper(WRITE_HELPER, project.path()) + .env("CHCTL_TEST_METADATA_PID", "12345") + .env("CHCTL_TEST_METADATA_VERSION", "new") + .env("CHCTL_TEST_METADATA_RENAME_READY", &ready) + .env("CHCTL_TEST_METADATA_RENAME_RELEASE", &release) + .spawn() + .unwrap(); + + wait_for_path(&ready); + let live: ServerInfo = + serde_json::from_slice(&std::fs::read(metadata_path(project.path())).unwrap()).unwrap(); + assert_eq!(live.version, "old"); + writer.kill().unwrap(); + writer.wait().unwrap(); + + let live: ServerInfo = + serde_json::from_slice(&std::fs::read(metadata_path(project.path())).unwrap()).unwrap(); + assert_eq!(live.version, "old"); + let abandoned = std::fs::read_dir(project.path().join(".clickhouse/servers")) + .unwrap() + .filter_map(|entry| entry.ok()) + .filter(|entry| entry.file_name().to_string_lossy().ends_with(".tmp")) + .count(); + assert_eq!(abandoned, 1); + } + + #[test] + fn stale_normalizer_cannot_overwrite_a_concurrent_restart() { + let project = tempfile::tempdir().unwrap(); + write_initial_metadata(project.path(), &test_info("default", u32::MAX, "stale")); + let ready = project.path().join("normalizer-ready"); + let release = project.path().join("normalizer-release"); + let mut normalizer = helper(NORMALIZE_HELPER, project.path()) + .env("CHCTL_TEST_STALE_NORMALIZE_READY", &ready) + .env("CHCTL_TEST_STALE_NORMALIZE_RELEASE", &release) + .spawn() + .unwrap(); + wait_for_path(&ready); + + let restart_pid = std::process::id(); + let restart_attempt = project.path().join("restart-attempt"); + let mut restart = helper(WRITE_HELPER, project.path()) + .env("CHCTL_TEST_METADATA_PID", restart_pid.to_string()) + .env("CHCTL_TEST_METADATA_VERSION", "restarted") + .env("CHCTL_TEST_METADATA_ATTEMPT", &restart_attempt) + .spawn() + .unwrap(); + wait_for_path(&restart_attempt); + assert!(restart.try_wait().unwrap().is_none()); + + std::fs::write(&release, b"release").unwrap(); + wait_success(&mut normalizer); + wait_success(&mut restart); + + let live: ServerInfo = + serde_json::from_slice(&std::fs::read(metadata_path(project.path())).unwrap()).unwrap(); + assert_eq!(live.pid, restart_pid); + assert_eq!(live.version, "restarted"); + } + + #[test] + fn concurrent_lifecycle_client_and_stop_observe_complete_metadata() { + let project = tempfile::tempdir().unwrap(); + write_initial_metadata(project.path(), &test_info("default", 0, "")); + let mut process = Command::new("sh") + .args(["-c", "exec sleep 30"]) + .spawn() + .unwrap(); + let pid = process.id(); + let reaper = std::thread::spawn(move || process.wait().unwrap()); + let ready = project.path().join("lifecycle-ready"); + let release = project.path().join("lifecycle-release"); + let client_result = project.path().join("client-result"); + let client_attempt = project.path().join("client-attempt"); + let stop_attempt = project.path().join("stop-attempt"); + + let mut lifecycle = helper(CONCURRENCY_HELPER, project.path()) + .env("CHCTL_TEST_METADATA_OPERATION", "lifecycle") + .env("CHCTL_TEST_METADATA_PID", pid.to_string()) + .env("CHCTL_TEST_LIFECYCLE_READY", &ready) + .env("CHCTL_TEST_LIFECYCLE_RELEASE", &release) + .spawn() + .unwrap(); + wait_for_path(&ready); + let mut client = helper(CONCURRENCY_HELPER, project.path()) + .env("CHCTL_TEST_METADATA_OPERATION", "client") + .env("CHCTL_TEST_METADATA_PID", pid.to_string()) + .env("CHCTL_TEST_CLIENT_RESULT", &client_result) + .env("CHCTL_TEST_METADATA_ATTEMPT", &client_attempt) + .spawn() + .unwrap(); + let mut stop = helper(CONCURRENCY_HELPER, project.path()) + .env("CHCTL_TEST_METADATA_OPERATION", "stop") + .env("CHCTL_TEST_METADATA_PID", pid.to_string()) + .env("CHCTL_TEST_METADATA_ATTEMPT", &stop_attempt) + .spawn() + .unwrap(); + wait_for_path(&client_attempt); + wait_for_path(&stop_attempt); + assert!(client.try_wait().unwrap().is_none()); + assert!(stop.try_wait().unwrap().is_none()); + + std::fs::write(&release, b"release").unwrap(); + wait_success(&mut lifecycle); + wait_success(&mut client); + wait_success(&mut stop); + + let client_state = std::fs::read_to_string(client_result).unwrap(); + assert!(matches!(client_state.as_str(), "running" | "stopped")); + let live: ServerInfo = + serde_json::from_slice(&std::fs::read(metadata_path(project.path())).unwrap()).unwrap(); + assert_eq!(live.pid, 0); + assert!(!reaper.join().unwrap().success()); + } #[test] fn engine_serializes_lowercase() { diff --git a/crates/clickhousectl/src/version_manager/install.rs b/crates/clickhousectl/src/version_manager/install.rs index 2f7018e3..aa268dda 100644 --- a/crates/clickhousectl/src/version_manager/install.rs +++ b/crates/clickhousectl/src/version_manager/install.rs @@ -195,7 +195,7 @@ pub async fn install_resolved( if !structured_output && is_master && replaced_existing - && version_in_use_by_running_server(&exact_version) + && version_in_use_by_running_server(&exact_version)? { eprintln!( "Note: running servers keep using the previous {} build until restarted", @@ -329,11 +329,11 @@ pub async fn ensure_installed( /// Whether a running managed server (in the current project) was started from /// this version. Recovers orphans first (like `local remove`) so a server that /// lost its metadata file is still counted. -fn version_in_use_by_running_server(version: &str) -> bool { - crate::local::server::recover_current_project_servers(); - crate::local::server::list_running_servers() +fn version_in_use_by_running_server(version: &str) -> Result { + crate::local::server::recover_current_project_servers()?; + Ok(crate::local::server::list_running_servers()? .iter() - .any(|s| s.version == version) + .any(|s| s.version == version)) } /// Detect the version of a clickhouse binary by running `./clickhouse --version` diff --git a/crates/clickhousectl/tests/local_server_metadata_test.rs b/crates/clickhousectl/tests/local_server_metadata_test.rs new file mode 100644 index 00000000..8c89cbdc --- /dev/null +++ b/crates/clickhousectl/tests/local_server_metadata_test.rs @@ -0,0 +1,145 @@ +//! End-to-end coverage for selected local server metadata failures (#472). + +use serde_json::Value; +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +fn clickhousectl_binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_clickhousectl")) +} + +fn run(project: &Path, home: &Path, json: bool) -> Output { + let mut command = Command::new(clickhousectl_binary()); + command + .env_clear() + .env("DO_NOT_TRACK", "1") + .env("HOME", home) + .current_dir(project) + .arg("local"); + if json { + command.arg("--json"); + } + command + .args(["server", "stop", "default"]) + .output() + .expect("run clickhousectl") +} + +fn metadata_path(project: &Path) -> PathBuf { + project.join(".clickhouse/servers/default.json") +} + +fn prepare_project(project: &Path) { + std::fs::create_dir_all(project.join(".clickhouse/servers/default/data")) + .expect("create server data directory"); +} + +fn valid_metadata(project: &Path) -> Vec { + serde_json::to_vec_pretty(&serde_json::json!({ + "name": "default", + "pid": std::process::id(), + "version": "26.8.1.1", + "http_port": 8123, + "tcp_port": 9000, + "started_at": "1700000000", + "cwd": project.display().to_string(), + "engine": "clickhouse" + })) + .unwrap() +} + +fn assert_json_error(output: &Output, code: &str, message_fragment: &str) { + assert_eq!( + output.status.code(), + Some(1), + "stdout: {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!(output.stdout.is_empty()); + let body: Value = serde_json::from_slice(&output.stderr).expect("parse structured error"); + assert_eq!(body["error"]["code"], code); + assert_eq!(body["error"]["command"], "clickhousectl local server list"); + assert!( + body["error"]["message"] + .as_str() + .unwrap() + .contains(message_fragment), + "{body}" + ); +} + +#[test] +fn selected_partial_json_and_invalid_utf8_are_parse_errors() { + for (label, contents) in [ + ("partial JSON", br#"{"name":"default","pid":1"#.as_slice()), + ("invalid UTF-8", b"{\"name\":\xff}".as_slice()), + ] { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + prepare_project(project.path()); + std::fs::write(metadata_path(project.path()), contents).expect("write invalid metadata"); + + let json = run(project.path(), home.path(), true); + assert_json_error( + &json, + "server_metadata_invalid", + "Metadata for server 'default'", + ); + + let human = run(project.path(), home.path(), false); + assert_eq!(human.status.code(), Some(1), "case: {label}"); + let stderr = String::from_utf8_lossy(&human.stderr); + assert!( + stderr.starts_with("Error: Metadata for server 'default'"), + "case: {label}; stderr: {stderr}" + ); + assert!(stderr.contains("Repair or remove the metadata file, then retry.")); + } +} + +#[test] +fn selected_metadata_read_failure_is_not_reported_as_stopped() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + prepare_project(project.path()); + std::fs::create_dir(metadata_path(project.path())).expect("create unreadable metadata shape"); + + let json = run(project.path(), home.path(), true); + assert_json_error( + &json, + "server_metadata_read", + "Could not read metadata for server 'default'", + ); + + let human = run(project.path(), home.path(), false); + let stderr = String::from_utf8_lossy(&human.stderr); + assert!(stderr.starts_with("Error: Could not read metadata for server 'default'")); + assert!(stderr.contains("Check that the file is readable and retry.")); +} + +#[test] +fn selected_metadata_permission_failure_has_its_own_action() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + prepare_project(project.path()); + let path = metadata_path(project.path()); + std::fs::write(&path, valid_metadata(project.path())).expect("write metadata"); + let original = std::fs::metadata(&path).unwrap().permissions(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o000)) + .expect("remove metadata permissions"); + + let json = run(project.path(), home.path(), true); + let human = run(project.path(), home.path(), false); + std::fs::set_permissions(&path, original).expect("restore metadata permissions"); + + assert_json_error( + &json, + "server_metadata_permission", + "Permission denied accessing metadata for server 'default'", + ); + let stderr = String::from_utf8_lossy(&human.stderr); + assert!(stderr.starts_with("Error: Permission denied accessing metadata for server 'default'")); + assert!(stderr.contains("Restore access to the file and its parent directory, then retry.")); +} From 107bf22e7c41339439b4b9eab9ad1a132938a50d Mon Sep 17 00:00:00 2001 From: sdairs Date: Mon, 24 Aug 2026 21:11:30 +0100 Subject: [PATCH 02/16] Fix metadata CI checks --- crates/clickhousectl/src/local/server.rs | 8 ++++---- .../clickhousectl/tests/local_structured_errors_test.rs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/clickhousectl/src/local/server.rs b/crates/clickhousectl/src/local/server.rs index 5833bf43..879637ad 100644 --- a/crates/clickhousectl/src/local/server.rs +++ b/crates/clickhousectl/src/local/server.rs @@ -900,10 +900,10 @@ pub fn now_timestamp() -> String { /// metadata file, a new `ServerInfo` is saved so it appears in `server list` /// and can be managed normally. pub fn recover_current_project_servers() -> Result<()> { - let current_dir = match std::env::current_dir().and_then(|p| p.canonicalize()) { - Ok(p) => p.display().to_string(), - Err(source) => return Err(source.into()), - }; + let current_dir = std::env::current_dir() + .and_then(|path| path.canonicalize())? + .display() + .to_string(); let processes = discovery::discover_clickhouse_processes(); for proc in processes { diff --git a/crates/clickhousectl/tests/local_structured_errors_test.rs b/crates/clickhousectl/tests/local_structured_errors_test.rs index c0715141..06b10389 100644 --- a/crates/clickhousectl/tests/local_structured_errors_test.rs +++ b/crates/clickhousectl/tests/local_structured_errors_test.rs @@ -365,7 +365,7 @@ fn foreground_child_exit_is_not_wrapped() { assert_eq!(output.stdout, b"server stdout\n"); let stderr = String::from_utf8(output.stderr).expect("stderr is UTF-8"); assert!(stderr.contains("Server 'default' running"), "{stderr}"); - assert!(stderr.ends_with("server stderr\n"), "{stderr}"); + assert!(stderr.contains("server stderr\n"), "{stderr}"); assert!(!stderr.contains("\"error\""), "{stderr}"); assert!(!stderr.contains("Error:"), "{stderr}"); } From 0b6ed3d79d305a28fec4b31042c630c023abee90 Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 10:53:52 +0100 Subject: [PATCH 03/16] Ignore non-file server metadata entries --- crates/clickhousectl/src/local/server.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/crates/clickhousectl/src/local/server.rs b/crates/clickhousectl/src/local/server.rs index 879637ad..94be1221 100644 --- a/crates/clickhousectl/src/local/server.rs +++ b/crates/clickhousectl/src/local/server.rs @@ -484,6 +484,9 @@ pub fn list_all_servers() -> Result> { Some(s) => s, None => continue, }; + if !entry.path().is_file() { + continue; + } let Some((info, running)) = normalize_server_info(stem)? else { continue; }; @@ -1039,6 +1042,7 @@ mod tests { const WRITE_HELPER: &str = "local::server::tests::metadata_write_subprocess"; const NORMALIZE_HELPER: &str = "local::server::tests::metadata_normalize_subprocess"; const CONCURRENCY_HELPER: &str = "local::server::tests::metadata_concurrency_subprocess"; + const LIST_HELPER: &str = "local::server::tests::list_all_servers_ignores_json_directories"; fn test_info(name: &str, pid: u32, version: &str) -> ServerInfo { ServerInfo { @@ -1167,6 +1171,23 @@ mod tests { } } + #[test] + fn list_all_servers_ignores_json_directories() { + if enter_helper_project().is_some() { + assert!(list_all_servers().unwrap().is_empty()); + return; + } + + let project = tempfile::tempdir().unwrap(); + std::fs::create_dir_all(project.path().join(".clickhouse/servers/foo.json")).unwrap(); + + let status = helper(LIST_HELPER, project.path()).status().unwrap(); + assert!( + status.success(), + "metadata list helper failed with {status}" + ); + } + #[test] fn interrupted_atomic_write_keeps_the_previous_document() { let project = tempfile::tempdir().unwrap(); From 0e8a422011cf9b04f60573c3bd9a4ece5c957f1f Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 10:58:25 +0100 Subject: [PATCH 04/16] Check metadata before replacing installed version --- .../src/version_manager/install.rs | 71 ++++++++++++++++--- 1 file changed, 63 insertions(+), 8 deletions(-) diff --git a/crates/clickhousectl/src/version_manager/install.rs b/crates/clickhousectl/src/version_manager/install.rs index aa268dda..288037a4 100644 --- a/crates/clickhousectl/src/version_manager/install.rs +++ b/crates/clickhousectl/src/version_manager/install.rs @@ -180,7 +180,7 @@ pub async fn install_resolved( detect_binary_version(&binary_path)? }; - let replaced_existing = commit_staged_binary( + let notify_running_servers = commit_staged_binary( &versions_dir, &binary_path, &exact_version, @@ -188,15 +188,13 @@ pub async fn install_resolved( is_master, platform, master_head.as_ref(), + !structured_output && is_master, + version_in_use_by_running_server, )?; // Replacing a build on disk never affects already-running servers (they keep // executing the old binary) — just say so, so the swap isn't silent. - if !structured_output - && is_master - && replaced_existing - && version_in_use_by_running_server(&exact_version)? - { + if notify_running_servers { eprintln!( "Note: running servers keep using the previous {} build until restarted", exact_version @@ -223,6 +221,8 @@ fn commit_staged_binary( is_master: bool, platform: &Platform, master_head: Option<&master::HeadInfo>, + check_running_servers: bool, + version_in_use: impl FnOnce(&str) -> Result, ) -> Result { let lock_path = versions_dir .join(".locks") @@ -237,6 +237,8 @@ fn commit_staged_binary( } let replaced_existing = target_binary.exists(); + let notify_running_servers = + check_running_servers && replaced_existing && version_in_use(exact_version)?; let new_head = if is_master { master_head } else { None }; master::commit_install_in(versions_dir, platform, exact_version, new_head, || { if version_dir.exists() { @@ -260,7 +262,7 @@ fn commit_staged_binary( Ok(()) })?; - Ok(replaced_existing) + Ok(notify_running_servers) } #[cfg(test)] @@ -285,7 +287,6 @@ fn pause_after_target_lock_for_test() { #[cfg(not(test))] fn pause_after_target_lock_for_test() {} - /// Like `install_resolved`, but returns the existing version instead of erroring /// when already installed. Intended for cases like `server start --version` where /// the goal is "make sure this version is available" rather than "install this". @@ -541,6 +542,8 @@ mod tests { false, &test_platform("amd64"), None, + false, + |_| Ok(false), ) .map(|_| ()) }); @@ -581,6 +584,8 @@ mod tests { etag, last_modified: None, }), + false, + |_| Ok(false), ) .unwrap(); } @@ -763,6 +768,8 @@ mod tests { etag: "new-etag".to_string(), last_modified: None, }), + false, + |_| Ok(false), ) .unwrap(); drop(staging); @@ -875,6 +882,54 @@ mod tests { assert!(message.contains("clickhouse or usr/bin/clickhouse")); } + #[test] + fn metadata_failure_leaves_existing_install_and_master_record_unchanged() { + let temp = tempfile::tempdir().unwrap(); + let versions_dir = temp.path().join("versions"); + let version_dir = versions_dir.join("26.5.1.1"); + std::fs::create_dir_all(&version_dir).unwrap(); + + let installed_binary = version_dir.join("clickhouse"); + std::fs::write(&installed_binary, b"existing master build").unwrap(); + + let master_record = versions_dir.join(".master-builds.json"); + let original_record = + br#"{"builds":{"macos-aarch64":{"etag":"old","version":"26.5.1.1"}}}"#; + std::fs::write(&master_record, original_record).unwrap(); + + let downloaded_binary = temp.path().join("downloaded-clickhouse"); + std::fs::write(&downloaded_binary, b"replacement master build").unwrap(); + + let platform = test_platform("macos-aarch64"); + let new_head = master::HeadInfo { + etag: "new".to_string(), + last_modified: None, + }; + let result = commit_staged_binary( + &versions_dir, + &downloaded_binary, + "26.5.1.1", + true, + true, + &platform, + Some(&new_head), + true, + |_| { + Err(Error::ServerMetadataRead { + name: "broken".to_string(), + source: std::io::Error::other("metadata unavailable"), + }) + }, + ); + + assert!(matches!(result, Err(Error::ServerMetadataRead { .. }))); + assert_eq!( + std::fs::read(&installed_binary).unwrap(), + b"existing master build" + ); + assert_eq!(std::fs::read(&master_record).unwrap(), original_record); + } + #[test] fn test_parse_version_output_client() { let output = "ClickHouse client version 25.12.9.61 (official build)."; From 333d0d1bb080ab54e16187632b091b8c239d972e Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 11:03:39 +0100 Subject: [PATCH 05/16] Keep start advisory count tolerant --- crates/clickhousectl/src/local/mod.rs | 2 +- crates/clickhousectl/src/local/server.rs | 68 ++++++++++++++++++++++-- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/crates/clickhousectl/src/local/mod.rs b/crates/clickhousectl/src/local/mod.rs index 318aa4f2..79ef6a19 100644 --- a/crates/clickhousectl/src/local/mod.rs +++ b/crates/clickhousectl/src/local/mod.rs @@ -391,7 +391,7 @@ async fn start_server( } // Show running server count - let running = server::running_server_count()?; + let running = server::advisory_running_server_count(); if !json && running > 0 { eprintln!( "Note: {} server{} already running (use `clickhousectl local server list` to see them)", diff --git a/crates/clickhousectl/src/local/server.rs b/crates/clickhousectl/src/local/server.rs index 94be1221..2b28cb7d 100644 --- a/crates/clickhousectl/src/local/server.rs +++ b/crates/clickhousectl/src/local/server.rs @@ -463,6 +463,10 @@ pub fn load_running_info(name: &str) -> Result> { /// it's `-pg`. Also runs process/container discovery so /// orphaned instances reappear. pub fn list_all_servers() -> Result> { + list_all_servers_inner(false) +} + +fn list_all_servers_inner(skip_entry_errors: bool) -> Result> { recover_current_project_servers()?; let dir = servers_dir(); @@ -487,7 +491,12 @@ pub fn list_all_servers() -> Result> { if !entry.path().is_file() { continue; } - let Some((info, running)) = normalize_server_info(stem)? else { + let normalized = match normalize_server_info(stem) { + Ok(normalized) => normalized, + Err(_) if skip_entry_errors => continue, + Err(error) => return Err(error), + }; + let Some((info, running)) = normalized else { continue; }; @@ -566,9 +575,11 @@ pub fn is_server_running(name: &str) -> Result { Ok(load_running_info(name)?.is_some()) } -/// Count running servers. -pub fn running_server_count() -> Result { - Ok(list_running_servers()?.len()) +/// Best-effort count used only for the informational notice during start. +pub fn advisory_running_server_count() -> usize { + list_all_servers_inner(true) + .map(|entries| entries.iter().filter(|entry| entry.running).count()) + .unwrap_or_default() } fn is_process_alive(pid: u32) -> bool { @@ -1043,6 +1054,9 @@ mod tests { const NORMALIZE_HELPER: &str = "local::server::tests::metadata_normalize_subprocess"; const CONCURRENCY_HELPER: &str = "local::server::tests::metadata_concurrency_subprocess"; const LIST_HELPER: &str = "local::server::tests::list_all_servers_ignores_json_directories"; + const ADVISORY_COUNT_HELPER: &str = + "local::server::tests::advisory_count_ignores_unrelated_corrupt_metadata"; + const STRICT_LIST_HELPER: &str = "local::server::tests::strict_list_reports_corrupt_metadata"; fn test_info(name: &str, pid: u32, version: &str) -> ServerInfo { ServerInfo { @@ -1188,6 +1202,52 @@ mod tests { ); } + #[test] + fn advisory_count_ignores_unrelated_corrupt_metadata() { + if enter_helper_project().is_some() { + assert_eq!(advisory_running_server_count(), 1); + return; + } + + let project = tempfile::tempdir().unwrap(); + let servers = project.path().join(".clickhouse/servers"); + std::fs::create_dir_all(&servers).unwrap(); + std::fs::write( + servers.join("healthy.json"), + serde_json::to_vec_pretty(&test_info("healthy", std::process::id(), "26.8.1.1")) + .unwrap(), + ) + .unwrap(); + std::fs::write(servers.join("corrupt.json"), b"not json").unwrap(); + + let status = helper(ADVISORY_COUNT_HELPER, project.path()) + .status() + .unwrap(); + assert!( + status.success(), + "advisory count helper failed with {status}" + ); + } + + #[test] + fn strict_list_reports_corrupt_metadata() { + if enter_helper_project().is_some() { + assert!(matches!( + list_all_servers(), + Err(Error::ServerMetadataParse { name, .. }) if name == "corrupt" + )); + return; + } + + let project = tempfile::tempdir().unwrap(); + let servers = project.path().join(".clickhouse/servers"); + std::fs::create_dir_all(&servers).unwrap(); + std::fs::write(servers.join("corrupt.json"), b"not json").unwrap(); + + let status = helper(STRICT_LIST_HELPER, project.path()).status().unwrap(); + assert!(status.success(), "strict list helper failed with {status}"); + } + #[test] fn interrupted_atomic_write_keeps_the_previous_document() { let project = tempfile::tempdir().unwrap(); From b18ab6b3359f3a240c810c43fa2b2c5a7739d2ab Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 11:10:22 +0100 Subject: [PATCH 06/16] Prepare Postgres image before lifecycle lock --- crates/clickhousectl/src/local/postgres.rs | 90 ++++--- crates/clickhousectl/src/local/server.rs | 9 + .../tests/local_postgres_start_lock_test.rs | 241 ++++++++++++++++++ 3 files changed, 303 insertions(+), 37 deletions(-) create mode 100644 crates/clickhousectl/tests/local_postgres_start_lock_test.rs diff --git a/crates/clickhousectl/src/local/postgres.rs b/crates/clickhousectl/src/local/postgres.rs index fe0a9cf0..49097faa 100644 --- a/crates/clickhousectl/src/local/postgres.rs +++ b/crates/clickhousectl/src/local/postgres.rs @@ -154,8 +154,6 @@ async fn start( // Disk identifier — uniquely scopes (name, major) so two majors of the // same name never share container/data/metadata. let key = server::pg_instance_key(&user_name, &major); - let metadata_lock = server::ServerLock::acquire(&key)?; - let docker = docker::connect().await?; let project_cwd = std::env::current_dir() @@ -163,48 +161,66 @@ async fn start( .map(|p| p.display().to_string()) .unwrap_or_default(); - // Resume path: an instance for this exact (name, major) already exists. - if let Some(prior) = metadata_lock.load_info()? { - let cid = prior.container_id.as_deref().unwrap_or(""); - let container_present = - !cid.is_empty() && docker.inspect_container(cid, None).await.is_ok(); - if container_present { - if metadata_lock.is_running()? { - return Err(Error::ServerAlreadyRunning(user_name)); - } - if !json - && (port.is_some() - || user.is_some() - || password.is_some() - || database.is_some() - || has_extra_env) - { - eprintln!( - "Note: postgres:{major} '{}' already exists; resuming with stored settings. \ - Run `local postgres remove {}` to start over.", - user_name, user_name - ); + let metadata_lock = loop { + // Image preparation can take minutes and does not mutate this + // instance. The existence probe is only advisory; the locked read + // below is authoritative. + let metadata_was_present = server::server_metadata_exists(&key)?; + if !metadata_was_present && !docker::image_exists(&docker, tag).await? { + docker::pull_image(&docker, tag, json).await?; + } + + let metadata_lock = server::ServerLock::acquire(&key)?; + + // Resume path: an instance for this exact (name, major) already exists. + if let Some(prior) = metadata_lock.load_info()? { + let cid = prior.container_id.as_deref().unwrap_or(""); + let container_present = + !cid.is_empty() && docker.inspect_container(cid, None).await.is_ok(); + if container_present { + if metadata_lock.is_running()? { + return Err(Error::ServerAlreadyRunning(user_name)); + } + if !json + && (port.is_some() + || user.is_some() + || password.is_some() + || database.is_some() + || has_extra_env) + { + eprintln!( + "Note: postgres:{major} '{}' already exists; resuming with stored settings. \ + Run `local postgres remove {}` to start over.", + user_name, user_name + ); + } + return resume_existing(&docker, prior, &metadata_lock, json).await; } - return resume_existing(&docker, prior, &metadata_lock, json).await; + // Metadata orphaned — container removed externally. Force explicit + // cleanup to avoid silently re-initing against potentially-stale data. + return Err(Error::PostgresRuntime(format!( + "Postgres '{}' (postgres:{}) has metadata but the container is gone. \ + Run `clickhousectl local postgres remove {}` to clear the data dir \ + and start fresh.", + user_name, major, user_name + ))); } - // Metadata orphaned — container removed externally. Force explicit - // cleanup to avoid silently re-initing against potentially-stale data. - return Err(Error::PostgresRuntime(format!( - "Postgres '{}' (postgres:{}) has metadata but the container is gone. \ - Run `clickhousectl local postgres remove {}` to clear the data dir \ - and start fresh.", - user_name, major, user_name - ))); - } - // Fresh create. + if !metadata_was_present { + break metadata_lock; + } + + // The instance was removed between the optimistic probe and locked + // read. Retry image preparation without holding the lifecycle lock. + drop(metadata_lock); + }; + + // Fresh create. Explicit ports were validated before Docker work; an + // omitted port is selected only after existing-instance resolution. let host_port = match host_port { Some(port) => port, None => resolve_port(None)?, }; - if !docker::image_exists(&docker, tag).await? { - docker::pull_image(&docker, tag, json).await?; - } let instance_dir = server::servers_dir_join(&key); let remove_fresh_data_on_failure = fresh_instance_dir_is_disposable(&instance_dir); diff --git a/crates/clickhousectl/src/local/server.rs b/crates/clickhousectl/src/local/server.rs index 2b28cb7d..b0b890ba 100644 --- a/crates/clickhousectl/src/local/server.rs +++ b/crates/clickhousectl/src/local/server.rs @@ -108,6 +108,15 @@ fn server_meta_path(name: &str) -> PathBuf { servers_dir().join(format!("{}.json", name)) } +/// Optimistic existence check for work that is safe to perform without the +/// lifecycle lock. Callers must re-read metadata after acquiring the lock. +pub(crate) fn server_metadata_exists(name: &str) -> Result { + validate_server_name(name)?; + server_meta_path(name) + .try_exists() + .map_err(|source| metadata_access_error(name, source)) +} + fn server_lock_path(name: &str) -> PathBuf { servers_dir().join(".locks").join(format!("{}.lock", name)) } diff --git a/crates/clickhousectl/tests/local_postgres_start_lock_test.rs b/crates/clickhousectl/tests/local_postgres_start_lock_test.rs new file mode 100644 index 00000000..a35eaf9f --- /dev/null +++ b/crates/clickhousectl/tests/local_postgres_start_lock_test.rs @@ -0,0 +1,241 @@ +//! Concurrency coverage for Postgres start's per-instance lifecycle lock. + +use std::fs::{File, OpenOptions}; +use std::io::{ErrorKind, Read, Write}; +use std::os::fd::AsRawFd; +use std::os::unix::net::{UnixListener, UnixStream}; +use std::path::{Path, PathBuf}; +use std::process::{Child, Command, Output, Stdio}; +use std::sync::mpsc::{self, Receiver, SyncSender}; +use std::thread::{self, JoinHandle}; +use std::time::{Duration, Instant}; + +fn clickhousectl_binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_clickhousectl")) +} + +fn write_response(stream: &mut UnixStream, status: &str, content_type: &str, body: &str) { + let response = format!( + "HTTP/1.1 {status}\r\nContent-Type: {content_type}\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ); + stream + .write_all(response.as_bytes()) + .expect("write fake Docker response"); +} + +fn read_request(stream: &mut UnixStream) -> String { + let mut request = Vec::new(); + let mut buffer = [0_u8; 1024]; + while !request.windows(4).any(|window| window == b"\r\n\r\n") { + let bytes = stream.read(&mut buffer).expect("read Docker request"); + assert!(bytes > 0, "Docker request ended before its headers"); + request.extend_from_slice(&buffer[..bytes]); + } + String::from_utf8(request).expect("Docker request is UTF-8") +} + +fn accept_connection(listener: &UnixListener, operation: &str) -> UnixStream { + let deadline = Instant::now() + Duration::from_secs(5); + loop { + match listener.accept() { + Ok((stream, _)) => { + stream + .set_nonblocking(false) + .expect("make Docker connection blocking"); + return stream; + } + Err(error) if error.kind() == ErrorKind::WouldBlock && Instant::now() < deadline => { + thread::sleep(Duration::from_millis(10)); + } + Err(error) => panic!("accept Docker {operation}: {error}"), + } + } +} + +fn expect_request(listener: &UnixListener, operation: &str, prefix: &str) -> UnixStream { + let mut stream = accept_connection(listener, operation); + let request = read_request(&mut stream); + assert!( + request.starts_with(prefix), + "unexpected Docker {operation} request: {request}" + ); + stream +} + +fn spawn_fake_docker( + socket_path: &Path, + pull_started: SyncSender<()>, + release_pull: Receiver<()>, +) -> JoinHandle<()> { + let listener = UnixListener::bind(socket_path).expect("bind fake Docker socket"); + listener + .set_nonblocking(true) + .expect("make fake Docker socket nonblocking"); + + thread::spawn(move || { + let mut recovery_ping = expect_request(&listener, "recovery ping", "GET /_ping "); + write_response(&mut recovery_ping, "200 OK", "text/plain", "OK"); + + let mut recovery_list = expect_request( + &listener, + "recovery container list", + "GET /containers/json?", + ); + write_response(&mut recovery_list, "200 OK", "application/json", "[]"); + + let mut start_ping = expect_request(&listener, "start ping", "GET /_ping "); + write_response(&mut start_ping, "200 OK", "text/plain", "OK"); + + let mut image_inspect = + expect_request(&listener, "image inspect", "GET /images/postgres:18/json "); + write_response( + &mut image_inspect, + "404 Not Found", + "application/json", + r#"{"message":"No such image"}"#, + ); + + let mut pull = expect_request(&listener, "image pull", "POST /images/create?"); + pull_started.send(()).expect("signal image pull"); + release_pull + .recv_timeout(Duration::from_secs(5)) + .expect("release image pull"); + write_response( + &mut pull, + "200 OK", + "application/json", + "{\"status\":\"Pull complete\"}\n", + ); + }) +} + +fn try_lock_instance(path: &Path) -> std::io::Result { + std::fs::create_dir_all(path.parent().expect("lock parent"))?; + let file = OpenOptions::new() + .create(true) + .read(true) + .write(true) + .truncate(false) + .open(path)?; + let result = unsafe { libc::flock(file.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) }; + if result == 0 { + Ok(file) + } else { + Err(std::io::Error::last_os_error()) + } +} + +fn unlock_instance(file: &File) { + let result = unsafe { libc::flock(file.as_raw_fd(), libc::LOCK_UN) }; + assert_eq!( + result, + 0, + "unlock instance: {}", + std::io::Error::last_os_error() + ); +} + +fn wait_for_output(mut child: Child) -> Output { + let deadline = Instant::now() + Duration::from_secs(5); + loop { + if child.try_wait().expect("poll clickhousectl").is_some() { + return child + .wait_with_output() + .expect("collect clickhousectl output"); + } + if Instant::now() >= deadline { + child.kill().expect("kill timed out clickhousectl"); + let output = child.wait_with_output().expect("collect timed out output"); + panic!( + "clickhousectl timed out\nstderr: {}\nstdout: {}", + String::from_utf8_lossy(&output.stderr), + String::from_utf8_lossy(&output.stdout) + ); + } + thread::sleep(Duration::from_millis(10)); + } +} + +#[test] +fn postgres_start_pulls_before_lock_and_revalidates_before_create() { + let project = tempfile::tempdir().expect("create project"); + let socket_path = project.path().join("docker.sock"); + let (pull_started_tx, pull_started_rx) = mpsc::sync_channel(0); + let (release_pull_tx, release_pull_rx) = mpsc::sync_channel(0); + let daemon = spawn_fake_docker(&socket_path, pull_started_tx, release_pull_rx); + + let mut command = Command::new(clickhousectl_binary()); + command + .env_clear() + .env("DO_NOT_TRACK", "1") + .env("HOME", project.path()) + .env("DOCKER_HOST", format!("unix://{}", socket_path.display())) + .current_dir(project.path()) + .args([ + "local", + "postgres", + "start", + "--name", + "default", + "--version", + "18", + ]) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + let mut child = command.spawn().expect("run clickhousectl"); + + pull_started_rx + .recv_timeout(Duration::from_secs(5)) + .expect("wait for image pull"); + + let lock_path = project + .path() + .join(".clickhouse/servers/.locks/default-pg18.lock"); + let instance_lock = match try_lock_instance(&lock_path) { + Ok(lock) => lock, + Err(error) => { + release_pull_tx.send(()).expect("release image pull"); + child.kill().expect("kill clickhousectl"); + child.wait().expect("reap clickhousectl"); + daemon.join().expect("fake Docker daemon"); + panic!("instance lock was held during image pull: {error}"); + } + }; + + let metadata_path = project.path().join(".clickhouse/servers/default-pg18.json"); + let metadata = serde_json::json!({ + "name": "default-pg18", + "pid": 0, + "version": "postgres:18", + "http_port": 0, + "tcp_port": 5432, + "started_at": "concurrent", + "cwd": project.path().canonicalize().unwrap(), + "engine": "postgres" + }); + std::fs::write( + &metadata_path, + serde_json::to_vec_pretty(&metadata).unwrap(), + ) + .expect("write concurrent metadata"); + + release_pull_tx.send(()).expect("release image pull"); + thread::sleep(Duration::from_millis(250)); + assert!( + child.try_wait().expect("poll blocked start").is_none(), + "start did not wait for the lifecycle lock before revalidation" + ); + + unlock_instance(&instance_lock); + let output = wait_for_output(child); + daemon.join().expect("fake Docker daemon"); + + let stderr = String::from_utf8(output.stderr).unwrap(); + assert_eq!(output.status.code(), Some(1), "stderr: {stderr}"); + assert!( + stderr.contains("has metadata but the container is gone"), + "start did not revalidate concurrent metadata: {stderr}" + ); + assert!(metadata_path.exists(), "concurrent metadata was mutated"); +} From 7083bf99303184587feecf01b6227a58b335e65d Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 11:15:03 +0100 Subject: [PATCH 07/16] Skip Postgres recovery setup for existing metadata --- crates/clickhousectl/src/local/docker.rs | 5 +- crates/clickhousectl/src/local/server.rs | 101 ++++++++++++++++++++++- 2 files changed, 100 insertions(+), 6 deletions(-) diff --git a/crates/clickhousectl/src/local/docker.rs b/crates/clickhousectl/src/local/docker.rs index 99b12619..36f26a83 100644 --- a/crates/clickhousectl/src/local/docker.rs +++ b/crates/clickhousectl/src/local/docker.rs @@ -923,7 +923,7 @@ pub fn start_existing_blocking(id: &str) -> Result<()> { /// timeout) and we return silently. pub fn recover_project_postgres_blocking(project_cwd: &str) -> Result<()> { use crate::local::server::{ - Engine, ServerInfo, ensure_pg_data_dir, pg_instance_key, save_recovered_server_info, + Engine, ServerInfo, pg_instance_key, save_recovered_postgres_server_info, }; let cwd_owned = project_cwd.to_string(); block_on(async move { @@ -937,7 +937,6 @@ pub fn recover_project_postgres_blocking(project_cwd: &str) -> Result<()> { }; for c in containers { let key = pg_instance_key(&c.user_name, &c.major); - ensure_pg_data_dir(&c.user_name, &c.major)?; let info = ServerInfo { name: key, pid: 0, @@ -949,7 +948,7 @@ pub fn recover_project_postgres_blocking(project_cwd: &str) -> Result<()> { engine: Engine::Postgres, container_id: Some(c.container_id.clone()), }; - save_recovered_server_info(&info, false)?; + save_recovered_postgres_server_info(&info, &c.user_name, &c.major)?; } Ok(()) }) diff --git a/crates/clickhousectl/src/local/server.rs b/crates/clickhousectl/src/local/server.rs index b0b890ba..525202cc 100644 --- a/crates/clickhousectl/src/local/server.rs +++ b/crates/clickhousectl/src/local/server.rs @@ -959,9 +959,8 @@ pub fn recover_current_project_servers() -> Result<()> { Ok(()) } -/// Install recovered metadata without racing a normal lifecycle write. ClickHouse -/// discovery may replace a stale stopped record; Postgres recovery only fills a -/// genuinely absent record because stopped containers are still valid state. +/// Install recovered ClickHouse metadata without racing a normal lifecycle write. +/// Discovery may replace a stale stopped record. pub fn save_recovered_server_info(info: &ServerInfo, replace_stale: bool) -> Result<()> { let lock = ServerLock::acquire(&info.name)?; if let Some(existing) = lock.load_info()? @@ -972,6 +971,21 @@ pub fn save_recovered_server_info(info: &ServerInfo, replace_stale: bool) -> Res lock.save_info(info) } +/// Install recovered Postgres metadata only when no lifecycle record exists. +/// Keep the existence check, data-directory creation, and write under one lock. +pub fn save_recovered_postgres_server_info( + info: &ServerInfo, + user_name: &str, + major: &str, +) -> Result<()> { + let lock = ServerLock::acquire(&info.name)?; + if lock.load_info()?.is_some() { + return Ok(()); + } + ensure_pg_data_dir(user_name, major)?; + lock.save_info(info) +} + /// A server entry for global listing — always running (discovered via process inspection). pub struct GlobalServerEntry { pub name: String, @@ -1066,6 +1080,7 @@ mod tests { const ADVISORY_COUNT_HELPER: &str = "local::server::tests::advisory_count_ignores_unrelated_corrupt_metadata"; const STRICT_LIST_HELPER: &str = "local::server::tests::strict_list_reports_corrupt_metadata"; + const POSTGRES_RECOVERY_HELPER: &str = "local::server::tests::postgres_recovery_subprocess"; fn test_info(name: &str, pid: u32, version: &str) -> ServerInfo { ServerInfo { @@ -1081,6 +1096,20 @@ mod tests { } } + fn test_postgres_info(version: &str) -> ServerInfo { + ServerInfo { + name: pg_instance_key("default", "17"), + pid: 0, + version: version.to_string(), + http_port: 0, + tcp_port: 5432, + started_at: "recovered".to_string(), + cwd: "/tmp/project".to_string(), + engine: Engine::Postgres, + container_id: Some("container-17".to_string()), + } + } + fn metadata_path(project: &Path) -> PathBuf { project.join(".clickhouse/servers/default.json") } @@ -1194,6 +1223,72 @@ mod tests { } } + #[test] + fn postgres_recovery_subprocess() { + if enter_helper_project().is_none() { + return; + } + save_recovered_postgres_server_info( + &test_postgres_info("postgres:17-recovered"), + "default", + "17", + ) + .unwrap(); + } + + #[test] + fn postgres_recovery_skips_failing_data_path_when_metadata_exists() { + let project = tempfile::tempdir().unwrap(); + let servers = project.path().join(".clickhouse/servers"); + let key = pg_instance_key("default", "17"); + let metadata = servers.join(format!("{key}.json")); + std::fs::create_dir_all(&servers).unwrap(); + std::fs::write( + &metadata, + serde_json::to_vec_pretty(&test_postgres_info("postgres:17-existing")).unwrap(), + ) + .unwrap(); + std::fs::write(servers.join(&key), b"blocks data directory creation").unwrap(); + + let status = helper(POSTGRES_RECOVERY_HELPER, project.path()) + .status() + .unwrap(); + assert!( + status.success(), + "Postgres recovery helper failed with {status}" + ); + + let live: ServerInfo = serde_json::from_slice(&std::fs::read(metadata).unwrap()).unwrap(); + assert_eq!(live.version, "postgres:17-existing"); + } + + #[test] + fn postgres_recovery_creates_data_dir_and_metadata_when_absent() { + let project = tempfile::tempdir().unwrap(); + + let status = helper(POSTGRES_RECOVERY_HELPER, project.path()) + .status() + .unwrap(); + assert!( + status.success(), + "Postgres recovery helper failed with {status}" + ); + + assert!( + project + .path() + .join(".clickhouse/servers/default-pg17/data") + .is_dir() + ); + let live: ServerInfo = serde_json::from_slice( + &std::fs::read(project.path().join(".clickhouse/servers/default-pg17.json")).unwrap(), + ) + .unwrap(); + assert_eq!(live.version, "postgres:17-recovered"); + assert_eq!(live.engine, Engine::Postgres); + assert_eq!(live.container_id.as_deref(), Some("container-17")); + } + #[test] fn list_all_servers_ignores_json_directories() { if enter_helper_project().is_some() { From 6a20fd4d483ac2a66fc4bcc8e863e602fb86ba2f Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 12:23:25 +0100 Subject: [PATCH 08/16] Preserve Postgres metadata during ClickHouse recovery --- crates/clickhousectl/src/local/server.rs | 60 ++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 3 deletions(-) diff --git a/crates/clickhousectl/src/local/server.rs b/crates/clickhousectl/src/local/server.rs index 525202cc..81a70ab6 100644 --- a/crates/clickhousectl/src/local/server.rs +++ b/crates/clickhousectl/src/local/server.rs @@ -964,7 +964,7 @@ pub fn recover_current_project_servers() -> Result<()> { pub fn save_recovered_server_info(info: &ServerInfo, replace_stale: bool) -> Result<()> { let lock = ServerLock::acquire(&info.name)?; if let Some(existing) = lock.load_info()? - && (!replace_stale || is_alive(&existing)) + && (!replace_stale || existing.engine != Engine::Clickhouse || is_alive(&existing)) { return Ok(()); } @@ -1081,6 +1081,7 @@ mod tests { "local::server::tests::advisory_count_ignores_unrelated_corrupt_metadata"; const STRICT_LIST_HELPER: &str = "local::server::tests::strict_list_reports_corrupt_metadata"; const POSTGRES_RECOVERY_HELPER: &str = "local::server::tests::postgres_recovery_subprocess"; + const CLICKHOUSE_RECOVERY_HELPER: &str = "local::server::tests::clickhouse_recovery_subprocess"; fn test_info(name: &str, pid: u32, version: &str) -> ServerInfo { ServerInfo { @@ -1111,11 +1112,15 @@ mod tests { } fn metadata_path(project: &Path) -> PathBuf { - project.join(".clickhouse/servers/default.json") + metadata_path_for(project, "default") + } + + fn metadata_path_for(project: &Path, name: &str) -> PathBuf { + project.join(format!(".clickhouse/servers/{name}.json")) } fn write_initial_metadata(project: &Path, info: &ServerInfo) { - let path = metadata_path(project); + let path = metadata_path_for(project, &info.name); std::fs::create_dir_all(path.parent().unwrap()).unwrap(); std::fs::write(path, serde_json::to_vec_pretty(info).unwrap()).unwrap(); } @@ -1236,6 +1241,55 @@ mod tests { .unwrap(); } + #[test] + fn clickhouse_recovery_subprocess() { + if enter_helper_project().is_none() { + return; + } + let name = std::env::var("CHCTL_TEST_METADATA_NAME").unwrap(); + save_recovered_server_info( + &test_info(&name, std::process::id(), "clickhouse-recovered"), + true, + ) + .unwrap(); + } + + #[test] + fn clickhouse_recovery_preserves_postgres_collision_and_replaces_stale_clickhouse() { + let project = tempfile::tempdir().unwrap(); + let postgres = test_postgres_info("postgres:17-existing"); + let stale_clickhouse = test_info("stale-clickhouse", u32::MAX, "clickhouse-stale"); + write_initial_metadata(project.path(), &postgres); + write_initial_metadata(project.path(), &stale_clickhouse); + + for name in [&postgres.name, &stale_clickhouse.name] { + let status = helper(CLICKHOUSE_RECOVERY_HELPER, project.path()) + .env("CHCTL_TEST_METADATA_NAME", name) + .status() + .unwrap(); + assert!( + status.success(), + "ClickHouse recovery helper failed with {status}" + ); + } + + let live_postgres: ServerInfo = serde_json::from_slice( + &std::fs::read(metadata_path_for(project.path(), &postgres.name)).unwrap(), + ) + .unwrap(); + assert_eq!(live_postgres.version, "postgres:17-existing"); + assert_eq!(live_postgres.engine, Engine::Postgres); + assert_eq!(live_postgres.container_id.as_deref(), Some("container-17")); + + let live_clickhouse: ServerInfo = serde_json::from_slice( + &std::fs::read(metadata_path_for(project.path(), &stale_clickhouse.name)).unwrap(), + ) + .unwrap(); + assert_eq!(live_clickhouse.version, "clickhouse-recovered"); + assert_eq!(live_clickhouse.engine, Engine::Clickhouse); + assert!(live_clickhouse.container_id.is_none()); + } + #[test] fn postgres_recovery_skips_failing_data_path_when_metadata_exists() { let project = tempfile::tempdir().unwrap(); From 71b40774dacca5c7802f2a8179b2b8af6be618c1 Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 12:49:05 +0100 Subject: [PATCH 09/16] Retain Postgres identity after incomplete cleanup --- crates/clickhousectl/src/local/postgres.rs | 29 +++++++++++++--------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/crates/clickhousectl/src/local/postgres.rs b/crates/clickhousectl/src/local/postgres.rs index 49097faa..465d899d 100644 --- a/crates/clickhousectl/src/local/postgres.rs +++ b/crates/clickhousectl/src/local/postgres.rs @@ -371,21 +371,26 @@ async fn rollback_fresh_start( } }; - if let Err(error) = std::fs::remove_file(&rollback.metadata_path) - && error.kind() != std::io::ErrorKind::NotFound - { - diagnostics.push(format!( - "failed to remove metadata '{}': {error}", - rollback.metadata_path.display() - )); - } - if rollback.remove_fresh_data_on_failure && container_removed { - if let Err(error) = docker::remove_host_dir_blocking(&rollback.instance_dir) { - diagnostics.push(format!( + match docker::remove_host_dir_blocking(&rollback.instance_dir) { + Ok(()) if !rollback.instance_dir.exists() => { + if let Err(error) = std::fs::remove_file(&rollback.metadata_path) + && error.kind() != std::io::ErrorKind::NotFound + { + diagnostics.push(format!( + "failed to remove metadata '{}': {error}", + rollback.metadata_path.display() + )); + } + } + Ok(()) => diagnostics.push(format!( + "failed to remove fresh Postgres data '{}': path still exists", + rollback.instance_dir.display() + )), + Err(error) => diagnostics.push(format!( "failed to remove fresh Postgres data '{}': {error}", rollback.instance_dir.display() - )); + )), } } else { let reason = if container_removed { From c2c2303b631e6f8c5bbc3135dff615ca87365121 Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 13:06:11 +0100 Subject: [PATCH 10/16] Expect metadata validation before Docker startup --- .../tests/local_postgres_readiness_test.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/crates/clickhousectl/tests/local_postgres_readiness_test.rs b/crates/clickhousectl/tests/local_postgres_readiness_test.rs index fc3d415d..91f4b17c 100644 --- a/crates/clickhousectl/tests/local_postgres_readiness_test.rs +++ b/crates/clickhousectl/tests/local_postgres_readiness_test.rs @@ -515,7 +515,7 @@ fn create_success_start_failure_rolls_back_container_and_partial_pgdata() { } #[test] -fn metadata_failure_uses_the_same_fresh_start_rollback() { +fn metadata_failure_precedes_docker_mutation() { let project = tempfile::tempdir().expect("create project tempdir"); let metadata_path = project .path() @@ -527,16 +527,13 @@ fn metadata_failure_uses_the_same_fresh_start_rollback() { StartBehavior::DelayedReady { ready_on_probe: 1 }, ); - let output = run_start(project.path(), &docker, "metadata-failure", false); + let output = run_start(project.path(), &docker, "metadata-failure", true); assert_eq!(output.status.code(), Some(1)); - let stderr = String::from_utf8_lossy(&output.stderr); - assert!(stderr.contains("IO error"), "stderr: {stderr}"); - assert!( - stderr.contains("failed to remove metadata"), - "stderr: {stderr}" - ); - assert_eq!(docker.starts(), 1); - assert_eq!(docker.removes(), 1); + let error: Value = serde_json::from_slice(&output.stderr).expect("parse metadata error JSON"); + assert_eq!(error["error"]["code"], "server_metadata_read"); + assert_eq!(docker.starts(), 0); + assert_eq!(docker.removes(), 0); + assert!(metadata_path.is_dir(), "metadata collision was mutated"); assert!( !project .path() From eeb08f602ccf7ea04c090d18e95b6349943a7754 Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 17:36:54 +0100 Subject: [PATCH 11/16] Sync metadata directory after rename --- crates/clickhousectl/src/local/server.rs | 51 +++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/crates/clickhousectl/src/local/server.rs b/crates/clickhousectl/src/local/server.rs index 81a70ab6..82369347 100644 --- a/crates/clickhousectl/src/local/server.rs +++ b/crates/clickhousectl/src/local/server.rs @@ -259,6 +259,17 @@ fn metadata_write_error(name: &str, source: std::io::Error) -> Error { } } +fn sync_metadata_directory(path: &Path) -> std::io::Result<()> { + #[cfg(test)] + if std::env::var_os("CHCTL_TEST_METADATA_DIR_SYNC_FAILURE").is_some() { + return Err(std::io::Error::other( + "injected metadata directory sync failure", + )); + } + + File::open(path)?.sync_all() +} + fn read_server_info(name: &str) -> Result> { let content = match std::fs::read(server_meta_path(name)) { Ok(content) => content, @@ -275,6 +286,9 @@ fn read_server_info(name: &str) -> Result> { fn write_server_info(name: &str, info: &ServerInfo) -> Result<()> { let path = server_meta_path(name); + let directory = path + .parent() + .expect("server metadata path has a parent directory"); let file_name = path .file_name() .expect("server metadata path has a file name") @@ -301,6 +315,7 @@ fn write_server_info(name: &str, info: &ServerInfo) -> Result<()> { .map_err(|source| metadata_write_error(name, source))?; pause_before_metadata_rename_for_test(); std::fs::rename(&temp_path, &path).map_err(|source| metadata_write_error(name, source))?; + sync_metadata_directory(directory).map_err(|source| metadata_write_error(name, source))?; temporary.committed = true; Ok(()) } @@ -1172,7 +1187,17 @@ mod tests { if let Some(attempt) = std::env::var_os("CHCTL_TEST_METADATA_ATTEMPT") { std::fs::write(attempt, b"attempt").unwrap(); } - save_server_info(&test_info("default", pid, &version)).unwrap(); + let result = save_server_info(&test_info("default", pid, &version)); + if std::env::var_os("CHCTL_TEST_METADATA_DIR_SYNC_FAILURE").is_some() { + assert!(matches!( + result, + Err(Error::ServerMetadataWrite { name, source }) + if name == "default" + && source.to_string() == "injected metadata directory sync failure" + )); + } else { + result.unwrap(); + } } #[test] @@ -1438,6 +1463,30 @@ mod tests { assert_eq!(abandoned, 1); } + #[test] + fn directory_sync_failure_is_reported_after_rename() { + let project = tempfile::tempdir().unwrap(); + write_initial_metadata(project.path(), &test_info("default", 0, "old")); + + let status = helper(WRITE_HELPER, project.path()) + .env("CHCTL_TEST_METADATA_PID", "12345") + .env("CHCTL_TEST_METADATA_VERSION", "new") + .env("CHCTL_TEST_METADATA_DIR_SYNC_FAILURE", "1") + .status() + .unwrap(); + assert!(status.success(), "metadata helper failed with {status}"); + + let live: ServerInfo = + serde_json::from_slice(&std::fs::read(metadata_path(project.path())).unwrap()).unwrap(); + assert_eq!(live.version, "new"); + let temporary_files = std::fs::read_dir(project.path().join(".clickhouse/servers")) + .unwrap() + .filter_map(|entry| entry.ok()) + .filter(|entry| entry.file_name().to_string_lossy().ends_with(".tmp")) + .count(); + assert_eq!(temporary_files, 0); + } + #[test] fn stale_normalizer_cannot_overwrite_a_concurrent_restart() { let project = tempfile::tempdir().unwrap(); From 91a5e6fc24e817e72b538ef1a031e44dfb123a0a Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 17:41:37 +0100 Subject: [PATCH 12/16] Report lifecycle lock failures accurately --- crates/clickhousectl/src/error.rs | 9 ++ crates/clickhousectl/src/local/output.rs | 16 ++++ crates/clickhousectl/src/local/server.rs | 91 ++++++++++++++++++- .../tests/local_server_metadata_test.rs | 73 +++++++++++++++ 4 files changed, 184 insertions(+), 5 deletions(-) diff --git a/crates/clickhousectl/src/error.rs b/crates/clickhousectl/src/error.rs index de74f462..e029ad76 100644 --- a/crates/clickhousectl/src/error.rs +++ b/crates/clickhousectl/src/error.rs @@ -132,6 +132,15 @@ pub enum Error { #[error("Server '{0}' not found")] ServerNotFound(String), + #[error("Could not {operation} at {}: {source}. {remediation}", path.display())] + ServerLock { + operation: &'static str, + path: PathBuf, + remediation: &'static str, + #[source] + source: std::io::Error, + }, + #[error( "Could not read metadata for server '{name}' at .clickhouse/servers/{name}.json: {source}. Check that the file is readable and retry." )] diff --git a/crates/clickhousectl/src/local/output.rs b/crates/clickhousectl/src/local/output.rs index f0ba903e..c5de79d9 100644 --- a/crates/clickhousectl/src/local/output.rs +++ b/crates/clickhousectl/src/local/output.rs @@ -15,6 +15,7 @@ pub enum LocalErrorCode { ServerNotFound, ServerNotRunning, ServerRunning, + ServerLock, ServerMetadataRead, ServerMetadataPermission, ServerMetadataInvalid, @@ -64,6 +65,11 @@ impl LocalErrorOutput { error.to_string(), "clickhousectl local server list", ), + Error::ServerLock { .. } => ( + LocalErrorCode::ServerLock, + error.to_string(), + "clickhousectl local server list", + ), Error::ServerMetadataRead { .. } => ( LocalErrorCode::ServerMetadataRead, error.to_string(), @@ -796,6 +802,16 @@ mod tests { LocalErrorCode::ServerRunning, "clickhousectl local server list", ), + ( + Error::ServerLock { + operation: "open server lifecycle lock file", + path: ".clickhouse/servers/.locks/default.lock".into(), + remediation: "Check lock access and retry.", + source: std::io::Error::other("open failed"), + }, + LocalErrorCode::ServerLock, + "clickhousectl local server list", + ), ( Error::ServerMetadataRead { name: "default".into(), diff --git a/crates/clickhousectl/src/local/server.rs b/crates/clickhousectl/src/local/server.rs index 82369347..707b4109 100644 --- a/crates/clickhousectl/src/local/server.rs +++ b/crates/clickhousectl/src/local/server.rs @@ -183,9 +183,16 @@ struct FileLock { } impl FileLock { - fn acquire(path: &Path, name: &str) -> Result { + fn acquire(path: &Path) -> Result { if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent).map_err(|source| metadata_write_error(name, source))?; + std::fs::create_dir_all(parent).map_err(|source| { + server_lock_error( + "create server lifecycle lock directory", + parent, + "Check write access to the parent directory and retry.", + source, + ) + })?; } let file = OpenOptions::new() .create(true) @@ -193,7 +200,14 @@ impl FileLock { .write(true) .truncate(false) .open(path) - .map_err(|source| metadata_access_error(name, source))?; + .map_err(|source| { + server_lock_error( + "open server lifecycle lock file", + path, + "Check read and write access to the lock file and its parent directory, then retry.", + source, + ) + })?; loop { // SAFETY: `file` owns this descriptor for the lifetime of the lock. @@ -203,7 +217,12 @@ impl FileLock { } let source = std::io::Error::last_os_error(); if source.kind() != std::io::ErrorKind::Interrupted { - return Err(metadata_access_error(name, source)); + return Err(server_lock_error( + "acquire server lifecycle lock", + path, + "Check that the lock file's filesystem supports advisory locks, then retry.", + source, + )); } } } @@ -231,6 +250,20 @@ impl Drop for TemporaryMetadata { } } +fn server_lock_error( + operation: &'static str, + path: &Path, + remediation: &'static str, + source: std::io::Error, +) -> Error { + Error::ServerLock { + operation, + path: path.to_path_buf(), + remediation, + source, + } +} + fn metadata_access_error(name: &str, source: std::io::Error) -> Error { if source.kind() == std::io::ErrorKind::PermissionDenied { Error::ServerMetadataPermission { @@ -334,7 +367,7 @@ impl ServerLock { ensure_servers_dir()?; Ok(Self { name: name.to_string(), - _file: FileLock::acquire(&server_lock_path(name), name)?, + _file: FileLock::acquire(&server_lock_path(name))?, }) } @@ -1098,6 +1131,54 @@ mod tests { const POSTGRES_RECOVERY_HELPER: &str = "local::server::tests::postgres_recovery_subprocess"; const CLICKHOUSE_RECOVERY_HELPER: &str = "local::server::tests::clickhouse_recovery_subprocess"; + #[test] + fn lock_directory_failure_preserves_operation_path_and_source() { + let project = tempfile::tempdir().unwrap(); + let lock_directory = project.path().join(".clickhouse/servers/.locks"); + std::fs::create_dir_all(lock_directory.parent().unwrap()).unwrap(); + std::fs::write(&lock_directory, b"not a directory").unwrap(); + let lock_path = lock_directory.join("default.lock"); + + let error = match FileLock::acquire(&lock_path) { + Ok(_) => panic!("lock acquisition unexpectedly succeeded"), + Err(error) => error, + }; + assert!(std::error::Error::source(&error).is_some()); + assert_eq!(error.exit_code(), 1); + assert!(matches!( + error, + Error::ServerLock { + operation: "create server lifecycle lock directory", + path, + source, + .. + } if path == lock_directory + && source.kind() == std::io::ErrorKind::AlreadyExists + )); + } + + #[test] + fn flock_failure_context_identifies_the_lock_file() { + let path = PathBuf::from(".clickhouse/servers/.locks/default.lock"); + let error = server_lock_error( + "acquire server lifecycle lock", + &path, + "Check that the lock file's filesystem supports advisory locks, then retry.", + std::io::Error::other("injected flock failure"), + ); + + assert!(std::error::Error::source(&error).is_some()); + assert!(matches!( + error, + Error::ServerLock { + operation: "acquire server lifecycle lock", + path: error_path, + source, + .. + } if error_path == path && source.to_string() == "injected flock failure" + )); + } + fn test_info(name: &str, pid: u32, version: &str) -> ServerInfo { ServerInfo { name: name.to_string(), diff --git a/crates/clickhousectl/tests/local_server_metadata_test.rs b/crates/clickhousectl/tests/local_server_metadata_test.rs index 8c89cbdc..3ae570d1 100644 --- a/crates/clickhousectl/tests/local_server_metadata_test.rs +++ b/crates/clickhousectl/tests/local_server_metadata_test.rs @@ -30,6 +30,14 @@ fn metadata_path(project: &Path) -> PathBuf { project.join(".clickhouse/servers/default.json") } +fn lock_directory(project: &Path) -> PathBuf { + project.join(".clickhouse/servers/.locks") +} + +fn lock_path(project: &Path) -> PathBuf { + lock_directory(project).join("default.lock") +} + fn prepare_project(project: &Path) { std::fs::create_dir_all(project.join(".clickhouse/servers/default/data")) .expect("create server data directory"); @@ -70,6 +78,71 @@ fn assert_json_error(output: &Output, code: &str, message_fragment: &str) { ); } +fn assert_lock_error(output: &Output, operation: &str, path: &Path) { + assert_eq!( + output.status.code(), + Some(1), + "stdout: {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!(output.stdout.is_empty()); + let body: Value = serde_json::from_slice(&output.stderr).expect("parse structured lock error"); + assert_eq!(body["error"]["code"], "server_lock"); + assert_eq!(body["error"]["command"], "clickhousectl local server list"); + let message = body["error"]["message"].as_str().unwrap(); + assert!(message.contains(operation), "{message}"); + assert!(message.contains(&path.display().to_string()), "{message}"); + assert!(message.contains("retry"), "{message}"); + assert!(!message.contains("metadata"), "{message}"); + assert!(!message.contains("default.json"), "{message}"); +} + +#[test] +fn lock_directory_creation_failure_reports_the_lock_directory() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + let locks = lock_directory(project.path()); + std::fs::create_dir_all(locks.parent().unwrap()).expect("create servers directory"); + std::fs::write(&locks, b"blocks lock directory creation").expect("block lock directory"); + + let json = run(project.path(), home.path(), true); + assert_lock_error(&json, "create server lifecycle lock directory", &locks); + + let human = run(project.path(), home.path(), false); + assert_eq!(human.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&human.stderr); + assert!( + stderr.starts_with("Error: Could not create server lifecycle lock directory"), + "{stderr}" + ); + assert!(stderr.contains(&locks.display().to_string()), "{stderr}"); + assert!(!stderr.contains("metadata"), "{stderr}"); + assert!(!stderr.contains("default.json"), "{stderr}"); +} + +#[test] +fn lock_file_open_failure_reports_the_lock_file() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + let lock = lock_path(project.path()); + std::fs::create_dir_all(&lock).expect("create directory at lock file path"); + + let json = run(project.path(), home.path(), true); + assert_lock_error(&json, "open server lifecycle lock file", &lock); + + let human = run(project.path(), home.path(), false); + assert_eq!(human.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&human.stderr); + assert!( + stderr.starts_with("Error: Could not open server lifecycle lock file"), + "{stderr}" + ); + assert!(stderr.contains(&lock.display().to_string()), "{stderr}"); + assert!(!stderr.contains("metadata"), "{stderr}"); + assert!(!stderr.contains("default.json"), "{stderr}"); +} + #[test] fn selected_partial_json_and_invalid_utf8_are_parse_errors() { for (label, contents) in [ From 041676de79b3c2b2a8ccd59a4fd9689d5e299ed0 Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 17:46:40 +0100 Subject: [PATCH 13/16] Serialize Postgres dotenv credential reads --- crates/clickhousectl/src/local/postgres.rs | 2 +- .../tests/local_postgres_dotenv_lock_test.rs | 326 ++++++++++++++++++ 2 files changed, 327 insertions(+), 1 deletion(-) create mode 100644 crates/clickhousectl/tests/local_postgres_dotenv_lock_test.rs diff --git a/crates/clickhousectl/src/local/postgres.rs b/crates/clickhousectl/src/local/postgres.rs index 465d899d..d04eff4a 100644 --- a/crates/clickhousectl/src/local/postgres.rs +++ b/crates/clickhousectl/src/local/postgres.rs @@ -957,12 +957,12 @@ fn dotenv(name: Option<&str>, version: Option<&str>, use_local: bool, json: bool if !metadata_lock.is_running()? { return Err(Error::ServerNotRunning(server_name.to_string())); } - drop(metadata_lock); // Read user/password/db from the container env so we always emit accurate creds. let (user, password, database) = docker::block_on(read_pg_env_for_dotenv( info.container_id.as_deref().unwrap_or_default(), )); + drop(metadata_lock); let vars: Vec<(&str, String)> = vec![ ("POSTGRES_HOST", "127.0.0.1".to_string()), diff --git a/crates/clickhousectl/tests/local_postgres_dotenv_lock_test.rs b/crates/clickhousectl/tests/local_postgres_dotenv_lock_test.rs new file mode 100644 index 00000000..5cd805f2 --- /dev/null +++ b/crates/clickhousectl/tests/local_postgres_dotenv_lock_test.rs @@ -0,0 +1,326 @@ +//! Concurrency coverage for Postgres dotenv's per-instance lifecycle lock. + +use serde_json::json; +use std::io::{ErrorKind, Read, Write}; +use std::os::unix::net::{UnixListener, UnixStream}; +use std::path::{Path, PathBuf}; +use std::process::{Child, Command, Output, Stdio}; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::mpsc::{self, Receiver, SyncSender}; +use std::sync::{Arc, Condvar, Mutex}; +use std::thread::{self, JoinHandle}; +use std::time::{Duration, Instant}; + +fn clickhousectl_binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_clickhousectl")) +} + +fn read_request(stream: &mut UnixStream) -> String { + let mut request = Vec::new(); + let mut buffer = [0_u8; 1024]; + while !request.windows(4).any(|window| window == b"\r\n\r\n") { + let bytes = stream.read(&mut buffer).expect("read Docker request"); + assert!(bytes > 0, "Docker request ended before its headers"); + request.extend_from_slice(&buffer[..bytes]); + } + String::from_utf8(request).expect("Docker request is UTF-8") +} + +fn write_response(stream: &mut UnixStream, status: &str, content_type: &str, body: &str) { + let response = format!( + "HTTP/1.1 {status}\r\nContent-Type: {content_type}\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ); + stream + .write_all(response.as_bytes()) + .expect("write fake Docker response"); +} + +struct ReleaseGate { + released: Mutex, + ready: Condvar, +} + +impl ReleaseGate { + fn new() -> Self { + Self { + released: Mutex::new(false), + ready: Condvar::new(), + } + } + + fn wait(&self) { + let mut released = self.released.lock().expect("lock release gate"); + while !*released { + released = self.ready.wait(released).expect("wait for release"); + } + } + + fn release(&self) { + *self.released.lock().expect("lock release gate") = true; + self.ready.notify_all(); + } +} + +struct FakeDocker { + credential_release: Arc, + removed: Arc, + stop: Arc, + daemon: JoinHandle<()>, +} + +impl FakeDocker { + fn spawn( + socket_path: &Path, + credential_started: SyncSender<()>, + second_recovery_finished: SyncSender<()>, + container_removed: SyncSender<()>, + ) -> Self { + let listener = UnixListener::bind(socket_path).expect("bind fake Docker socket"); + listener + .set_nonblocking(true) + .expect("make fake Docker socket nonblocking"); + + let credential_release = Arc::new(ReleaseGate::new()); + let removed = Arc::new(AtomicBool::new(false)); + let stop = Arc::new(AtomicBool::new(false)); + let daemon_release = Arc::clone(&credential_release); + let daemon_removed = Arc::clone(&removed); + let daemon_stop = Arc::clone(&stop); + let daemon = thread::spawn(move || { + let inspect_count = Arc::new(AtomicUsize::new(0)); + let list_count = Arc::new(AtomicUsize::new(0)); + let mut handlers = Vec::new(); + + while !daemon_stop.load(Ordering::SeqCst) { + match listener.accept() { + Ok((mut stream, _)) => { + stream + .set_nonblocking(false) + .expect("make Docker connection blocking"); + let request = read_request(&mut stream); + let inspect_count = Arc::clone(&inspect_count); + let list_count = Arc::clone(&list_count); + let credential_release = Arc::clone(&daemon_release); + let removed = Arc::clone(&daemon_removed); + let credential_started = credential_started.clone(); + let second_recovery_finished = second_recovery_finished.clone(); + let container_removed = container_removed.clone(); + + handlers.push(thread::spawn(move || { + if request.contains("/_ping ") { + write_response(&mut stream, "200 OK", "text/plain", "OK"); + } else if request.contains("/containers/json?") { + let list = list_count.fetch_add(1, Ordering::SeqCst); + write_response(&mut stream, "200 OK", "application/json", "[]"); + if list == 1 { + let _ = second_recovery_finished.send(()); + } + } else if request.contains("GET /containers/existing-container/json ") { + let inspect = inspect_count.fetch_add(1, Ordering::SeqCst); + if inspect == 1 { + let _ = credential_started.send(()); + credential_release.wait(); + if removed.load(Ordering::SeqCst) { + write_response( + &mut stream, + "404 Not Found", + "application/json", + r#"{"message":"No such container"}"#, + ); + } else { + write_container_inspect(&mut stream, true); + } + } else { + write_container_inspect(&mut stream, inspect == 0); + } + } else if request.contains("POST /containers/existing-container/stop") { + write_response(&mut stream, "204 No Content", "text/plain", ""); + } else if request.contains("DELETE /containers/existing-container?") { + removed.store(true, Ordering::SeqCst); + write_response(&mut stream, "204 No Content", "text/plain", ""); + let _ = container_removed.send(()); + } else { + panic!("unexpected Docker request: {request}"); + } + })); + } + Err(error) if error.kind() == ErrorKind::WouldBlock => { + thread::sleep(Duration::from_millis(5)); + } + Err(error) => panic!("accept fake Docker connection: {error}"), + } + } + + for handler in handlers { + handler.join().expect("fake Docker request handler"); + } + }); + + Self { + credential_release, + removed, + stop, + daemon, + } + } + + fn release_credentials(&self) { + self.credential_release.release(); + } + + fn finish(self) { + assert!( + self.removed.load(Ordering::SeqCst), + "serialized remove did not remove the container" + ); + self.stop.store(true, Ordering::SeqCst); + self.daemon.join().expect("fake Docker daemon"); + } +} + +fn write_container_inspect(stream: &mut UnixStream, running: bool) { + let body = json!({ + "Id": "existing-container", + "Config": { + "Env": [ + "POSTGRES_USER=stored-user", + "POSTGRES_PASSWORD=stored-password", + "POSTGRES_DB=stored-database" + ] + }, + "State": { "Running": running } + }); + write_response(stream, "200 OK", "application/json", &body.to_string()); +} + +fn command(project: &Path, socket_path: &Path) -> Command { + let mut command = Command::new(clickhousectl_binary()); + command + .env_clear() + .env("DO_NOT_TRACK", "1") + .env("HOME", project) + .env("DOCKER_HOST", format!("unix://{}", socket_path.display())) + .current_dir(project) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + command +} + +fn wait_for_output(mut child: Child) -> Output { + let deadline = Instant::now() + Duration::from_secs(5); + loop { + if child.try_wait().expect("poll clickhousectl").is_some() { + return child + .wait_with_output() + .expect("collect clickhousectl output"); + } + if Instant::now() >= deadline { + child.kill().expect("kill timed out clickhousectl"); + let output = child.wait_with_output().expect("collect timed out output"); + panic!( + "clickhousectl timed out\nstderr: {}\nstdout: {}", + String::from_utf8_lossy(&output.stderr), + String::from_utf8_lossy(&output.stdout) + ); + } + thread::sleep(Duration::from_millis(10)); + } +} + +fn write_postgres_metadata(project: &Path) { + let servers = project.join(".clickhouse/servers"); + std::fs::create_dir_all(servers.join("default-pg18/data")) + .expect("create Postgres data directory"); + std::fs::write( + servers.join("default-pg18.json"), + serde_json::to_vec_pretty(&json!({ + "name": "default-pg18", + "pid": 0, + "version": "postgres:18", + "http_port": 0, + "tcp_port": 5432, + "started_at": "1700000000", + "cwd": project.canonicalize().unwrap(), + "engine": "postgres", + "container_id": "existing-container" + })) + .unwrap(), + ) + .expect("write Postgres metadata"); +} + +fn receive_with_timeout(receiver: &Receiver<()>, operation: &str) { + receiver + .recv_timeout(Duration::from_secs(5)) + .unwrap_or_else(|error| panic!("wait for {operation}: {error}")); +} + +#[test] +fn postgres_dotenv_holds_lock_through_container_credential_read() { + let project = tempfile::tempdir().expect("create project"); + write_postgres_metadata(project.path()); + let original_dotenv = + "APP_SETTING=keep\nPOSTGRES_USER=old-user\nPOSTGRES_PASSWORD=old-password\n"; + std::fs::write(project.path().join(".env"), original_dotenv).expect("write original .env"); + + let socket_path = project.path().join("docker.sock"); + let (credential_started_tx, credential_started_rx) = mpsc::sync_channel(1); + let (remove_recovery_tx, remove_recovery_rx) = mpsc::sync_channel(1); + let (container_removed_tx, container_removed_rx) = mpsc::sync_channel(1); + let docker = FakeDocker::spawn( + &socket_path, + credential_started_tx, + remove_recovery_tx, + container_removed_tx, + ); + + let mut dotenv_command = command(project.path(), &socket_path); + dotenv_command.args([ + "local", + "postgres", + "dotenv", + "--name", + "default", + "--version", + "18", + ]); + let dotenv = dotenv_command.spawn().expect("run postgres dotenv"); + receive_with_timeout(&credential_started_rx, "paused credential read"); + + let mut remove_command = command(project.path(), &socket_path); + remove_command.args(["local", "postgres", "remove", "default", "--version", "18"]); + let remove = remove_command.spawn().expect("run postgres remove"); + receive_with_timeout(&remove_recovery_rx, "remove recovery"); + + let removed_during_credential_read = container_removed_rx + .recv_timeout(Duration::from_millis(500)) + .is_ok(); + docker.release_credentials(); + + let dotenv_output = wait_for_output(dotenv); + let remove_output = wait_for_output(remove); + docker.finish(); + + assert!( + !removed_during_credential_read, + "remove deleted the container while dotenv was reading credentials" + ); + assert!( + dotenv_output.status.success(), + "dotenv stderr: {}", + String::from_utf8_lossy(&dotenv_output.stderr) + ); + assert!( + remove_output.status.success(), + "remove stderr: {}", + String::from_utf8_lossy(&remove_output.stderr) + ); + + let dotenv = std::fs::read_to_string(project.path().join(".env")).expect("read .env"); + assert_eq!( + dotenv, + "APP_SETTING=keep\nPOSTGRES_USER=stored-user\nPOSTGRES_PASSWORD=stored-password\nPOSTGRES_HOST=127.0.0.1\nPOSTGRES_PORT=5432\nPOSTGRES_DATABASE=stored-database\n" + ); + assert!(!dotenv.contains("POSTGRES_PASSWORD=\"\"")); +} From 7ddc1a6684489b394d8fdcb0433168cdb7bbcda2 Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 17:59:16 +0100 Subject: [PATCH 14/16] Validate ClickHouse process identity --- crates/clickhousectl/src/local/discovery.rs | 53 +++++- crates/clickhousectl/src/local/server.rs | 192 +++++++++++++++++--- 2 files changed, 220 insertions(+), 25 deletions(-) diff --git a/crates/clickhousectl/src/local/discovery.rs b/crates/clickhousectl/src/local/discovery.rs index 7361187e..deeefadd 100644 --- a/crates/clickhousectl/src/local/discovery.rs +++ b/crates/clickhousectl/src/local/discovery.rs @@ -4,7 +4,7 @@ //! and command-line arguments to recover server metadata (project root, name, //! ports, version). Used for orphaned server recovery and global server listing. -use std::{collections::HashMap, process::Command}; +use std::{collections::HashMap, path::Path, process::Command}; /// A ClickHouse process discovered via OS-level process inspection. #[derive(Debug, Clone)] @@ -17,6 +17,14 @@ pub struct DiscoveredProcess { pub version: Option, } +fn process_command(standard_path: &'static str, fallback: &'static str) -> Command { + Command::new(if Path::new(standard_path).is_file() { + standard_path + } else { + fallback + }) +} + /// Find all running ClickHouse processes started by the CLI and parse their metadata. /// /// Only returns processes whose cwd matches the `.clickhouse/servers//data/` pattern, @@ -39,9 +47,46 @@ pub fn discover_clickhouse_processes() -> Vec { discovered } +/// Confirm that a PID is a ClickHouse process in the managed data directory +/// for the recorded project and server name. +pub fn is_managed_clickhouse_process(pid: u32, project_root: &str, server_name: &str) -> bool { + if pid == 0 { + return false; + } + let is_clickhouse = find_clickhouse_pids().contains(&pid) + || get_process_cmdline(pid).is_some_and(|cmdline| { + cmdline.split_whitespace().take(2).any(|arg| { + Path::new(arg) + .file_name() + .is_some_and(|name| name == "clickhouse") + }) + }); + if !is_clickhouse { + return false; + } + + let pids = [pid]; + let Some(cwd) = get_process_cwds(&pids).remove(&pid) else { + return false; + }; + let expected_cwd = Path::new(project_root) + .join(".clickhouse") + .join("servers") + .join(server_name) + .join("data"); + + match (Path::new(&cwd).canonicalize(), expected_cwd.canonicalize()) { + (Ok(actual), Ok(expected)) => actual == expected, + _ => false, + } +} + /// Find PIDs of running `clickhouse` processes. fn find_clickhouse_pids() -> Vec { - let output = Command::new("pgrep").arg("-x").arg("clickhouse").output(); + let output = process_command("/usr/bin/pgrep", "pgrep") + .arg("-x") + .arg("clickhouse") + .output(); match output { Ok(out) if out.status.success() => String::from_utf8_lossy(&out.stdout) @@ -88,7 +133,7 @@ fn get_process_cwds(pids: &[u32]) -> HashMap { // -a is required to AND the conditions; without it macOS lsof OR's // -d and -p, returning the cwd of every process on the system. - let output = Command::new("lsof") + let output = process_command("/usr/sbin/lsof", "lsof") .args(["-a", "-d", "cwd", "-Fn", "-p", &pid_list]) .output(); @@ -135,7 +180,7 @@ fn get_process_cwds(pids: &[u32]) -> HashMap { /// Get the command-line string of a process. fn get_process_cmdline(pid: u32) -> Option { - let output = Command::new("ps") + let output = process_command("/bin/ps", "ps") .args(["-o", "args=", "-p", &pid.to_string()]) .output() .ok()?; diff --git a/crates/clickhousectl/src/local/server.rs b/crates/clickhousectl/src/local/server.rs index 707b4109..40f7d4d2 100644 --- a/crates/clickhousectl/src/local/server.rs +++ b/crates/clickhousectl/src/local/server.rs @@ -416,7 +416,7 @@ impl ServerLock { .ok_or_else(|| Error::ServerNotRunning(self.name.clone()))?; match info.engine { Engine::Clickhouse => { - kill_process(info.pid)?; + kill_clickhouse_process(&info)?; self.mark_stopped(info.pid)?; } Engine::Postgres => { @@ -454,7 +454,9 @@ pub fn mark_server_stopped(name: &str, pid: u32) -> Result<()> { /// Engine-aware liveness check. fn is_alive(info: &ServerInfo) -> bool { match info.engine { - Engine::Clickhouse => is_process_alive(info.pid), + Engine::Clickhouse => { + discovery::is_managed_clickhouse_process(info.pid, &info.cwd, &info.name) + } Engine::Postgres => match info.container_id.as_deref() { Some(id) => docker::is_container_running_blocking(id), None => false, @@ -658,23 +660,34 @@ fn send_signal(pid: u32, signal: i32) -> Result<()> { } } +fn kill_clickhouse_process(info: &ServerInfo) -> Result<()> { + kill_process_while(info.pid, &info.name, || is_alive(info)) +} + /// Attempt to terminate a process: SIGTERM, wait, SIGKILL if needed, then verify exit. -fn kill_process(pid: u32) -> Result<()> { +fn kill_process_while( + pid: u32, + identity: &str, + is_target_process: impl Fn() -> bool, +) -> Result<()> { + if !is_target_process() { + return Err(Error::ServerNotRunning(identity.to_string())); + } send_signal(pid, libc::SIGTERM)?; // Wait briefly for graceful shutdown std::thread::sleep(std::time::Duration::from_millis(500)); - if is_process_alive(pid) { + if is_target_process() { std::thread::sleep(std::time::Duration::from_secs(2)); - if is_process_alive(pid) { + if is_target_process() { send_signal(pid, libc::SIGKILL)?; // Give the kernel a moment to reap the process std::thread::sleep(std::time::Duration::from_millis(100)); } } - if is_process_alive(pid) { + if is_target_process() { return Err(Error::Exec(format!( "Process {} did not exit after SIGKILL", pid @@ -684,6 +697,10 @@ fn kill_process(pid: u32) -> Result<()> { Ok(()) } +fn kill_process(pid: u32) -> Result<()> { + kill_process_while(pid, &format!("PID {pid}"), || is_process_alive(pid)) +} + /// Stop a running server by name. /// /// * ClickHouse: SIGTERM (then SIGKILL on timeout); metadata is retained with @@ -1118,6 +1135,7 @@ fn pause_during_stale_normalization_for_test() {} #[cfg(test)] mod tests { use super::*; + use std::os::unix::process::CommandExt; use std::process::{Child, Command, Stdio}; use std::time::Instant; @@ -1130,6 +1148,8 @@ mod tests { const STRICT_LIST_HELPER: &str = "local::server::tests::strict_list_reports_corrupt_metadata"; const POSTGRES_RECOVERY_HELPER: &str = "local::server::tests::postgres_recovery_subprocess"; const CLICKHOUSE_RECOVERY_HELPER: &str = "local::server::tests::clickhouse_recovery_subprocess"; + const MANAGED_PROCESS_HELPER: &str = + "local::server::tests::managed_clickhouse_process_subprocess"; #[test] fn lock_directory_failure_preserves_operation_path_and_source() { @@ -1248,6 +1268,42 @@ mod tests { assert!(status.success(), "metadata helper failed with {status}"); } + fn spawn_managed_clickhouse(project: &Path, name: &str) -> Child { + let data_dir = project.join(".clickhouse/servers").join(name).join("data"); + std::fs::create_dir_all(&data_dir).unwrap(); + let mut child = Command::new(std::env::current_exe().unwrap()) + .arg0("clickhouse") + .args(["--exact", MANAGED_PROCESS_HELPER, "--nocapture"]) + .env("CHCTL_TEST_MANAGED_PROCESS", "1") + .current_dir(data_dir) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .unwrap(); + + let deadline = Instant::now() + Duration::from_secs(5); + while !discovery::is_managed_clickhouse_process( + child.id(), + &project.display().to_string(), + name, + ) { + assert_eq!(child.try_wait().unwrap(), None, "test process exited"); + assert!( + Instant::now() < deadline, + "timed out waiting for managed ClickHouse test process" + ); + std::thread::sleep(Duration::from_millis(10)); + } + child + } + + #[test] + fn managed_clickhouse_process_subprocess() { + if std::env::var_os("CHCTL_TEST_MANAGED_PROCESS").is_some() { + std::thread::sleep(Duration::from_secs(30)); + } + } + fn enter_helper_project() -> Option { let project = std::env::var_os("CHCTL_TEST_METADATA_PROJECT")?; let project = PathBuf::from(project); @@ -1305,8 +1361,9 @@ mod tests { "lifecycle" => { let lock = ServerLock::acquire("default").unwrap(); wait_for_test_release("CHCTL_TEST_LIFECYCLE_READY", "CHCTL_TEST_LIFECYCLE_RELEASE"); - lock.save_info(&test_info("default", pid, "26.8.1.1")) - .unwrap(); + let mut info = test_info("default", pid, "26.8.1.1"); + info.cwd = std::env::current_dir().unwrap().display().to_string(); + lock.save_info(&info).unwrap(); } "client" => { std::fs::write( @@ -1349,15 +1406,28 @@ mod tests { #[test] fn clickhouse_recovery_subprocess() { - if enter_helper_project().is_none() { + let Some(project) = enter_helper_project() else { return; - } + }; let name = std::env::var("CHCTL_TEST_METADATA_NAME").unwrap(); - save_recovered_server_info( - &test_info(&name, std::process::id(), "clickhouse-recovered"), - true, - ) - .unwrap(); + match std::env::var("CHCTL_TEST_METADATA_OPERATION").as_deref() { + Ok("stop") => kill_server(&name).unwrap(), + Ok("reject-stop") => assert!(matches!( + kill_server(&name), + Err(Error::ServerNotRunning(server)) if server == name + )), + _ => { + let pid = std::env::var("CHCTL_TEST_METADATA_PID") + .ok() + .and_then(|pid| pid.parse().ok()) + .unwrap_or_else(std::process::id); + let version = std::env::var("CHCTL_TEST_METADATA_VERSION") + .unwrap_or_else(|_| "clickhouse-recovered".to_string()); + let mut info = test_info(&name, pid, &version); + info.cwd = project.display().to_string(); + save_recovered_server_info(&info, true).unwrap(); + } + } } #[test] @@ -1396,6 +1466,85 @@ mod tests { assert!(live_clickhouse.container_id.is_none()); } + #[test] + fn recovery_replaces_reused_pid_and_stop_never_signals_unrelated_process() { + let project = tempfile::tempdir().unwrap(); + let mut unrelated = Command::new("/bin/sleep") + .arg("30") + .current_dir(project.path()) + .spawn() + .unwrap(); + let mut discovered = spawn_managed_clickhouse(project.path(), "default"); + let mut stale = test_info("default", unrelated.id(), "clickhouse-stale"); + stale.cwd = project.path().display().to_string(); + write_initial_metadata(project.path(), &stale); + + let rejected_stop = helper(CLICKHOUSE_RECOVERY_HELPER, project.path()) + .env("CHCTL_TEST_METADATA_NAME", "default") + .env("CHCTL_TEST_METADATA_OPERATION", "reject-stop") + .status() + .unwrap(); + assert!( + rejected_stop.success(), + "stop helper failed with {rejected_stop}" + ); + assert!(unrelated.try_wait().unwrap().is_none()); + + let recovered = helper(CLICKHOUSE_RECOVERY_HELPER, project.path()) + .env("CHCTL_TEST_METADATA_NAME", "default") + .env("CHCTL_TEST_METADATA_PID", discovered.id().to_string()) + .env("CHCTL_TEST_METADATA_VERSION", "clickhouse-recovered") + .status() + .unwrap(); + assert!( + recovered.success(), + "recovery helper failed with {recovered}" + ); + + let live: ServerInfo = + serde_json::from_slice(&std::fs::read(metadata_path(project.path())).unwrap()).unwrap(); + assert_eq!(live.pid, discovered.id()); + assert_eq!(live.version, "clickhouse-recovered"); + + let stopped = helper(CLICKHOUSE_RECOVERY_HELPER, project.path()) + .env("CHCTL_TEST_METADATA_NAME", "default") + .env("CHCTL_TEST_METADATA_OPERATION", "stop") + .status() + .unwrap(); + assert!(stopped.success(), "stop helper failed with {stopped}"); + assert!(!discovered.wait().unwrap().success()); + assert!(unrelated.try_wait().unwrap().is_none()); + unrelated.kill().unwrap(); + unrelated.wait().unwrap(); + } + + #[test] + fn recovery_preserves_existing_managed_clickhouse_identity() { + let project = tempfile::tempdir().unwrap(); + let mut managed = spawn_managed_clickhouse(project.path(), "default"); + let mut existing = test_info("default", managed.id(), "clickhouse-existing"); + existing.cwd = project.path().display().to_string(); + write_initial_metadata(project.path(), &existing); + + let recovered = helper(CLICKHOUSE_RECOVERY_HELPER, project.path()) + .env("CHCTL_TEST_METADATA_NAME", "default") + .env("CHCTL_TEST_METADATA_PID", managed.id().to_string()) + .env("CHCTL_TEST_METADATA_VERSION", "clickhouse-recovered") + .status() + .unwrap(); + assert!( + recovered.success(), + "recovery helper failed with {recovered}" + ); + + let live: ServerInfo = + serde_json::from_slice(&std::fs::read(metadata_path(project.path())).unwrap()).unwrap(); + assert_eq!(live.pid, managed.id()); + assert_eq!(live.version, "clickhouse-existing"); + managed.kill().unwrap(); + managed.wait().unwrap(); + } + #[test] fn postgres_recovery_skips_failing_data_path_when_metadata_exists() { let project = tempfile::tempdir().unwrap(); @@ -1476,10 +1625,12 @@ mod tests { let project = tempfile::tempdir().unwrap(); let servers = project.path().join(".clickhouse/servers"); std::fs::create_dir_all(&servers).unwrap(); + let mut process = spawn_managed_clickhouse(project.path(), "healthy"); + let mut healthy = test_info("healthy", process.id(), "26.8.1.1"); + healthy.cwd = project.path().display().to_string(); std::fs::write( servers.join("healthy.json"), - serde_json::to_vec_pretty(&test_info("healthy", std::process::id(), "26.8.1.1")) - .unwrap(), + serde_json::to_vec_pretty(&healthy).unwrap(), ) .unwrap(); std::fs::write(servers.join("corrupt.json"), b"not json").unwrap(); @@ -1491,6 +1642,8 @@ mod tests { status.success(), "advisory count helper failed with {status}" ); + process.kill().unwrap(); + process.wait().unwrap(); } #[test] @@ -1606,10 +1759,7 @@ mod tests { fn concurrent_lifecycle_client_and_stop_observe_complete_metadata() { let project = tempfile::tempdir().unwrap(); write_initial_metadata(project.path(), &test_info("default", 0, "")); - let mut process = Command::new("sh") - .args(["-c", "exec sleep 30"]) - .spawn() - .unwrap(); + let mut process = spawn_managed_clickhouse(project.path(), "default"); let pid = process.id(); let reaper = std::thread::spawn(move || process.wait().unwrap()); let ready = project.path().join("lifecycle-ready"); From f17063456db711e41a33784266f732554faeafe6 Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 19:14:52 +0100 Subject: [PATCH 15/16] Preserve lock errors during startup cleanup --- crates/clickhousectl/src/local/server.rs | 74 +++++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-) diff --git a/crates/clickhousectl/src/local/server.rs b/crates/clickhousectl/src/local/server.rs index 40f7d4d2..a2fbda88 100644 --- a/crates/clickhousectl/src/local/server.rs +++ b/crates/clickhousectl/src/local/server.rs @@ -871,7 +871,8 @@ pub async fn wait_for_server_ready( Err(error) if matches!( &error, - Error::ServerMetadataRead { .. } + Error::ServerLock { .. } + | Error::ServerMetadataRead { .. } | Error::ServerMetadataPermission { .. } | Error::ServerMetadataParse { .. } | Error::ServerMetadataWrite { .. } @@ -1150,6 +1151,8 @@ mod tests { const CLICKHOUSE_RECOVERY_HELPER: &str = "local::server::tests::clickhouse_recovery_subprocess"; const MANAGED_PROCESS_HELPER: &str = "local::server::tests::managed_clickhouse_process_subprocess"; + const READINESS_LOCK_HELPER: &str = + "local::server::tests::readiness_timeout_lock_failure_subprocess"; #[test] fn lock_directory_failure_preserves_operation_path_and_source() { @@ -1311,6 +1314,43 @@ mod tests { Some(project) } + #[tokio::test] + async fn readiness_timeout_lock_failure_subprocess() { + if enter_helper_project().is_none() { + return; + } + let mut child = Command::new("sh") + .args(["-c", "exec sleep 10"]) + .spawn() + .unwrap(); + let pid = child.id(); + + let error = wait_for_server_ready( + &mut child, + "readiness-lock-test", + 0, + 0, + Path::new("server.log"), + Duration::ZERO, + ) + .await + .unwrap_err(); + + assert!(matches!(&error, Error::ServerLock { .. })); + assert!(!is_process_alive(pid)); + let structured = crate::local::output::LocalErrorOutput::from_error(&error); + std::fs::write( + std::env::var_os("CHCTL_TEST_STRUCTURED_ERROR").unwrap(), + serde_json::to_vec(&structured).unwrap(), + ) + .unwrap(); + std::fs::write( + std::env::var_os("CHCTL_TEST_HUMAN_ERROR").unwrap(), + format!("Error: {error}\n"), + ) + .unwrap(); + } + #[test] fn metadata_write_subprocess() { if enter_helper_project().is_none() { @@ -1888,6 +1928,38 @@ mod tests { assert!(!is_process_alive(pid)); } + #[test] + fn readiness_timeout_preserves_lock_failure_output() { + let project = tempfile::tempdir().unwrap(); + let lock_directory = project.path().join(".clickhouse/servers/.locks"); + std::fs::create_dir_all(lock_directory.parent().unwrap()).unwrap(); + std::fs::write(&lock_directory, b"not a directory").unwrap(); + let structured_path = project.path().join("structured-error.json"); + let human_path = project.path().join("human-error.txt"); + + let status = helper(READINESS_LOCK_HELPER, project.path()) + .env("CHCTL_TEST_STRUCTURED_ERROR", &structured_path) + .env("CHCTL_TEST_HUMAN_ERROR", &human_path) + .status() + .unwrap(); + assert!(status.success(), "readiness helper failed with {status}"); + + let body: serde_json::Value = + serde_json::from_slice(&std::fs::read(structured_path).unwrap()).unwrap(); + assert_eq!(body["error"]["code"], "server_lock"); + assert_eq!(body["error"]["command"], "clickhousectl local server list"); + let message = body["error"]["message"].as_str().unwrap(); + assert!(message.contains("create server lifecycle lock directory")); + assert!(message.contains(&lock_directory.display().to_string())); + assert!(message.contains("Check write access to the parent directory and retry.")); + + let human = std::fs::read_to_string(human_path).unwrap(); + assert!(human.starts_with("Error: Could not create server lifecycle lock directory")); + assert!(human.contains(&lock_directory.display().to_string())); + assert!(human.contains("Check write access to the parent directory and retry.")); + assert!(!human.contains("did not become ready")); + } + #[test] fn explicit_ports_must_be_available() { let listener = std::net::TcpListener::bind(("127.0.0.1", 0)).unwrap(); From 92404ad67b332cad988feb665f969f14caa63958 Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 25 Aug 2026 19:36:39 +0100 Subject: [PATCH 16/16] Preserve metadata on process inspection errors --- crates/clickhousectl/src/error.rs | 11 + crates/clickhousectl/src/local/discovery.rs | 226 +++++++++++++++++--- crates/clickhousectl/src/local/output.rs | 16 ++ crates/clickhousectl/src/local/server.rs | 173 +++++++++++++-- 4 files changed, 373 insertions(+), 53 deletions(-) diff --git a/crates/clickhousectl/src/error.rs b/crates/clickhousectl/src/error.rs index e029ad76..91c4f9c5 100644 --- a/crates/clickhousectl/src/error.rs +++ b/crates/clickhousectl/src/error.rs @@ -177,6 +177,17 @@ pub enum Error { source: std::io::Error, }, + #[error( + "Could not verify process identity for server '{name}' (PID {pid}) while attempting to {operation}: {source}. Metadata was preserved. Check that process inspection tools are available and that you have permission to inspect the process, then retry." + )] + ServerProcessInspection { + name: String, + pid: u32, + operation: &'static str, + #[source] + source: std::io::Error, + }, + #[error( "No server name was provided and multiple non-default ClickHouse servers exist. Pass a name or run `clickhousectl local server stop-all`; use `clickhousectl local server list` to see available servers." )] diff --git a/crates/clickhousectl/src/local/discovery.rs b/crates/clickhousectl/src/local/discovery.rs index deeefadd..1d593d88 100644 --- a/crates/clickhousectl/src/local/discovery.rs +++ b/crates/clickhousectl/src/local/discovery.rs @@ -4,7 +4,7 @@ //! and command-line arguments to recover server metadata (project root, name, //! ports, version). Used for orphaned server recovery and global server listing. -use std::{collections::HashMap, path::Path, process::Command}; +use std::{collections::HashMap, io, path::Path, process::Command}; /// A ClickHouse process discovered via OS-level process inspection. #[derive(Debug, Clone)] @@ -17,6 +17,24 @@ pub struct DiscoveredProcess { pub version: Option, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ManagedProcessIdentity { + Managed, + NotManaged, +} + +#[derive(Debug)] +pub struct ProcessInspectionError { + pub operation: &'static str, + pub source: io::Error, +} + +impl ProcessInspectionError { + fn new(operation: &'static str, source: io::Error) -> Self { + Self { operation, source } + } +} + fn process_command(standard_path: &'static str, fallback: &'static str) -> Command { Command::new(if Path::new(standard_path).is_file() { standard_path @@ -30,7 +48,7 @@ fn process_command(standard_path: &'static str, fallback: &'static str) -> Comma /// Only returns processes whose cwd matches the `.clickhouse/servers//data/` pattern, /// meaning they were started by this CLI. Other ClickHouse processes are ignored. pub fn discover_clickhouse_processes() -> Vec { - let pids = find_clickhouse_pids(); + let pids = find_clickhouse_pids().unwrap_or_default(); let cwds = get_process_cwds(&pids); let mut discovered = Vec::new(); @@ -47,27 +65,74 @@ pub fn discover_clickhouse_processes() -> Vec { discovered } -/// Confirm that a PID is a ClickHouse process in the managed data directory -/// for the recorded project and server name. -pub fn is_managed_clickhouse_process(pid: u32, project_root: &str, server_name: &str) -> bool { - if pid == 0 { - return false; - } - let is_clickhouse = find_clickhouse_pids().contains(&pid) - || get_process_cmdline(pid).is_some_and(|cmdline| { - cmdline.split_whitespace().take(2).any(|arg| { - Path::new(arg) - .file_name() - .is_some_and(|name| name == "clickhouse") - }) - }); +/// Determine whether a PID is a ClickHouse process in the managed data +/// directory for the recorded project and server name. +pub fn inspect_managed_clickhouse_process( + pid: u32, + project_root: &str, + server_name: &str, +) -> Result { + if pid == 0 || i32::try_from(pid).is_err() { + return Ok(ManagedProcessIdentity::NotManaged); + } + if !process_exists(pid).map_err(|source| { + ProcessInspectionError::new("check whether the recorded process exists", source) + })? { + return Ok(ManagedProcessIdentity::NotManaged); + } + + inject_inspection_failure("command", "inspect the recorded process executable")?; + let is_clickhouse = match (find_clickhouse_pids(), get_process_cmdline(pid)) { + (Ok(pids), cmdline) => { + pids.contains(&pid) || cmdline.is_ok_and(|cmdline| cmdline_is_clickhouse(&cmdline)) + } + (Err(_), Ok(cmdline)) => cmdline_is_clickhouse(&cmdline), + (Err(pgrep_error), Err(ps_error)) => { + if !process_exists(pid).map_err(|source| { + ProcessInspectionError::new("recheck whether the recorded process exists", source) + })? { + return Ok(ManagedProcessIdentity::NotManaged); + } + return Err(ProcessInspectionError::new( + "inspect the recorded process executable", + io::Error::other(format!( + "pgrep failed: {pgrep_error}; ps failed: {ps_error}" + )), + )); + } + }; if !is_clickhouse { - return false; + return Ok(ManagedProcessIdentity::NotManaged); } - let pids = [pid]; - let Some(cwd) = get_process_cwds(&pids).remove(&pid) else { - return false; + inject_inspection_failure("cwd", "read the recorded process working directory")?; + let cwd = match get_process_cwd(pid) { + Ok(Some(cwd)) => cwd, + Ok(None) => { + if !process_exists(pid).map_err(|source| { + ProcessInspectionError::new("recheck whether the recorded process exists", source) + })? { + return Ok(ManagedProcessIdentity::NotManaged); + } + return Err(ProcessInspectionError::new( + "read the recorded process working directory", + io::Error::other("process inspection returned no working directory"), + )); + } + Err(source) => { + if !process_exists(pid).map_err(|recheck_source| { + ProcessInspectionError::new( + "recheck whether the recorded process exists", + recheck_source, + ) + })? { + return Ok(ManagedProcessIdentity::NotManaged); + } + return Err(ProcessInspectionError::new( + "read the recorded process working directory", + source, + )); + } }; let expected_cwd = Path::new(project_root) .join(".clickhouse") @@ -75,28 +140,95 @@ pub fn is_managed_clickhouse_process(pid: u32, project_root: &str, server_name: .join(server_name) .join("data"); - match (Path::new(&cwd).canonicalize(), expected_cwd.canonicalize()) { - (Ok(actual), Ok(expected)) => actual == expected, - _ => false, - } + inject_inspection_failure("canonicalize", "resolve the managed process paths")?; + let actual = Path::new(&cwd).canonicalize().map_err(|source| { + ProcessInspectionError::new("resolve the recorded process working directory", source) + })?; + let expected = expected_cwd.canonicalize().map_err(|source| { + ProcessInspectionError::new("resolve the expected managed data directory", source) + })?; + + Ok(if actual == expected { + ManagedProcessIdentity::Managed + } else { + ManagedProcessIdentity::NotManaged + }) } /// Find PIDs of running `clickhouse` processes. -fn find_clickhouse_pids() -> Vec { +fn find_clickhouse_pids() -> io::Result> { let output = process_command("/usr/bin/pgrep", "pgrep") .arg("-x") .arg("clickhouse") - .output(); + .output()?; - match output { - Ok(out) if out.status.success() => String::from_utf8_lossy(&out.stdout) + match output.status.code() { + Some(0) => Ok(String::from_utf8_lossy(&output.stdout) .lines() .filter_map(|line| line.trim().parse::().ok()) - .collect(), - _ => Vec::new(), + .collect()), + Some(1) => Ok(Vec::new()), + _ => Err(command_error("pgrep", &output)), } } +fn cmdline_is_clickhouse(cmdline: &str) -> bool { + cmdline.split_whitespace().take(2).any(|arg| { + Path::new(arg) + .file_name() + .is_some_and(|name| name == "clickhouse") + }) +} + +fn process_exists(pid: u32) -> io::Result { + let pid = i32::try_from(pid).map_err(io::Error::other)?; + if unsafe { libc::kill(pid, 0) } == 0 { + return Ok(true); + } + + let source = io::Error::last_os_error(); + match source.raw_os_error() { + Some(libc::ESRCH) => Ok(false), + Some(libc::EPERM) => Ok(true), + _ => Err(source), + } +} + +fn command_error(command: &str, output: &std::process::Output) -> io::Error { + let stderr = String::from_utf8_lossy(&output.stderr); + io::Error::other(format!( + "{command} exited with {}{}", + output.status, + if stderr.trim().is_empty() { + String::new() + } else { + format!(": {}", stderr.trim()) + } + )) +} + +#[cfg(test)] +fn inject_inspection_failure( + failure: &str, + operation: &'static str, +) -> Result<(), ProcessInspectionError> { + if std::env::var("CHCTL_TEST_PROCESS_INSPECTION_FAILURE").as_deref() == Ok(failure) { + return Err(ProcessInspectionError::new( + operation, + io::Error::other(format!("injected {failure} inspection failure")), + )); + } + Ok(()) +} + +#[cfg(not(test))] +fn inject_inspection_failure( + _failure: &str, + _operation: &'static str, +) -> Result<(), ProcessInspectionError> { + Ok(()) +} + /// Inspect a single process to extract server metadata from its cwd and cmdline. fn inspect_process(pid: u32, cwd: &str) -> Option { let (project_root, server_name) = parse_server_cwd(cwd)?; @@ -146,6 +278,20 @@ fn get_process_cwds(pids: &[u32]) -> HashMap { parse_lsof_cwds(&String::from_utf8_lossy(&output.stdout)) } +#[cfg(target_os = "macos")] +fn get_process_cwd(pid: u32) -> io::Result> { + let pid_arg = pid.to_string(); + let output = process_command("/usr/sbin/lsof", "lsof") + .args(["-a", "-d", "cwd", "-Fn", "-p", &pid_arg]) + .output()?; + let cwd = parse_lsof_cwds(&String::from_utf8_lossy(&output.stdout)).remove(&pid); + if cwd.is_some() || output.status.success() { + Ok(cwd) + } else { + Err(command_error("lsof", &output)) + } +} + /// Parse `lsof -Fn` output into process working directories. /// /// `p` starts a process record and `n` names its cwd. Other fields @@ -178,17 +324,27 @@ fn get_process_cwds(pids: &[u32]) -> HashMap { .collect() } +#[cfg(target_os = "linux")] +fn get_process_cwd(pid: u32) -> io::Result> { + let path = std::fs::read_link(format!("/proc/{pid}/cwd"))?; + path.into_os_string().into_string().map(Some).map_err(|_| { + io::Error::new( + io::ErrorKind::InvalidData, + "process working directory is not valid UTF-8", + ) + }) +} + /// Get the command-line string of a process. -fn get_process_cmdline(pid: u32) -> Option { +fn get_process_cmdline(pid: u32) -> io::Result { let output = process_command("/bin/ps", "ps") .args(["-o", "args=", "-p", &pid.to_string()]) - .output() - .ok()?; + .output()?; if output.status.success() { - Some(String::from_utf8_lossy(&output.stdout).trim().to_string()) + Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) } else { - None + Err(command_error("ps", &output)) } } diff --git a/crates/clickhousectl/src/local/output.rs b/crates/clickhousectl/src/local/output.rs index c5de79d9..d24456e8 100644 --- a/crates/clickhousectl/src/local/output.rs +++ b/crates/clickhousectl/src/local/output.rs @@ -20,6 +20,7 @@ pub enum LocalErrorCode { ServerMetadataPermission, ServerMetadataInvalid, ServerMetadataWrite, + ServerProcessInspection, InvalidVersion, VersionUnavailable, PortInUse, @@ -90,6 +91,11 @@ impl LocalErrorOutput { error.to_string(), "clickhousectl local server list", ), + Error::ServerProcessInspection { .. } => ( + LocalErrorCode::ServerProcessInspection, + error.to_string(), + "clickhousectl local server list", + ), Error::InvalidVersion(_) => ( LocalErrorCode::InvalidVersion, error.to_string(), @@ -844,6 +850,16 @@ mod tests { LocalErrorCode::ServerMetadataWrite, "clickhousectl local server list", ), + ( + Error::ServerProcessInspection { + name: "default".into(), + pid: 12345, + operation: "read the recorded process working directory", + source: std::io::Error::other("inspection failed"), + }, + LocalErrorCode::ServerProcessInspection, + "clickhousectl local server list", + ), ( Error::InvalidVersion("invalid version".into()), LocalErrorCode::InvalidVersion, diff --git a/crates/clickhousectl/src/local/server.rs b/crates/clickhousectl/src/local/server.rs index a2fbda88..6e753e66 100644 --- a/crates/clickhousectl/src/local/server.rs +++ b/crates/clickhousectl/src/local/server.rs @@ -380,7 +380,10 @@ impl ServerLock { } pub fn load_running_info(&self) -> Result> { - Ok(self.load_info()?.filter(is_alive)) + let Some(info) = self.load_info()? else { + return Ok(None); + }; + Ok(is_alive(&info)?.then_some(info)) } pub fn is_running(&self) -> Result { @@ -451,15 +454,25 @@ pub fn mark_server_stopped(name: &str, pid: u32) -> Result<()> { ServerLock::acquire(name)?.mark_stopped(pid) } -/// Engine-aware liveness check. -fn is_alive(info: &ServerInfo) -> bool { +/// Engine-aware liveness check. ClickHouse inspection failures remain unknown +/// instead of being treated as proof that the recorded process is stale. +fn is_alive(info: &ServerInfo) -> Result { match info.engine { Engine::Clickhouse => { - discovery::is_managed_clickhouse_process(info.pid, &info.cwd, &info.name) + match discovery::inspect_managed_clickhouse_process(info.pid, &info.cwd, &info.name) { + Ok(discovery::ManagedProcessIdentity::Managed) => Ok(true), + Ok(discovery::ManagedProcessIdentity::NotManaged) => Ok(false), + Err(error) => Err(Error::ServerProcessInspection { + name: info.name.clone(), + pid: info.pid, + operation: error.operation, + source: error.source, + }), + } } Engine::Postgres => match info.container_id.as_deref() { - Some(id) => docker::is_container_running_blocking(id), - None => false, + Some(id) => Ok(docker::is_container_running_blocking(id)), + None => Ok(false), }, } } @@ -575,7 +588,7 @@ fn normalize_server_info(name: &str) -> Result> { let Some(mut info) = lock.load_info()? else { return Ok(None); }; - let running = is_alive(&info); + let running = is_alive(&info)?; if !running && info.engine == Engine::Clickhouse && info.pid != 0 { pause_during_stale_normalization_for_test(); set_stopped(&mut info); @@ -668,9 +681,9 @@ fn kill_clickhouse_process(info: &ServerInfo) -> Result<()> { fn kill_process_while( pid: u32, identity: &str, - is_target_process: impl Fn() -> bool, + is_target_process: impl Fn() -> Result, ) -> Result<()> { - if !is_target_process() { + if !is_target_process()? { return Err(Error::ServerNotRunning(identity.to_string())); } send_signal(pid, libc::SIGTERM)?; @@ -678,16 +691,16 @@ fn kill_process_while( // Wait briefly for graceful shutdown std::thread::sleep(std::time::Duration::from_millis(500)); - if is_target_process() { + if is_target_process()? { std::thread::sleep(std::time::Duration::from_secs(2)); - if is_target_process() { + if is_target_process()? { send_signal(pid, libc::SIGKILL)?; // Give the kernel a moment to reap the process std::thread::sleep(std::time::Duration::from_millis(100)); } } - if is_target_process() { + if is_target_process()? { return Err(Error::Exec(format!( "Process {} did not exit after SIGKILL", pid @@ -698,7 +711,7 @@ fn kill_process_while( } fn kill_process(pid: u32) -> Result<()> { - kill_process_while(pid, &format!("PID {pid}"), || is_process_alive(pid)) + kill_process_while(pid, &format!("PID {pid}"), || Ok(is_process_alive(pid))) } /// Stop a running server by name. @@ -1030,7 +1043,7 @@ pub fn recover_current_project_servers() -> Result<()> { pub fn save_recovered_server_info(info: &ServerInfo, replace_stale: bool) -> Result<()> { let lock = ServerLock::acquire(&info.name)?; if let Some(existing) = lock.load_info()? - && (!replace_stale || existing.engine != Engine::Clickhouse || is_alive(&existing)) + && (!replace_stale || existing.engine != Engine::Clickhouse || is_alive(&existing)?) { return Ok(()); } @@ -1151,6 +1164,8 @@ mod tests { const CLICKHOUSE_RECOVERY_HELPER: &str = "local::server::tests::clickhouse_recovery_subprocess"; const MANAGED_PROCESS_HELPER: &str = "local::server::tests::managed_clickhouse_process_subprocess"; + const INSPECTION_FAILURE_HELPER: &str = + "local::server::tests::process_inspection_failure_subprocess"; const READINESS_LOCK_HELPER: &str = "local::server::tests::readiness_timeout_lock_failure_subprocess"; @@ -1285,10 +1300,13 @@ mod tests { .unwrap(); let deadline = Instant::now() + Duration::from_secs(5); - while !discovery::is_managed_clickhouse_process( - child.id(), - &project.display().to_string(), - name, + while !matches!( + discovery::inspect_managed_clickhouse_process( + child.id(), + &project.display().to_string(), + name, + ), + Ok(discovery::ManagedProcessIdentity::Managed) ) { assert_eq!(child.try_wait().unwrap(), None, "test process exited"); assert!( @@ -1307,6 +1325,42 @@ mod tests { } } + #[test] + fn process_inspection_failure_subprocess() { + let Some(project) = enter_helper_project() else { + return; + }; + let name = "default"; + let pid: u32 = std::env::var("CHCTL_TEST_METADATA_PID") + .unwrap() + .parse() + .unwrap(); + let result = match std::env::var("CHCTL_TEST_METADATA_OPERATION") + .unwrap() + .as_str() + { + "normalize" => normalize_server_info(name).map(|_| ()), + "recover" => { + let mut replacement = test_info(name, std::process::id(), "replacement"); + replacement.cwd = project.display().to_string(); + save_recovered_server_info(&replacement, true) + } + "start" => is_server_running(name).map(|_| ()), + "remove" => ServerLock::acquire(name).and_then(|lock| lock.is_running().map(|_| ())), + "stop" => kill_server(name), + operation => panic!("unknown process inspection operation {operation}"), + }; + + assert!(matches!( + result, + Err(Error::ServerProcessInspection { + name: error_name, + pid: error_pid, + .. + }) if error_name == name && error_pid == pid + )); + } + fn enter_helper_project() -> Option { let project = std::env::var_os("CHCTL_TEST_METADATA_PROJECT")?; let project = PathBuf::from(project); @@ -1470,6 +1524,89 @@ mod tests { } } + #[test] + fn process_identity_distinguishes_managed_unrelated_and_dead_pids() { + let project = tempfile::tempdir().unwrap(); + let mut managed = spawn_managed_clickhouse(project.path(), "default"); + let mut unrelated = Command::new("/bin/sleep").arg("30").spawn().unwrap(); + let unrelated_pid = unrelated.id(); + + assert_eq!( + discovery::inspect_managed_clickhouse_process( + managed.id(), + &project.path().display().to_string(), + "default", + ) + .unwrap(), + discovery::ManagedProcessIdentity::Managed + ); + assert_eq!( + discovery::inspect_managed_clickhouse_process( + unrelated_pid, + &project.path().display().to_string(), + "default", + ) + .unwrap(), + discovery::ManagedProcessIdentity::NotManaged + ); + + unrelated.kill().unwrap(); + unrelated.wait().unwrap(); + assert_eq!( + discovery::inspect_managed_clickhouse_process( + unrelated_pid, + &project.path().display().to_string(), + "default", + ) + .unwrap(), + discovery::ManagedProcessIdentity::NotManaged + ); + + managed.kill().unwrap(); + managed.wait().unwrap(); + } + + #[test] + fn unknown_process_identity_preserves_metadata_and_blocks_lifecycle_actions() { + let project = tempfile::tempdir().unwrap(); + let mut managed = spawn_managed_clickhouse(project.path(), "default"); + let mut existing = test_info("default", managed.id(), "clickhouse-existing"); + existing.cwd = project.path().display().to_string(); + write_initial_metadata(project.path(), &existing); + + let cases = [ + ("command", "normalize"), + ("cwd", "normalize"), + ("canonicalize", "normalize"), + ("canonicalize", "recover"), + ("canonicalize", "start"), + ("canonicalize", "remove"), + ("canonicalize", "stop"), + ]; + for (failure, operation) in cases { + let status = helper(INSPECTION_FAILURE_HELPER, project.path()) + .env("CHCTL_TEST_METADATA_PID", managed.id().to_string()) + .env("CHCTL_TEST_METADATA_OPERATION", operation) + .env("CHCTL_TEST_PROCESS_INSPECTION_FAILURE", failure) + .status() + .unwrap(); + assert!( + status.success(), + "{operation} helper with {failure} failure failed with {status}" + ); + + let preserved: ServerInfo = + serde_json::from_slice(&std::fs::read(metadata_path(project.path())).unwrap()) + .unwrap(); + assert_eq!(preserved.pid, managed.id()); + assert_eq!(preserved.version, "clickhouse-existing"); + assert_eq!(managed.try_wait().unwrap(), None); + } + + managed.kill().unwrap(); + managed.wait().unwrap(); + } + #[test] fn clickhouse_recovery_preserves_postgres_collision_and_replaces_stale_clickhouse() { let project = tempfile::tempdir().unwrap();