Fix incremental restore to survive SchemeShard reboots and handle shard failures#35
Fix incremental restore to survive SchemeShard reboots and handle shard failures#35maximyurchuk wants to merge 11 commits into
Conversation
Code review of the prior commits in this PR surfaced two correctness defects in the shard-failure retry path plus several cleanup items. Correctness fixes: 1. RetryNeeded overwrite race in CheckForCompletedOperations. Two TTxProgressIncrementalRestore runs can interleave: the first erases the failure flag from FailedIncrementalRestoreOperations and sets RetryNeeded=true; if a second run arrives before the retry branch executes, it sees no failures and overwrites RetryNeeded back to false, silently losing the retry signal. Changed assignment to |=. 2. Retry budget. The retry loop had no terminal condition — a permanently broken shard caused infinite scan re-creation. Added per-incremental retry counter (transient, reset on advance) capped at 5 attempts. On exhaustion, state is set to EState::Failed and persisted; get/list APIs report PROGRESS_DONE with GENERIC_ERROR. Cleanups: - Removed dead 1-arg TEvRunIncrementalRestore constructor (zero callers after the orphan-recovery fix in this PR). - Extracted TrackIncrementalRestoreShardReply helper to deduplicate the ~26-line shard-reply tracking block that was identical between TProposeAtTable::HandleReply and NIncrRestore::TProposedWaitParts::HandleReply. Test coverage: - Added IncrementalRestoreShardFailureTriggersRetry: uses runtime.AddObserver to inject one TEvFinished failure and verifies the retry path recovers data integrity. - Added TestIncrementalRestoreFinalizingStateRecoveryAfterReboot: exercises TTxInit resume from EState::Finalizing via a 5-incremental chain and bucket reboots. - Hardened WaitForRestoreDone with required bool expectRegistered parameter — prior version silently passed when ListRestores returned empty entries, masking real registration regressions.
Add MaxIncrementalRestoreTablesInFlight ImmediateControl (default 32, sentinel -1 = unbounded). The orchestrator now enqueues table and index sub-ops onto PendingTables and DispatchPendingTables drains in waves bounded by the cap, topped up after each completion. Prevents unbounded fan-out on backup collections with many tables/indexes.
Funnel TrackIncrementalRestoreShardReply through TTableOperationState:: RecordShardResult so the first terminal report wins and re-deliveries (DataShard restart replays, schemeshard reboot re-sends, interconnect retransmits) are dropped. Prevents double-counting that would put a shardIdx in both Completed and Failed sets and break AllShardsComplete's sum invariant. Adds a contract unit test.
Line numbers in cross-file comments rot as soon as either file is edited. The adjacent code is self-explanatory.
Add MaxIncrementalRestoreRetriesPerIncremental ImmediateControl (default 50, sentinel -1 = unbounded) to bound the orchestrator's per-incremental retry attempts. Propagate a Retriable bit on TShardOpResult / TEvFinished so non-retriable scan failures (anything other than Done/Term) short- circuit the retry loop instead of burning the budget. Failed restores now report GENERIC_ERROR via the list endpoint instead of the SUCCESS default. Adds integration tests for backoff timing, budget enforcement, unlimited-cap sentinel, non-retriable short-circuit, reboot survival, and concurrent-completion de-duplication. Bumps reboot suite SPLIT_FACTOR to 50 and adds explicit ram/cpu requirements.
Extract the per-entry TBackupCollectionRestore progress/status switch into FillRestoreProgress in schemeshard_restore_incremental_progress.h and call it from both __get and __list, so the two endpoints can't diverge on Failed -> GENERIC_ERROR or progress percent. Trim Retriable bit comments down to one-liners.
Move the ad-hoc "_full" / "_incremental" string literals scattered across backup, restore, and init paths into FullBackupSuffix / IncrementalBackupSuffix constants plus FullBackupDirName / IncrementalBackupDirName builders in schemeshard__backup_collection_common.h. Eliminates the risk of one site drifting from another when the convention ever changes.
…odies Pull out RestoreIncrementalRestoreOpPathStates and ScheduleOrphanedIncrementalRestoreOp from TTxInit::ReadEverything, HandleRetryPath and HandleAllOperationsComplete from TTxProgressIncrementalRestore::Execute, ValidateProposePaths from TNewRestoreFromAtTable::Propose, and TrackSubOpAndExpectedShards reused by both CreateSingleTableRestoreOperation and CreateSingleIndexRestoreOperation. Also tighten the reboot-tests file. Pure refactor — no behavior change. The new TTxInit helper keeps using NBackup::FullBackupDirName / IncrementalBackupDirName so the suffix centralization from 1102753 stays.
Pure comment cleanup across PR-touched files: collapse multi-paragraph block comments to one or two short sentences, drop comments that just paraphrase the next line of code, and move trailing end-of-line comments to the line above. No code or behavior changes.
The InvolvedShards set was populated but never read — cleanup paths key by OperationId. Remove the in-memory set, the TTxInit reload block, and the per-sub-op insert; reserve proto tag 7 in TLongIncrementalRestoreOp (formerly InvolvedShards) to prevent reuse.
There was a problem hiding this comment.
openai/gpt-5.4
AI Review Summary
Verdict: ✅ No critical issues found
No critical issues found.
Other findings
- Major | High: Batching sub-ops can deadlock an incremental restore when a queued item cannot be dispatched.
DispatchPendingTables()pops an entry before callingCreateSingleTableRestoreOperation()/CreateSingleIndexRestoreOperation(), but those helpers may early-return without creating any sub-operation when the target path or backup path no longer resolves. Because the item is discarded, nothing is added toInProgressOperations; once the currently running batch drains,AreAllCurrentOperationsComplete()returns true and the restore advances/finalizes as if that table/index had been restored successfully. This regresses correctness compared with the old eager path, which skipped missing inputs before completion accounting was introduced. —ydb/core/tx/schemeshard/schemeshard_incremental_restore_scan.cpp:542
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
AI Review SummaryVerdict: ✅ No critical issues found No critical issues found. Other findings
This review was generated automatically. Critical issues require attention; other findings are advisory. |
maximyurchuk
left a comment
maximyurchuk
left a comment
There was a problem hiding this comment.
Severity: Major
Confidence: High
FillRestoreProgress() maps EState::Failed to PROGRESS_DONE with 100%, and TTxList now uses that helper verbatim for every entry. As a result, ListBackupCollectionRestores will report a failed restore as completed successfully, while TTxGet reports the same restore as GENERIC_ERROR. That inconsistency breaks clients that use list results to detect failed restores.
Please make the list response surface the failed status too, either by encoding failure distinctly in FillRestoreProgress() or by adding the same failed-state override that TTxGet now applies.
maximyurchuk
left a comment
maximyurchuk
left a comment
There was a problem hiding this comment.
Severity: Major
Confidence: Medium
During restart recovery this code reconstructs IncrementalBackups from trimmed names only and passes 0 as the timestamp for every entry. AddIncrementalBackup() sorts by timestamp, so after reboot the restore order is no longer derived from the original backup times and instead depends on whatever ordering GetIncrementalBackupTrimmedNames() happens to return. If that proto field is not already chronologically sorted, a resumed restore can replay incrementals out of order.
Please persist and restore the original timestamps (or sort by a stable time-derived key extracted from the backup names) before resuming the operation.
maximyurchuk
left a comment
maximyurchuk
left a comment
There was a problem hiding this comment.
Review in progress...
|
|
||
| // Two-phase backoff guard: set when a retry is scheduled, cleared when it fires. | ||
| // Prevents concurrent completion events from double-counting retries. | ||
| bool RetryScheduled = false; |
There was a problem hiding this comment.
Severity: Major
Confidence: High
CurrentIncrementalRetryCount is not persisted to the IncrementalRestoreState database table. After a SchemeShard reboot, it resets to 0, effectively resetting the retry budget. A workload with persistent retriable failures could survive indefinitely across reboots even though MaxIncrementalRestoreRetriesPerIncremental is set.
The MaxIncrementalRestoreRetriesPerIncremental ICB control cannot be enforced across reboots. Consider persisting CurrentIncrementalRetryCount (e.g., in SerializedData) so the retry budget survives reboots.
| state.RetryScheduled = false; | ||
| state.NextRetryAttemptAt = TInstant::Zero(); | ||
| db.Table<Schema::IncrementalRestoreState>().Key(OperationId).Update( | ||
| NIceDb::TUpdate<Schema::IncrementalRestoreState::State>(static_cast<ui32>(state.State)) |
There was a problem hiding this comment.
Severity: Major
Confidence: High
When the incremental restore enters Failed state, there is no cleanup of path states. Tables remain stuck in EPathStateIncomingIncrementalRestore and backup paths remain in EPathStateOutgoingIncrementalRestore/EPathStateAwaitingOutgoingIncrementalRestore. This could block subsequent operations on those paths (e.g., drop, alter) since the IsUnderOperation() check will return true.
A finalize-like cleanup path is needed for the Failed state to reset path states back to EPathStateNoChanges and allow the system to recover.
| state.CompletedOperations.clear(); | ||
| state.PendingTables.clear(); | ||
| state.CurrentIncrementalStarted = false; | ||
|
|
There was a problem hiding this comment.
Severity: Major
Confidence: Medium
When a retry fires (backoff timer expires), InProgressOperations, CompletedOperations, PendingTables are cleared, but TableOperations is NOT cleared. Stale TTableOperationState entries from the failed attempt remain. This means:
ExpectedShardsfrom the previous attempt may include shards that no longer apply after table restructure.FailedShards/CompletedShardsfrom the previous attempt accumulate.HasNonRetriableFailurefrom the previous attempt persists even though the operation is being retried.
The CheckForCompletedOperations function iterates TableOperations to check HasNonRetriableFailure, which could trigger a false NonRetriableFailure from stale data.
Consider clearing TableOperations alongside the other collections in this retry reset block.
| return; | ||
| } | ||
|
|
||
| LOG_I("Creating separate restore operation for table: " << incrBackupPathStr << " -> " << targetTablePath); |
There was a problem hiding this comment.
Severity: Critical
Confidence: Medium
When CreateSingleTableRestoreOperation returns early (path split failure or unresolved backup path), the pending op has already been removed from the PendingTables queue by DispatchPendingTables. No operation is tracked in InProgressOperations. This means:
- The table is silently skipped — no restore operation is created, no error is propagated.
- If ALL tables fail at dispatch,
AreAllCurrentOperationsComplete()returns true (via theCurrentIncrementalStarted && InProgressOperations.empty() && CompletedOperations.empty()branch), causing the incremental to be marked complete and the restore advances to the next incremental. - The result is silent data loss: tables that should have been restored are not, and the restore completes "successfully".
Consider either: (a) not popping from PendingTables until the operation is successfully created, or (b) tracking failed dispatches and triggering a retry/failure, or (c) keeping the pending op in the queue for the next dispatch cycle.
| } | ||
|
|
||
| THolder<NTable::IScan> CreateIncrementalRestoreScan( | ||
| NActors::TActorId parent, |
There was a problem hiding this comment.
Severity: Minor
Confidence: High
The comment mentions Aborted as a retriable status ("Operator/system actions (Term) and transient conditions (Aborted) are retriable"), but the code only includes Done and Term in the return expression. NTable::EStatus::Aborted is NOT treated as retriable.
Either add Aborted to the retriable set, or update the comment to remove the mention of Aborted.
| } else { | ||
| restore.SetProgressPercent(1); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Severity: Minor
Confidence: Medium
incrProgress can exceed total if CalcCurrentIncrementalProgress() returns close to 1.0 for a later incremental (e.g., when CurrentIncrementalIdx == total - 1 and progress is 1.0). The expression 1 + static_cast<ui32>(incrProgress * 97 / total) could then yield 98 or 99, overlapping with the Finalizing (99%) and Completed/Failed (100%) values.
Consider clamping the result to the range [1, 97] to keep the progress ranges non-overlapping and meaningful.
| bool retriable = !failed | ||
| || !record.HasOpResult() | ||
| || !record.GetOpResult().HasRetriable() | ||
| || record.GetOpResult().GetRetriable(); |
There was a problem hiding this comment.
Severity: Minor
Confidence: Medium
When failed is true but record.HasOpResult() is false (no OpResult attached), retriable falls through to true via !record.HasOpResult(). This means a failure without an OpResult is assumed retriable. This is a reasonable default for forward compatibility (older DataShards that do not set Retriable), but it means that genuinely non-retriable failures from such DataShards will be retried until the budget is exhausted rather than short-circuiting to Failed.
This is a deliberate design choice documented by the "meaningful only when !Success" comment on the proto field, but worth noting as a potential improvement: consider adding a log warning when a failure is assumed retriable due to missing OpResult.
| }; | ||
|
|
||
| // Incremental restore shard progress tracking | ||
| // Deprecated: kept for compatibility |
There was a problem hiding this comment.
Severity: Minor
Confidence: Low
The IncrementalRestoreShardProgress table is marked as deprecated but the schema still exists. Rows written by earlier versions of the code will remain in the database indefinitely. The cleanup code in schemeshard__operation_drop_backup_collection.cpp that used to clean up these rows was removed in this PR.
Consider adding a one-time migration to clean up existing rows from this deprecated table during init, or at least document that these rows will accumulate until the database is recreated.
|
|
||
| state.BackupCollectionPathId = TPathId( | ||
| op.GetBackupCollectionPathId().GetOwnerId(), | ||
| op.GetBackupCollectionPathId().GetLocalId()); |
There was a problem hiding this comment.
Severity: Major
Confidence: Medium
During TTxInit, AddIncrementalBackup is called with dummyPathId and timestamp=0. Since AddIncrementalBackup sorts by timestamp after insertion, and all rebooted entries have timestamp 0, the sort is based on the order of insertion which depends on op.GetIncrementalBackupTrimmedNames() iteration order. If this differs from the original insertion order (which used real timestamps), the IncrementalBackups vector will be in a different order after reboot, potentially processing incrementals out of chronological order.
This could lead to data corruption if an older incremental is applied after a newer one.
Consider persisting the backup timestamps alongside the trimmed names (e.g., in the TLongIncrementalRestoreOp proto), or using the backup directory creation time to restore the correct order.
| << ", currentIdx: " << state.CurrentIncrementalIdx | ||
| << ", at schemeshard: " << Self->TabletID()); | ||
| OnComplete.Send(Self->SelfId(), | ||
| new TEvPrivate::TEvProgressIncrementalRestore(operationId)); |
There was a problem hiding this comment.
Severity: Major
Confidence: Medium
After reboot, Running states resume the incremental restore from the beginning of the current incremental (as CompletedOperations are intentionally not restored for Running states). However, the sub-operations that were in flight before the reboot may still be executing. When these complete, NotifyIncrementalRestoreOperationCompleted will insert them into CompletedOperations, but ProcessNextIncrementalBackup may have already re-created them (with new txIds). This could lead to:
- Duplicate operations for the same table within a single incremental.
- Stale operations completing and being counted as the new operation.
The current code relies on sub-operations being idempotent, but duplicate sub-operations for the same table could race against each other, leading to unpredictable behavior (e.g., two concurrent scans writing to the same target table).
Consider explicitly cancelling or waiting for in-flight sub-operations before restarting the current incremental after reboot.
|
|
||
| // Advisory mirror of SchemeShardControls.MaxIncrementalRestoreRetriesPerIncremental. | ||
| static constexpr ui32 MaxRetriesPerIncremental = 5; | ||
|
|
There was a problem hiding this comment.
Severity: Major
Confidence: High
NonRetriableFailure is in-memory only and not persisted. After a reboot, a non-retriable failure (e.g., schema mismatch, corrupt data) will be retried instead of short-circuiting to Failed. This wastes the retry budget on an operation that will never succeed, and delays the user-visible failure.
Combined with the CurrentIncrementalRetryCount reset (separate comment), a non-retriable failure after reboot will burn through the entire retry budget (default 50 attempts) before finally failing, instead of failing immediately.
Consider persisting NonRetriableFailure to the database, or at minimum adding a check during reboot reconstruction that inspects the in-flight sub-operations for non-retriable failures.
| break; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Severity: Minor
Confidence: Low
The default case in FillRestoreProgress sets PROGRESS_PREPARING with 0% progress but inherits Status = SUCCESS from the top of the function. If a new EState enum value is added in the future (or if there is an unexpected state), the API would report SUCCESS with PROGRESS_PREPARING, which is contradictory.
Consider either moving SetStatus(SUCCESS) into each explicit case, or setting a non-SUCCESS status in the default case.
| && (i64)state.CurrentIncrementalRetryCount >= cap; | ||
| if (state.NonRetriableFailure || budgetExceeded) { | ||
| LOG_E("Incremental #" << state.CurrentIncrementalIdx | ||
| << " short-circuiting to Failed: nonRetriable=" |
There was a problem hiding this comment.
Severity: Minor
Confidence: Medium
The budget check is skipped when RetryScheduled is true (!state.RetryScheduled && (i64)state.CurrentIncrementalRetryCount >= cap). This means that if the cap is dynamically lowered via ICB while a retry is in flight, the operation will not fail until the next non-scheduled progress event. This is likely intentional (to avoid killing an in-flight retry), but the comment says "Skip budget check while a retry is in flight to avoid premature failure" which could be misleading — the failure is not "premature", it is deferred.
Consider clarifying the comment to explain the design rationale: the cap is checked only when a retry completes and new failures are detected, not while a retry is pending execution.
| ); | ||
| } | ||
|
|
||
| Response->Record.SetStatus(Ydb::StatusIds::SUCCESS); |
There was a problem hiding this comment.
Severity: Nit
Confidence: High
For the Failed state, Fill() (via FillRestoreProgress) already sets Status = GENERIC_ERROR on the restore proto. Then the code sets GENERIC_ERROR again on the record and via Reply(). The double-set is redundant but not harmful.
However, the control flow is fragile: if a new state is added that should return a different status, both FillRestoreProgress and this handler need to be updated independently. Consider centralizing the status decision in FillRestoreProgress and reading it back from the proto here.
| @@ -1265,26 +1269,48 @@ class TSchemeShard | |||
| // Incremental Restore event handlers | |||
There was a problem hiding this comment.
Severity: Major
Confidence: Medium
FailedIncrementalRestoreOperations is transient (cleared on reboot). After a reboot, completed sub-operations that had failed before the reboot will not be in this set. When CheckForCompletedOperations checks Self->FailedIncrementalRestoreOperations.erase(opId), it will not find the previously-failed operations, so hasFailedOperations will be false, and RetryNeeded will not be set. This means previously-failed operations will be treated as successful after reboot.
This is partially mitigated by the design decision to restart the current incremental from scratch after reboot (clearing CompletedOperations). However, it means the failure signal is lost, and if the sub-operation succeeded on its own after reboot (e.g., because the underlying issue was transient), the orchestrator will never know it originally failed.
| ui32 doneShards = 0; | ||
| for (const auto& [_, tableOp] : TableOperations) { | ||
| totalShards += tableOp.ExpectedShards.size(); | ||
| doneShards += tableOp.CompletedShards.size() + tableOp.FailedShards.size(); |
There was a problem hiding this comment.
Severity: Minor
Confidence: Medium
CalcCurrentIncrementalProgress() counts FailedShards.size() in the doneShards count: doneShards += tableOp.CompletedShards.size() + tableOp.FailedShards.size(). This means that when shards are failing, the progress percentage increases as if they succeeded, which is misleading to the user. A restore with 50% shard failures would still show 100% progress within the incremental, even though it will be retried.
Consider either excluding FailedShards from the progress calculation, or showing a separate failure indicator.
| optional string FullBackupTrimmedName = 5; | ||
| repeated string IncrementalBackupTrimmedNames = 6; | ||
| repeated uint64 InvolvedShards = 7; | ||
| reserved 7; // formerly InvolvedShards (never written; cleanup paths key by OperationId) |
There was a problem hiding this comment.
Severity: Nit
Confidence: Low
Changing field 7 from repeated uint64 InvolvedShards to reserved 7 is a forward-compatible proto change. However, existing persisted TLongIncrementalRestoreOp protobufs in the database may still have field 7 populated. The reserved declaration prevents new writes but does not affect reading. Since the old InvolvedShards data was never meaningfully used (per the comment), this is safe, but existing rows with field 7 data will persist until the operation is cleaned up.
| ui64 txId = 100; | ||
|
|
||
| TControlBoard::SetValue(2, runtime.GetAppData().Icb->SchemeShardControls.MaxIncrementalRestoreRetriesPerIncremental); | ||
|
|
There was a problem hiding this comment.
Severity: Major
Confidence: Medium
The test suite covers retry budget enforcement, non-retriable short-circuits, and reboot scenarios, but there is no test that verifies the system state after an incremental restore enters the Failed state. Specifically:
- No test verifies that paths in
EPathStateIncomingIncrementalRestore/EPathStateOutgoingIncrementalRestoreare cleaned up or remain usable afterFailed. - No test verifies that a new restore can be started after a previous one failed.
- No test verifies that the
IncrementalRestoreStatesentry for aFailedoperation does not leak (it is never deleted in the current code).
Without these tests, the Failed state is a dead end that has not been validated in an integration test.
| THashSet<TOperationId> InProgressOperations; | ||
| THashSet<TOperationId> CompletedOperations; | ||
|
|
||
| // Table operation state tracking for DataShard completion |
There was a problem hiding this comment.
Severity: Minor
Confidence: High
MaxRetriesPerIncremental = 5 is defined as a static constexpr but is never used. The actual retry cap comes from IncrementalRestoreSettings.MaxIncrementalRestoreRetriesPerIncremental (ICB control, default 50). This dead constant is misleading — it suggests the cap is 5 when it is actually 50 by default.
Consider removing this unused constant, or adding a comment explaining it is advisory/dead code.
| state.NextRetryAttemptAt = TInstant::Zero(); | ||
| db.Table<Schema::IncrementalRestoreState>().Key(OperationId).Update( | ||
| NIceDb::TUpdate<Schema::IncrementalRestoreState::State>(static_cast<ui32>(state.State)) | ||
| ); |
There was a problem hiding this comment.
Severity: Minor
Confidence: High
When the state transitions to Failed, the entry in IncrementalRestoreStates is persisted but never cleaned up. The IncrementalRestoreShardProgress table row is deleted by the finalize operation, but for Failed state there is no finalize. Over time, IncrementalRestoreStates will accumulate entries for failed restores, consuming memory. The entries are only cleaned up when the backup collection is dropped via CleanupIncrementalRestoreState.
Consider adding cleanup logic for Failed states, or at minimum documenting the lifecycle expectation.
AI Review SummaryVerdict: ❌ 1 critical issue(s) found Critical issues
Other findings
This review was generated automatically. Critical issues require attention; other findings are advisory. |
2 participants
Fixes ydb-platform#22274
Related: ydb-platform#21487, ydb-platform#31068
Changelog entry
...
Changelog category
Description for reviewers
...