Two independent WebDAV bugs, both diagnosed by @austinc3030 inside #86. Recording them here because they are unrelated to that PR's desktop-sync subject matter, are isolated to internal/storage/webdav/webdav.go, and should not wait on the desktop-sync direction decision in #91.
Fixes already exist as commits in #86 — f0fe4035 and a4daf886. The ask is to split them into their own PR; they would land on their own merits.
1. Connection pool too small for the worker pool, tripping a crypto/x509 race
internal/storage/webdav/webdav.go builds its client as &http.Client{Timeout: 60 * time.Second}, so it inherits http.DefaultTransport, whose MaxIdleConnsPerHost is 2. The syncer runs defaultWorkers = 10 concurrent requests against the same host (internal/sync/sync.go), so eight of every ten requests cannot reuse a pooled connection and perform a fresh TLS handshake.
Beyond the latency, a burst of simultaneous handshakes against the same certificate chain reproducibly trips a data race in crypto/x509's concurrent policy validation — reported as fatal error: concurrent map writes, and also observed as heap corruption and segfaults in the HTTP/2 read loop, which is the same race surfacing elsewhere.
This is a crash, not a slowdown, and it affects every WebDAV/Nextcloud user on every push or pull large enough to saturate the pool.
The fix is two lines — clone the default transport and raise MaxIdleConnsPerHost to cover the worker pool:
transport := http.DefaultTransport.(*http.Transport).Clone()
transport.MaxIdleConnsPerHost = 32
Worth adding a comment tying the number to defaultWorkers, since the coupling is otherwise invisible — raising defaultWorkers past the pool size silently reintroduces this.
2. ensureParentDirs accepts every MKCOL outcome, hiding a real race
ensureParentDirs's status check examines 201, 405, 409 and 404-with-continue and returns no error in any case — and implicitly accepts anything else too, since no other status is ever examined. A genuinely failed MKCOL therefore falls through to a doomed PUT rather than surfacing, so a file silently fails to upload and the user is told the sync succeeded.
That masks a concrete race: with up to 10 concurrent uploads, the first two files landing in a brand-new directory — two tool-result files in one session, or two subagent records — issue concurrent MKCOLs for the same not-yet-existing path. One of them can get an ambiguous response, which the current code swallows.
Silent non-upload is the worst failure mode for a backup tool: the data is simply not there when the user goes to restore it.
Notes
Two independent WebDAV bugs, both diagnosed by @austinc3030 inside #86. Recording them here because they are unrelated to that PR's desktop-sync subject matter, are isolated to
internal/storage/webdav/webdav.go, and should not wait on the desktop-sync direction decision in #91.Fixes already exist as commits in #86 —
f0fe4035anda4daf886. The ask is to split them into their own PR; they would land on their own merits.1. Connection pool too small for the worker pool, tripping a crypto/x509 race
internal/storage/webdav/webdav.gobuilds its client as&http.Client{Timeout: 60 * time.Second}, so it inheritshttp.DefaultTransport, whoseMaxIdleConnsPerHostis 2. The syncer runsdefaultWorkers = 10concurrent requests against the same host (internal/sync/sync.go), so eight of every ten requests cannot reuse a pooled connection and perform a fresh TLS handshake.Beyond the latency, a burst of simultaneous handshakes against the same certificate chain reproducibly trips a data race in
crypto/x509's concurrent policy validation — reported asfatal error: concurrent map writes, and also observed as heap corruption and segfaults in the HTTP/2 read loop, which is the same race surfacing elsewhere.This is a crash, not a slowdown, and it affects every WebDAV/Nextcloud user on every push or pull large enough to saturate the pool.
The fix is two lines — clone the default transport and raise
MaxIdleConnsPerHostto cover the worker pool:Worth adding a comment tying the number to
defaultWorkers, since the coupling is otherwise invisible — raisingdefaultWorkerspast the pool size silently reintroduces this.2.
ensureParentDirsaccepts every MKCOL outcome, hiding a real raceensureParentDirs's status check examines 201, 405, 409 and 404-with-continue and returns no error in any case — and implicitly accepts anything else too, since no other status is ever examined. A genuinely failedMKCOLtherefore falls through to a doomedPUTrather than surfacing, so a file silently fails to upload and the user is told the sync succeeded.That masks a concrete race: with up to 10 concurrent uploads, the first two files landing in a brand-new directory — two tool-result files in one session, or two subagent records — issue concurrent
MKCOLs for the same not-yet-existing path. One of them can get an ambiguous response, which the current code swallows.Silent non-upload is the worst failure mode for a backup tool: the data is simply not there when the user goes to restore it.
Notes
MKCOLsurfaces as an error rather than returning nil — CI enforces 60% coverage oninternal/*.