Skip to content

fix(dav): keep part files when overwriting on interrupted uploads - #63096

Open
solracsf wants to merge 1 commit into
masterfrom
fix/interrupted-chunk-assembly
Open

fix(dav): keep part files when overwriting on interrupted uploads#63096
solracsf wants to merge 1 commit into
masterfrom
fix/interrupted-chunk-assembly

Conversation

@solracsf

@solracsf solracsf commented Aug 9, 2026

Copy link
Copy Markdown
Member

Fix #63071

Summary

The guard deciding whether a DAV write goes through a part file asked View::isCreatable() about the part file path itself. isCreatable() answers whether something can be created inside a path, so for a file that does not exist yet it is always false and the guard collapsed to "skip the part file whenever the target is updatable" - that is, for every overwrite. An upload interrupted during assembly then truncated the user's existing file in place, while oc_filecache kept asserting the previous size and etag, so no client had any reason to re-fetch until a later occ files:scan turned the divergence into
a download of the empty file.

Ask isCreatable() about the directory that will hold the part file instead.

Restoring part files for overwrites uncovered three further problems, fixed here because the first fix is not safe without them:

  • Part file names were always hashed, although the commit introducing the hashing only meant to do so for names too long to fit. Encryption resolves a part file's key by stripping the .ocTransferId suffix, which only leads back to the target while the real name is kept, so a hashed name left an encrypted overwrite undecryptable. Hash only when the name would overflow the filesystem limit, and keep writing directly to the target when it must be.

  • AssemblyStream compared each chunk against a size it re-read while streaming, but Sabre\File::get() repairs a stale filecache entry and refreshes the node, so a chunk short on storage compared equal to itself and the assembled file was silently truncated. Snapshot the sizes at stream_open.

  • A failed storage write was reported as success whenever the request carried no content-length, which the assembly MOVE never does. On object storage that answered 204 while the previous object was still in place - a lost update the client could not detect.

Also verify the assembled file against OC-Total-Length after the move, and close both streams when an encrypted write fails: an encryption stream left open is only closed during engine shutdown, where writing back into the
storage layer crashes the process.

Checklist

AI (if applicable)

  • The content of this PR was reviewed using AI (Fable 5)

@solracsf solracsf added this to the Nextcloud 35 milestone Aug 9, 2026
@solracsf
solracsf requested a review from a team as a code owner August 9, 2026 17:54
@solracsf solracsf added the bug label Aug 9, 2026
@solracsf
solracsf requested review from leftybournes, provokateurin and salmart-dev and removed request for a team August 9, 2026 17:54
@solracsf solracsf added the 3. to review Waiting for reviews label Aug 9, 2026
@solracsf
solracsf requested a review from come-nc August 9, 2026 17:54
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
@solracsf
solracsf force-pushed the fix/interrupted-chunk-assembly branch from b5fd10d to efa4ba1 Compare August 9, 2026 20:35
@come-nc

come-nc commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

I’m not knowledeable enough on files internals to review this.
Ideally it should be split into commits for the different fixes.

@come-nc
come-nc removed their request for review August 10, 2026 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

2 participants