[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 2739aeb.

[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:

WordPress-iOS/WordPress/Classes/Services/PostService.m

Lines 222 to 223 in c1d3284

	post.remoteStatus = AbstractPostRemoteStatusPushing;
	[[ContextManager sharedInstance] saveContext:self.managedObjectContext];

And this:

WordPress-iOS/WordPress/Classes/Services/PostCoordinator.swift

Line 37 in c1d3284

change(post: post, status: .pushing)

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:

WordPress-iOS/WordPress/Classes/Services/PostService.m

Line 261 in c1d3284

postInContext.remoteStatus = AbstractPostRemoteStatusFailed;

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:

WordPress-iOS/WordPress/Classes/Services/PostService.m

Lines 71 to 77 in c1d3284

	NSString *entityName = NSStringFromClass([AbstractPost class]);
	NSFetchRequest *request = [NSFetchRequest fetchRequestWithEntityName:entityName];
	request.predicate = [NSPredicate predicateWithFormat:@"remoteStatusNumber == %d", AbstractPostRemoteStatusFailed];
	NSError *error = nil;
	NSArray *results = [self.managedObjectContext executeFetchRequest:request error:&error];

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:

WordPress-iOS/WordPress/Classes/Services/PostService+RefreshStatus.swift

Lines 3 to 10 in c1d3284

	/// This method checks the status of all post objects and updates them to the correct status if needed.
	/// The main cause of wrong status is the app being killed while uploads of posts are happening.
	///
	/// - Parameters:
	/// - onCompletion: block to invoke when status update is finished.
	/// - onError: block to invoke if any error occurs while the update is being made.
	///
	func refreshPostStatus(onCompletion: (() -> Void)? = nil, onError: ((Error) -> Void)? = nil) {

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.

WordPress-iOS/WordPress/Classes/Models/AbstractPost.m

Line 43 in 1cac04f

[self setPrimitiveValue:@(AbstractPostRemoteStatusFailed) forKey:@"remoteStatusNumber"];

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

WordPress-iOS/WordPress/Classes/Services/PostService+RefreshStatus.swift

Line 10 in 1cac04f

func refreshPostStatus(onCompletion: (() -> Void)? = nil, onError: ((Error) -> Void)? = nil) {

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