Make local Postgres startup transactional

README.md

@@ -342,7 +342,7 @@ The Postgres `dotenv` command includes the generated password. Do not commit its
 
 `local postgres start --name dev` (no `--version`) resumes the existing instance when there's exactly one for that name; if multiple majors share the name, the command exits and asks you to pass `--version`. Stop preserves the container and metadata so the next start resumes it; only `remove` tears down the container and deletes the data directory. The unified `local server stop-all` stops both ClickHouse and Postgres instances in the current project; the dedicated `local postgres stop-all` remains available when only Postgres should be stopped.
 
-Fresh and resumed starts wait until `pg_isready` reports that PostgreSQL is accepting connections inside the container. The readiness timeout defaults to 60 seconds and can be set from 1 to 600 seconds with `--wait-timeout`. A timeout or early container exit fails the command and prints a bounded tail of the container logs instead of connection credentials. After a failed fresh start, data created by that attempt is removed only when rollback completes; otherwise recovery metadata is retained so `local postgres remove` can finish cleanup safely.
+Fresh and resumed starts wait until `pg_isready` reports that PostgreSQL is accepting connections inside the container. The readiness timeout defaults to 60 seconds and can be set from 1 to 600 seconds with `--wait-timeout`. A timeout or early container exit fails the command and prints a bounded tail of the container logs instead of connection credentials. A failed fresh startup removes the newly created container, metadata, and PGDATA created by that attempt only when rollback completes. Pre-existing PGDATA is preserved, and recovery metadata is retained whenever cleanup is incomplete. A failed resume stops the existing container but preserves its metadata and data.
 
 Containers are tagged with `clickhousectl.engine=postgres`, `clickhousectl.name=<name>`, `clickhousectl.major=<major>`, `clickhousectl.project=<cwd>`, and `created_by=clickhousectl_<version>` labels. `server list` recovers orphaned containers belonging to the current project via these labels, so deleting `.clickhouse/servers/<name>-pg<major>.json` is non-destructive — the next list/start rediscovers it.
 

crates/clickhousectl/src/local/cli.rs

@@ -463,6 +463,7 @@ CONTEXT FOR AGENTS:
   Defaults to 18. Image is pulled if not already present locally.
   When --port is omitted, port 5432 is used if free or another free port is auto-selected.
   An explicitly requested port is rejected if it is occupied.
+  If a fresh startup fails, its new container and attempt-created data are removed; existing data is preserved.
   A random POSTGRES_PASSWORD is generated unless --password or `-e POSTGRES_PASSWORD=...` is given.
   POSTGRES_USER, POSTGRES_DB, and PGDATA are reserved; use --user/--database for the first two.
   `-e POSTGRES_PASSWORD=...` remains a compatibility alternative to --password, but the two cannot

crates/clickhousectl/src/local/docker.rs

@@ -350,10 +350,13 @@ pub struct PostgresRunOpts<'a> {
     pub extra_env: Vec<String>,
 }
 
-/// Create + start a Postgres container; return its ID.
-pub async fn run_postgres(docker: &Docker, opts: PostgresRunOpts<'_>) -> Result<String> {
+/// Create a Postgres container without starting it; return its ID.
+///
+/// Keeping creation separate gives the caller the exact container ID needed
+/// to roll back every later startup step.
+pub async fn create_postgres(docker: &Docker, opts: PostgresRunOpts<'_>) -> Result<String> {
     use bollard::models::{ContainerCreateBody, HostConfig, PortBinding};
-    use bollard::query_parameters::{CreateContainerOptionsBuilder, StartContainerOptions};
+    use bollard::query_parameters::CreateContainerOptionsBuilder;
 
     let mut port_bindings: HashMap<String, Option<Vec<PortBinding>>> = HashMap::new();
     port_bindings.insert(
@@ -413,12 +416,6 @@ pub async fn run_postgres(docker: &Docker, opts: PostgresRunOpts<'_>) -> Result<
         .create_container(Some(create_opts), container_config)
         .await
         .map_err(|e| Error::DockerError(e.to_string()))?;
-
-    docker
-        .start_container(&created.id, None::<StartContainerOptions>)
-        .await
-        .map_err(|e| Error::DockerError(e.to_string()))?;
-
     Ok(created.id)
 }
 
@@ -579,14 +576,24 @@ pub async fn stop_container(docker: &Docker, id: &str) -> Result<()> {
 
 pub async fn remove_container(docker: &Docker, id: &str) -> Result<()> {
     use bollard::query_parameters::RemoveContainerOptionsBuilder;
-    docker
-        .remove_container(
-            id,
-            Some(RemoveContainerOptionsBuilder::default().force(true).build()),
-        )
-        .await
-        .map_err(|e| Error::DockerError(e.to_string()))?;
-    Ok(())
+    remove_container_result(
+        docker
+            .remove_container(
+                id,
+                Some(RemoveContainerOptionsBuilder::default().force(true).build()),
+            )
+            .await,
+    )
+}
+
+fn remove_container_result(result: std::result::Result<(), BollardError>) -> Result<()> {
+    match result {
+        Ok(())
+        | Err(BollardError::DockerResponseServerError {
+            status_code: 404, ..
+        }) => Ok(()),
+        Err(e) => Err(Error::DockerError(e.to_string())),
+    }
 }
 
 pub async fn container_logs_tail(
@@ -1013,12 +1020,7 @@ pub fn remove_host_dir_blocking(host_path: &std::path::Path) -> Result<()> {
         let bind = format!("{}:/work", parent_str);
         let cfg = ContainerCreateBody {
             image: Some("alpine:latest".into()),
-            cmd: Some(vec![
-                "rm".into(),
-                "-rf".into(),
-                "--".into(),
-                format!("/work/{basename}"),
-            ]),
+            cmd: Some(privileged_remove_command(&basename)),
             host_config: Some(HostConfig {
                 binds: Some(vec![bind]),
                 auto_remove: Some(true),
@@ -1049,6 +1051,15 @@ pub fn remove_host_dir_blocking(host_path: &std::path::Path) -> Result<()> {
     Ok(())
 }
 
+fn privileged_remove_command(basename: &str) -> Vec<String> {
+    vec![
+        "rm".into(),
+        "-rf".into(),
+        "--".into(),
+        format!("/work/{basename}"),
+    ]
+}
+
 pub fn stop_and_remove_blocking(id: &str) -> Result<()> {
     let id = id.to_string();
     block_on(async move {
@@ -1339,6 +1350,45 @@ mod tests {
         );
     }
 
+    #[cfg(not(target_os = "macos"))]
+    #[test]
+    fn remove_host_dir_removes_normal_directory() {
+        let tempdir = tempfile::tempdir().expect("create cleanup tempdir");
+        let directory = tempdir.path().join("normal-pg18");
+        std::fs::create_dir_all(directory.join("data")).expect("create data directory");
+        std::fs::write(directory.join("data/PG_VERSION"), "18").expect("write data file");
+
+        remove_host_dir_blocking(&directory).expect("remove host directory");
+
+        assert!(!directory.exists());
+    }
+
+    #[test]
+    fn privileged_remove_passes_metacharacters_as_one_argument() {
+        let basename = "db; touch injected; $(whoami) *";
+
+        assert_eq!(
+            privileged_remove_command(basename),
+            vec![
+                "rm".to_string(),
+                "-rf".to_string(),
+                "--".to_string(),
+                format!("/work/{basename}"),
+            ]
+        );
+    }
+
+    #[test]
+    fn missing_container_is_already_removed() {
+        assert!(
+            remove_container_result(Err(BollardError::DockerResponseServerError {
+                status_code: 404,
+                message: "No such container".to_string(),
+            }))
+            .is_ok()
+        );
+    }
+
     #[test]
     fn log_tail_is_bounded_by_bytes() {
         let mut buffer = Vec::new();

crates/clickhousectl/src/local/postgres.rs

@@ -12,6 +12,7 @@ use crate::local::server::{self, Engine, ServerInfo};
 use rand::distr::{Alphanumeric, SampleString};
 use std::collections::HashSet;
 use std::future::Future;
+use std::path::Path;
 use std::process::Command;
 use std::time::Duration;
 
@@ -345,7 +346,9 @@ async fn start(
         docker::pull_image(&docker, tag, json).await?;
     }
 
-    let created_instance_dir = server::ensure_pg_data_dir(&user_name, &major)?;
+    let instance_dir = server::servers_dir_join(&key);
+    let remove_fresh_data_on_failure = fresh_instance_dir_is_disposable(&instance_dir);
+    server::ensure_pg_data_dir(&user_name, &major)?;
     let data_dir = server::pg_data_dir(&user_name, &major);
 
     // Defensive cleanup of any unmanaged container colliding on our chosen
@@ -372,7 +375,7 @@ async fn start(
         extra_env,
     };
 
-    let container_id = docker::run_postgres(&docker, opts).await?;
+    let container_id = docker::create_postgres(&docker, opts).await?;
 
     let info = ServerInfo {
         name: key.clone(),
@@ -385,19 +388,30 @@ async fn start(
         engine: Engine::Postgres,
         container_id: Some(container_id.clone()),
     };
-    server::save_server_info(&info)?;
+    let startup_result = async {
+        docker::start_existing(&docker, &container_id).await?;
+        server::save_server_info(&info)?;
+        if let Err(failure) = wait_for_postgres_ready(&docker, &container_id, wait_timeout).await {
+            return Err(postgres_readiness_error(
+                &docker,
+                &container_id,
+                &user_name,
+                wait_timeout,
+                failure,
+            )
+            .await);
+        }
+        Ok(())
+    }
+    .await;
 
-    if let Err(failure) = wait_for_postgres_ready(&docker, &container_id, wait_timeout).await {
-        let error =
-            postgres_readiness_error(&docker, &container_id, &user_name, wait_timeout, failure)
-                .await;
-        let _ = docker::stop_container(&docker, &container_id).await;
+    if let Err(primary) = startup_result {
         return Err(rollback_failed_fresh_start(
             &docker,
             &container_id,
             &info,
-            created_instance_dir,
-            error,
+            remove_fresh_data_on_failure,
+            primary,
         )
         .await);
     }
@@ -415,11 +429,36 @@ async fn start(
     Ok(())
 }
 
+/// A fresh attempt owns an absent or empty instance directory, including one
+/// containing only an empty `data/` from an earlier pre-container step.
+fn fresh_instance_dir_is_disposable(path: &Path) -> bool {
+    let mut entries = match std::fs::read_dir(path) {
+        Ok(entries) => entries,
+        Err(error) if error.kind() == std::io::ErrorKind::NotFound => return true,
+        Err(_) => return false,
+    };
+    let entry = match entries.next() {
+        None => return true,
+        Some(Ok(entry)) => entry,
+        Some(Err(_)) => return false,
+    };
+    if entries.next().is_some()
+        || entry.file_name() != "data"
+        || !entry.file_type().is_ok_and(|file_type| file_type.is_dir())
+    {
+        return false;
+    }
+    match std::fs::read_dir(entry.path()) {
+        Ok(mut data_entries) => data_entries.next().is_none(),
+        Err(_) => false,
+    }
+}
+
 async fn rollback_failed_fresh_start(
     docker: &bollard::Docker,
     container_id: &str,
     info: &ServerInfo,
-    created_instance_dir: bool,
+    remove_fresh_data_on_failure: bool,
     primary: Error,
 ) -> Error {
     let instance_dir = server::servers_dir_join(&info.name);
@@ -436,7 +475,7 @@ async fn rollback_failed_fresh_start(
         }
     };
 
-    let instance_removed = if created_instance_dir && container_removed {
+    let instance_removed = if remove_fresh_data_on_failure && container_removed {
         match docker::remove_host_dir_blocking(&instance_dir) {
             Ok(()) if !instance_dir.exists() => true,
             Ok(()) => {
@@ -455,10 +494,10 @@ async fn rollback_failed_fresh_start(
             }
         }
     } else {
-        let reason = if created_instance_dir {
+        let reason = if remove_fresh_data_on_failure {
             "the container could not be removed"
         } else {
-            "the directory existed before this start attempt"
+            "the directory contained data before this start attempt"
         };
         diagnostics.push(format!(
             "retained Postgres data '{}' because {reason}",
@@ -1351,6 +1390,23 @@ mod tests {
         assert!(matches!(err, Error::Postgres(msg) if msg.contains("--port 0")));
     }
 
+    #[test]
+    fn fresh_data_cleanup_ownership_is_conservative() {
+        let tempdir = tempfile::tempdir().expect("create policy tempdir");
+        let instance_dir = tempdir.path().join("policy-pg18");
+        assert!(fresh_instance_dir_is_disposable(&instance_dir));
+
+        std::fs::create_dir(&instance_dir).expect("create empty instance dir");
+        assert!(fresh_instance_dir_is_disposable(&instance_dir));
+
+        let data_dir = instance_dir.join("data");
+        std::fs::create_dir(&data_dir).expect("create empty data dir");
+        assert!(fresh_instance_dir_is_disposable(&instance_dir));
+
+        std::fs::write(data_dir.join("PG_VERSION"), "existing").expect("write existing PGDATA");
+        assert!(!fresh_instance_dir_is_disposable(&instance_dir));
+    }
+
     #[test]
     fn parse_pg_port_rejects_zero_with_actionable_error() {
         let err = parse_pg_port_arg("0").unwrap_err();

crates/clickhousectl/src/local/server.rs

@@ -191,7 +191,17 @@ pub fn save_server_info(info: &ServerInfo) -> Result<()> {
 
 /// Remove a server's metadata file.
 pub fn remove_server_info(name: &str) {
-    let _ = std::fs::remove_file(server_meta_path(name));
+    let _ = try_remove_server_info(name);
+}
+
+/// Remove a server's metadata file while retaining cleanup errors for callers
+/// that are rolling back a transaction.
+pub fn try_remove_server_info(name: &str) -> Result<()> {
+    match std::fs::remove_file(server_meta_path(name)) {
+        Ok(()) => Ok(()),
+        Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()),
+        Err(error) => Err(error.into()),
+    }
 }
 
 /// Mark a ClickHouse server as stopped without discarding its metadata.