fix(lockservice): backport owner-local remote lock deadlock fix to 4.1-dev

[aptend]

What type of PR is this?

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

Which issue(s) this PR fixes:

Backport of #25447 to 4.1-dev.

Fixes #25297
Fixes #25278
Related to #25279

What this PR does / why we need it:

Backports the lockservice fix for owner-local remote lock deadlocks:

• detects owner-local wait cycles rooted at the actual conflicting holders;
• caps owner-side remote lock waits at two minutes by default;
• retains remote lock metadata so rollback can release partially granted locks;
• fixes deadlock detector enqueue bookkeeping and adds focused metrics;
• removes completed synchronous waiters from the deadlock-event set before a waiter can be reused, and drops retained waiter references during shutdown (the relevant lockservice portion of fix ut #25792).

Method_Lock is still protocol v1 and has no peer capability signal. The owner therefore keeps the dedicated ErrRemoteLockWaitTimeout internally but sends the existing ErrDeadLockDetected over v1. Every supported 4.1 origin recognizes that code as a whole-transaction rollback, so it issues remote unlock even for a partial multi-row grant during a rolling upgrade.

The cherry-pick applies cleanly to the current 4.1-dev / v4.1.2 baseline. Follow-up #25469 remains unnecessary on this branch: 4.1 already releases the forwarded transaction lock with defer txn.Unlock() before entering the asynchronous wait path.

Validation

• go test -v ./pkg/lockservice -count=1 -timeout=20m — PASS (573.999s)
• go build ./pkg/lockservice — PASS
• go vet ./pkg/lockservice — PASS
• focused waiter and V1 compatibility regression tests under -race -count=10 — PASS
• go test ./pkg/frontend -run Test_isErrorRollbackWholeTxn -count=1 -timeout=5m (with repository CGO/rpath settings) — PASS

The V1 compatibility test makes a two-row remote request acquire row 1 then time out on row 2, checks the legacy deadlock code, performs the old-origin whole-transaction unlock, and verifies row 1 is immediately lockable.

[qodo-code-review]

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 →

[XuPeng-SH]

Requesting changes for one confirmed waiter-lifecycle correctness gap.

This backport includes the waiterEvents.blockedWaiters ownership model from #25447, but it does not include the lockservice part of the follow-up fix #25792 (c1843634bf). On the synchronous lock path, waiterEvents.add increments the waiter reference and appends it to blockedWaiters; after c.w.wait(...) returns, localLockTable.doLock continues without removing that entry. The lock context can then progress toward closing/reusing the waiter while the background checker still owns and may access the old entry until its next scan. That violates the waiter generation/ownership boundary and is the data-race root cause fixed by #25792.

I reproduced the state violation deterministically on this exact head with a review-only test that checks events.mu.blockedWaiters in afterWait: the returned synchronous waiter was still retained. The focused test failed under -race with waiterEvents retained a synchronous waiter after wait returned. (The existing TestManyRangeLockInManyGoroutines happened to pass 10 stress iterations; that scheduling-dependent pass does not close the ownership gap.)

Please backport the relevant lockservice closure from #25792, not its unrelated query/dispatch changes:

• remove and release the synchronous waiter event reference immediately after c.w.wait(...) returns;
• make waiterEvents.close() clear the retained slice after releasing its references, so later removal cannot double-release;
• include the deterministic ownership/close tests from #25792 (or equivalent), and run the focused transition tests under -race.

The owner-local graph, bounded owner timeout, partial-grant cleanup, error mapping, and 4.1-specific forwarded-lock adaptation otherwise look internally consistent in this review.

[XuPeng-SH]

Blocker: this enables the owner timeout for every v1 remote Lock request without checking whether the origin supports the new error semantics. During a rolling 4.1 upgrade, a new owner can partially grant a multi-row request and then return the new numeric code 20709 to an old origin. The old moerr decoder accepts the code and message, but its frontend rollback map does not contain 20709, so an explicit transaction takes RollbackLastStatement rather than whole-txn rollback. That path does not call lockservice Unlock, leaving the partially granted locks and transaction active and defeating the safety cap.

Please make the wire behavior backward-compatible: negotiate/capability-gate the dedicated code and fall back to an existing whole-txn-rollback code for old peers, or stage recognition before emitting it. Add a mixed-version regression that uses the old-origin classifier/rollback behavior.

[aptend]

Addressed in 1c9aae6. Method_Lock has no v1 capability signal, so the owner now maps ErrRemoteLockWaitTimeout to the long-established ErrDeadLockDetected only for the wire response. That error is recognized as whole-transaction rollback by old 4.1 frontends. TestRemoteLockOwnerWaitTimeoutFallbackReleasesPartialRemoteLock covers the partial two-row grant: it releases the first remote row through the legacy rollback/unlock path and proves the row is immediately available.

[XuPeng-SH]

Additional confirmed blocker after tracing the rolling-upgrade closure: the new owner-side timeout error is not backward-compatible with an old 4.1 origin.

Method_Lock remains protocol v1 and there is no peer capability gate. A new owner may return code 20709 after a partial grant. An old origin deserializes arbitrary moerr codes successfully, but its frontend does not classify 20709 as whole-txn rollback; explicit transactions therefore execute only RollbackLastStatement, which does not call LockService.Unlock. The partial remote locks remain held and the origin transaction stays active, contradicting the core guarantee that owner timeout forces the transaction to unwind.

Please gate/fallback the wire error for old peers or use a staged compatibility rollout, with a mixed-version regression. This is independent of the already-filed synchronous waiter ownership blocker from #25792; both need closure before approval.

Local verification on this head otherwise passed: focused race tests x10, full lockservice package, frontend rollback test x10 under race, moerr package, build/vet for all changed packages, and virtual merge against current 4.1-dev.

[aptend]

Updated with 1c9aae6:

• incorporated the relevant waiter-lifecycle fix from fix ut #25792: synchronous waiters are removed from waiterEvents before reuse, and close clears retained waiter references;
• made the owner timeout safe for rolling 4.1 upgrades: Method_Lock v1 emits the existing ErrDeadLockDetected rollback code rather than the new 20709 code;
• added deterministic waiter and partial remote-grant cleanup regressions.

Validation is in the PR description; full lockservice, build, vet, race x10, and the frontend rollback classifier all pass.

[XuPeng-SH]

Re-reviewed the latest head. The previous synchronous-waiter lifecycle blocker is closed, and the new Method_Lock wire fallback closes the direct remote-lock compatibility path. One rolling-upgrade blocker remains: Method_ForwardLock is also protocol v1 and now enables the same owner-local timeout, but its callback still serializes ErrRemoteLockWaitTimeout (20709) directly. An old 4.1 origin does not classify that code as a whole-transaction rollback error, so a ForwardTo request that grants an earlier row and then times out can keep the partial remote lock after statement rollback. Please apply remoteLockWireError to handleForwardLock as well and add a ForwardTo partial-grant timeout regression that verifies origin cleanup releases the earlier grant. Local verification on this head passed the focused race tests (count=10), full ./pkg/lockservice, frontend rollback-classifier race tests (count=10), go vet, and go build. The PR merges cleanly and diff-check passes; remote CI is still in progress.

[XuPeng-SH]

Blocker: Method_ForwardLock is also v1, but unlike handleRemoteLock this handler still writes err directly. During a rolling upgrade, an old sender using ForwardTo can receive the new 20709 timeout after an earlier row was already granted; the old frontend treats that unknown code as statement-only rollback and may leave the partial remote lock held. Use remoteLockWireError in this callback too, and add a ForwardTo partial-grant owner-timeout regression.

[aptend]

Addressed in 562cb23. handleForwardLock now uses the same remoteLockWireError fallback. TestForwardLockOwnerWaitTimeoutFallbackReleasesPartialRemoteLock sends a two-row ForwardTo request, lets the first row be granted and the second time out, then verifies the old-origin whole-transaction Unlock releases the first row.

[aptend]

Updated in 562cb23: Method_ForwardLock now uses the same v1-compatible rollback error as Method_Lock, with a ForwardTo partial-grant cleanup regression. The focused lockservice transition suite passes under -race -count=10. Re-review has been requested from XuPeng-SH through the GitHub requested-reviewers API.

[XuPeng-SH]

Re-reviewed latest head 562cb23093 and approving.

The previous blockers are closed end-to-end:

• synchronous waiters are removed from waiterEvents immediately after wait() returns, before the object can progress toward reuse; close() clears retained entries, so later removal cannot double-release;
• both Method_Lock and Method_ForwardLock translate the owner timeout to the established v1 deadlock code on the wire;
• an old 4.1 origin already records remote bind/rows for remotely observed errors, classifies that code as whole-transaction rollback, and therefore sends remote unlock for partial grants;
• deterministic regressions cover waiter ownership, close-after-remove safety, direct remote partial grants, and ForwardTo partial grants.

Fresh local evidence on this exact head:

• focused lockservice lifecycle/deadlock/compatibility suite: -race -count=10, PASS;
• frontend rollback classifier: -race -count=10, PASS;
• moerr tests -count=10, PASS;
• go vet for lockservice/moerr/metrics, PASS;
• diff-check and virtual merge against current 4.1-dev, PASS.

I also quantified the new synchronous-waiter removal path rather than assuming its cost: cohort cleanup was ~7.5us/100 waiters, 0.46ms/1k, 9.15ms/5k, and 34.6ms/10k on Apple M4, with zero allocations. It is O(N^2) across a simultaneous completion cohort, but it is outside the uncontended lock hot path and remains sub-millisecond at a 1k blocked-waiter cohort; I consider this a low, acceptable tradeoff for closing the proven reuse race, not a merge blocker.

I found no remaining correctness, liveness, resource-lifecycle, compatibility, or material performance blocker. Remote CI is still the merge gate.