fix(webdav): bind resumed downloads to a remote entity validator - #3
Merged
Conversation
A retained .part file was resumed using only its byte length as the range offset. Nothing tied those bytes to the remote entity that produced them, so when the remote file changed between attempts the stale prefix and the new suffix were spliced into a file that had never existed on either side. The error was nil and --verify did not catch it: the size check compares the final length and the ETag check compares the last response against a fresh PROPFIND, which agree because both describe the new entity. The retained prefix is never re-read. Resume is now guarded by the validator of the entity being written, recorded in a .part.etag sidecar as the bytes are written. Ranged requests carry it as If-Range, so a changed entity yields 200 and restarts from zero, and a .part file with no recorded validator is discarded rather than trusted. Resume: true is the default for `ocis download` and is unconditional in the sync path, where a corrupt result would also be written into the baseline as converged.
The validator sidecar was published before the stale prefix was truncated, so an interruption between the two steps left bytes from the old entity labelled with the new one. The next attempt then resumed against that label and appended a suffix of the new entity to a prefix of the old, reintroducing the silent corruption the sidecar exists to prevent. A restart now invalidates the sidecar, truncates, and only then records the validator, so a label can only ever describe bytes from the response that wrote it. An interruption in between leaves an unlabelled prefix, which the next attempt discards. A 206 keeps the saved validator and rejects a response ETag naming a different entity, which would otherwise splice two entities together. Sidecar cleanup also moves ahead of the destination replacement. Removing it afterwards reported a tidy-up failure as a transfer failure once the destination had already changed, which makes a sync caller skip its baseline update for a file that is already in place.
mzner
force-pushed
the
fix/resume-validates-remote-identity
branch
from
August 9, 2026 14:44
b4107b8 to
15671a0
Compare
mzner
enabled auto-merge (squash)
August 9, 2026 14:45
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.
Problem
DownloadWithOptionsresumed a retained.partfile using only its byte length as the range offset. Nothing tied those bytes to the remote entity that produced them, so if the remote file changed between attempts the stale prefix and the new suffix were spliced into a file that had never existed on either side.The failure is silent. The returned error is
nil, and--verifydoes not catch it:PROPFIND, which agree because both describe the new entity.The retained prefix is never re-read, so nothing ever inspects the corrupt bytes.
Reproduced against an
httptestserver: a stale 5-byte.partof"AAAAA"with the remote now"BBBBBBBBBB"produced"AAAAABBBBB"witherr == nil, including withVerify: true.Exposure is not narrow.
Resume: trueis the default forocis download(client.go:327) and is unconditional in the sync path (sync_service.go:548), where a corrupt result is additionally written into the sync baseline as converged content.Fix
Resume is now bound to the validator of the entity being written:
.part.etagsidecar as the bytes are written;If-Range, so a changed entity returns200and the download restarts from offset zero;.partfile with no recorded validator is discarded rather than trusted, which fails closed for partials left by older versions or any foreign process;.partfile and its sidecar are cleaned up on success.Tests
Two new tests in
internal/webdav/client_test.go:TestDownloadResumeRevalidatesRemoteIdentity— the original corruption scenario now yields the correct current content.TestDownloadResumeSendsIfRangeValidator— asserts theIf-Rangeheader is actually sent on a resumed range.Existing resume tests encoded the old unsafe contract (writing a
.partwith no validator and expecting a resume); they now record a validator via a newwritePartialhelper.writeDAVFilegained awriteDAVFileETagvariant so a test can control the served ETag.Full suite passes,
go vetclean,golangci-lintreports 0 issues,gofmtclean.Docs
README.mdandARCHITECTURE.mdupdated to state the new resume guarantee.Follow-up from review
A second commit (
b4107b8) closes two ordering gaps found in the review of this branch.A restart could label stale bytes with the new entity. The validator was recorded before the
.partfile was truncated. An interruption in between — a crash, or theos.OpenFilefailing — left the old prefix on disk under the new ETag, which is precisely the pairing a later resume is required never to trust: the next attempt would sendIf-Rangefor an entity the retained bytes do not belong to, receive a206, and splice again. The sequence is now invalidate → truncate → record, so a recorded validator can only ever describe bytes this response produced. An interruption anywhere in between leaves an unlabelled prefix, which the next attempt discards.A
206was accepted without checking the entity it described.Content-Rangewas not parsed and a responseETagnaming a different entity was ignored, so a server that honoured the range but served a different entity reopened the original splice. The offset inContent-Rangemust now match the requested offset, and anETagthat disagrees with the validator the continuation was requested for is an error. The saved validator is deliberately kept on the resume path — it still describes the retained prefix.Sidecar cleanup no longer masks a successful transfer. Cleanup is ordered before the destination is committed, leaving the commit as the only fallible step after it; a cleanup failure is logged at debug rather than returned. Reporting tidy-up as transfer failure would make a sync caller skip its baseline update for a file already in place.
Three tests added: a resume with a conflicting validator is rejected, a restart never advances the saved validator when the reopen fails, and an error never accompanies an already-replaced destination.