Add a service-scoped repair path for stale query keys

[sdairs]

Summary

• add an explicit cloud service repair-query-key <service-id> write operation with human and JSON output
• report stored-key 401/403 failures with an exact repair command without silently provisioning
• verify ownership metadata, replace only the owned endpoint binding, preserve unrelated credentials, and retain exact cleanup metadata across partial failures
• refuse legacy or incomplete records and extend service deletion to clean pending exact key IDs
• reuse a concurrent repair winner and retain a replacement key when endpoint rollback fails

Design notes

The dedicated subcommand intentionally supersedes the over-engineering concern raised on #508. Issue #454 calls for an explicit recovery operation, and keeping it as a separate write command avoids mutating local credentials as a side effect of a failed read. Although users can create a key and update a query endpoint with existing commands, that manual sequence does not provide the repair flow ownership checks, rollback ordering, exact pending-cleanup metadata, or same-project repair serialization. Repair is never triggered automatically.

Concurrent repairs in the same project snapshot the expected stale credential before waiting and reuse a changed lock winner without rotating it again. A repair explicitly started while a newly provisioned endpoint is still converging can still rotate that new key; the README tells users to wait for first-use provisioning and readiness to finish.

The endpoint API has no conditional update operation. Repair therefore reads and rewrites the complete endpoint configuration while replacing only the owned key ID. It preserves the configuration it read, but a concurrent endpoint update can be overwritten; this limitation and the required operational serialization are documented in the README.

Tests

• cargo test -p clickhousectl cloud::service_query::tests
• cargo test -p clickhousectl --test cli_request_shape_test repair -- --nocapture
• cargo test -p clickhousectl --no-fail-fast
• cargo check -p clickhousectl --all-targets
• cargo clippy -p clickhousectl --all-targets -- -D warnings
• cargo fmt --all --check

Closes #454

[sdairs]

Carry-over review from PR #508 (old PR for #454). This PR is technically strong: it meets all five acceptance criteria, the subcommand surface (cloud service repair-query-key <service-id>) fits AGENTS.md write-command conventions better than a dual-mode flag, ownership verification is stronger than anything in #508 (stored org + exact key ID + stored endpoint ID, returned endpoint ID re-verified, legacy records refused before any cloud request), and pending_cleanup_api_key_ids genuinely solves the cleanup-failure-retry class. Items to address:

1. Reconcile with sdairs' explicit review comment on Repair stale stored query keys #508. On Repair stale stored query keys #508, sdairs wrote: "This is over-engineered. The ability to create a key and associated it to a QE is already possible. This should simply be described in the output when it occurs, baking the functionality into another flag is unnecessary." (Repair stale stored query keys #508's final state was accordingly reduced to guidance + automatic local-credential retirement.) This PR reintroduces a full create-key/associate-to-QE implementation as a new CLI surface. Counterweights: issue Add a service-scoped repair path for stale stored query keys #454's acceptance criteria ask for "an explicit operation such as --repair-query-key or a credential removal command", and Repair stale stored query keys #508's auto-retire mutates local state as a side effect of a failed read command (which the issue discourages). State explicitly in the PR description why the dedicated subcommand supersedes that objection — otherwise the review will bounce on the same grounds.

2. Rollback-failure ordering (carries over from Repair stale stored query keys #508). On Repair stale stored query keys #508, sdairs committed to: "Replacement-key cleanup now runs only after endpoint rollback succeeds; rollback failure retains it for recovery." Here, fail_after_repair_binding (service_query.rs:~302-328) restores the endpoint then deletes the new key unconditionally — the two cleanups are independent if let Err blocks. If rollback fails while the endpoint still carries the new binding, the key is deleted and the endpoint is left bound to a dead key; the error only names the key if the delete fails. Gate the new-key deletion on rollback success, report the new key ID when rollback fails, and add the missing subprocess test for the rollback-failure branch.

3. Concurrent-repair reuse (carries over from Repair stale stored query keys #508). On Repair stale stored query keys #508, sdairs: "Each repair now snapshots the expected stale credential before waiting, reloads under the provisioning lock, and reuses a changed winner without rotating or deleting it." Here a second concurrent repair simply rotates the winner again (no snapshot-and-reuse). Not a correctness bug, but either implement the reuse or document the rotation-twice semantics as intentional. Also note: repair holds the provisioning lock, but a just-provisioned query's readiness retry runs unlocked, so a repair issued while another process's new key is still converging will rotate/delete that key — decide whether to document or close that residual race.

4. Full endpoint upsert without conditional update (carries over from Repair stale stored query keys #508). On Repair stale stored query keys #508, sdairs declared this superseded: "The stale-key recovery path no longer performs a full endpoint upsert." Here the repair is exactly a read-modify-write upsert of the endpoint config (inspect_repair_endpoint snapshots roles/openApiKeys/allowedOrigins; build_repair_endpoint_request replays them with the key swapped), with no ETag/conditional update. The existing provisioning path does the same, so it is not a regression vs main — but it resurrects a risk sdairs declared superseded. Acknowledge it explicitly in the PR.

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 2 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 7ab6c91. Configure here.}

[cursor]

Rollback failure desyncs credentials

Medium Severity

When endpoint bind succeeds but local persistence fails, and restore_repair_endpoint then also fails, fail_after_repair_binding keeps the replacement key in the cloud while discarding the in-memory credentials that match that binding. Local state still points at the unbound superseded key, so queries keep failing. A later repair rotates again and leaves the retained key ID in openApiKeys with no cleanup path.

Additional Locations (1)

• crates/clickhousectl/src/cloud/service_query.rs#L502-L514

 

^{Reviewed by Cursor Bugbot for commit 7ab6c91. Configure here.}

[cursor]

Missing rollback deletes concurrent endpoint

Medium Severity

For RepairEndpointState::Missing, any failure after the repair upsert—including when create_query_endpoint itself returns an error—runs delete_query_endpoint_if_exists. If another process created an endpoint after the initial 404 GET, that rollback deletes their configuration. Provisioning only deletes the new API key on bind failure and does not remove the endpoint.

Additional Locations (1)

• crates/clickhousectl/src/cloud/service_query.rs#L447-L457

 

^{Reviewed by Cursor Bugbot for commit 7ab6c91. Configure here.}