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

[Selfeer]

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: Antalya 26.1 - Forward port of export part and partition ClickHouse#1388
• Full audit review: Antalya 26.1 - Forward port of export part and partition ClickHouse#1388 (comment)

[arthurpassos]

It actually can happen, but it is ok to be like this. Killing is best effort. If we try to fix it, we would need to rollback data from 3rd party storage, which is VEEERY problematic.

[arthurpassos]

This should be fixed by Altinity/ClickHouse#1912 (comment)