always directly download nupkg and cache the tfms #9230
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
brettfo
commented
Mar 6, 2024
| @@ -66,23 +66,31 @@ def project_tfms | |||
| end | |||
|
|
|||
| def fetch_package_tfms(dependency_version) | |||
| nupkg_buffer = NupkgFetcher.fetch_nupkg_buffer(dependency_urls, dependency.name, dependency_version) | |||
There was a problem hiding this comment.
The only change to this function is wrapping the body in begin/end so the result can be cached. No change otherwise.
brettfo
commented
Mar 6, 2024
| @@ -111,7 +111,13 @@ def self.fetch_stream(stream_url, auth_header, max_redirects = 5) | |||
| current_redirects = 0 | |||
|
|
|||
| loop do | |||
| response = fetch_url_with_auth(current_url, auth_header) | |||
There was a problem hiding this comment.
fetch_url_with_auth was caching the result, but apparently repeatedly passing the result to Zip::File.open_buffer() will result in a corrupted buffer, so the fix is to always download and to not cache anything.
ryanbrandenburg
approved these changes
Mar 6, 2024
edgarrs
approved these changes
Mar 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Internal logs were showing
Zip::Errorwhen trying to unpack the.nupkginsidecompatibility_checker.rb. The.nupkgcontents were being cached, but when those contents are passed toZip::File.open_buffer()the contents were changed which meant that subsequent attempts to open the zip stream might fail because it suddenly wasn't a valid zip archive. The size increase didn't always result in a failure to open the zip, only sometimes. I wasn't able to find any information aboutopen_buffer()changing the buffer contents/increasing the size.I was able to manually verify this against a test repo (100% repro before the fix, 0% after), but I couldn't figure out why or when to add a unit test and it didn't seem appropriate to check in a binary zip file to this repo. Instead I don't cache the
.nupkgcontents and will always fetch a fresh copy, but do cache the TFM results so in practice this should be the same behavior as before.Since this behavior came out of another bug modifying how we download
.nupkgcontents (see #9204), I decided it would be beneficial to explicitly state what's going on in the downloader with a comment and a direct call to theExcondownload API. This way if the behavior of the previous method ever changes, this functionality won't be broken.