Avoid deadlocking the Packet Tunnel after a reboot

[buggmagnet]

This PR fixes a deadlock that was triggered by rebooting the phone when the VPN was connected.

---

This change is 

[linear]

IOS-909 App hangs on launch after reboot

[rablador]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 176 at r1 (raw file):

    }

    private func performSettingsMigration() {

Although we won't deadlock here, the code could potentially repeat forever. That's still by design then?

[mojganii]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 177 at r1 (raw file):

    private func performSettingsMigration() {
        var hasNotMigratead = true

typo : hasNotMigrated

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 178 at r1 (raw file):

    private func performSettingsMigration() {
        var hasNotMigratead = true
        repeat {

this loop tries until the migration is done, is it the thing? Can't return default value when it fails after last retry?

[mojganii]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet)

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 179 at r1 (raw file):

        var hasNotMigratead = true
        repeat {
            migrationManager.migrateSettings(

this code can be shorten like this :

Code snippet:

      while true {
            migrationManager.migrateSettings(
                store: SettingsManager.store,
                migrationCompleted: { [unowned self] migrationResult in
                    switch migrationResult {
                    case .success:
                        providerLogger.debug("Successful migration from PacketTunnel")
                        break
                    case .nothing:
                        providerLogger.debug("Attempted migration from PacketTunnel, but found nothing to do")
                        break
                    case let .failure(error):
                        providerLogger
                            .error(
                                "Failed migration from PacketTunnel: \(error)"
                            )
                        // `next` returns an Optional value, but this iterator is guaranteed to always have a next value
                        if let delay = migrationFailureIterator.next() {
                            providerLogger.error("Retrying migration in \(delay.timeInterval) seconds")
                            // Block the launch of the Packet Tunnel for as long as the settings migration fail.
                            // The process watchdog introduced by iOS 17 will kill this process after 60 seconds.
                            Thread.sleep(forTimeInterval: delay.timeInterval)
                        } else {
                            break
                        }
                    }
                }
            )
        }

[buggmagnet]

Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @mojganii and @rablador)

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 176 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Although we won't deadlock here, the code could potentially repeat forever. That's still by design then?

Yes

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 178 at r1 (raw file):

Previously, mojganii wrote…

this loop tries until the migration is done, is it the thing? Can't return default value when it fails after last retry?

This is the intention, the PacketTunnel either succeeds the migration, or gets killed after 1 minute because of the new watchdog timer introduced in iOS 17

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 179 at r1 (raw file):

Previously, mojganii wrote…

this code can be shorten like this :

This suggestion is problematic because it forces the PacketTunnelProvider to sleep in the callback from the MigrationManager effectively blocking the UI Process to perform migration while the sleep is happening.

[mojganii]

Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 179 at r1 (raw file):

Previously, buggmagnet wrote…

This suggestion is problematic because it forces the PacketTunnelProvider to sleep in the callback from the MigrationManager effectively blocking the UI Process to perform migration while the sleep is happening.

My mistake, that's true.

[rablador]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @mojganii)

[pinkisemils]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadREST/RetryStrategy/RetryStrategy.swift line 86 at r2 (raw file):

            delay: .exponentialBackoff(
                initial: .seconds(5),
                multiplier: UInt64(2),

As discussed on the day, I think there is no tangible difference between attempting to migrate 4 times a minute vs 12 - so we might as well go for the option that has a lower likelihood of getting killed and leave the multiplier at 1.

[pinkisemils]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadREST/RetryStrategy/RetryStrategy.swift line 87 at r2 (raw file):

                initial: .seconds(5),
                multiplier: UInt64(2),
                maxDelay: .minutes(1)

As discussed on the day, I think there is no tangible difference between attempting to migrate 4 times a minute vs 12 - so we might as well go for the option that has a lower likelihood of getting killed and leave the multiplier at 1.

[mojganii]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

[buggmagnet]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii)

---

ios/MullvadREST/RetryStrategy/RetryStrategy.swift line 86 at r2 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

As discussed on the day, I think there is no tangible difference between attempting to migrate 4 times a minute vs 12 - so we might as well go for the option that has a lower likelihood of getting killed and leave the multiplier at 1.

Done

[rablador]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)

[pinkisemils]

Reviewed 1 of 3 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @mojganii)