Repository navigation
fix: don't leave or roll back into the revision of a failed older upload - #884
Draft
pbleser-oc wants to merge 3 commits into
Draft
pbleser-oc wants to merge 3 commits into
pbleser-oc wants to merge 3 commits into
Conversation
RestoreRevision in the posix tree truncated the live file and copied the revision into it in place. Concurrent readers could see an empty or partially restored file, and a failed copy left the file truncated while its xattrs still described the previous content. The posix RestoreRevision now copies the revision into a temp file in the space's .oc-tmp directory, fsyncs it, carries over the target's oc xattrs, mode and owner, applies the revision's checksum, blob id, blob size and mtime, and renames it over the target, like the blobstore does for uploads. Separately, rolling back an aborted upload (Cleanup with a versionID) doesn't check that the session still owns the node. An older upload whose postprocessing fails after a newer upload has finished therefore reverts the node to the old version, replacing the newer content. Cleanup now locks the node, checks that it is still processing this session, and only then calls RevertUpload, under the same lock (the metadata lock is re-entrant for the node object). Otherwise it keeps the node and leaves the revision as an ordinary version. Assisted-by: Claude:claude-opus-5-5
RestoreRevision only carried the user.oc.* xattrs over to the restored file, so other attributes set on the file (Samba's user.DOSATTRIB, macOS com.apple.*, custom xattrs, ACLs in system.posix_acl_access) were lost with every restore. It now copies all xattrs of the target; a failure on a non-oc attribute (e.g. security.* without privileges) is logged as a warning. The setuid, setgid and sticky bits are kept too, with the chown done before the chmod since it clears them. When two uploads replace a file and the newer one fails first, its rollback restored the version it had created for the older upload, which was still in postprocessing, and cleared the processing status. The node got the metadata of an upload that never finished, and the older upload's own rollback, which now checks that it owns the node, left it like that. The newer upload now records the older upload's id when it creates the version, and if that upload is still processing when it rolls back, it marks the node as processing for it again, so that the older upload's outcome decides. Cleanup marks a kept session as failed when it rolls back, so that an aborted upload is not handed the node. Assisted-by: Claude:claude-opus-5-5
When a newer upload of a file is received while an older one is still in postprocessing, the file's metadata at that moment, the older upload's, is kept as a revision, into which the older upload writes its blob if it succeeds (see Finalize). That revision only becomes valid if the older upload succeeds. With opencloud-eu#839, two cases still went wrong (opencloud-eu#883): - When the older upload failed after the newer one had replaced the node, the revision was left behind without its content: with decomposed, it pointed to a blob that was never written (downloads failed), with posix it held the previous content's bytes under the older upload's metadata. Cleanup now deletes it, if it still belongs to the failed upload (its blob id is the session id). That's the FIXME in Cleanup, for existing and for new files. - When the newer upload failed after that, it rolled back to that deleted revision. It now rolls back to the latest remaining revision instead, the content the older upload had replaced. This also covers an older upload that was aborted. Tests: the expectations that were commented out with FIXMEs are enabled, the pending test of both uploads failing, the older one first, is enabled, and the cases where both succeed and where the aborted older upload is followed by a failing newer one check the versions. "the first can fail after the second fails" for a new file now expects the file to be deleted once both uploads failed. Builds on opencloud-eu#839. The size propagation of overlapping failed uploads (owncloud/ocis#8848) is unchanged.
pbleser-oc
force-pushed
the
fix/883-overlapping-upload-versions
branch
from
October 19, 2026 08:31
91dcc41 to
80daa79
Compare
This was referenced Oct 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #883.
Builds on #839, which is in another fork: until #839 is merged, this PR's diff contains #839's commits (
4c0efbd4d,a0ba3b373) as well. The change of this PR is the last commit,80daa79e9. I'll rebase it ontomainonce #839 is merged. Draft until then.Reworked on top of
a0ba3b373, which hands the node back to the older upload when the newer one fails (versionProcessingID) and marks an aborted session as failed: this PR no longer does that itself.What it fixes
When a newer upload of a file is received while an older one is still in postprocessing, the file's metadata at that moment, the older upload's, is kept as a revision, into which the older upload writes its blob if it succeeds (
DecomposedFsSession.Finalize). That revision only becomes valid if the older upload succeeds. With #839, two cases still go wrong (see #883 for the details and all orders):Changes
pkg/storage/pkg/decomposedfs/upload/upload.go,DecomposedFsSession.Cleanup, after a failed postprocessing:deleteRevisionCreatedForUploaddeletes the revision a newer upload created for its content, i.e. the revision named after the session's mtime, if it still belongs to this upload (its blob id is the session id). This is theFIXMEinCleanup, now also handled in the branch for new files.Node.DeleteRevision(ctx, "")), the content the older upload had replaced.Tests
pkg/storage/pkg/decomposedfs/upload_async_test.go:FIXMEexpectations are enabled (only the original version is left), the pending "restores the original file when both fail, the older one first" is enabled, "does not hand the node back to an older upload that was aborted" also checks that the original file is restored without versions, and two tests check the versions when both uploads succeed, in either order;FIXME we need to delete the revisionare enabled, and "the first can fail after the second fails" expects the file to be deleted once both uploads failed, which resolves theFIXMEthere.Disabling either part of the fix makes tests fail.
go test -raceofpkg/storage/pkg/decomposedfs/...andpkg/storage/fs/posix/...passes,golangci-lintreports no issues.End-to-end check
With an OpenCloud server, for posix and decomposed, this PR merged onto current
main(it merges cleanly): every order in which the two uploads can finish, each on a file of its own. Postprocessing was held withPOSTPROCESSING_DELAY=1h, and thePostprocessingFinishedevents were published in the order of each case (as described in my comment in #838). After each step, the script checked the file's content (or that it is still processing), downloaded every remaining version, and with posix compared the bytes of the file and of each version with theiruser.oc.blobsizeanduser.oc.cs.sha1xattrs.mainSo every case this PR changes is fixed for both drivers, and posix stays consistent: in these cases, none of the uploads that failed had replaced the file's bytes.
The two cases where both uploads succeed show a separate posix bug, unchanged by this PR and by #839, and also on
main: when the older upload succeeds after the newer one was received, its version gets the older upload's bytes, but the newer upload's size (measured: 21 bytes instead of 19), and, as alluser.oc.*xattrs are copied, its blob id and checksums. See #887.Not addressed: the size propagation of overlapping failed uploads (owncloud/ocis#8848).