Skip to content

worker(dm): isolate source errors in query status snapshots (#12883) - #12890

Open
ti-chi-bot wants to merge 2 commits into
pingcap:release-nextgen-202609from
ti-chi-bot:cherry-pick-12883-to-release-nextgen-202609
Open

ti-chi-bot wants to merge 2 commits into
pingcap:release-nextgen-202609from
ti-chi-bot:cherry-pick-12883-to-release-nextgen-202609

Conversation

@ti-chi-bot

@ti-chi-bot ti-chi-bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

This is an automated cherry-pick of #12883

What problem does this PR solve?

Issue Number: close #12882

When a worker already has a saved source error and the upstream connection remains
unavailable, each QueryStatus call appends another error to the saved result.
Repeated polling can grow the response beyond the gRPC receive limit, making even
queries for a nonexistent task fail.

What is changed and how it works?

  • Keep getSourceStatus unchanged. In QueryStatus, clone an existing
    ProcessResult only when appending the current upstream error. This follows
    the result-copying pattern already used by getAllSubTaskStatus and avoids
    changing the saved result or an earlier response.
  • Add a regression test for both nil and existing saved results. It exercises
    repeated and concurrent queries, checks stable response content/size, and
    verifies that the saved status and earlier response remain unchanged.
  • Keep the wire format, existing error reporting, and gRPC receive limit
    unchanged. This PR fixes the source-error accumulation; it does not change the
    OpenAPI handling of unrelated worker RPC failures.

Check List

Tests

  • Unit test
    • make dm_unit_test_pkg PKG=github.com/pingcap/tiflow/dm/worker (full package, race detector and failpoints enabled)
    • go test -race ./dm/worker -run '^TestQueryStatusDoesNotAccumulateSourceErrors$' -count=10
    • The regression test fails on the original implementation: the second query
      contains three errors instead of the expected two.
  • Manual test
    • go vet ./dm/worker
    • make fmt
    • git diff --check

Questions

Will it cause performance regression or break compatibility?

No wire-format or configuration compatibility change. Only queries that append
an upstream error to an existing result copy that small result. The normal path
and the nil-result path remain unchanged, while error-list growth and shared
mutation are avoided.

Do you need to update user documentation, design documentation or monitoring documentation?

No.

Release note

Fix an issue where repeated DM status queries could accumulate source errors and exceed the gRPC message size limit when the upstream connection was unavailable.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed query status reporting for missing tasks so repeated and concurrent checks return consistent source-error information. Existing errors are preserved without being duplicated, and checking status no longer changes previously returned results or the stored status. This makes error details more reliable across repeated checks and avoids unexpected changes in reported task status.

Deep-copy the saved source status while holding the server lock so repeated and concurrent queries cannot append errors to the shared result. Cover nil and existing saved errors, stable response size, and unchanged earlier snapshots.
Keep the source status getter unchanged. Clone the saved ProcessResult
only when QueryStatus is about to append the current upstream error,
following the existing result-copying pattern in getAllSubTaskStatus.
@ti-chi-bot ti-chi-bot added area/dm Issues or PRs related to DM. lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/cherry-pick-for-release-nextgen-202609 labels Oct 13, 2026
@ti-chi-bot

ti-chi-bot Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

This cherry pick PR is for a release branch and has not yet been approved by triage owners.
Adding the do-not-merge/cherry-pick-not-approved label.

To merge this cherry pick:

  1. It must be LGTMed and approved by the reviewers firstly.
  2. For pull requests to TiDB-x branches, it must have no failed tests.
  3. AFTER it has lgtm and approved labels, please wait for the cherry-pick merging approval from triage owners.
Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a7cc9dfe-4868-4821-b490-8cfaa76b4fb3
📥 Commits

Reviewing files that changed from the base of the PR and between 22d515b and ae06c6e.

📒 Files selected for processing (2)
  • dm/worker/server.go
  • dm/worker/server_test.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

📝 Walkthrough

QueryStatus now clones an existing saved source result before appending the current source error. Tests verify that repeated and concurrent queries do not change saved status or earlier responses.

Changes: Status query error handling

Layer / File(s) Summary
Clone the saved result before appending errors
dm/worker/server.go, dm/worker/server_test.go
QueryStatus clones an existing source result before appending the current source error. Tests check repeated and concurrent responses and verify that saved status and earlier responses remain unchanged.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to ae06c

The change appears ready to merge after normal checks; no actionable issue remains identified.

Security architecture risk: ⚪ Minimal · up to ae06c

The change prevents status polling from repeatedly adding errors to saved worker state. It preserves the existing endpoint, response format and access paths. No introduced or worsened security risk was identified within the changed behavior.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected availability scope is status reporting for the worker's handled source and task queries. The fix removes polling-induced growth in shared source-error state without adding tenant, service, credential or upstream access.

Trust Boundaries and Controls

  • observed — The caller-supplied task name follows the same worker QueryStatus path as before. The changed branch operates only on returned error snapshots; it introduces no new caller, authorization bypass, sensitive sink or authority transition. This does not establish the security of unchanged access controls.
Pre-merge checks · Passed 4 · Failed 1

Failed Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title clearly identifies the DM worker change: isolating source errors in QueryStatus snapshots. It is concise and directly related to the changes.
Description check Passed The description covers the problem, issue reference, implementation, regression tests, compatibility impact, documentation impact, and release note. It matches the repository template and explains the…
Linked Issues check Passed Issue #12882 is closed. It provides historical context only. No active directly linked issue supplies coding requirements for this pull request.
Out of Scope Changes check Passed The changes stay within the stated QueryStatus fix. The server change clones the saved ProcessResult before appending the current source error. The added test covers repeated, concurrent, nil-result, …
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

🔧 golangci-lint (2.13.2)

golangci-lint (2.13.2) skipped for .golangci.yml: configuration requires version: "2". Migrate older configurations using golangci-lint migrate and review the result; see https://golangci-lint.run/docs/product/migration-guide/.

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

A rabbit checks the status stream,
No growing errors crowd the screen.
Each saved result stays as it was,
New errors join a copy because—
Then quiet queries hop along.

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

@ti-chi-bot

ti-chi-bot Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GMHDBJD

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:

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 ti-chi-bot Bot added the approved label Oct 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved area/dm Issues or PRs related to DM. do-not-merge/cherry-pick-not-approved lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/cherry-pick-for-release-nextgen-202609

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants