search

mnutt / davros

Personal file storage server
Apache License 2.0
297 stars 35 forks source link

Large files corrupted after sync by desktop clients #114

Open rpezzi opened 3 years ago

rpezzi commented 3 years ago

Hi.

On my setup, large files uploaded by desktop clients gets corrupted on the Davros app (observed this for files of 500MB and 1GB).

Soon after a large file is uploaded to Davros by a desktop client, a corrupted version of the file is downloaded back to the PC by the client, overwriting the original file. I reproduced this behavior with Davros on both, a private sandstorm instance and on a demo instance at alpha.sandstorm.io. Same problem for NextCloud desktop client 2.5.1 (official Debian repository) & the latest Owncloud client (2.6.3). Clients running on Debian 9. This was present on Davros 0.26.1 and persists on 0.27.3.

No problem if the file is uploaded though the Sandstorm web interface.

By the way, thanks for giving more life to Davros.

ocdtrekkie commented 3 years ago

@zenhack Is this a sandstorm-io/sandstorm#2464 related issue or is it likely Davros-specific?

zenhack commented 3 years ago

I poked around a bit, and I think that in theory nextcloud desktop should handle not supporting range requests OK:

https://github.com/nextcloud/desktop/blob/56a6fe4731a8d71c314c44822a960a66acfb9f67/src/libsync/propagatedownload.cpp#L205

That said it's entirely possible that that logic isn't super battle tested, or that I've misunderstood something.

mnutt commented 2 years ago

FWIW I don't know that owncloud/nextcloud clients typically use Range requests; from what I remember they upload hundreds of smaller files and then send a command to finalize and concatenate them together. I tested davros itself (sans-sandstorm) by manually hashing a particular 2GB file before and after upload and wasn't able to reproduce, unfortunately. I'll take another look at that concatenation code and see if I can reproduce it in sandstorm.

mnutt commented 2 years ago

I was able to reproduce on sandstorm, but I don't think it's a sandstorm problem. I think I see the likely cause of this in reading the above nextcloud docs, which unfortunately was undocumented at the time I wrote the uploading chunking.

Originally, ownCloud client would pick a chunk size prior to starting the upload, and upload chunks in order. While ownCloud's own implementation saved the chunks to a temporary directory and then concatenated them together, mine aimed to be a bit more efficient: I had just one tempfile, and fseek()'d to the calculated offset to write each chunk. This meant we didn't need double the file's size temporarily, was very cheap to finalize, and didn't involve a bunch of little files.

My guess is that the client has started adaptively changing the chunk size, which will cause the calculation (chunk size + chunk number) to be wrong. The fact that the corrupted file has the right filesize supports this.

mnutt commented 2 years ago

Once sandstorm allows a few extra request headers through I’ll be able to checksum the final result and ensure that at least no data is ever corrupted. But it wouldn’t actually solve the underlying issue which I think may be pieces coming in out of order. The easiest way to solve that is to opt into the newer NextCloud/OwnCloud chunking mechanism that passes an oc-file-offset header which means we can always write the resulting file in the right order. That will be coming shortly after the sandstorm header changes.

ocdtrekkie commented 2 years ago

@rpezzi I believe this should be resolved by the experimental Davros version 0.31.1 in the experimental market: https://apps.sandstorm.io/app/8aspz4sfjnp8u89000mh2v1xrdyx97ytn8hq71mdzv4p4d8n0n3h?experimental=true

If you get a chance to test this version in a way that doesn't put your production data at risk, that would be really helpful!