Skip to content

Delete chunks if the move on an upload failed - #22128

Merged
MorrisJobke merged 3 commits into
masterfrom
bugfix/noid/cleanup-chunks-on-failure
Aug 13, 2020
Merged

Delete chunks if the move on an upload failed#22128
MorrisJobke merged 3 commits into
masterfrom
bugfix/noid/cleanup-chunks-on-failure

Conversation

@juliusknorr

@juliusknorrjuliusknorr commented Aug 6, 2020

Copy link
Copy Markdown
Member

This should fix#16517 where chunks don't get cleaned up if the move operation after a chunked upload fails for some reason, e.g. because the file is blocked by files_accesscontrol.

Steps to reproduce:

  • Setup a files_accesscontrol rule for test.iso
  • Upload a file called test.iso (needs to be big enough to trigger chunking)
  • Observe the data/admin/uploads directory

Comment threadapps/dav/lib/Upload/ChunkingPlugin.php Outdated

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works 👍

@MorrisJobke

Copy link
Copy Markdown
Member

Tests fail.

@MorrisJobke

Copy link
Copy Markdown
Member

Also the part file is still in the users home directory. Shouldn't it be deleted as well (this hasn't changed at all between master and this branch, but I just noticed it)

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch

@MorrisJobke
MorrisJobkeforce-pushed the bugfix/noid/cleanup-chunks-on-failure branch from 99f79d3 to 0e45c99CompareAugust 10, 2020 14:20
@MorrisJobke

Copy link
Copy Markdown
Member

Autosquashed and force pushed

@MorrisJobkeMorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 10, 2020
@MorrisJobke

Copy link
Copy Markdown
Member

There was 1 failure:

1) OCA\DAV\Tests\unit\Upload\ChunkingPluginTest::testBeforeMoveFutureFileSkipNonExisting
Sabre\HTTP\ResponseInterface::setStatus(204) was not expected to be called.
/drone/src/apps/dav/lib/Upload/ChunkingPlugin.php:109
/drone/src/apps/dav/lib/Upload/ChunkingPlugin.php:76
/drone/src/apps/dav/tests/unit/Upload/ChunkingPluginTest.php:138

@MorrisJobkeMorrisJobke mentioned this pull request Aug 11, 2020
57 tasks
@juliusknorr

Copy link
Copy Markdown
MemberAuthor

Also the part file is still in the users home directory. Shouldn't it be deleted as well (this hasn't changed at all between master and this branch, but I just noticed it)

Are you sure that those are not from previous uploads? Then they should be cleaned up after 24 hours iirc.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
juliusknorrforce-pushed the bugfix/noid/cleanup-chunks-on-failure branch from 0e45c99 to cda5cfcCompareAugust 12, 2020 06:18
@juliusknorr

Copy link
Copy Markdown
MemberAuthor

Hm i assume this is also related.

build/integration/features/webdav-related.feature:602

I'll have another look

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr

Copy link
Copy Markdown
MemberAuthor

Should be aligned to the previous behavior now as https://github.com/nextcloud/3rdparty/blob/b0afba6d6508a1c85332cf8c61e90ad91b289ebc/sabre/dav/lib/DAV/CorePlugin.php#L653 was called before

Let's see what CI thinks about that.

@faily-bot

faily-botBot commented Aug 13, 2020

Copy link
Copy Markdown

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 31762: failure

sqlite

Show full log
There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
--
There was 1 failure:
1) OCA\Files_Versions\Tests\VersioningTest::testRestoreMovedShare
File content has not changed
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'version 2'
+'version 1'
/drone/src/apps/files_versions/tests/VersioningTest.php:729

acceptance-app-files

  • tests/acceptance/features/app-files.feature:262
Show full log
 Scenario: unmarking a file as favorite causes the file list to be sorted again # /drone/src/tests/acceptance/features/app-files.feature:262
Given I am logged in # LoginPageContext::iAmLoggedIn()
And I create a new folder named "A name alphabetically lower than welcome.txt" # FileListContext::iCreateANewFolderNamed()
And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()
And I close the details view # FilesAppContext::iCloseTheDetailsView()
And I see that the details view is closed # FilesAppContext::iSeeThatTheDetailsViewIsClosed()
And I mark "welcome.txt" as favorite # FileListContext::iMarkAsFavorite()
And I see that "welcome.txt" is marked as favorite # FileListContext::iSeeThatIsMarkedAsFavorite()
And I see that "welcome.txt" precedes "A name alphabetically lower than welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()
When I unmark "welcome.txt" as favorite # FileListContext::iUnmarkAsFavorite()
Then I see that "welcome.txt" is not marked as favorite # FileListContext::iSeeThatIsNotMarkedAsFavorite()
Not favorited state icon for file welcome.txt in file list could not be found after 100 seconds (NoSuchElementException)
And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()

@MorrisJobke
MorrisJobke merged commit e9f5c7f into masterAug 13, 2020
@MorrisJobke
MorrisJobke deleted the bugfix/noid/cleanup-chunks-on-failure branch August 13, 2020 18:47
@MorrisJobke

Copy link
Copy Markdown
Member

@juliushaertl I guess we backport this down to stable17?

@MorrisJobke

Copy link
Copy Markdown
Member

/backport to stable19

@MorrisJobke

Copy link
Copy Markdown
Member

/backport to stable18

@MorrisJobke

Copy link
Copy Markdown
Member

/backport to stable17

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishbug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flie access control : files upload and then denied (TransferId*.part present).

5 participants

@juliusknorr@MorrisJobke@rullzer@georgehrke@icewind1991