ceph_assert(pending_reservation_nonce == 0);

Setting pending_reservation_nonce is appropriate for the if (async_request) branch below, but not the else branch. Move this into the if branch to preserve the invariant that pending_reservation_nonce != 0 if and only if there is an pending reservation in the reserver.

Yes. A bug introduced by too many changes in a short time

See above, use m->reservation_nonce as pending_reservation_nonce should not have been set in this branch.

This isn't a necessarily a bug. Depending on timing, a cancellation may occur between when the reserver puts the callback into the finisher and when the callback gets the PG lock. That callback will then see a different pending_reservation_nonce (likely though not necessarily 0) by design. Let's not log this to the central logger -- dout(5) perhaps, but not more.

The !reservation_granted is superfluous -- reservation_granted must be false if pending_reservation_nonce != 0.

~ReplicaActive does

  DECLARE_LOCALS;  // 'scrbr' & 'pg_id' aliases
  if (reservation_granted) {
    dout(10) << "ReplicaActive::~ReplicaActive(): clearing reservation"
	     << dendl;
    clear_reservation_by_remote_primary(false);
  }

Without the if (reservation_granted), it would also handle this case in ~ReplicaWaitingReservation. I think it would be simpler for ReplicaActive to own clearing reservation state upon exit -- not least because it owns the pending_reservation_nonce member -- rather than splitting the responsibility between these two destructors, both of which will run in the event of an interval change. Is there another case where ReplicaWaitingReservation can exit early?

Rename registration_expected to warn_if_no_reservation.

  /**
   * cancel a granted or pending reservation
   *
   * warn_if_no_reservation is set to true if the call is in response to a
   * cancellation from the primary.  In that event, we *must* find a
   * a granted or pending reservation and failing to do so warrants
   * a warning to clog as it is a bug.
   */

Use the actual member names in the log line.

"ReplicaActive::clear_remote_reservation(): "
"pending_reservation_nonce {}, reservation_granted {}",
reservation_granted,
pending_reservation_nonce);

Nit: with this change we effectively allow osd_max_scrubs primary scrubs and osd_max_scrubs replica scrubs, right? That's reasonable to me, but it's probably worth noting that in the config description.

That change was introduced in another PR, on November 2023.
And - yes - I should change the documentation.

Let's state the invariant explicitly:

 * invariant: !((pending_reservation_nonce != 0) && reservation_granted)

It's also worth noting how these states relate to the synchronous/compat case:

 * Note that in the event that the primary is too old to support asynchronous reservation,
 * MOSDScrubReserve::wait_for_resources will be set to false by the decoder and we
 * bypass the 2nd case above.  See ReplicaActive::on_reserve_request.

No remaining users, right? Let's remove it.

Initialize nonce and request_epoch to 0 or disallow the default constructor and ensure that all constructors initialize them to a parameter. We don't want it to be possible to construct an AsyncScrubResData with uninitialized fields.

async_request rather than async in the log line.