Repository navigation
fix: clean up application images when an upload fails - #1057
eternal-flame-AD merged 2 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1057 +/- ##
==========================================
+ Coverage 76.50% 76.57% +0.07%
==========================================
Files 68 68
Lines 3694 3714 +20
==========================================
+ Hits 2826 2844 +18
- Misses 656 657 +1
- Partials 212 213 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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
|
@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. |
jmattheis
left a comment
There was a problem hiding this comment.
LGTM, @eternal-flame-AD do you want to recheck?
eternal-flame-AD
left a comment
There was a problem hiding this comment.
I think this is fine although I would prefer migrating to atomic fs with fixed file names (or your database inline solution) directly
|
I agree, but it does improve things so I'm okay with merging. |
2942fe9
|
I merged the latest GitHub dismissed the earlier approvals when the branch was updated, so @jmattheis @eternal-flame-AD could you re-approve when you have a moment? CI is running on the merge commit. |
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.