Repository navigation
worker(dm): isolate source errors in query status snapshots - #12883
Conversation
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.
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
release-note
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to Repeated status queries now avoid accumulating source errors while preserving saved status; no actionable merge risk remains. Security Architecture Review
Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
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 checks (4 passed)
-
Check name Status Explanation Linked Issues check Issue #12882requires repeated and concurrentQueryStatusresponses to include the saved and current source errors without mutating saved status, earlier responses, or response size. `dm/worker/serv…Out of Scope Changes check The changes are limited to the issue-specific clone in dm/worker/server.goand its regression test indm/worker/server_test.go. The changes do not alter the wire format, gRPC receive limit, or unr…Title check The title clearly and concisely describes the main change: isolating source errors in DM worker query status snapshots. Description check The description includes the issue number, problem statement, implementation details, regression tests, compatibility assessment, documentation assessment, and release note.
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
🧪 Generate unit tests (beta)
-
- Create a new PR
🛠️ Fix failing CI checks 💡
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks each status call,
No saved error grows at all.
Cloned results keep their place,
Old replies retain their face.
Eight quick queries pass in view,
The saved source stays true.
Comment @coderabbitai help to get the list of available commands.
ti-chi-bot
Bot
added
needs-cherry-pick-release-nextgen-202603
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
/retest |
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, joechenrh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
…5361) ### What is changed Revert #4909: remove the `GOPROXY=direct` prefix from the `error-log-review` presubmit jobs, so the go command resolves the tool and its dependencies through the default Go module proxy again. Files (5): - `prow-jobs/pingcap/ticdc/latest-presubmits.yaml` - `prow-jobs/pingcap/tiflash/latest-presubmits.yaml` - `prow-jobs/pingcap/tiflow/latest-presubmits.yaml` - `prow-jobs/tikv/pd/latest-presubmits.yaml` - `prow-jobs/tikv/tikv/latest-presubmits.yaml` ### Why `GOPROXY=direct` makes the go command resolve every dependency from its origin host instead of a module proxy. On 2026-10-13 (~02:00–02:35 UTC) `gopkg.in` returned a transient `502 Proxy Error`, which broke all of these presubmits with: ``` config.go:9:2: unrecognized import path "gopkg.in/yaml.v3": reading https://gopkg.in/yaml.v3?go-get=1: 502 Proxy Error ``` Example failed run: https://prow.tidb.net/view/gs/prow-tidb-logs/pr-logs/pull/pingcap_tiflow/12883/pull-error-log-review/2109830078522200064 Failing runs in that window: pingcap/tiflow#12883, pingcap/ticdc#6466, tikv/pd#11337, tikv/pd#11338, tikv/pd#11349; a later run at 02:36 UTC passed. The error-log-review presubmits are non-optional, so this blocked Tide merge. #4909 added `GOPROXY=direct` so that `go run ...@main` always resolves the latest commit immediately, bypassing module-proxy branch-query caching. Reverting trades that freshness for reliability: right after a push to main, `@main` may briefly resolve to a stale pseudo-version, which is acceptable for this presubmit. ### Validation - `git revert 99b33ce` applies cleanly (auto-merge, no conflicts). - `prow-jobs/kustomization.yaml` uses `disableNameSuffixHash: true`, so no hash regeneration is needed.
|
/retest |
|
In response to a cherrypick label: new pull request created to branch |
|
In response to a cherrypick label: new pull request created to branch |
5 participants
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
QueryStatuscall 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?
getSourceStatusunchanged. InQueryStatus, clone an existingProcessResultonly when appending the current upstream error. This followsthe result-copying pattern already used by
getAllSubTaskStatusand avoidschanging the saved result or an earlier response.
repeated and concurrent queries, checks stable response content/size, and
verifies that the saved status and earlier response remain unchanged.
unchanged. This PR fixes the source-error accumulation; it does not change the
OpenAPI handling of unrelated worker RPC failures.
Check List
Tests
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=10contains three errors instead of the expected two.
go vet ./dm/workermake fmtgit diff --checkQuestions
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
Summary by CodeRabbit