fix(lockservice): bind CN drain completion to service instance

[VioletQwQ-0]

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #29477 (kernel-side work; keep open for Operator/cloud validation and QA).

Related: matrixorigin/matrixone-operator#623 (no automatic closure).
Companion Operator PR: matrixone-operator#624.

What this PR does / why we need it:

A CN can have no client sessions while another CN still holds locks through its lock service. Normal eviction must not treat an allocator lookup miss or a UUID match to another incarnation as safe completion. This PR adds a separate instance-bound BeginDrain/QueryDrain protocol. The running CN supplies its exact lock-service ID; the allocator accepts a particular attempt and returns its identity/epoch; completion is valid only for that tuple. Missing/ambiguous state, replacement, allocator restart, and stale attempts fail closed. Repeated requests and delayed heartbeats cannot regress a completed drain.

BeginDrain/QueryDrain use MORPC v102 and wire methods 23/24. Upstream main allocated v100 to view metadata and v101 to JSON source-domain semantics; both are preserved, with drain confirmation allocated the next unique version (v102). Legacy methods retain their meaning and are not a fallback for this safety protocol. IdentityOnly returns CN and lock-service identity without enumerating locks; legacy requests still return the actual lock list. Companion Operator integration is tracked separately in matrixorigin/matrixone-operator#624; this update does not change or validate its dependency pin or lifecycle behavior.

Kernel CR repair at b8a96a9

• Preserve the existing two-layer defense: a CN already draining at handler admission returns ErrNewTxnInCNRollingRestart; allocation invalidated after admission returns ErrLockTableBindChanged. Existing bounded retry may retry the latter, but a subsequent drain rejection terminates. No new retry mechanism or state-machine/wire change.
• Reject invalid, ownerless, wrong-group or wrong-table allocator responses before publication. Valid local/remote owners and physical ByRow bindings remain accepted. The service regression verifies no cache pollution, removal/closure of allocation state, waiter exit, and subsequent successful publication.
• An instance-local, default-nil test callback controls the real handler between admission and Get without holding allocator locks. Channel barriers cover BeginDrain on new/existing binds, legacy restart, safe retirement and a no-drain control. Retirement proof cannot be revoked or its instance resurrected by the late request.
• Real canonical NewClient.Send tests now prove v99/v100/v101 reject both methods before server dispatch, while v102 traverses the allocator server, returns the exact proof, observes incomplete/completed drain, rejects mismatched proof fields and exits on cancellation. This is a capability test in one process, not a mixed-binary test.
• Identity tests use one actual held lock: IdentityOnly never calls IterLocks, and the legacy response includes table/key/mode/holder. Retry tests cover bind-change success, bind-change followed by terminal drain rejection, cancellation and the existing retry budget.
• The two refreshed-binding fixtures retain Valid=true. Upstream main b9013dc was already merged by da0370e to address the unrelated historical TAE SCA/coverage failures.

Allocator-loss recovery at 7c3a8d7

• Address the CHANGES_REQUESTED case: after allocator A loses volatile state, a CN already in Waiting/UnLockSucc/CanRestart can re-handshake with allocator B without returning to Enable. B keeps the exact service bind pending and non-admitting until the CN echoes B's allocator ID/version in a subsequent heartbeat. A stale-epoch or transaction-bearing terminal heartbeat cannot clear pending state.
• Repeated BeginDrain for the same service/attempt is idempotent; a different attempt or epoch cannot adopt an old completion. The CN's existing local checks still govern in-flight operations, remote holders and binding migration. See the allocator-loss recovery contract for the state sequence, safety invariant and mixed-version limit.
• Merge current main without overwriting its MORPC v100 assignment for view metadata. The drain RPCs require v101; v99/v100 are rejected before dispatch.

Validation

Local validation (Go 1.26.4, darwin/arm64, repository mo-cgo-test wrapper and matching native dependencies):

• PASS: 16 focused lockservice tests; real handler interleavings, binding validation, allocation cleanup, canonical RPC and the corrected refreshed-binding fixtures.
• PASS: owning lockservice package in CI-equivalent -short -tags matrixone_test -cover -count=1 mode, 646 top-level tests, 139.036s. Owning package -race -short -tags matrixone_test -count=1, 646 tests, 180.771s.
• PASS: individual test repetition under race detection: TestGetBindDrainInterleaving ×75; TestGetBindInvalidResponseReleasesAllocation ×100; TestInstanceBoundDrainCanonicalClient ×100. Counts use measured first-run time, a 30s budget and a 100-run cap.
• PASS: final-content cnservice package, 135 tests/13.183s, and lockop package, 139 tests/9.637s, normal -short -tags matrixone_test -count=1. Focused identity and seven retry-contract tests also passed normally.
• PASS: final-content standard vet, formatting and diff check. Repository-pinned molint exited 0 with no new diagnostics; its existing warnings are in unchanged lockservice/test_helper.go:50 and txn_test.go:22. PASS: incremental golangci-lint v2.6.2 (three owning packages, --new-from-rev=da0370e0), zero issues. Cold loading/analysis exceeded the initial limit; the same check completed from its warmed cache without disabling rules or changing code.
• EXPECTED_FAIL: applying the same deterministic handler regression plus scheduling-only hook to old review head c742b6a6 fails all four drain/retirement cells, while the no-drain control passes. The diagnostic confirms error=nil, Valid=false, owner empty, table=0.
• Recorded first-run failure: the initial full-package race run hit unchanged TestUnknownCommitFenceFrontierCollapsesAtBound (1.11s) because its overflow fence expires at test-start+1s. No race report occurred. An isolated pre-repair-source overlay passed in 0.54s; after compilation pressure subsided, the entire package passed and this case took 0.14s. No test assertions or expiry thresholds were changed.
• Initial native-preflight and lint-loading environment blocks were resolved with matching native builds and repository-pinned tooling compiled by Go1.26.4. Final-head CI and human re-review are reported separately; pending/skipped checks are not PASS.

Reproducible selections, source/log SHA-256 values, modes and outcomes are preserved locally in /tmp/pr29291-cr-evidence.md and /tmp/pr29291-cr-manifest.json, with individual /tmp/pr29291-*.jsonl logs. Evidence collected before the upstream TAE merge is reused only for lockservice, whose package/dependency inputs are unchanged; consumer package checks and final vet/molint use the final content.

Final-head validation for 7c3a8d72efd10b0689b89bae2cb3b3c5f08a39fc (Go 1.26.4, darwin/arm64, CGO_ENABLED=1, GOWORK=off):

• PASS: matching native C library rebuilt in this worktree; mo-native-provenance verify succeeded. cgo/libmo.dylib SHA-256: edddc13eec83f945ed135ac611f644736475a7e8de3b1543762b5e2380774a13.
• PASS: .agents/skills/mo-dev/scripts/mo-cgo-test -count=1 ./pkg/lockservice and -race -count=1 ./pkg/lockservice on the final head (104.818s and 110.286s); GOWORK=off go vet ./pkg/lockservice; git diff upstream/main...HEAD --check.
• PASS: TestInstanceBoundDrainResumesAfterAllocatorLoss, terminal re-handshake cases, and TestLiveRemoteLockDrainResumesAfterAllocatorLoss. The latter uses two real lock services, an active remote holder, allocator-state loss, old-epoch rejection, Waiting re-handshake, and safe completion only after Unlock. The focused recovery selection passed 20 consecutive executions without a fixed sleep.
• PASS: final-head ./pkg/cnservice and ./pkg/sql/colexec/lockop package tests with the matching native library (13.227s and 8.262s).
• EXPECTED_FAIL: the new recovery regression failed on the pre-fix implementation because a live draining CN could not re-handshake. This is a deterministic in-process allocator-state-loss test, not a real allocator process restart, mixed-binary test, Operator/Kruise eviction, or cloud release validation. Final-head CI and reviewer re-approval are separate gates.

BVT: N/A for admission/drain and eviction timing: static SQL cannot control the handler admission/allocator transition window. Deterministic handler, service and real-client RPC tests provide the regression evidence. cn_safe_drain_dml.sql/.result are supplementary transaction-outcome coverage, not an eviction regression; no new SQL execution is claimed in this update.

Separate acceptance gates

QA required: yes. Kernel reviewer re-approval and final-head CI remain gates. Real allocator process restart, old/new CN/TN/Operator binary combinations, actual Kruise/cloud eviction, acquisition-in-flight/late-unlock process interleavings, timeout recovery, and post-retirement business retry remain separate validation. No merge, incident closure or cloud acceptance is claimed.

Historical evidence (not rerun for this update)

Previous head c742b6a6f1ec5fafde970f597c2d4823f42e2ab0 verification on mo-55 (Go 1.26.6, isolated directory /home/sunyuze/cn-safe-drain-verify-20260924-wgALG4):

• The mo-service binary was built from a git archive of this exact head; SHA-256 8e21b2b23a4240c5c312b297f6f5edca5acd3a80f5196e7316d4fa880e3d909e. Native dependencies came from the previous build of unchanged C sources. Build log: evidence/build-mo-service-v4.log.
• A real standalone LogService/TN/CN A/CN B cluster used fresh local data. CN A created the test table, then its SQL client was closed. CN B's lock log showed remote=true for that table (272565) with the exact CN A lock-service ID in the binding; the TN log maps table 272565 to the test database/table.
• CN B executed INSERT and ROLLBACK (no residue), then held a transaction performing UPDATE, INSERT, and DELETE. BeginDrain for CN A returned an instance/attempt/allocator-epoch proof; QueryDrain was safe=false while B held the transaction. After B's COMMIT it became safe=true; committed result was count 2, sum 41.
• CN A was signalled only after the safe proof. CN B's first subsequent INSERT returned an explicit lock table bind changed retryable error; a separate second attempt succeeded. Final result was count 3, sum 91. No client-visible connection refusal or unknown commit was observed in this test. Internal CN B logs did record a refused connection to the retired CN during stale-binding recovery; this is not claimed as a seamless/no-retry migration.
• Test harness: dual_cn_verify.go (SHA-256 d541829e5dead53c5618f7c65aeb84ae8480cd21d305a7b656ec8f347b5d67f5). Raw output: evidence/dual-cn-current-head-v2.log (SHA-256 f042434a7ecaabd0853d5332f920351083ee0cf10f17bf66eb5bb189a9416938); service logs: run/logs/{logservice,tnservice,cna,cnb}.log. The test-only processes are stopped; data and logs are preserved. This is a PASS for the held-remote-lock protocol scenario, not for Operator/Kruise eviction or cloud release.

Main conflict resolution at b350c0c

• Merged main 76862df5c89e8868bed5963c428f65b9eafb34b3; regenerated lock bindings with protoc 3.21.1 / gogofast from the merged schema, retaining upstream KeepRows and the drain/allocator-epoch fields.
• Preserved upstream MORPC v101 and moved both drain method gates to v102. Regression includes canonical-client rejection at v101 before transport and acceptance at v102.
• Local PASS (GOWORK=off, CGO_ENABLED=1, matching native libraries): complete ./pkg/lockservice ./pkg/pb/lock ./pkg/defines tests, complete lockservice -race -count=1, and ./pkg/sql/colexec/lockop ./pkg/cnservice tests. PR diff against main passes git diff --check. Logs retained as /tmp/cn-drain-merge-{list,normal,race,consumers}.log on the development host.
• Final-head CI is newly running; previous CI success is not carried over. Historical dual-CN evidence above is not a rerun of this merged head. Operator TestAOEEngine failed  #624 must pin this exact commit before companion validation.

[aptend]

Deep re-review at exact head 06bcdb8. I read all prior reviews/replies (no inline threads), compared the c742b6a→current repair and full allocator/client/CN protocol diff, and traced the prior GetBind race, instance/attempt tombstones, cold-CN heartbeat, and method negotiation. The previous two code findings are addressed in the current implementation; focused drain tests pass with exact-head native dependencies.

[P1][Deterministic validation regression] pkg/lockservice/lock_table_remote_test.go:2639 defines refreshedBind without Valid=true. The new validation in pkg/lockservice/service_remote.go:1310-1315 correctly rejects this mock response, but TestGetLockHolderRemoteReturnsBindChangedAfterBindRefresh runs with context.Background and keeps retrying ErrLockTableBindChanged rather than reaching its one-call assertion. Exact-head UT Coverage timed out pkg/lockservice after 20 minutes in this test (job 108772374186); I reproduced the endless “failed to get bind” loop locally with -timeout=90s. Temporarily making this fixture a valid changed bind (Valid:true) made this exact test pass in 0.03s, and I removed that temporary change afterward. Please update the stale fixture and run the owning package/coverage tests before merge. This is a test-contract blocker caused by the new guard, not evidence that a production GetBind response is invalid. Other CI failures in unrelated packages/SCA are not attributed to this PR here.

[VioletQwQ-0]

Fixed the stale allocator-response fixtures in commit 594c5e3. Both TestGetLockHolderRemoteReturnsBindChangedAfterBindRefresh and TestRemoteWithBindChanged now return a binding with Valid: true; their existing bind-change and call-count assertions remain intact.

Validation on macOS with native dependencies built from this checkout:

• The focused GetLockHolder regression passed.
• The full pkg/lockservice package passed with -count=1 -timeout=15m -coverprofile=... (108.696s, 88.2% statement coverage).
• git diff --check passed.

This is a test-fixture-only correction; production behavior is unchanged. The new head's CI is pending and is separate from these local results. @aptend, please re-review the validation regression.

[aptend]

Deep re-review at exact head 594c5e3. I read all six historical reviews and author replies (there are no inline review threads), compared the 06bcdb8→current increment, and rechecked the complete allocator/client/CN drain and GetBind paths. The increment only marks the two mock refreshed bindings Valid=true in pkg/lockservice/lock_table_remote_test.go; the prior hanging-test blocker is closed without changing production behavior. The earlier zero-bind publication race and canonical BeginDrain/QueryDrain method-version gap remain addressed. Exact-head local CGo validation: the focused bind-change/drain regressions passed, and the full pkg/lockservice package passed with -count=1 (101.172s). No remaining blocking issue found in this PR. Current SCA failure points to unchanged pkg/vm/engine/tae/db/merge/scheduler_promotion_test.go:144; UT/Coverage/BVT checks are still running and should remain merge gates. This approval does not claim completion of the separate Operator/mixed-version/pod-eviction validation.

[VioletQwQ-0]

Follow-up at da0370e: merged upstream main b9013dc, which fixes the pre-existing scheduler_promotion_test.go SendConfig argument mismatch that failed this PR SCA twice. The merge had no conflict and git diff --check passed. The two stale binding fixtures were already fixed at 594c5e3 and validated in the previous comment. New-head CI is running; I did not claim a completed local TAE test because this shared workstation had concurrent native builds. @aptend, please re-review the fixture fix on the new head; prior code CRs and cloud/Kruise validation remain separately tracked.

[VioletQwQ-0]

@iamlinjunhong Please re-review your P1/P2 findings at b8a96a9da3.

P1 — stale admission and publication: an already-draining CN still returns ErrNewTxnInCNRollingRestart at admission. If admission succeeded but drain/retirement invalidates allocation, the handler now returns ErrLockTableBindChanged for the existing bounded retry path. Client validation rejects invalid/ownerless/wrong-group/wrong-table responses and still accepts valid remote owners. TestGetBindDrainInterleaving pauses the real handler between canGetBind and Get and covers new/existing binds, legacy restart, retirement and a no-drain control. The same oracle on c742b6a reproduces successful empty binding in all four negative cells. TestGetBindInvalidResponseReleasesAllocation proves no cache pollution, removal/closure of allocation state, waiter exit and successful later publication. Race repetition passes at 75 and 100 runs respectively.

P2 — canonical client: TestInstanceBoundDrainCanonicalClient uses actual NewClient.Send and allocator handlers. v99 rejects both methods before server dispatch; v100 traverses the transport and checks the complete proof, incomplete/completed drain, wrong proof fields and cancellation. It passes 100 race repetitions. MORPC v100/method IDs 23/24 remain unchanged; no legacy fallback is added. Single-process version switching is not described as mixed-binary testing.

Validation uses the repository mo-cgo-test wrapper and matching native dependencies (Go1.26.4, macOS arm64). All final regression selections pass; the owning lockservice package passes normal/coverage and race (646 tests), cnservice passes 135 tests, and lockop passes 139 tests in CI-equivalent short/tag mode. Final standard vet, formatting, diff check and incremental static checks are recorded in the PR body. One unchanged wall-clock-sensitive test failed the initial package race run under compilation load; its 1-second expiry explains the failure, the isolated pre-repair overlay passes, and the final whole-package race passes without changing assertions. The initial failure is retained in the evidence.

The PR remains Ready. Final-head CI and your explicit re-review remain gates; this update does not claim Operator integration, mixed binaries, Kubernetes/Kruise eviction or cloud acceptance.

[VioletQwQ-0]

@XuPeng-SH Please re-review your P1/P2 findings at b8a96a9da3.

P1 — stale admission contract and terminal behavior: TestGetBindDrainInterleaving now controls the real handler with channel barriers, including an existing binding and safe retirement. No late bind is allocated, no retired service is resurrected and an established proof remains safe. Old c742b6a fails the same oracle with error=nil/Valid=false/empty owner/table=0. Client tests independently vary Valid, owner, Group and Table; local/remote owners and ByRow are positive controls. The service case verifies allocation cleanup and recovery. TestLockWithRetryStopsAfterBindChangedThenDrainRejection and TestLockWithRetryBindChangeHonorsCancellation, together with existing success/budget tests, prove that the new bind-change error uses bounded retry and the next drain rejection terminates. No frontend rollback policy or global retry layer changed.

P2 — usable negotiated API: the real canonical-client test proves the v99 rejection/v100 server round-trip, matching proof, incomplete/completed state and mismatched proof rejection. TestGetLockServiceIdentityDoesNotEnumerateLocks additionally proves identity-only does not iterate and a legacy request returns an actual held row with table/key/mode/holder. We do not claim the Operator has enabled IdentityOnly.

The Valid=true fixture corrections reviewed by aptend are retained; both focused refreshed-binding tests and the owning package/coverage checks pass.

Validation uses the repository mo-cgo-test wrapper and matching native dependencies (Go1.26.4, macOS arm64). All final regression selections pass; the owning lockservice package passes normal/coverage and race (646 tests), cnservice passes 135 tests, and lockop passes 139 tests in CI-equivalent short/tag mode. Final standard vet, formatting, diff check and incremental static checks are recorded in the PR body. One unchanged wall-clock-sensitive test failed the initial package race run under compilation load; its 1-second expiry explains the failure, the isolated pre-repair overlay passes, and the final whole-package race passes without changing assertions. The initial failure is retained in the evidence.

The PR remains Ready. Final-head CI and your explicit re-review remain gates; this update does not claim Operator integration, mixed binaries, Kubernetes/Kruise eviction or cloud acceptance.

[XuPeng-SH]

Design-focused deep re-review of b9013dc6bb058ec74935a62ac3a35f57d13c2f98..b8a96a9da3c717a9d85cf4a9d9f8815fdbbc7b5c. No subagents were used.

REQUEST_CHANGES: the original kernel defects are repaired, but the allocator-loss recovery contract remains open. This continues the recovery concern in the earlier design review; it is not another report of the old zero-binding bug.

Remaining design blocker

A CN may enter Waiting and then lose its allocator's state before completion is durably observed. Its old proof is correctly rejected by the new allocator epoch. However, asking the new allocator for a fresh attempt creates a pending bind that accepts only a ServiceLockEnable heartbeat. The same live CN continues sending Waiting, UnLockSucc or CanRestart; the real heartbeat handler rejects those before clearing drainAwaitingHeartbeat. CN keepalive failure purges/fences bindings but does not return the CN to Enable. Repeated BeginDrain/QueryDrain therefore cannot recover this instance even after its transactions finish.

I independently exercised the fresh-allocator boundary through the real heartbeat handler. With identical instance/attempt setup, the Enable control passes; Waiting, UnLockSucc and CanRestart each keep heartbeat OK=false and awaiting=true through three heartbeats, and the expected fresh-attempt acceptance fails. This is a deterministic component witness plus CN caller tracing, not a process-restart/Kruise experiment. It confirms the missing recovery transition, not an unsafe success response. The existing cold-CN test deliberately rejecting stale completion is a useful safety control and should remain.

Please define and validate an executable safe recovery sequence: a fresh, instance-bound handshake that works for a still-draining CN, or a concrete operational recovery procedure with its independent fencing/completion evidence. Simply changing the attempt ID does not work. Do not accept an old completion heartbeat without a fresh proof or reopen normal admission to manufacture Enable.

This change adds wire methods/versioning and a cross-CN/TN/Operator lifecycle protocol. I did not find a stable, versioned design with a traceable approval in either PR. Record the final state transitions, allocator-loss recovery, compatibility/rollout order and validation ownership in one final contract document. Under the design-first review gate, implementation approval remains blocked here; this review does not claim an exhaustive generated-code/rollout approval.

Historical comments

• Original P1 closed: late admission now returns ErrLockTableBindChanged; the client validates Valid/owner/group/table before publication. The deterministic new/existing/legacy/retirement/control cells and allocation-waiter cleanup test pass.
• Original P2 closed: canonical client capability checks register BeginDrain/QueryDrain at v100; real transport tests prove v99 rejection and v100 proof round trips.
• aptend's fixture blocker closed: the two changed-binding mocks retain Valid=true. Current-head required CI, UT, coverage and BVT pass.
• Identity-probe cost addressed: IdentityOnly returns before IterLocks; its test also checks a real legacy lock entry.
• Recovery and release acceptance remain open: fresh proof after allocator loss, mixed binaries and real Operator/Kruise eviction are not supplied by the repaired kernel unit tests. The companion PR's timeout repair is outside this kernel review's verified implementation scope.

Ownership, liveness and cost

Audit	Result
Q1 ownership	Rejected binding responses are released; allocating entries/channels terminate without publishing an invalid route.
Q2 termination	Stale-bind retry/drain rejection and cancellation are covered; allocator-loss drain recovery is the outstanding state-transition problem above.
Q3 growth	Retirement records have a 24-hour default cleanup horizon; instance identity lookup avoids lock-count-dependent enumeration. No new worker is added.

Fresh local CGo validation on macOS arm64/Go 1.26.4: the five focused handler/client/publication tests pass under race detection (package 2.927s). The separate recovery challenge has one passing control and three expected failing recovery cells (package 0.573s). Native inputs match the provenance-verified artifact source. I also checked the author-reported final-content coverage/race evidence and live current-head CI; green checks do not establish the missing recovery contract.

QA sources and logs are retained under /private/tmp/mo-review-29291-29425.q67o1N/. PR production code and tests were not changed.

[aptend]

Deep re-review at exact head b8a96a9da3c717a9d85cf4a9d9f8815fdbbc7b5c (full PR diff plus the increment since my prior approval, historical reviews/replies and the unresolved thread). The earlier zero-bind and client-version defects, and the stale test fixture, are closed; required CI is green. I independently traced one remaining blocking liveness path.

[P1] pkg/lockservice/lock_table_allocator.go:466-478,1407-1418,1566-1581: if a CN has already received Waiting from allocator A, then A restarts before completion, allocator B has no bind for that exact service. BeginDrain on B creates drainAwaitingHeartbeat=true and returns OK=false. The CN's actual lock_table_keeper.go:544-550 heartbeat reports its local Waiting/UnLockSucc/CanRestart, never Enable; confirmPendingDrainHeartbeat rejects each, so B never clears the pending flag. service.go:2594-2635 purges stale bindings on OK=false but does not reset the CN's drain status. Thus even after remote transactions release, repeated BeginDrain/QueryDrain on B cannot yield a fresh safe proof; a protected CN cannot be evicted without an out-of-band, independently verified recovery. This is a concrete allocator-restart interleaving, not a repeat of the repaired GetBind race. The current cold-CN test's Enable control does not cover a previously draining CN.

Please add a safe instance-bound re-handshake or specify and validate a concrete operational recovery/fencing procedure, with a regression that runs the old-allocator Waiting state into a fresh allocator and verifies eventual completion after transactions drain. Do not accept stale completion heartbeats as proof. I also confirmed the current v100 method negotiation, identity-only fast path and bounded bind-change retry; no separate blocker there. The separate Operator rollout/pod-eviction acceptance remains open.

[VioletQwQ-0]

@aptend @XuPeng-SH The allocator-loss recovery blocker is addressed at 7c3a8d7. A CN already in Waiting/UnLockSucc/CanRestart re-handshakes with the fresh allocator by echoing its new ID/version on a later heartbeat; the pending bind remains non-admitting until that exact-epoch heartbeat. Terminal heartbeats carrying transactions are rejected, and the real two-lockservice held-remote-lock regression remains unsafe until Unlock. The final state/rollout contract is documented in docs/design/20260928-cn-safe-drain-allocator-recovery.md. Final-head lockservice normal/race, cnservice and lockop tests passed with a matching native build; full CI is rerunning on the pushed head. This is still not a real process-restart or mixed-binary/cloud eviction claim. Could you re-review the transition and remaining release gates?

[XuPeng-SH]

Reviewed 7c3a8d72efd10b0689b89bae2cb3b3c5f08a39fc against merge-base afa347996b878336ff294f5cd070492194352b30, including all 22 changed files, historical reviews and the allocator-loss design contract. No subagents were used.

APPROVE for the kernel change. The previous allocator-loss recovery finding is closed; this is not cloud/Operator release approval.

Design and correctness

The identity contract is coherent: exact lock-service incarnation + attempt + allocator identity/epoch, with no UUID/lookup-miss fallback. BeginDrain atomically closes allocator bind admission; a cold/replacement allocator creates a pending non-admitting record. Requiring the CN to observe and echo the replacement epoch allows Waiting/UnLockSucc/CanRestart recovery without reopening Enable. The CN's existing local completion checks remain authoritative. An epoch echo establishes observation, not completion: Waiting and terminal heartbeats carrying transactions remain unsafe.

I traced bind publication, keeper observation/fencing, terminal status, timeout cleanup and retirement reuse. The GetBind admission/Get interleaving is rechecked under allocator ownership; invalid responses cannot enter the client's bind cache, and allocation waiters are released on failure. Same-attempt retries and delayed phases are monotonic; exact retirement records cannot authorize another incarnation. Retirement cleanup bounds retention by the configured disconnect duration; expiration loses the proof and fails closed.

This is a justified protocol change rather than another heuristic based on session/lock counts. Normal GetBind adds constant-time identity/status validation, not a lock-table scan. IdentityOnly exits before IterLocks, avoiding copying an entire busy CN's lock list. No new worker or unbounded retry loop is introduced. Existing aliases are compatibility-only and do not provide the new safety contract.

Historical comment closure

• Admission revocation returning an invalid successful bind: fixed on both allocator and client boundaries, including waiter cleanup and the next terminal drain rejection.
• Canonical RPC capability registration/version collision: methods 23/24 require v101; v99/v100 reject before transport; main's v100 assignment is preserved.
• Refreshed-binding fixtures: retain Valid=true; no weakened production validation.
• Identity query cost: real-lock legacy control retained; IdentityOnly does not enumerate.
• Allocator-loss dead end: resolved by the fresh-epoch handshake and real remote-holder test, not by resetting the CN to Enable.

Validation and limits

Fresh reviewer validation on this head: focused drain/identity/admission/bind tests PASS normally (3.330s) and under race detection (4.181s). A separate reviewer-owned handler challenge combines three successive allocator-state losses with different attempts, stale terminal proofs, active-transaction terminal rejection, continued bind rejection, and eventual completion. It passed 20 repetitions under race detection (4.587s). Both generated protobuf files reproduce exactly with protoc 3.21.1 and the matching gogo generators/import mapping. Diff check passes.

I reused the author's exact-head owning lockservice normal/race package evidence and cnservice/lockop results, rather than claiming to have rerun those full packages. Current CI Required is SUCCESS. Local checks used Go 1.26.4 darwin/arm64 CPU CGo artifacts with verified provenance; native CPU source inputs are unchanged by the intervening GPU-toolchain changes.

Actual allocator process restart, mixed CN/TN/Operator binaries, Pod/container incarnation binding, timeout protection and all Kruise/cloud eviction routes remain the explicitly documented release gates owned by the companion integration. The in-process state-loss test is not those tests, and the supplementary SQL case does not exercise eviction. Do not close operator #623 or claim seamless/no-retry business migration from this approval.

Local reviewer evidence: /private/tmp/mo-review-drain-drop-OKsx8n/. No PR source, branch, CI or review-thread resolution state was modified by this review.

[aptend]

Deep re-review at exact head 7c3a8d7. I read all historical reviews, author replies, and the resolved/outdated allocator-loss thread; compared the repair after my b8a96a9 review with the full PR diff; and traced the allocator, CN keeper, canonical RPC/version checks, GetBind publication, tombstone cleanup and exact-instance/attempt/epoch proof. The old invalid-binding admission race, version negotiation gap, stale test fixtures, and previously blocking allocator-loss dead end are closed. In the fresh-allocator path, pending bind admission remains disabled until an exact-epoch heartbeat, and a Waiting heartbeat does not imply Safe; terminal reports with active TxnIDs are rejected. The new design contract at docs/design/20260928-cn-safe-drain-allocator-recovery.md (blob 7dc53f708241244a93c6041e059d8c6ebd4dfb21) is traceably reviewed and accepted at this same head in review 5341334387; the kernel implementation aligns with it. The exact-head lockservice normal/race, CN, lockop and CI evidence is green; diff check passes. No remaining blocker in this kernel PR. This is not an approval of real allocator-process restart, mixed-version rollout, Operator/Kruise/cloud eviction, timeout recovery, or incident closure: those documented release gates still require their own evidence.

[fengttt]

Where is the PR link?

[VioletQwQ-0]

@fengttt The companion Operator PR is matrixorigin/matrixone-operator#624. I added this explicit link and the MatrixOne tracking issue #29477 under the PR issue section. This kernel PR does not close #29477: Operator/cloud eviction and QA remain separate validation gates. Could you please re-review this clarification?

[aunjgr]

Reviewed 7c3a8d7 as a kernel-only drain protocol. The allocator binds BeginDrain/QueryDrain to exact service incarnation, attempt, allocator identity/epoch; missing or ambiguous state fails closed, and re-handshake after allocator loss remains pending until the CN echoes the new epoch. GetBind publication is rechecked after admission, and retirement proof is bounded and not inherited by another incarnation. The focused protocol/race and two-CN remote-holder tests plus green CI support this kernel scope. I found no concrete remaining kernel blocker. This approval does not certify the separate Operator/cloud eviction rollout or mixed-binary deployment gate.