fix(dav): keep part files when overwriting on interrupted uploads - #63096
Open
solracsf wants to merge 5 commits into
Open
fix(dav): keep part files when overwriting on interrupted uploads#63096solracsf wants to merge 5 commits into
solracsf wants to merge 5 commits into
Conversation
solracsf
requested review from
leftybournes,
provokateurin and
salmart-dev
and removed request for
a team
August 9, 2026 17:54
solracsf
force-pushed
the
fix/interrupted-chunk-assembly
branch
from
August 9, 2026 20:35
b5fd10d to
efa4ba1
Compare
Contributor
|
I’m not knowledeable enough on files internals to review this. |
come-nc
removed their request for review
August 10, 2026 07:44
…aims AssemblyStream compares what it read from each chunk against the size that chunk reports, but it asks for that size again while streaming. Sabre\File::get() repairs a filecache entry that disagrees with the storage and refreshes the node on the way past, so a chunk that was short on storage ends up compared against its own repaired size, matches, and the assembled file is silently truncated. Snapshot the sizes when the stream is opened and compare against those. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
Encryption::writeStream() only closed the source and target streams when the copy returned. If the source throws part way through, both were left open and the encryption stream was closed during engine shutdown instead, where writing back into the storage layer is no longer safe and takes the process down. Close them in a finally, the way Common::writeStream() already does. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
… move The chunks are checked against the length the client declared before the move, but nothing checks what actually landed. An assembly cut short leaves the destination truncated while the response still reports the upload as a success. Compare the destination against the declared length once the move is done. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
…-length A failed write was only reported when the request declared a content-length to check against. The MOVE that assembles a chunked upload never declares one, so on object storage a write that the backend rejected was answered with 204 while the previous object was still in place - a lost update the client cannot see. A write that failed is a failure whether or not a size was declared. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
…annot destroy the target 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, and keep the part file out of the two cases where it cannot stand in for the target: - 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. - A single file share maps the target and nothing else, so a part file named beside it lands on a different storage - the recipient's own, under their quota - rather than next to the file being written. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
solracsf
force-pushed
the
fix/interrupted-chunk-assembly
branch
from
August 10, 2026 08:13
efa4ba1 to
222b54f
Compare
Member
Author
|
@come-nc split and pushed. |
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
3 participants
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.
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, whileoc_filecachekept asserting the previoussizeandetag, so no client had any reason to re-fetch until a laterocc files:scanturned the divergence intoa 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
.ocTransferIdsuffix, 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.AssemblyStreamcompared each chunk against a size it re-read while streaming, butSabre\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-Lengthafter 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 thestorage layer crashes the process.
Checklist
3. to review, feature component)stable32)AI (if applicable)