cockroachdb / cockroachdb/cockroach
kvserver: lock durability upgrade can result in abandoned replicated lock
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
Given the following sequence:
1. Transaction acquires unreplicated lock on k=a.
2. Leaseholder transfer of range holding key=a writes out the replicated lock.
3. Transaction commits (in a manner allowing for 1PC).
Then:
1. 1PC commit means the lock is not cleaned up during EndTxn processing.
2. Since the lock is owned by the range it is not enqueued for async intent resolution.
The lock then is left on disk. The following test demonstrates this:
```go
func TestOnePCDoesNotCleanUpPromotedReplicatedLocks(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
skip.UnderDuress(t, "can cause uncooperative lease change under leader leases")
ctx := context.Background()
st := base.TestClusterArgs{
ServerArgs: base.TestServerArgs{
DefaultTestTenant: base.TestIsSpecificToStorageLayerAndNeedsASystemTenant,
RaftConfig: base.RaftConfig{
// Suppress timeout-based elections to prevent unexpected
// leadership changes that would clear the lock table without
// exporting unreplicated locks.
RaftElectionTimeoutTicks: 1000000,
},
Knobs: base.TestingKnobs{
Store: &kvserver.StoreTestingKnobs{
RaftTestingKnobs: &raft.TestingKnobs{
DisablePreCampaignStoreLivenessCheck: true,
},
},
},
},
}
tc := testcluster.StartTestCluster(t, 2, st)
defer tc.Stopper().Stop(ctx)
// Enable unreplicated lock promotion on lease transfers.
settings := tc.Server(0).ClusterSettings()
concurrency.UnreplicatedLockReliabilityLeaseTransfer.Override(ctx, &settings.SV, true)
scratch := tc.ScratchRange(t)
// Add a replica on node 1 so we can transfer the lease there.
desc := tc.AddVotersOrFatal(t, scratch, tc.Target(1))
k := append(scratch[:len(scratch):len(scratch)], uuid.MakeV4().String()...)
// Write initial value so the key exists (GetForUpdate needs an existing key).
require.NoError(t, tc.Server(0).DB().Put(ctx, k, "initial"))
// Start with lease on node 0.
require.NoError(t, tc.TransferRangeLease(desc, tc.Target(0)))
txn := tc.Server(0).DB().NewTxn(ctx, "test-1pc-lock-cleanup")
_, err := txn.GetForUpdate(ctx, k, kvpb.BestEffort)
require.NoError(t, err)
// Transfer the lease to node 1. With UnreplicatedLockReliabilityLeaseTransfer
// enabled, this promotes the unreplicated lock to a replicated lock in the
// engine.
t.Log("transferring lease from node 0 -> node 1")
require.NoError(t, tc.TransferRangeLease(desc, tc.Target(1)))
// Force a read through node 1 to ensure the lease transfer raft command
// (which includes the promoted lock) has been applied.
_, err = tc.Server(1).DB().Get(ctx, k)
require.NoError(t, err)
// Verify the promoted lock exists on node 1.
s1 := tc.GetFirstStoreFromServer(t, 1)
locks, err := storage.ScanLocks(ctx, s1.TODOEngine(), k, k.Next(), 0, 0)
require.NoError(t, err)
require.Len(t, locks, 1, "expected promoted replicated lock on node 1")
// Record 1PC metric before commit.
onePCBefore := s1.Metrics().OnePhaseCommitSuccess.Count()
// Commit with no writes. Since the GetForUpdate is a locking read (not an
// intent write), it doesn't consume a write sequence number. The EndTxn has
// maxSeq=1 which IsCompleteTransaction treats as "no writes" → 1PC eligible.
require.NoError(t, txn.Commit(ctx))
// Verify 1PC was used.
onePCAfter := s1.Metrics().OnePhaseCommitSuccess.Count()
require.Greater(t, onePCAfter, onePCBefore, "expected 1PC to be used for commit")
// Check for orphaned replicated lock in the engine on node 1 (leaseholder).
//
// The bug: evaluate1PC() only cleans up the in-memory lock table via
// OnLockUpdated() but does NOT call MVCCResolveWriteIntent(), so the
// promoted replicated lock persists in the engine as an orphan.
locks, err = storage.ScanLocks(ctx, s1.TODOEngine(), k, k.Next(), 0, 0)
require.NoError(t, err)
require.Empty(t, locks,
"expected no replicated locks after 1PC commit, but found %d orphaned lock(s); "+
"1PC does not call MVCCResolveWriteIntent to clean up replicated locks "+
"that were promoted from unreplicated locks during a lease transfer", len(locks))
}
```
Jira issue: CRDB-61083
Contributor guide
Assessment
This issue has not been assessed yet.