Skip to content

[16] Make local installs atomic and concurrency-safe - #545

Open
sdairs wants to merge 3 commits into
stack/459-bound-version-networkfrom
stack/456-atomic-local-installs
Open

[16] Make local installs atomic and concurrency-safe#545
sdairs wants to merge 3 commits into
stack/459-bound-version-networkfrom
stack/456-atomic-local-installs

Conversation

@sdairs

@sdairssdairs commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • give every install invocation UUID-owned staging protected by an owner lock and conservative 24-hour stale cleanup
  • serialize short target and master-sidecar commits across processes, atomically swap complete binaries, and durably replace sidecar state
  • add deterministic subprocess races for same and different versions, interruptions on both sides of replacement, and stale cleanup during a live install
  • keep post-commit master freshness recording best-effort and treat a version as installed only when its binary exists
  • intentionally serialize local remove with install commits and require durable sidecar invalidation before deletion, preventing a successful removal from leaving stale master state

Tests

  • cargo test -p clickhousectl version_manager::install::tests:: -- --test-threads=1 (repeated)
  • cargo test -p clickhousectl
  • cargo fmt --all -- --check
  • cargo clippy -p clickhousectl --all-targets -- -D warnings

Closes#456

@sdairs

Copy link
Copy Markdown
CollaboratorAuthor

Carry-over review from PR #502 (old PR for #456). This PR is strictly more complete than #502 (no unsafe, real stale-staging GC with owner-lock probing, interruption tested on both sides of the rename, power-loss-durable fsyncs, cross-platform sidecar invalidation, single commit-lock design). No sdairs review comments existed on #502 (its only review was an LGTM), so no carry-over concerns. Three items to address before merge:

  1. record_install failure now hard-fails a completed install. In commit_staged_install_locked (crates/clickhousectl/src/version_manager/install.rs:~225-235), the post-rename record_install uses ? propagation. Both the pre-existing code and Make local ClickHouse installs atomic and concurrency-safe #502 deliberately kept the post-commit sidecar write best-effort ("a sidecar write failure must not fail an otherwise-successful install"). A sidecar write error after the binary is safely committed now reports the install as failed even though the version is usable, and the user may retry with --force. Restore best-effort semantics for the post-commit record write, or document why it should fail the install.

  2. ensure_installed pre-check inconsistency.install_resolved was upgraded to check binary_path(...).exists(), but ensure_installed (install.rs:~245-247) still checks version_dir.exists(). Make local ClickHouse installs atomic and concurrency-safe #502 fixed both. A legacy empty version dir (from pre-fix partial installs) would be treated as installed by server start --version. Align ensure_installed to binary_path.

  3. Confirm the local remove scope expansion is intended. Taking the commit lock and invalidating sidecar records before remove_dir_all in local/mod.rs:~214-238 is beyond Make local ClickHouse installs atomic and concurrency-safe #456's literal criteria. It is small and same-class (concurrent remove vs install commit), defensible — but it changes remove semantics: a sidecar write error now blocks removal entirely (was best-effort). Confirm intentional.

Minor: the checkpoint: impl FnMut(...) -> Result<()> injection param is test-only and its Err arm is never exercised in production (stylistic, fine).

@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch from 7f68e8a to f445404CompareAugust 26, 2026 18:58
@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch 2 times, most recently from 4d6e1c8 to 8882a2eCompareAugust 26, 2026 19:03
@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch from 8882a2e to f53fe98CompareAugust 26, 2026 19:07
@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch from f53fe98 to aa8b340CompareAugust 26, 2026 19:10
@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch from aa8b340 to eba0288CompareAugust 26, 2026 19:12
Comment threadcrates/clickhousectl/src/version_manager/install.rs Outdated
@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch from eba0288 to ceb4711CompareAugust 26, 2026 19:20
@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch from ceb4711 to 7052c22CompareAugust 26, 2026 19:24
@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch from 7052c22 to 3918675CompareAugust 26, 2026 19:28
@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch 2 times, most recently from 0955366 to 47dbbc4CompareAugust 26, 2026 19:34
@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch from 47dbbc4 to 063a1cdCompareAugust 26, 2026 19:36

@cursorcursorBot 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 063a1cd. Configure here.

Comment threadcrates/clickhousectl/src/local/mod.rs
@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch 2 times, most recently from 0e4fe54 to 857ef90CompareAugust 26, 2026 20:30
@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch from 857ef90 to c6c89dbCompareAugust 26, 2026 20:35
@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch from c6c89db to 80b0c24CompareAugust 26, 2026 20:38
@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch from 333dcef to 95bf213CompareAugust 27, 2026 10:13
@sdairs
sdairsforce-pushed the stack/456-atomic-local-installs branch from 95bf213 to 69d3481CompareAugust 27, 2026 10:51
Sign up for freeto 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 ClickHouse installs atomic and concurrency-safe

1 participant

@sdairs