fix(davsync): detect a WebDAV write the server kept short (#531)

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 <noreply@anthropic.com>

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 {

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)
+	}
+}