Repository navigation
fix: clean up application images when an upload fails - #1057
SulimanAbdulrazzaq wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1057 +/- ##
==========================================
+ Coverage 76.09% 76.20% +0.10%
==========================================
Files 67 67
Lines 3619 3639 +20
==========================================
+ Hits 2754 2773 +19
Misses 654 654
- Partials 211 212 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
eternal-flame-AD
left a comment
eternal-flame-AD
left a comment
There was a problem hiding this comment.
the core for this issue isn't the magic string in the database having race condition (it does not, it guarantees to store the most recent value). The problem is the file system has race conditions
There was a problem hiding this comment.
This is implemented as an io.Copy() without a cleanup, so incomplete uploads and oversized uploads will still lead to a file being left over
There was a problem hiding this comment.
Fixed in efb90f3. The upload now goes through saveImage/writeImage, which removes the file if it can't be written completely. The old image is also only deleted after UpdateApplication succeeds; if the update fails, the new file is removed instead.
|
|
||
| // Read the application again, a concurrent request may have | ||
| // changed or deleted its image in the meantime. | ||
| app, err = a.DB.GetApplicationByID(id) |
There was a problem hiding this comment.
This is not atomic either (app could be updated in the mean time), it should be implemented as a DB transaction (see #1047) , a CAS primitive implemented in the database abstraction (BEGIN .. ;SELECT ... FOR UPDATE ... ; UPDATE ... ; COMMIT) , or a special method that only update this field.
I would leave this out as it doesn't resolve the core issue, software mutex around relational database actions is an anti-pattern that can lead to deadlocks, and can easily be incorporated after #1047 as transactions if necessary.
There was a problem hiding this comment.
Agreed. I removed the mutex and the re-read. The overlapping-upload part can be done with a transaction or a dedicated DB method later, as you suggested.
A failed write left a partially written image file behind, and a failed database update after saving the new image had already deleted the image the application still referenced. Write the uploaded image with a helper that removes the file if it can't be written completely, and only delete the old image once the application has been updated. If the update fails, remove the new image instead.
e2e2bc4 to
efb90f3
Compare
SulimanAbdulrazzaq
changed the title
|
@eternal-flame-AD thanks for the review. I reworked this along the lines you suggested: no mutex, and cleanup of the files on failed uploads and failed updates. Details are in the updated description. Could you take another look? |
|
@eternal-flame-AD @jmattheis when you have a moment, could one of you take a look at this one? The checks are green on the current head. |
2 participants
Refs #1047
As suggested in the review, this drops the mutex and fixes the file-system side of image uploads:
ctx.SaveUploadedFilecopies the upload with no cleanup, so a copy that fails partway leaves a partial file in the image directory. The upload now goes throughsaveImage/writeImage, which removes the file if it can't be written completely.UpdateApplication. If the update failed, the application still referenced a deleted image, and the new file was left behind unreferenced. The old image is now deleted only after the update succeeds. If the update fails, the new file is removed and the old image is kept.Two uploads to the same application overlapping in time still need an atomic update of the image field. As discussed, that's better done with a transaction or a dedicated DB method, so it's left out here.
Tests:
Test_UploadAppImage_UpdateFails_KeepsExistingImage: on master the image directory ends up holding only the new, unreferenced file (expected: "existing.png",actual: "<new name>.png"). With this change the application keepsexisting.pngand it's the only file left.Test_WriteImage_RemovesPartialFileOnError: a reader that fails after some data leaves no file behind.make test-coverage(go test --race ./...) passes, andgolangci-lintreports0 issues.