Always set a terminal status when a backup or restore fails - #110990
Open
Onyx2406 wants to merge 4 commits into
Open
Always set a terminal status when a backup or restore fails#110990Onyx2406 wants to merge 4 commits into
Onyx2406 wants to merge 4 commits into
Conversation
If the cleanup after a failed backup threw a second exception (e.g. under memory pressure), the exception escaped before the terminal status was written, leaving the operation CREATING_BACKUP in system.backups forever and blocking clients waiting for it. Wrap the cleanup so the terminal status is always set, and catch all exceptions in the async runner.
Restore two em dashes corrupted by an editor round-trip, move the fault injection after setIsCorrupted so the BackupImpl destructor check is not tripped in the test, and make the test no-parallel because the failpoint is server-global.
The `try`/`catch` around the cleanup was not enough to guarantee a terminal status: the log message formatting before it, the formatting in the `catch` handler and the allocations inside `setStatus` (exception message with a stack trace, backup log entry) could all throw `MEMORY_LIMIT_EXCEEDED` again and escape `onException`, reproducing the stuck-status hang the previous commits fixed. `LockMemoryExceptionInThread` suppresses memory limit exceptions for the whole scope, the established idiom for such cleanup paths.
jkartseva
added
the
can be tested
Contributor
|
Workflow [PR], commit [808e092] Summary: ❌
|
clickhouse-gh
Bot
added
the
pr-bugfix
jkartseva
reviewed
Jul 27, 2026
| (is_internal_backup ? "internal backup" : "backup"), backup_name_for_logging)); | ||
| } | ||
|
|
||
| backups_worker.setStatusSafe(backup_id, getBackupStatusFromCurrentException()); |
Member
There was a problem hiding this comment.
setStatusSafe name is misleading: it can throw since it allocates
I makes sense to add a try-catch
void BackupsWorker::setStatusSafe(const String & id, BackupStatus status) noexcept
{
try
{
setStatus(id, status, /* throw_if_error = */ false);
}
catch (...)
{
tryLogCurrentException(log, fmt::format("Failed to set status for {}", id));
}
}
jkartseva
reviewed
Jul 27, 2026
|
|
||
| $CLICKHOUSE_CLIENT -q "SYSTEM ENABLE FAILPOINT backup_cleanup_error" | ||
|
|
||
| $CLICKHOUSE_CLIENT -q "BACKUP TABLE ${CLICKHOUSE_DATABASE}.table_does_not_exist TO Disk('backups', '${backup_name}')" 2>/dev/null || echo 'backup failed' |
Member
There was a problem hiding this comment.
Please add a test for the RESTORE path.
jkartseva
requested changes
Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: #92649
An aborted backup could stay in
CREATING_BACKUPstatus insystem.backupsforever (withBACKUP ... ASYNCclients waiting for the status never waking up), even though the backup had already failed — e.g. when the failure wasMEMORY_LIMIT_EXCEEDEDand the server was under memory pressure.Root cause
When a backup fails,
BackupStarter::onExceptionruns the cleanup (marking the backup corrupted, notifying coordination, removing files) before writing the terminal status. Any of those steps can throw a second exception — under memory pressure even an allocation in the cleanup fails — and that exception escapedonExceptionbeforesetStatusSafe(..., BACKUP_FAILED)ran. In the async path the exception disappeared into a discarded future, so nothing was recorded anywhere: no terminal status, nosystem.backup_logrow, andBackupsWorker::waitblocked forever.Fix
Wrap the cleanup of both
BackupStarter::onExceptionandRestoreStarter::onExceptionintry/catch(the cleanup failure is logged), so the terminal status is always written afterwards;std::current_exceptionat that point again refers to the original error, so the recorded status/exception are unchanged. Also widen the async runner'scatch (const std::exception &)tocatch (...), matching the sync path.A new
backup_cleanup_errorfailpoint injects a cleanup failure deterministically; the stateless test04618_backup_failed_status_on_cleanup_erroruses it to verify the status still reachesBACKUP_FAILED.Verified by code-path analysis plus the included test; no local build was run, so correctness relies on CI.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed a failed
BACKUPorRESTOREstaying in theCREATING_BACKUP/RESTORINGstatus forever when the cleanup after the failure threw a second exception; the terminal status is now always set.