From f52eb45734ae4d466088b82fdad13ec4ce36f4f8 Mon Sep 17 00:00:00 2001 From: sdairs Date: Wed, 26 Aug 2026 15:17:52 +0100 Subject: [PATCH 1/3] Make local Postgres startup transactional --- README.md | 2 +- crates/clickhousectl/src/local/cli.rs | 1 + crates/clickhousectl/src/local/docker.rs | 15 +- crates/clickhousectl/src/local/postgres.rs | 27 +- crates/clickhousectl/src/local/server.rs | 12 +- .../tests/local_postgres_readiness_test.rs | 347 ++++++++++++++++-- .../local_postgres_start_validation_test.rs | 1 + 7 files changed, 358 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index 2cd84903..4f792955 100644 --- a/README.md +++ b/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 partial PGDATA only when rollback completes; otherwise recovery metadata is retained so `local postgres remove` can finish cleanup safely. A failed resume stops the existing container but preserves its metadata and data. Containers are tagged with `clickhousectl.engine=postgres`, `clickhousectl.name=`, `clickhousectl.major=`, `clickhousectl.project=`, and `created_by=clickhousectl_` labels. `server list` recovers orphaned containers belonging to the current project via these labels, so deleting `.clickhouse/servers/-pg.json` is non-destructive — the next list/start rediscovers it. diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index 4911ee0f..072e305c 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/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 partial data are removed; resumed 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 diff --git a/crates/clickhousectl/src/local/docker.rs b/crates/clickhousectl/src/local/docker.rs index bccb5861..ff724511 100644 --- a/crates/clickhousectl/src/local/docker.rs +++ b/crates/clickhousectl/src/local/docker.rs @@ -350,10 +350,13 @@ pub struct PostgresRunOpts<'a> { pub extra_env: Vec, } -/// Create + start a Postgres container; return its ID. -pub async fn run_postgres(docker: &Docker, opts: PostgresRunOpts<'_>) -> Result { +/// 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 { use bollard::models::{ContainerCreateBody, HostConfig, PortBinding}; - use bollard::query_parameters::{CreateContainerOptionsBuilder, StartContainerOptions}; + use bollard::query_parameters::CreateContainerOptionsBuilder; let mut port_bindings: HashMap>> = 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::) - .await - .map_err(|e| Error::DockerError(e.to_string()))?; - Ok(created.id) } diff --git a/crates/clickhousectl/src/local/postgres.rs b/crates/clickhousectl/src/local/postgres.rs index e1c8ce47..9aab4988 100644 --- a/crates/clickhousectl/src/local/postgres.rs +++ b/crates/clickhousectl/src/local/postgres.rs @@ -372,7 +372,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 +385,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, + primary, ) .await); } diff --git a/crates/clickhousectl/src/local/server.rs b/crates/clickhousectl/src/local/server.rs index 85ce6b5b..a79e4e5c 100644 --- a/crates/clickhousectl/src/local/server.rs +++ b/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. diff --git a/crates/clickhousectl/tests/local_postgres_readiness_test.rs b/crates/clickhousectl/tests/local_postgres_readiness_test.rs index de7082b2..a1c82444 100644 --- a/crates/clickhousectl/tests/local_postgres_readiness_test.rs +++ b/crates/clickhousectl/tests/local_postgres_readiness_test.rs @@ -24,10 +24,12 @@ enum ContainerOutcome { struct DockerScenario { existing: bool, outcome: ContainerOutcome, + start_statuses: Vec, + remove_statuses: Vec, readiness_exit_codes: Vec, readiness_create_errors: usize, logs: Vec, - remove_fails: bool, + write_partial_data: bool, } #[derive(Clone, Debug)] @@ -44,7 +46,7 @@ struct FakeDocker { } impl FakeDocker { - fn start(socket_path: &Path, project: &Path, scenario: DockerScenario) -> Self { + fn start(socket_path: &Path, project_path: &Path, scenario: DockerScenario) -> Self { let listener = UnixListener::bind(socket_path).expect("bind fake Docker socket"); listener .set_nonblocking(true) @@ -53,18 +55,24 @@ impl FakeDocker { let requests = Arc::new(Mutex::new(Vec::new())); let thread_stop = Arc::clone(&stop); let thread_requests = Arc::clone(&requests); - let project = project.to_path_buf(); + let project = project_path.to_path_buf(); + let partial_data_path = + project_path.join(".clickhouse/servers/default-pg18/data/partial-init"); let thread = thread::spawn(move || { let DockerScenario { existing, outcome, + start_statuses, + remove_statuses, readiness_exit_codes, readiness_create_errors, logs, - remove_fails, + write_partial_data, } = scenario; let mut started = false; let mut next_exec = 0_usize; + let mut start_statuses: VecDeque = start_statuses.into(); + let mut remove_statuses: VecDeque = remove_statuses.into(); let mut readiness_create_errors = readiness_create_errors; let mut readiness_exit_codes: VecDeque = readiness_exit_codes.into(); let mut exec_exit_codes = HashMap::new(); @@ -112,16 +120,30 @@ impl FakeDocker { write_json(&mut stream, 201, r#"{"Id":"cleanup-id","Warnings":[]}"#); } else { assert!(!existing, "resumed start created a new container"); + started = false; write_json(&mut stream, 201, r#"{"Id":"pg-id","Warnings":[]}"#); } } ("POST", path) if path.starts_with("/containers/pg-id/start") => { - started = true; - let data_dir = project.join(".clickhouse/servers/default-pg18/data"); - std::fs::create_dir_all(&data_dir).expect("create simulated PGDATA"); - std::fs::write(data_dir.join("PG_VERSION"), "18") - .expect("write simulated PGDATA marker"); - write_response(&mut stream, 204, "application/json", b""); + let status = start_statuses.pop_front().unwrap_or(204); + if status == 204 { + started = true; + let data_dir = project.join(".clickhouse/servers/default-pg18/data"); + std::fs::create_dir_all(&data_dir).expect("create simulated PGDATA"); + std::fs::write(data_dir.join("PG_VERSION"), "18") + .expect("write simulated PGDATA marker"); + if write_partial_data { + std::fs::write(&partial_data_path, "partial PGDATA") + .expect("write partial PGDATA marker"); + } + write_response(&mut stream, 204, "application/json", b""); + } else { + write_json( + &mut stream, + status, + r#"{"message":"start failed by test"}"#, + ); + } } ("GET", path) if path.starts_with("/containers/pg-id/json") => { let running = started && matches!(outcome, ContainerOutcome::Running); @@ -187,14 +209,16 @@ impl FakeDocker { write_json(&mut stream, 200, r#"{"StatusCode":0}"#) } ("DELETE", path) if path.starts_with("/containers/pg-id?") => { - if remove_fails { + let status = remove_statuses.pop_front().unwrap_or(204); + if status == 204 { + started = false; + write_response(&mut stream, 204, "application/json", b"") + } else { write_json( &mut stream, - 500, - r#"{"message":"simulated cleanup failure"}"#, + status, + r#"{"message":"remove failed by test"}"#, ) - } else { - write_response(&mut stream, 204, "application/json", b"") } } _ => panic!("unexpected fake Docker request: {request:?}"), @@ -270,6 +294,7 @@ fn write_response(stream: &mut UnixStream, status: u16, content_type: &str, body 201 => "Created", 204 => "No Content", 404 => "Not Found", + 500 => "Internal Server Error", _ => "Response", }; let headers = format!( @@ -355,14 +380,35 @@ fn run_start( } let socket_path = home.path().join("docker.sock"); let docker = FakeDocker::start(&socket_path, project.path(), scenario); + let output = run_start_command( + home.path(), + project.path(), + &socket_path, + resumed, + telemetry_debug, + wait_timeout, + ); + let requests = docker.requests(); + drop(docker); + (output, requests, project) +} + +fn run_start_command( + home: &Path, + project: &Path, + socket_path: &Path, + resumed: bool, + telemetry_debug: bool, + wait_timeout: u16, +) -> Output { let port = reserve_port().to_string(); let wait_timeout = wait_timeout.to_string(); let mut command = Command::new(clickhousectl_binary()); command .env_clear() - .env("HOME", home.path()) + .env("HOME", home) .env("DOCKER_HOST", format!("unix://{}", socket_path.display())) - .current_dir(project.path()) + .current_dir(project) .args([ "local", "--json", @@ -381,10 +427,7 @@ fn run_start( } else { command.env("DO_NOT_TRACK", "1"); } - let output = command.output().expect("run clickhousectl"); - let requests = docker.requests(); - drop(docker); - (output, requests, project) + command.output().expect("run clickhousectl") } fn readiness_requests(requests: &[DockerRequest]) -> Vec<&DockerRequest> { @@ -400,10 +443,12 @@ fn fresh_start_waits_for_delayed_postgres_readiness_without_exposing_password() DockerScenario { existing: false, outcome: ContainerOutcome::Running, + start_statuses: vec![204], + remove_statuses: vec![], readiness_exit_codes: vec![1, 0], readiness_create_errors: 1, logs: vec![], - remove_fails: false, + write_partial_data: false, }, false, false, @@ -446,10 +491,12 @@ fn resumed_start_also_waits_for_postgres_readiness() { DockerScenario { existing: true, outcome: ContainerOutcome::Running, + start_statuses: vec![204], + remove_statuses: vec![], readiness_exit_codes: vec![1, 0], readiness_create_errors: 0, logs: vec![], - remove_fails: false, + write_partial_data: false, }, true, false, @@ -480,10 +527,12 @@ fn wall_clock_timeout_fails_and_rolls_back_fresh_data() { DockerScenario { existing: false, outcome: ContainerOutcome::Running, + start_statuses: vec![204], + remove_statuses: vec![204], readiness_exit_codes: vec![], readiness_create_errors: 0, logs: vec![], - remove_fails: false, + write_partial_data: false, }, false, false, @@ -525,10 +574,12 @@ fn immediate_exit_reports_bounded_logs_and_error_telemetry_without_setup_success DockerScenario { existing: false, outcome: ContainerOutcome::ImmediateExit, + start_statuses: vec![204], + remove_statuses: vec![204], readiness_exit_codes: vec![], readiness_create_errors: 0, logs, - remove_fails: false, + write_partial_data: false, }, false, true, @@ -582,10 +633,12 @@ fn failed_fresh_start_preserves_preexisting_data_and_recovery_metadata() { DockerScenario { existing: false, outcome: ContainerOutcome::ImmediateExit, + start_statuses: vec![204], + remove_statuses: vec![204], readiness_exit_codes: vec![], readiness_create_errors: 0, logs: vec![], - remove_fails: false, + write_partial_data: false, }, false, false, @@ -629,10 +682,12 @@ fn incomplete_container_cleanup_retains_pgdata_and_recovery_metadata() { DockerScenario { existing: false, outcome: ContainerOutcome::ImmediateExit, + start_statuses: vec![204], + remove_statuses: vec![500], readiness_exit_codes: vec![], readiness_create_errors: 0, logs: vec![], - remove_fails: true, + write_partial_data: false, }, false, false, @@ -642,7 +697,7 @@ fn incomplete_container_cleanup_retains_pgdata_and_recovery_metadata() { assert_eq!(output.status.code(), Some(1)); let stderr = String::from_utf8_lossy(&output.stderr); - assert!(stderr.contains("simulated cleanup failure"), "{stderr}"); + assert!(stderr.contains("remove failed by test"), "{stderr}"); assert!(stderr.contains("recovery metadata retained"), "{stderr}"); assert!( project @@ -666,3 +721,239 @@ fn incomplete_container_cleanup_retains_pgdata_and_recovery_metadata() { 1 ); } + +fn fresh_instance_dir(project: &Path) -> PathBuf { + project.join(".clickhouse/servers/default-pg18") +} + +fn metadata_path(project: &Path) -> PathBuf { + project.join(".clickhouse/servers/default-pg18.json") +} + +fn request_index(requests: &[DockerRequest], method: &str, path_fragment: &str) -> usize { + requests + .iter() + .position(|request| request.method == method && request.path.contains(path_fragment)) + .unwrap_or_else(|| panic!("missing {method} request containing {path_fragment}")) +} + +#[test] +fn create_success_start_failure_rolls_back_exact_container_and_fresh_data() { + let home = tempfile::tempdir().expect("create home tempdir"); + let project = tempfile::tempdir().expect("create project tempdir"); + let socket_path = home.path().join("docker.sock"); + let docker = FakeDocker::start( + &socket_path, + project.path(), + DockerScenario { + existing: false, + outcome: ContainerOutcome::Running, + start_statuses: vec![500], + remove_statuses: vec![204], + readiness_exit_codes: vec![], + readiness_create_errors: 0, + logs: vec![], + write_partial_data: false, + }, + ); + + let output = run_start_command(home.path(), project.path(), &socket_path, false, false, 2); + let requests = docker.requests(); + + assert_eq!(output.status.code(), Some(1)); + assert!(String::from_utf8_lossy(&output.stderr).contains("start failed by test")); + let create = request_index(&requests, "POST", "/containers/create?"); + let start = request_index(&requests, "POST", "/containers/pg-id/start"); + let remove = request_index(&requests, "DELETE", "/containers/pg-id?"); + assert!(create < start && start < remove); + assert!(!fresh_instance_dir(project.path()).exists()); + assert!(!metadata_path(project.path()).exists()); +} + +#[test] +fn initialization_timeout_removes_partial_pgdata() { + let home = tempfile::tempdir().expect("create home tempdir"); + let project = tempfile::tempdir().expect("create project tempdir"); + let socket_path = home.path().join("docker.sock"); + let docker = FakeDocker::start( + &socket_path, + project.path(), + DockerScenario { + existing: false, + outcome: ContainerOutcome::Running, + start_statuses: vec![204], + remove_statuses: vec![204], + readiness_exit_codes: vec![1; 100], + readiness_create_errors: 0, + logs: vec!["database system is starting up".to_string()], + write_partial_data: true, + }, + ); + + let output = run_start_command(home.path(), project.path(), &socket_path, false, false, 1); + let requests = docker.requests(); + + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("did not become ready within 1 seconds")); + assert!(stderr.contains("database system is starting up")); + request_index(&requests, "DELETE", "/containers/pg-id?"); + assert!(!fresh_instance_dir(project.path()).exists()); + assert!(!metadata_path(project.path()).exists()); +} + +#[test] +fn metadata_failure_uses_the_fresh_start_rollback() { + use std::os::unix::fs::symlink; + + let home = tempfile::tempdir().expect("create home tempdir"); + let project = tempfile::tempdir().expect("create project tempdir"); + let servers = project.path().join(".clickhouse/servers"); + let metadata_target = project.path().join("metadata-target-directory"); + std::fs::create_dir_all(&servers).expect("create servers directory"); + std::fs::create_dir(&metadata_target).expect("create metadata failure target"); + symlink(&metadata_target, metadata_path(project.path())).expect("create metadata symlink"); + + let socket_path = home.path().join("docker.sock"); + let docker = FakeDocker::start( + &socket_path, + project.path(), + DockerScenario { + existing: false, + outcome: ContainerOutcome::Running, + start_statuses: vec![204], + remove_statuses: vec![204], + readiness_exit_codes: vec![], + readiness_create_errors: 0, + logs: vec![], + write_partial_data: true, + }, + ); + + let output = run_start_command(home.path(), project.path(), &socket_path, false, false, 2); + let requests = docker.requests(); + + assert_eq!(output.status.code(), Some(1)); + request_index(&requests, "DELETE", "/containers/pg-id?"); + assert!(readiness_requests(&requests).is_empty()); + assert!(!fresh_instance_dir(project.path()).exists()); + assert!(!metadata_path(project.path()).exists()); +} + +#[test] +fn retry_after_rolled_back_start_failure_succeeds_cleanly() { + let home = tempfile::tempdir().expect("create home tempdir"); + let project = tempfile::tempdir().expect("create project tempdir"); + let socket_path = home.path().join("docker.sock"); + let docker = FakeDocker::start( + &socket_path, + project.path(), + DockerScenario { + existing: false, + outcome: ContainerOutcome::Running, + start_statuses: vec![500, 204], + remove_statuses: vec![204], + readiness_exit_codes: vec![0], + readiness_create_errors: 0, + logs: vec![], + write_partial_data: false, + }, + ); + + let first = run_start_command(home.path(), project.path(), &socket_path, false, false, 2); + assert_eq!(first.status.code(), Some(1)); + assert!(!fresh_instance_dir(project.path()).exists()); + + let second = run_start_command(home.path(), project.path(), &socket_path, false, false, 2); + let requests = docker.requests(); + + assert!( + second.status.success(), + "stderr: {}", + String::from_utf8_lossy(&second.stderr) + ); + assert_eq!( + requests + .iter() + .filter(|request| request.method == "POST" + && request.path.starts_with("/containers/create?") + && request.body.contains(r#""Image":"postgres:18""#)) + .count(), + 2 + ); + assert!(fresh_instance_dir(project.path()).exists()); + assert!(metadata_path(project.path()).is_file()); +} + +#[test] +fn resume_failure_preserves_existing_container_metadata_and_data() { + let home = tempfile::tempdir().expect("create home tempdir"); + let project = tempfile::tempdir().expect("create project tempdir"); + write_resumed_server(project.path()); + let marker = fresh_instance_dir(project.path()).join("data/user-data"); + std::fs::create_dir_all(marker.parent().unwrap()).expect("create resumed data directory"); + std::fs::write(&marker, "keep me").expect("write resumed data marker"); + + let socket_path = home.path().join("docker.sock"); + let docker = FakeDocker::start( + &socket_path, + project.path(), + DockerScenario { + existing: true, + outcome: ContainerOutcome::ImmediateExit, + start_statuses: vec![204], + remove_statuses: vec![], + readiness_exit_codes: vec![], + readiness_create_errors: 0, + logs: vec!["resume failed".to_string()], + write_partial_data: false, + }, + ); + + let output = run_start_command(home.path(), project.path(), &socket_path, true, false, 2); + let requests = docker.requests(); + + assert_eq!(output.status.code(), Some(1)); + assert!(marker.is_file()); + assert!(metadata_path(project.path()).is_file()); + assert!(requests.iter().any(|request| { + request.method == "POST" && request.path.starts_with("/containers/pg-id/stop?") + })); + assert!(!requests.iter().any(|request| request.method == "DELETE")); +} + +#[test] +fn cleanup_failure_keeps_primary_start_error_and_adds_diagnostics() { + let home = tempfile::tempdir().expect("create home tempdir"); + let project = tempfile::tempdir().expect("create project tempdir"); + let socket_path = home.path().join("docker.sock"); + let docker = FakeDocker::start( + &socket_path, + project.path(), + DockerScenario { + existing: false, + outcome: ContainerOutcome::Running, + start_statuses: vec![500], + remove_statuses: vec![500], + readiness_exit_codes: vec![], + readiness_create_errors: 0, + logs: vec![], + write_partial_data: false, + }, + ); + + let output = run_start_command(home.path(), project.path(), &socket_path, false, false, 2); + let requests = docker.requests(); + + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + let primary = stderr.find("start failed by test").expect("primary error"); + let rollback = stderr + .find("Postgres startup rollback incomplete") + .expect("rollback diagnostics"); + let cleanup = stderr.find("remove failed by test").expect("cleanup error"); + assert!(primary < rollback && rollback < cleanup, "{stderr}"); + request_index(&requests, "DELETE", "/containers/pg-id?"); + assert!(fresh_instance_dir(project.path()).exists()); + assert!(metadata_path(project.path()).is_file()); +} diff --git a/crates/clickhousectl/tests/local_postgres_start_validation_test.rs b/crates/clickhousectl/tests/local_postgres_start_validation_test.rs index 6b5e09cc..71c940ba 100644 --- a/crates/clickhousectl/tests/local_postgres_start_validation_test.rs +++ b/crates/clickhousectl/tests/local_postgres_start_validation_test.rs @@ -359,6 +359,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 partial data are removed; resumed 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 From f3336580deacc00650b54b7a4bd63be0c5443f1e Mon Sep 17 00:00:00 2001 From: sdairs Date: Thu, 27 Aug 2026 07:40:22 +0100 Subject: [PATCH 2/3] Address Postgres rollback review --- README.md | 2 +- crates/clickhousectl/src/local/cli.rs | 2 +- crates/clickhousectl/src/local/docker.rs | 56 +++++++++++++++--- crates/clickhousectl/src/local/postgres.rs | 57 +++++++++++++++++-- .../tests/local_postgres_readiness_test.rs | 41 ++++++++++++- .../local_postgres_start_validation_test.rs | 2 +- 6 files changed, 141 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 4f792955..dece1393 100644 --- a/README.md +++ b/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. A failed fresh startup removes the newly created container, metadata, and partial PGDATA only when rollback completes; otherwise recovery metadata is retained so `local postgres remove` can finish cleanup safely. A failed resume stops the existing container but preserves its metadata and data. +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=`, `clickhousectl.major=`, `clickhousectl.project=`, and `created_by=clickhousectl_` labels. `server list` recovers orphaned containers belonging to the current project via these labels, so deleting `.clickhouse/servers/-pg.json` is non-destructive — the next list/start rediscovers it. diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index 072e305c..ec04a857 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -463,7 +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 partial data are removed; resumed data is preserved. + 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 diff --git a/crates/clickhousectl/src/local/docker.rs b/crates/clickhousectl/src/local/docker.rs index ff724511..67c6dfb0 100644 --- a/crates/clickhousectl/src/local/docker.rs +++ b/crates/clickhousectl/src/local/docker.rs @@ -575,15 +575,21 @@ pub async fn stop_container(docker: &Docker, id: &str) -> Result<()> { } pub async fn remove_container(docker: &Docker, id: &str) -> Result<()> { + use bollard::errors::Error as BErr; use bollard::query_parameters::RemoveContainerOptionsBuilder; - docker + match docker .remove_container( id, Some(RemoveContainerOptionsBuilder::default().force(true).build()), ) .await - .map_err(|e| Error::DockerError(e.to_string()))?; - Ok(()) + { + Ok(()) + | Err(BErr::DockerResponseServerError { + status_code: 404, .. + }) => Ok(()), + Err(e) => Err(Error::DockerError(e.to_string())), + } } pub async fn container_logs_tail( @@ -1010,12 +1016,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), @@ -1046,6 +1047,15 @@ pub fn remove_host_dir_blocking(host_path: &std::path::Path) -> Result<()> { Ok(()) } +fn privileged_remove_command(basename: &str) -> Vec { + 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 { @@ -1336,6 +1346,34 @@ 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 log_tail_is_bounded_by_bytes() { let mut buffer = Vec::new(); diff --git a/crates/clickhousectl/src/local/postgres.rs b/crates/clickhousectl/src/local/postgres.rs index 9aab4988..a71dfd94 100644 --- a/crates/clickhousectl/src/local/postgres.rs +++ b/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 @@ -407,7 +410,7 @@ async fn start( &docker, &container_id, &info, - created_instance_dir, + remove_fresh_data_on_failure, primary, ) .await); @@ -426,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); @@ -447,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(()) => { @@ -466,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}", @@ -1362,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(); diff --git a/crates/clickhousectl/tests/local_postgres_readiness_test.rs b/crates/clickhousectl/tests/local_postgres_readiness_test.rs index a1c82444..ce109203 100644 --- a/crates/clickhousectl/tests/local_postgres_readiness_test.rs +++ b/crates/clickhousectl/tests/local_postgres_readiness_test.rs @@ -11,6 +11,8 @@ use std::sync::{Arc, Mutex}; use std::thread::{self, JoinHandle}; use std::time::Duration; +static START_COMMAND_LOCK: Mutex<()> = Mutex::new(()); + fn clickhousectl_binary() -> PathBuf { PathBuf::from(env!("CARGO_BIN_EXE_clickhousectl")) } @@ -401,6 +403,9 @@ fn run_start_command( telemetry_debug: bool, wait_timeout: u16, ) -> Output { + let _guard = START_COMMAND_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); let port = reserve_port().to_string(); let wait_timeout = wait_timeout.to_string(); let mut command = Command::new(clickhousectl_binary()); @@ -649,7 +654,7 @@ fn failed_fresh_start_preserves_preexisting_data_and_recovery_metadata() { assert_eq!(output.status.code(), Some(1)); let stderr = String::from_utf8_lossy(&output.stderr); assert!( - stderr.contains("directory existed before this start attempt"), + stderr.contains("directory contained data before this start attempt"), "{stderr}" ); assert!(stderr.contains("recovery metadata retained"), "{stderr}"); @@ -770,6 +775,40 @@ fn create_success_start_failure_rolls_back_exact_container_and_fresh_data() { assert!(!metadata_path(project.path()).exists()); } +#[test] +fn already_gone_container_is_a_successful_rollback() { + let home = tempfile::tempdir().expect("create home tempdir"); + let project = tempfile::tempdir().expect("create project tempdir"); + let socket_path = home.path().join("docker.sock"); + let docker = FakeDocker::start( + &socket_path, + project.path(), + DockerScenario { + existing: false, + outcome: ContainerOutcome::Running, + start_statuses: vec![500], + remove_statuses: vec![404], + readiness_exit_codes: vec![], + readiness_create_errors: 0, + logs: vec![], + write_partial_data: false, + }, + ); + + let output = run_start_command(home.path(), project.path(), &socket_path, false, false, 2); + + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("start failed by test"), "{stderr}"); + assert!( + !stderr.contains("Postgres startup rollback incomplete"), + "{stderr}" + ); + assert!(!fresh_instance_dir(project.path()).exists()); + assert!(!metadata_path(project.path()).exists()); + request_index(&docker.requests(), "DELETE", "/containers/pg-id?"); +} + #[test] fn initialization_timeout_removes_partial_pgdata() { let home = tempfile::tempdir().expect("create home tempdir"); diff --git a/crates/clickhousectl/tests/local_postgres_start_validation_test.rs b/crates/clickhousectl/tests/local_postgres_start_validation_test.rs index 71c940ba..9ffcf5de 100644 --- a/crates/clickhousectl/tests/local_postgres_start_validation_test.rs +++ b/crates/clickhousectl/tests/local_postgres_start_validation_test.rs @@ -359,7 +359,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 partial data are removed; resumed data is preserved. + 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 From 7004ae47ec22273e4fdf7b36d1ed619ad758186a Mon Sep 17 00:00:00 2001 From: sdairs Date: Thu, 27 Aug 2026 07:42:40 +0100 Subject: [PATCH 3/3] Test missing container removal directly --- crates/clickhousectl/src/local/docker.rs | 33 +++++++++++++----- .../tests/local_postgres_readiness_test.rs | 34 ------------------- 2 files changed, 24 insertions(+), 43 deletions(-) diff --git a/crates/clickhousectl/src/local/docker.rs b/crates/clickhousectl/src/local/docker.rs index 67c6dfb0..fe9b5653 100644 --- a/crates/clickhousectl/src/local/docker.rs +++ b/crates/clickhousectl/src/local/docker.rs @@ -575,17 +575,21 @@ pub async fn stop_container(docker: &Docker, id: &str) -> Result<()> { } pub async fn remove_container(docker: &Docker, id: &str) -> Result<()> { - use bollard::errors::Error as BErr; use bollard::query_parameters::RemoveContainerOptionsBuilder; - match docker - .remove_container( - id, - Some(RemoveContainerOptionsBuilder::default().force(true).build()), - ) - .await - { + 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(BErr::DockerResponseServerError { + | Err(BollardError::DockerResponseServerError { status_code: 404, .. }) => Ok(()), Err(e) => Err(Error::DockerError(e.to_string())), @@ -1374,6 +1378,17 @@ mod tests { ); } + #[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(); diff --git a/crates/clickhousectl/tests/local_postgres_readiness_test.rs b/crates/clickhousectl/tests/local_postgres_readiness_test.rs index ce109203..88e25e17 100644 --- a/crates/clickhousectl/tests/local_postgres_readiness_test.rs +++ b/crates/clickhousectl/tests/local_postgres_readiness_test.rs @@ -775,40 +775,6 @@ fn create_success_start_failure_rolls_back_exact_container_and_fresh_data() { assert!(!metadata_path(project.path()).exists()); } -#[test] -fn already_gone_container_is_a_successful_rollback() { - let home = tempfile::tempdir().expect("create home tempdir"); - let project = tempfile::tempdir().expect("create project tempdir"); - let socket_path = home.path().join("docker.sock"); - let docker = FakeDocker::start( - &socket_path, - project.path(), - DockerScenario { - existing: false, - outcome: ContainerOutcome::Running, - start_statuses: vec![500], - remove_statuses: vec![404], - readiness_exit_codes: vec![], - readiness_create_errors: 0, - logs: vec![], - write_partial_data: false, - }, - ); - - let output = run_start_command(home.path(), project.path(), &socket_path, false, false, 2); - - assert_eq!(output.status.code(), Some(1)); - let stderr = String::from_utf8_lossy(&output.stderr); - assert!(stderr.contains("start failed by test"), "{stderr}"); - assert!( - !stderr.contains("Postgres startup rollback incomplete"), - "{stderr}" - ); - assert!(!fresh_instance_dir(project.path()).exists()); - assert!(!metadata_path(project.path()).exists()); - request_index(&docker.requests(), "DELETE", "/containers/pg-id?"); -} - #[test] fn initialization_timeout_removes_partial_pgdata() { let home = tempfile::tempdir().expect("create home tempdir");