Repository navigation
isCrossDiskDownloadIncompatible classifies an encrypted disk absent from system.disks as plain local, so a cross-type diff copy is allowed #1670
Description
After #1667, isCrossDiskDownloadIncompatible (pkg/backup/download.go) refuses a cross-disk diff reuse when exactly one of the two disks is a plain local disk. When the required disk is no longer present in system.disks (the rebalance or rename case the fallback was written for), it synthesizes the disk from the required backup disk_types:
func (b *Backuper) isCrossDiskDownloadIncompatible(disks []clickhouse.Disk, requiredBackup *metadata.BackupMetadata, requiredDisk, disk string) bool {
currentDisk := b.findDiskByName(disks, disk)
requiredDiskInfo := b.findDiskByName(disks, requiredDisk)
if requiredDiskInfo == nil && requiredBackup.DiskTypes[requiredDisk] != "" {
requiredDiskInfo = &clickhouse.Disk{Name: requiredDisk, Type: requiredBackup.DiskTypes[requiredDisk]}
}
if currentDisk == nil || requiredDiskInfo == nil {
return false
}
return isPlainLocalDisk(currentDisk) != isPlainLocalDisk(requiredDiskInfo)
}The synthesized clickhouse.Disk leaves IsEncrypted at its zero value, and disk_types cannot carry encryption at all: create.go records the type string alone.
for _, disk := range disks {
diskMap[disk.Name] = disk.Path
diskTypes[disk.Name] = disk.Type
}#1667 adds is_encrypted precisely because the type string is not enough (22.9+ reports the underlying type for an encrypted disk over a local disk, so the recorded value is local). The fallback therefore turns an encrypted disk into a plain local disk, isPlainLocalDisk(currentDisk) != isPlainLocalDisk(requiredDiskInfo) returns false, and the copy proceeds. For a part that moved to such a disk, the files in the required backup shadow are ciphertext, which is the corruption #1667 sets out to prevent.
Reproduction, predicate only, no server needed:
disks := []clickhouse.Disk{
{Name: "default", Type: "local"},
{Name: "hdd1", Type: "local"},
} // current system.disks, the encrypted disk was renamed away
diskTypes := map[string]string{
"default": "local",
"hdd1": "local",
"hdd3_encrypted": "local", // <type>encrypted</type> over a local disk, recorded as local
"disk_s3": "s3",
}| required disk | target disk | incompatible | expected |
|---|---|---|---|
| hdd3_encrypted, absent from system.disks | default | false | true |
| hdd3_encrypted, present with is_encrypted=1 | default | true | true |
| disk_s3, absent | default | true | true |
| removed_local, absent | default | false | false |
Only the encrypted and absent row is wrong. The upload side fails closed in the same situation: isCrossDiskReuseCompatible returns false when findDiskByName is nil, so the part is re-uploaded. The download fallback is the fail-open side of the same rule.
Suggested fix, either:
- record encryption in the backup metadata, a new field such as disk_is_encrypted or a distinct type value, and read it in the fallback; or
- fail closed while the fallback is in use and the recorded type is local, so a local to local reuse after a rebalance is allowed only for a backup that records both disks as unencrypted.
Test gap: TestIsCrossDiskDownloadIncompatible covers removed_local and removed_s3 but has no removed-encrypted case, so this path is not exercised.
Activity
minguyen9988 commentedon Nov 6, 2026
Correction to the consequence sentence in the first version, after tracing reachability.
The code-level defect above stands: isCrossDiskDownloadIncompatible synthesizes clickhouse.Disk{Name, Type} with IsEncrypted zero-valued, disk_types carries only the type string, and on 22.9+ an encrypted disk over a local disk records local. So an encrypted disk absent from system.disks is classified as plain local.
What I could not construct is a backup produced by the current write path that reaches it. markDuplicatedParts sets Required only when existsDisk == disk or isCrossDiskReuseCompatible holds, and that predicate denies an encrypted disk both when it is in system.disks (IsEncrypted=1) and when it is gone (findDiskByName returns nil). So the fail-open fallback is currently unreachable through upload, and the sentence claiming a corrupted restore should read as a fail-open on an invariant rather than a demonstrated corruption path.
It becomes live if metadata written by a version or tool without the reuse gate is read back, if the write-side gate is later relaxed and the read side is not, or if the invariant is ever assumed rather than enforced upstream of the fallback.
Related, and not analysed to the same depth: restore.go downloadRequiredPartForRestore walks for requiredDisk := range requiredBackup.Disks and findRestoreRequiredPartRemote with no disk-type check at all, so the restore path relies on the same invariant holding. Worth a look while this is open.
A guard whose stated job is to fail closed should not depend on a caller three files away for that.
minguyen9988 commentedon Nov 6, 2026
Correction to my correction above. The reachability statement I just posted is wrong, and the record should be accurate.
I said markDuplicatedParts cannot set Required for an encrypted disk. That holds only for the cross-disk branch. The same-disk branch has no disk-type check:
When existsDisk == disk the guard is skipped entirely, the fingerprint check passes, and newParts[i].Required = true records the part under the encrypted disk name. That is the ordinary case: a daily --diff-from-remote incremental on an encrypted disk reuses the previous backup parts from that same encrypted disk. Backups therefore do reference Required parts on an encrypted disk, and the test in this PR asserts that row ("0_1_1_0": true, // same disk).
The misclassification then bites on the read side, on a host where that disk is not in system.disks — the rebalance case the disk_types fallback was written for:
What is still unproven is an end-to-end reproduction: that needs a ClickHouse server with an encrypted disk and a disk rename, which I did not build. The predicate misclassification is proven; the corrupted-restore consequence follows from it and is inferred rather than demonstrated.
The restore path has the same shape and no check at all: downloadRequiredPartForRestore and findRestoreRequiredPartRemote walk requiredBackup.Disks and requiredTable.Parts with no disk-type comparison.