feat/SYNC-4244_stale

[jrconlin]

Description

Logs to STDERR a message indicating the FxA UID and client state for any transaction that uses a stale value.
This also generates a new request.error.invalid_client_state metric to track how many instances of this sort of thing we see.

Issue(s)

Closes: SYNC-4244

[pjenvey]

This is a pretty normal situation, is even more common with the current state of the purge script so I don't see it being that useful?

Suggested change

	if !old_client_states.is_empty() {
	info!(
	"Tokenserver user has old client states";
	"uid" => raw_user.uid,
	"count" => old_client_states.len()
	)
	}

[jrconlin]

One of the things that was discussed prior was that we didn't have a clear idea how many users were being impacted by the a user client state change. Once the purge script runs, I would suspect that there would be less clients with that potential state. While normal, we don't really know how common it is.

I'm fine specifying this as a metric instead, if that might be useful. If this is a very common occurrence or we have other ways to address the concern about users with excessive old client states, then I'm fine dropping this as well. I'd also be fine setting a reporting threshold of old_client_states.len() > 1 for this log message.

[pjenvey]

My understanding was we wanted to know how many users were affected by https://bugzilla.mozilla.org/show_bug.cgi?id=1892904 triggered by invalid-client-state w/ a description of "Unacceptable client-state value stale value".

old_client_states is soley used to trigger that error, but it's only triggered when the current client_state matches one of those old ones. I'm certain the large majority of users with non empty old_client_states (especially w/ the state of the purge script) don't actually hit that error -- the fact that it's non empty doesn't tell us very much in my opinion -- just that a user changed a password at some point and that replaced_at record hasn't been cleaned up by purge (yet).

Once the purge script catches up the only old_client_states we should be seeing are those within the purge script's grace period of 24 hours.

[pjenvey]

It's probably worth having a different metric (or preferably tag, but we don't have ReportableError::tags in this codebase yet which let's you do that) for the specific "Unacceptable client-state value stale value" issue in question.

Though I suppose we can alternatively look at the warn log messages for a count of those 🤷‍♂️

[jrconlin]

Yeah, let's hold off on adding a tag for that. I see that there's already a ticket for that work.

In any case, I'll add a different ticket to do that and we can add the tag once that lands.