Skip to content

domain: warn when multiple TiDB instances advertise the same status endpoint#69899

Open
OliverS929 wants to merge 8 commits into
pingcap:masterfrom
OliverS929:issue-69689-endpoint-uniqueness-warning
Open

domain: warn when multiple TiDB instances advertise the same status endpoint#69899
OliverS929 wants to merge 8 commits into
pingcap:masterfrom
OliverS929:issue-69689-endpoint-uniqueness-warning

Conversation

@OliverS929

@OliverS929 OliverS929 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #69689

Problem Summary:

TiDB publishes each instance's advertised status endpoint in server info, but it does not report when two different TiDB instances publish the same endpoint. This makes duplicate advertise-address and status-port configuration difficult to diagnose.

What changed and how does it work?

TiDB now detects when different TiDB instances advertise the same status endpoint. The conflict produces a structured warning containing both TiDB identities and the duplicated endpoint, without blocking server-info registration or startup.

To make this detection reliable during concurrent startup, each serving primary TiDB tries to create an etcd claim for its normalized advertised endpoint. The claim records the TiDB identity and uses the same lease as its server-info record.

  • An etcd transaction atomically selects one claimant. Another TiDB identity observing the claim reports the conflict instead of overwriting it.
  • When the same TiDB identity restarts its server-info session, it can safely move the claim to the new lease.
  • Cleanup removes the claim only when both the TiDB identity and lease still match, so an old registration cannot remove a newer claim.

In keyspace-enabled deployments, endpoint claims use the same logical-keyspace namespace as server info, so duplicate detection is scoped to one keyspace. CrossKS virtual registrations, the non-serving Domain created during global-variable initialization, and registrations without an enabled, usable status endpoint skip this check. The check is warning-only and does not continuously reconcile claims after the current holder exits.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Manual test:

  1. Started two TiDB instances with different advertised endpoints. Each created its own claim under the corresponding server-info lease, both served SQL, and neither logged a conflict warning.
  2. Started two TiDB instances with different identities but the same advertised endpoint. One claim remained, the conflicting instance logged exactly one warning containing both identities, and both instances remained registered and served SQL. The pre-change binary created no claim or warning under the same topology.
  3. Repeated simultaneous startup three times. Every run produced one claim winner, one warning from the other instance, two server-info records, and two working SQL endpoints.
  4. Tested graceful shutdown and forced termination of the claim holder. Graceful shutdown removed the claim promptly and allowed a replacement to claim the endpoint. After forced termination, the claim and holder server info expired together under the existing 90-second lease while the replacement continued serving SQL.
  5. Revoked a live server-info lease. The Domain keeper restarted the session, and the same TiDB identity moved both its claim and server-info record to a new shared lease without a conflict warning or SQL interruption.
  6. Removed the claim holder while the conflicting TiDB remained running. The remaining TiDB continued serving SQL but did not automatically acquire the empty claim or emit repeated warnings during an observation longer than the periodic server-info sync interval.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

TiDB now logs a warning when another TiDB identity in the same logical keyspace already holds an active claim for the same advertised status endpoint. The warning does not block server-info registration or startup.

Summary by CodeRabbit

  • New Features
    • Added status endpoint claim management with configurable behavior, including an option to disable claim handling for temporary initialization.
  • Bug Fixes
    • Improved best-effort cleanup of server info and associated status claims when storing fails or during removal.
    • Refined status sync loop shutdown/restart gating behavior.
  • Tests
    • Expanded integration and unit coverage for endpoint/key normalization, conflict handling, recovery/reattachment, stale cleanup, and global variable initialization claim skipping.
  • Documentation
    • Updated TiDB test guideline reference maps with new/adjusted test descriptions.

@ti-chi-bot ti-chi-bot Bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 17, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 17, 2026

Copy link
Copy Markdown

[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 d3hunter 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

@ti-chi-bot ti-chi-bot Bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 17, 2026
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The change adds etcd-backed advertised status endpoint claims to server-info synchronization, including ownership, reattachment, cleanup, conflict warnings, and shutdown handling. Domain and session initialization now propagate options that disable claims for temporary domains, with unit and integration coverage.

Changes

Status endpoint claims

Layer / File(s) Summary
Claim contract and configuration
pkg/domain/serverinfo/status_endpoint_claim.go, pkg/domain/serverinfo/syncer.go, pkg/domain/infosync/info.go, pkg/domain/serverinfo/BUILD.bazel
Defines endpoint normalization, deterministic etcd claim keys, claim options, and initialization wiring.
Claim lifecycle and restart handling
pkg/domain/serverinfo/status_endpoint_claim.go, pkg/domain/serverinfo/syncer.go
Adds claim acquisition, reattachment, conflict reporting, ownership-safe removal, failed-registration cleanup, and shutdown-aware restart behavior.
Option propagation and temporary domains
pkg/domain/domain.go, pkg/session/tidb.go, pkg/session/global_init.go, pkg/domain/infosync/info.go
Forwards server-info options through domain initialization and disables endpoint claims during global-variable initialization.
Claim validation and integration coverage
pkg/domain/serverinfo/syncer_test.go, pkg/session/tidb_test.go, pkg/domain/serverinfo/BUILD.bazel, pkg/session/BUILD.bazel, .agents/skills/tidb-test-guidelines/references/*
Adds endpoint, etcd lifecycle, failure-injection, logging, temporary-domain, and synchronization tests with supporting Bazel dependencies and test-map updates.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant TiDBDomain
  participant ServerInfoSyncer
  participant Etcd
  TiDBDomain->>ServerInfoSyncer: initialize with claim options
  ServerInfoSyncer->>Etcd: create, inspect, or reattach endpoint claim
  ServerInfoSyncer->>Etcd: store server information
  ServerInfoSyncer->>Etcd: remove claim during cleanup
Loading

Suggested reviewers: yangkeao, bb7133

Poem

A rabbit hops where endpoints gleam,
Etcd guards each shared address dream.
Claims are checked, then leases tied,
Stale little keys are swept aside.
Temporary domains quietly abstain.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.26% 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
Linked Issues check ✅ Passed The changes implement the requested warning-only duplicate status-endpoint check and its keyspace/session handling.
Out of Scope Changes check ✅ Passed The added tests, docs, and build updates support the endpoint-claim feature and do not appear unrelated.
Title check ✅ Passed The title clearly describes the main change: warning on duplicate advertised status endpoints across TiDB instances.
Description check ✅ Passed The description includes the required issue link, problem summary, change details, tests, side effects, documentation, and release note.
✨ 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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
pkg/domain/serverinfo/syncer_test.go (1)

820-825: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Avoid tight wall-clock upper bounds in integration tests.

The 2–2.5 second ceilings can fail under CI scheduling pauses even when cleanup deadlines and backoff are correct. Prefer the captured deadline/backoff state plus a generous outer test timeout.

As per coding guidelines, test changes should remain minimal and deterministic.

Also applies to: 936-938

🤖 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/domain/serverinfo/syncer_test.go` around lines 820 - 825, Update the
integration tests around syncer.NewSessionAndStoreServerInfo and the
corresponding assertions near the other referenced case to avoid tight
elapsed-time upper bounds. Assert the captured cleanup deadline/backoff state
directly, and use a generous outer test timeout for runaway protection while
preserving the existing error assertion and minimal deterministic coverage.

Source: Coding guidelines

pkg/session/tidb_test.go (1)

88-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Exercise and verify the complete active-claim warning contract.

The seeded key has no lease, so it does not model a live serving claimant. Attach it to a granted lease, then assert the warning includes the endpoint, local identity, and existing-server identity—not only the local ID.

🤖 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/session/tidb_test.go` around lines 88 - 115, Update the active-claim
setup in the test around BootstrapSession to grant an etcd lease and attach the
seeded claim key to that lease, modeling a live claimant. Expand the warning
assertions for “advertised status endpoint already has an active claim” to
verify the endpoint, the local server identity, and the existing
“existing-server” identity in the warning context.
🤖 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.

Inline comments:
In `@pkg/domain/serverinfo/syncer.go`:
- Around line 172-176: Update the host normalization logic around
netip.ParseAddr to skip unspecified IP addresses such as 0.0.0.0 and :: when
constructing claims, rather than adding them as usable endpoints. Preserve the
existing normalized hostname handling and ensure unusable status endpoints do
not participate in duplicate detection.
- Around line 591-593: Update the restart flow around waitForServerInfoRestart
so it checks the server shutdown signal before invoking Restart or registering a
new session/endpoint claim. Ensure s.Done() takes precedence when both s.Done()
and exitCh are ready, while preserving the existing exitCh handling and return
behavior.

---

Nitpick comments:
In `@pkg/domain/serverinfo/syncer_test.go`:
- Around line 820-825: Update the integration tests around
syncer.NewSessionAndStoreServerInfo and the corresponding assertions near the
other referenced case to avoid tight elapsed-time upper bounds. Assert the
captured cleanup deadline/backoff state directly, and use a generous outer test
timeout for runaway protection while preserving the existing error assertion and
minimal deterministic coverage.

In `@pkg/session/tidb_test.go`:
- Around line 88-115: Update the active-claim setup in the test around
BootstrapSession to grant an etcd lease and attach the seeded claim key to that
lease, modeling a live claimant. Expand the warning assertions for “advertised
status endpoint already has an active claim” to verify the endpoint, the local
server identity, and the existing “existing-server” identity in the warning
context.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 03a095c1-04eb-4bb6-bed9-c5e3266d5068

📥 Commits

Reviewing files that changed from the base of the PR and between 57b5f22 and c1707f4.

📒 Files selected for processing (12)
  • .agents/skills/tidb-test-guidelines/references/domain-case-map.md
  • .agents/skills/tidb-test-guidelines/references/session-case-map.md
  • pkg/domain/domain.go
  • pkg/domain/infosync/info.go
  • pkg/domain/serverinfo/BUILD.bazel
  • pkg/domain/serverinfo/info.go
  • pkg/domain/serverinfo/syncer.go
  • pkg/domain/serverinfo/syncer_test.go
  • pkg/session/BUILD.bazel
  • pkg/session/global_init.go
  • pkg/session/tidb.go
  • pkg/session/tidb_test.go

Comment thread pkg/domain/serverinfo/syncer.go Outdated
Comment thread pkg/domain/serverinfo/syncer.go Outdated
@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 16.92308% with 216 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.5334%. Comparing base (12d24af) to head (388e17e).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #69899        +/-   ##
================================================
- Coverage   76.3191%   75.5334%   -0.7858%     
================================================
  Files          2041       2087        +46     
  Lines        559917     583409     +23492     
================================================
+ Hits         427324     440669     +13345     
- Misses       131692     140572      +8880     
- Partials        901       2168      +1267     
Flag Coverage Δ
integration 45.6328% <16.9230%> (+5.9276%) ⬆️

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

Components Coverage Δ
dumpling 60.4471% <ø> (ø)
parser ∅ <ø> (∅)
br 64.0465% <ø> (+1.3252%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@OliverS929
OliverS929 requested review from YangKeao and bb7133 July 17, 2026 05:30
Comment thread pkg/session/global_init.go Outdated
// Note: collate.newCollationEnabled is set to 1 in init(), so if the
// new_collate=false during first bootstrap, they mismatch.
dom, err := domap.GetOrCreateWithFilter(store, systemDBFilter{})
dom, err := domap.getTemporaryWithFilter(store, systemDBFilter{})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporary is not a good term here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed this to describe its actual purpose: the Domain used for global-variable initialization. Updated in 5c5a1d0.

Comment thread pkg/domain/serverinfo/syncer.go Outdated
return storeErr
}

func (s *Syncer) claimStatusEndpoint(ctx context.Context) statusEndpointClaimResult {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

server info syncer already contains a lot of functionality, maybe abstract those endpoint claim related staff into a new struct, or new pkg

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the endpoint claim logic into a private statusEndpointClaim struct in 5c5a1d0.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@pkg/domain/serverinfo/status_endpoint_claim.go`:
- Around line 85-105: Update buildStatusEndpointClaim so the claim key includes
info.Keyspace as a separate path segment in addition to the encoded endpoint.
Preserve the existing endpoint value and validation behavior, and construct the
returned claim path using the keyspace segment plus the endpoint segment.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8ac3be41-dbf3-4823-944d-9fcf1072138d

📥 Commits

Reviewing files that changed from the base of the PR and between defd4ab and 5c5a1d0.

📒 Files selected for processing (9)
  • .agents/skills/tidb-test-guidelines/references/session-case-map.md
  • pkg/domain/infosync/info.go
  • pkg/domain/serverinfo/BUILD.bazel
  • pkg/domain/serverinfo/status_endpoint_claim.go
  • pkg/domain/serverinfo/syncer.go
  • pkg/domain/serverinfo/syncer_test.go
  • pkg/session/global_init.go
  • pkg/session/tidb.go
  • pkg/session/tidb_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
  • .agents/skills/tidb-test-guidelines/references/session-case-map.md
  • pkg/domain/infosync/info.go
  • pkg/session/tidb_test.go
  • pkg/session/tidb.go
  • pkg/domain/serverinfo/syncer_test.go

Comment thread pkg/domain/serverinfo/status_endpoint_claim.go Outdated
@OliverS929
OliverS929 requested a review from D3Hunter July 17, 2026 16:21
@ingress-bot

Copy link
Copy Markdown

🔍 Starting code review for this PR...

@ingress-bot ingress-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.

Summary

  • Total findings: 9
  • Inline comments: 6
  • Summary-only findings (no inline anchor): 3
Findings (highest risk first)

⚠️ [Major] (1)

  1. acquire() duplicates the tryCreate-then-classify block instead of extracting a helper (pkg/domain/serverinfo/status_endpoint_claim.go:130, pkg/domain/serverinfo/status_endpoint_claim.go:158)

🟡 [Minor] (7)

  1. check name hides that it writes/claims the status endpoint in etcd (pkg/domain/serverinfo/status_endpoint_claim.go:109, pkg/domain/serverinfo/syncer.go:144)
  2. Endpoint-claim cleanup logging is copy-pasted across three call sites (pkg/domain/serverinfo/syncer.go:163, pkg/domain/serverinfo/syncer.go:169, pkg/domain/serverinfo/syncer.go:339)
  3. Tests in two packages hardcode the internal status-endpoint-claim etcd key encoding instead of reusing serverinfo helpers/constants (pkg/session/tidb_test.go:91, pkg/session/tidb_test.go:144, pkg/domain/serverinfo/syncer_test.go:171, pkg/domain/serverinfo/syncer_test.go:836, pkg/domain/serverinfo/status_endpoint_claim.go:34)
  4. Endpoint-claim enable parameter renamed to enabled, diverging from reportStatus used at every other hop (pkg/domain/serverinfo/status_endpoint_claim.go:72, pkg/domain/serverinfo/syncer.go:120)
  5. check silently drops the claim result on context cancellation with no comment explaining why (pkg/domain/serverinfo/status_endpoint_claim.go:111)
  6. Stale-server cleanup does not reclaim the new status endpoint claim, causing false duplicate-config warnings after ungraceful restart (pkg/domain/serverinfo/syncer.go:145, pkg/domain/serverinfo/syncer.go:300, pkg/domain/serverinfo/status_endpoint_claim.go:250)
  7. New exit-vs-restart precedence and fixed backoff in ServerInfoSyncLoop are undocumented (pkg/domain/serverinfo/syncer.go:368, pkg/domain/serverinfo/syncer.go:395)

ℹ️ [Info] (1)

  1. Info-syncer restart backoff is a fixed 1s interval with no jitter (pkg/domain/serverinfo/syncer.go:396, pkg/domain/serverinfo/syncer.go:372)

Unanchored findings

⚠️ [Major] (1)

  1. acquire() duplicates the tryCreate-then-classify block instead of extracting a helper
    • Request: Extract a private helper, e.g. classifyTryCreate(ctx, lease) (done bool, result statusEndpointClaimResult), that runs tryCreate and applies the shared classification, returning whether acquire should return immediately. Call it once before reattach and once after, instead of inlining the same 11+ line block twice.

🟡 [Minor] (2)

  1. Endpoint-claim cleanup logging is copy-pasted across three call sites
    • Request: Add a small helper on Syncer or statusEndpointClaim, e.g. logClaimOpFailure(level, msg, lease, action, err), and call it from all three sites instead of repeating the fields-build-then-log pattern.
  2. Tests in two packages hardcode the internal status-endpoint-claim etcd key encoding instead of reusing serverinfo helpers/constants
    • Request: Export a small helper from serverinfo (e.g. reuse statusEndpointClaimTestKey-equivalent logic as an exported test helper, or expose the key-building function) and have tidb_test.go call it instead of re-deriving the literal; replace the raw "/tidb/server/info/" literal with serverinfo.ServerInformationPath.

return endpoint, claimKey
}

func (c *statusEndpointClaim) check(ctx context.Context, lease clientv3.LeaseID) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Minor] check name hides that it writes/claims the status endpoint in etcd

Impact
A maintainer reading NewSessionAndStoreServerInfo or the sync-loop restart path only sees a call to check, described in a comment as best-effort, and can reasonably assume it is a read-only inspection.
If a future change short-circuits or skips check under that assumption (for example a fast path that thinks it is only diagnostic), the actual etcd write that claims this instance's status endpoint is silently lost, and duplicate-status-endpoint detection stops working for that instance.

Scope

  • pkg/domain/serverinfo/status_endpoint_claim.go:109statusEndpointClaim.check
  • pkg/domain/serverinfo/syncer.go:144

Evidence
check calls acquire, which performs a clientv3.Txn(...).Then(clientv3.OpPut(...)) compare-and-swap write to actually claim the endpoint key via tryCreate/reattach, not merely read it. The call-site comment at syncer.go:144 reinforces the misleading framing ("Endpoint claim checks are best-effort"), and the failure state is named statusEndpointClaimCheckFailed even though the underlying operation writes to etcd.

Change request
Rename check to a verb that signals the mutation, e.g. claim or tryClaim, and adjust the accompanying comment and statusEndpointClaimCheckFailed state name so callers can tell at a glance that this call mutates etcd state rather than just inspecting it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed the wrapper to make the claim attempt explicit and adjusted the call-site comment in 388e17e. I kept statusEndpointClaimCheckFailed because it describes the diagnostic result when the claim cannot be read or updated.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ The rename to tryAcquireAndReport plus the reworded call-site comment now make the etcd mutation obvious at the call site, which was the crux here. Retaining statusEndpointClaimCheckFailed for the read-or-update diagnostic result is a fair distinction.

report func(statusEndpointClaimResult)
}

func newStatusEndpointClaim(etcdClient *clientv3.Client, info *ServerInfo, enabled bool) *statusEndpointClaim {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Minor] Endpoint-claim enable parameter renamed to enabled, diverging from reportStatus used at every other hop

Impact
The same boolean concept, whether the syncer should advertise/report its status endpoint, is called reportStatus at the call site in newSyncer and again in buildStatusEndpointClaim, but enabled in newStatusEndpointClaim.
A reader jumping from newStatusEndpointClaim(etcdCli, info, reportStatus) to its definition sees an unrelated, generic parameter name and has to re-derive that enabled means "report the configured status endpoint" rather than some standalone claim-feature toggle.

Scope

  • pkg/domain/serverinfo/status_endpoint_claim.go:72newStatusEndpointClaim
  • pkg/domain/serverinfo/syncer.go:120

Evidence
newSyncer computes reportStatus := config.GetGlobalConfig().Status.ReportStatus && !args.skipStatusEndpointClaim (syncer.go:120) and passes it into newStatusEndpointClaim(etcdCli, info, reportStatus), whose signature names the same value enabled (status_endpoint_claim.go:72), while the function it forwards to, buildStatusEndpointClaim, calls it reportStatus again (status_endpoint_claim.go:85).

Change request
Use one consistent parameter name, e.g. reportStatus, across newStatusEndpointClaim and buildStatusEndpointClaim so the concept reads the same at every call boundary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aligned this path on claimEnabled in 388e17e; it represents both report-status and the explicit claim opt-out.


func (c *statusEndpointClaim) check(ctx context.Context, lease clientv3.LeaseID) {
result := c.acquire(ctx, lease)
if ctx.Err() == nil && c.report != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Minor] check silently drops the claim result on context cancellation with no comment explaining why

Impact
When the caller's context is canceled while acquire is still running, check swallows the result, including a genuine statusEndpointClaimCheckFailed error, instead of reporting it, and there is no comment stating this is intentional.
A future reader modifying check or investigating a missing claim-conflict warning has no signal in the code that this suppression on cancellation is deliberate shutdown-quieting behavior rather than an oversight.

Scope

  • pkg/domain/serverinfo/status_endpoint_claim.go:111

Evidence
check guards the c.report(result) call with ctx.Err() == nil (status_endpoint_claim.go:111); the intent is only documented indirectly by the syncer_test.go case "parent cancellation produces no misleading claim warning", not in the production code itself.

Change request
Add a short comment on the ctx.Err() == nil guard in check stating that reporting is suppressed on cancellation to avoid emitting a misleading claim warning during shutdown/abort.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment in 388e17e explaining that result reporting is suppressed after parent cancellation to avoid a misleading warning during shutdown or abort.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ The guard now carries a comment noting that reporting is skipped once the context is canceled, so the shutdown-quieting behavior is visible in the production code rather than only inferable from the test name.

Comment thread pkg/domain/serverinfo/syncer.go Outdated
return s.StoreServerInfo(ctx)

// Endpoint claim checks are best-effort; conflicts and check errors must not block server info registration.
s.endpointClaim.check(ctx, session.Lease())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Minor] Stale-server cleanup does not reclaim the new status endpoint claim, causing false duplicate-config warnings after ungraceful restart

Impact
After a kill-9/OOM restart, the same TiDB node comes back with a fresh syncer UUID (ddl.GetID() = uuid.New()), so its endpoint-claim check sees the dead predecessor's claim (whose lease survives up to SessionTTL = 90s) as a different-ID owner and reports statusEndpointClaimConflict.
This logs "advertised status endpoint already has an active claim" on a node that is actually the sole owner, undermining the feature's stated purpose of detecting duplicate advertise-address/status-port settings, and the legitimate node never holds the advisory claim during that window.

Scope

  • pkg/domain/serverinfo/syncer.go:145NewSessionAndStoreServerInfo
  • pkg/domain/serverinfo/syncer.go:300cleanupStaleServerAndOwnerInfo
  • pkg/domain/serverinfo/status_endpoint_claim.go:250acquire

Evidence
cleanupStaleServerAndOwnerInfo runs right before s.endpointClaim.check(...) and deletes the stale server-info key and DDL owner key for a same-IP+Port predecessor, but not the stale serverStatusAddressPath claim keyed by the same IP+StatusPort. The claim state machine in acquire/tryCreate treats the leftover observed.id != localID as statusEndpointClaimConflict, which reportResult logs as a warning.

Change request
In cleanupStaleServerAndOwnerInfo, also remove the stale status endpoint claim for each matched dead predecessor (reconstruct its claim key from si.IP+si.StatusPort, or delete by owner ID), so a legitimate crash-restarted node does not emit a spurious duplicate-endpoint conflict and can immediately reclaim the endpoint.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The claim intentionally stays attached to the server-info lease and can remain for up to SessionTTL after an ungraceful exit; the replacement still registers and serves SQL. The legacy stale-server cleanup only has decoded server info and does not retain the old etcd KV lease, so deleting by endpoint or owner ID alone could remove a newer claim. Safe eager cleanup would require additional identity-and-lease fencing and would change the accepted TTL behavior, so I would keep the current cleanup in this PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Makes sense — the stale-cleanup path only carries decoded server info, not the predecessor's etcd lease, so deleting a claim by endpoint or owner ID could clobber a newer owner's claim. Letting it lapse with the lease inside SessionTTL is the safer default; the replacement still registers and serves SQL during that window. Eager fenced cleanup is reasonable as a separate follow-up.

case <-ticker.C:
s.reporter.ReportMinStartTS(store, s.session)
case <-s.Done():
if serverInfoSyncLoopExitRequested(exitCh) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Minor] New exit-vs-restart precedence and fixed backoff in ServerInfoSyncLoop are undocumented

Impact
ServerInfoSyncLoop now calls serverInfoSyncLoopExitRequested right after <-s.Done() fires, even though the same select already has a case <-exitCh: return, and waits a fixed 1s via waitForServerInfoRestart after a failed restart before retrying. Neither the explicit re-check nor the fixed wait is explained, so a future edit to this select (reordering cases, dropping the check) could reintroduce a restart-during-shutdown race with nothing warning against it.

Scope

  • pkg/domain/serverinfo/syncer.go:368ServerInfoSyncLoop
  • pkg/domain/serverinfo/syncer.go:395waitForServerInfoRestart

Evidence
serverInfoSyncLoopExitRequested exists because Go's select picks pseudo-randomly among ready cases, so <-s.Done() firing does not guarantee <-exitCh would have been chosen even if both channels are ready at the same time; that reasoning is not written near the check at syncer.go:368 or near the 1s wait in waitForServerInfoRestart at syncer.go:395.

Change request
Add a brief comment at the serverInfoSyncLoopExitRequested check explaining the select-ordering race it guards against, and a short comment on waitForServerInfoRestart stating the fixed wait exists to avoid a tight restart loop on persistent failures while staying responsive to exitCh.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified in 388e17e why exitCh is rechecked after s.Done() and renamed the wait helper to describe the exit-interruptible restart backoff.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Both spots are now documented — the recheck after s.Done() explains the select race where a ready exitCh may not win, and waitForServerInfoRestartRetry was renamed with a doc comment describing the exit-interruptible backoff. Resolving.

}

func waitForServerInfoRestart(exitCh <-chan struct{}) bool {
timer := time.NewTimer(time.Second)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ [Info] Info-syncer restart backoff is a fixed 1s interval with no jitter

When many TiDB nodes lose their server-info etcd session at the same moment (etcd leader change or a brief partition), every node re-enters this restart loop and retries on the same fixed 1s cadence.
Without jitter the retries stay phase-aligned across nodes, producing synchronized session-grant bursts against etcd during the exact window it is recovering. Blast radius is low (about one attempt per node per second, and NewSession's own internal retries partially desynchronize it), so this is an observation rather than a defect.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fixed one-second wait keeps the outer restart loop to at most one new Restart attempt per second after a failure. NewSession also performs its own retries, which provides some natural desynchronization. This delay was added only to prevent a tight loop, not to define a complete distributed backoff policy, so I would keep jitter out of this PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Agreed this is just a tight-loop guard, not a full distributed backoff policy — one Restart attempt per second, with NewSession's own retries adding some natural desync. Given the low blast radius, leaving jitter out of this PR is fine.

@OliverS929

Copy link
Copy Markdown
Contributor Author

/retest

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

Labels

release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Warn when multiple live TiDB instances advertise the same status endpoint

3 participants