diff --git a/crates/clickhousectl/src/local/docker.rs b/crates/clickhousectl/src/local/docker.rs index 65c7844bc..72af319c2 100644 --- a/crates/clickhousectl/src/local/docker.rs +++ b/crates/clickhousectl/src/local/docker.rs @@ -1179,9 +1179,8 @@ pub async fn start_existing(docker: &Docker, id: &str) -> Result<()> { /// a metadata file under `.clickhouse/servers/`, and write a `ServerInfo` for /// each so they show up in `local server list` and can be managed. /// -/// 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. +/// Safe to call multiple times in one CLI invocation. Docker being unreachable +/// is not an error: see [`discover_project_postgres_blocking`]. pub fn recover_project_postgres_blocking( project_cwd: &str, lock: &crate::local::server::MetadataLock, @@ -1190,36 +1189,41 @@ pub fn recover_project_postgres_blocking( Engine, ServerInfo, ensure_pg_data_dir, load_info_locked, pg_instance_key, save_server_info_locked, }; - let cwd_owned = project_cwd.to_string(); - block_on(async move { - let docker = match connect().await { - Ok(d) => d, - Err(_) => return Ok::<(), Error>(()), - }; - let containers = match list_project_postgres(&docker, &cwd_owned).await { - Ok(c) => c, - Err(_) => return Ok(()), + for c in discover_project_postgres_blocking(project_cwd) { + let key = pg_instance_key(&c.user_name, &c.major); + if load_info_locked(&key, lock)?.is_some() { + continue; + } + ensure_pg_data_dir(&c.user_name, &c.major)?; + let info = ServerInfo { + name: key, + pid: 0, + version: c.image, + http_port: 0, + tcp_port: c.host_port.unwrap_or(0), + started_at: "recovered".to_string(), + cwd: project_cwd.to_string(), + engine: Engine::Postgres, + container_id: Some(c.container_id), }; - for c in containers { - let key = pg_instance_key(&c.user_name, &c.major); - if load_info_locked(&key, lock)?.is_some() { - continue; - } - ensure_pg_data_dir(&c.user_name, &c.major)?; - let info = ServerInfo { - name: key, - pid: 0, - version: c.image.clone(), - http_port: 0, - tcp_port: c.host_port.unwrap_or(0), - started_at: "recovered".to_string(), - cwd: cwd_owned.clone(), - engine: Engine::Postgres, - container_id: Some(c.container_id.clone()), - }; - save_server_info_locked(&info, lock)?; + save_server_info_locked(&info, lock)?; + } + Ok(()) +} + +/// Postgres containers labelled with `project_cwd`, writing nothing. +/// +/// Discovery is best effort: when Docker isn't reachable, `connect()` fails +/// fast (no socket → immediate error, no I/O timeout) and this finds nothing, +/// as does a failed container listing. +pub fn discover_project_postgres_blocking(project_cwd: &str) -> Vec { + block_on(async { + match connect().await { + Ok(docker) => list_project_postgres(&docker, project_cwd) + .await + .unwrap_or_default(), + Err(_) => Vec::new(), } - Ok(()) }) } diff --git a/crates/clickhousectl/src/local/mod.rs b/crates/clickhousectl/src/local/mod.rs index fc83890db..4bf3a60cb 100644 --- a/crates/clickhousectl/src/local/mod.rs +++ b/crates/clickhousectl/src/local/mod.rs @@ -391,12 +391,13 @@ fn run_client( None, ) }; - let metadata_lock = server::lock_metadata().map_err(project_state_error)?; - server::recover_current_project_servers_locked(&metadata_lock) - .map_err(project_state_error)?; + let server_not_found = || managed_error(ManagedClientErrorKind::ServerNotFound, None); + let metadata_lock = server::lock_recovered_metadata() + .map_err(project_state_error)? + .ok_or_else(server_not_found)?; let entry = server::server_entry_locked(server_name, &metadata_lock) .map_err(project_state_error)? - .ok_or_else(|| managed_error(ManagedClientErrorKind::ServerNotFound, None))?; + .ok_or_else(server_not_found)?; if !entry.running { return Err(managed_error( ManagedClientErrorKind::ServerNotRunning, @@ -793,10 +794,10 @@ fn dotenv_server( json: bool, ) -> Result<()> { let server_name = name.unwrap_or("default"); - let metadata_lock = server::lock_metadata()?; - server::recover_current_project_servers_locked(&metadata_lock)?; - let entry = server::server_entry_locked(server_name, &metadata_lock)? - .ok_or_else(|| Error::ServerNotFound(server_name.to_string()))?; + let server_not_found = || Error::ServerNotFound(server_name.to_string()); + let metadata_lock = server::lock_recovered_metadata()?.ok_or_else(server_not_found)?; + let entry = + server::server_entry_locked(server_name, &metadata_lock)?.ok_or_else(server_not_found)?; if !entry.running { return Err(Error::ServerNotRunning(server_name.to_string())); } @@ -1038,16 +1039,19 @@ fn stop_server( let project_dir = init::canonical_project_dir()?; let project_state_missing = !init::local_dir().is_dir(); - // Recover orphaned servers so we can stop processes - // that lost their metadata files. - let metadata_lock = server::lock_metadata()?; - server::recover_current_project_servers_locked(&metadata_lock)?; + // Recover orphaned servers so we can stop processes that lost their + // metadata files. Without server state or an orphan there is nothing to + // stop. + let metadata_lock = server::lock_recovered_metadata()?; let (name, selection, known) = match name_input { ServerNameInput::Positional(name) | ServerNameInput::NameFlag(name) => { (name, output::ServerSelection::Explicit, false) } ServerNameInput::Omitted => { - let names = server::list_clickhouse_server_names_locked(&metadata_lock)?; + let names = match &metadata_lock { + Some(lock) => server::list_clickhouse_server_names_locked(lock)?, + None => Vec::new(), + }; if names.iter().any(|name| name == "default") { ( "default".to_string(), @@ -1083,6 +1087,14 @@ fn stop_server( } }; + let Some(metadata_lock) = metadata_lock else { + // Only reachable for an explicit name: an omitted one returned above. + return Err(Error::ProjectServerNotFound(ProjectServerNotFound { + command: ProjectServerCommand::Stop, + project_dir, + server_name: name, + })); + }; match classify_stop( server::is_server_running_locked(&name, &metadata_lock)?, known || server::server_data_dir(&name).exists(), @@ -1132,16 +1144,19 @@ fn remove_server(name_input: ServerNameInput, json: bool) -> Result<()> { let project_dir = init::canonical_project_dir()?; let project_state_missing = !init::local_dir().is_dir(); - // Recover orphaned servers so we correctly detect a running - // process even when its metadata file is missing. - let metadata_lock = server::lock_metadata()?; - server::recover_current_project_servers_locked(&metadata_lock)?; + // Recover orphaned servers so we correctly detect a running process even + // when its metadata file is missing. Without server state or an orphan + // there is nothing to remove. + let metadata_lock = server::lock_recovered_metadata()?; let (name, selection) = match name_input { ServerNameInput::Positional(name) | ServerNameInput::NameFlag(name) => { (name, output::ServerSelection::Explicit) } ServerNameInput::Omitted => { - let names = server::list_clickhouse_server_names_locked(&metadata_lock)?; + let names = match &metadata_lock { + Some(lock) => server::list_clickhouse_server_names_locked(lock)?, + None => Vec::new(), + }; if names.iter().any(|name| name == "default") { ("default".to_string(), output::ServerSelection::Implicit) } else { @@ -1160,6 +1175,17 @@ fn remove_server(name_input: ServerNameInput, json: bool) -> Result<()> { } }; + let server_not_found = |server_name| { + Error::ProjectServerNotFound(ProjectServerNotFound { + command: ProjectServerCommand::Remove, + project_dir, + server_name, + }) + }; + // Only reachable for an explicit name: an omitted one returned above. + let Some(metadata_lock) = metadata_lock else { + return Err(server_not_found(name)); + }; if server::is_server_running_locked(&name, &metadata_lock)? { return Err(Error::ServerRunningCannotRemove { command: format!("clickhousectl local server stop {name}"), @@ -1168,11 +1194,7 @@ fn remove_server(name_input: ServerNameInput, json: bool) -> Result<()> { } let data_dir = server::server_data_dir(&name); if !data_dir.exists() { - return Err(Error::ProjectServerNotFound(ProjectServerNotFound { - command: ProjectServerCommand::Remove, - project_dir, - server_name: name, - })); + return Err(server_not_found(name)); } // Remove the whole server directory (parent of data/) let server_dir = data_dir.parent().unwrap(); @@ -1374,11 +1396,14 @@ fn global_stop_target(name_input: ServerNameInput) -> Result<(String, output::Se } fn stop_all_servers_local(json: bool) -> Result<()> { - let metadata_lock = server::lock_metadata()?; - server::recover_current_project_servers_locked(&metadata_lock)?; - let servers = server::list_running_servers_locked(&metadata_lock)?; - let out = stop_servers(&servers, json, |name| { - server::kill_server_locked(name, &metadata_lock) + let metadata_lock = server::lock_recovered_metadata()?; + let servers = match &metadata_lock { + Some(lock) => server::list_running_servers_locked(lock)?, + None => Vec::new(), + }; + let out = stop_servers(&servers, json, |name| match &metadata_lock { + Some(lock) => server::kill_server_locked(name, lock), + None => Err(Error::ServerNotRunning(name.to_string())), }); if json { output::print_output(&out, json); diff --git a/crates/clickhousectl/src/local/postgres.rs b/crates/clickhousectl/src/local/postgres.rs index 7e9402ea7..bb04b1ac9 100644 --- a/crates/clickhousectl/src/local/postgres.rs +++ b/crates/clickhousectl/src/local/postgres.rs @@ -617,6 +617,33 @@ fn resolve_pg_start_version_locked( } } +/// Lock project metadata, recover orphans, and resolve the target under that +/// lock. A project with neither server state nor an orphan has no target, so +/// this then creates no `.clickhouse/servers/`. +fn lock_and_resolve_pg_target( + user_name: &str, + version: Option<&str>, +) -> Result<(server::ServerInfo, server::MetadataLock)> { + let Some(metadata_lock) = server::lock_recovered_metadata()? else { + return Err(missing_pg_target(user_name, version)); + }; + let target = resolve_pg_target_locked(user_name, version, &metadata_lock)?; + Ok((target, metadata_lock)) +} + +/// The error for a `[NAME] [--version ]` selection that matches nothing. +fn missing_pg_target(user_name: &str, version: Option<&str>) -> Error { + match version { + Some(v) => match validate_pg_tag(v) { + Ok(()) => { + Error::ServerNotFound(format!("{user_name} (postgres:{})", pg_major_from_tag(v))) + } + Err(error) => error, + }, + None => Error::ServerNotFound(user_name.to_string()), + } +} + /// Resolve `[NAME] [--version ]` to a single Postgres instance on disk. /// If `version` is given, target the (X, major(V)) pair directly. Otherwise: /// 0 instances → ServerNotFound; 1 → use it; >1 → ask for `--version`. @@ -631,11 +658,11 @@ fn resolve_pg_target_locked( let key = server::pg_instance_key(user_name, &major); return server::load_info_locked(&key, metadata_lock)? .filter(|i| i.engine == Engine::Postgres) - .ok_or_else(|| Error::ServerNotFound(format!("{user_name} (postgres:{major})"))); + .ok_or_else(|| missing_pg_target(user_name, version)); } let instances = server::find_pg_instances_locked(user_name, metadata_lock)?; match instances.len() { - 0 => Err(Error::ServerNotFound(user_name.to_string())), + 0 => Err(missing_pg_target(user_name, None)), 1 => Ok(instances.into_iter().next().unwrap()), _ => { let versions: Vec = instances.iter().map(|i| i.version.clone()).collect(); @@ -998,9 +1025,7 @@ fn generate_password() -> String { async fn stop(name: &str, version: Option<&str>, json: bool) -> Result<()> { server::validate_server_name(name)?; - let metadata_lock = server::lock_metadata()?; - server::recover_current_project_servers_locked(&metadata_lock)?; - let target = resolve_pg_target_locked(name, version, &metadata_lock)?; + let (target, metadata_lock) = lock_and_resolve_pg_target(name, version)?; if !json { let display = format!("{} ({})", user_name_from_key(&target.name), target.version); println!("Stopping Postgres {}...", display); @@ -1016,19 +1041,22 @@ async fn stop(name: &str, version: Option<&str>, json: bool) -> Result<()> { } async fn stop_all(json: bool) -> Result<()> { - let metadata_lock = server::lock_metadata()?; - server::recover_current_project_servers_locked(&metadata_lock)?; - let servers: Vec<_> = server::list_running_servers_locked(&metadata_lock)? - .into_iter() - .filter(|s| s.engine == Engine::Postgres) - .collect(); + let metadata_lock = server::lock_recovered_metadata()?; + let servers: Vec<_> = match &metadata_lock { + Some(lock) => server::list_running_servers_locked(lock)?, + None => Vec::new(), + } + .into_iter() + .filter(|s| s.engine == Engine::Postgres) + .collect(); if !json && servers.is_empty() { println!("No running Postgres servers"); return Ok(()); } - let out = super::stop_servers(&servers, json, |name| { - server::kill_server_locked(name, &metadata_lock) + let out = super::stop_servers(&servers, json, |name| match &metadata_lock { + Some(lock) => server::kill_server_locked(name, lock), + None => Err(Error::ServerNotRunning(name.to_string())), }); if json { output::print_output(&out, json); @@ -1040,10 +1068,7 @@ async fn stop_all(json: bool) -> Result<()> { fn remove(name: &str, version: Option<&str>, json: bool) -> Result<()> { server::validate_server_name(name)?; - let metadata_lock = server::lock_metadata()?; - server::recover_current_project_servers_locked(&metadata_lock)?; - - let target = resolve_pg_target_locked(name, version, &metadata_lock)?; + let (target, metadata_lock) = lock_and_resolve_pg_target(name, version)?; let key = target.name.clone(); if server::is_server_running_locked(&key, &metadata_lock)? { let tag = target @@ -1114,10 +1139,8 @@ async fn client( ); } - let metadata_lock = server::lock_metadata()?; - server::recover_current_project_servers_locked(&metadata_lock)?; let server_name = name.as_deref().unwrap_or("default"); - let info = resolve_pg_target_locked(server_name, version.as_deref(), &metadata_lock)?; + let (info, metadata_lock) = lock_and_resolve_pg_target(server_name, version.as_deref())?; if !server::is_server_running_locked(&info.name, &metadata_lock)? { return Err(Error::ServerNotRunning(server_name.to_string())); } @@ -1259,10 +1282,8 @@ fn exec_host_psql( } fn dotenv(name: Option<&str>, version: Option<&str>, use_local: bool, json: bool) -> Result<()> { - let metadata_lock = server::lock_metadata()?; - server::recover_current_project_servers_locked(&metadata_lock)?; let server_name = name.unwrap_or("default"); - let info = resolve_pg_target_locked(server_name, version, &metadata_lock)?; + let (info, metadata_lock) = lock_and_resolve_pg_target(server_name, version)?; if !server::is_server_running_locked(&info.name, &metadata_lock)? { return Err(Error::ServerNotRunning(server_name.to_string())); } diff --git a/crates/clickhousectl/src/local/server.rs b/crates/clickhousectl/src/local/server.rs index 0aefe97aa..b8e7c5da9 100644 --- a/crates/clickhousectl/src/local/server.rs +++ b/crates/clickhousectl/src/local/server.rs @@ -108,12 +108,20 @@ fn servers_dir() -> PathBuf { /// from their final state read through their state-determining write. No code /// holding it may acquire an install lock, and metadata helpers with a /// `_locked` suffix never acquire it again. +/// +/// Only commands that create servers may create the lock directory +/// ([`lock_metadata`]). Commands that read, stop or remove existing servers use +/// [`lock_existing_metadata`], or [`lock_recovered_metadata`] when they also +/// recover orphans. Both return `None` when the project has no server state +/// (and, for recovery, nothing running to recover), so they never leave +/// `.clickhouse/` behind (issue #1077). pub(crate) struct MetadataLock { _file: File, dir: PathBuf, } impl MetadataLock { + /// Acquire the lock in `dir`, creating the directory first. pub(crate) fn acquire_at(dir: &Path) -> Result { std::fs::create_dir_all(dir).map_err(|source| { server_lock_error( @@ -123,21 +131,47 @@ impl MetadataLock { source, ) })?; + Self::open_and_lock(dir)?.ok_or_else(|| { + server_lock_error( + "open the server metadata lock file", + &dir.join(METADATA_LOCK_FILE), + "Check that nothing removes the server metadata directory, then retry.", + std::io::ErrorKind::NotFound.into(), + ) + }) + } + + /// Acquire the lock in `dir` only if the directory already exists. + /// + /// There is no separate existence check: opening the lock file is the + /// check, so a concurrent `start` that creates the directory is either + /// seen (and waited for, through the lock) or ordered after this caller, + /// which then correctly observes no servers. + pub(crate) fn acquire_existing_at(dir: &Path) -> Result> { + Self::open_and_lock(dir) + } + + /// Open and lock `dir/.metadata.lock`. `None` means `dir` does not exist. + fn open_and_lock(dir: &Path) -> Result> { let path = dir.join(METADATA_LOCK_FILE); - let file = OpenOptions::new() + let file = match OpenOptions::new() .read(true) .write(true) .create(true) .truncate(false) .open(&path) - .map_err(|source| { - server_lock_error( + { + Ok(file) => file, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(source) => { + return Err(server_lock_error( "open the server metadata lock file", &path, "Check read and write access to the lock file and its directory, then retry.", source, - ) - })?; + )); + } + }; file.lock().map_err(|source| { server_lock_error( "acquire the server metadata lock", @@ -146,17 +180,25 @@ impl MetadataLock { source, ) })?; - Ok(Self { + Ok(Some(Self { _file: file, dir: dir.to_path_buf(), - }) + })) } } +/// Lock this project's server metadata, creating `.clickhouse/servers/`. +/// Only for commands that create servers. pub(crate) fn lock_metadata() -> Result { MetadataLock::acquire_at(&servers_dir()) } +/// Lock this project's server metadata if it has any. `None` means the project +/// has no server state: nothing to read, recover, stop or remove. +pub(crate) fn lock_existing_metadata() -> Result> { + MetadataLock::acquire_existing_at(&servers_dir()) +} + /// Data directory for a ClickHouse server: .clickhouse/servers//data/. pub fn server_data_dir(name: &str) -> PathBuf { servers_dir().join(name).join("data") @@ -305,7 +347,9 @@ pub(crate) fn try_remove_server_info_locked(name: &str, lock: &MetadataLock) -> /// 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 lock = lock_metadata()?; + let Some(lock) = lock_existing_metadata()? else { + return Ok(()); + }; mark_server_stopped_locked(name, pid, &lock) } @@ -427,8 +471,9 @@ fn load_running_info_locked(name: &str, lock: &MetadataLock) -> Result-pg`. Also runs process/container discovery so /// orphaned instances reappear. pub fn list_all_servers() -> Result> { - let lock = lock_metadata()?; - recover_current_project_servers_locked(&lock)?; + let Some(lock) = lock_recovered_metadata()? else { + return Ok(Vec::new()); + }; list_all_servers_locked(&lock) } @@ -579,7 +624,10 @@ pub(crate) fn advisory_running_server_count_locked(lock: &MetadataLock) -> usize /// Check if a named server is currently running. pub fn is_server_running(name: &str) -> Result { - let lock = lock_metadata()?; + let Some(lock) = lock_existing_metadata()? else { + validate_server_name(name)?; + return Ok(false); + }; is_server_running_locked(name, &lock) } @@ -702,7 +750,10 @@ 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 lock = lock_metadata()?; + let Some(lock) = lock_existing_metadata()? else { + validate_server_name(name)?; + return Err(Error::ServerNotRunning(name.to_string())); + }; kill_server_locked(name, &lock) } @@ -733,19 +784,34 @@ pub(crate) fn kill_server_locked(name: &str, lock: &MetadataLock) -> Result<()> /// or generate a random name if "default" is already running. /// Returns an error if the provided name contains path traversal characters. pub fn resolve_name(name: Option<&str>) -> Result { - let lock = lock_metadata()?; - resolve_name_locked(name, &lock) + match lock_existing_metadata()? { + Some(lock) => resolve_name_locked(name, &lock), + // No metadata, so no running "default" to avoid. + None => resolve_name_with(name, || Ok(false), || unreachable!()), + } } pub(crate) fn resolve_name_locked(name: Option<&str>, lock: &MetadataLock) -> Result { + resolve_name_with( + name, + || is_server_running_locked("default", lock), + || generate_random_name_locked(lock), + ) +} + +fn resolve_name_with( + name: Option<&str>, + default_running: impl FnOnce() -> Result, + generate: impl FnOnce() -> Result, +) -> Result { match name { Some(n) => { validate_server_name(n)?; Ok(n.to_string()) } None => { - if is_server_running_locked("default", lock)? { - generate_random_name_locked(lock) + if default_running()? { + generate() } else { Ok("default".to_string()) } @@ -1011,36 +1077,92 @@ pub fn now_timestamp() -> String { /// reports, so it is the process `stop` has to signal — the watchdog when the /// server has one — exactly as if `server start` had written it. pub fn recover_current_project_servers() -> Result<()> { - let lock = lock_metadata()?; - recover_current_project_servers_locked(&lock) + lock_recovered_metadata().map(drop) } pub(crate) fn recover_current_project_servers_locked(lock: &MetadataLock) -> Result<()> { recover_from_discovered_locked(&discovery::discover_clickhouse_processes(), lock) } -/// Recovery over an already-completed process scan, so a caller that also needs -/// the global view pays for `pgrep`/`lsof` once. -fn recover_from_discovered_locked( +/// Lock this project's server metadata and recover orphaned servers into it. +/// +/// Deleting `.clickhouse/` does not stop a server, so a project without server +/// state still runs discovery. Only when that finds an instance this project +/// owns are `.clickhouse/servers/` created, locked and recovered into, exactly +/// as if the state had been there. `None` means there is no server state and +/// nothing to recover, and then nothing is created (issue #1077). +pub(crate) fn lock_recovered_metadata() -> Result> { + lock_recovered_metadata_from(&discovery::discover_clickhouse_processes()) +} + +/// [`lock_recovered_metadata`] over an already-completed process scan. +fn lock_recovered_metadata_from( processes: &[discovery::DiscoveredProcess], - lock: &MetadataLock, -) -> Result<()> { - let current_dir = std::env::current_dir()? +) -> Result> { + let lock = lock_metadata_if_recoverable_at(&servers_dir(), || { + let current_dir = current_project_dir()?; + Ok(current_project_processes(processes, ¤t_dir) + .next() + .is_some() + || !docker::discover_project_postgres_blocking(¤t_dir).is_empty()) + })?; + let Some(lock) = lock else { + return Ok(None); + }; + // Recovery re-checks under the lock: it skips any instance a concurrent + // command recorded after discovery and lists the containers again. + recover_from_discovered_locked(processes, &lock)?; + Ok(Some(lock)) +} + +/// Lock the metadata in `dir`. When `dir` is absent, `recoverable` decides +/// whether there is anything to recover; only then is `dir` created. +fn lock_metadata_if_recoverable_at( + dir: &Path, + recoverable: impl FnOnce() -> Result, +) -> Result> { + if let Some(lock) = MetadataLock::acquire_existing_at(dir)? { + return Ok(Some(lock)); + } + if !recoverable()? { + return Ok(None); + } + // A concurrent command may have created `dir` since; acquiring waits for it. + MetadataLock::acquire_at(dir).map(Some) +} + +/// The canonical current directory, as recorded in recovered metadata and in +/// the Postgres containers' project label. +fn current_project_dir() -> Result { + Ok(std::env::current_dir()? .canonicalize()? .display() - .to_string(); + .to_string()) +} - for proc in processes { +/// The discovered ClickHouse processes started from `current_dir`. +fn current_project_processes<'a>( + processes: &'a [discovery::DiscoveredProcess], + current_dir: &'a str, +) -> impl Iterator { + processes.iter().filter(move |proc| { // Canonicalize the discovered project root for comparison - let discovered_root = match std::path::Path::new(&proc.project_root).canonicalize() { - Ok(p) => p.display().to_string(), - Err(_) => proc.project_root.clone(), - }; - - if discovered_root != current_dir { - continue; + match Path::new(&proc.project_root).canonicalize() { + Ok(root) => root.display().to_string() == current_dir, + Err(_) => proc.project_root == current_dir, } + }) +} +/// Recovery over an already-completed process scan, so a caller that also needs +/// the global view pays for `pgrep`/`lsof` once. +fn recover_from_discovered_locked( + processes: &[discovery::DiscoveredProcess], + lock: &MetadataLock, +) -> Result<()> { + let current_dir = current_project_dir()?; + + for proc in current_project_processes(processes, ¤t_dir) { validate_server_name(&proc.server_name)?; let info = ServerInfo { name: proc.server_name.clone(), @@ -1130,12 +1252,11 @@ pub fn servers_using_version(version: &str) -> Result> { // One process scan feeds both sources: discovery shells out to // `pgrep`/`lsof`/`ps`, so scanning twice would double the cost of the guard. let processes = discovery::discover_clickhouse_processes(); - let project_servers = { - let lock = lock_metadata()?; - // Recover orphans first so a server whose metadata file is missing is - // still counted. - recover_from_discovered_locked(&processes, &lock)?; - list_running_servers_locked(&lock)? + // Recover orphans first so a server whose metadata file is missing is + // still counted as this project's. + let project_servers = match lock_recovered_metadata_from(&processes)? { + Some(lock) => list_running_servers_locked(&lock)?, + None => Vec::new(), }; Ok(select_version_users( &project_servers, @@ -1392,6 +1513,178 @@ mod tests { )); } + #[test] + fn recovery_lock_creates_nothing_when_there_is_nothing_to_recover() { + let directory = tempfile::tempdir().unwrap(); + let lock_directory = directory.path().join(".clickhouse").join("servers"); + + let lock = lock_metadata_if_recoverable_at(&lock_directory, || Ok(false)).unwrap(); + + assert!(lock.is_none()); + assert!(!directory.path().join(".clickhouse").exists()); + } + + #[test] + fn recovery_lock_creates_the_state_for_a_recoverable_orphan() { + let directory = tempfile::tempdir().unwrap(); + let lock_directory = directory.path().join(".clickhouse").join("servers"); + + let lock = lock_metadata_if_recoverable_at(&lock_directory, || Ok(true)) + .unwrap() + .expect("an orphan to recover yields a lock"); + + assert_eq!(lock.dir, lock_directory); + assert!(lock_directory.join(METADATA_LOCK_FILE).is_file()); + } + + #[test] + fn recovery_lock_skips_discovery_when_the_state_exists() { + let directory = tempfile::tempdir().unwrap(); + + let lock = lock_metadata_if_recoverable_at(directory.path(), || { + panic!("existing state needs no discovery check") + }) + .unwrap(); + + assert!(lock.is_some()); + } + + #[test] + fn recovery_lock_propagates_a_failed_discovery_check() { + let directory = tempfile::tempdir().unwrap(); + let lock_directory = directory.path().join("servers"); + + let result = lock_metadata_if_recoverable_at(&lock_directory, || { + Err(Error::Io(std::io::Error::other("no current directory"))) + }); + + assert!(result.is_err()); + assert!(!lock_directory.exists()); + } + + #[test] + fn current_project_processes_select_only_this_projects_clickhouse() { + let project = tempfile::tempdir().unwrap(); + let other = tempfile::tempdir().unwrap(); + let current_dir = project.path().canonicalize().unwrap().display().to_string(); + let process = |root: &Path, name: &str| discovery::DiscoveredProcess { + pid: 42, + project_root: root.display().to_string(), + server_name: name.to_string(), + http_port: Some(8123), + tcp_port: Some(9000), + version: None, + }; + let processes = [ + process(project.path(), "default"), + process(other.path(), "elsewhere"), + // A project directory that no longer canonicalizes still matches + // by its recorded path. + process(Path::new("/nonexistent/project"), "gone"), + ]; + + let names: Vec<_> = current_project_processes(&processes, ¤t_dir) + .map(|proc| proc.server_name.as_str()) + .collect(); + assert_eq!(names, ["default"]); + + let names: Vec<_> = current_project_processes(&processes, "/nonexistent/project") + .map(|proc| proc.server_name.as_str()) + .collect(); + assert_eq!(names, ["gone"]); + } + + #[test] + fn existing_metadata_lock_is_none_without_creating_the_directory() { + let directory = tempfile::tempdir().unwrap(); + let lock_directory = directory.path().join(".clickhouse").join("servers"); + + assert!( + MetadataLock::acquire_existing_at(&lock_directory) + .unwrap() + .is_none() + ); + assert!(!directory.path().join(".clickhouse").exists()); + } + + #[test] + fn existing_metadata_lock_reads_an_existing_directory() { + let directory = tempfile::tempdir().unwrap(); + let writer = MetadataLock::acquire_at(directory.path()).unwrap(); + save_server_info_locked(&test_info(0, ""), &writer).unwrap(); + drop(writer); + + let lock = MetadataLock::acquire_existing_at(directory.path()) + .unwrap() + .expect("existing directory yields a lock"); + assert_eq!(lock.dir, directory.path()); + let entries = list_all_servers_locked(&lock).unwrap(); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].name, "default"); + } + + #[test] + fn existing_metadata_lock_creates_only_the_lock_file_in_an_existing_directory() { + let directory = tempfile::tempdir().unwrap(); + + let lock = MetadataLock::acquire_existing_at(directory.path()).unwrap(); + + assert!(lock.is_some()); + let names: Vec<_> = std::fs::read_dir(directory.path()) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .collect(); + assert_eq!(names, [METADATA_LOCK_FILE]); + } + + #[test] + fn existing_metadata_lock_reports_a_non_directory_path() { + let directory = tempfile::tempdir().unwrap(); + let lock_directory = directory.path().join("servers"); + std::fs::write(&lock_directory, b"not a directory").unwrap(); + + let error = match MetadataLock::acquire_existing_at(&lock_directory) { + Ok(_) => panic!("metadata lock acquisition unexpectedly succeeded"), + Err(error) => error, + }; + + assert!(matches!( + error, + Error::ServerLock { + operation: "open the server metadata lock file", + path, + .. + } if path == lock_directory.join(METADATA_LOCK_FILE) + )); + } + + #[test] + fn existing_metadata_lock_waits_for_a_creating_holder() { + let directory = tempfile::tempdir().unwrap(); + let lock_directory = directory.path().join("servers"); + let holder = MetadataLock::acquire_at(&lock_directory).unwrap(); + let (acquired_tx, acquired_rx) = std::sync::mpsc::channel(); + let reader_dir = lock_directory.clone(); + + let reader = std::thread::spawn(move || { + let lock = MetadataLock::acquire_existing_at(&reader_dir).unwrap(); + acquired_tx.send(()).unwrap(); + let lock = lock.expect("directory created by the holder"); + list_all_servers_locked(&lock).unwrap().len() + }); + assert!( + acquired_rx + .recv_timeout(Duration::from_millis(200)) + .is_err(), + "reader acquired the lock while the creator held it" + ); + save_server_info_locked(&test_info(0, ""), &holder).unwrap(); + drop(holder); + + acquired_rx.recv_timeout(Duration::from_secs(10)).unwrap(); + assert_eq!(reader.join().unwrap(), 1); + } + #[test] fn atomic_save_ignores_interrupted_sibling_temp_files() { let directory = tempfile::tempdir().unwrap(); diff --git a/crates/clickhousectl/src/local/udf.rs b/crates/clickhousectl/src/local/udf.rs index 90e593efd..ee28a1410 100644 --- a/crates/clickhousectl/src/local/udf.rs +++ b/crates/clickhousectl/src/local/udf.rs @@ -491,7 +491,12 @@ async fn deploy( if let Some(info) = &target.running { drop(lock); was_loaded = check_name_is_free(info, server_name, name).await?; - lock = server::lock_metadata()?; + // The server existed a moment ago; if its state has gone since, write + // nothing rather than recreate it. + lock = match server::lock_existing_metadata()? { + Some(lock) => lock, + None => return Err(server_not_found(server_name)?), + }; } let scripts_dir = target.data_dir.join(SCRIPTS_DIR).join(name); @@ -1118,8 +1123,14 @@ struct ServerTarget { /// callers can hold it across their file writes and release it before any /// HTTP round trip. fn server_target(name: &str) -> Result<(ServerTarget, MetadataLock)> { - let lock = server::lock_metadata()?; - server::recover_current_project_servers_locked(&lock)?; + // Without server state or an orphan there is no server to target, and + // nothing is created to find that out. + let Some(lock) = server::lock_recovered_metadata()? else { + if server::is_pg_instance_key(name) { + return Err(Error::UdfServerIsPostgres(name.to_owned())); + } + return Err(server_not_found(name)?); + }; let entry = server::server_entry_locked(name, &lock)?; let is_postgres = match entry.as_ref().and_then(|entry| entry.info.as_ref()) { Some(info) => info.engine != Engine::Clickhouse, @@ -1133,16 +1144,20 @@ fn server_target(name: &str) -> Result<(ServerTarget, MetadataLock)> { .and_then(|entry| entry.info); let data_dir = server::server_data_dir(name); if running.is_none() && !data_dir.is_dir() { - let project_dir = crate::init::canonical_project_dir()?; - return Err(Error::UdfServerNotFound { - name: name.to_owned(), - project_has_state: looks_like_project_root(&project_dir), - project_dir, - }); + return Err(server_not_found(name)?); } Ok((ServerTarget { data_dir, running }, lock)) } +fn server_not_found(name: &str) -> Result { + let project_dir = crate::init::canonical_project_dir()?; + Ok(Error::UdfServerNotFound { + name: name.to_owned(), + project_has_state: looks_like_project_root(&project_dir), + project_dir, + }) +} + /// Whether `dir` is plausibly where servers are started: it has the /// `clickhouse/` scaffold or some server's state. Local commands never search /// parent directories, so a subdirectory of a project has neither. @@ -1150,8 +1165,9 @@ fn looks_like_project_root(dir: &Path) -> bool { if dir.join("clickhouse").is_dir() { return true; } - // The metadata lock file is created by any `local udf` call, even in a - // subdirectory, so it does not count as server state. + // Dotfiles are metadata bookkeeping (the lock file, interrupted temporary + // writes), not a server's state. Read-only commands no longer create the + // lock file (#1077), but it outlives the last removed server. std::fs::read_dir(dir.join(".clickhouse").join("servers")) .map(|entries| { entries diff --git a/crates/clickhousectl/tests/local_client_project_scope_errors_test.rs b/crates/clickhousectl/tests/local_client_project_scope_errors_test.rs index 878407577..eb7d7b89e 100644 --- a/crates/clickhousectl/tests/local_client_project_scope_errors_test.rs +++ b/crates/clickhousectl/tests/local_client_project_scope_errors_test.rs @@ -370,7 +370,7 @@ fn invalid_metadata_lock_path_keeps_managed_scope_in_json_and_human_errors() { stderr.starts_with("Error: Managed client mode: server 'default' could not be resolved") ); assert!(stderr.contains(&project_path)); - assert!(stderr.contains("create the server metadata lock directory")); + assert!(stderr.contains("open the server metadata lock file")); assert!(stderr.contains("`clickhousectl local server list`")); assert!(stderr.contains("`clickhousectl local client --host --port `")); } diff --git a/crates/clickhousectl/tests/local_init_json_test.rs b/crates/clickhousectl/tests/local_init_json_test.rs index a64c4706e..0258b3cb8 100644 --- a/crates/clickhousectl/tests/local_init_json_test.rs +++ b/crates/clickhousectl/tests/local_init_json_test.rs @@ -154,12 +154,13 @@ fn init_after_server_list_creates_runtime_gitignore() { "stderr: {}", String::from_utf8_lossy(&list.stderr) ); - assert!( - project - .path() - .join(".clickhouse/servers/.metadata.lock") - .is_file() - ); + // Listing no longer creates project state (#1077), but releases before it + // left the metadata lock behind without an ignore file. Init must repair + // that partially initialized directory. + assert!(!project.path().join(".clickhouse").exists()); + let servers = project.path().join(".clickhouse/servers"); + std::fs::create_dir_all(&servers).expect("create legacy servers directory"); + std::fs::write(servers.join(".metadata.lock"), "").expect("create legacy metadata lock"); let init = run(project.path(), home.path(), &["local", "init"]); assert!( diff --git a/crates/clickhousectl/tests/local_readonly_no_project_state_test.rs b/crates/clickhousectl/tests/local_readonly_no_project_state_test.rs new file mode 100644 index 000000000..a314b6c43 --- /dev/null +++ b/crates/clickhousectl/tests/local_readonly_no_project_state_test.rs @@ -0,0 +1,446 @@ +//! Read-only and lookup local commands leave no project state behind (#1077). +//! +//! Each command runs in an empty directory, or one with only `local init` +//! state, and must report its usual empty or not-found result without creating +//! `.clickhouse/` or `.clickhouse/servers/`. Deleting that state does not stop +//! a running instance, though, so one this project still owns is recovered +//! exactly as when the state is present. + +use serde_json::{Value, json}; +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output, Stdio}; +use wiremock::matchers::{method, path_regex}; +use wiremock::{Mock, MockServer, ResponseTemplate}; + +fn clickhousectl_binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_clickhousectl")) +} + +/// Run with Docker unreachable: discovery must tolerate it and find nothing. +fn run(project: &Path, home: &Path, args: &[&str]) -> Output { + let docker_host = format!("unix://{}/missing-docker.sock", home.display()); + run_with_docker(project, home, &docker_host, args) +} + +fn run_with_docker(project: &Path, home: &Path, docker_host: &str, args: &[&str]) -> Output { + Command::new(clickhousectl_binary()) + .env_clear() + .env("DO_NOT_TRACK", "1") + .env("HOME", home) + .env("PATH", "/usr/bin:/bin") + .env("DOCKER_HOST", docker_host) + .current_dir(project) + .args(args) + .stdin(Stdio::null()) + .output() + .expect("run clickhousectl") +} + +fn created_entries(project: &Path) -> Vec { + let mut entries: Vec<_> = std::fs::read_dir(project) + .expect("read project directory") + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .collect(); + entries.sort(); + entries +} + +fn stdout_json(output: &Output) -> Value { + serde_json::from_slice(&output.stdout).unwrap_or_else(|error| { + panic!( + "parse stdout JSON: {error}; stdout: {}", + String::from_utf8_lossy(&output.stdout) + ) + }) +} + +fn stderr_json(output: &Output) -> Value { + serde_json::from_slice(&output.stderr).unwrap_or_else(|error| { + panic!( + "parse stderr JSON: {error}; stderr: {}", + String::from_utf8_lossy(&output.stderr) + ) + }) +} + +/// Run `args` in a fresh empty directory and assert it stays empty. +fn run_in_empty_project(args: &[&str]) -> Output { + let project = tempfile::tempdir().expect("create project"); + let home = tempfile::tempdir().expect("create home"); + let output = run(project.path(), home.path(), args); + assert_eq!( + created_entries(project.path()), + Vec::::new(), + "{args:?} created project state; stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + output +} + +fn assert_exit(output: &Output, code: i32, args: &[&str]) { + assert_eq!( + output.status.code(), + Some(code), + "{args:?}; stdout: {}; stderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn server_list_is_empty_without_creating_project_state() { + let args = ["local", "--json", "server", "list"]; + let output = run_in_empty_project(&args); + assert_exit(&output, 0, &args); + let body = stdout_json(&output); + assert_eq!(body["servers"], serde_json::json!([])); + assert_eq!(body["total_servers"], 0); + assert_eq!(body["project_scope"]["kind"], "exact_current_project"); + + let args = ["local", "server", "list"]; + let output = run_in_empty_project(&args); + assert_exit(&output, 0, &args); + assert!(String::from_utf8_lossy(&output.stdout).starts_with("No servers found in project")); +} + +#[test] +fn server_stop_reports_nothing_to_stop_without_creating_project_state() { + let args = ["local", "--json", "server", "stop"]; + let output = run_in_empty_project(&args); + assert_exit(&output, 0, &args); + let body = stdout_json(&output); + assert_eq!(body["stopped"], false); + assert_eq!(body["reason"], "no_clickhouse_servers"); + assert_eq!(body["project_scope"]["kind"], "exact_current_project"); + + for args in [ + ["local", "--json", "server", "stop", "x"], + ["local", "--json", "server", "remove", "x"], + ] { + let output = run_in_empty_project(&args); + assert_exit(&output, 1, &args); + let error = &stderr_json(&output)["error"]; + assert_eq!(error["code"], "server_not_found", "{args:?}"); + assert_eq!(error["server"]["name"], "x", "{args:?}"); + } + + let args = ["local", "--json", "server", "remove"]; + let output = run_in_empty_project(&args); + assert_exit(&output, 1, &args); + assert_eq!( + stderr_json(&output)["error"]["code"], + "server_selection_required" + ); +} + +#[test] +fn stop_all_reports_no_servers_without_creating_project_state() { + for args in [ + ["local", "--json", "server", "stop-all"], + ["local", "--json", "postgres", "stop-all"], + ] { + let output = run_in_empty_project(&args); + assert_exit(&output, 0, &args); + assert_eq!( + stdout_json(&output), + serde_json::json!({ "servers": [] }), + "{args:?}" + ); + } + + let args = ["local", "server", "stop-all"]; + let output = run_in_empty_project(&args); + assert_exit(&output, 0, &args); + assert_eq!( + String::from_utf8_lossy(&output.stdout), + "No running servers\n" + ); + + let args = ["local", "postgres", "stop-all"]; + let output = run_in_empty_project(&args); + assert_exit(&output, 0, &args); + assert_eq!( + String::from_utf8_lossy(&output.stdout), + "No running Postgres servers\n" + ); +} + +#[test] +fn managed_client_and_dotenv_report_not_found_without_creating_project_state() { + for (args, code) in [ + ( + &["local", "--json", "client"][..], + "managed_client_server_not_found", + ), + ( + &["local", "--json", "client", "--name", "x"][..], + "managed_client_server_not_found", + ), + ( + &["local", "--json", "server", "dotenv"][..], + "server_not_found", + ), + ] { + let output = run_in_empty_project(args); + assert_exit(&output, 1, args); + assert_eq!(stderr_json(&output)["error"]["code"], code, "{args:?}"); + } +} + +#[test] +fn udf_commands_report_server_not_found_without_creating_project_state() { + for args in [ + &["local", "--json", "udf", "list"][..], + &["local", "--json", "udf", "reload"][..], + &["local", "--json", "udf", "remove", "f"][..], + ] { + let output = run_in_empty_project(args); + assert_exit(&output, 1, args); + let error = &stderr_json(&output)["error"]; + assert_eq!(error["code"], "server_not_found", "{args:?}"); + assert!( + error["message"] + .as_str() + .unwrap() + .contains("no clickhouse/ directory or servers"), + "{args:?}: {error}" + ); + } +} + +#[test] +fn postgres_lookups_report_not_found_without_creating_project_state() { + for (args, message) in [ + ( + &["local", "--json", "postgres", "stop", "x"][..], + "Server 'x' not found", + ), + ( + &[ + "local", + "--json", + "postgres", + "stop", + "x", + "--version", + "17", + ][..], + "Server 'x (postgres:17)' not found", + ), + ( + &["local", "--json", "postgres", "remove", "x"][..], + "Server 'x' not found", + ), + ( + &["local", "--json", "postgres", "client"][..], + "Server 'default' not found", + ), + ( + &["local", "--json", "postgres", "dotenv"][..], + "Server 'default' not found", + ), + ] { + let output = run_in_empty_project(args); + assert_exit(&output, 1, args); + let error = &stderr_json(&output)["error"]; + assert_eq!(error["code"], "server_not_found", "{args:?}"); + assert_eq!(error["message"], message, "{args:?}"); + } +} + +#[test] +fn postgres_lookup_still_validates_the_version_without_project_state() { + let args = ["local", "--json", "postgres", "stop", "x", "--version", "9"]; + let output = run_in_empty_project(&args); + assert_exit(&output, 1, &args); + assert_ne!(stderr_json(&output)["error"]["code"], "server_not_found"); +} + +#[test] +fn removing_an_installed_version_creates_no_project_state() { + let project = tempfile::tempdir().expect("create project"); + let home = tempfile::tempdir().expect("create home"); + let version = "25.12.9.61"; + let binary = home + .path() + .join(format!(".clickhouse/versions/{version}/clickhouse")); + std::fs::create_dir_all(binary.parent().unwrap()).expect("create fake version directory"); + std::fs::write(&binary, b"#!/bin/sh\nexit 0\n").expect("write fake ClickHouse"); + std::fs::set_permissions(&binary, std::fs::Permissions::from_mode(0o755)) + .expect("make fake ClickHouse executable"); + + let args = ["local", "--json", "remove", version]; + let output = run(project.path(), home.path(), &args); + assert_exit(&output, 0, &args); + assert!(!binary.exists()); + assert_eq!(created_entries(project.path()), Vec::::new()); +} + +#[test] +fn initialized_project_without_servers_gains_no_servers_directory() { + let project = tempfile::tempdir().expect("create project"); + let home = tempfile::tempdir().expect("create home"); + let init = run(project.path(), home.path(), &["local", "init"]); + assert_exit(&init, 0, &["local", "init"]); + let servers = project.path().join(".clickhouse/servers"); + assert!(!servers.exists()); + + for (args, code) in [ + (&["local", "--json", "server", "list"][..], 0), + (&["local", "--json", "server", "stop", "x"][..], 1), + (&["local", "--json", "server", "remove", "x"][..], 1), + (&["local", "--json", "client"][..], 1), + (&["local", "--json", "udf", "list"][..], 1), + (&["local", "--json", "postgres", "dotenv"][..], 1), + ] { + let output = run(project.path(), home.path(), args); + assert_exit(&output, code, args); + assert!(!servers.exists(), "{args:?} created {}", servers.display()); + } + + // `local init` state is not server state, so omitted remove keeps asking + // for a selection rather than reporting missing `.clickhouse/` state. + let args = ["local", "--json", "server", "remove"]; + let output = run(project.path(), home.path(), &args); + assert_exit(&output, 1, &args); + assert_eq!( + stderr_json(&output)["error"]["code"], + "server_selection_required" + ); + assert!(!servers.exists()); +} + +/// A Docker daemon that still runs this project's labelled Postgres container, +/// although the project's `.clickhouse/` state is gone. +async fn docker_with_orphan_postgres(project: &Path) -> MockServer { + let project = project.canonicalize().expect("canonicalize project"); + let docker = MockServer::start().await; + Mock::given(method("GET")) + .and(path_regex(r"/_ping$")) + .respond_with(ResponseTemplate::new(200).set_body_string("OK")) + .mount(&docker) + .await; + // Docker filters on the project label, so only that project's list + // carries the container; every other project lists nothing. + let project_label = format!("clickhousectl.project={}", project.display()); + Mock::given(method("GET")) + .and(path_regex(r"/containers/json$")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([]))) + .with_priority(10) + .mount(&docker) + .await; + Mock::given(method("GET")) + .and(path_regex(r"/containers/json$")) + .and(move |request: &wiremock::Request| { + request + .url + .query_pairs() + .any(|(key, value)| key == "filters" && value.contains(&project_label)) + }) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([{ + "Id": "orphan-container", + "Image": "postgres:18", + "Labels": { + "clickhousectl.engine": "postgres", + "clickhousectl.name": "default", + "clickhousectl.major": "18", + "clickhousectl.project": project.display().to_string(), + }, + "Ports": [{ "PrivatePort": 5432, "PublicPort": 55432, "Type": "tcp" }], + }]))) + .mount(&docker) + .await; + Mock::given(method("GET")) + .and(path_regex(r"/containers/orphan-container/json$")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "Id": "orphan-container", + "State": { "Running": true, "Status": "running" }, + }))) + .mount(&docker) + .await; + Mock::given(method("POST")) + .and(path_regex(r"/containers/orphan-container/stop$")) + .respond_with(ResponseTemplate::new(204)) + .mount(&docker) + .await; + docker +} + +async fn stop_requests(docker: &MockServer) -> usize { + docker + .received_requests() + .await + .unwrap() + .iter() + .filter(|request| request.method.as_str() == "POST") + .filter(|request| { + request + .url + .path() + .ends_with("/containers/orphan-container/stop") + }) + .count() +} + +#[tokio::test(flavor = "multi_thread")] +async fn running_postgres_container_is_recovered_when_project_state_is_missing() { + for args in [ + &["local", "--json", "postgres", "stop-all"][..], + &["local", "--json", "postgres", "stop", "default"][..], + ] { + let project = tempfile::tempdir().expect("create project"); + let home = tempfile::tempdir().expect("create home"); + let docker = docker_with_orphan_postgres(project.path()).await; + let host = docker.uri().replace("http://", "tcp://"); + + let output = run_with_docker(project.path(), home.path(), &host, args); + + assert_exit(&output, 0, args); + assert_eq!(stop_requests(&docker).await, 1, "{args:?}"); + // Recovery records the instance, as it does when the state is present. + assert!( + project + .path() + .join(".clickhouse/servers/default-pg18.json") + .is_file(), + "{args:?}" + ); + } + + let project = tempfile::tempdir().expect("create project"); + let home = tempfile::tempdir().expect("create home"); + let docker = docker_with_orphan_postgres(project.path()).await; + let host = docker.uri().replace("http://", "tcp://"); + let args = ["local", "--json", "server", "list"]; + let output = run_with_docker(project.path(), home.path(), &host, &args); + assert_exit(&output, 0, &args); + let servers = stdout_json(&output)["servers"].clone(); + assert_eq!(servers.as_array().map(Vec::len), Some(1), "{servers}"); + assert_eq!(servers[0]["name"], "default", "{servers}"); + assert_eq!(servers[0]["engine"], "postgres", "{servers}"); + assert_eq!(servers[0]["running"], true, "{servers}"); +} + +#[tokio::test(flavor = "multi_thread")] +async fn other_projects_postgres_containers_create_no_project_state() { + let other = tempfile::tempdir().expect("create other project"); + let docker = docker_with_orphan_postgres(other.path()).await; + let host = docker.uri().replace("http://", "tcp://"); + for (args, code) in [ + (&["local", "--json", "server", "list"][..], 0), + (&["local", "--json", "postgres", "stop-all"][..], 0), + (&["local", "--json", "postgres", "stop", "default"][..], 1), + ] { + let project = tempfile::tempdir().expect("create project"); + let home = tempfile::tempdir().expect("create home"); + let output = run_with_docker(project.path(), home.path(), &host, args); + assert_exit(&output, code, args); + assert_eq!( + created_entries(project.path()), + Vec::::new(), + "{args:?}" + ); + } + assert_eq!(stop_requests(&docker).await, 0); +} diff --git a/crates/clickhousectl/tests/local_server_state_machine_test.rs b/crates/clickhousectl/tests/local_server_state_machine_test.rs index 6ed46678d..b3046881f 100644 --- a/crates/clickhousectl/tests/local_server_state_machine_test.rs +++ b/crates/clickhousectl/tests/local_server_state_machine_test.rs @@ -714,32 +714,48 @@ fn fresh_project(fixture: &Fixture) { label: "fresh omitted stop".into(), cwd: TestCwd::Root, args: strings(&["local", "--json", "server", "stop"]), + // The fresh list above created no `.clickhouse/` (#1077), so the + // project still has no state and the no-op explains its scope. expected: ExpectedOutput::JsonSuccess(json!({ "stopped": false, "selection": "implicit", - "reason": "no_clickhouse_servers" + "reason": "no_clickhouse_servers", + "project_scope": { + "kind": "exact_current_project", + "path": canonical(&fixture.project), + "parent_projects_searched": false + }, + "guidance": scope_guidance(true) })), }, CommandCase { label: "fresh omitted stop human output".into(), cwd: TestCwd::Root, args: strings(&["local", "server", "stop"]), - expected: ExpectedOutput::HumanSuccess( - "No ClickHouse servers found; nothing to stop\n".into(), - ), + expected: ExpectedOutput::HumanSuccess(format!( + "No ClickHouse servers found; nothing to stop\n\ + No `.clickhouse` directory existed under project '{}' when the command started.\n\ + Project-local server stop uses the exact current working directory; parent `.clickhouse` directories are not searched.\n\ + The `.clickhouse` directory typically lives in the local project root where the server was started. Return there and run `clickhousectl local server list`, or use `clickhousectl local server list --global` to locate running servers in other projects.\n", + canonical(&fixture.project) + )), }, CommandCase { label: "fresh omitted remove".into(), cwd: TestCwd::Root, args: strings(&["local", "--json", "server", "remove"]), - expected: ExpectedOutput::JsonFailure(error( - "server_selection_required", - "No server name was provided and the default ClickHouse server does not exist \ - (custom ClickHouse servers available: 0); no server was removed. Inspect them with \ - `clickhousectl local server list`; to remove one, pass its name with \ - `clickhousectl local server remove `.", - Some("clickhousectl local server list"), - )), + expected: ExpectedOutput::JsonFailure(json!({ + "error": { + "code": "server_selection_required", + "message": "No project-local server state was found in the current directory; no server was removed", + "project_scope": { + "kind": "exact_current_project", + "path": canonical(&fixture.project), + "parent_projects_searched": false + }, + "guidance": scope_guidance(false) + } + })), }, CommandCase { label: "fresh explicit unknown stop".into(), diff --git a/docs/guides/local-development.md b/docs/guides/local-development.md index 8d8cd179b..b3a51b276 100644 --- a/docs/guides/local-development.md +++ b/docs/guides/local-development.md @@ -2,7 +2,7 @@ [All documentation](../README.md) -Run project-local commands from the directory that owns your `.clickhouse/`. The CLI uses the exact current directory; it does not search parents. ClickHouse binaries are shared globally in `~/.clickhouse/`, while each project's server data is isolated. +Run project-local commands from the directory that owns your `.clickhouse/`. The CLI uses the exact current directory; it does not search parents. Of the local commands, only `local init` and server starts create `.clickhouse/`; commands that list, stop, remove or connect to servers leave a directory without one unchanged, unless a server started from it is still running, which they recover as usual. ClickHouse binaries are shared globally in `~/.clickhouse/`, while each project's server data is isolated. ## Start ClickHouse and run SQL diff --git a/scripts/classify-cloud-integration.py b/scripts/classify-cloud-integration.py index de4143f15..a714a79c2 100644 --- a/scripts/classify-cloud-integration.py +++ b/scripts/classify-cloud-integration.py @@ -118,6 +118,7 @@ "crates/clickhousectl/tests/skills_usage_test.rs": NO_SUITES, "crates/clickhousectl/tests/local_client_output_contract_test.rs": NO_SUITES, "crates/clickhousectl/tests/local_postgres_client_input_test.rs": NO_SUITES, + "crates/clickhousectl/tests/local_readonly_no_project_state_test.rs": NO_SUITES, "crates/clickhouse-cloud-api/tests/clickpipes/driver.rs": NO_SUITES, "crates/clickhouse-cloud-api/tests/clickpipes/e2e_test.rs": NO_SUITES, "crates/clickhouse-cloud-api/tests/clickpipes/kafka_test.rs": NO_SUITES, diff --git a/scripts/classify-install-integration.py b/scripts/classify-install-integration.py index 133edba1c..7090c7c8b 100644 --- a/scripts/classify-install-integration.py +++ b/scripts/classify-install-integration.py @@ -85,6 +85,7 @@ "crates/clickhousectl/tests/local_postgres_client_input_test.rs", "crates/clickhousectl/tests/local_postgres_readiness_test.rs", "crates/clickhousectl/tests/local_postgres_start_validation_test.rs", + "crates/clickhousectl/tests/local_readonly_no_project_state_test.rs", "crates/clickhousectl/tests/local_remove_default_test.rs", "crates/clickhousectl/tests/local_remove_global_guard_test.rs", "crates/clickhousectl/tests/local_server_managed_config_test.rs", diff --git a/scripts/tests/test_classify_install_integration.py b/scripts/tests/test_classify_install_integration.py index 868d49bc1..df314e0b6 100644 --- a/scripts/tests/test_classify_install_integration.py +++ b/scripts/tests/test_classify_install_integration.py @@ -160,6 +160,7 @@ def test_exact_path_mappings(self): "crates/clickhousectl/tests/local_postgres_client_input_test.rs", "crates/clickhousectl/tests/local_postgres_readiness_test.rs", "crates/clickhousectl/tests/local_postgres_start_validation_test.rs", + "crates/clickhousectl/tests/local_readonly_no_project_state_test.rs", "crates/clickhousectl/tests/local_remove_default_test.rs", "crates/clickhousectl/tests/local_remove_global_guard_test.rs", "crates/clickhousectl/tests/local_server_managed_config_test.rs",