fix cdc resume recovery for running tasks - #26641
Open
jiangxinmeng1 wants to merge 7 commits into
Open
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
requested changes
Aug 4, 2026
XuPeng-SH
left a comment
XuPeng-SH
left a comment
Contributor
There was a problem hiding this comment.
Blocking lifecycle race on exact head be9ef8c:
[P1] Preserve recovery when the executor becomes Failed after the request is admitted. UpdateCDCTask now admits daemon Running -> ResumeRequested, but handling is asynchronous. Before resumeTask.Handle calls the active routine, the existing TableDetector callback can observe the persisted table error and run failTaskForPermanentTableError, which changes the in-memory executor from Running to Failed and unregisters/stops it. The handler then writes the daemon row back to Running and calls this Resume; because the new branch matches only StateRunning and the state machine has no Failed -> Resume transition, TransitionResume fails. handleTask only logs the error and does not retry or restore ResumeRequested. The terminal state is therefore daemon Running + executor Failed + uncleared table error, exactly the recovery command this PR is meant to make reliable.
Please make table-error recovery close both reachable states without converting it to reset-watermark restart semantics, and do not publish daemon Running before recovery succeeds (or restore/retry the request on failure). Add a deterministic barrier test for Running request admission -> TableDetector SetFailed -> resume handling, asserting the request is not lost and processing resumes from the recorded watermark. The PR body also appears to link #26634, while this change addresses #26633.
Q1: existing reader/detector cleanup remains owned. Q2: the admitted recovery control chain currently terminates without a live executor or retry. Q3: no new unbounded accumulation found. Existing focused tests pass 10x and race 3x, but they hold the executor in StateRunning throughout and do not exercise this reachable interleaving.
matrix-meow
added
size/M
XuPeng-SH
requested changes
Aug 4, 2026
XuPeng-SH
left a comment
XuPeng-SH
left a comment
Contributor
There was a problem hiding this comment.
Re-review on exact head 36dc7e5.
The previous lost-request race is closed: ResumeRequested now remains durable until executor recovery succeeds, the failed executor has a resume transition, replacement startup is generation-fenced and bounded, and daemon Running is published with a status/runner CAS only after local readiness.
[P1] Keep the CDC catalog failure admission honest until recovery actually succeeds. There is still a reachable split between the daemon/executor state and mo_cdc_task:
- TableDetector finishes failTaskForPermanentTableError before the user issues RESUME, leaving the daemon row Running, the executor Failed, and mo_cdc_task at state=failed with a task-level err_msg.
- The RESUME request enters onPreUpdateCDCTasks. Every non-pause request is mapped to CDCState_Running, and PrepareUpdateTask changes only state, so this transaction publishes state=running before taskservice invokes ActiveRoutine.Resume while retaining the old err_msg.
- Failed recovery registers a waiter whose expected catalog state is Failed. On successful startup, the failed-to-running update affects zero rows because the request already wrote Running. validateCDCStateTransitionResult accepts that as success solely because state already equals the target, so the task-level err_msg is never cleared. SHOW CDC therefore reports running with the permanent failure after successful recovery.
- The failure side is worse: clear-error failure, scheduler rejection, or startup timeout returns the executor to Failed but does not restore the pre-published catalog state. The daemon correctly stays ResumeRequested and retries, while SHOW CDC can keep reporting Running with no live executor.
I reproduced both outcomes with an exact-head deterministic catalog executor: successful publication left err_msg="permanent table error", and scheduler rejection left executor Failed but catalog Running. These are not timing flakes; they follow directly from the unconditional request-side state update plus the state-only zero-row validation.
Please preserve Failed or use an explicit recovery-admission state until startup owns readiness, then publish state=running and err_msg="" together. On any pre-readiness failure, the durable catalog must remain/return non-running. Add deterministic coverage for TableDetector failure committed before RESUME admission, for both successful recovery and scheduler/timeout failure, while continuing to assert the recorded watermark is preserved.
Q1: replacement ownership, cancellation, detector cleanup, and request retry ownership are closed on this head. Q2: the catalog publication chain can still terminate in a false Running state and can skip the required err_msg side effect. Q3: no new unbounded accumulation found.
Validation: changed focused tests passed 10x; the same set passed under -race 3x. CI is green.
XuPeng-SH
approved these changes
Aug 5, 2026
XuPeng-SH
left a comment
XuPeng-SH
left a comment
Contributor
There was a problem hiding this comment.
Re-review of exact head ff4f164b4fc1747936c2a84d59fd6ada8022064e.
The previous catalog-publication blocker is resolved. RESUME admission now uses one conditional catalog update that cannot overwrite a committed Failed row. Failed recovery keeps that durable state and task-level error until replacement startup succeeds, then publishes Failed -> Running together with err_msg = ''. The zero-affected-row idempotency path now verifies both state and error text, so stale errors or false Running publication are no longer accepted as success.
I traced the concurrent detector-before-admission, detector-concurrent-with-admission, scheduler rejection, startup timeout, retry, and successful replacement paths:
- Q1: the replacement attempt, waiter, reader/detector cleanup, and durable retry remain single-owned and generation-fenced.
- Q2: ResumeRequested remains the retry owner until bounded startup readiness; cancellation and timeout terminate the wait without publishing daemon/catalog Running.
- Q3: waiter and attempt state are removed per generation; no new unbounded accumulation was introduced.
Exact-head validation passed: focused recovery/catalog tests, the same lifecycle set under -race, full pkg/cdc, pkg/frontend, and pkg/taskservice, plus build, vet, and git diff --check.
Approve.
Contributor
Merge Queue Status
Waiting for
All merge conditions
Waiting for any of
All queue conditions
|
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
3 participants
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.
What type of PR is this?
Which issue(s) this PR fixes:
issue #26633
What this PR does / why we need it:
fix cdc resume recovery for running tasks