search

tus / tusd

Reference server implementation in Go of tus: the open protocol for resumable file uploads
https://tus.github.io/tusd
MIT License
2.92k stars 465 forks source link

Fix graceful terminate in filestore. #1120

Closed blmhemu closed 2 months ago

blmhemu commented 2 months ago

In the current logic, we first delete the bin file and then the info file. We should instead do the reverse and ignore any errors when deleting the bin file. This ensures that any disgraceful shutdown would properly recover.

Note: This is only fixed in the filestore for now - might need to fix for other stores as well.

Acconut commented 2 months ago

Thank you for the PR. I am fine with reversing the order but ignoring all errors appears dangerous to me. A better approach would be to ignore errors when the file was already deleted (using errors.Is(err, fs.ErrNotExist)), but properly returning any different error. Or did you experience a different error?

blmhemu commented 2 months ago

Thanks for the review - updated to account for other errors.

blmhemu commented 2 months ago

TY - Fixed the comments.

Acconut commented 2 months ago

I wonder if this change will actually allow the upload to be cleaned up if only one of the two files remain. Yes, the Terminate method would do that, but a DELETE request would fail because the filestore needs both files present in order to load the upload (and then terminate it). At least that's what I remember right now.

Did you test the termination of an upload where one file has been deleted?

blmhemu commented 2 months ago

I wonder if this change will actually allow the upload to be cleaned up if only one of the two files remain. Yes, the Terminate method would do that, but a DELETE request would fail because the filestore needs both files present in order to load the upload (and then terminate it). At least that's what I remember right now.

Did you test the termination of an upload where one file has been deleted?

Ahh, you are right, the handler would not allow it to pass and returns err not found at getUpload method. Do you have any ideas over here ? Also, seeing the NewUpload code, it seems to first create the bin file and then the info file. So a server crash here could also lead to un-freed resources. It seems like we may need to relax our checks in some handlers.

Acconut commented 2 months ago

Also, seeing the NewUpload code, it seems to first create the bin file and then the info file. So a server crash here could also lead to un-freed resources. It seems like we may need to relax our checks in some handlers.

I am not sure if adjusting the handler logic is the correct way here. If the upload state on disk got corrupted (e.g. one of the two files is missing), the client will not be able to resume the upload and start a new one. Trying to consider and recover from every possible type of corruption in tusd would be challenging if not impossible. The better way to deal with corrupted states is to start a new upload and let the previous, corrupt upload be cleaned up by a cron job (e.g. a job that removes all files which are untouched for the last 24hrs). This approach would be more thorough as it can also clean up uploads that tusd considers corrupt. Does that make sense?

Nevertheless, I would still merge this PR. Ignoring fs.ErrNotExist when deleting files is a good improvement.

blmhemu commented 2 months ago

Also, seeing the NewUpload code, it seems to first create the bin file and then the info file. So a server crash here could also lead to un-freed resources. It seems like we may need to relax our checks in some handlers.

I am not sure if adjusting the handler logic is the correct way here. If the upload state on disk got corrupted (e.g. one of the two files is missing), the client will not be able to resume the upload and start a new one. Trying to consider and recover from every possible type of corruption in tusd would be challenging if not impossible. The better way to deal with corrupted states is to start a new upload and let the previous, corrupt upload be cleaned up by a cron job (e.g. a job that removes all files which are untouched for the last 24hrs). This approach would be more thorough as it can also clean up uploads that tusd considers corrupt. Does that make sense?

Yep ! The handler can only do so much. The cronjob approach looks good (the job can look at info file and match it with the os.stat and remove them after a threshold).

Nevertheless, I would still merge this PR. Ignoring fs.ErrNotExist when deleting files is a good improvement.

Sure. NP.

Acconut commented 2 months ago

Thank you for working on this!