Skip to content

Recovery invariant related to old generation tlogs and remote tlogs #12576

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
spraza wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Recovery invariant related to old generation tlogs and remote tlogs #12576

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
347ab56
Recovery invariant related to old generation tlogs and remote tlogs
spraza Nov 21, 2025
5618b5a
move down
spraza Nov 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions fdbserver/ClusterRecovery.actor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,8 @@ ACTOR Future<Void> trackTlogRecovery(Reference<ClusterRecoveryData> self,
state bool allLogs =
newState.tLogs.size() ==
configuration.expectedLogSets(self->primaryDcId.size() ? self->primaryDcId[0] : Optional<Key>());
state bool finalUpdate = !newState.oldTLogData.size() && allLogs;

state bool finalUpdate = newState.oldTLogData.empty() && allLogs;
TraceEvent("TrackTLogRecovery")
.detail("FinalUpdate", finalUpdate)
.detail("NewState.tlogs", newState.tLogs.size())
Expand All @@ -503,6 +504,19 @@ ACTOR Future<Void> trackTlogRecovery(Reference<ClusterRecoveryData> self,
wait(minRecoveryDuration);
self->logSystem->coreStateWritten(newState);

// When remote tlogs are recruited and caught up, only then we purge the old tlog generation state
// Therefore these invariants should hold:
// allLogs false => newState.oldTLogData non-empty
// newState.oldTLogData empty => allLogs true (contrapositive of above)
// The following line codifies these invariants to be checked in simulation
if (!(allLogs || !newState.oldTLogData.empty())) {
TraceEvent(SevError, "FooRecoveryInvariant1")
.detail("AllLogs", allLogs)
.detail("OldTLogSize", newState.oldTLogData.size())
.detail("NewTLogSize", newState.tLogs.size());
}
Comment on lines +512 to +517
Copy link
Contributor

@jzhou77 jzhou77 Nov 22, 2025

Choose a reason for hiding this comment

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

You have ASSERT_WE_THINK below, so these lines are not needed.

Copy link
Collaborator Author

@spraza spraza Nov 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@jzhou77 Actually this trace was not there originally, I was experimenting by adding logging since I found failures (that's why turned this PR to draft).

Have a look at: https://github.com/apple/foundationdb/pull/12577/files. This is minimal code needed to reproduce the issue. Tests fail pretty quickly with it.

Example failure: fdbserver -r simulation -f /root/src/foundationdb/tests/fast/ConfigIncrement.toml --buggify on --seed 2729610066.

I can see that at accepting_commits, all logs is false but old tlogs is 0. I think that's because this is the first recovery of the cluster (new cluster). But in this case, we set RecoveryCompleteWrittenToCoreState to true while we are not at fully_recovered. Sev40 below. I spot checked more failures and in all cases so far, it's the first recovery of the cluster that seems to break the invariant.

Sev40:

<Event Severity="40" ErrorKind="Unset" Time="10.031027" DateTime="2025-11-22T06:19:17Z" Type="FooRecoveryInvariant1" Machine="[abcd::2:0:1:0]:1" ID="0000000000000000" AllLogs="0" OldTLogSize="0" NewTLogSize="2" FinalUpdate="0" WillBeFullyRecovered="0" CurrRecoveryState="6" RecoveryCompleteWrittenToCoreStateWillBeSetToTrue="1" ThreadID="9766125937575351112" Backtrace="/usr/local/bin/llvm-addr2line -e /root/cnd_build_output/bin/fdbserver -p -C -f -i 0x556d7ef 0x556dae9 0x5567cd4 0x2227f94 0x2227df7 0x2226f90 0x2269ed8 0x226afc9 0x226ec83 0x248ea18 0x248e61e 0x2490313 0x248aeb8 0x248b243 0x248a631 0x248a933 0x1ed45c8 0x1ed423b 0x1ef6ac8 0x2489fc8 0x2477158 0x2476bb2 0x246f528 0x246f321 0x246bfde 0x246c858 0x246c622 0x246fe68 0x246f6d2 0x2470a78 0x2470140 0x247bd48 0x247bbba 0x53141c4 0x5313abc 0x1d84af8 0x541b8b7 0x541b3e0 0x3204d2a 0x7fad024745d0" LogGroup="default" Roles="CC,CD,CP,GP,SS,TL" />

Copy link
Contributor

@jzhou77 jzhou77 Dec 11, 2025

Choose a reason for hiding this comment

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

Yes. For the first generation, there is no previous tlogs, i.e., newState.oldTLogData.empty() is always true. However, here allLogs is not necessarily true. So the assertion should take the generation into account, i.e., with an additional condition that this is not the first generation.

Copy link
Collaborator Author

@spraza spraza Dec 13, 2025

Choose a reason for hiding this comment

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

I tried skipping first generation (ClusterRecoveryData->lastEpochEnd is 0). The invariant still breaks. I looked at one failure and can confirm there were multiple recoveries. Cluster is HA (I see LogRouters being recruited).

Will debug more.

ASSERT_WE_THINK(allLogs || !newState.oldTLogData.empty());

if (self->recoveryReadyForCommits.canBeSet()) {
self->recoveryReadyForCommits.send(Void());
}
Expand All @@ -519,7 +533,7 @@ ACTOR Future<Void> trackTlogRecovery(Reference<ClusterRecoveryData> self,
self->dbgid)
.detail("ActiveGenerations", 1)
.trackLatest(self->clusterRecoveryGenerationsEventHolder->trackingKey);
} else if (!newState.oldTLogData.size() && self->recoveryState < RecoveryState::STORAGE_RECOVERED) {
} else if (newState.oldTLogData.empty() && self->recoveryState < RecoveryState::STORAGE_RECOVERED) {
self->recoveryState = RecoveryState::STORAGE_RECOVERED;
TraceEvent(getRecoveryEventName(ClusterRecoveryEventType::CLUSTER_RECOVERY_STATE_EVENT_NAME).c_str(),
self->dbgid)
Expand Down