updater: add retry downloadUpdate()

[juliangruber]

Ref https://filecoinproject.slack.com/archives/C03PVLW721L/p1709739129656009

[bajtos]

I think this is a good improvement, but I don't think it's going to fix all edge cases. If I am on a long train ride with a spotty internet connection, all 10 retries may happen while I am offline. Then, I end up in the same broken state.

Given that, I am not sure if it's worth landing this PR at all.

WDYT?

A more robust solution to consider:

• Change the "check for update" frequency from 12 hours to 1 hour
• When checking for a new version, check our state. If there is a new version that we could not download before, then trigger the download. If there is no new version, then trigger the check for a new version.

Alternatively, we can also enable the electron-updater's auto-download feature; maybe it will work better. It makes me wonder why we have disabled it in the past.

[juliangruber]

I think this is a good improvement, but I don't think it's going to fix all edge cases. If I am on a long train ride with a spotty internet connection, all 10 retries may happen while I am offline. Then, I end up in the same broken state.

Given that, I am not sure if it's worth landing this PR at all.

I agree this doesn't solve all cases, but it can handle more cases than the current implementation. Plus, it doesn't introduce any complexity in how we're handling our state.

A more robust solution to consider:

Change the "check for update" frequency from 12 hours to 1 hour
When checking for a new version, check our state. If there is a new version that we could not download before, then trigger the download. If there is no new version, then trigger the check for a new version.

Isn't this effectively the same thing as in this PR, only without exponential backoff and with infinite retries? What about instead configuring the retry logic to have 100 retries? I like this way because we're not introducing any existing state to the updater library.

Alternatively, we can also enable the electron-updater's auto-download feature; maybe it will work better. It makes me wonder why we have disabled it in the past.

Do you have a link to this feature? I'm not finding it in their docs

[bajtos]

I think this is a good improvement, but I don't think it's going to fix all edge cases. If I am on a long train ride with a spotty internet connection, all 10 retries may happen while I am offline. Then, I end up in the same broken state.
Given that, I am not sure if it's worth landing this PR at all.

I agree this doesn't solve all cases, but it can handle more cases than the current implementation. Plus, it doesn't introduce any complexity in how we're handling our state.

Fair enough 👍🏻

A more robust solution to consider:
Change the "check for update" frequency from 12 hours to 1 hour
When checking for a new version, check our state. If there is a new version that we could not download before, then trigger the download. If there is no new version, then trigger the check for a new version.

Isn't this effectively the same thing as in this PR, only without exponential backoff and with infinite retries? What about instead configuring the retry logic to have 100 retries? I like this way because we're not introducing any existing state to the updater library.

This PR does introduce a new state to the updater library and this state is hidden inside pRetry logic. We have no control of that state - for example, it's not possible to abort pRetry loop trying to download version X after we detect that version X+1 is available. We can end up with multiple pRetry loops running at the same time. (Fortunately, the API for downloading the update is not version specific, so in the edge case I described, both pRetry loops will try to download the version X+1.) A possible fix is to setup an AbortController, pass the signal to pRetry (assuming it supports that?) and abort the retry loop when we detect a newer version.

Personally, I prefer to be explicit and manage the state ourselves.

Alternatively, we can also enable the electron-updater's auto-download feature; maybe it will work better. It makes me wonder why we have disabled it in the past.

Do you have a link to this feature? I'm not finding it in their docs

Docs (very brief, unfortunately):
https://github.com/electron-userland/electron-builder/blob/9bcede88f2083d41266e48dfa712adc2d223bd7f/docs/auto-update.md#appupdater--moduleeventseventemitter

Our usage:
https://github.com/filecoin-station/desktop/blob/3c9bf056da8a3f51c97d9dd9decbb419a0a46592/main/updater.js#L39

[juliangruber]

This PR does introduce a new state to the updater library and this state is hidden inside pRetry logic.

Agreed, I should have been more explicit here. This PR doesn't introduce any state that we need to manage. To the outside it's indistinguishable if we have one download call that takes 10 minutes, or whether we are on the 5th retry of faster performing download calls.

We have no control of that state - for example, it's not possible to abort pRetry loop trying to download version X after we detect that version X+1 is available. We can end up with multiple pRetry loops running at the same time. (Fortunately, the API for downloading the update is not version specific, so in the edge case I described, both pRetry loops will try to download the version X+1.) A possible fix is to setup an AbortController, pass the signal to pRetry (assuming it supports that?) and abort the retry loop when we detect a newer version.

As you suggested, this is possible: https://github.com/sindresorhus/p-retry?tab=readme-ov-file#signal. To me this is also very similar to not having retry logic, but one small running download call (downloading one electron app over train internet can also take long. Admittedly not 24h, but just as long maybe as 10 retries on a faster internet.

Personally, I prefer to be explicit and manage the state ourselves.

I disagree here, as this also complicates the updater, just in another way. What about we merge this as is, as it's already an improvement, and then discuss further improving this logic in a separate PR? I personally am not interested in following up at this moment, until we get new bug reports after this logic has been merged and proved to still be insufficient.

[bajtos]

What about we merge this as is, as it's already an improvement

Agreed 👍🏻