Make local server metadata atomic and concurrency-safe - #516
Conversation
sdairs
changed the title
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| pub fn running_server_count() -> usize { | ||
| list_running_servers().len() | ||
| pub fn running_server_count() -> Result<usize> { | ||
| Ok(list_running_servers()?.len()) |
There was a problem hiding this comment.
🟠 High local/server.rs:564
running_server_count fails with an I/O error when .clickhouse/servers contains a directory such as foo.json/, causing commands that count running servers—and therefore starting another server—to fail. list_all_servers now treats every .json directory entry as metadata and calls normalize_server_info without verifying it is a regular file; restore the file-type check before normalizing entries.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhousectl/src/local/server.rs around line 564:
`running_server_count` fails with an I/O error when `.clickhouse/servers` contains a directory such as `foo.json/`, causing commands that count running servers—and therefore starting another server—to fail. `list_all_servers` now treats every `.json` directory entry as metadata and calls `normalize_server_info` without verifying it is a regular file; restore the file-type check before normalizing entries.
| && is_master | ||
| && replaced_existing | ||
| && version_in_use_by_running_server(&exact_version) | ||
| && version_in_use_by_running_server(&exact_version)? |
There was a problem hiding this comment.
🟠 High version_manager/install.rs:177
A metadata/recovery error at version_in_use_by_running_server(&exact_version)? reports the master install as failed after the existing version directory and binary have already been replaced. Because the error returns before cleanup and master::record, the installed state is changed but the master record remains stale, forcing later downloads; perform this fallible check before mutating version_dir and retain its result for the notification.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/clickhousectl/src/version_manager/install.rs around line 177:
A metadata/recovery error at `version_in_use_by_running_server(&exact_version)?` reports the master install as failed after the existing version directory and binary have already been replaced. Because the error returns before cleanup and `master::record`, the installed state is changed but the master record remains stale, forcing later downloads; perform this fallible check before mutating `version_dir` and retain its result for the notification.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ 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 b40f62e. Configure here.
|
|
||
| entries.sort_by(|a, b| b.running.cmp(&a.running).then(a.name.cmp(&b.name))); | ||
| entries | ||
| Ok(entries) |
There was a problem hiding this comment.
Corrupt metadata blocks unrelated servers
High Severity
list_all_servers now returns on the first metadata read/parse failure instead of skipping that entry. Callers such as running_server_count during start, stop-all, version removal, and find_pg_instances therefore fail for every server when a single unrelated .json file is corrupt or unreadable. Previously those files were treated as absent and other servers remained usable.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit b40f62e. Configure here.
|
|
||
| /// Default user-facing name when `--name` is omitted: `"default"` if no | ||
| /// postgres "default" is running, otherwise a random adjective-noun. | ||
| fn default_pg_name() -> String { |
There was a problem hiding this comment.
Lock held across Postgres pull
Medium Severity
ServerLock for a Postgres instance is acquired before Docker connect/image pull and kept through wait_running (and through resume_existing). Cross-process flock therefore blocks list, stop, client, and other commands on that key for the full pull/start duration. ClickHouse start drops the lock right after saving metadata, so the engines behave inconsistently and Postgres start can stall the CLI for minutes.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit b40f62e. Configure here.
| container_id: Some(c.container_id.clone()), | ||
| }; | ||
| let _ = save_server_info(&info); | ||
| save_recovered_server_info(&info, false)?; |
There was a problem hiding this comment.
Recovery fails before metadata check
Medium Severity
Postgres orphan recovery always calls ensure_pg_data_dir before save_recovered_server_info, and no longer skips containers that already have metadata. A permission or I/O failure creating a data dir aborts recovery even when metadata already exists and no write is needed. Because recover_current_project_servers runs on most local commands, that failure surfaces as a hard error across start/stop/list/client.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b40f62e. Configure here.
1 participant
Summary
Closes #472
Tests
cargo fmt --all --checkcargo build -p clickhousectlcargo check -p clickhousectl --no-default-featurescargo test -p clickhousectlcargo clippy -p clickhousectl --all-targets -- -D warningsStack
issue-473-omitted-server-selection)