Add fallback/cutoff to our backend calls similar to how we handle GitHub queries #3519

[sergeibbb]

Description

handleException and requestExceptionCount etc. We should stop calling our APIs in a session if there’s a problem and X attempts in a row fail.

Checklist

• I have followed the guidelines in the Contributing document
• My changes follow the coding style of this project
• My changes build without any errors or warnings
• My changes have been formatted and linted
• My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
• My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
• My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

[axosoft-ramint]

We're throwing an error now in places where we weren't before. Should we be catching/handling this from the callers? And if not, have we verified that it doesn't break anything?

[sergeibbb]

fetchApi is private inside of SubscriptionService and in all 3 places where it's called all exceptions are caught now.

But, we probably can handle it differently from other types of exceptions. For example, when checkInAndValidateCore catches and exception it changes subscription status. Maybe we shouldn't do it for this type of error?

[sergeibbb]

@axosoft-ramint
Well, what I see is that it has started hitting debugger expressions that haven't been hit before. Here is an example on the picture, but probably, there are more such places. Should we do anything about it now or it can be postponed?

[axosoft-ramint]

That's probably fine? Before these changes, it would hit this anyway if the fetch threw an error, so really the only new error we're introducing that hits this debugger is from too many failed requests, in which case, we would probably want to hit this line and know about it anyway.

[eamodio]

@sergeibbb At the high-level I'm confused by the need to have both ServerConnection (existing) and FetchingService (new), why did we need to introduce FetchingService? Why couldn't ServerConnection (or a holistic replacement of it) serve our needs here?

For example, the DraftsService now has a reference to both, and uses them for different usages that feels like it make things more complex.

[sergeibbb]

@eamodio

At the high-level I'm confused by the need to have both ServerConnection (existing) and FetchingService (new), why did we need to introduce FetchingService? Why couldn't ServerConnection (or a holistic replacement of it) serve our needs here?

For example, the DraftsService now has a reference to both, and uses them for different usages that feels like it make things more complex.

Because DraftsService sends requests both to GK API and to S3. It seemed strange to block calls to S3 when GKDev is out.
So, Fetching service is responsible for making any types of requests to different services and the ServerConnection is responsible for making connection to the GK Dev server.

It was discussed here: https://gitkraken.atlassian.net/browse/GLVSC-625?focusedCommentId=123664

[eamodio]

But those were already handled differently in ServerConnection, the "raw" fetch method allowed for more general fetching, and the other fetch versions covered more specific cases for the GK API.

Sure, having a "generic" fetch on ServerConnection was always a bit odd, since that class' intent was connecting to our backend, but today that was conceptually still true because the S3 calls are all in service of taking to our backend (just an implementation detail). At the same time, we could move that more generic fetch into a pure utility function.

I'm also unsure of why we wouldn't encapsulate the connection/exception tracking within ServerConnection rather than have it in SubscriptionService. It feels like implementation details are leaking there. And if we change that, do we still need the Retriever?

[sergeibbb]

@eamodio

that more generic fetch into a pure utility function.

Could be. But it needs a link to the container to build the userAgent value.

Anyway, I can join those two classes back and keep that little oddness.

why we wouldn't encapsulate the connection/exception tracking within ServerConnection rather than have it in SubscriptionService.

because it's going to be reset with a new session, but the session is controlled by SubscriptionService. Anyway, it can be moved back to the ServerConnection.

[sergeibbb]

@eamodio

And if we change that, do we still need the Retriever?

We only don't need the retriever, if we stop handling the request error inside of the generic fetch and move the handling to the fetchApi, that is GK specific. That is OK while we don't expect outage or rate-limit problems from S3.

[axosoft-ramint]

Latest version looks good. I didn't really have time to exhaust all the possibilities but what I did simulate, it handles well. Thanks for working on it!

Let's wait to merge this until after our patch release, because it's targeting main and I would like this to sit in the prerelease for longer before we release it.

[sergeibbb]

Hi @axosoft-ramint
A release has happened. Can I merge?

[axosoft-ramint]

A release has happened. Can I merge?

@sergeibbb Thanks for the reminder! Merged it.