server/storage/mvcc: fix progressIfSync lock-order inversion that deadlocks the apply loop#22185
server/storage/mvcc: fix progressIfSync lock-order inversion that deadlocks the apply loop#22185lazureykis wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lazureykis The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @lazureykis. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
…dlocks the apply loop
progressIfSync held watchableStore.mu and then acquired store.mu (via
s.rev() -> store.Read()), the reverse of the order documented on
watchableStore.mu ("should never be locked before locking store.mu").
With a concurrent KV().Commit() queuing a writer on store.mu (as the
snapshot-send path does) and an in-flight write txn, this formed a
three-way deadlock that wedged the apply loop.
Read the current revision directly under revMu instead, matching the
watchable.mu -> revMu ordering already used by watch(), syncWatchers()
and moveVictims().
Adds TestProgressIfSyncDeadlock, which deadlocks before this change and
passes after.
Fixes etcd-io#22184
Assisted-by: Claude Opus 4.8
Signed-off-by: Pavel Lazureykis <pavel@lazureykis.dev>
999c375 to
aad3f3a
Compare
Fixes #22184.
progressIfSyncheldwatchableStore.muand then acquiredstore.mu(vias.rev()→store.Read()), the reverse of the order documented onwatchableStore.mu("should never be locked before locking store.mu"). With a concurrentKV().Commit()queuing a writer onstore.mu(as the snapshot-send path increateMergedSnapshotMessagedoes) and an in-flight write txn, this formed a three-way deadlock that wedged the apply loop. Full analysis and a deterministic reproduction are in #22184.This reads the current revision directly under
revMu— the samewatchable.mu → revMuordering already used bywatch(),syncWatchers(), andmoveVictims()— instead of opening a full read transaction that reaches forstore.mu.Adds
TestProgressIfSyncDeadlock, which deadlocks (blocks at a 3s deadline) before this change and passes after. The existingmvccwatch/sync tests remain green under-race.AI disclosure: this change was prepared with AI assistance (Claude). I reviewed the analysis, verified the fix and the regression test against the source, and ran the tests myself. The commit carries an
Assisted-by:trailer accordingly.