fix(lockservice): honor context cancellation in lock waits#25901
Conversation
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? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Request changes: the cancellation work is mostly sound, but the new admission/drain protocol is not a closed quiescence protocol yet.
-
pkg/lockservice/service.go:458-480,521-543:checkCanMoveGroupTableswaits forlockAdmissions == 0while the service remainsServiceLockEnable; thereforebeginLockAdmissioncontinues admitting new work. Under continuous traffic, or one lock wait without a deadline, the count may never reach zero and rolling restart can starve indefinitely. The drain request must atomically close the admission gate first, then wait only for the already-admitted set. -
pkg/lockservice/service_remote.go:278-372pluspkg/lockservice/txn.go:463-475: remote owner locks are asynchronous. Once a waiter is queued,l.lock(... async:true ...)returns and the handler defer releases admission before the callback. At that point the transaction has onlytableBindIntents, while the drain snapshot counts onlytableBinds. A concrete ordering is: holder owns table T; remote waiter for T is queued and its admission ends; drain snapshots the holder only; holder Unlock decrements the last ref before releasing/notifying; T is published as movable; the waiter then acquires T, but its already-finished handler can no longer add the ref. The service can therefore migrate a table with a live/pending transaction. The stack-locallockErris also written by the async callback but may be read by the handler defer when admission began in Waiting state, which is an unsynchronized race.
Please make the linearization explicit and shared by synchronous and asynchronous paths: close/latch the drain gate under s.mu, publish an admitted operation as an active bind intent before releasing admission, and make drain ref accounting plus its decrement symmetric over acquired binds and in-flight intents (or an equivalent design with the same invariants). Do not rely on a transient zero count while the gate is open.
Please add deterministic tests for: (a) a drain request closes admission while an existing Lock is blocked, (b) an async remote waiter survives holder unlock without its table becoming movable, (c) continuous new Lock attempts cannot starve the drain, and (d) the Waiting-state async path under -race.
Validation on current head 1c7910fa2368d3037a294081d61188434f48f2ca: all newly added focused tests passed; full go test -count=1 -timeout=180s ./pkg/lockservice passed in 125.6s; focused race tests, go vet, go build, and git diff --check passed.
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Two blocking flaws remain in the new rolling-restart admission/drain protocol. Focused tests could not start because this checkout lacks cgo/libmo.dylib.
P1 - Close the admission gate before waiting for a drain (pkg/lockservice/service.go:461)
checkCanMoveGroupTables returns while lockAdmissions > 0 but leaves the status at ServiceLockEnable; beginLockAdmission therefore continues admitting new Lock calls. A blocked lock with no deadline, or sustained traffic that keeps the count nonzero, prevents the service from ever reaching ServiceLockWaiting/ServiceUnLockSucc, so a rolling restart can starve indefinitely. The drain transition must atomically reject new admissions first, then wait only for the set already admitted.
P1 - Keep asynchronous remote waiters protected by the drain snapshot (pkg/lockservice/service_remote.go:344)
For a conflicting remote-owner lock, localLockTable.doLock queues the waiter and returns immediately in async mode. The handler then releases its admission while the transaction has only tableBindIntents; incLockTableRef snapshots only tableBinds (txn.go:463), so the drain can publish that table as movable before the callback acquires it. The identical deferred accounting also reads lockErr before the later callback writes it (lines 369/471), creating a race and potentially adding a permanent ref for a failed wait. Account the in-flight intent through callback completion, with one synchronized linearization point shared by both handlers and acquisition/failure paths.
Merge Queue Status
This pull request spent 2 hours 16 minutes 22 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonThe merge conditions cannot be satisfied due to failing checks Failing checks:
HintYou may have to fix your CI before adding the pull request to the queue again. Requeued — the merge queue status continues in this comment ↓. |
# Conflicts: # pkg/lockservice/service.go # pkg/lockservice/service_forward.go # pkg/lockservice/service_remote.go # pkg/lockservice/service_test.go # pkg/lockservice/waiter.go
Merge Queue Status
This pull request spent 1 hour 31 minutes 56 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #25901 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
Failing checks:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Tick the box to put this pull request back in the merge queue (same as
|
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #25790
Fixes #25795
What this PR does / why we need it:
Validation:
-count=100-race -count=20pkg/lockservicefull test suitepkg/lockservicefull test suite with-racego build ./pkg/lockservice ./pkg/cnservicego vet ./pkg/lockservice ./pkg/cnserviceTestDefaultCnConfig