[pbleser-oc]

Implements the reva side of opencloud-eu/opencloud#1419: old revisions (versions) are deleted according to a configurable cleanup strategy, for the decomposed, decomposeds3 and posix drivers.

The OpenCloud side (configuration and the opencloud storage-users revisions cleanup command) follows in a separate PR in opencloud-eu/opencloud, once this one is merged.

What it does

Cleanup whenever a file gets a new revision

Triggers:

• after an upload replaced an existing file, once it is complete (with async postprocessing, once postprocessing has succeeded);
• after a revision was restored; the revision holding the content from before the restore is always kept;
• with posix and enable_fs_revisions, after a change on disk created a revision, once the new content has been copied to .CURRENT;
• in the new upload pipeline (not called yet), once per upload: when MarkProcessing clears the processing mark of its session. Since reimplement commit upload #902, CommitUpload only writes the bytes of an upload and no longer creates a revision; the revision and the processing mark come from the planned PrepareUpload (feat: OCISDEV-1058 prepare upload owncloud/reva#664, not ported yet), so clearing the mark is when the upload is done.

Files with an upload still in postprocessing are skipped.

Strategies

• auto (keeps one revision per time slot, with longer slots for older revisions),
• a number of revisions (10),
• a duration (30d; with an uppercase unit, 1D or 1W, counting from midnight),
  and combinations: 14d, auto, auto, 90d, 5, 30d, 7d, auto, 90d.

Empty or disabled (the default) keeps everything. An invalid strategy makes the driver fail at startup.

In the background by default

• a queue and workers per driver, so the cleanup doesn't slow down the write that triggered it
• requests are de-duplicated per file,
• and a file never occupies more than one worker

The worker count, the queue size and what happens when the queue is full are configurable. By default, a cleanup that finds the queue full is skipped (drop, logged with the file's location on disk), and happens with the file's next revision, so that a write never waits for the cleanup. With block, the write waits for room, but at most for a timeout (5s by default, 0 for as long as it takes), after which the cleanup is skipped as with drop: blocking without a bound would, under sustained load, slow down every write, e.g. the consumer of the postprocessing events.

A file whose space was disabled (moved to the trash) between being queued and being cleaned up is skipped, and its files are left alone.

Decomposedfs.Shutdown stops the workers.

A queued cleanup doesn't keep the request that triggered it alive: its context only carries the logger, with the request's fields and id, and it gets a trace of its own, a span revision cleanup linked to the request's span.

Node Lock

The node lock is held briefly: the blobs of deleted revisions are deleted after the lock is released, as with decomposeds3 every blob is a request. A revision only counts as deleted once its blob is; otherwise it is reported and counted as a failure.

Posix with use_space_groups

The cleanup switches to the group of the space before it touches its files (revisioncleanup.SpaceScoper), as the middleware does for user requests, also in background workers and in on-demand runs.

On Demand

Decomposedfs.CleanupRevisionsUnchecked (the revisioncleanup.OnDemand interface) cleans up all spaces, given spaces, or a file or folder within a space.

With posix, it can also clean up a file or folder given by its path on disk.

It supports

• a dry run,
• its own strategy and async settings,
• a timeout with a graceful stop,
• and a random order for runs that can't cover everything at once.

It reports a Result for every file.

It needs no user in the context and checks no permissions, as it is meant for command line tools.

Prometheus Metrics

reva_revision_cleanup_*: deleted revisions and bytes, failed deletions, queue length, capacity and full events, workers.

A Reason for Every Decision

Logged at debug level for every deleted revision, e.g. deleted by "auto" (last hour): its slot of 1 minute starting … already keeps a newer revision.

Configuration

New decomposedfs options, which posix inherits:

option	default
revisions_cleanup_strategy	"" (disabled)
revisions_cleanup_async	true
revisions_cleanup_async_workers	0 (half the CPUs, at least 2)
revisions_cleanup_async_queue_size	0 (64 per worker)
revisions_cleanup_async_queue_full	drop
revisions_cleanup_async_queue_full_timeout	5s (for block; 0 waits as long as it takes)

Changes to shared code

• Naming of functions without permission checks: they are named after their checked counterpart with an Unchecked suffix, so that they aren't used by mistake where permissions must be checked. For that, a separate commit renames the existing Node.DeleteRevision to Node.DeleteRevisionUnchecked, as Decomposedfs.DeleteRevision, of the same name, checks permissions. The functions of the cleanup that keep their names, e.g. Cleaner.Cleanup, say in their doc comment that they check no permissions.
• node.Tree.ListRevisionsUnchecked lists revisions without permission checks; ListRevisions now checks permissions and calls it. There is also a new ListFolderKnownUnchecked for the on-demand walk, which, unlike ListFolder, checks no permissions and lists only the children known to the storage: with posix, it skips files created on disk that aren't assimilated yet instead of assimilating them, so that a cleanup, a dry run in particular, changes nothing but the revisions it deletes.
• Deleting a revision needs no driver-specific code. revisioncleanup.PurgeRevisionUnchecked validates the key (node.ValidateRevisionKey, which rejects keys that would turn into an empty version), then calls Node.DeleteRevisionUnchecked and Tree.DeleteBlob.
• aspects.Aspects has a new field, RevisionsCleanup. The posix test env passes it on, as posix.New does.
• UnixMapper.ScopeUserByIds, which the cleanup calls for every file with use_space_groups, leaves the goroutine locked to its OS thread when it fails. That would pin a background worker to its thread for good; the fix is in its own PR, fix: unlock the OS thread when ScopeUserByIds fails #899.
• Decomposedfs.MarkProcessing, new upstream, triggers the cleanup, see above.
• spaceidindex.Index.Indexes lists the indexes, so that "all spaces" covers every space type.
• posixFS forwards CleanupRevisionsUnchecked.
• The strategies live in the new package pkg/storage/revisions, which only plans what to keep and what to delete and touches no storage. It rejects X1, X2 pairs of the same kind, with the equivalent single value in the error. A number of revisions is counted in absolute terms. Only days and weeks can count from midnight, with the uppercase units D and W.

Documentation

• pkg/storage/pkg/decomposedfs/revisioncleanup/README.md, for operators: options, strategies with examples, how the age of a revision is measured, when the cleanup runs, metrics with PromQL examples, logging.
• pkg/storage/pkg/decomposedfs/README.md, new: how the three drivers are built on decomposedfs, with diagrams. It documents the code as it is on main, so it has a commit of its own, the first one, together with package comments for decomposedfs, decomposed, decomposeds3 and posix that point to it. The feature commits then add the few lines about the revision cleanup.

Review notes

• Fifteen commits: the documentation of the existing drivers and the Node.DeleteRevision rename first, then one per step of the feature. The first four steps build the cleanup bottom up: the strategies, listing and deleting revisions, the cleaner that applies a strategy to a node, and only then the wiring that calls it. Each commit builds and passes its tests. Reviewing commit by commit is probably easier than the whole diff (70 files, ~8900 lines, more than half of them tests).
• Overlapping async uploads: the processing status only tracks the latest upload of a file, so the cleanup can run while an older upload is still in postprocessing. That is safe. If the cleanup deletes the revision the older upload writes into, the older upload creates it again. And the revision the older upload would roll back to must not be restored after a newer upload finished. Separate, pre-existing bugs around overlapping uploads: posix: version restore overwrites the live file in place, and an aborted upload can roll back newer content #838 / fix(posix): restore revisions atomically and guard upload rollback #839, Overlapping uploads: a failed older upload leaves a broken version behind, and a failing newer upload can roll back into it #883 / fix: don't leave or roll back into the revision of a failed older upload #884, posix: when an older upload succeeds after a newer one was received, its version gets the newer upload's metadata #887.
• Changes to the backported upload pipeline (pkg/storage/pkg/decomposedfs/commit_upload.go, from Backport wp1 715 #882 and Backport wp1 716 #888, @dragonchaser): MarkProcessing now triggers the revision cleanup when it clears the processing mark of an upload, and releases the node lock a bit earlier for it, before the cleanup is handed over, as RestoreRevision does (a cleanup waiting for room in a full queue must not hold the lock). CommitUpload no longer triggers it: since reimplement commit upload #902 it doesn't create a revision anymore. When PrepareUpload and the coordinator are ported, every path through the coordinator, including the failure path, should end with MarkProcessing(false), otherwise the cleanup of that file waits for its next revision. Further backports of that code may need to be adapted to these changes.
• The age of a revision is the modification time of its content, not the time it became a revision. A file unchanged for two months and then changed gets a revision that is two months old, so 30d deletes it right away. This is documented, with ways to avoid it (5, 30d, auto). Whether to record the replacement time instead is open for discussion in Revision cleanup: should the age of a revision be the age of its content, or the time it was replaced? opencloud#3735.

Follow-ups

• Per-space revision cleanup strategies opencloud#3733: strategies per space (and maybe per file type).
• Unify the deletion of single revisions in decomposedfs opencloud#3734: Decomposedfs.deleteRevisionFile to use revisioncleanup.PurgeRevisionUnchecked, to get the key validation on that path too.
• Revision cleanup: should the age of a revision be the age of its content, or the time it was replaced? opencloud#3735: content age vs. replacement age.

The legacy copy pkg/storage/utils/decomposedfs (ocis and s3ng drivers) is left unchanged; it gets no revision cleanup.

Tests

Unit tests for the strategies and the parser (including the reasons), the Service (de-duplication, queue full policies, shutdown, Wait, the context of a queued cleanup), the metrics, and the cleanup on real decomposedfs and posix trees: uploads, async postprocessing, MarkProcessing clearing the processing mark of its session, restores, changes on disk, overlapping uploads, blobs deleted only after the node lock is released, and on-demand runs including dry runs, timeouts, paths on disk and spaces given twice. Every commit builds and passes go vet, golangci-lint and the tests; make test and make lint pass on the head. The integration tests (tests/integration) fail locally in the same way with and without this PR: 8 public-link specs fail on main too, and the whole suite runs into its 15 minute timeout. End to end, the OpenCloud PR adds Behat acceptance tests for the command and for the configured strategy.

Not tested: the actual switch to the group of a space with use_space_groups. The tests check that the cleanup asks for it (with a stand-in) and that the posix tree puts the group of the space root into the context, but switching the fsgid needs CAP_SETGID, and as root file permissions don't apply. The switch itself is the existing UnixMapper.ScopeUserByIds, which the middleware and the assimilation use too; what's new is only that the cleanup calls it. A check on a server with use_space_groups is still to be done.

[butonic]

Ok, I could lament that some of the changes are not really necessary for the retention, but then again, they all prepare the code to be testable. Ultimately, they make the retention feature properly testable.

I'm happy to merge this now and change the implementation after we decided opencloud-eu/opencloud#3735, unless someone thinks we should have a non-surprising retention policy from the start.

[butonic]

two comments from a local review:

pkg/storage/pkg/decomposedfs/revisioncleanup/service.go, Service.send:

With the default block policy, a driver Service blocks the calling request goroutine (FinishUploadDecomposed, MarkProcessing, CommitUpload, RestoreRevision) until a worker frees a queue slot: stopped is only set for on-demand runs, so the request's context is never considered. This is documented as intentional backpressure and can't deadlock (the node lock is released before enqueueing, workers never block in send), but under sustained load with a slow blobstore (decomposeds3) it directly throttles uploads — and with FinishUploadDecomposed's context.WithoutCancel, even a disconnected client keeps waiting.

Options worth considering: default to drop (nothing is lost permanently, the node is cleaned up again at its next revision, and the on-demand run covers the rest), or make block bounded (wait for room up to a timeout, then drop and count it on reva_revision_cleanup_queue_full_total).

Since this can be changed and is disabled by default, not a merge blocker and we can revisit this after a loadtest.

---

pkg/storage/pkg/decomposedfs/revisioncleanup/service.go, Service.cleanupNode:

node.ReadNode(ctx, s.lu, spaceID, nodeID, "", false, nil, false) passes canListDisabledSpace=false, so a queued cleanup of a file in a disabled (trashed) space fails with "could not read node" and is reported as an error result — while the on-demand path (Decomposedfs.revisionCleanupRoot) explicitly distinguishes disabled spaces and reports them as skipped. Cosmetic, but the worker path could do the same (read with true, check IsDisabled, report Skipped) so disabled spaces don't show up as cleanup failures in the results/logs.

[pbleser-oc]

Thanks, both fixed, amended into the commits they belong to:

1. Unbounded block: block now waits at most revisions_cleanup_async_queue_full_timeout (default 5s, 0 for as long as it takes), then skips the cleanup of the node like drop, logs it with the node's location on disk, and counts it as dropped in reva_revision_cleanup_queue_full_total (after blocked when it started to wait). In addition, drop is now the default, so that a write never waits for the cleanup unless configured to. The on-demand runs are the exception: they queue files as fast as the walk finds them, so dropping them would skip most of them. Without AsyncOptions of their own, they wait for room as long as it takes, and the opencloud storage-users revisions cleanup command defaults to --queue-full block with --queue-full-timeout 0.
2. Disabled space: cleanupNode now reads the node with canListDisabledSpace=true, and skips it if its space is disabled. On-demand runs report it as skipped ("the space is disabled") instead of as an error.

[pbleser-oc]

Issues and PRs connected to this one, grouped by how they relate to it:

Merge order

1. this PR
2. feat: clean up old revisions according to a cleanup strategy opencloud#3778, configuration and opencloud storage-users revisions cleanup command. It temporarily uses this PR's branch from my fork; once this PR is merged, that becomes a regular reva bump.
3. docs(admin): document the cleanup of old file versions docs#1222, admin docs for the configuration and the command of feat: clean up old revisions according to a cleanup strategy opencloud#3778

Independent, in any order: #899, ScopeUserByIds leaves the goroutine locked to its OS thread on errors. Found here, as the cleanup's background workers call it for every file with use_space_groups.

Follow-ups of this PR

• Per-space revision cleanup strategies opencloud#3733, cleanup strategies per space, maybe per file type
• Unify the deletion of single revisions in decomposedfs opencloud#3734, use this PR's revisioncleanup.PurgeRevisionUnchecked to delete single revisions too (needs this PR)
• Revision cleanup: should the age of a revision be the age of its content, or the time it was replaced? opencloud#3735, should the age of a revision be the age of its content or the time it was replaced (decision pending, see the discussion there)

Overlapping uploads

Found while checking how the cleanup copes with a file that gets a new upload while an older one is still in postprocessing:

• posix: version restore overwrites the live file in place, and an aborted upload can roll back newer content #838, a version restore overwrites the live file in place, and an aborted upload can roll back newer content (confirmed and detailed in a comment)
  • fix(posix): restore revisions atomically and guard upload rollback #839, fix for posix: version restore overwrites the live file in place, and an aborted upload can roll back newer content #838 (reviewed)
    • Overlapping uploads: a failed older upload leaves a broken version behind, and a failing newer upload can roll back into it #883, what remains after fix(posix): restore revisions atomically and guard upload rollback #839: a failed older upload leaves a broken version behind, and a failing newer upload can roll back into it
      • fix: don't leave or roll back into the revision of a failed older upload #884, fix for Overlapping uploads: a failed older upload leaves a broken version behind, and a failing newer upload can roll back into it #883, builds on fix(posix): restore revisions atomically and guard upload rollback #839 (draft until fix(posix): restore revisions atomically and guard upload rollback #839 is merged). Whichever of fix: don't leave or roll back into the revision of a failed older upload #884 and this PR is merged second has to call Node.DeleteRevisionUnchecked, as this PR renames Node.DeleteRevision.
• posix: when an older upload succeeds after a newer one was received, its version gets the newer upload's metadata #887, posix: when an older upload succeeds after a newer one, its version gets the newer upload's metadata. Found while testing fix: don't leave or roll back into the revision of a failed older upload #884, but unrelated to it; the same on main.

posix

• posix: the .CURRENT copy of enable_fs_revisions is written without the node lock, and lost when a process exits opencloud#3769, the .CURRENT copy of enable_fs_revisions is written without the node lock and lost when the process exits. This PR's cleanup runs on the revisions that posix creates from that copy.
• posix: the scan switches to a space's group without ever switching back, so its workers use an unpredictable group #912, the assimilation switches to a space's group (use_space_groups) without ever switching back, and can crash the process after enough scans. Found while looking into fix: unlock the OS thread when ScopeUserByIds fails #899; this PR's cleanup workers do switch back.

[pbleser-oc]

I added one commit after approval, 2bcbce0

Why: opencloud storage-users revisions cleanup (opencloud-eu/opencloud#3778) runs its own storage driver in its own process, next to the running server. With the default in-memory file metadata cache (STORAGE_USERS_FILEMETADATA_CACHE_STORE=memory), the server doesn't learn about the revisions the command deletes: their entries stay in the server's cache, for up to 24 hours. The same problem with opencloud posixfs scan is opencloud-eu/opencloud#3804.

The command only deletes revisions, so the only stale entries are those of deleted revisions. But RestoreRevision and DownloadRevision trusted that metadata without checking the disk. Reproduced locally with a server and the command built from these branches:

restore of a revision the command deleted	restore	file afterwards
decomposed, the server still has the revision's metadata cached (e.g. the Versions panel listed it)	HTTP 204, reported as successful	broken: GET returns 500, the node points to the revision's deleted blob; the previous content survives as a new revision
decomposed, server restarted before the restore	HTTP 500	intact
posix, still cached	HTTP 500	intact

All three cases also left a revision of the current content behind, as RestoreRevision created it before reading the revision.

The fix:

• Decomposedfs.RestoreRevision checks that the revision exists on disk, under the node's lock (which deleting a revision takes too), before it creates the revision of the current content; otherwise it drops the stale cache entry and returns NotFound.
• Tree.DownloadRevision (decomposed and posix) checks the same and returns NotFound.

With the fix, the stale-cache restore above returns HTTP 404, the file stays intact, and no revision is left behind. The new tests in revisions_test.go simulate a revision deleted by another process (cache entry kept, files removed) and fail without the fix. They cover decomposed only: the posix test helper builds its tree without permissions, so the posix DownloadRevision change has no test of its own.

This affects any stale metadata cache (e.g. several storage-users instances with the memory store), not only the cleanup command. The remaining parts belong to the other PRs: in opencloud-eu/opencloud#3778, failing fast without NATS and reacting to Ctrl+C while the command opens the driver (it has the problem of opencloud-eu/opencloud#3805), and in opencloud-eu/docs#1222, describing what running the command next to the server implies.

[pbleser-oc]

@aduffeck could you review 2bcbce0? It touches RestoreRevision for both drivers and the posix DownloadRevision, see the comment above.