Overlapping uploads: a failed older upload leaves a broken version behind, and a failing newer upload can roll back into it #883

Follow-up of #838 and #839.

#839 makes sure that an older upload that fails its postprocessing no longer rolls back a file that a newer upload has replaced in the meantime. Two related problems remain, with the same cause: the version that is created for an older upload while a newer one arrives.

Background

When two uploads replace an existing file one after the other, and the first one (A) is still in postprocessing when the second one's (B) bytes are received:

1. A's bytes received: the previous content C0 is kept as version R0; A's session records R0 as the version to roll back to (MetaData["versionID"]).
2. B's bytes received: the file's metadata at that moment, which is A's, is kept as a new version RA, named after A's mtime; B's session records RA as the version to roll back to. A's content isn't written yet: when A succeeds later, DecomposedFsSession.Finalize writes A's bytes into RA ("uploading to revision node, that was created for us by another upload").

So RA only becomes a valid version if A succeeds. Until then, it is A's metadata without A's content: with decomposed, it points to A's blob, which doesn't exist yet; with posix, Tree.CreateRevision copied the file's bytes at that moment, which were still C0, so RA holds C0's bytes under A's metadata (blob id, size, checksums).

Problem 1: the version created for a failed older upload is left behind

When A fails, RA is never completed, but it stays as a version:

• decomposed: downloading it fails with HTTP 500 (A's blob was never written);
• posix: it downloads as C0, while it claims A's size and checksum.

This is the FIXME in DecomposedFsSession.Cleanup ("else if the upload has become a revision, delete the revision, or if it is the last one, delete the node") and in upload_async_test.go ("FIXME we need to delete the revision"). #839 keeps it on purpose ("leaves the revision as an ordinary version").

Problem 2: a failing newer upload rolls back into the older upload's version

When B fails, it rolls back to the version recorded in its session, RA, which only holds A's metadata, with no content, while A is still in postprocessing or has failed. The file then points to A's content, which doesn't exist:

• both fail, A first: B rolls back to RA after A failed; on main and with fix(posix): restore revisions atomically and guard upload rollback #839;
• both fail, B first: B rolls back to RA while A is still processing, and B's roll back also removes the processing status, although A isn't done. On main, A's own roll back repairs this afterwards (back to C0); with fix(posix): restore revisions atomically and guard upload rollback #839, A doesn't roll back anymore once it doesn't own the node, so the file stays broken. fix(posix): restore revisions atomically and guard upload rollback #839 makes this case worse.

All orders

From a test in pkg/storage/pkg/decomposedfs/upload_async_test.go (decomposed), with an original of 10 bytes, A of 15 bytes and B of 20 bytes: the file size and the sizes of the versions at the end.

(Not in the table: when B fails first while A is still processing, the file should be "processing" again until A is done, i.e. not downloadable; on main and with #839, it isn't marked as processing anymore, and points to A's content, which doesn't exist yet.)

Proposed fix (on top of #839)

In DecomposedFsSession.Cleanup, after a failed postprocessing:

1. The failing upload doesn't own the node anymore (the case fix(posix): restore revisions atomically and guard upload rollback #839 now keeps): delete the version that was created for it, i.e. the version named after the session's mtime (n.ID + ".REV." + session.MTime()), but only if it is still this upload's version: its blob id is the session id. That's the FIXME, and it fixes problem 1.
2. The failing upload owns the node, and the version it would roll back to was created for an older upload that hasn't succeeded (its blob id is the id of another upload session):
   • if that older upload is still in postprocessing, restore the version's metadata and mark the node as processing for that upload again (processing:<its id>), and remove the version as RevertUpload does: the node is then in the state it had before the newer upload arrived, and the older upload's Finalize (it owns the node again) or roll back (to its own versionID) works as if the newer upload had never happened;
   • if that older upload has failed already (and, with 1., deleted its version), the version doesn't exist anymore: roll back to the newest remaining version instead, which is the content the older upload had replaced (Node.DeleteRevision(ctx, "") already implements "restore the latest revision, or purge the node if there is none").

This fixes problem 2 in both orders, and gives the expected outcomes of the table above for decomposed. For posix, RevertUpload only copies metadata; in the cases of 2., the file's bytes haven't been replaced by any of the uploads (none of them succeeded), so the bytes on disk are still the ones the restored metadata describes (C0), which keeps the file consistent. This should be confirmed by tests for both drivers.

A more fundamental alternative is the one described in owncloud/ocis#8848 (open), which covers the same family of overlapping-upload cases for new files: tracking all ongoing upload sessions of a node in its metadata, so that every outcome can be resolved against the actual list of pending uploads. The fix above is local to Cleanup and doesn't need that, but owncloud/ocis#8848's approach would also resolve the size and processing status cases listed there.

Tests

• extend "two uploads overwrite an existing file in parallel" in upload_async_test.go (added in fix(posix): restore revisions atomically and guard upload rollback #839) with all orders of the table above, checking the file's size and content, its processing status, and the remaining versions (and that each of them can be downloaded);
• the same for posix, checking that the file's bytes match its size and checksum xattrs, and that each version's bytes match its own;
• enable the commented-out expectations in "the first can fail before the second succeeds" and "the first can fail after the second succeeds".