release-23.2: rangefeed: fix scheduler catchup iterator race

[blathers-crl]

Backport 1/1 commits from #114240 on behalf of @erikgrinaker.

/cc @cockroachdb/release

---

It was possible for the scheduled processor to hand ownership of the catchup iterator over to the registration, but claim that it didn't by returning false from Register().

This can happen if the registration request is queued concurrently with a processor shutdown, where the registration will execute the catchup scan and close the iterator, but the caller will think it wasn't registered and double-close the iterator.

This patch fixes the race, and also documents the necessary invariant along with a runtime assertion.

Resolves #114192.
Epic: none
Release note: None

---

Release justification: fixes a potential panic.

[blathers-crl]

Thanks for opening a backport.

Please check the backport criteria before merging:

• Backports should only be created for serious
  issues or test-only changes.
• Backports should not break backwards-compatibility.
• Backports should change as little code as possible.
• Backports should not change on-disk formats or node communication protocols.
• Backports should not add new functionality (except as defined
  here).
• Backports must not add, edit, or otherwise modify cluster versions; or add version gates.
• All backports must be reviewed by the owning areas TL and one additional
  TL. For more information as to how that review should be conducted, please consult the backport
  policy.

If your backport adds new functionality, please ensure that the following additional criteria are satisfied:

• There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
• The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
• New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters. State changes must be further protected such that nodes running old binaries will not be negatively impacted by the new state (with a mixed version test added).
• The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.
• Your backport must be accompanied by a post to the appropriate Slack
  channel (#db-backports-point-releases or #db-backports-XX-X-release) for awareness and discussion.

Also, please add a brief release justification to the body of your PR to justify this
backport.

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @blathers-crl[bot] and @erikgrinaker)

---

pkg/kv/kvserver/rangefeed/scheduled_processor.go line 591 at r1 (raw file):

		// Assert that we never process requests after stoppedC is closed. This is
		// necessary to coordinate catchup iter ownership and avoid double-closing.
		if buildutil.CrdbTestBuild {

This assertion is safe to make because it's run on the scheduler goroutine, right? So we know that p.stoppedC can't be closed concurrently with the assertion?

---

pkg/kv/kvserver/rangefeed/scheduled_processor.go line 603 at r1 (raw file):

		return r
	case <-p.stoppedC:
		// If the request was processed concurrently with a stop, there's a 50%

"processed concurrently"

Is this the right phrasing? The request is processed on the same thread of execution that closes stoppedC, right? So this isn't guarding against the case where the processing is concurrent with a stop, but rather, when the request is processed (and result signaled) and the scheduler is stopped (and stoppedC closed) in quick succession.

I guess that is concurrent from the perspective of external users of the scheduler, but since we're listening on internal channels in this code, we're not exactly an external user. Spelling this out might help readers though, or at least those who are just starting to familiarize themselves with this code.

[erikgrinaker]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)

---

pkg/kv/kvserver/rangefeed/scheduled_processor.go line 591 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This assertion is safe to make because it's run on the scheduler goroutine, right? So we know that p.stoppedC can't be closed concurrently with the assertion?

That's right.

---

pkg/kv/kvserver/rangefeed/scheduled_processor.go line 603 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

"processed concurrently"

Is this the right phrasing? The request is processed on the same thread of execution that closes stoppedC, right? So this isn't guarding against the case where the processing is concurrent with a stop, but rather, when the request is processed (and result signaled) and the scheduler is stopped (and stoppedC closed) in quick succession.

I guess that is concurrent from the perspective of external users of the scheduler, but since we're listening on internal channels in this code, we're not exactly an external user. Spelling this out might help readers though, or at least those who are just starting to familiarize themselves with this code.

That's right. From the point of view of this select, they were processed concurrently -- it can observe both result and stoppedC becoming ready at the same time, and that's the condition for the race. But you're right that the actual processing and stopping did not overlap in real time.

Updated the comments here, submitted #114493 for master.