Skip to content

worker(dm): keep query status responsive during task operations - #12783

Open
GMHDBJD wants to merge 1 commit into
pingcap:masterfrom
GMHDBJD:fix-12778-query-status-lock
Open

worker(dm): keep query status responsive during task operations#12783
GMHDBJD wants to merge 1 commit into
pingcap:masterfrom
GMHDBJD:fix-12778-query-status-lock

Conversation

@GMHDBJD

@GMHDBJD GMHDBJD commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #12778

A slow or stuck subtask Pause, Stop, or Delete operation holds SourceWorker's write lock while waiting for the current unit to exit. QueryStatus used to block on the matching read lock, so status RPCs timed out and their handlers remained queued even after their contexts were canceled.

What is changed and how it works?

  • Use a non-blocking SourceWorker read-lock attempt in QueryStatus.
  • When the worker lock is busy, return a concurrency-safe snapshot of each subtask's name, stage, and result instead of waiting.
  • Keep the existing detailed source, unit, and relay status path unchanged when the worker lock is immediately available.
  • Preserve the existing SourceWorker write-lock scope so lifecycle operations, worker Stop, and relay/subtask disable operations remain serialized.
  • Add regression tests for blocked Pause, Stop, and Delete, canceled concurrent status calls, and lifecycle-operation serialization.

Check List

Tests

  • Unit test
    • go test -race ./dm/worker -run 'Test(QueryStatusDuringBlockedSubTaskOperation|CanceledQueryStatusDoesNotQueueBehindSubTaskOperation|BlockedSubTaskOperationStillSerializesLifecycleOperations|QueryStatusSourceStatusTimeout)$' -count=1
    • go test -race ./dm/worker -run 'Test(QueryStatusDuringBlockedSubTaskOperation|CanceledQueryStatusDoesNotQueueBehindSubTaskOperation|BlockedSubTaskOperationStillSerializesLifecycleOperations)$' -count=10
    • go test -race ./dm/worker -run 'Test(Check|CheckTaskIndependent)$' -count=1
  • Manual test
    • go vet ./dm/worker
    • make fmt
    • git diff --check

The full go test ./dm/worker -count=1 run encountered the existing testServer.TestHandleSourceBoundAfterError etcd-event timing failure. The same focused gocheck case fails unchanged on upstream/master; the new and affected tests above pass with the race detector.

Questions

Will it cause performance regression or break compatibility?

No protocol compatibility break is expected. The normal detailed QueryStatus path is unchanged. During SourceWorker write-lock contention, QueryStatus now returns a lightweight subtask state snapshot instead of blocking; unit, relay, and live source details are intentionally omitted only in that fallback response.

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

No.

Release note

Fix an issue where DM query-status could time out while a subtask was pausing, stopping, or being deleted.

Summary by CodeRabbit

  • Bug Fixes
    • Status checks now remain responsive while pause, stop, or delete operations are in progress.
    • Status queries return useful transitional subtask information instead of waiting behind blocked operations.
    • Canceled status requests no longer queue unnecessarily.
    • Closed workers and missing subtasks return appropriate empty or not-found results.
    • Improved handling of subtasks without available results.

Avoid queuing QueryStatus behind a SourceWorker write lock held by a blocking subtask lifecycle operation. Return a concurrency-safe snapshot of the subtask name, stage, and result when the detailed worker state cannot be locked immediately.

Keep the existing lifecycle serialization unchanged and add regression coverage for blocked pause, stop, delete, canceled status requests, and concurrent lifecycle operations.
@ti-chi-bot

ti-chi-bot Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 3, 2026
@ti-chi-bot

ti-chi-bot Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign benjamin2037 for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found 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

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

QueryStatus now avoids blocking on long-running subtask lifecycle operations. It returns lock-independent subtask snapshots, preserves transitional states, handles canceled queries, and safely copies nil results. Tests cover pause, stop, delete, cancellation, and serialization behavior.

Changes

Status query responsiveness

Layer / File(s) Summary
Subtask snapshot status path
dm/worker/source_worker.go, dm/worker/status.go
QueryStatus falls back to lock-independent subtask snapshots when the worker lock is unavailable. Snapshots support named and all-subtask queries, preserve name ordering, report not-found status, and safely handle nil results.
Blocked lifecycle regression coverage
dm/worker/source_worker_test.go
Tests verify responsive transitional statuses during pause, stop, and delete operations; canceled queries do not queue behind the lock; lifecycle operations remain serialized; and blocked operations release safely.

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

Poem

I’m a rabbit with status to see,
Pausing and stopping now flow free.
Locks may wait, but queries hop,
Nil results safely know when to stop.
Thump, thump—the tests agree!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy issue #12778 by keeping QueryStatus responsive, reporting transitional stages, handling cancellation, and preserving lifecycle serialization.
Out of Scope Changes check ✅ Passed The implementation and regression tests are directly related to the QueryStatus lock-contention problem described in issue #12778.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly summarizes the main change: keeping DM worker status queries responsive during task operations.
Description check ✅ Passed The description covers the issue, implementation, tests, compatibility impact, documentation needs, and release note.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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

@ti-chi-bot ti-chi-bot Bot added area/dm Issues or PRs related to DM. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 3, 2026
@GMHDBJD
GMHDBJD marked this pull request as ready for review August 3, 2026 10:34
@ti-chi-bot ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/needs-triage-completed and removed do-not-merge/needs-linked-issue do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 3, 2026
@ti-chi-bot

ti-chi-bot Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

@GMHDBJD: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-syncdiff-integration-test cf7b48e link true /test pull-syncdiff-integration-test
pull-dm-integration-test cf7b48e link true /test pull-dm-integration-test
pull-dm-integration-test-next-gen cf7b48e link true /test pull-dm-integration-test-next-gen

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

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

Labels

area/dm Issues or PRs related to DM. do-not-merge/needs-triage-completed release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[DM] QueryStatus is blocked by SourceWorker write lock while a subtask operation is pending

1 participant