Skip to content

Make local server metadata writes atomic and concurrency-safe - #554

Open
sdairs wants to merge 1 commit into
stack/470-client-query-multiplicityfrom
stack/472-atomic-server-metadata
Open

Make local server metadata writes atomic and concurrency-safe#554
sdairs wants to merge 1 commit into
stack/470-client-query-multiplicityfrom
stack/472-atomic-server-metadata

Conversation

@sdairs

Copy link
Copy Markdown
Collaborator

Summary

  • write server metadata through unique sibling temporary files, sync the file, atomically rename it, and sync the containing directory
  • serialize ClickHouse and Postgres lifecycle, recovery, and stale-state transitions with a project-wide cross-process lock
  • preserve actionable metadata read, UTF-8, JSON, and durable-write failures without treating corrupt entries as absent
  • add deterministic corruption, interrupted-write, stale-PID, restart-normalization, permission, and concurrent lifecycle coverage

Tests

  • cargo test -p clickhousectl -- --test-threads=1
  • focused concurrent start/client/stop test, repeated 3 times
  • focused restart-during-normalization test, repeated 3 times
  • cargo build -p clickhousectl
  • cargo clippy -p clickhousectl --all-targets -- -D warnings
  • cargo fmt --all -- --check

Closes#472

Comment threadcrates/clickhousectl/src/local/postgres.rs
Comment threadcrates/clickhousectl/src/local/mod.rs
Comment threadcrates/clickhousectl/src/local/postgres.rs
@sdairs

Copy link
Copy Markdown
CollaboratorAuthor

Carry-over review from PR #516 (old PR for #472). This PR is the right vehicle (roughly 1/3 the size of #516, cleaner primitives — tempfile + std File::lock() vs hand-rolled unsafe flock/uuid, lock released before ClickHouse readiness waits, good deterministic + subprocess concurrency tests incl. a live-reader concurrent start/client/stop test). #516 should be closed in favor of it. No sdairs review comments existed on #516 (all 12 of his comments were author replies to bot findings), but several fixes he confirmed there are regressed here. Required:

  1. Narrow the Postgres start critical section (High, also open Cursor Bugbot finding here). The project-wide metadata lock is held across Postgres image connect/inspect/pull and readiness (crates/clickhousectl/src/local/postgres.rs:~239 through startup_result). A slow pull blocks every lifecycle command in the project, including stop of unrelated servers. On Make local server metadata atomic and concurrency-safe #516, sdairs confirmed the equivalent fix: "Fixed in c641fa8… Docker connection and fresh-image inspection/pull now happen before the per-instance lifecycle lock." Move connect/pull/inspect before lock_metadata(), then revalidate under the lock, and release before readiness waits. This is strictly worse than Make local server metadata atomic and concurrency-safe #516's final state.

  2. Post-persist() directory-sync failure must not kill the committed child (Medium, also the last unresolved Cursor finding on Make local server metadata atomic and concurrency-safe #516: "Start kills server after metadata commit").save_server_info_locked can fail at sync_directory (server.rs:~224) afterpersist() succeeded; local/mod.rs:~535-537 then calls clean_up_untracked_child, killing a server whose metadata is already committed. Make post-persist dir-sync failure non-fatal (or at least not "untracked"). This was open on Make local server metadata atomic and concurrency-safe #516 too — fix it once, here.

  3. Restore the advisory-count tolerance and file-type guard (regressions of Make local server metadata atomic and concurrency-safe #516 fixes). On Make local server metadata atomic and concurrency-safe #516, sdairs: "Partially valid: the running-server count shown during start is advisory… added a tolerant path used only by advisory_running_server_count" (commit a1b48dd) and "Fixed in 4b4a5b3… ignores non-files before normalization". Here: start uses the strict list_running_servers_locked(&metadata_lock)?.len() (local/mod.rs:~443), so one corrupt default.json blocks starting an unrelated server; and list_all_servers_locked (server.rs:~368) has no is_file() check, so a stray foo.json/ directory fails list/start (load_info_at reads it). Port both behaviors. The test selected_metadata_reports_read_io_errors currently pins a directory as an error — adjust to match.

  4. Reorder the install.rs version-in-use check. On Make local server metadata atomic and concurrency-safe #516, sdairs: "Fixed in 5df646c… check evaluated and cached before the existing version directory is removed." Here install.rs:~155-156 still evaluates the now-fallible version_in_use_by_running_server()aftercommit_staged_install_locked, so a metadata error aborts install after the binary was already swapped. Cache the result before the commit.

  5. Minor: permission failures fold into ServerMetadataRead (criterion 4 asked for distinct user-facing errors — decide if folding is acceptable); lock failures surface as generic IO errors with no dedicated actionable variant (Make local server metadata atomic and concurrency-safe #516 had Error::ServerLock per "Fixed in 91a5e6f"); dotenv holds the lock across a blocking Docker credential read (postgres.rs:~1012-1021, per Make local server metadata atomic and concurrency-safe #516's "Fixed in 041676d" — kept, but inconsistent with client which drops at :~899; and it is the subject of Cursor's Medium latency finding here). PID-reuse identity verification from Make local server metadata atomic and concurrency-safe #516 was deliberately dropped as out of scope — fine, but note the residual exposure.

CI note: local postgres edge cases fails identically on the base branch — pre-existing stack breakage from #547, not this PR.

@sdairs
sdairsforce-pushed the stack/472-atomic-server-metadata branch 2 times, most recently from bdcaf1e to d3a3d1dCompareAugust 26, 2026 19:01
@sdairs
sdairsforce-pushed the stack/472-atomic-server-metadata branch from d3a3d1d to f8ba2c2CompareAugust 26, 2026 19:03
@sdairs
sdairsforce-pushed the stack/472-atomic-server-metadata branch from f8ba2c2 to 37dcaacCompareAugust 26, 2026 19:07
@sdairs
sdairsforce-pushed the stack/472-atomic-server-metadata branch 2 times, most recently from eeb03fe to 828af2fCompareAugust 26, 2026 19:12
@sdairs
sdairsforce-pushed the stack/472-atomic-server-metadata branch 2 times, most recently from cbf640b to fcd1797CompareAugust 26, 2026 19:24
@sdairs
sdairsforce-pushed the stack/472-atomic-server-metadata branch from fcd1797 to 299267cCompareAugust 26, 2026 19:28
@sdairs
sdairsforce-pushed the stack/472-atomic-server-metadata branch 2 times, most recently from 6e2cc60 to e9a1758CompareAugust 26, 2026 19:34
@sdairs
sdairsforce-pushed the stack/472-atomic-server-metadata branch from e9a1758 to 8fe5a8dCompareAugust 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 8fe5a8d. Configure here.

Comment threadcrates/clickhousectl/src/local/server.rs
@sdairs
sdairsforce-pushed the stack/472-atomic-server-metadata branch from 8fe5a8d to aaaed8fCompareAugust 26, 2026 20:10
@sdairs
sdairsforce-pushed the stack/472-atomic-server-metadata branch from aaaed8f to cd9663eCompareAugust 26, 2026 20:30
@sdairs
sdairsforce-pushed the stack/472-atomic-server-metadata branch 2 times, most recently from 02984b8 to d856048CompareAugust 26, 2026 20:38
@sdairs
sdairsforce-pushed the stack/472-atomic-server-metadata branch from d856048 to 406f0e9CompareAugust 26, 2026 20:41
@sdairs
sdairsforce-pushed the stack/472-atomic-server-metadata branch 2 times, most recently from b50cc18 to 42e1f05CompareAugust 26, 2026 20:49
@sdairs
sdairsforce-pushed the stack/472-atomic-server-metadata branch from 42e1f05 to 33241adCompareAugust 26, 2026 20:55
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 server metadata writes atomic and concurrency-safe

1 participant

@sdairs