EXPORT PARTITION: `KILLED` status can be overwritten to `COMPLETED` due to race condition

Summary

A race condition in replicated partition export allows the KILLED terminal status to be overwritten to COMPLETED when KILL EXPORT PARTITION is issued concurrently with the last part's completion/commit. This violates the kill contract — cancellation is acknowledged to the operator, but the export is still finalized and committed.

Severity

High — cancellation is a control-plane safety mechanism. Fail-open behavior after acknowledged kill can cause unexpected data movement side effects.

Root Cause

The commit path in ExportPartitionUtils::commit() writes status=COMPLETED using trySet(..., -1) (unconditional version), which overwrites any concurrent status change. Meanwhile, killExportPartition() correctly uses compare-and-set (trySet(..., stat.version)) to transition PENDING → KILLED, but nothing prevents the commit path from subsequently overwriting KILLED → COMPLETED.

Commit path (src/Storages/MergeTree/ExportPartitionUtils.cpp):

destination_storage->commitExportPartitionTransaction(manifest.transaction_id, manifest.partition_id, exported_paths, context);

LOG_INFO(log, "ExportPartition: Committed export, mark as completed");
if (Coordination::Error::ZOK == zk->trySet(fs::path(entry_path) / "status",
    String(magic_enum::enum_name(ExportReplicatedMergeTreePartitionTaskEntry::Status::COMPLETED)).data(), -1))
{
    LOG_INFO(log, "ExportPartition: Marked export as completed");
}

Kill path (src/Storages/StorageReplicatedMergeTree.cpp):

if (status_from_zk.value() != ExportReplicatedMergeTreePartitionTaskEntry::Status::PENDING)
{
    LOG_INFO(log, "Export partition task is {}, can not cancel it",
        String(magic_enum::enum_name(status_from_zk.value())));
    return CancellationCode::CancelCannotBeSent;
}

if (zk->trySet(status_path,
    String(magic_enum::enum_name(ExportReplicatedMergeTreePartitionTaskEntry::Status::KILLED)),
    stat.version) != Coordination::Error::ZOK)

The problem: the commit path uses version -1 (unconditional overwrite), so even if killExportPartition has already successfully set the status to KILLED, the commit path blindly overwrites it to COMPLETED.

Reproduction Steps

1. Start a replicated partition export with multiple parts.
2. Delay destination writes so the completion callback timing is controllable.
3. Issue KILL EXPORT PARTITION <txid> after the last part upload begins but before the commit path writes the status.
4. Observe that KILL returns success, but the commit path subsequently sets status=COMPLETED.
5. Final ZooKeeper status is COMPLETED instead of KILLED.

Expected Behavior

Once KILLED is set, it must remain terminal. The commit path should fail-closed if the status has been changed from PENDING to KILLED during execution.

Suggested Fix Direction

Use compare-and-set semantics when writing COMPLETED in the commit path — only transition if the current status is still PENDING. If the status has changed (e.g., to KILLED), skip the commit finalization or roll back:

// Read current status + version before committing
auto [current_status, stat] = zk->get(fs::path(entry_path) / "status");

if (current_status != magic_enum::enum_name(ExportReplicatedMergeTreePartitionTaskEntry::Status::PENDING))
{
    LOG_WARNING(log, "ExportPartition: Status changed to {}, skipping commit finalization", current_status);
    return;
}

destination_storage->commitExportPartitionTransaction(...);

if (zk->trySet(fs::path(entry_path) / "status",
    String(magic_enum::enum_name(ExportReplicatedMergeTreePartitionTaskEntry::Status::COMPLETED)).data(),
    stat.version) != Coordination::Error::ZOK)
{
    LOG_WARNING(log, "ExportPartition: Failed to mark as COMPLETED (status was concurrently modified)");
    // Handle rollback or mark as indeterminate
}

Additionally, consider checking status before calling commitExportPartitionTransaction() to avoid committing data that should have been cancelled.

Suggested Test Direction

• Integration test with deterministic delay injection + concurrent KILL EXPORT PARTITION.
• Assert that the final ZooKeeper status remains KILLED after the race.
• Assert that no terminal successful finalize is recorded.
• Stress test with concurrent exports/kills across replicas with ZooKeeper fault injection.

Affected Components

• src/Storages/MergeTree/ExportPartitionUtils.cpp — commit() function
• src/Storages/StorageReplicatedMergeTree.cpp — killExportPartition() function
• Replicated export partition orchestration subsystem

Related

• Introduced in PR: https://github.com/Altinity/ClickHouse/pull/1388
• Full audit review: https://github.com/Altinity/ClickHouse/pull/1388#issuecomment-4005227275

Start with commit() in src/Storages/MergeTree/ExportPartitionUtils.cpp and killExportPartition() in src/Storages/StorageReplicatedMergeTree.cpp; trace their ZooKeeper status reads and writes. Add a deterministic-delay integration test that races completion with KILL EXPORT PARTITION, then verify the final status remains KILLED and no successful finalization is recorded.