Merge cockroachdb#135042

135042: kv: acquire clock reading for lease request under lock r=nvanbenschoten a=nvanbenschoten

Fixes cockroachdb#134171.

This commit adds validation that lease construction that the min lease proposed timestamp is never in advance of the current time.

The commit then fixes a case where this could occur by acquiring a clock reading under the replica mutex in redirectOnOrAcquireLeaseForRequest.

While I was never able to reproduce the issue in `TestRequestsOnLaggingReplica`, I did independently and reliably hit the same issue when adding lease type changes to `kvnemesis` (cockroachdb#125260, PR to follow soon) and confirmed that this change fixes the issue.

Epic: None

Co-authored-by: Nathan VanBenschoten <[email protected]>

pkg/kv/kvserver/batcheval/cmd_lease_request.go

@@ -87,6 +87,12 @@ func RequestLease(
 		} else {
 			newLease.MinExpiration.Forward(minExp)
 		}
+
+		// Forwarding the lease's (minimum) expiration is safe because we know that
+		// the lease's sequence number has been incremented. Assert this.
+		if newLease.Sequence <= prevLease.Sequence {
+			log.Fatalf(ctx, "lease sequence not incremented: prev=%s, new=%s", prevLease, newLease)
+		}
 	}
 
 	log.VEventf(ctx, 2, "lease request: prev lease: %+v, new lease: %+v", prevLease, newLease)

pkg/kv/kvserver/batcheval/cmd_lease_test.go

@@ -224,7 +224,7 @@ func TestLeaseRequestTypeSwitchForwardsExpiration(t *testing.T) {
 				Replica:    replicas[0],
 				ProposedTS: now,
 				Start:      now,
-				Sequence:   prevLease.Sequence,
+				Sequence:   prevLease.Sequence + 1,
 			}
 			switch leaseType {
 			case roachpb.LeaseExpiration:

pkg/kv/kvserver/batcheval/cmd_lease_transfer.go

@@ -105,6 +105,12 @@ func TransferLease(
 	// previous lease was revoked).
 	newLease.Start.Forward(cArgs.EvalCtx.Clock().NowAsClockTimestamp())
 
+	// Forwarding the lease's start time is safe because we know that the
+	// lease's sequence number has been incremented. Assert this.
+	if newLease.Sequence <= prevLease.Sequence {
+		log.Fatalf(ctx, "lease sequence not incremented: prev=%s, new=%s", prevLease, newLease)
+	}
+
 	log.VEventf(ctx, 2, "lease transfer: prev lease: %+v, new lease: %+v", prevLease, newLease)
 	return evalNewLease(ctx, cArgs.EvalCtx, readWriter, cArgs.Stats,
 		newLease, prevLease, true /* isTransfer */)

pkg/kv/kvserver/leases/build.go

@@ -174,6 +174,9 @@ func (i BuildInput) validate() error {
 	if i.Now.IsEmpty() {
 		return errors.AssertionFailedf("no clock timestamp provided")
 	}
+	if i.Now.Less(i.MinLeaseProposedTS) {
+		return errors.AssertionFailedf("clock timestamp earlier than minimum lease proposed timestamp")
+	}
 	if i.RaftStatus == nil {
 		return errors.AssertionFailedf("no raft status provided")
 	}
@@ -595,17 +598,17 @@ func prevLeaseManipulation(
 		return PrevLeaseManipulation{}
 	case i.Extension():
 		// If the previous lease has its expiration extended indirectly (i.e. not
-		// through a lease record update) and we are switching lease types, we must
-		// take care to ensure that the expiration does not regress. This is more
-		// involved than the common case because the lease's expiration may continue
-		// to advance on its own if we take no action. We avoid any expiration
-		// regression by revoking the lease and then advancing the minimum
-		// expiration of the next lease beyond the maximum expiration that the
-		// previous lease had.
+		// through a lease record update) and we are switching lease types before it
+		// has expired, we must take care to ensure that the expiration does not
+		// regress. This is more involved than the common case because the lease's
+		// expiration may continue to advance on its own if we take no action. We
+		// avoid any expiration regression by revoking the lease and then advancing
+		// the minimum expiration of the next lease beyond the maximum expiration
+		// that the previous lease had.
 		prevType := i.PrevLease.Type()
 		indirectExp := prevType != roachpb.LeaseExpiration
 		switchingType := prevType != nextLease.Type()
-		if indirectExp && switchingType && st.MinExpirationSupported {
+		if indirectExp && switchingType && !i.PrevLeaseExpired && st.MinExpirationSupported {
 			return PrevLeaseManipulation{
 				RevokeAndForwardNextExpiration: true,
 			}

pkg/kv/kvserver/leases/build_test.go

@@ -45,6 +45,22 @@ func TestInputValidation(t *testing.T) {
 			input:  BuildInput{},
 			expErr: "no lease target provided",
 		},
+		{
+			name: "no timestamp",
+			input: BuildInput{
+				NextLeaseHolder: repl2,
+			},
+			expErr: "no clock timestamp provided",
+		},
+		{
+			name: "invalid minimum lease proposed timestamp",
+			input: BuildInput{
+				NextLeaseHolder:    repl2,
+				Now:                cts20,
+				MinLeaseProposedTS: cts30,
+			},
+			expErr: "clock timestamp earlier than minimum lease proposed timestamp",
+		},
 		{
 			name: "remote transfer",
 			input: BuildInput{
@@ -918,6 +934,30 @@ func TestBuild(t *testing.T) {
 					},
 				},
 			},
+			{
+				name: "switch epoch to expiration, previous expired",
+				st:   useExpirationSettings(),
+				input: func() BuildInput {
+					i := defaultInput
+					i.Now = cts30
+					i.PrevLeaseExpired = true
+					return i
+				}(),
+				expOutput: Output{
+					NextLease: roachpb.Lease{
+						Replica:               repl1,
+						Start:                 cts10,
+						ProposedTS:            cts30,
+						Expiration:            &ts50,
+						DeprecatedStartStasis: &ts50,
+						Sequence:              8, // sequence changed
+						AcquisitionType:       roachpb.LeaseAcquisitionType_Request,
+					},
+					NodeLivenessManipulation: NodeLivenessManipulation{
+						Heartbeat: &defaultNodeLivenessRecord(repl1.NodeID).Liveness,
+					},
+				},
+			},
 			{
 				name:  "switch leader lease to expiration",
 				st:    useExpirationSettings(),
@@ -955,6 +995,27 @@ func TestBuild(t *testing.T) {
 					},
 				},
 			},
+			{
+				name: "switch leader lease to expiration, prev expired",
+				st:   useExpirationSettings(),
+				input: func() BuildInput {
+					i := leaderInput
+					i.Now = cts30
+					i.PrevLeaseExpired = true
+					return i
+				}(),
+				expOutput: Output{
+					NextLease: roachpb.Lease{
+						Replica:               repl1,
+						Start:                 cts10,
+						ProposedTS:            cts30,
+						Expiration:            &ts50,
+						DeprecatedStartStasis: &ts50,
+						Sequence:              8, // sequence changed
+						AcquisitionType:       roachpb.LeaseAcquisitionType_Request,
+					},
+				},
+			},
 			{
 				name: "min proposed timestamp",
 				input: func() BuildInput {

pkg/kv/kvserver/replica_range_lease.go

@@ -1245,7 +1245,6 @@ func (r *Replica) redirectOnOrAcquireLeaseForRequest(
 	// Loop until the lease is held or the replica ascertains the actual lease
 	// holder. Returns also on context.Done() (timeout or cancellation).
 	for attempt := 1; ; attempt++ {
-		now = r.store.Clock().NowAsClockTimestamp()
 		llHandle, status, transfer, pErr := func() (*leaseRequestHandle, kvserverpb.LeaseStatus, bool, *kvpb.Error) {
 			r.mu.Lock()
 			defer r.mu.Unlock()
@@ -1259,6 +1258,7 @@ func (r *Replica) redirectOnOrAcquireLeaseForRequest(
 				return r.mu.pendingLeaseRequest.JoinRequest(), kvserverpb.LeaseStatus{}, true /* transfer */, nil
 			}
 
+			now := r.store.Clock().NowAsClockTimestamp()
 			status := r.leaseStatusForRequestRLocked(ctx, now, reqTS)
 			switch status.State {
 			case kvserverpb.LeaseState_ERROR: