Skip to content

Make local Postgres startup transactional - #550

Open
sdairs wants to merge 1 commit into
stack/457-postgres-readinessfrom
stack/464-transactional-postgres-start
Open

Make local Postgres startup transactional#550
sdairs wants to merge 1 commit into
stack/457-postgres-readinessfrom
stack/464-transactional-postgres-start

Conversation

@sdairs

@sdairs sdairs commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • split fresh Postgres container creation from startup and wrap start, metadata, and readiness in one rollback boundary
  • remove the exact new container, startup metadata, and partial PGDATA while preserving resumed data
  • retain primary startup errors and append cleanup diagnostics when rollback is incomplete
  • document the fresh-start cleanup policy and add deterministic fake-Docker coverage

Tests

  • cargo test -p clickhousectl
  • cargo clippy -p clickhousectl --all-targets -- -D warnings
  • cargo fmt --all --check

Closes #464

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 284e021. Configure here.

Comment thread crates/clickhousectl/src/local/postgres.rs
@sdairs

sdairs commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Carry-over review from PR #501 (old PR for #464). This PR's single-rollback-boundary design is clean and simpler, and resume data preservation by construction is good. But it silently drops two fixes produced by #501's review cycle. Required:

  1. BLOCKER — shell-injection sink re-introduced and now reachable from this PR's new rollback path. On Make local Postgres startup transactional #501, macroscope flagged (Critical) that the rollback's remove_host_dir_blocking fallback sh -c "rm -rf /work/{basename}" executes shell metacharacters from a user-controlled server name and can delete sibling instances; sdairs confirmed the fix: "Fixed in 3fec2d1… The privileged fallback now invokes rm directly with argv (rm -rf -- /work/<basename>), so the user-controlled basename is never interpreted by sh -c. Added focused tests for normal cleanup and a metacharacter-heavy basename remaining one literal argument." That fix commit is not in this stack: crates/clickhousectl/src/local/docker.rs:~999-1002 on this branch still builds format!("rm -rf /work/{}", basename) and runs ["sh", "-c", cmd], and this PR's new rollback_fresh_postgres (postgres.rs:~429-432) feeds a user-controlled name into it. validate_server_name blocks only /, \, NUL, ., ..;, $(), backticks, spaces all pass. Port the argv-based rm fix and its two unit tests (privileged_remove_passes_metacharacters_as_one_argument, remove_host_dir_removes_normal_directory).

  2. Do not delete PGDATA/metadata when container removal fails. Here the three rollback steps are independent best-effort (postgres.rs:~421-432), so if remove_container fails, the instance dir and metadata are still deleted — then label-based recovery (recover_current_project_servers()recover_project_postgres_blocking) resurrects the orphaned labeled container as a managed instance over deleted data. Cursor Bugbot flagged this (Medium) here, and the test cleanup_failure_keeps_primary_start_error_and_adds_diagnostics currently pins the bad behavior (assert!(!fresh_instance_dir(...).exists()) after a failed remove). Make local Postgres startup transactional #501 deliberately retains data + recovery metadata in this case; adopt that policy and fix the test.

  3. Guard against deleting an instance dir that contained data before the attempt. Unconditional instance-dir deletion on the fresh path loses data that predates the attempt (e.g. user deleted the metadata json — which the README calls "non-destructive" — leaving the data dir). Port Make local Postgres startup transactional #501's fresh_instance_dir_is_disposable predicate (~20 lines); this serves issue Make local Postgres startup transactional #464's "existing data is preserved" acceptance criterion beyond just the resume path.

  4. Make remove_container 404-tolerant (Make local Postgres startup transactional #501 did) so removing an already-gone container doesn't add spurious "could not remove newly-created container" diagnostics.

  5. Fix the red local postgres edge cases job (inherited from Validate local Postgres start options #547's script-expectation drift, but confirm resolved before this merges).

@sdairs
sdairs force-pushed the stack/464-transactional-postgres-start branch 2 times, most recently from e48ba30 to d04a362 Compare August 26, 2026 19:01
@sdairs
sdairs force-pushed the stack/464-transactional-postgres-start branch 2 times, most recently from 1d4311a to 93b2f7d Compare August 26, 2026 19:07
@sdairs
sdairs force-pushed the stack/464-transactional-postgres-start branch from 93b2f7d to a8d49d0 Compare August 26, 2026 19:10
@sdairs
sdairs force-pushed the stack/464-transactional-postgres-start branch 2 times, most recently from 36ebf33 to 09cd717 Compare August 26, 2026 19:20
@sdairs
sdairs force-pushed the stack/464-transactional-postgres-start branch from 09cd717 to e44c257 Compare August 26, 2026 19:24
@sdairs
sdairs force-pushed the stack/464-transactional-postgres-start branch from e44c257 to 221dc87 Compare August 26, 2026 19:28
@sdairs
sdairs force-pushed the stack/464-transactional-postgres-start branch 2 times, most recently from 9959f0d to 4b9186c Compare August 26, 2026 19:34
@sdairs
sdairs force-pushed the stack/464-transactional-postgres-start branch from 4b9186c to aa7aae7 Compare August 26, 2026 19:36
@sdairs
sdairs force-pushed the stack/464-transactional-postgres-start branch 2 times, most recently from 2fcd381 to 73a341f Compare August 26, 2026 20:30
@sdairs
sdairs force-pushed the stack/464-transactional-postgres-start branch 2 times, most recently from 32654cd to 07b5e28 Compare August 26, 2026 20:38
@sdairs
sdairs force-pushed the stack/464-transactional-postgres-start branch from 07b5e28 to 1ae380f Compare August 26, 2026 20:41
@sdairs
sdairs force-pushed the stack/464-transactional-postgres-start branch from 1ae380f to 7e9bd04 Compare August 26, 2026 20:44
@sdairs
sdairs force-pushed the stack/464-transactional-postgres-start branch from 7e9bd04 to 577f702 Compare August 26, 2026 20:49
@sdairs
sdairs force-pushed the stack/464-transactional-postgres-start branch from 577f702 to 6b158d8 Compare August 26, 2026 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make local Postgres startup transactional

1 participant