Skip to content

Guard local remove against running servers in any project - #632

Open
sdairs wants to merge 3 commits into
fix/599-local-remove-default-guardfrom
fix/600-local-remove-global-guard
Open

Guard local remove against running servers in any project#632
sdairs wants to merge 3 commits into
fix/599-local-remove-default-guardfrom
fix/600-local-remove-global-guard

Conversation

@sdairs

@sdairs sdairs commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

What

local remove's running-server guard was project-scoped: it called recover_current_project_servers() and filtered list_running_servers(), both of which only see .clickhouse/servers/ under the current working directory. A server started from another project directory was invisible, so local remove <its-version> exited 0 and deleted the binary from under it — the server kept running on its open fd, but local client -n <name> in that project then failed with managed_client_binary_not_found (issue #600).

The guard now enumerates running servers across every project using the same process-discovery machinery behind local server list --global, merged with this project's metadata and de-duplicated by PID:

  • Both sources are consulted because each covers a gap in the other: metadata is project-scoped but survives a failed process scan, while discovery spans projects but depends on pgrep/lsof//proc.
  • A single discovery pass feeds both orphan recovery and the global view, so the guard does not shell out to pgrep/lsof twice (recover_from_discovered_locked / global_entries).
  • A discovered process whose version cannot be read is treated as non-matching — blocking every removal on an unreadable command line would be unfixable by the user.

Why the error and --force changed shape

  • Refusal is still exit 1 / JSON server_running, but the message now names each blocking server as '<name>' in <project-root> (PID <pid>), in both the human error and the structured one, and points at clickhousectl local server list --global. Without the project root, a blocker outside the current directory is unactionable.
  • --force stops blockers wherever they run: through metadata in this project (so the recorded state stays consistent) and by PID elsewhere, exactly like server stop --global. Its confirmation line now names the project.

Tests

  • src/local/server.rs unit tests for the pure helpers select_version_users (no match, other-project match, same server via both sources reported once, merge of both sources, unreadable version, Postgres container never matching a ClickHouse version) and describe_version_users.
  • New subprocess suite crates/clickhousectl/tests/local_remove_global_guard_test.rs: isolated HOME, three fake installed versions, two project dirs, and deterministic pgrep/lsof/ps stand-ins on PATH (same technique as local_server_state_machine_test.rs, so the scan can never see or kill anything else on the machine). Covers the refusal, the structured error and its command, --force stopping the other project's server before removal, the unchanged same-project path (including metadata left at pid: 0), and an unrelated version still removing cleanly while the other project keeps running.
  • All four cross-project tests fail against the previous guard (verified by stashing the handler change).
  • cargo fmt --all, cargo clippy --workspace --all-targets -- -D warnings, cargo test -p clickhousectl all clean.

Docs

README.md and the local remove agent help now state that the check spans every project and that the error names the project root and PID.

Stacked PR

Part of a stacked chain: this PR is based on fix/599-local-remove-default-guard (the default-version guard in the same function), not main.

Fixes #600

🤖 Generated with Claude Code

@sdairs
sdairs force-pushed the fix/600-local-remove-global-guard branch from bce6449 to c019c42 Compare August 28, 2026 14:46
@sdairs sdairs linked an issue Aug 28, 2026 that may be closed by this pull request
@sdairs
sdairs force-pushed the fix/600-local-remove-global-guard branch from c019c42 to 3e3e732 Compare August 28, 2026 19:42
@sdairs
sdairs force-pushed the fix/600-local-remove-global-guard branch from 3e3e732 to f57abd2 Compare September 1, 2026 15:05
sdairs and others added 3 commits September 1, 2026 21:27
`local remove`'s running-server guard only looked at the current project:
it recovered orphans and filtered project-scoped `list_running_servers()`,
so a server started from another directory was invisible and its binary was
deleted from under it — leaving `local client` there failing with
`managed_client_binary_not_found` (issue #600).

The guard now enumerates running servers across every project using the
same process-discovery machinery behind `local server list --global`,
merged with this project's metadata (each source covers a gap in the
other: metadata survives a failed process scan, discovery spans
projects), de-duplicated by PID. One discovery pass feeds both orphan
recovery and the global view, so the guard does not pay for
`pgrep`/`lsof` twice.

Refusal stays exit 1 / `server_running`, and now names each blocking
server with its project root and PID in both the human and JSON message,
pointing at `local server list --global`. `--force` stops the blockers
wherever they run: through metadata in this project (so it stays
consistent) and by PID elsewhere, exactly like `server stop --global`.

Tests: unit coverage for the pure merge/selection and error-description
helpers, plus a new subprocess suite
(`tests/local_remove_global_guard_test.rs`) that starts a fake server in
project B with deterministic `pgrep`/`lsof`/`ps` stand-ins and asserts the
refusal, the structured error, `--force` behaviour, the unchanged
same-project path, and that an unrelated version still removes cleanly.
All four cross-project tests fail against the previous guard.

Fixes #600

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The subprocess test drives the cross-project running-server guard with
fake installed versions and deterministic pgrep/lsof/ps stand-ins; it
never downloads a binary, and the live install matrix in
test-install.yml does not run `local remove`, so a change here cannot
affect live install checks.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ve --force`

`local remove --force` stops a blocking server in another project with
`kill_server_by_pid`, which returns `Error::ServerNotRunning("PID n")`
when the process is already gone. Discovery and the kill are separated
by other blockers being stopped, so a server that exited in between
aborted the removal with exit 1 and a hint about a server that no longer
exists — the state --force was trying to reach.

Add `server::ensure_stopped_by_pid`, which maps `ServerNotRunning` to
success and passes every other failure through, and use it at that call
site only. `kill_server_by_pid` itself is unchanged so `server stop
--global` keeps reporting a PID that vanished since the listing. A unit
test drives both through a reaped child PID: the strict variant still
errors, the removal variant succeeds.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@sdairs
sdairs force-pushed the fix/600-local-remove-global-guard branch from f57abd2 to 1671239 Compare September 1, 2026 20:36
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.

local remove running-server guard only checks the current project

1 participant