Fix broken local uri's when relaunching the app

[jonkan]

Hey! I asked about this in the discord as well.

Since simulators update the path to the app's sandbox each time the app is run I'm hitting this issue where the local uri's stored in the database break the next time I run the app.

What I've observed:

State A: clean slate, not signed in, no sync enabled (yet).

1. Create an attachment. Everything looks good.
2. Restart the app. Local uri is broken, image fails to load from the local uri.
3. Sign in, sync is enabled.
4. The attachment gets the state .archived in the verifyAttachments, because exists is false.
5. The attachment then gets state .queuedDownload in the processWatchedAttachments, because hasSynced is false and it has a localUri.
6. It tries to download the attachment and gets 404 (expected, because it hasn't been uploaded yet).
7. Retries the downloading every 1 sec indefinitely (if no errorHandler, which is the default).

State B, already signed in:

1. Create an attachment.
2. Attachment is uploaded as expected.
3. Restart the app.
4. Local uri is broken.
5. Attachment is download.
6. Everything looks good again.

I've created a partial fix in this PR, by updating the local uri's in the verifyAttachments. The issue that remains is that verifyAttachments isn't run until sync is enabled. I.e. I'm still hitting the bug in State A until step 2.

Can we run verify earlier? Other input/ideas?

Also, this should only be an issue when running on a Simulator. Not for real devices.

[stevensJourney]

Thanks! The fix here looks good to me.

I think it should be fine to run verifyAttachments earlier (in addition to running it when syncing starts).

The simplest solution might be to expose a public method like verifyAttachments which users can call before startSync.

[jonkan]

It appears very simple to just kick off this in the queue's constructor like this:

        Task {
            do {
                try await attachmentsService.withContext { context in
                    try await self.verifyAttachments(context: context)
                }
            } catch {
                self.logger.error("Error verifying attachments: \(error.localizedDescription)", tag: logTag)
            }
        }

If there's no downsides to it, I would of course prefer it if the SDK fixed it automatically. More user friendly than having to realize why it's not working and how to fix it.

• Verify attachments doesn't look to be a very heavy task. Should be ok to kick off silently.
• attachmentsService.withContext should ensure exclusive access to the context, so it should be fine to call this even if startSync is called directly after creating the queue. It would be executed twice, yes. Not seeing that it would be an issue though?

Do you want me to add this, or prefer a more cautious approach of just exposing it like:

    public func verifyAttachments() async throws {
        try await attachmentsService.withContext { context in
            try await self.verifyAttachments(context: context)
        }
    }

[jonkan]

Actually, this bug affects real devices as well. So might be a bit more serious than I first thought.

I misremembered that it's only simulators that get an updated app sandbox URL (probably because I usually don't debug filesystem-stuff on devices, since you can't browse it in Finder).
Not sure exactly when the path does change though. I can confirm (real device):

• The path does change every time I build and run from Xcode.
• The path did not change from relaunching the app (force close and open from Home Screen).
• The path did not change from rebooting the device and relaunching the app.

I think my fix does work but think it's probably a good idea to try and avoid storing absolute paths all together (e.g. shouldn't filename + attachmentsDirectory be enough?).

This Technical Note TN2406 does say:

Code which attempts to derive the path to the Documents or Library directories will return an invalid path on iOS 8. Attempting to access this path will fail, and may terminate your app.

And this on Locating Files Using Bookmarks:

Whereas path- and file reference URLs are potentially fragile between launches of your app, a bookmark can usually be used to re-create a URL to a file even in cases where the file was moved or renamed.

https://stackoverflow.com/questions/47864143/document-directory-path-change-when-rebuild-application
https://stackoverflow.com/questions/26988024/document-or-cache-path-changes-on-every-launch-in-ios-8

[stevensJourney]

@jonkan Thank you for the detailed updates. I was not aware that the absolute path would change on an actual device, this is definitely more concerning.

I'm not against kicking off the task in the background in the initializer. I think having some method of indicating that the verification has completed would be nice though. E.g. some indication that the attachments present are ready to be used. This becomes slightly trickier to implement since you'd need to track a reference to the spawned Task and the Task captures Self before that reference has been set.

I think my fix does work but think it's probably a good idea to try and avoid storing absolute paths all together (e.g. shouldn't filename + attachmentsDirectory be enough?).

This is a fair point. Currently the local_uri is more of a convenience used internally and externally by consumers of the API. A simple SQL join can be used to return the URI to the file in the query result set, example here.
Perhaps we could keep that convenience as an option (which should be guarded by waiting for an initial verification), and also allow users to determine the URI themselves if they'd like (which is also currently possible)

[jonkan]

@stevensJourney Ok. Adding a hasVerifiedAttachments: Bool should be trivial, or maybe didVerifyAttachmentsSubject: PassthroughSubject<Void, Never> if you want an event.
If you want to avoid the unnecessary verifyAttachments call (if start sync is called soon after creation), you could store a reference to the task that you await in the start sync. Shouldn't be anything especially tricky about this, the task reference would be optional and assigned after all members have been initialized.

Personally I don't (currently) see any of these as strictly necessary, not sure what issues they fix. Maybe it can be tracked/discussed in a separate issue? That being said, I'll happily do any of them in order to get this merged. I do need the bug fixed.

[stevensJourney]

@jonkan Thanks. This looks good to me. I'll add some minor items like a changelog entry and potentially some of the items mentioned above in a follow up PR.