[Bug]: wincfapi 33.0.x sends truncated uploads on large online-only trees (works on 4.0.x)

[mfeddani]

Summary

Large VFS tree, files kept online-only. On 33.0.7, local changes never reach the server. On the 4.0.x line the same workflow works. This is a regression on the upload path (same VFS layer as #9858).

Scope note: the differentiating change (on-demand cfapi fetch, see the root-cause comment) is present in all 33.0.x tags and disabled in all 4.0.x tags, so the affected code path is in every 33.0.x. Only 33.0.7 was tested in the field.

Core failure: truncated PUTs

The client announces a Content-Length but sends fewer bytes, and the count differs on each retry of the same file. The server rejects the PUT:

PUT .../01-Implantation-V14...SLDASM   (size 48501396)
Sabre\DAV\Exception\BadRequest: "Expected filesize: 48501396 bytes but read ...: 10387456 bytes"
# retry, same file: "...: 843776 bytes"
mirall/33.0.7 (build 20260629) wincfapi

In the client discovery logs, each failing file is a regular local file (ItemTypeFile, not virtual), queued Up, with local size = the announced Content-Length (= the server's Expected filesize exactly), while far fewer bytes are read:

01-Implantation-...SLDASM  db/local/remote = 48167772/48501396/48167772  ItemTypeFile  SYNC/Up  (read 10387456)
11158 -- PLAN IMPLANTATION.SLDDRW  0/7695064/0  db=ItemTypeSkip (no DB record)  NEW/Up  (read 5849088)
INVERSE V3 ASSEMBLAGE ...SLDASM  1568662/1826201/1568662  ItemTypeFile  SYNC/Up  (read 720896)

So a placeholder whose content is not fully present locally is uploaded as a normal file at full logical size, and the read truncates it. Two also show metadata missing: /true/. Other files are correctly virtual (a .STEP is ItemTypeVirtualFile / Down). No size-threshold claim.

Consequence: sync never converges

Rejected/interrupted uploads get classified as network errors, which terminate and retry the sync, so nothing lands. This is downstream, not a trigger (account-state, folderman and syncengine are unchanged vs 4.0.x):

AccountState: "Connected" -> "Network error"  ->  folder Terminating!  ->  Aborting sync in propagator...

48h capture: 963 files queued Up, 1 sync Success, 3 aborts, RemoteHostClosedError x7, OperationCanceledError x9.

Expected

Reads for upload should return the full content (as on 4.0.x). A file should not be uploaded before it is fully materialised.

Environment

Nextcloud Desktop 33.0.7 (wincfapi), git 497d66, Windows 11 (10.0.26200). Works on the 4.0.x line (incl. 4.0.10). Server: Nextcloud 33.0.1.

Related: #9858

[mfeddani]

Root cause, comparing the 4.0.x line (works) and 33.0.7 (broken), with a log correlation.

Truncation mechanism (same code in both). UploadDevice announces the logical size as Content-Length (propagateupload.cpp ~L492-510; chunk size propagateuploadng.cpp ~L347-358) and reads with no short-read guard (readData ~L525-560: read()==0 is treated as normal EOF; atEnd() is _read >= _size). For a partially-materialised placeholder, read() returns only the present bytes, so the body is truncated by a variable amount. Identical in 4.0.x, so not the regression itself.

Logs for the failing files. All three are classified as regular local files (not virtual), queued Up at full logical size:

01-Implantation-...SLDASM  db/local/remote = 48167772/48501396/48167772  ItemTypeFile  SYNC/Up  metadata missing:/true/
11158 -- PLAN IMPLANTATION.SLDDRW  0/7695064/0  ItemTypeSkip (no DB record)  NEW/Up
INVERSE V3 ASSEMBLAGE ...SLDASM  1568662/1826201/1568662  ItemTypeFile  SYNC/Up  metadata missing:/true/

The announced local size = the server's Expected filesize exactly (48501396/7695064/1826201), while bytes read were far fewer (10387456/5849088/720896). So a not-fully-present placeholder is uploaded as a normal file at full size and truncated. Other files are correctly virtual (a .STEP is ItemTypeVirtualFile/Down).

What differs between the lines. On-demand cfapi folder population: disabled in 4.0.x (CF_POPULATION_POLICY_ALWAYS_FULL, cfApiFetchPlaceHolders under #if 0), enabled in 33.0.x (CF_POPULATION_POLICY_PARTIAL). Commit 0ea4062e "fix(vfs/cfapi): disable on-demand folder fetching" (reverting the on-demand parts of #8359) is present in every 4.0.x tag (from 4.0.1) and absent from every 33.0.x tag (33.0.0..33.0.7) - so the affected path is in all 33.0.x, not just 33.0.7 (only 33.0.7 was tested in the field). That path was still being fixed in 33.x shortly before 33.0.7 ("do not create DB record for invalid virtual items", "use proper relative local path for DB records"), matching the missing metadata above.

Together: on-demand population leaves some placeholders with incomplete metadata or no DB record; discovery treats them as regular local files at full size and queues them Up; UploadDevice announces that size but reads only the materialised bytes, so the PUT is truncated. Not on 4.0.x, where on-demand fetch is disabled.

cc @mgallien

[mfeddani]

One possible stopgap, up to you: the 4.0.x line carries 0ea4062e "disable on-demand folder fetching" as a temporary measure. The same revert on 33.x would likely restore uploads on large online-only trees until the placeholder/DB path is fixed. Just a suggestion. Happy to test a patch or share a full 33.0.7 debug archive.

[mfeddani]

A concrete, low-risk mitigation regardless of the root cause: UploadDevice::readData currently treats a premature EOF as a normal end of stream, so a short read of a not-fully-materialised placeholder is uploaded as a truncated body. Failing the read instead turns silent truncation into a visible, retryable error:

     auto c = _file.read(data, maxlen);
     if (c == 0) {
-        setErrorString({});
-        return c;
+        // Premature EOF: fewer bytes than the size announced as Content-Length
+        // (e.g. an online-only placeholder not fully materialised). Returning 0
+        // would upload a truncated body the server rejects. Fail loudly instead.
+        qCWarning(lcUploadDevice) << "Short read while uploading" << _file.fileName()
+                                  << "- got" << _read << "of expected" << _size;
+        setErrorString(tr("File could not be read completely for upload (%1 of %2 bytes)")
+                           .arg(_read).arg(_size));
+        return -1;
     } else if (c < 0) {

This does not fix the underlying placeholder read, but it prevents silent truncation/corruption and makes the failure diagnosable. Offered as a suggestion, not a PR.

We can test this and any real fix on the reproducing environment (Windows 11, wincfapi, ~500 GB online-only CAD tree), which is usually the hard part to set up. Happy to run patches and share full debug archives.

The guard above is only a band-aid. The real fix: don't read a placeholder for upload until it is fully materialised locally. On the Up direction an online-only placeholder with pending changes should trigger hydration (or defer) before the PUT, instead of streaming a partially-present file; and the on-demand population (#8359) should create placeholders with complete metadata and a DB record, so discovery stops treating them as full-size local files.

Note: the guard I sketch above already exists on the 4.0.x line as fix(upload): ensure we see a warning if we cannot read a file to upload (in 4.0.8, commit 843f4de). The 33.0.x line does not carry it. Porting that guard to 33.x would at least surface the truncation instead of silently sending a short body, on top of the real fix (hydrate before upload).