Skip to content

Make ShardsAffectedByTeamFailure::moveShard range-safe - #13787

Draft
saintstack wants to merge 1 commit into
apple:mainfrom
saintstack:dd-reconcile-shards-on-exclude
Draft

Make ShardsAffectedByTeamFailure::moveShard range-safe#13787
saintstack wants to merge 1 commit into
apple:mainfrom
saintstack:dd-reconcile-shards-on-exclude

Conversation

@saintstack

@saintstack saintstack commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

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
saintstack marked this pull request as draft July 24, 2026 20:36
@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@saintstack
saintstack requested a review from gxglass July 24, 2026 23:37
@saintstack
saintstack force-pushed the dd-reconcile-shards-on-exclude branch from 4d6f735 to e64153b Compare July 27, 2026 01:09
@saintstack saintstack changed the title Reconcile stranded ShardsAffectedByTeamFailure count on graceful exclude Make ShardsAffectedByTeamFailure::moveShard range-safe Jul 27, 2026
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
saintstack force-pushed the dd-reconcile-shards-on-exclude branch from e64153b to e459167 Compare July 27, 2026 01:19
@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-ide on Linux RHEL 9

  • Commit ID: e459167
  • Duration 0:24:06
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-arm on Linux RHEL 9

  • Commit ID: e459167
  • Duration 0:46:48
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos-m1 on macOS 14.x

  • Commit ID: e459167
  • Duration 0:52:31
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux RHEL 9

  • Commit ID: e459167
  • Duration 0:55:56
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang on Linux RHEL 9

  • Commit ID: e459167
  • Duration 1:01:36
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

  • Commit ID: e459167
  • Duration 1:28:23
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos on macOS 14.x

  • Commit ID: e459167
  • Duration 1:32:51
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants