From 842ddb8c1f5c3a6535a38dfe9622371ac201059e Mon Sep 17 00:00:00 2001 From: sdairs Date: Tue, 16 Jun 2026 10:46:10 +0100 Subject: [PATCH] Make `local server stop ` idempotent (#255) Stopping a server that exists on disk but is already stopped now exits 0 ("Server 'x' is already stopped") instead of erroring with ServerNotRunning. This matches docker/systemctl semantics and the existing idempotent behavior of `stop-all`, so scripts no longer need to guard against an already-stopped server. An unknown server name (no data dir in the project) still returns ServerNotFound (exit 1), so typos are caught. `--global` stop and the Postgres stop command are unchanged. The stop decision is extracted into a pure `classify_stop` helper and unit-tested for all three outcomes; `ServerStopOutput` gains an `already_stopped` flag surfaced in both human and `--json` output. Co-Authored-By: Claude Opus 4.8 (1M context) --- README.md | 2 + crates/clickhousectl/src/local/cli.rs | 2 + crates/clickhousectl/src/local/mod.rs | 73 ++++++++++++++++++++-- crates/clickhousectl/src/local/output.rs | 24 ++++++- crates/clickhousectl/src/local/postgres.rs | 1 + 5 files changed, 95 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 2d3abcf..3f28427 100644 --- a/README.md +++ b/README.md @@ -189,6 +189,8 @@ clickhousectl local server dotenv --local # Write to .env.local i clickhousectl local server dotenv --user default --password secret --database mydb # Include credentials ``` +**Idempotent stop:** `server stop ` is idempotent — stopping a server that exists but is already stopped exits 0 (it reports "is already stopped" rather than erroring), so scripts don't need to guard against it. An unknown server name still errors, so typos are caught. `server stop-all` likewise exits 0 when nothing is running. + **Server naming:** Without `--name`, the first server is called "default". If "default" is already running, a random name is generated (e.g. "bold-crane"). Use `--name` for stable identities you can start/stop repeatedly. **Ports:** Defaults are HTTP 8123 and TCP 9000. If these are already in use, free ports are automatically assigned and shown in the output. Use `--http-port` and `--tcp-port` to set explicit ports. diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index d5f08a5..99deec1 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -233,6 +233,8 @@ CONTEXT FOR AGENTS: Stops a named ClickHouse server. Use the name from `clickhousectl local server list`. Sends SIGTERM first, then SIGKILL if the process doesn't exit gracefully. The server's data directory is preserved — restart with `clickhousectl local server start --name `. + Idempotent: a server that exists but is already stopped exits 0 (no error). + An unknown server name still errors so typos are caught. Related: `clickhousectl local server list` to see servers.")] Stop { /// Name of the server to stop diff --git a/crates/clickhousectl/src/local/mod.rs b/crates/clickhousectl/src/local/mod.rs index e94cdc5..5f6722a 100644 --- a/crates/clickhousectl/src/local/mod.rs +++ b/crates/clickhousectl/src/local/mod.rs @@ -635,13 +635,36 @@ async fn run_server_commands(command: ServerCommands, json: bool) -> Result<()> // that lost their metadata files. server::recover_current_project_servers(); - if !json { - println!("Stopping server '{}'...", name); + match classify_stop( + server::is_server_running(&name), + server::server_data_dir(&name).exists(), + ) { + StopOutcome::Stop => { + if !json { + println!("Stopping server '{}'...", name); + } + server::kill_server(&name)?; + let out = output::ServerStopOutput { + name, + already_stopped: false, + }; + output::print_output(&out, json); + Ok(()) + } + StopOutcome::AlreadyStopped => { + // Server exists on disk but isn't running. `stop` is + // idempotent: this is the desired end state, so succeed + // instead of erroring. + let out = output::ServerStopOutput { + name, + already_stopped: true, + }; + output::print_output(&out, json); + Ok(()) + } + // No such server in this project — surface the typo. + StopOutcome::NotFound => Err(Error::ServerNotFound(name)), } - server::kill_server(&name)?; - let out = output::ServerStopOutput { name }; - output::print_output(&out, json); - Ok(()) } } ServerCommands::StopAll { global } => { @@ -683,6 +706,26 @@ async fn run_server_commands(command: ServerCommands, json: bool) -> Result<()> } } +/// What a project-scoped `server stop ` should do, given whether the +/// server is currently running and whether its data directory exists on disk. +#[derive(Debug, PartialEq, Eq)] +enum StopOutcome { + /// Running — kill it. + Stop, + /// Exists on disk but not running — idempotent noop (success). + AlreadyStopped, + /// Unknown server name — error, so typos surface. + NotFound, +} + +fn classify_stop(running: bool, exists_on_disk: bool) -> StopOutcome { + match (running, exists_on_disk) { + (true, _) => StopOutcome::Stop, + (false, true) => StopOutcome::AlreadyStopped, + (false, false) => StopOutcome::NotFound, + } +} + fn list_servers_local(json: bool) -> Result<()> { let entries = server::list_all_servers(); let running_count = entries.iter().filter(|e| e.running).count(); @@ -803,6 +846,7 @@ fn stop_server_global(name: &str, project: Option<&str>, json: bool) -> Result<( server::kill_server_by_pid(entry.pid)?; let out = output::ServerStopOutput { name: name.to_string(), + already_stopped: false, }; output::print_output(&out, json); Ok(()) @@ -904,6 +948,23 @@ fn stop_all_servers_global(json: bool) -> Result<()> { mod tests { use super::*; + #[test] + fn classify_stop_running_server_is_stopped() { + // Running takes precedence regardless of on-disk state. + assert_eq!(classify_stop(true, true), StopOutcome::Stop); + assert_eq!(classify_stop(true, false), StopOutcome::Stop); + } + + #[test] + fn classify_stop_existing_but_stopped_is_idempotent_noop() { + assert_eq!(classify_stop(false, true), StopOutcome::AlreadyStopped); + } + + #[test] + fn classify_stop_unknown_name_is_not_found() { + assert_eq!(classify_stop(false, false), StopOutcome::NotFound); + } + #[test] fn parse_postgres_install_spec_recognizes_at_and_colon() { assert_eq!(parse_postgres_install_spec("postgres@17"), Some("17")); diff --git a/crates/clickhousectl/src/local/output.rs b/crates/clickhousectl/src/local/output.rs index 7f13716..7120494 100644 --- a/crates/clickhousectl/src/local/output.rs +++ b/crates/clickhousectl/src/local/output.rs @@ -464,11 +464,17 @@ impl fmt::Display for PostgresDotenvOutput { #[derive(Debug, Clone, Serialize)] pub struct ServerStopOutput { pub name: String, + /// True when the server existed but was already stopped (idempotent noop). + pub already_stopped: bool, } impl fmt::Display for ServerStopOutput { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "Server '{}' stopped", self.name) + if self.already_stopped { + write!(f, "Server '{}' is already stopped", self.name) + } else { + write!(f, "Server '{}' stopped", self.name) + } } } @@ -771,11 +777,26 @@ mod tests { fn server_stop_json() { let output = ServerStopOutput { name: "default".to_string(), + already_stopped: false, }; let json: serde_json::Value = serde_json::from_str(&serde_json::to_string_pretty(&output).unwrap()).unwrap(); assert_eq!(json["name"], "default"); + assert_eq!(json["already_stopped"], false); + } + + #[test] + fn server_stop_already_stopped() { + let output = ServerStopOutput { + name: "default".to_string(), + already_stopped: true, + }; + assert_eq!(output.to_string(), "Server 'default' is already stopped"); + + let json: serde_json::Value = + serde_json::from_str(&serde_json::to_string_pretty(&output).unwrap()).unwrap(); + assert_eq!(json["already_stopped"], true); } #[test] @@ -1083,6 +1104,7 @@ mod tests { fn server_stop_display() { let output = ServerStopOutput { name: "default".to_string(), + already_stopped: false, }; assert_eq!(output.to_string(), "Server 'default' stopped"); } diff --git a/crates/clickhousectl/src/local/postgres.rs b/crates/clickhousectl/src/local/postgres.rs index e633163..1560bbb 100644 --- a/crates/clickhousectl/src/local/postgres.rs +++ b/crates/clickhousectl/src/local/postgres.rs @@ -408,6 +408,7 @@ async fn stop(name: &str, version: Option<&str>, json: bool) -> Result<()> { server::kill_server(&target.name)?; let out = output::ServerStopOutput { name: user_name_from_key(&target.name).to_string(), + already_stopped: false, }; output::print_output(&out, json); Ok(())