syncer: isolate flow-only updates from full sync

[rleungx]

What problem does this PR solve?

Issue Number: ref #10719

RegionSyncer retains live Region updates while sending a full snapshot. In
clusters with millions of Regions, high-rate flow-only updates can exhaust the
bounded catch-up history and repeatedly restart full sync.

This supersedes #10914.

What is changed and how does it work?

Classify RegionSyncer updates as flow-only or correctness-critical. While a
full sync retains catch-up records, omit flow-only updates from the history so
they cannot displace metadata, epoch, peer, leader, down/pending peer, or
bucket changes.

When the scheduling microservice is enabled, do not enqueue flow-only updates
into PD RegionSyncer because the scheduling service receives complete Region
heartbeats directly. Outside full-sync retention, monolithic PD continues to
synchronize flow-only updates.

Check List

Tests

• Unit test

Release note

None.

Summary by CodeRabbit

• New Features
  • Improved region synchronization by distinguishing full updates from statistics-only updates, including more precise task selection and batching.
  • Reduced unnecessary synchronization by skipping statistics-only updates during full sync and in microservice mode where applicable.
  • Ensured critical changes are still synchronized reliably even with task coalescing.
• Bug Fixes
  • History replay/notification behavior now triggers only when history records are actually appended.
• Tests
  • Added coverage for stats-only skipping, critical update persistence under coalescing, and correct classification behavior.

[ti-chi-bot]

[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 okjiang 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:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 512db74a-b562-4c25-86e9-35332bcded16

📥 Commits

Reviewing files that changed from the base of the PR and between fed054d and 4076c3f.

📒 Files selected for processing (4)

• pkg/ratelimit/runner.go
• pkg/syncer/history_buffer_test.go
• server/cluster/cluster.go
• server/cluster/cluster_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• pkg/syncer/history_buffer_test.go
• server/cluster/cluster.go

---

📝 Walkthrough

Walkthrough

Changes

Region syncer update flow

Layer / File(s)	Summary
RegionUpdate classification contract pkg/syncer/server.go, server/cluster/cluster.go, server/cluster/cluster_test.go, pkg/ratelimit/runner.go	Region heartbeat notifications use RegionUpdate, classify stats-only versus synchronization-critical changes, route them through distinct tasks, and apply scheduling-mode filtering.
Classified history recording and broadcast pkg/syncer/history_buffer.go, pkg/syncer/history_buffer_test.go, pkg/syncer/server.go	The syncer batches updates, excludes stats-only entries during retained history, records eligible updates, and broadcasts only when records are appended.

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

Suggested reviewers: okjiang, lhy1024

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ 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%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately summarizes the main change: isolating flow-only updates from full sync.
Description check	✅ Passed	The PR description follows the template with problem statement, issue number, change summary, tests, and release note.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

pkg/syncer/metrics.go (1)

212-248: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Unchecked map lookups in metric helpers can panic on an unregistered key.

incHistoryBufferMissMetrics/incStreamEventMetrics (and the regionSyncerFullSyncLastDurationGauges[result] lookup in observeFullSyncMetrics) index their maps directly and call .Inc()/.Set() without an existence check. All current call sites use the pre-registered constants, so there's no live bug today, but any future new label value added at a call site without a matching entry in initRegionSyncerMetrics would return a nil prometheus.Counter/Gauge and panic, crashing PD from a hot heartbeat/sync path. observeFullSyncMetrics's own counter lookup already demonstrates the safer comma-ok + fallback pattern used elsewhere in this same function.

🛡️ Proposed defensive guard

 func incHistoryBufferMissMetrics(phase string) {
-	regionSyncerHistoryBufferMissCounters[phase].Inc()
+	if counter, ok := regionSyncerHistoryBufferMissCounters[phase]; ok {
+		counter.Inc()
+	}
 }
 
 func incStreamEventMetrics(event string) {
-	regionSyncerStreamEventCounters[event].Inc()
+	if counter, ok := regionSyncerStreamEventCounters[event]; ok {
+		counter.Inc()
+	}
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/syncer/metrics.go` around lines 212 - 248, Guard metric map lookups
before calling methods: update observeFullSyncMetrics for
regionSyncerFullSyncLastDurationGauges[result], incHistoryBufferMissMetrics for
regionSyncerHistoryBufferMissCounters[phase], and incStreamEventMetrics for
regionSyncerStreamEventCounters[event] to use comma-ok checks and skip updates
when keys are unregistered. Preserve the existing registered-key behavior and
full-sync counter fallback.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@pkg/syncer/metrics.go`:
- Around line 212-248: Guard metric map lookups before calling methods: update
observeFullSyncMetrics for regionSyncerFullSyncLastDurationGauges[result],
incHistoryBufferMissMetrics for regionSyncerHistoryBufferMissCounters[phase],
and incStreamEventMetrics for regionSyncerStreamEventCounters[event] to use
comma-ok checks and skip updates when keys are unregistered. Preserve the
existing registered-key behavior and full-sync counter fallback.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 397875fe-1ba9-424a-904a-978669a11c6f

📥 Commits

Reviewing files that changed from the base of the PR and between 276877c and b3d1663.

📒 Files selected for processing (9)

• metrics/grafana/pd.json
• pkg/syncer/client.go
• pkg/syncer/history_buffer.go
• pkg/syncer/history_buffer_test.go
• pkg/syncer/metrics.go
• pkg/syncer/server.go
• pkg/syncer/server_test.go
• server/cluster/cluster.go
• server/cluster/cluster_test.go

[codecov]

Codecov Report

❌ Patch coverage is 94.20290% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.26%. Comparing base (070828c) to head (4076c3f).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11058      +/-   ##
==========================================
+ Coverage   79.23%   79.26%   +0.03%     
==========================================
  Files         540      541       +1     
  Lines       76010    76404     +394     
==========================================
+ Hits        60226    60563     +337     
- Misses      11533    11567      +34     
- Partials     4251     4274      +23     

Flag	Coverage Δ
unittests	79.26% <94.20%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[okJiang]

Potential correctness regression during full sync: StatsOnly classification is not preserved correctly across syncRegionRunner task coalescing.

ConcurrentRunner.RunTask replaces a pending task's closure when another task with the same Region ID and task name arrives. This creates the following sequence:

1. A full sync captures the old Region in its snapshot.
2. A correctness-critical update (for example, a leader, epoch, peer, down/pending peer, or bucket change) updates the cache and queues a sync task.
3. Before that task runs, a flow-only heartbeat for the same Region arrives. Relative to the already-updated cache, prepareRegionUpdateForSync classifies this latest Region as StatsOnly.
4. The runner replaces the pending critical task with the flow-only task. Although the replacement still carries the complete latest RegionInfo, recordUpdatesFrom drops it while full-sync retention is active.

The follower then receives the critical change from neither the snapshot nor catch-up history. Before this PR, the coalesced latest RegionInfo was still recorded, so this loss path did not exist.

Could we make criticality sticky when tasks are coalesced, or classify against the last state actually enqueued/recorded rather than the current cache? A regression test covering critical update -> flow-only update -> pending-task replacement during full sync would also be helpful.

[ti-chi-bot]

@rleungx: The following test 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-unit-test-next-gen-3	4076c3f	link	true	/test pull-unit-test-next-gen-3

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.

[okJiang]

The separate task names fix the original critical -> stats-only replacement issue, but they introduce a cross-class ordering regression.

ConcurrentRunner.RunTask replaces a matching pending task's closure in place without moving that task to the tail. With a sufficiently backed-up runner, consider this sequence for one Region:

1. Critical update C1 is pending under SyncRegionToFollower.
2. Stats-only update S1 is pending under SyncRegionStatsToFollower.
3. A newer critical update C2 arrives and replaces C1 at C1's original queue position.

The resulting execution order is C2 -> S1, even though the arrival order was C1 -> S1 -> C2. Outside full-sync retention both records are appended to history. Because S1 contains the Region state from before C2, the follower can accept and install it after C2 when the epoch/term is unchanged, rolling back newer leader, bucket, or statistics state.

I reproduced the C2 -> S1 execution order with the real ConcurrentRunner. The new coalescingTaskRunner test does not cover this: it stores tasks in a map and manually invokes the critical task before the stats task, so it cannot model FIFO positions or in-place replacement.

Could we keep a single ordering/coalescing domain and merge pending updates using the latest full RegionInfo while making criticality sticky, or otherwise preserve global arrival order across both classes? Please also add a real-runner regression test for C1 -> S1 -> C2.