[EliShteinman]

Fixes #16

Summary

• Adds preflight_database_replicas(). main() runs it right after connect_to_ch() and before init_run_log(), whose CREATE TABLE is the first DDL of every phase.
• The check resolves the auxiliary database: the one in COLLECTTABLEPREFIX, or currentDatabase() when the prefix has none.
• It confirms in system.databases that the database engine is Replicated. A configured cluster that only shares the database's name is ignored. A database that is not Replicated is skipped without waiting.
• It reads each registered replica's is_active from system.clusters (the replica's Keeper session). Only rows with a database_replica_name count, so a configured cluster that happens to share the database's name is ignored. An inactive (0) or unknown (NULL) replica raises UserVisibleError at once: exit 1 with one ERROR line naming the database and only the inactive replicas. This read is immediate and always runs.
• With --replica-liveness-recheck / S3GC_REPLICA_LIVENESS_RECHECK=true (off by default), it also waits the Keeper session timeout read from system.zookeeper_connection, plus --replica-liveness-margin-ms / S3GC_REPLICA_LIVENESS_MARGIN_MS (default 1000 ms, negative values rejected), and reads is_active again. A replica that crashed just before the run stays active until Keeper expires its session, so the second read is what catches it.
• A Replicated database with no system.clusters entry, or a server that cannot report liveness, logs a warning and continues, so the check is never disabled silently and never blocks a server that cannot answer it.
• The delete-time preflight_cluster() is unchanged; this check is in addition to it.
• No extra grant: the check reads only system tables.

Why not clusterAllReplicas

An earlier revision of this PR counted replicas answering clusterAllReplicas(<db>, system.one). ClickHouse runs that query on the other replicas as the Replicated database's cluster_username (default unless configured), not as the s3gc user. On a lab where default has a password and s3gc runs as its own user, ch2's query_log showed user = default, initial_user = s3gc. The preflight then failed a healthy database with default: Authentication failed (exit 1), while master passed and a real CREATE TABLE in that database, which goes through Keeper, succeeded instantly. It could also pass intermittently, because connections authenticated before the password change stay pooled. That revision also required the REMOTE grant; this one does not.

Operator-visible change

By default the check adds only a few system-table reads. The optional re-check adds one Keeper session timeout (about 31 s with defaults), and only when the auxiliary database is Replicated. A database that is not Replicated is unaffected. S3GC_REPLICA_LIVENESS_RECHECK and S3GC_REPLICA_LIVENESS_MARGIN_MS are not exposed through render.py, so Jobs use the defaults.

Known limit

A replica that dies after the preflight still stalls the next DDL for distributed_ddl_task_timeout and surfaces as IncompleteRead. That is tracked in TODO.md.

Validation

Local: 146 tests pass on Python 3.11, on top of current master (merged, including --collectdatabase). The new offline tests cover:

• passing when every replica stays active
• never calling clusterAllReplicas
• failing at once, without waiting, on an inactive replica
• treating an unknown (NULL) replica as inactive
• naming only the inactive replicas
• no wait, and still failing an inactive replica, when the re-check is off (the default)
• ignoring a same-named configured cluster's rows (is_active NULL)
• rejecting a negative margin from the CLI and the environment
• the wait of session timeout plus margin
• a replica that expires during the wait
• skipping a non-Replicated database without waiting
• the warning when liveness cannot be read

The rendered manifest validates with kubeconform.

Mutation check: six deliberate mutations, each caught by at least one test:

• remove the wait
• remove the second read
• treat NULL as active
• drop the margin
• remove the first check
• never fail after the wait

Live lab: ClickHouse 26.1.3.52, ClickHouse Keeper, nodes ch1–ch3 in cluster lab, and an S3 disk on SeaweedFS. Hardened users: default has a password, and s3gc runs as a separate s3gc user. Keeper session timeout 30 s. Auxiliary databases:

• gc: Replicated on all 3 nodes, with ch3 removed (still registered, hostname no longer resolves).
• gc2: Replicated on ch1 and ch2.
• lab: Atomic, beside the configured cluster lab.

Scenario	Previous revision	This revision
gc2, both replicas up	default: Authentication failed, exit 1	preflight passed, collect OK, exit 0 after 31 s
gc, ch3 long gone	—	inactive or unknown replicas in Keeper: ch3:9000, exit 1 after 1 s
lab (Atomic)	—	skipped, collect OK, exit 0 after 1 s
gc2, ch2 SIGKILLed 1 s before the run	—	first read all active; second read after 31 s: ch2:9000, exit 1

Keeper timing on the lab: after docker stop, is_active turned 0 at once; after docker kill it turned 0 after 30 s, the Keeper session timeout.

master, same lab, gc2 with only ch1 alive: the run-log CREATE fails after 181 s with IncompleteRead(441 bytes read), then the aux-table CREATE fails, traceback, exit 1 after 362 s. Server side: code 159, not finished on 1 of 2 hosts. A direct CREATE TABLE IF NOT EXISTS in gc with 1 of 3 alive returned not finished on 2 of 3 hosts. So the DDL waits for every registered replica, not a quorum, which is why the check requires all of them.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

A same-named configured cluster can cause the check to reject valid runs against a non-Replicated database.

1 open finding

• Avoid treating same-named clusters as Replicated databases · New

What changed in this PR

Adds an early replica check to prevent lengthy DDL failures when a Replicated auxiliary database has unavailable replicas, addressing #16.

Changes:

• Checks replica availability before run-log initialization.
• Adds nine offline regression tests.
• Documents the safeguard and collect’s additional REMOTE requirement.

File	Description
tests/​test_s3gc.py	Tests replica checks, failures, and execution order.
s3gc.py	Adds and invokes the database replica preflight.
README.md	Describes the new safeguard.
deploy/​kubernetes/​README.md	Explains the additional permission requirement.
CHANGELOG.md	Records the fix and operational context.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[EliShteinman]

Moved to draft. The first revision of the Replicated-database preflight checked replicas with clusterAllReplicas(<db>, system.one). ClickHouse runs that query on the other replicas as the database's cluster_username, default unless configured, not as the s3gc user. So wherever default has a password or is restricted, the preflight failed a healthy database (default: Authentication failed, exit 1), while master ran fine.

Commit 2892d41 replaces the remote probe with system.clusters.is_active from Keeper. It also re-checks after the Keeper session timeout, and no longer needs the REMOTE grant. That revision passes in the lab with a password on default and a dedicated s3gc user. It is held as a draft until it is validated on a real environment with a non-default user.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-22T11:56:43.825165Z	2892d41	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2892d41e7a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[lesandie]

Hey @EliShteinman , i see the point of the PR but I've got another question here:

Why supporting Replicated auxiliary databases is actually a requirement?. We could specify a new database (Atomic) and use plain ReplacingMergeTree, since s3gc runs against one pinned replica the inventory itself lives on the pinned host. It is true that a run can clean an installation-wide S3 prefix using CLUSTERNAME but I still don't see the point of adding complexity to the tool by supporting Replicated DB. Can you elaborate your use case and why do you think this is worth to implement

Thank you for your contribution! i'm looking forward to hearing from you about your thoughts!

[EliShteinman]

@lesandie Thanks for the question. We reach ClickHouse from an operator's workstation through the cluster's OpenShift Route, which load-balances across all replicas, so we cannot pin s3gc to one replica. A cleanup is several separate runs (collect, dry-run, delete, verify), and each can land on a different replica, so the auxiliary table must exist on every replica. That is why it lives in a Replicated database.

We may also run it in-cluster later, and we want one configuration that works in both cases. A Replicated database works whether the host is pinned or load-balanced. A node-local table works only when it is pinned.

The cost of a Replicated database is that every DDL waits for all registered replicas. One dead replica stalls the run for distributed_ddl_task_timeout and surfaces only as IncompleteRead, with no hint of the cause. This PR reads replica liveness from Keeper before the first DDL and fails at once with a clear error. That read is immediate. The extra wait of one Keeper session timeout (~31 s), which catches a replica that crashed just before the run, is now opt-in behind --replica-liveness-recheck.

[lesandie]

OK thanks for the explanation @EliShteinman, now I have a clear view of what you want to achieve. I guess you have precreated the auxiliary table as ReplicatedReplacingMergeTree right?, and how do you ensure its data is synchronized when requests fall into different replicas? SYSTEM SYNC REPLICA ON CLUSTER adb.investory LIGHTWEIGHT;

I think that we can merge this. The old mechanism (create table ReplacingMErgeTree) will work and for each run pinned in a replica I would create a different Replicated tables so you can read it from any replica. Same for the log table.

I leave you to address the reviews from the aibots and when ready ping me and i'll merge.

[lesandie]

Dev-cluster E2E validation

The opt-in dev_cluster test validates a two-replica development ClickHouse cluster using workload identity for S3. No credentials or target-specific configuration are stored in the test.

This run specifically checked cross-replica phase placement: collect was pinned to replica 1 (chi-dnieto-test-aws-dnieto-test-aws-0-0), while dry-run and delete were pinned to replica 2 (chi-dnieto-test-aws-dnieto-test-aws-0-1). The cluster metadata confirmed both endpoints are replicas 1 and 2 of the same shard.

The test created a uniquely named Replicated database and ReplicatedReplacingMergeTree inventory, then seeded 256 objects under a unique S3 prefix. After collection on replica 1, dry-run on replica 2 produced exactly the 256 fixture keys; delete on replica 2 removed those keys. After synchronization, both replicas showed inactive inventory rows and the test verified every fixture key was absent from S3. The test dropped its database and cleaned up its fixtures.

The DDL behavior was also exercised: create the auxiliary table once inside the Replicated database. CREATE TABLE ... ON CLUSTER is rejected there; the Replicated database distributes its DDL. The test waits with SYSTEM SYNC DATABASE REPLICA and synchronizes inventory data between phases with SYSTEM SYNC REPLICA ... LIGHTWEIGHT.

Latest run: live E2E 1 passed, 1 deselected (19.13s). Post-run verification found no generated E2E database on either replica and no temporary runner pod.

[lesandie]

Review finding: the optional replica-liveness recheck can report success with zero replicas.

preflight_database_replicas() handles an empty initial replica list by logging a warning and skipping the preflight. With --replica-liveness-recheck, however, the second _inactive_database_replicas(database) result has no equivalent check. If that read returns [], _raise_for_inactive_replicas() receives an empty inactive list and the function logs database replica preflight passed: ... replicas=0.

I've applied the same empty-result handling to the second read: warn that the preflight was skipped rather than report a successful check. A regression test should return healthy replicas for the first snapshot and an empty list after the wait, then assert that the warning is emitted and no successful-preflight message is logged.

This finding concerns the accuracy of the database DDL preflight and the existing delete-time cluster/expected-replica checks still apply.

[lesandie]

cross-replica E2E passed, and all review threads are resolved. Merged @EliShteinman