Skip to content

coordinator: clarify CheckStatus error handling precedence #5246

Description

@3AceShowHand

What did you do?

While reviewing the table route conflict fix, we found that coordinator/changefeed.Backoff.CheckStatus has two different precedence rules for maintainer-reported errors:

  • fast-fail / unretryable errors are handled before checkpoint progress;
  • retryable errors are only handled after checkpoint progress does not advance.

A deterministic code-level case is:

  1. Backoff.checkpointTs is 1.
  2. Maintainer reports a new MaintainerStatus with CheckpointTs = 2.
  3. The same status also contains a retryable status.Err.

Current code path: CheckStatus observes checkpoint progress first and returns StateNormal, so the retryable error is not surfaced in that call.

The current PR only fixes the narrower table-route fast-fail path by checking unretryable errors before checkpoint progress. The broader retryable-error precedence is intentionally left for a separate change because it affects backoff/recovery semantics.

What did you expect to see?

We should clarify and likely enforce one consistent invariant for CheckStatus:

  • if a maintainer status reports Err, should the error always be handled before considering checkpoint progress?
  • should m.checkpointTs advance when the same status also reports an error?
  • when should a later no-error status be allowed to recover the changefeed from warning to normal?

If the intended invariant is “reported error wins”, then CheckStatus should handle status.Err first and let HandleError classify fast-fail vs retryable errors. The function comment should also be updated so it does not imply retryable errors are ignored whenever checkpoint advances.

What did you see instead?

Today the behavior is split:

  • fast-fail errors can fail the changefeed immediately;
  • retryable errors can be bypassed by checkpoint progress in the same status.

Changing this is not just a one-line bug fix. It affects:

  • when a changefeed enters warning;
  • when isRestarting is set;
  • whether m.checkpointTs advances on an errored status;
  • when a later no-error status recovers the changefeed.

Suggested follow-up tests:

  • retryable error + advanced checkpoint;
  • fast-fail error + advanced checkpoint;
  • recovery from warning only after a later no-error status advances checkpoint.

Related code:

  • coordinator/changefeed/backoff.go
  • coordinator/changefeed/backoff_test.go

Versions of the cluster

This is a code-path issue found during PR review, not an environment-specific runtime report.

Upstream TiDB cluster version (execute SELECT tidb_version(); in a MySQL client):

N/A

Upstream TiKV version (execute tikv-server --version):

N/A

TiCDC version (execute cdc version):

current development branch

Activity

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

Metadata

Metadata

Assignees

Labels

affects-8.5This bug affects the 8.5.x(LTS) versions.severity/moderatetype/bugThe issue is confirmed as a bug.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions