replace all rmdir by a recursive variant

[caco3]

Superseded by #510

---

This PR tries to solve the failing updates due to issues on removing the old files.
See #158 for details.

Issue

Updater fails to delete old files:

updater log on first deletion failure:

2023-10-23T00:06:59+0200 xGP6zzWOT0 [info] startStep("9")
2023-10-23T00:06:59+0200 xGP6zzWOT0 [info] deleteOldFiles()
2023-10-23T00:07:25+0200 xGP6zzWOT0 [info] config sample exists
2023-10-23T00:07:25+0200 xGP6zzWOT0 [info] themes README exists
2023-10-23T00:07:25+0200 xGP6zzWOT0 [error] POST request failed with other exception
2023-10-23T00:07:25+0200 xGP6zzWOT0 [error] Exception: Exception
Message: Could not rmdir: /home/xxx/www/nextcloud/updater/../lib/l10n
Code:0
Trace:
#0 /home/xxx/www/nextcloud/updater/index.php(1388): Updater->deleteOldFiles()
#1 {main}
File:/home/xxx/www/nextcloud/updater/index.php
Line:918

The general issue seems to be that rmdir only is able to delete empty folders.
The first time the issue occurs for me is on deleting the lib/l10n folder.
Looking on the folder, it clearly is not empty at that time:

> ls
af.json    az.js      br.js      el.js      es_AR.json es_EC.json es_PR.json fo.js      gd.json    hy.js      id.json    km.js      lb.js      mk.json    nl.json    pt_BR.js   si.js      th.js      vi.json
an.js      az.json    bs.js      el.json    es_CL.js   es_HN.json es_UY.json fo.json    he.js      hy.json    is.json    kn.js      lb.json    ms_MY.js   oc.js      pt_BR.json sk.js      th.json    zh_CN.js
ar.js      be.js      cs.json    eo.js      es_CL.json es_MX.json et_EE.json fr.js      hr.json    ia.js      it.js      kn.json    lo.json    nb.json    pl.js      pt_PT.js   sq.js      tk.json    zh_TW.js
ast.js     bg.json    da.js      eo.json    es_CO.js   es_NI.json eu.json    fr.json    hu.js      ia.json    ja.js      ko.js      lt_LT.json ne.json    pl.json    pt_PT.json sq.json    ur_PK.js
ast.json   bn_BD.js   de_DE.json es_419.js  es_DO.js   es_PE.js   fi.json    gd.js      hu.json    id.js      ka_GE.js   ko.json    lv.json    nl.js      ps.js      sc.json    ta.json    ur_PK.json

Note that it still contains 93 files. Odly, originally it contained 200 files, so some successed to get deleted...

Fix

The linked PHP doc shows a snipped of of a recursive variant (rrmdir) which recursively deletes all contained files/folders first.

Using that variant, I am successful to update Nextcloud 27.0.0 to the latest (currently 27.1.2).

[solracsf]

This should be integrated into https://github.com/nextcloud/updater/blob/master/lib/Updater.php too.

[marcelstoer]

Thanks @caco3 👍

[caco3]

This should be integrated into https://github.com/nextcloud/updater/blob/master/lib/Updater.php too.

Thanks, I just updated it, but I do not know how to test this.

[caco3]

Thanks @caco3 👍

Are you a user with this issue? Can you confirm it works?

Whenever I try it, it seems to be wsuccessful until the last step wheere one has to press the button to disable the maintenance mode. How ever this always runs into an error 500! I do not know if this is related or another issue on hostpoint.

I tested it with a MariaDB and as well with sqlite. With sqlite, if I replace the db file with the one before the update, and reload the page, it starts the DB migration which then works fine.

[marcelstoer]

Yep, I'm from here #158 (comment), another Hostpoint victim. Didn't get around to testing your changes yet.

[caco3]

Yep, I'm from here #158 (comment), another Hostpoint victim. Didn't get around to testing your changes yet.

please do so, but not with your life instance!

[caco3]

I did a comparison of 2 installations and an update from 27.0.0 to 27.1.2 with the original updater and with this patched updater.
Most of the files between both installations where identical afterwards. The differences where only due to different paths, hashes and timestamps:

Because of this I believe my fix is save to be merged.

I run this test on my Synology NAS.
Not that there still seems to be a different issue on hostpoint.ch: #509, how ever it is most likely not related to this fix.

[caco3]

@MorrisJobke, @LukasReschke or @solracsf would you be so kind and have a look on this fix?

[LukasReschke]

I don't work on Nextcloud anymore since several years.

[kesselb]

updater/index.php

Lines 824 to 848 in 55eca48

	$iterator = new \RecursiveIteratorIterator(
	new \RecursiveDirectoryIterator($folder, \RecursiveDirectoryIterator::SKIP_DOTS),
	\RecursiveIteratorIterator::CHILD_FIRST
	);
	$directories = [];
	$files = [];
	foreach ($iterator as $fileInfo) {
	if ($fileInfo->isDir()) {
	$directories[] = $fileInfo->getRealPath();
	} else {
	if ($fileInfo->isLink()) {
	$files[] = $fileInfo->getPathName();
	} else {
	$files[] = $fileInfo->getRealPath();
	}
	}
	}
	foreach ($files as $file) {
	unlink($file);
	}
	foreach ($directories as $dir) {
	rmdir($dir);
	}

What you implemented is already there. Actually, two times already. The iterator is creating a tree and sorting the files before the folders. Then we iterate over it and put it into files and directories, and delete the files first and the directories after.

I understand there is an issue for you and some others, but if that code would not work in general, we would have a lot more bug reports.

What we could do is to improve the above code and read the unlink/rmdir return value. If deleting a file fails, we could indeed retry or at least print an error.

[caco3]

@kesselb Yes, the existing algo seems not to work as expected for some users. I will try to extend the existing algo with some log emasages/check to see what goes wrong.
On the other side, the proposed rrmdir works without issues and is trivial.

What is the easiest to extract more info? Should I just use $this->silentLog()?

[marcelstoer]

@kesselb I noticed this as well when I briefly scanned that (to me) unfamiliar code base. However, the chosen approach obviously doesn't consider all possible system states. Otherwise, the directory to delete at the end of the algorithm (rmdir($dir);) would actually be empty. It isn't and, therefore, the proposed fix does actually make a difference.

In fact, I don't understand and don't have the project history information to understand why this rather complicated iterator and sorting algorithm exists in the first place when the much simpler rrmdir() function could likely fully replace it.

[caco3]

Dear @kesselb

You are right, the referenced recursiveDelete() works as needed.
How ever there are at least 2 direct rmdir() outside this function which fail because the directory is not empty. My proposal is to replace all those rmdirs with recursiveDelete(), see #510
Note that I also fixed some other minor bugs/missing checks.

[caco3]

PR replaced by #510

[caco3]

@marcelstoer I came to the same conclusion and fixed it in #510.
And I also agree that the used recursiveDelete() is over complicated and just using rrmdir() would be simpler.

I do not have much experience with iterators, maybe they work more efficient memory wise instead of using ugly recursive calls.

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!