Make ShardsAffectedByTeamFailure::moveShard range-safe - #13787
Draft
saintstack wants to merge 1 commit into
Draft
Conversation
saintstack
marked this pull request as draft
July 24, 2026 20:36
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
saintstack
force-pushed
the
dd-reconcile-shards-on-exclude
branch
from
July 27, 2026 01:09
4d6f735 to
e64153b
Compare
Lowering DD_MAX_PIPELINE_MOVES surfaced graceful excludes that hung for hours, and in one case indefinitely, *after* the excluded machine's data had already drained on schedule. Raising the cap made it go away. The cap is not the defect though: it changes how often the defect is hit. In principle the same stranding can happen with the cap wide open -- DDQueue produces partial-range moves on its own -- but that has not been demonstrated, so treat the cap as the trigger we have evidence for and the accounting bug below as the cause. The removal gate (waitForAllDataRemoved) needs both canRemoveStorageServer(), which reads the authoritative on-disk serverKeys, and getNumberOfShards() == 0, which reads the in-memory ShardsAffectedByTeamFailure map. They can disagree, and then the server is never removed and `fdbcli exclude` never returns. They disagree because moveShard() is not range-safe. When the move's key range fully covers a tracked range it erases the source teams; when it only partially covers one it appends the destination and never erases, and never splits the tracked range at the move boundary. erase() holds the only decrement of the per-server counter, so the drained servers stay counted until a later fully-covering move, a defineShard(), or a DD restart rebuilds the map from disk. Partial-range moves are routine: a queued relocation's keys go stale when a shard merge coarsens the tracked range underneath it, queueRelocation truncates queued relocations against overlapping newer ones, and launchQueuedWork launches the boundary fragments getRangesAffectedByInsertion returns, which are strict subsets of an older relocation's range by construction. A low cap holds requests outside DDQueue where its supersede logic cannot correct them, which is why a small cap raises the rate. Fix: split the tracked range at the move's boundaries first, so every affected range is fully contained and the source teams are erased for exactly the range that moved. cancelMove() already did this identical split for the same reason; factor it out as splitTrackedShardsAtBoundaries() and share it. Boundaries are added only where one is missing and the next defineShard() merges them back. Gated by DD_SPLIT_TRACKED_SHARDS_ON_MOVE (default true) since this is a data distribution hot path; the partial-overlap branch stays as a safety net with ASSERT_WE_THINK rather than becoming an assert, because dropping a destination team would be worse than over-counting a source. Also enforce at the removal gate the invariant DDTeamCollection.actor.cpp:5964 documents as a commented-out ASSERT and nothing checks: when the on-disk state says the server is empty, no team containing it should remain in the map. If one does, reconcile to the on-disk truth and trace loudly instead of hanging. Gated by DD_RECONCILE_SHARDS_ON_EXCLUDE (default true). The new scrubServer() drops the whole team reference rather than reusing removeFailedServerForRange(), whose helper edits team membership in place and would leave a shrunken team that exists nowhere in DDTeamCollection -- which teamTracker() then re-relocates forever at PRIORITY_TEAM_REDUNDANT. This was undiagnosable in production: waitForAllDataRemoved's trace was SevVerbose and the map's erase/insert traces are DisabledTraceEvent, so a server sat empty and registered for 33 hours with no signal. Add a repeating SevWarnAlways when canRemove is true but the count is not zero. SAF::check() cannot catch this on its own -- the two halves of the map stay mutually consistent and only the semantics are wrong. Tests cover a sub-range move erasing the source team for the moved half and retaining it for the other, the scrub leaving the range on the real replacement team, and the degenerate scrub paths. Simulation test excludes a machine with the cap pinned small. Validated on Joshua with 100k- and 10k-run correctness ensembles, both clean; the boundary split touches every data move, so a broad corpus run is the signal that matters. Known risk: the split adds tracked-range entries on a hot path. Growth should be bounded, but the steady-state count has not been measured at scale.
saintstack
force-pushed
the
dd-reconcile-shards-on-exclude
branch
from
July 27, 2026 01:19
e64153b to
e459167
Compare
This comment has been minimized.
This comment has been minimized.
Contributor
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
Contributor
Result of foundationdb-pr on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-clang on Linux RHEL 9
|
This comment has been minimized.
This comment has been minimized.
Contributor
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-macos on macOS 14.x
|
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
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.
Lowering DD_MAX_PIPELINE_MOVES surfaced graceful excludes that hung for hours,
and in one case indefinitely, after the excluded machine's data had already
drained on schedule. Raising the cap made it go away. The cap is not the defect
though: it changes how often the defect is hit. In principle the same stranding
can happen with the cap wide open -- DDQueue produces partial-range moves on its
own -- but that has not been demonstrated, so treat the cap as the trigger we
have evidence for and the accounting bug below as the cause.
The removal gate (waitForAllDataRemoved) needs both canRemoveStorageServer(),
which reads the authoritative on-disk serverKeys, and getNumberOfShards() == 0,
which reads the in-memory ShardsAffectedByTeamFailure map. They can disagree,
and then the server is never removed and
fdbcli excludenever returns.They disagree because moveShard() is not range-safe. When the move's key range
fully covers a tracked range it erases the source teams; when it only partially
covers one it appends the destination and never erases, and never splits the
tracked range at the move boundary. erase() holds the only decrement of the
per-server counter, so the drained servers stay counted until a later
fully-covering move, a defineShard(), or a DD restart rebuilds the map from disk.
Partial-range moves are routine: a queued relocation's keys go stale when a shard
merge coarsens the tracked range underneath it, queueRelocation truncates queued
relocations against overlapping newer ones, and launchQueuedWork launches the
boundary fragments getRangesAffectedByInsertion returns, which are strict subsets
of an older relocation's range by construction. A low cap holds requests outside
DDQueue where its supersede logic cannot correct them, which is why a small cap
raises the rate.
Fix: split the tracked range at the move's boundaries first, so every affected
range is fully contained and the source teams are erased for exactly the range
that moved. cancelMove() already did this identical split for the same reason;
factor it out as splitTrackedShardsAtBoundaries() and share it. Boundaries are
added only where one is missing and the next defineShard() merges them back.
Gated by DD_SPLIT_TRACKED_SHARDS_ON_MOVE (default true) since this is a data
distribution hot path; the partial-overlap branch stays as a safety net with
ASSERT_WE_THINK rather than becoming an assert, because dropping a destination
team would be worse than over-counting a source.
Also enforce at the removal gate the invariant DDTeamCollection.actor.cpp:5964
documents as a commented-out ASSERT and nothing checks: when the on-disk state
says the server is empty, no team containing it should remain in the map. If one
does, reconcile to the on-disk truth and trace loudly instead of hanging. Gated
by DD_RECONCILE_SHARDS_ON_EXCLUDE (default true). The new scrubServer() drops the
whole team reference rather than reusing removeFailedServerForRange(), whose
helper edits team membership in place and would leave a shrunken team that exists
nowhere in DDTeamCollection -- which teamTracker() then re-relocates forever at
PRIORITY_TEAM_REDUNDANT.
This was undiagnosable in production: waitForAllDataRemoved's trace was
SevVerbose and the map's erase/insert traces are DisabledTraceEvent, so a server
sat empty and registered for 33 hours with no signal. Add a repeating
SevWarnAlways when canRemove is true but the count is not zero. SAF::check()
cannot catch this on its own -- the two halves of the map stay mutually
consistent and only the semantics are wrong.
Tests cover a sub-range move erasing the source team for the moved half and
retaining it for the other, the scrub leaving the range on the real replacement
team, and the degenerate scrub paths. Simulation test excludes a machine with the
cap pinned small. Validated on Joshua with 100k- and 10k-run correctness
ensembles, both clean; the boundary split touches every data move, so a broad
corpus run is the signal that matters.
Known risk: the split adds tracked-range entries on a hot path. Growth should be
bounded, but the steady-state count has not been measured at scale.