compare cached filesize on download

[ArtificialOwl]

• need more test on specific storages (external, s3, ...)

• might need a refresh of parent[s] folder cached data

• TEST: with master-key encryption

[PVince81]

does the scanner not automatically update the parent folder's sizes ?
if not, maybe try with writeUpdate like in https://github.com/nextcloud/server/pull/33566/files#diff-64a02ee1149d33d0747360694c2abf4ef1bdbfc379b4bb40f76c22b5b1fc06d9R1011

[icewind1991]

The scanner does not update parent sizes on it's own

[ArtificialOwl]

@PVince81

drone issue looks unrelated. tested on encrypted file, looks good on my side. might need more test !?

[github-actions]

Possible performance regression detected

Show Output

1 queries added
= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
≠ /remote.php/dav/files/test/new_file.txt with 1 queries added
- UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3, "size" = CASE WHEN "size" > '-1' THEN MAX("size" + :dcValue4, :dcValue5) ELSE "size" END WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
+ UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3 WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
+ UPDATE "oc_filecache" SET "size" = MAX("size" + :dcValue1, :dcValue2) WHERE ("storage" = :dcValue3) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1')) AND ("size" > '-1')
= /remote.php/dav/files/test/new_file.txt

[icewind1991]

changed it to pass the stream size up through the seekable wrapper

[github-actions]

Possible performance regression detected

Show Output

1 queries added
= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
≠ /remote.php/dav/files/test/new_file.txt with 1 queries added
- UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3, "size" = CASE WHEN "size" > '-1' THEN MAX("size" + :dcValue4, :dcValue5) ELSE "size" END WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
+ UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3 WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
+ UPDATE "oc_filecache" SET "size" = MAX("size" + :dcValue1, :dcValue2) WHERE ("storage" = :dcValue3) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1')) AND ("size" > '-1')
= /remote.php/dav/files/test/new_file.txt

[github-actions]

Possible performance regression detected

Show Output

1 queries added
= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
≠ /remote.php/dav/files/test/new_file.txt with 1 queries added
- UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3, "size" = CASE WHEN "size" > '-1' THEN MAX("size" + :dcValue4, :dcValue5) ELSE "size" END WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
+ UPDATE "oc_filecache" SET "mtime" = MAX("mtime", :dcValue1), "etag" = :dcValue3 WHERE ("storage" = :dcValue2) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1'))
+ UPDATE "oc_filecache" SET "size" = MAX("size" + :dcValue1, :dcValue2) WHERE ("storage" = :dcValue3) AND ("path_hash" IN ('d41d8cd98f00b204e9800998ecf8427e', '45b963397aa40d4a0063e0d85e4fe7a1')) AND ("size" > '-1')
= /remote.php/dav/files/test/new_file.txt

[ArtificialOwl]

squashed and rebased; also adding a return false if handle is false to keep backward compat on calls to read()

working on my side (local files, s3 primary, encrypted)

[PVince81]

what's missing to get this merged ? @ArtificialOwl@icewind1991

also see my comments

[PVince81]

@icewind1991 can you do the 2nd review ?

[ArtificialOwl]

So ran some manual test....

All tests are made on a 25.0.1 patched with 34232.diff

• s3

  • internal download
  • internal folder.zip download => size is fixed, download of the full folder is ok, but top folder size is not updated
  • public share => size is updated, but download needs to be restarted
  • groupfolder (including public share, internal folder.zip download)
  • internal shared file
  • Desktop client

• local filesystem

  • internal download
  • internal folder.zip download => size is fixed, download of the full folder is ok, but top folder size is not updated
  • public share
  • groupfolder (including public share, public folder.zip download)
  • internal shared file
  • Desktop client

• not working with encryption

[ArtificialOwl]

testing on encrypted local filesystem for data/:

• fresh install of nc25.0.1
• enable encryption
• enable default server encryption app
• master key enabled by default
• encrypt-all
• creating new text file
• stored size in filecache is the one of the clear version, unencrypted_size is 0
• edit test file
• file size in filecache is updated with clear version, unencrypted_size still 0
• editing size value in filecache (with lower value)
• downloaded file is crop after n chars, which is the new edited value for size in filecache
• editing size value in filecache (with slightly higher value)
• downloaded file is fine and correct size on my disk
• editing size value in filecache (with slightly higher value)
• cannot download file, click will open a new empty tab in firefox
• unencrypted_size is still 0

[PVince81]

as discussed, @ArtificialOwl will retest on master to see if the encryption behavior is the same.

if this PR doesn't affect encryption the let's ship it as is

[ArtificialOwl]

as discussed, @ArtificialOwl will retest on master to see if the encryption behavior is the same.

if this PR doesn't affect encryption the let's ship it as is

On encrypted FS, same test failed the exact same way without the patch

[PVince81]

ok, so this PR doesn't affect encryption behavior, let's merge it

[PVince81]

linter not happy though, please have a look @ArtificialOwl