Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix double upload bug and improve test #663

Conversation

AndreasAlbertQC
Copy link
Collaborator

@AndreasAlbertQC AndreasAlbertQC commented Sep 8, 2023

In our deployment we have been encountering a bug that happens when:

  1. a package file is uploaded through the /upload/ route.
  2. a package file with the same package version inside and the same file name, but different hashes is uploaded again.

We observe that in the second upload, we get a 409, which is good. However, we also find that the binary package file is still updated on the server, even though the http request was rejected. The repodata is not updated, as expected. This means that we get hash mismatch errors because the incorrectly updated binary does not agree with the correct hashes for the original file in the repodata.

I think the problem is here, where we call pkgstore.add_package_async before the try/except block that ultimately results in the 409.

During debugging I found that the other upload route is also affected.

Changes in this PR:

  • Extended the existing duplicate upload test:
    • we now use files that have the same name. This is crucial to trigger the bug. In previous testing, files with different names were used.
    • we now also check the file that is actually present in the store. Previously, we just uploaded and checked the repodata, which leads us to miss the bug that manifests only in the stored files.
  • Bug fix:
    • in /channels/{channel_name}/packages/{package_name}/files/, the upload is handled through a call to handle_package_files, which in turn uses _extract_and_upload_package. I added a force argument and a file existence check to _extract_and_upload_package, which is then transformed into an HTTP 409 in handle_package_files
    • In /channels/{channel_name}/upload/{filename}, the solution is more straightforward as the call to pkgstore.add_package_async happens right in the top-level function. I moved the call after the try/except block that checks for 409.

Expected test failure before fix here

@AndreasAlbertQC AndreasAlbertQC added the bug Something isn't working label Sep 8, 2023
@AndreasAlbertQC AndreasAlbertQC changed the title Extend test to cover double upload bug Fix double upload bug and improve test Sep 8, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (f20db64) 83.63% compared to head (e4a831e) 83.64%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #663      +/-   ##
==========================================
+ Coverage   83.63%   83.64%   +0.01%     
==========================================
  Files          79       79              
  Lines        6226     6230       +4     
==========================================
+ Hits         5207     5211       +4     
  Misses       1019     1019              
Files Changed Coverage Δ
quetz/main.py 89.89% <100.00%> (+0.05%) ⬆️

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

@AndreasAlbertQC AndreasAlbertQC marked this pull request as ready for review September 8, 2023 10:03
@AndreasAlbertQC AndreasAlbertQC merged commit 0854d44 into mamba-org:main Sep 8, 2023
12 checks passed
@AndreasAlbertQC AndreasAlbertQC deleted the 2023-09-08-fix-double-upload-bug branch September 8, 2023 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants