Prevent apps upgrade through cron or remote calls

[PVince81]

In some cases it could happen that the core has already upgraded but some apps didn't.
In such case the needsUpgrade() call returns false because ownCloud core itself is already up to date.

But there is a risk that one or multiple remote cron.php or remote.php calls trigger app updates at the same time and cause duplicate keys or unexpected behavior to happen.

To prevent this we need to find a way to prevent apps to be updated from cron.php or remote.php, maybe by returning "503 not available" or throwing an exception.
Not sure whether this could have other consequences.

What do you think @icewind1991 @DeepDiver1975 @bantu ?

[PVince81]

Another idea is to add an early check needsAppUpgrades() that will check whether any enabled app needs to be updated, regardless of whether it would be loaded in the current run, and return "503 Service Not Available".

[polaris20]

Is there anything I can do to manually resolve this issue, or will a patch be necessary?

[RobBentley]

I guess this isn't much of an isolated issue as I've seen this too trying to upgrade from OC 6.0.4 to OC 7.0.0. Luckily our OC server is a VM, so i've reverted it to a snapshot I took before the attempted upgrade.

[Grobwiefein]

I’ve just disabled the files_sharing app manually in the database (oc_appconfig table). For now, the owncloud instance seems to work properly with this quick and dirty workaround, of course without any sharing capability. At least I’m able to access it again and the synchronisation seems to work as well.

Hopefully there’s an easy way to fix the issue...

[polaris20]

@Grobwiefein How did you do that? I'm not really a MySQL guru, so any assistance you can provide would be appreciated.

[PVince81]

This problem should only happen if one of the app updates failed during the core update and the app is trying to upgrade itself during "normal" ownCloud access.

One idea to fix the initial issue would be to NOT consider the core update complete if an app update failed at this time. Which means NOT setting the new OC version in the DB but keeping the old one, in which case the whole update process can be triggered again, and clients will be blocked during the usual maintenance mode during upgrade. If at this time an app update fails again, it will be repeatable and can be investigated.

About your question: some of your issues might be fixable manually in the DB but it will be in a case by case basis. This is because if app updates are happening in parallel due to multiple client requests, then it is hard to predict what could happen, like entries added multiple times, etc. So... sorry, no quick fix for that.
You need to find out what bogus DB entries there are, using error messages as clues.

Redoing the upgrade at this point from backups might not help, because we still need to find out what app update messed up in the first place during core upgrade, which should hopefully be possible after developing a patch as described above.

[Grobwiefein]

@polaris20 Just access the owncloud database (e.g. with phpmyadmin) and search for “files_sharing” in the appid row within the oc_appconfig table. Find the row with the configkey “enabled” and change the value from “yes” to “no”.

[PVince81]

Yeah, version should really be set at the very end:
https://github.com/owncloud/core/blob/master/lib/private/updater.php#L215

[PVince81]

Okay, here is an experimental patch: #9819
Note that this will NOT fix the already broken state your OC might be in.

But if you revert back to your backup, this should prevent the broken state to happen in the first place and should avoid running OC after an unsuccessful app update.

We can still look into fixing your individual DBs later if you like but I'd like to fix the core issue first.

[PVince81]

@RobBentley can you help with testing the patch with your magic VM ? Thanks a lot.

[RobBentley]

@PVince81 Sure can. Can you give me some destructions of how to get hold of the patched version?

[PVince81]

@RobBentley

1. Revert your VM to the old state
2. Download the patch file https://github.com/owncloud/core/commit/6f329dcb6cc535113fb45dd07f60a075a97a6db0.patch
3. Then "cd" into your owncloud directory
4. Run:

patch -p1 < 6f329dcb6cc535113fb45dd07f60a075a97a6db0.patch

Then run the upgrade.

[RobBentley]

@PVince81 So do the patch, then I can download the 7.0.0 tar and extract into the owncloud directory, to commence the upgrade? Just checking so I don't get it wrong :-)

[RobBentley]

@PVince81 Trying to apply the patch gives me the following...

[root@OwnCloud owncloud]# patch -p1 < 6f329dc.patch
patching file lib/private/updater.php
Hunk #1 FAILED at 212.
Hunk #2 FAILED at 227.
2 out of 2 hunks FAILED -- saving rejects to file lib/private/updater.php.rej
[root@OwnCloud owncloud]#

[PVince81]

Oh sorry... It was late.
You need to extract the 7.0.0 first, and then apply the patch. And then run the upgrade.

[PVince81]

The clean solution for the app's individual upgrades might to completely forbid on-demand update and expect users to update apps in the app store (except for apps that were successfully updated through the main upgrade). I raised this ticket to discuss this: #9856

Edit: typo

[icewind1991]

I don't think we can for on-demand updates, if the code of an app gets updated somehow, we need to update the db etc to or the apps breaks completly

[PVince81]

Then we need some kind of locking to prevent an app upgrade to happen twice simultaneously.
I think the reason why it seems to break in OC 7 but didn't in OC 6 is because the new apps are doing more upgrade changes than before.

[PVince81]

@icewind1991 can you look into that ?

Busy fixing console.php which also has loadApps in it...

[icewind1991]

Maybe we can disable apps instead of upgrading them when being loaded trough anything but the web interface (iirc we only trigger core updates from the web ui).

That way we can provide the same update process for single apps as when updating core (provide a landing page, run update tests, etc...)

[PVince81]

Yes, that's an idea. You probably meant "disable apps upgrade". We still need to load them, but disallow updating in the following cases:

1. Ajax calls
2. remote.php calls
3. cron.php
4. console.php

not sure if there are others.
Do you think there is a reliable way to detect web UI access ? index.php ?

[DeepDiver1975]

Do you think there is a reliable way to detect web UI access ? index.php ?

maybe we need to introduce some variable we set in the entrypoint (index.php, public.php, remote.php .....) which gives an indication on the usage scenario

[icewind1991]

I mean disable them completely, we can't load apps without running the upgrade due to db changes etc.

The upgrade from web ui would be done in the same place where currently checkUpgrade() for core is done

[icewind1991]

Only having the upgrade be done from the web ui would also help with cases where the upgrade failed since we can show proper errors to the user etc

[PVince81]

I have the feeling we should not only disable the apps, but directly jump out and return 503.
We probably don't want to have only half the apps loaded and carry on with an ajax call just because a single app hasn't updated.
So something along a "NeedsUpdateException()"

[icewind1991]

Yes, a 503 is probably a cleaner sollution

[icewind1991]

I will have a go at it

[PVince81]

@icewind1991 thanks a lot.

Just finished the PR for console.php to avoid app loading there as well: #9861

[PVince81]

PR from @icewind1991 to prevent concurrent app updates: #9866

Edit: wrong link

[RobBentley]

Just a note - even with the patch mentioned above, I've still ended up with the files_sharing problem after upgrading to OC7. Reverted by VM back to 6 again.

[PVince81]

Fixed by #9866

@RobBentley please try with OC 7.0.2 and do the upgrade on the command line with ./occ upgrade just to be sure you don't run into a timeout. I hope this will rule out all potential issues.