Wordpress-ios: [Auto-upload Published] Posts confirmed for auto-upload are not automatically uploaded

[shiki]

Part of #12240. The fix should target the issue/12240-master-branch.

This was found while testing #12419 but it does not seem to be related to it.

Expected behavior

Posts that are confirmed for auto-uploading are uploaded.

Actual behavior

Posts that are confirmed for auto-uploading do not get uploaded after the app is restarted.

Steps to reproduce the behavior

I was able to reproduce this on the #12419 branch.

1. Go offline.
2. Create a post and publish it. You should see that the post has a _Cancel_ button in the Post List.
3. Force close the app.
4. Open the app again. Navigate back to the Post List.
5. Go online. Notice that the post will not be automatically uploaded.

This video shows the reproduction steps.

Tested on iPhone XS, iOS 12.4.1, WPiOS 2739aebc14.

[shiki]

@jklausa

Something that I found today that might be related to this. While working on #12324, I found out that whenever I edit and save a published post while offline, the remoteStatus somehow ends up in AbstractPostRemoteStatusPushing. It stays with that value even if the uploading was completed. It should have been changed to AbstractPostRemoteStatusFailed but it didn't. I'm guessing that this is a race condition.

You can most probably reproduce this easier in develop. Consider this scenario:

1. Pick and edit a published post that has already been uploaded to the server.
2. Make some changes.
3. Click on X on the top left of the editor.
4. Click on Update Post.

When that happens, there are 2 places where we update the remoteStatus to .pushing:

https://github.com/wordpress-mobile/WordPress-iOS/blob/c1d32840deca04240a7ff1d26a843b5b37add607/WordPress/Classes/Services/PostService.m#L222-L223

And this:

https://github.com/wordpress-mobile/WordPress-iOS/blob/c1d32840deca04240a7ff1d26a843b5b37add607/WordPress/Classes/Services/PostCoordinator.swift#L37

I'm going to guess that one of those changes is delayed and executed after this failure block which (supposedly) correctly sets the remoteStatus to .failed:

https://github.com/wordpress-mobile/WordPress-iOS/blob/c1d32840deca04240a7ff1d26a843b5b37add607/WordPress/Classes/Services/PostService.m#L261

So the sequence probably ended up like this:

1. remoteStatus = .pushing
2. remoteStatus = .failed yay
3. remoteStatus = .pushing nay

Stack Trace

Here are 2 screenshots that show the different places where remoteStatus is changed.

_ | _
--------|-------
| | |

Why This Matters

Because the remoteStatus stays at the .pushing value, it is not picked up by our auto-uploading which uses the remoteStatus = .failed condition to determine which posts should be auto-uploaded:

https://github.com/wordpress-mobile/WordPress-iOS/blob/c1d32840deca04240a7ff1d26a843b5b37add607/WordPress/Classes/Services/PostService.m#L71-L77

Status Refresh

The scenario I described here most probably only happens if the app has not been restarted. It looks like the app does attempt to “fix” these dangling .pushing values here:

https://github.com/wordpress-mobile/WordPress-iOS/blob/c1d32840deca04240a7ff1d26a843b5b37add607/WordPress/Classes/Services/PostService%2BRefreshStatus.swift#L3-L10

That gets executed on app launch.

---

TLDR Race conditions suck.

Tagging @diegoreymendez @leandroalonso in case this information is useful for them.

[yaelirub]

After some investigation, seems like the underline issue here is with setting the remoteStatus in the awakeFromFetch method in AbstractPost to 'failed' if the post status is 'pushing'.

When commenting out the remoteStatus setting in that method, the failed status is correctly set and the post will be picked up by the autoUploader when a connection is regained.

https://github.com/wordpress-mobile/WordPress-iOS/blob/1cac04f61f73c0fab1d5a50251f16e95c420b4ee/WordPress/Classes/Models/AbstractPost.m#L43

According to the comment there:

If we've just been fetched and our status is AbstractPostRemoteStatusPushing then something
when wrong saving -- the app crashed for instance. So change our remote status to failed.

In the process of starting up the app (runStartupSequence in WordPressAppDelegate) we are calling the refreshStatus method that aims to do the same thing (convert posts with status pushing to failed) on a backgroundservice with a derived context

https://github.com/wordpress-mobile/WordPress-iOS/blob/1cac04f61f73c0fab1d5a50251f16e95c420b4ee/WordPress/Classes/Services/PostService%2BRefreshStatus.swift#L10

I'm not sure why the awakeFromFetch method causes this issue but I'd like to know if we still need to refresh the status there on the main context if we're doing it in the refreshStatus method.

@koke, I know you wrote the awakeFromFetch override a long time ago (2013) but I'm hoping maybe you could help shed some light ?
cc: @shiki

[koke]

I think the purpose of that awakeFromFetch was to do exactly the same as refreshPostStatus in a lazy way, so we only need one of those.

I think I went with that option because I was concerned about the performance hit during initialization (and maybe we weren't even using derived contexts back then). On that note, have we considered NSBatchUpdateRequest for refreshPostStatus?

[yaelirub]

Thank @koke .
I don't think we considered NSBatchUpdateRequest for refreshStatus but that's a good point.
I'll create an enhancement ticket for that and will proceed with removing the refresh code in awakeFromFetch if you have no objections

[shiki]

Done in #12567