From 93a1e3e7fc85fc036eb778697bed7cb3dd90e494 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=B0=94=E7=AB=8B?= Date: Fri, 2 Oct 2026 16:42:37 +0800 Subject: [PATCH] fix(davsync): detect a WebDAV write the server kept short (#531) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Relays and tunnels cut long uploads short. Over a Cloudflare Tunnel with a 3MB write this came up 2 of 3 times, and each time the client saw it happen: the relay answered 502 or 423, which the sync reports as itself. The quiet shape is a server that answers 2xx with half the file on disk: every later sync then reads the half and fails as "not a magpie backup", with no way to tell what actually went wrong. A write is now looked at after it lands: a HEAD read of the backup is compared with the size sent, a short write is retried over the version the short one made (its ETag, from the PUT's answer or the HEAD's), and after three tries reported as itself. A HEAD that can't be done — an error, a 429 or 503, a non-200 — leaves the write unchecked: it landed either way. Servers without HEAD check as before. Co-authored-by: Claude Code --- internal/davsync/dav.go | 51 ++++++++++++- internal/davsync/dav_test.go | 137 +++++++++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+), 2 deletions(-) diff --git a/internal/davsync/dav.go b/internal/davsync/dav.go index f9f623d4..2f9389a5 100644 --- a/internal/davsync/dav.go +++ b/internal/davsync/dav.go @@ -274,13 +274,16 @@ func (d *dav) get(ctx context.Context, have version) (data []byte, v version, er } // put writes the backup, only over the version read (etag) when there was -// one, making the folder if the server has none. +// one, making the folder if the server has none. Relays and tunnels cut +// long writes short, and the half that lands reads back as "not a magpie +// backup" on every sync after, so what a write kept is read back and +// compared; a short write is tried again and told as itself in the end. func (d *dav) put(ctx context.Context, data []byte, etag string) (version, error) { h := map[string]string{"Content-Type": "application/octet-stream"} if etag != "" { h["If-Match"] = etag } - for try := 0; ; try++ { + for try, writes := 0, 0; ; try++ { res, err := d.send(ctx, http.MethodPut, d.url(folder, file), data, h) if err != nil { return version{}, err @@ -288,6 +291,25 @@ func (d *dav) put(ctx context.Context, data []byte, etag string) (version, error res.Body.Close() switch { case res.StatusCode >= 200 && res.StatusCode < 300: + writes++ + he, cerr := d.check(ctx, len(data)) + if cerr != nil { + if writes < 3 { + // the half file is a new version: the retry matches + // it — the ETag the PUT answered, else the one the + // check read; with neither, the old one goes, for it + // can't hold any more + if e := res.Header.Get("ETag"); e != "" { + h["If-Match"] = e + } else if he != "" { + h["If-Match"] = he + } else { + delete(h, "If-Match") + } + continue + } + return version{}, cerr + } // an ETag said here is the version written; with none, the // next sync reads the file in full, as before return version{ETag: res.Header.Get("ETag")}, nil @@ -308,6 +330,31 @@ func (d *dav) put(ctx context.Context, data []byte, etag string) (version, error } } +// check says so when the server kept less than what was sent: relays and +// tunnels cut long writes short, and the half that lands reads back as +// "not a magpie backup" on every sync after. The size is enough — a short +// file is the whole failure — so no download rides on a write. It gives +// back the ETag it saw, for a retry to match the file as the short write +// left it. A HEAD that can't be done — an error, or not a 200 — leaves the +// write unchecked: it landed either way. +func (d *dav) check(ctx context.Context, want int) (etag string, err error) { + res, err := d.send(ctx, http.MethodHead, d.url(folder, file), nil, nil) + if err != nil { + return "", nil + } + res.Body.Close() + // a server without a say on HEAD can't be checked; the read after it + // tells + if res.StatusCode != http.StatusOK { + return "", nil + } + etag = res.Header.Get("ETag") + if n := res.ContentLength; n >= 0 && int(n) < want { + return etag, fmt.Errorf("the WebDAV server kept %d of %d bytes of %s: the write was cut short — sync again, and if it keeps happening the network to the server is dropping long uploads", n, want, file) + } + return etag, nil +} + func (d *dav) mkcol(ctx context.Context) error { res, err := d.do(ctx, "MKCOL", d.url(folder)+"/", nil, nil) if err != nil { diff --git a/internal/davsync/dav_test.go b/internal/davsync/dav_test.go index 5be63f14..9f40759b 100644 --- a/internal/davsync/dav_test.go +++ b/internal/davsync/dav_test.go @@ -5,6 +5,7 @@ import ( "io" "net/http" "net/http/httptest" + "strconv" "strings" "testing" ) @@ -131,3 +132,139 @@ func TestDAVForbiddenUntilMade(t *testing.T) { t.Fatalf("after: %q %v", data, err) } } + +// A relay or tunnel can cut a long write short: the server keeps half the +// file and answers 2xx anyway, and every later read trips over the half as +// "not a magpie backup". A write that kept less than was sent is read back, +// tried again over the version the short one made, and told as itself in +// the end. A HEAD that can't be done leaves the write unchecked: it landed. +func TestDAVShortWrite(t *testing.T) { + data := []byte(strings.Repeat("x", 8192)) + // cut is a small WebDAV with one file and, when etags, an ETag it + // changes on every write and If-Match it holds writes to — so a retry + // on the version read before the short one would be refused. The cut + // write keeps half: the writes-th one, or every one at -1. headfail + // answers the HEAD a check needs with 503. + cut := func(cut int, etags, headfail bool) (*dav, *int) { + var kept []byte + var etag, writes int + etagOf := func() string { + if !etags { + return "" + } + return strconv.Quote("v" + strconv.Itoa(etag)) + } + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if u, p, ok := r.BasicAuth(); !ok || u != "me" || p != "pw" { + w.WriteHeader(http.StatusUnauthorized) + return + } + switch r.Method { + case http.MethodPut: + if im := r.Header.Get("If-Match"); im != "" && im != etagOf() { + w.WriteHeader(http.StatusPreconditionFailed) + return + } + body, _ := io.ReadAll(r.Body) + writes++ + if cut == -1 || cut == writes { + kept = body[:len(body)/2] // the tunnel drops the rest + } else { + kept = body + } + etag++ + if e := etagOf(); e != "" { + w.Header().Set("ETag", e) + } + w.WriteHeader(http.StatusCreated) + case http.MethodGet: + if e := etagOf(); e != "" { + w.Header().Set("ETag", e) + } + w.Write(kept) + case http.MethodHead: + if headfail { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + w.Header().Set("Content-Length", strconv.Itoa(len(kept))) + if e := etagOf(); e != "" { + w.Header().Set("ETag", e) + } + default: + w.WriteHeader(http.StatusForbidden) + } + })) + t.Cleanup(srv.Close) + d, err := newDAV(Config{URL: srv.URL, User: "me", Password: "pw"}) + if err != nil { + t.Fatal(err) + } + return d, &writes + } + ctx := context.Background() + kept := func(d *dav) []byte { + got, _, err := d.get(ctx, version{}) + if err != nil { + t.Fatal(err) + } + return got + } + + // the write after a cut one is kept whole, so the sync goes through, + // and the server ends with the file in full + d, writes := cut(1, true, false) + if _, err := d.put(ctx, data, ""); err != nil { + t.Fatalf("the write after a short one: %v", err) + } + if *writes != 2 { + t.Fatalf("writes: %d, want 2: the short one was to be tried again", *writes) + } + if got := kept(d); len(got) != len(data) { + t.Fatalf("the server kept %d bytes, want the whole %d", len(got), len(data)) + } + + // the same over a version read before: the short write changed it, so + // the retry matches what the short write left, not that one + d, writes = cut(2, true, false) + if _, err := d.put(ctx, data, ""); err != nil { + t.Fatalf("the seed write: %v", err) + } + _, v, err := d.get(ctx, version{}) + if err != nil { + t.Fatal(err) + } + if v.ETag == "" { + t.Fatal("no ETag to write over") + } + if _, err := d.put(ctx, data, v.ETag); err != nil { + t.Fatalf("over a version read: %v", err) + } + if *writes != 3 { + t.Fatalf("writes: %d, want 3: the short one was to be tried again", *writes) + } + if got := kept(d); len(got) != len(data) { + t.Fatalf("the server kept %d bytes, want the whole %d", len(got), len(data)) + } + + // a server that keeps cutting: three tries, then the error says what + // was kept + d, writes = cut(-1, true, false) + _, err = d.put(ctx, data, "") + if err == nil || !strings.Contains(err.Error(), "cut short") || !strings.Contains(err.Error(), "4096 of 8192") { + t.Fatalf("always short: %v, want a kept-of-sent count", err) + } + if *writes != 3 { + t.Fatalf("writes: %d, want 3 before giving up", *writes) + } + + // a HEAD that can't be done (here 503) leaves the write unchecked: + // it landed, and it isn't failed over the check + d, writes = cut(0, true, true) + if _, err := d.put(ctx, data, ""); err != nil { + t.Fatalf("a write the HEAD couldn't be done for: %v", err) + } + if *writes != 1 { + t.Fatalf("writes: %d, want 1: the write landed", *writes) + } +}