fix(lockservice): check bind before lock success - #24395
Merged
mergify[bot] merged 1 commit intoMay 14, 2026
Merged
Conversation
Wait for any pending bind-change fence before reporting lock success locally or writing remote lock success responses. This closes the stale-success race where a bind change is waiting on the txn lock while an in-flight lock returns success. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
Pull request overview
Closes a stale-success race in lockservice where a lock op could return success after a bind change had begun fencing. After a successful lock acquisition, the code now re-checks under bindChangeMu (with the same lock ordering as the entry path) that the txn id, txn.bindChanged flag, and the lock table's bind are still current; otherwise it returns ErrTxnNotFound or ErrLockTableBindChanged.
Changes:
- Add
checkBindChangedBeforeLockSuccesshelper that briefly unlocks the txn, takesbindChangeMu(RLock), reacquires the txn lock, and validates txn id / bindChanged / table bind. - Invoke the helper on the success path of
service.Lock,handleRemoteLock, andhandleForwardLock, mapping any failure to a cleanpb.Result{}plus error. - Add
TestBindChangedBeforeLockSuccessReturnsBindChangedcovering the race usingbeforeLockAdded+ concurrenthandleBindChanged.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/lockservice/service.go | Adds the post-lock bind/txn revalidation helper and uses it in the local Lock success path. |
| pkg/lockservice/service_remote.go | Applies the same revalidation in the remote and forward lock async callbacks (with a defensive txnID copy). |
| pkg/lockservice/service_test.go | New test that fences a lock between waiter add and success return to assert ErrLockTableBindChanged is returned. |
iamlinjunhong
approved these changes
May 14, 2026
Contributor
Merge Queue Status
This pull request spent 21 seconds in the queue, including 4 seconds running CI. Required conditions to merge
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Which issue(s) this PR fixes:
Refs #24346
What this PR does / why we need it:
This is a focused follow-up for the bind-change fencing fix. It closes the stale-success race where a bind change has already started and is waiting behind the active txn lock while an in-flight lock operation is about to return success.
The change adds a final success-path check before returning local lock success or writing remote/forward lock success responses. The check releases the txn lock, waits behind any pending bind-change fence using the existing bindChangeMu ordering, reacquires the txn lock, and only then allows success if the txn and bind are still current.
This keeps the fix narrow: no retry policy, allocator behavior, commit validation, or logging behavior is changed.
Validation
go test ./pkg/lockservice -run 'Test(BindChangedBeforeLockSuccessReturnsBindChanged|BindChangedFencesActiveTxnHoldingOldBind|BindChangedFencesActiveTxnAfterOldTableRemoved|Issue3538)$' -count=1 -timeout=3m\n-go test ./pkg/lockservice -count=1 -timeout=5m