ddl: prevent partial ADD INDEX plans after transient TSO errors

[D3Hunter]

What problem does this PR solve?

Issue Number: close #69789

Problem Summary:

generatePlanForPhysicalTable returned (retryable=true, err=nil) when it
detected discontinuous regions or failed to allocate a timestamp for a subtask.
RunWithRetry treats a nil error as success, so a timestamp failure after an
earlier region batch had been planned could publish only the retained prefix of
the physical-table plan. Propagating the timestamp error alone was insufficient
because metadata from failed attempts remained in the shared result slice and
would be duplicated by the next attempt.

What changed and how does it work?

• Return concrete errors for retryable region-discontinuity and timestamp-allocation failures.
• Build subtask metadata in an attempt-local slice and publish it only when the entire retry attempt succeeds.
• Add deterministic failpoints and regression coverage for both transient failures, including a two-region case that proves a retry returns the complete plan exactly once.
• Use pkg/testkit/testfailpoint for checked failpoint setup and automatic cleanup in the affected tests.

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No need to test

  • I checked and no code files have been changed.

Commands run:

make bazel_prepare
./tools/check/failpoint-go-test.sh pkg/ddl -run '^TestBackfillingScheduler(LocalMode|GlobalSortMode)$' -count=1
make lint
git diff --check

Side effects

• Performance regression: Consumes more CPU
• Performance regression: Consumes more Memory
• Breaking backward compatibility

Documentation

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Fix an issue where distributed `ADD INDEX` could publish an incomplete index after a transient timestamp allocation error.

Summary by CodeRabbit

• Bug Fixes
  • Improved retry behavior during physical-table backfilling when regions become non-continuous, now returning a clear error instead of silently succeeding.
  • Ensured timestamp allocation failures are correctly surfaced during retry processing.
  • Improved consistency of generated subtask metadata across retry attempts.
• Tests
  • Expanded retry coverage for both local and physical-table scenarios, including transient region discontinuity and timestamp allocation failures.
• Documentation
  • Updated DDL test reference notes to reflect the broader retry coverage.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: da8b69d1-4016-49b7-b08c-a84bb05185aa

📥 Commits

Reviewing files that changed from the base of the PR and between baf3ac1 and 81ad8c2.

📒 Files selected for processing (3)

• .agents/skills/tidb-test-guidelines/references/ddl-case-map.md
• pkg/ddl/backfilling_dist_scheduler.go
• pkg/ddl/backfilling_dist_scheduler_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• .agents/skills/tidb-test-guidelines/references/ddl-case-map.md
• pkg/ddl/backfilling_dist_scheduler_test.go

---

📝 Walkthrough

Walkthrough

The physical-table backfill planner now propagates retry errors, rebuilds subtask metadata per attempt, and supports failpoint-driven testing for region discontinuity and timestamp allocation failures. Scheduler tests use the test failpoint helper and validate generated subtask metadata.

Changes

Distributed backfill retry correctness

Layer / File(s)	Summary
Retry-safe physical-table plan generation pkg/ddl/backfilling_dist_scheduler.go	Region discontinuity and timestamp allocation failures now return errors to the retry mechanism, while subtask metadata is accumulated per attempt for both physical-table and temporary-index planning.
Retry regression coverage pkg/ddl/backfilling_dist_scheduler_test.go, .agents/skills/tidb-test-guidelines/references/ddl-case-map.md	Tests use testfailpoint.Enable, cover discontinuity and timestamp allocation retries, verify subtask metadata fields, and document the expanded coverage.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: sig/planner, approved, lgtm

Suggested reviewers: joechenrh, windtalker

Poem

A rabbit hopped through regions wide,
And watched each retry turn the tide.
No partial plans slipped through the door,
Timestamps now rebuild the score.
“Hop-hop!” said Bun, “the ranges are whole!”

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title matches the main change: preventing partial ADD INDEX plans after transient TSO failures.
Description check	✅ Passed	The PR description follows the template and includes the issue number, problem summary, changes, checklist, side effects, and release note.
Linked Issues check	✅ Passed	The code and tests address #69789 by propagating retryable errors and rebuilding plan metadata without retaining partial attempts.
Out of Scope Changes check	✅ Passed	The docs tweak and test helper changes are directly tied to the TSO-failure retry fix, with no unrelated scope visible.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: context deadline exceeded"
level=error msg="Timeout exceeded: try increasing it by passing --timeout option"

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 0% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.8566%. Comparing base (59f6e85) to head (81ad8c2).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #69834        +/-   ##
================================================
- Coverage   76.3196%   75.8566%   -0.4630%     
================================================
  Files          2041       2082        +41     
  Lines        560096     580871     +20775     
================================================
+ Hits         427463     440629     +13166     
- Misses       131732     138085      +6353     
- Partials        901       2157      +1256     

Flag	Coverage Δ
integration	45.8533% <0.0000%> (+6.1480%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	60.4471% <ø> (ø)
parser	∅ <ø> (∅)
br	64.0282% <ø> (+1.3068%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ingress-bot]

🔍 Starting code review for this PR...

[D3Hunter]

/retest

[ingress-bot]

This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.

Summary

• Total findings: 3
• Inline comments: 2
• Summary-only findings (no inline anchor): 0

Findings (highest risk first)

⚠️ [Major] (1)

1. Fix duplicates region-continuity retry logic instead of consolidating with genMergeTempPlanForOneIndex, leaving the twin with the same bug this PR fixes (pkg/ddl/backfilling_dist_scheduler.go:331, pkg/ddl/backfilling_dist_scheduler.go:892)

ℹ️ [Info] (2)

1. New retryable error lacks the root-cause note that motivates retrying at all (pkg/ddl/backfilling_dist_scheduler.go:382)
2. No invariant comment guards the per-attempt attemptMetas buffer this PR's fix depends on (pkg/ddl/backfilling_dist_scheduler.go:420)

[ingress-bot]

ℹ️ [Info] No invariant comment guards the per-attempt attemptMetas buffer this PR's fix depends on

The whole point of this diff (per the commit title, prevent partial add-index plans after transient retries) is that subTaskMetas must only be replaced atomically from a fresh attemptMetas slice on a fully successful attempt, never appended to directly across retries.
That invariant is encoded purely by variable naming/placement (attemptMetas local to the retry closure, subTaskMetas in the outer scope) with no comment marking it, so a future edit that reintroduces direct appends to subTaskMetas inside the retry closure (the exact shape of the bug this PR fixes) would compile cleanly and pass review without an obvious signal.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wjhuang2016

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [wjhuang2016]
• pkg/ddl/OWNERS [wjhuang2016]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2026-07-14 06:41:47.061671711 +0000 UTC m=+695893.097766757: ☑️ agreed by wjhuang2016.