fix(trash bin): Improve clarity of files retention config

[artonge]

[artonge]

I would keep the parenthesis or some form of it, to make it clear that deletion may never happen.

I think deletion will always happen if the quota is reached.

Also, quota can be infinite, in which case it will still delete files when storage is full I think?

I am not sure, at least I haven't found any bit of logic for that. I think expiration is only based on user quota.

[come-nc]

I would keep the parenthesis or some form of it, to make it clear that deletion may never happen.

I think deletion will always happen if the quota is reached.

But quota may never be reached. That is what is clearer with the previous parenthesis content.

Also, quota can be infinite, in which case it will still delete files when storage is full I think?

I am not sure, at least I haven't found any bit of logic for that. I think expiration is only based on user quota.

Hum, I don’t know, I know that shown quota is storage space when no quota is set, but I do not know for expiration. It should be fixed if it’s not currently the case though, as there is no reason to not expire or full storage if we do it for full quota?

[artonge]

Changes need to be done in server: https://github.com/nextcloud/server/pull/55834

[tcitworld]

Hum, I don’t know, I know that shown quota is storage space when no quota is set, but I do not know for expiration. It should be fixed if it’s not currently the case though, as there is no reason to not expire or full storage if we do it for full quota?

@artonge @come-nc mind giving a look at https://github.com/nextcloud/server/pull/55742 which should address that?

[artonge]

mind giving a look at nextcloud/server#55742 which should address that?

@tcitworld I don't get your changes in that linnked PR.

I understand that deleteExpiredFiles was purposefully not based on the quota to only expire files that are older than the max retention time. So it feels like a duplicate to change it like that, as we then call deleteFiles which will do the same, no?

However, it seems to make sense to replace the call to Trashbin::deleteExpiredFiles($dirContent, $uid); with Trashbin::expire($uid); in order to actually remove trashed files if the quota is reached.

Can you clarify if I understand your changes properly? And you think that they still make sense? Also, would definitely appreciate your feedback on the doc update :).

[tcitworld]

I understand that deleteExpiredFiles was purposefully not based on the quota to only expire files that are older than the max retention time. So it feels like a duplicate to change it like that, as we then call deleteFiles which will do the same, no?

Indeed, thanks for your vigilance! I simply completely missed that deleteFiles was indeed calling isExpired properly (bad navigation in my IDE because of lack of proper typing). :see_no_evil:

I'll revert 195d347240ed370d2ddcfc8f985c35bebe83ab8c and keep cea23fb53d58e5173f745adf61f22345651cca1f.

However, my initial issue still stands. If there's no quota set and little or no available space left on disk then the trashbin is not cleaned. We can at least change $availableSpace < 0 to $availableSpace <= 0 in deleteFiles so that when the disk is really full it triggers a cleanup (because if no user quota is set $availableSpace will always be 0 or above), but I wonder if I should just remove the if ($softQuota) { condition in calculateFreeSpace so that it applies as well to the case with no space left on disk but not user quota set.

Possibly there's a need for a allow_trashbin_emptying_when_disk_full setting to make sure.

This whole thing needs a huge refactoring :sob:

[artonge]

I'll revert 195d347240ed370d2ddcfc8f985c35bebe83ab8c and keep cea23fb53d58e5173f745adf61f22345651cca1f.

Make sense, thanks!

We can at least change $availableSpace < 0 to $availableSpace <= 0 in deleteFiles

Make sense too!

but I wonder if I should just remove the if ($softQuota) { condition in calculateFreeSpace so that it applies as well to the case with no space left on disk but not user quota set.

I don't think so. If there is no quota, then we don't need to compute the soft quota because we have "unlimited" space, no?

This whole thing needs a huge refactoring 😭

Yeah, the whole code is really hard to understand :people_hugging:

[tcitworld]

If there is no quota, then we don't need to compute the soft quota because we have "unlimited" space, no?

I mean when we actually don't have unlimited space, when Filesystem::free_space('/') returns a low value ("hard quota"). So that we make sure the disk isn't filled just because big trashed files of a quota-less user are not expired.

[artonge]

I mean when we actually don't have unlimited space, when Filesystem::free_space('/') returns a low value ("hard quota"). So that we make sure the disk isn't filled just because big trashed files of a quota-less user are not expired.

That would allow Nextcloud to free some space before actually having a filled disk? In other words, if a user stores more trashed files than 50% of the free disk space, then some files will be trashed for good.

Not sure, I think for now we should stick to the documented behaviour instead of preemptively deleting files.

1. This will only push back the issue for some time.
2. If a disk is full, I think it is not Nextcloud business, the sysadmin should be alerted in some way.