Were there supposed to be one commits or two in here? I see what with a message looking like the squash of two.

Nit: there's a lot diffs created by the exporting of that Cfg guy. Moving that change to a dedicated commit would make reviewing easier.

Nit: the release note is too technical; users don't understand this. Also the tense is wrong (as per https://wiki.crdb.io/wiki/spaces/CRDB/pages/186548364/Release+notes) - say "Fixed a bug".

Release note (bug fix): Fixes a bug that allowed follower reads
during the critical state of a merge transaction.

I'd say

Fixed a rare bug causing transaction serializability violations when using follower reads.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @ajwerner, @andreimatei, @nvanbenschoten, and @tbg)

pkg/kv/kvserver/closed_timestamp_test.go, line 710 at r1 (raw file):

// done.
func setupClusterForClosedTSTestingWithCustomKnobs(
	ctx context.Context, t *testing.T, targetDuration time.Duration, knobs base.TestingKnobs,

might as well take a TestClusterArgs.

pkg/kv/kvserver/closed_timestamp_test.go, line 801 at r1 (raw file):

	repls []*kvserver.Replica,
) {
	defaultTestingKnobs := base.TestingKnobs{

Consider keeping a single setupClusterForClosedTSTesting() function by extracting defaultTestingKnobs into a aggressiveResolvedTSPushKnobs that everybody passed in. That way it'll be easier for tests to compose these default knobs with other additional knobs.

pkg/kv/kvserver/replica.go, line 1330 at r1 (raw file):

// neighbor is in its critical phase and, if so, arranges to block all requests
// until the merge completes.
func (r *Replica) maybeWatchForMerge(ctx context.Context, untrack closedts.ReleaseFunc) error {

pls comment the new param

pkg/kv/kvserver/replica.go, line 1361 at r1 (raw file):

		// Nothing more to do.
		r.mu.Unlock()
		return nil

don't we need to call untrack() with LAI+1 in this case?

pkg/kv/kvserver/replica.go, line 1370 at r1 (raw file):

	// arrived and now, which is rare but entirely legal.
	r.unquiesceLocked()
	// We prevent replicas from being able to serve follower reads on timestamps

I think this comment needs to spell things out more: say that we block follower reads by publishing a MLAI that's never going to come in the happy case, and we do that because the closed timestamps that are published from now on will not be reflected in the LHS's tscache after the merge.

pkg/kv/kvserver/replica.go, line 1372 at r1 (raw file):

	// We prevent replicas from being able to serve follower reads on timestamps
	// that fall between the subsumption time (FreezeStart) and the timestamp at
	// which the merge transaction commits or aborts (i.e. when a merge is in its

s/when a merge/the timestamp window representing the range's critical state

Also I don't see the syntagm "critical state" used elsewhere around here. Maybe say something more descriptive like "subsumed state".

pkg/kv/kvserver/replica.go, line 1382 at r1 (raw file):

	// has been subsumed are lease requests, which do not bump the LAI.
	//
	// See https://github.com/cockroachdb/cockroach/issues/44878 for discussion.

nit: I personally consider most such issue links to be just click bait. The comments should usually be sufficient for explaining what's going on; when they're not sufficient because we failed to write them properly, there's git blame.

pkg/kv/kvserver/replica_read.go, line 127 at r1 (raw file):

	log.Event(ctx, "executing read-only batch")

	untrack := r.maybeTrackProposal(ctx, ba)

can this down, move next to res.Local.ReleaseFunc = untrack ?

pkg/kv/kvserver/replica_read.go, line 130 at r1 (raw file):

		}
		br, res, pErr = evaluateBatch(ctx, kvserverbase.CmdIDKey(""), rw, rec, nil, ba, true /* readOnly */)

spurious diff

pkg/kv/kvserver/replica_read.go, line 150 at r1 (raw file):

		return nil, res, pErr
	}
	res.Local.ReleaseFunc = untrack

Is this indirection really needed? Can't we simply untrack here?
I see that there are some early returns in maybeWatchForMerge that don't untrack with LAI+1, but I can't tell which ones are expected. Plus one of them seems wrong to me.

If the indirection is necessary, can we at least bind all the arguments to the closure we put in res.Local.ReleaseFunc? They shouldn't change in between here and maybeWatchForMerge, should they? The closure could take a single bool about whether we're watching for the merge or not (a false would translate to all zeros to untrack()).

pkg/kv/kvserver/replica_read.go, line 154 at r1 (raw file):

}

func (r *Replica) maybeTrackProposal(

This funky method has one caller. Inline it there and then I won't have to ask for a better name and comments.

pkg/kv/kvserver/replica_read.go, line 160 at r1 (raw file):

		// We start tracking SubsumeRequests as part of our guarantee to never
		// broadcast a closed timestamp entry between when we evaluate the Subsume
		// request and the time that we run maybeWatchForMerge for the first time.

I don't quite understand the "for the first time" part. Consider axing it.

pkg/kv/kvserver/replica_read.go, line 162 at r1 (raw file):

		// request and the time that we run maybeWatchForMerge for the first time.
		// See comment in maybeWatchForMerge() for details.
		_, untrack := r.store.Cfg.ClosedTimestamp.Tracker.Track(ctx)

I think there's something missing here - the other return value from Track is a minimum timestamp for the (Subsume) request. I think we need to reject the Subsume request if its timestamp is above it. I think the comment above should spell out the important of Subsume's ts.

pkg/kv/kvserver/batcheval/result/result.go, line 69 at r1 (raw file):

	// Call maybeWatchForMerge.
	MaybeWatchForMerge bool
	// ReleaseFunc, when it's not the empty function, stores the callback returned

I think this needs more words; the read-only SubsumeRequest doesn't have a LeaseAppliedIndex.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @ajwerner, @andreimatei, @nvanbenschoten, and @tbg)

pkg/kv/kvserver/replica.go, line 1361 at r1 (raw file):

We only need to release this tracked proposal the first time we run maybeWatchForMerge (which should always be right after the subsume request evaluation unless there is a lease transfer). Any time we see that we've already started the mergeComplete channel, we should already have released this proposal then. This is what the comment below (in maybeTrackProposal) talks about, I'll make it clearer.

pkg/kv/kvserver/replica_read.go, line 127 at r1 (raw file):

Starting to track the proposal after evaluation of the subsume request seemed problematic for the given scenario:
t1: RHS leaseholder begins evaluating the subsume request (i.e. t1 is our FreezeStart)
t2: The leaseholder's store broadcasts a closed timestamp that is above t1
t3: Now we begin tracking the proposal, but it's too late because we've already emitted a closed timestamp above FreezeStart

Does that make sense? Let me know if I'm missing something.

I reorganized a bunch of stuff and added a new test that forces a lease transfer on the RHS during the merge (& ensures that our closed timestamp guarantees are upheld).

With regards to ComputeChecksumRequest: It turns out that the ComputeChecksum request does "acquire latches" even though it doesn't declare any keys. However, checkExecutionCanProceed only cares about the latchGuard being non-nil on the leaseholder. It almost seems like an unintentional consequence. Should we be more explicit about this? Sorry for not investigating this properly earlier. I added an assertion that blows up if it sees a proposal that would bump the LAI while the range is subsumed, along with an accompanying test.

This is RFAL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @ajwerner, @andreimatei, and @nvanbenschoten)

pkg/kv/kvserver/closed_timestamp_test.go, line 710 at r1 (raw file):

Done, let me know if I misunderstood you.

pkg/kv/kvserver/closed_timestamp_test.go, line 801 at r1 (raw file):

Done

pkg/kv/kvserver/replica.go, line 1330 at r1 (raw file):

Removed the indirection.

pkg/kv/kvserver/replica.go, line 1370 at r1 (raw file):

Done, let me know if you still find it lacking.

pkg/kv/kvserver/replica.go, line 1372 at r1 (raw file):

Done.

pkg/kv/kvserver/replica.go, line 1382 at r1 (raw file):

Removed.

pkg/kv/kvserver/replica_read.go, line 150 at r1 (raw file):

You're correct that the indirection isn't really necessary since we don't need to untrack() in the lease acquisition path (which is what the initial check in maybeWatchForMerge is for)

pkg/kv/kvserver/replica_read.go, line 154 at r1 (raw file):

Done

pkg/kv/kvserver/replica_read.go, line 160 at r1 (raw file):

Done.

pkg/kv/kvserver/replica_read.go, line 162 at r1 (raw file):

Not applicable as discussed offline.

pkg/kv/kvserver/batcheval/result/result.go, line 69 at r1 (raw file):

Removed this, see above.

LGTM

nit: I imagine I'm probably driving you crazy with this this stuff, but I think r4 and r5 should be squashed with r2. The testing really belongs with the commit fixing the issue. And the assertion is tiny, and also tied to the main thing this patch is doing. TestStoreRangeMergeCheckConsistencyAfterSubsumption is more random, that can be a separate commit indeed.

Btw, I also find it much better to rewrite all the commits every time I update a PR (like, rebase them all), so that reviewable shows the new commits as a contiguous block, instead of interspersing old commits and new commits.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @ajwerner, @andreimatei, and @nvanbenschoten)

pkg/kv/kvserver/client_merge_test.go, line 1534 at r5 (raw file):

// 2. Once a merge is aborted, pending (and future) requests will be allowed to
// be proposed. This is meant as a sanity check for the assertion at the
// end of Replica.propose().

pls put more words here describing this assertion

pkg/kv/kvserver/client_merge_test.go, line 1599 at r5 (raw file):

	case <-checkConsistencyResp:
		t.Fatalf("expected the consistency check to wait until the merge was complete")
	case <-time.After(1 * time.Second):

you probably want more than a second here and below; tests run on loaded machines, particularly under testrace.
Consider testutils.SucceedsSoon() - that's the go to for waiting a while

pkg/kv/kvserver/closed_timestamp_test.go, line 469 at r4 (raw file):

	t.Run("without lease transfer", func(t *testing.T) {
		ctx := context.Background()
		blockMergeTrigger := make(chan interface{}, 10) // headroom in case the merge txn retries

why interface{} and not hlc.Timestamp ?

pkg/kv/kvserver/replica_raft.go, line 337 at r5 (raw file):

	if maxLeaseIndex != 0 {
		r.mu.RLock()
		defer r.mu.RUnlock()

nit: I'd just put the RUnlock() below, without a defer. You want to hold this lock for as little as possible, and tie it with the mergeInProgressRLocked() call. Like, if there's more code added to this method, you don't want it running under the lock.
If the log.Fatal() is hit, the lock won't be release anyway.

pkg/kv/kvserver/replica_read.go, line 127 at r1 (raw file):

ack

pkg/kv/kvserver/replica_read.go, line 126 at r2 (raw file):

	log.Event(ctx, "executing read-only batch")

	untrack := func(_ context.Context, _ ctpb.Epoch, _ roachpb.RangeID, _ ctpb.LAI) {}

I'd suggest simplifying this untrack() to only take a bool - whether or not the range has been subsumed, and closing over all the other arguments right here.

Reviewed 1 of 23 files at r14, 21 of 21 files at r15, 1 of 1 files at r16.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15, @ajwerner, and @andreimatei)

pkg/kv/kvserver/replica_closedts.go, line 37 at r6 (raw file):

because it gets interpreted as the range getting quiesced

This doesn't quite capture the conclusion of that conversation. Mind updating it to be a little more clear?

pkg/kv/kvserver/replica_write.go, line 156 at r12 (raw file):

Subsume() cmd_subsume.go

Is this missing an "in"?

pkg/kv/kvserver/batcheval/cmd_compute_checksum.go, line 39 at r16 (raw file):

a rare a closed timestamp

Delete the second "a".

Thank you for the extremely detailed reviews @nvanbenschoten and @andreimatei

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15, @ajwerner, @andreimatei, and @nvanbenschoten)

pkg/kv/kvserver/replica_closedts.go, line 37 at r6 (raw file):

Done.

pkg/kv/kvserver/replica_write.go, line 156 at r12 (raw file):

Fixed.

pkg/kv/kvserver/batcheval/cmd_compute_checksum.go, line 39 at r16 (raw file):

Done.