Skip to content

Do not delete an already-published backup when BACKUP finalization fails - #112373

Open
groeneai wants to merge 5 commits into
ClickHouse:masterfrom
groeneai:groeneai/fix-112271-backup-finalize-keeps-published-backup
Open

Do not delete an already-published backup when BACKUP finalization fails#112373
groeneai wants to merge 5 commits into
ClickHouse:masterfrom
groeneai:groeneai/fix-112271-backup-finalize-keeps-published-backup

Conversation

@groeneai

@groeneai groeneai commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Closes: #112271
Related: #111394

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 into CHANGELOG.md):

A BACKUP that failed in the last steps of finalization, after its backup had already been written to the destination, deleted that complete backup during failure cleanup. Any incremental chained onto it was reported as BACKUP_CREATED but then failed during RESTORE with Code: 599 ... not found. A backup is now protected from the moment it is published, and the failure is still reported. Closes #112271.

Description

BackupImpl::finalizeWriting published the backup and only then armed the guard protecting it from failure cleanup:

writeBackupMetadata();               // directory backup published here
closeArchive(/* finalize= */ true);  // archive becomes readable here
setCompressedSize();                 // archive: a remote getFileSize, can fail
removeLockFile();                    // a live DELETE, can fail
writing_finalized = true;            // guard armed only here

If either of the last two steps throws, writing_finalized is still false, so setIsCorrupted succeeds, remove_backup_files_after_failure (default true) lets tryRemoveAllFiles run, and it removes .backup (or the archive) first. Its only bail-out, if (!checkLockFile(false)) return false;, cannot fire precisely because the lock file is still there, which is the case when removing it is what failed.

The intent already existed and was correct; only the window was mis-sized. The arming moves to the publication boundary, right after closeArchive(/* finalize= */ true) -- the only point correct for both writers, because an archive becomes readable only once IArchiveWriter::finalize has written its central directory. Arming earlier would gate off cleanup for a never-finalized archive, so a test arm covers that direction. The client still sees the error: the status comes from getBackupStatusFromCurrentException, called unconditionally.

The .lock object may now be left behind, since removing it is what failed. That is deliberate: locks are consulted only in write mode, so RESTORE and use as a base_backup are unaffected, and a new BACKUP there is still refused.

04652_backup_finalize_error_keeps_published_backup asserts, for a directory and an archive backup, that the published backup survives, restores, and still reports the error; that an incremental chained onto it restores; that the retained .lock is present and the destination refused; plus three pre-publication controls.

#111394 edits the same body, so whichever merges second needs a rebase.

groeneai and others added 2 commits July 28, 2026 18:39
BackupImpl::finalizeWriting published the backup and only then armed
writing_finalized, the flag that stops the failure-cleanup path from removing a
completed backup. A throw in setCompressedSize() or removeLockFile(), which run
after publication, therefore left the flag false, so setIsCorrupted() succeeded,
remove_backup_files_after_failure (default true) let tryRemoveAllFiles() run,
and it deleted `.backup` (or the archive) first. Its only bail-out,
checkLockFile(), cannot fire in this scenario precisely because the lock file is
still present, which is what happens when removing it is the operation that
failed.

The result was a complete, externally readable backup being destroyed, plus
every incremental chained onto it: an incremental stores only a locator and per
file (base_size, base_checksum) pairs, and nothing validates the base until a
RESTORE reaches a file with base_size != 0, so those incrementals were reported
as BACKUP_CREATED and only failed partway through recovery with Code 599.

Arm the flag at the publication boundary instead, immediately after
closeArchive(finalize=true). That is the only point correct for both writers: a
directory backup is published once writeBackupMetadata finalizes its buffer,
while an archive only becomes readable after IArchiveWriter::finalize() writes
the central directory and flushes the object. Arming any earlier would make
setIsCorrupted() return false for an archive that was never finalized, gating
off its cleanup entirely.

The failure is still reported to the client: the status comes from
getBackupStatusFromCurrentException(), which is called unconditionally, and the
synchronous path rethrows. setIsCorrupted() returning false gates only the file
removal.

One consequence is deliberate. The `.lock` object may be left behind after such
a failure, since removing it is what failed. Lock files are consulted only in
write mode, so RESTORE and use as a base_backup are unaffected, and a new BACKUP
to the same destination is refused by the `.backup`/archive existence check
first. A stray lock object is preferable to a destroyed backup.

Two test-only failpoints are added for the injection points, one inside the
publication window and one after it.

Closes: ClickHouse#112271
…vacuous

Review round follow-up on the test, no functional change.

Keeping a published backup after a finalize failure means the lock file stays
at the destination, because removing it is what failed. That is a deliberate,
user-visible consequence of this change, so assert it: the archive lock object
survives, and a new BACKUP to the same destination is still refused - by the
published-backup existence check rather than by the stray lock, which
checkBackupDoesntExist distinguishes through two different messages. The
archive lock is a sibling object next to the archive rather than an entry
inside it, so the test also removes it during cleanup instead of leaking one
per run.

The incremental chain arm could previously pass without exercising the chain:
if a background merge replaced the part the base backup holds, the incremental
became self-contained and restored to the same result while proving nothing.
Stop merges on the table and assert that the incremental's manifest actually
marks entries as reusing base data.

Also backtick setIsCorrupted in the new comment, per the repo convention for
literal function names.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@groeneai

Copy link
Copy Markdown
Contributor Author
Internal second-model review

Two review rounds, both by a model that did not write the code. Round 1 reviewed
187fa4bb0f98, bounced one major back for a fix; round 2 reviewed 7f89c3b610dd
and found nothing further. 7 findings total: 6 agreed and fixed, 1 noted.
Independent gate: 2 findings in round 1, 0 in round 2.

❌ The deliberately retained .lock was published as a consequence but nothing observed it (agreed, fixed).
The description states that this failure now leaves the .lock object behind, and
that the destination is nevertheless still refused for a new backup. Neither half
was asserted, and the archive arm also leaked <unique>_pub.zip.lock on every run,
because the archive lock is a sibling object next to the archive rather than an
entry inside it. Both halves are now asserted by one token: checkBackupDoesntExist
reports already exists for the archive name before it ever looks at the lock, so
seeing that message proves the destination is refused and that it is refused by
the published-backup check rather than by the stray lock. Cleanup extended
accordingly; no lock files remain after a run or after a 50-run batch.

❌ The incremental-chain arm could pass without exercising the chain (agreed, fixed).
If a background merge replaced the part the base backup holds, the incremental
would become self-contained and its restore would no longer depend on the base at
all, leaving the arm green but vacuous. Rather than measuring that draw, the fix
removes it: merges are stopped for the table, and the arm now asserts
<use_base>true</use_base> in the incremental's metadata, which is written only
for an entry that really reuses base bytes.

⚠️ Reported for information: a category question was raised and declined.
The gate asked for Critical Bug Fix. Category selection is a project decision
rather than a property of the diff, and the standing convention is that the
critical category is applied on a maintainer's request, so this PR keeps
Bug Fix. Please relabel if you would rather have it as critical. The body was
mechanically validated against the CI category parser.

💡 Noted, not changed: a comment now backticks setIsCorrupted per the repo
convention; the description was condensed for length, with no claim altered.

Checks that were re-derived rather than accepted. Every countable claim in
the validation-gate comment was recomputed from the tree. The arming point was
verified correct for both writer families independently, and each new assertion
was confirmed to move when the pre-fix ordering is restored, so none of them is
decorative. One case was checked specifically because getting it wrong would mean
data loss: the new arm that re-runs BACKUP against the surviving destination
cannot itself delete that backup, because the existence check throws from the
BackupImpl constructor, so no backup object reaches the cleanup path.

Session id: cron:clickhouse-review-slot-11:20260728-223900

@groeneai

Copy link
Copy Markdown
Contributor Author
Pre-PR validation gate
# Check Result
a Deterministic repro Yes. A ONCE failpoint injected inside the previously unprotected window of finalizeWriting reproduces the report's end state (the lock removal never happens, so the .lock object remains) with no proxy, no S3 and no fault-injecting object store. tests/clickhouse-test 04652_backup_finalize_failure_keeps_published_backup.
b Root cause explained Yes. finalizeWriting published the backup and armed writing_finalized only after setCompressedSize() and removeLockFile(). A throw in either leaves the flag false, so setIsCorrupted() succeeds, remove_backup_files_after_failure (default true) lets tryRemoveAllFiles() run, and it removes .backup (or the archive) first. Its checkLockFile bail-out cannot fire precisely because the lock file is still present, which is the case when removing it is what failed.
c Fix matches root cause Yes. The arming moves to the publication boundary, right after closeArchive(/* finalize= */ true). No widened bound, no retry, no guard bolted onto the deletion site. Retrying removeLockFile, and making tryRemoveAllFiles refuse when .backup exists, were both rejected: the first does not help an outage that outlasts the retries and leaves the ordering defect for setCompressedSize, the second does not transfer to archives where .backup lives inside the archive.
d Test intent preserved, new tests added Yes. No existing test is weakened, retagged or removed. One new stateless test with six arms; 04509 and 04510 remain the pre-publication controls and stay green.
e Demonstrated in both directions Yes. With the pre-fix ordering the new test fails on its own signature: the published .backup and the .zip are both gone, all three RESTOREs fail with Code: 599 ... not found, and the incremental cannot even be created. With the fix it passes. Each direction was rebuilt and re-measured against a server whose buildId() was checked against the binary.
f General across code paths Yes. writing_finalized has eight references, all inside BackupImpl.{cpp,h}; no other subsystem reads it. BackupImpl is the only IBackup implementation. The restore path is not a carrier: setIsCorrupted and tryRemoveAllFiles are called only from the backup path in BackupsWorker. OpenMode::UNLOCK is not a carrier. The internal-backup path still arms the flag, as ~BackupImpl and the writeFile guard require. Everything that can throw after finalizeWriting returns was already protected and stays protected, since the flag is now set strictly earlier.
g Generalizes across inputs Yes. Directory and archive writers each get a first-class arm, plus the incremental chain that is the user-visible blast radius, plus unpublished-failure controls at two distinct points (before the metadata, and inside archive finalization) and a control with no fault injected. One placement covers zip and tar because both go through IArchiveWriter::finalize.
h Backward compatible Yes. No setting, no serialization format, no protocol or ABI change, so no SettingsChangesHistory.cpp entry is needed. The only behaviour change is that this failure class no longer deletes data.
i Invariants and contracts preserved Yes. The failure is still reported to the client: the status comes from getBackupStatusFromCurrentException(), called unconditionally, and the synchronous path rethrows; setIsCorrupted() returning false gates only the file removal, and every bug arm asserts the client still receives the error. The arming stays inside the same mutex critical section. Disclosed trade: the .lock object may remain, which is benign because lock files are consulted only in write mode and a new backup to the same destination is refused by the existence check first.

50 runs of the new test with CI randomization enabled: 50 passed, 0 failed.

@groeneai

Copy link
Copy Markdown
Contributor Author

cc @SmitaRKulkarni @hanfei1991 - could you review this? BackupImpl::finalizeWriting armed writing_finalized only after setCompressedSize and removeLockFile, so a throw in either let the failure cleanup delete a backup that was already published at the destination, taking any incremental chained onto it with it. The arming moves to the publication boundary, right after closeArchive(/* finalize= */ true), which is the only point correct for both the directory and the archive writer.

@tiandiwonder tiandiwonder self-assigned this Jul 28, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [bc1732d]

Summary:

job_name test_name status info comment
Stress test (amd_debug) FAIL
Logical error: Query context must be created after authentication (STID: 3197-391f) FAIL cidb

AI Review

Summary

This PR moves writing_finalized up to the publication boundary inside BackupImpl::finalizeWriting and adds a focused stateless regression test for the local directory/archive cases. I still see one blocker: on S3/Azure backups with post-upload verification enabled, the code still arms the guard after the object-storage writer may already have published the object, so the same published-backup deletion window remains open there.

Findings

❌ Blockers

  • [dismissed by author -- https://github.com/Do not delete an already-published backup when BACKUP finalization fails #112373#discussion_r3670740638] [src/Backups/BackupImpl.cpp:1321-1327] The new arming point is still outside the real publication boundary for object-storage backups when s3_check_objects_after_upload / azure_check_objects_after_upload are enabled. The backup writers do consume those supported settings (src/Backups/BackupIO_S3.cpp:546-548, src/Backups/BackupIO_AzureBlobStorage.cpp:182-183, src/Core/Settings.cpp:647-655), and both lower-level writers still have post-publication throw sites (src/IO/WriteBufferFromS3.cpp:223-233, src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp:342-395). When that happens, writeBackupMetadata / closeArchive never return, writing_finalized stays false, and setIsCorrupted + tryRemoveAllFiles will still delete a published .backup or archive. I still consider this real because the PR comment and changelog claim the backup is protected from the moment it is published, but that invariant still does not hold for these supported backup settings. Suggested fix: either propagate an inner “published” signal out of IArchiveWriter / WriteBuffer and arm there, or narrow the PR contract and follow up with the object-storage fix before treating this as the general solution.
Final Verdict

Request changes.

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Jul 28, 2026
@alexey-milovidov alexey-milovidov added the can be tested Allows running workflows for external contributors label Jul 29, 2026
@tiandiwonder

Copy link
Copy Markdown
Contributor

@groeneai please fix the CI test failures.

groeneai added 2 commits July 29, 2026 02:07
The style check rejects a stateless test whose filename contains the substring
"fail" (ci/jobs/check_style.py, functional_tests_check), because it collides
with the runner's own failure matching. The new test carried it in
"..._finalize_failure_...", which failed Style check and, since that check
gates the workflow, left every build and every functional, integration,
fuzzer and bugfix-validation node dropped.

"error" carries the same meaning and matches the vocabulary the test already
uses for its own arm labels (bug_dir_error, bug_archive_error,
control_early_error, control_mid_error). Both files are renamed together and
their contents are unchanged; the test derives its own name at runtime from
BASH_SOURCE, so nothing inside it refers to the old name.
`BACKUP` and `closeArchive` were left as plain text in two comments. The
repository instructions ask for literal names from the ClickHouse SQL language,
classes and functions to be wrapped in inline code blocks, and for a function
to be written as `f` rather than as its application, so the second one also
loses its argument list and names the argument instead.
@groeneai

Copy link
Copy Markdown
Contributor Author

Fixed in 85e0a41.

There was one real failure: Style check rejects a stateless test whose filename contains the substring fail, because it collides with the runner's own failure matching (ci/jobs/check_style.py, functional_tests_check). My test carried it in ..._finalize_failure_..., so I renamed the pair to 04652_backup_finalize_error_keeps_published_backup. error matches the vocabulary the test already uses for its own arm labels. Both files are bit-identical after the rename, so the logic you approved is unchanged, and the test derives its own name at runtime from BASH_SOURCE, so nothing inside it referred to the old one.

I verified the fix by driving check_functional_test_cases over the real tree in both directions: 1 error before the rename, byte-identical to the CI message, and 0 after, over the same 12800 files. The total error length dropping to 0 also confirms this test was the only violator in the tree, so nothing else is waiting behind it.

The other two reds were downstream of that one. Style check gates the workflow, so all 13 builds and every functional, integration, fuzzer and bugfix-validation node were dropped, and Finish Workflow then failed because with all four Bugfix validation jobs dropped no per-arch validator ever measured the new test. Both clear once the builds actually run.

I also corrected the test name in the description, and wrapped BACKUP and closeArchive in inline code in two comments.

The new head has no workflow run yet, so it needs your approval to start.

writeBackupMetadata();
closeArchive(/* finalize= */ true);

/// The backup is published at this point: `.backup` is readable at the destination, or the archive

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

closeArchive/writeBackupMetadata are not the real publication boundary on remote writers, so this still leaves the old data-loss window open for archives and .backup objects stored on S3/Azure. ZipArchiveWriter::finalize and LibArchiveWriter::finalize both call the wrapped archive-file buffer's finalize after sealing the archive (src/IO/Archives/ZipArchiveWriter.cpp:362-373, src/IO/Archives/LibArchiveWriter.cpp:317-323), and those buffers explicitly treat preFinalize as a point where the file could already exist (src/IO/WriteBufferFromS3.cpp:159-223, src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp:151-316).

If that lower-level finalize/commit path throws after publishing the object, writing_finalized is still false here, so setIsCorrupted + tryRemoveAllFiles will delete an already-visible archive exactly like before. The same issue also exists for directory backups, because writeBackupMetadata finalizes the .backup buffer before returning. I think this needs an internal split between “seal archive / finish .backup contents” and “final publish the wrapped object-storage buffer”, with writing_finalized flipped at that inner boundary rather than after these helpers return.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right that the wrapped buffer's finalize runs after the archive is sealed, and right that a
throw inside it can leave a visible object. I measured where that is reachable, and it is narrower
than the finding states: at default settings there is no post-visibility throw site at all, so the
arming point is correct as written. The window you describe exists only with post-upload verification
turned on, and it is pre-existing.

Where the object becomes visible, per destination:

  • WriteBufferFromS3::finalizeImpl (src/IO/WriteBufferFromS3.cpp:206-238): preFinalize at :218
    only schedules the put / complete-multipart (task_tracker->addFinal at :198). The object is
    visible once task_tracker->waitAll() at :223 returns. A throw from waitAll means the upload
    failed, so nothing is published.
  • WriteBufferFromAzureBlobStorage::finalizeImpl (src/Disks/IO/WriteBufferFromAzureBlobStorage.cpp:306-397):
    CommitBlockList at :342, or the inline single-block Upload inside preFinalize.

Throw sites strictly after those points:

  • ZipArchiveWriter.cpp:373-377, after the wrapped buffer finalize at :372: zero.
  • LibArchiveWriter.cpp:323-326, after :322: zero.
  • After WriteBufferFromS3.cpp:223: only :229 checkObjectExists and the :233 size mismatch,
    both inside if (request_settings[S3RequestSetting::check_objects_after_upload]).
  • After Azure :342: only :391 and :395, both inside if (check_objects_after_upload).

Both settings default to false (s3_check_objects_after_upload and
azure_check_objects_after_upload, declared in src/Core/Settings.cpp), and the backup writers do
not force them on:
grep -rn check_objects_after_upload src/Backups/ returns one hit,
BackupIO_AzureBlobStorage.cpp:43, which serializes the value into a log map.

The second half is the same: for a directory backup writeBackupMetadata does finalize the .backup
buffer before returning, at BackupImpl.cpp:562, but between that and the arming at :1326 the only
statements are an arithmetic assignment, a LOG_TRACE, and closeArchive, whose body is a no-op when
archive_writer is null. Zero throw sites.

So with defaults, on both S3 and Azure and for both directory and archive backups, publication and
arming are adjacent and the fix holds. The changelog entry is scoped to that.

The residual you found is real, and worth stating precisely: with
s3_check_objects_after_upload=1 or azure_check_objects_after_upload=1, a throw at
WriteBufferFromS3.cpp:229/:233 or Azure :391/:395 does leave a visible object with
writing_finalized still false. WriteBuffer::finalize (src/IO/WriteBuffer.cpp:93-105) catches and
calls cancel(), but for S3 that reaches abortMultipartUpload (:459) for an upload whose
CompleteMultipartUpload already succeeded, so the abort fails and the object stays; the Azure buffer
has no cancelImpl override at all.

I am not folding that into this PR, for three reasons. It is pre-existing: all four carrier files are
byte-identical between this branch's merge-base, its head, and origin/master, so this PR neither
introduces nor widens it. It needs a design decision rather than a move, because a throw at :229/:233
means the object is visible but possibly the wrong size, and protecting a wrong-sized object from
cleanup may be worse than deleting it. And arming at the inner boundary would have to thread state out
of IArchiveWriter and WriteBuffer, which is wider than this one-line move and would redden this
PR's own control_mid_* arm, which asserts that a throw before archive_writer->finalize() still
removes the archive.

I filed it separately with the measurements and the constraints above, and it is reachable only with a
non-default setting enabled, on a failure that is itself an "it's a bug in S3 or S3 API" condition.

No source change in this round, so the approved change is untouched.

@groeneai

Copy link
Copy Markdown
Contributor Author

CI finish ledger - 85e0a41

Every failure below has an owner: a fixing PR (ours or external), or a full-effort fix task
whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test Reason Owner / fixing PR
Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel) / 01666_merge_tree_max_query_limit the per-table concurrency throttle the test asserts is keyed on query id, and secondary parallel-replicas reads are sent without one, so the holder query competes against itself (3/3 reruns, deterministic) #112385 (ours, open)

Session id: cron:our-pr-ci-monitor:20260729-120000

@clickhouse-gh

clickhouse-gh Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 86.50% 86.50% +0.00%
Functions 91.90% 91.90% +0.00%
Branches 78.70% 78.70% +0.00%

Changed lines: Changed C/C++ lines covered: 32/33 (96.97%) · Uncovered code

Full report · Diff report

@groeneai

groeneai commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

CI finish ledger - bc1732d

Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix task
whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

CI is fully finished on this head: 174 check-runs, 0 queued or in progress, and on the latest
workflow attempt (run attempt 3) both Config Workflow and Finish Workflow report success.
The Config Workflow / Finish Workflow / Post Hooks failures still visible in the CI database
for this commit belong to attempt 2 on 2026-08-01, which was dropped when praktika could not reach
the CI database; they are superseded, not live.

Check / test Reason Owner / fixing PR
Stress test (amd_debug) / Logical error: Query context must be created after authentication (STID 3197-391f) a pre-authentication PostgreSQL cancel-request builds a query context before authentication completes: Session::makeQueryContextImpl (Session.cpp:690) via PostgreSQLHandler::cancelRequest (PostgreSQLHandler.cpp:867). Not this PR: the diff is BackupImpl.cpp, FailPoint.cpp and one backup stateless test, and the signature spans 37 other carriers plus 7 master runs in 30 days #112940 (external, open)

#112940 is the causal fix rather than a coincidental same-area PR: it modifies
src/Server/PostgreSQLHandler.{cpp,h} and adds
04669_postgresql_protocol_cancel_request.sh, which targets exactly this abort site. It is open,
so there is no merged fix to pick up onto this branch.

Session id: cron:our-pr-ci-monitor:20260803-053000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-bugfix Pull request with bugfix, not backported by default

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A failed BACKUP finalize deletes a backup that already completed, making any incremental chained onto it unrestorable

3 participants