25.8.16 Backport of #95597 - Fix zero‑copy unlock check before part dir move

[mkmkme]

Fix zero‑copy unlock check before part dir move

Changelog category (leave one):

• Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Fix regression in (experimental) zero‑copy replication introduced by ClickHouse#94262 where shared parts could be deleted before other replicas finished fetching them (ClickHouse#95597 by @filimonov).

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

• Fast test
• Integration Tests
• Stateless tests
• Stateful tests
• Performance tests
• All with ASAN
• All with TSAN
• All with MSAN
• All with UBSAN
• All with Coverage
• All with Aarch64
• All Regression
• Disable CI Cache

Regression jobs to run:

• Fast suites (mostly <1h)
• Aggregate Functions (2h)
• Alter (1.5h)
• Benchmark (30m)
• ClickHouse Keeper (1h)
• Iceberg (2h)
• LDAP (1h)
• Parquet (1.5h)
• RBAC (1.5h)
• SSL Server (1h)
• S3 (2h)
• S3 Export (2h)
• Swarms (30m)
• Tiered Storage (2h)

[github-actions]

Workflow [PR], commit [ccca4b9]

[zvonand]

this PR needs to be rebased onto stable-25.8 branch

The base branch was changed.

[mkmkme]

@zvonand done

[alsugiliazova]

AI audit note: This review comment was generated by AI (claude-sonnet-4-6).

Audit for PR #1738 (backport ClickHouse#95597 — fix zero-copy unlock check before part dir move):

---

Call graph

• IMergeTreeDataPart::remove (caller)
  → can_remove_callback lambda (captures this, calls canRemovePart)
  → IMergeTreeDataPart::canRemovePart
  → StorageReplicatedMergeTree::unlockSharedData(part)
  → unlockSharedData(part, zookeeper)
  → side effect: zookeeper_ptr->tryRemove(zookeeper_part_replica_node) (removes ZK lock)
  → checks sibling children → returns {can_remove, files_not_to_remove}
  → DataPartStorageOnDiskBase::remove(can_remove_callback, ...)
  → [optional] can_remove_callback() ← this is the callsite being fixed
  → disk->moveDirectory(from, to)
  → clearDirectory(to, *can_remove_description, ...) → removeSharedRecursive

---

Root-cause analysis of the regression (verified)

can_remove_callback() was called after disk->moveDirectory(from, to) (at line 790 in the old
code). After the rename:

• part_dir still points to the original path (intentionally not updated to avoid races with
  system.parts readers).
• unlockSharedData calls part.getDataPartStorage().existsFile(FILE_FOR_REFERENCES_CHECK).
• Because the directory was already renamed to delete_tmp_*, existsFile on the original path
  returns false.
• The code takes the "Part looks temporary" branch
  (src/Storages/StorageReplicatedMergeTree.cpp:11027) and returns {true, {}} without
  deleting the ZK replica lock node and without checking whether other replicas still hold the part.
• clearDirectory then runs with can_remove_anything = true, deleting S3 blobs that other
  replicas were still fetching — the confirmed regression.

---

Correctness of the fix (verified)

Moving can_remove_callback() to before disk->moveDirectory (lines 758–759) is correct:

• existsFile(FILE_FOR_REFERENCES_CHECK) is evaluated while the original directory still exists
  → returns true → ref count check runs correctly → tryRemove(zookeeper_part_replica_node)
  executes → other replicas' locks are examined → {can_remove = false} returned when other
  holders exist → clearDirectory preserves blobs.
• The if (!can_remove_description) guard prevents double-evaluation when the to directory
  already existed and the callback was already invoked at line 726.
• Removing the now-redundant callback calls in the FILE_DOESNT_EXIST exception handlers is safe:
  can_remove_description is already set before the move attempt.

---

Interleaving analysis

Critical window between ZK unlock (inside can_remove_callback) and actual removeSharedRecursive
in clearDirectory: another concurrent replica could observe the ZK state change and itself decide
to delete blobs. This is an inherent structural race in the zero-copy design — unchanged by this PR.
The tryGetChildren check at line 11270 handles the concurrent-lock case safely via ZNOTEMPTY.

---

Confirmed defects

None introduced by this fix.

---

Pre-existing issue (not introduced by this PR, not blocking merge)

Low: ZK replica-lock orphan when from directory is absent before the rename

• Impact: When a part directory is manually deleted while the replica's ZK lock node still
  exists, the "Part looks temporary" branch in unlockSharedData returns {true, {}} without
  calling tryRemove(zookeeper_part_replica_node), leaving an orphaned ZK node. Subsequent
  replicas that attempt to remove the same part will see the dangling lock and conservatively
  refuse to delete S3 blobs, causing a storage leak.
• Anchor: DataPartStorageOnDiskBase.cpp:747–753 /
  StorageReplicatedMergeTree.cpp:11026–11030
• Trigger: Part directory is externally deleted (manual intervention) while the ZK lock node
  remains.
• Why defect: existsFile fails → "looks temporary" branch → ZK lock not released →
  perpetual blob retention by other replicas. Not introduced by this PR; identical behavior
  existed in the pre-94262 codebase.
• Fix direction: In the "from absent" early-return path, also check and remove the ZK
  replica node explicitly (bypass the existsFile guard for the unlock side-effect).
• Regression test direction: Simulate manual part removal; verify ZK node is cleaned up
  on next remove() attempt.

[alsugiliazova]

PR #1738 CI Triage

Summary

Category	Count	Notes
PR-caused regression	0	None
Infrastructure (regression workflow)	9 jobs	Missing .github/upload_results_to_database.sh on the 25.8.16 backport branch
Pre-existing flaky (upstream)	3 tests	All flaky for weeks/months across many unrelated PRs

Verdict: CI verified. No regressions attributable to PR #1738. Safe as merged.

Infrastructure failures (9 Regression jobs)

All Regression jobs failed identically with:

/home/ubuntu/_work/_temp/<uuid>.sh: line 1: .github/upload_results_to_database.sh: No such file or directory
##[error]Process completed with exit code 127.

Failing jobs:

• RegressionTestsRelease / Iceberg (1) / iceberg_1
• RegressionTestsRelease / Iceberg (2) / iceberg_2
• RegressionTestsRelease / Parquet / parquet
• RegressionTestsRelease / ParquetS3 (minio) / parquet_minio
• RegressionTestsAarch64 / Iceberg (1) / iceberg_1
• RegressionTestsAarch64 / Iceberg (2) / iceberg_2
• RegressionTestsAarch64 / Parquet / parquet
• RegressionTestsAarch64 / ParquetS3 (minio) / parquet_minio
• RegressionTestsAarch64 / ParquetS3 (aws_s3) / parquet_aws_s3

Evidence this is infra, not the PR:

• No report.html was uploaded to S3 for any of these jobs (HTTP 404 at s3.amazonaws.com/altinity-build-artifacts/PRs/1738/<sha>/regression/.../report.html).
• The CI database (gh-data.clickhouse_regression_results) has 0 rows for commit_hash = ccca4b96.... The regression tests never ran.
• Root cause: the PR workflow on the backports/25.8.16/95597 branch references .github/upload_results_to_database.sh, a helper added later on newer branches but missing on this backport branch.

Pre-existing flaky upstream tests

Queried from gh-data.checks for test_status = 'FAIL' over the last 60 days:

Check	Test	Fails (60d)	Unique PRs	First → Last
Integration tests (amd_binary, 4/5)	test_storage_rabbitmq/test.py::test_rabbitmq_mv_combo	6	4	2026-05-21 → 2026-07-01
Stateless tests (amd_binary, ParallelReplicas, s3 storage, parallel)	03707_set_index_bad_get_null_bug	30	13	2026-05-10 → 2026-07-03
Stateless tests (amd_binary, old analyzer, s3 storage, DatabaseReplicated, sequential)	03174_exact_rows_before_aggregation	3	2	2026-05-11 → 2026-07-06

The last two Stateless jobs appear GREEN on GitHub because they passed on rerun; the DB captured the earlier fail.

None of these tests touch zero-copy replication, part moves, or ReplicatedMergeTree — the areas modified by this backport (upstream ClickHouse#95597, fixing a regression from #94262).

Recommendation

The backports/25.8.16/95597 branch (and likely the whole antalya-25.8.16 line) is missing .github/upload_results_to_database.sh. Any future PR targeting this branch will show the same 9 red Regression jobs with no test signal. Cherry-pick the workflow-helper commit onto the 25.8.16 backport branch so future backport PRs get real regression CI signal.