PROTO-1335: relay upload delay

[alecsavvy]

Description

There appears to be a race condition when uploads happen where immediately querying for a track after the relay returns can result in a 404. This causes the upload state to hang in the "finalizing upload" status forever even though you can open a new tab and view the upload without issue. All things considered, the upload was a success.

This PR adds just a slight delay to the upload process to minimize the chances of this race condition. Considering that SDK has a lot of logic around what it should do on error responses, having custom code in the post or onError middleware didn't seem correct. Open to comments here but figured those should remain generic and not contain custom instructions for uploads.

A more robust way to solve this problem would be to not only verify if a block/transaction was confirmed but the operation was correctly indexed. I'm not as familiar with confirmer but this kind of change is maybe something better piloted in the relay plugin itself.

I figured adding 1-1.5 seconds to uploads is negligible in the grand scheme of the upload process with the gain of uploads not falsely hanging. The other gains is waiting here doesn't trigger a reselection in SDK should the response 404.

How Has This Been Tested?

Manually set npm run web:prod to discovery relay, reproed the issue, then was able to not repro after this change.

[alecsavvy]

haha welcome @DejayJD feel free to review this, @AudiusProject/client bot just threw you into the deep end

[alecsavvy]

Another thought, we could modify the sdk reselection logic to only reselect on less than 500 error codes which would indicate the node is healthy but the request wasn't. This would imply that the selection sdk made is good but the request the user made is incorrect. At that point we could add retry logic here without the overhead of reselecting a node. @raymondjacobson @Kyle-Shanks curious about your thoughts here?

        } else if (response.ok) {
          // Even when successful, copy response to read JSON body to
          // check for signs the DN is unhealthy and reselect if necessary.
          // This will get the client to pick new discovery providers
          // if the selected one falls behind, even if requests are succeeding
          const responseClone = response.clone()
          const data = (await responseClone.json()) as ApiHealthResponseData
          const { health, reason } = parseApiHealthStatusReason({
            data,
            healthCheckThresholds: this.config.healthCheckThresholds
          })
          const blockDiff = isFullFlaskResponse(data)
            ? (data.latest_chain_block ?? 0) - (data.latest_indexed_block ?? 0)
            : 0
          const version = isFullFlaskResponse(data)
            ? data.version?.version ?? ''
            : ''
          await this.reselectIfNecessary({
            endpoint,
            health,
            reason,
            data: {
              block_difference: blockDiff,
              version
            }
          })
        }

i believe this would trigger a reselect on anything not 2xx

[audius-infra]

Preview this change https://demo.audius.co/as/relay-uploads

[Kyle-Shanks]

Adding @sliptype and @amendelsohn to this bc they will have more context than me about the implications here and possible alternative solutions to fix this issue

[amendelsohn]

It's tempting to make confirmer poll for the entry on discovery, but not sure the difference matters in most of our use cases.

I'd be happy with a polling loop or the reselection change if we can verify that it eliminates this race.

I'm also okay with the delay, but would rather keep things as snappy as reasonably possible.

[amendelsohn]

Think I'd prefer to have a polling loop instead of the simple wait to give us a chance at hitting the first time. We can do something like

while (!response)
	api.getTrack
	if (!response) sleep

Not sure what the timing looks like on the indexing side, but this would give us good behavior whether it's fast or not.

If the change to reselection logic you proposed will get us the same behavior, I'm happy with that as well.

[raymondjacobson]

+1 to the "polling" - except this misses the fact that the endpoint gives a 400 and we re-select discovery nodes, which we shouldn't do. So we need both of these changes basically.
I don't feel good merging the 1500ms delay because it'll still be non-deterministic. If the node you're talking to get stuck or takes longer than 1s to index the block, we're going to be in trouble still

And eventually POST endpoints to encapsulate all of this logic with optimistic writes :)

[raymondjacobson]

Another thought, we could modify the sdk reselection logic to only reselect on less than 500 error codes which would indicate the node is healthy but the request wasn't. This would imply that the selection sdk made is good but the request the user made is incorrect. At that point we could add retry logic here without the overhead of reselecting a node. @raymondjacobson @Kyle-Shanks curious about your thoughts here?

        } else if (response.ok) {
          // Even when successful, copy response to read JSON body to
          // check for signs the DN is unhealthy and reselect if necessary.
          // This will get the client to pick new discovery providers
          // if the selected one falls behind, even if requests are succeeding
          const responseClone = response.clone()
          const data = (await responseClone.json()) as ApiHealthResponseData
          const { health, reason } = parseApiHealthStatusReason({
            data,
            healthCheckThresholds: this.config.healthCheckThresholds
          })
          const blockDiff = isFullFlaskResponse(data)
            ? (data.latest_chain_block ?? 0) - (data.latest_indexed_block ?? 0)
            : 0
          const version = isFullFlaskResponse(data)
            ? data.version?.version ?? ''
            : ''
          await this.reselectIfNecessary({
            endpoint,
            health,
            reason,
            data: {
              block_difference: blockDiff,
              version
            }
          })
        }

i believe this would trigger a reselect on anything not 2xx

This makes sense to me. +1 here!!!!!!

[audius-infra]

Preview this change https://demo.audius.co/as/relay-uploads

[raymondjacobson]

nice work ramping up on this code. the only consideration is that we will have to wait a bit of time for this to be omnipresent in users hands before we flip things on again.

another solve for that specific issue may be to create another feature flag to gate decentralized relay. that way only new users who get this change will have the feature

[audius-infra]

Preview this change https://demo.audius.co/as/relay-uploads

[amendelsohn]

Thanks for digging into this. Great job, and a nice solution!

[amendelsohn]

Should we use a variable delay to disperse groups of failed calls?

[alecsavvy]

like add a back off of some kind?

[amendelsohn]

Yeah... I'm not sure if it's necessary though. Concurrent upload volume shouldn't be high enough to stress anything on a 1.5s interval.

[alecsavvy]

I wouldn't think so. This is also a GET query, not an actual write operation.

[amendelsohn]

ya let's leave as is. Ty for entertaining my thinking out loud lol

[amendelsohn]

this is all post confirmation, right? so we're not depending on upload or confirmation time to fit within the retry window.

[alecsavvy]

no, the upload has already succeeded, this is purely for indexing

[amendelsohn]

brilliant. Sounds good to me!