data loss: named versions are lost from group folder after a while

[sena73]

"Versions" extension is not working properly with the "Group folder" extension in NC 29.

If I create named versions of a file in a group folder, they are created, but disappear after a while (noticed after days).

Old versions simply disappear in a group folder without any notice or log message. I would tell it is a severe bug, as I do not see a way to recover deleted versions.

I understand that it gonna take time to fix. But please, make at least this 2 extensions conflicting, so people will not be able to install both of them simultaneously. Or better make "Versions" menu disabled for the files in "Group folder".

I found other bugs that indicate problems when both extensions are working:

#2965 #2343

[joshtrichards]

If I create named versions of a file in a group folder, they are created, but disappear after a while (noticed after days).

You may be correct. I suspect we're expiring labeled (named) versions within GF. Offhand I don't see the GF Versions handler checking for labeled versions in the same way that the non-GF versions does:

https://github.com/nextcloud/server/blob/2480567005776cc96d543b52992c83a41d75d1a6/apps/files_versions/lib/Storage.php#L569-L570

This may have been lost during the #2543 -> #2857 jump though offhand I don't see it in the former either.

Cc: @artonge

[artonge]

@joshtrichards is right, we do have the check for "normal" versions, but I missed to implement it for groupfolders' versions.

[joshtrichards]

Thanks for reporting this, @sena73. Will be fixed in the next release!

[XueSheng-GIT]

@artonge @joshtrichards I've upgraded groupfolders to 17.0.4 (NC 28.0.7) which includes the backport of #3213 but named versions still expire!

Can you double check and re-open this issue? Or shall I create a new one?

[joshtrichards]

I believe that API is only applicable to >28. I'm not certain offhand, but you may be right as far as NC 28 goes. I think we'd have to do something unique just for 28 🤔 built on #2543.

Aside: Are you sure you're running 17.0.4? It is only for >=29.

[XueSheng-GIT]

@joshtrichards I'm on NC 29.0.7 (28 was a typo). Sorry for that confusion.

[XueSheng-GIT]

I did some tests on NC 30.0.1 RC2 with groupfolders 18.0.2 and can reproduce the issue (labled versions expire over time). Thus, it seems it's not only related to older NC versions < 28.

@joshtrichards the question still is whether this issue needs to be reopened or a new one created?

[XueSheng-GIT]

@artonge Sorry to bug again. But I lost a lot of labled versions lately which were saved in groupfolders.
Looking at #3213 it seems the main change regarding expiry was done in regards to function getExpiredVersions:

groupfolders/lib/Versions/ExpireManager.php

Lines 108 to 135 in 37174bb

	public function getExpiredVersion(array $versions, int $time, bool $quotaExceeded): array {
	if ($this->expiration->shouldAutoExpire()) {
	$autoExpire = $this->getAutoExpireList($time, $versions);
	} else {
	$autoExpire = [];
	}
	$versionsLeft = array_udiff($versions, $autoExpire, function (IVersion $a, IVersion $b) {
	return ($a->getRevisionId() <=> $b->getRevisionId()) *
	($a->getSourceFile()->getId() <=> $b->getSourceFile()->getId());
	});
	$expired = array_filter($versionsLeft, function (IVersion $version) use ($quotaExceeded) {
	// Do not expire current version.
	if ($version->getTimestamp() === $version->getSourceFile()->getMtime()) {
	return false;
	}
	// Do not expire versions with a label.
	if ($version instanceof IMetadataVersion && $version->getMetadataValue('label') !== null && $version->getMetadataValue('label') !== '') {
	return false;
	}
	return $this->expiration->isExpired($version->getTimestamp(), $quotaExceeded);
	});
	return array_merge($autoExpire, $expired);
	}

On the first view it returns the versions which shall expire. Those consists of two parts: $autoExpire and $expired

It seems #3213 does only protect labeled versions for $expired (=> quotaExceeded). But regular auto expire done by getAutoExpireList and reflected by $autoExpire does not seem to include any check for labled versions. Thus, I assume it is expected that labled versions are deleted!

I recommend to reopen this issue again until finally fixed.

CC @provokateurin

[XueSheng-GIT]

Just in case somebody stumbles over this, I now changed the following as quick fix to stop auto expiry of labelled versions. Maybe not the cleanest solution but it seems to work.

diff --git a/lib/Versions/ExpireManager.php b/lib/Versions/ExpireManager.php
index e4f63bb29..97012e0eb 100644
--- a/lib/Versions/ExpireManager.php
+++ b/lib/Versions/ExpireManager.php
@@ -66,8 +66,11 @@ protected function getAutoExpireList(int $time, array $versions): array {
 			while ($newInterval) {
 				if ($nextInterval === -1 || $prevTimestamp > $nextInterval) {
 					if ($version->getTimestamp() > $nextVersion) {
-						//distance between two version too small, mark to delete
-						$toDelete[] = $version;
+						// Do not expire versions with a label.
+						if ($version->getMetadataValue('label') === null || $version->getMetadataValue('label') === '') {
+							//distance between two version too small, mark to delete
+							$toDelete[] = $version;
+						}
 					} else {
 						$nextVersion = $version->getTimestamp() - $step;
 						$prevTimestamp = $version->getTimestamp();

[artonge]

@XueSheng-GIT, could you open a PR applying the changes done in #3213 to getAutoExpireList ?

[XueSheng-GIT]

@artonge thanks for reopening the issue. I'm not sure whether I get your request in the right way. The diff mentioned in #3095 (comment) does already change getAutoExpireList in the way #3213 handles it for the out of quota versions.

I created a draft pull accordingly: #3393
If this wasn't your intention, I'll close it again.