[Mortimer-RR]

Fixes #838

Changes

• pkg/storage/fs/posix/tree/revisions.go RestoreRevision: copies the revision into a
  temp file in the space's .oc-tmp directory, fsyncs it, carries over the target's
  user.oc.* xattrs, mode and owner (a failed chown is logged, not fatal), applies the
  revision's checksum, blob id, blob size and type attributes and the mtime, and
  rename()s it over the target, like blobstore.Upload does for uploads. The mtime is then
  set through the metadata backend so its cache picks up the new attributes. The "current"
  copy for EnableFSRevisions is unchanged.
• pkg/storage/pkg/decomposedfs/upload/upload.go Cleanup (versionID branch): takes the
  node's metadata lock and re-reads the status through the metadata backend (bypassing the
  node's attribute cache). It only restores when the node is still processing:<this session>. Otherwise it logs, keeps the node, and leaves the revision as an ordinary
  version. The lock is released before the rest of Cleanup, whose UnmarkProcessing
  locks again through a different node object.

Tests

• pkg/storage/fs/posix/tree/revisions_test.go:
  • restore keeps the node metadata and applies the revision's blob metadata;
  • a failed copy leaves the target untouched (fails on main: target truncated to 0 bytes);
  • concurrent readers only ever see the old or the new content (fails on main: 3831 of
    3841 reads saw partial content).
• pkg/storage/pkg/decomposedfs/upload_async_test.go, "two uploads overwrite an existing
  file in parallel": the older upload fails after the newer one succeeded; the newer content
  and the original version must survive (fails on main: the file is rolled back from 20
  to 10 bytes).
• go test -race clean on pkg/storage/fs/posix/tree, pkg/storage/pkg/decomposedfs and
  its upload package. pkg/storage/... passes.
• End to end (OpenCloud, posix): restores under load with 10 readers had 0 bad downloads
  (upstream: 50 of 50 bad); aborting the older upload keeps the newer content (upstream:
  rolled back).

Note: downloads still don't take the node lock; with the atomic rename they don't need it
for consistent bytes (an open file descriptor keeps the old inode).

🤖 Generated with Claude Code

[Mortimer-RR]

Resolved the merge conflict

[micbar]

I approved the CI run.

[pbleser-oc]

Thanks for this, the guard in Cleanup is exactly what's missing. I tested the PR merged onto current main (it merges cleanly), with the unit tests and end to end with an OpenCloud server, and found two things that I think should be addressed before merging, plus one that can be a follow-up.

What I tested

• go test -race of pkg/storage/fs/posix/tree, pkg/storage/pkg/decomposedfs and .../decomposedfs/upload: pass.
• End to end, posix and decomposed, with the scenario of posix: version restore overwrites the live file in place, and an aborted upload can roll back newer content #838 (the older upload A fails after the newer upload B succeeded; postprocessing held with POSTPROCESSING_DELAY=1h, the PostprocessingFinished events published in that order, see my comment in posix: version restore overwrites the live file in place, and an aborted upload can roll back newer content #838): B's content is kept with both drivers, where main loses it (decomposed) or corrupts the file (posix).
• All orders in which two uploads replacing an existing file can finish, as a temporary test in upload_async_test.go (sizes: original 10, older upload A 15, newer upload B 20):

order	main: file / versions	this PR: file / versions	expected
A fails after B succeeded	10 / [15]	20 / [10, 15]	20 / [10]
A fails before B succeeds	10 / [15]	20 / [10, 15]	20 / [10]
both fail, A first	15 / []	15 / [10]	10 / []
both fail, B first	10 / []	15 / [10]	10 / []
A succeeds after B failed	15 / [10]	15 / [10]	15 / [10]

1. Both uploads fail, the newer one first: a regression

When B fails first, B is the latest upload, so it rolls back to the version recorded in its session. But that version is the one that was created for A's content when B's bytes were received, and A hasn't succeeded (and later fails): the node gets A's metadata, without A's content, and B's roll back removes the processing status although A is still in postprocessing. On main, A's own roll back afterwards repaired this (back to the original). With this PR, A's roll back now sees that it doesn't own the node anymore and keeps it, so the file ends up with the metadata of a failed upload whose content was never written: with decomposed, its blob doesn't exist; with posix, the file keeps the original bytes under A's metadata.

"Both fail, A first" ends the same way, on main as well as with this PR.

The root cause is the same as for the version that is left behind (see 3.): a newer upload's "previous version" can be the version of an older upload that never succeeded. Fixing that properly is probably a follow-up of its own, but since this PR changes the outcome of "both fail, B first" for the worse, I think it should at least be covered by a test here, e.g. with the expected outcome marked as a known issue, or B's roll back should skip a version whose upload hasn't succeeded.

2. RestoreRevision drops everything on the file that isn't user.oc.*

The temp file + rename makes the restore atomic, which is great, but only the user.oc.* xattrs are carried over (revisions.go, L332 ff.), and the file gets a new inode. Everything else that was set on the file is lost with every restore. Tested on posix, setting a custom xattr and an ACL on a file and restoring one of its versions through the API (COPY /remote.php/dav/meta/<fileid>/v/<version>):

after the restore	main (in place)	this PR
content	restored	restored
user.custom (a non-OpenCloud xattr)	kept	lost
ACL (setfacl -m u:nobody:r)	kept	lost
inode	unchanged	new

Since posixfs is meant to share its files with other tools (Samba keeps user.DOSATTRIB, macOS com.apple.*, admins set ACLs), I think the restore should copy all the xattrs of the target, including system.posix_acl_access (ACLs are xattrs as well, so xattr.List/Set covers them), not only user.oc.*. security.* (e.g. an SELinux label) may need privileges; maybe best effort with a warning, like the Chown. The new inode also breaks hard links to the file; that's probably an acceptable trade-off for an atomic restore, but worth a note in the code.

Related, minor: Chmod(fi.Mode().Perm()) (L318) drops setuid/setgid/sticky; for regular files probably irrelevant.

3. The version created for the failed older upload stays (follow-up)

When the older upload fails after the newer one created a version for it, that version is kept "as an ordinary version". It's broken, though: with decomposed it points to A's blob, which was never written (downloading it fails with HTTP 500; confirmed end to end with this PR); with posix it contains the original's bytes under A's metadata (it downloads as the original content, while claiming A's size and checksum). This is the known FIXME in Cleanup, so I don't think it has to be part of this PR: I'll open an issue and a PR on top of this one for it, which can also take care of 1.

Restore with watch_fs and enable_fs_revisions: fine

Since the rename into place is a new file for the watcher (IN_MOVED_TO), I checked that it doesn't trigger anything unexpected, end to end on posix with the inotifywait watcher, comparing this PR with main:

• watch_fs on, versions from uploads: restoring a version through the API (COPY /remote.php/dav/meta/<fileid>/v/<version>) restores the content, keeps the file id, leaves the size and checksum xattrs consistent with the bytes on disk, and leaves exactly one version (the replaced content). The watcher only logs "skip already known item".
• watch_fs and enable_fs_revisions on, versions from changes on disk (uploads fail in that setup, according to posix: with enable_fs_revisions every upload fails at finalize #840): same result after the restore, the .CURRENT copy holds the restored content, and the next change on disk creates a version with the restored content, as expected.

The behaviour was the same as on main in all of these. (Only the inotifywait watcher; not tested with the GPFS or CephFS watchers.)

A remark

• .oc-tmp is in the space root: if a folder within the space is on another file system (e.g. a bind mount), the rename fails with EXDEV. That's consistent with uploads, whose final rename from .oc-tmp (blobstore.Upload) has the same limitation, so just mentioning it.

[Mortimer-RR]

@pbleser-oc Thanks a lot for the thorough testing, and for the table, that made the cases very easy to follow. I pushed a commit on top that addresses 1. and 2.

1. Both fail, the newer one first

Rather than skipping the version, B's roll back now leaves the decision to the upload it belongs to. B records, when it creates the version, whether the node was still in postprocessing for an older upload (versionProcessingID in its session). When B rolls back to that version and the older upload is still processing, B hands the node back to it: the node gets A's metadata and processing:A again, so A's outcome decides. A's own roll back then reverts to the original as it did on main, and the size diff is propagated for A as well, since it owns the node again. If A succeeds instead, it finalizes onto the node as usual.

"Still processing" is checked through A's session (it exists and its status is processing). An upload whose postprocessing is aborted keeps its session, so Cleanup now marks a kept session as failed when it rolls back; otherwise B could hand the node to an upload that will never finish.

I added your orders as tests in upload_async_test.go:

order	file / versions with this commit
A fails after B succeeded	20 / [10, 15] (FIXME, 3.)
A fails before B succeeds	20 / [10, 15] (FIXME, 3.)
both fail, B first	10 / []
A succeeds after B failed	15 / [10]
A aborted, then B fails	status not left on processing
both fail, A first	pending spec (PIt) with the expected 10 / [], marked as the known issue

"Both fail, A first" and the leftover version stay for your follow-up; the FIXMEs point at #838.

This changes one existing expectation: "the first can fail after the second fails" (two uploads creating a new file) expected the status to be cleared after the second upload fails, while the first one is still in postprocessing. That's the status removal you described, so the test now expects processing there.

2. RestoreRevision and the file's other attributes

The restore now copies all the xattrs of the target to the temp file, not only user.oc.*, so user.DOSATTRIB, com.apple.*, custom attributes and ACLs (system.posix_acl_access) are kept. A failure on a user.oc.* attribute still fails the restore; for anything else (e.g. security.* without privileges) it logs a warning and carries on, like the Chown. The xattrs are set after the chmod, so an ACL leaves the mode the same as on the target. The mode now keeps setuid/setgid/sticky as well (chown before chmod, since chown clears them), and there's a comment that the new inode means hard links keep the replaced content.

A test covers a custom xattr and setgid surviving a restore. I haven't added an ACL test to the unit tests, since it depends on the test file system having ACLs enabled.

.oc-tmp and EXDEV

Agreed, same limitation as uploads. I left it as is to stay consistent with blobstore.Upload.

[pbleser-oc]

Thanks for a0ba3b373, that addresses both points of my review. I checked it:

• Restoring a revision keeps all xattrs: with posix, a custom user.* xattr, a POSIX ACL and the setgid bit survive the restore, although the file gets a new inode.
• Hand-back to the older upload: when the newer upload fails while the older one is still in postprocessing, the file is processing for the older upload again, which then finishes or rolls back properly ("both fail, B first" and "A succeeds after B failed" below).
• go test -race of pkg/storage/pkg/decomposedfs/... and pkg/storage/fs/posix/... passes.

End to end with an OpenCloud server, posix and decomposed, every order in which two overlapping uploads A (older) and B (newer) of an existing file can finish (same setup as in my comment in #838):

order	this PR
A fails after B succeeded	ok, except for a version left without its content (#883)
A fails before B succeeds	same
both fail, A first	file broken (HTTP 500 with decomposed, download error with posix): B rolls back into the version created for A, which never got its content (the pending test)
both fail, B first	ok
A succeeds after B failed	ok
both succeed, B first	ok (decomposed), version metadata wrong with posix (#887, also on main)
both succeed, A first	same
A aborted, then B fails	file broken, like "both fail, A first"

The remaining failures are all about the version created for the older upload, which is #883. I've rebased #884 onto a0ba3b373 and dropped my own hand-back in favour of yours. #884 now only deletes that version when the older upload fails, and rolls back to the latest remaining version when the one to roll back to is gone. With it, every order above passes with both drivers, except the posix metadata bug of #887. It also enables your pending test and the FIXME expectations.

From my side, this PR is good to go; #884 follows on top of it.