Skip to content

fix(lockservice): recover active txn checks after CN recreation#25651

Merged
XuPeng-SH merged 12 commits into
matrixorigin:mainfrom
XuPeng-SH:fix/lockservice-hot-recreate
Jul 14, 2026
Merged

fix(lockservice): recover active txn checks after CN recreation#25651
XuPeng-SH merged 12 commits into
matrixorigin:mainfrom
XuPeng-SH:fix/lockservice-hot-recreate

Conversation

@XuPeng-SH

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

Fixes #25650

What this PR does / why we need it:

Fixes TN-side lockservice recovery after CN hot recreation without restarting TN.

  • Adds MORPC targeted backend detachment with per-remote epochs, preventing queued or in-flight pre-reset backend creation from repopulating a stale pool.
  • Uses a bounded pure-Go DNS lookup only on the GetActiveTxn recovery path, avoiding stale long-lived CGO hostname resolution while leaving normal lock/transaction hot paths unchanged.
  • Confirms the first successful Valid=false response after resetting the endpoint, preventing a stale IP reassigned to another CN from being treated as an authoritative response.
  • Makes cleanup retries bounded and context-aware; handles timeouts, EOF, network errors, unavailable backends, and cancellation through MORPC error classification.
  • Linearizes Valid commit admission with invalid-CN fencing and protects Resume from stale cleanup re-fencing.
  • Bounds the recovery endpoint cache and preserves public MORPC interface compatibility.

Validation:

  • go test ./pkg/common/morpc -count=1
  • go test ./pkg/lockservice -count=1
  • Focused race tests (-race, repeated 10/20 times)
  • make dev-build with typecheck
  • BenchmarkValidDuringRecoveryChanges: 125-145 ns/op, 0 B/op, 0 allocs/op
  • Docker hot-recreate reproduction: no post-ready 20708 in the observed cleanup window.

@XuPeng-SH XuPeng-SH requested a review from iamlinjunhong as a code owner July 13, 2026 04:34
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@mergify mergify Bot added kind/bug Something isn't working kind/test-ci labels Jul 13, 2026
@matrix-meow matrix-meow added the size/XL Denotes a PR that changes [1000, 1999] lines label Jul 13, 2026
@XuPeng-SH

Copy link
Copy Markdown
Contributor Author

Blocking issue:

  • ResetBackend() is still not actually bounded by its caller context on the cluster-discovery path. It acquires the global recovery-reset slot and then does both pre/post-refresh address lookups via MOCluster.GetCNServiceWithoutWorkingState() (pkg/lockservice/rpc.go), but the built-in cluster implementation waits on readyC without any context check (pkg/clusterservice/cluster.go). So if the cluster has not finished its first refresh yet, or is stuck behind that wait, one ResetBackend call can block indefinitely while holding recoveryResetC. Every later recovery attempt then times out behind it, and cleanup/shutdown can wedge.

Why I think this is a real bug:

  • The PR already added the context-aware helper GetCNServiceWithoutWorkingStateWithContext(...) plus a cancellation test for exactly this startup-wait case in pkg/clusterservice.
  • But ResetBackend() still calls the old context-insensitive method twice.
  • Because the reset slot is acquired before those lookups, this is not just one slow request: it can serialize and starve all later reset attempts too.

Suggestion:

  • Switch both lookupAddress() calls in ResetBackend() to clusterservice.GetCNServiceWithoutWorkingStateWithContext(ctx, ...), so the whole reset path really respects defaultRPCTimeout.
  • Add a lockservice-side regression where the cluster is not ready (or the built-in wait is blocked), and assert ResetBackend() returns on context deadline instead of hanging while holding the reset slot.

@XuPeng-SH

Copy link
Copy Markdown
Contributor Author

合理采纳并已修复,提交 7b18585

根因是 ResetBackend 虽然为 refresh/reset gate 使用了 bounded context,但 pre/post-refresh 两次 snapshot lookup 仍调用 context-insensitive API;built-in cluster 未 ready 时会阻塞在 readyC,并持续占用全局 reset slot。

修复:

  • 两次 lookup 都改用 GetCNServiceWithoutWorkingStateWithContext,并贯穿同一个 reset context;
  • lookup 超时立即返回,且不会执行 DNS、backend close 或 recovery cache mutation;
  • 新增真实 built-in cluster 首次 HAKeeper refresh 被阻塞的回归,验证 deadline 返回、零 backend 副作用,以及失败后全局 reset slot 已释放。

验证:新回归 count=50;相关 race count=10;完整 clusterservice/common-morpc/lockservice 测试、go vet、go build、git diff --check 均通过。分支已 rebase 到最新 main。

@XuPeng-SH

Copy link
Copy Markdown
Contributor Author

已系统性核验并合理采纳,修复提交 308a27b

采纳:

  • RPC queue admission:快路径无额外 timer;仅队列满时进入有界等待,同时监听 request context、session context 和 5s queue deadline。超时通过正常 RPC error response 拒绝单个请求,不返回 handler error,避免关闭共享 MORPC session。
  • 补齐 admission/shutdown ownership:所有拒绝分支 cancel + release pooled request;RPC server 成功停止后 drain 两个 buffered queue,避免遗留 request/CancelFunc。
  • Recovery observability:新增低基数 active_txn_recovery_total 与 rpc_queue_rejected_total;ResetBackend 记录成功/失败、旧/新地址、解析 endpoint、耗时。未把 txn/service ID 放入 metric label。
  • incarnation 前提:补充注释说明完整 lockservice identifier 包含进程启动版本,并依赖编排层先终止旧 listener 再发布 replacement。

Decision log:

  • 部分 CloseBackendFor 失败残留:生产 MORPC 实现先从所有 backend/generation/breaker map 摘除,再关闭 detached backend,且 CloseBackendFor 本身不返回底层 close error;不存在失败后复用旧连接。ResetBackend 失败现已有 warning。
  • 二次 Valid=false:完整 service identifier 含 incarnation;authoritative refresh + 单 IPv4 endpoint + fresh transport 后的 mismatch 表示旧 incarnation 已被替代,保留现有语义。
  • ErrNotSupported fallback:server 未注册方法统一返回该错误码,契约闭合。
  • recovery cache:4096 硬上限且 entry 仅为 hint,随机驱逐只导致重新 discovery/reset,不影响正确性,暂不引入 LRU。

验证:新增 queue/session/drain 测试 count=50;相关 race count=10;完整 clusterservice/common-morpc/lockservice/metric 测试、go vet、go build、git diff --check 全部通过。

@XuPeng-SH

Copy link
Copy Markdown
Contributor Author

Another blocker on the latest head:

  • ResetBackend() now uses the context-aware cluster lookup, so the previous unbounded wait bug is fixed. But it still keeps recoveryBackends[sid] pinned to the old resolved recovery IP until after authoritative refresh and re-lookup succeed. If Refresh(ctx) fails, the function returns early and the stale pinned endpoint is left in place.

Why this matters:

  • The whole point of the reset path is to stop later CheckActiveTxn / GetActiveTxn probes from reusing a known-stale endpoint after CN hot recreation.
  • Right now, a transient control-plane refresh failure preserves exactly that stale override.
  • On the next probe, activeTxnBackend("cn-id", discoveredHostname) can still route to the old cached 10.0.0.1:18101 instead of falling back to the hostname path that may already resolve to the replacement CN.
  • cleanCommitStateOnce() will then keep probing the dead IP and can fence a healthy recreated CN after bounded failures.

There is even a test locking in the stale-pin behavior today: TestResetBackendRejectsFailedDiscoveryRefresh still expects activeTxnBackend(...) to return the old cached endpoint after reset failure.

Suggestion:

  • Clear the cached recovery override before any early return in ResetBackend() (or otherwise force post-failure probes to fall back to the discovered hostname path instead of the old pinned endpoint).
  • Update the refresh-failure test to assert that reset failure preserves “unknown state” without preserving the stale endpoint override.

@XuPeng-SH

Copy link
Copy Markdown
Contributor Author

合理采纳并已修复,提交 5e7326f

这个 blocker 的本质是:refresh 失败必须保持业务状态 unknown,但 route invalidation 必须是单调的;已被负响应证明可疑的 endpoint 不能因为 control-plane 失败而重新变成可用 hint。

修复:

  • ResetBackend 获取 reset slot 后,先原子删除 recoveryBackends[sid],再执行任何可能失败的 discovery/refresh;
  • 立即 detach 旧 cached discovered address 和 pinned endpoint;
  • pre-refresh lookup 成功后,在 authoritative refresh 之前 detach 当前 hostname backend;
  • authoritative refresh、二次 lookup 或 DNS 失败时返回 unknown/error,但不会恢复旧 override;
  • 成功路径同时 detach refreshed hostname 和 newly-resolved endpoint,保证确认请求创建新的 MORPC generation;
  • close error 与 discovery/refresh error 使用 errors.Join 保留完整失败信息。

测试更新:

  • refresh failure:断言旧 hostname/IP 均被 detach,cache miss 回退 hostname,且仍返回 refresh error;
  • startup lookup timeout:即使首次 snapshot 尚未 ready,旧 pin 也先失效;
  • slow refresh:并发 route lookup 立即回退 hostname,不再暴露旧 IP;
  • 覆盖首次 pin、地址变化、close 部分失败、新 resolved endpoint。

验证:ResetBackend count=50;相关 recovery race count=10;完整 clusterservice/common-morpc/lockservice/metric 测试、go vet、go build、git diff --check 均通过。分支已 rebase 到最新 main 78f14f9

@XuPeng-SH

Copy link
Copy Markdown
Contributor Author

补充:main 在上一轮推送后立即新增提交,已再次 rebase 到 b6b91bc;修复提交的新 SHA 为 64cb51d。rebase 后 ResetBackend count=20、相关 race count=5 再次通过。

@aptend

aptend commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

已复审当前 head 64cb51df9b317aaf5438b261dd25183ee3b5d098

此前提出的 correctness/lifecycle blockers 均已闭环:peer CheckActiveTxn recovery、reset 失败保持 unknown、旧 generation create 隔离与有界回收、独立 active-txn transport、circuit-breaker incarnation 隔离、context-aware discovery,以及 stale recovery route 的单调失效。未发现新的 blocker。

本地验证通过:

  • go test ./pkg/common/morpc ./pkg/lockservice ./pkg/clusterservice -count=1
  • 关键 recovery/generation/reset race tests,-race -count=3
  • git diff --check

结论:可以合入。

@aptend aptend left a comment

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.

Re-reviewed current head 64cb51d. Previous blocking findings are resolved, focused tests and race tests pass, and no new blockers were found.

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

Labels

kind/bug Something isn't working kind/test-ci size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: TN lockservice keeps fencing a CN after hot recreation

4 participants