Discard partial range responses without etag

[deivid-rodriguez]

What was the end-user or developer problem that led to this PR?

Some custom gem servers (at least sidekiq's pro one) don't support "incremental API content", yet they respond to partial range responses, providing no Etag.

Previously bundler would handle this just fine, because it would discard the response given and retry a "full range request" in this case.

However, in #3865 we changed bundler to accept the response in this case, which would be appended to the existing response. However, since new versions are not appended to the remote versions file (no "incremental API content"), this would result in a bad response being written to the local client compact index cache. This should be the reason for the problems reported in #4514.

What is your fix for the problem, implemented in this PR?

My fix, for now, is to revert #3865. But as a further improvement, we can keep the "accept the response if no etag" idea, but only if we get a 200 response, not a partial content (206) response.

If we did the latter, sidekiq-pro can fix their server code to always serve full responses, even when requested partial ranges, and things will work in a more efficient way, since it wouldn't require double fallback requests.

EDIT: Extra improvement was also implemented 👍.

Closes #4514.
Closes #4543.

Make sure the following tasks are checked

• Describe the problem / feature
• Write tests for features and bug fixes
• Write code to solve the problem
• Make sure you follow the current code style and write meaningful commit messages without tags

[indirect]

This seems reasonable! My one caution is to make sure that Bundler actually accepts responses from servers that don't include an etag at all. At the time of my original PR, Bundler would refuse responses with no etag, and act like the compact index didn't exist. If that's no longer the case, sounds like this takes care of everything. 😄

[deivid-rodriguez]

This seems reasonable! My one caution is to make sure that Bundler actually accepts responses from servers that don't include an etag at all. At the time of my original PR, Bundler would refuse responses with no etag, and act like the compact index didn't exist. If that's no longer the case, sounds like this takes care of everything. smile

Yes. What it does now is falling back to "full update" mode by retrying the request without If-None-Match and Range headers, which seems reasonable.

[indirect]

Perfect 👍🏻

[deivid-rodriguez]

Alright, I keep going a bit in circles here, but at the same time knowing more about the issue.

After all you're 100% right when you said:

This seems reasonable! My one caution is to make sure that Bundler actually accepts responses from servers that don't include an etag at all. At the time of my original PR, Bundler would refuse responses with no etag, and act like the compact index didn't exist. If that's no longer the case, sounds like this takes care of everything. 😄

This is actually how bundler behaved before #3865. Both partial range requests and full requests required an Etag, so we would indeed end up raising a MisMatchedChecksumError in the missing Etag case. The question is whether this is a good behaviour or not. The result of the error is that it would be rescued here to decide that the compact index API is not an available fetching method:

rubygems/bundler/lib/bundler/fetcher/compact_index.rb

Lines 70 to 84 in 563933a

	def available?
	unless SharedHelpers.md5_available?
	Bundler.ui.debug("FIPS mode is enabled, bundler can't use the CompactIndex API")
	return nil
	end
	if fetch_uri.scheme == "file"
	Bundler.ui.debug("Using a local server, bundler won't use the CompactIndex API")
	return false
	end
	# Read info file checksums out of /versions, so we can know if gems are up to date
	compact_index_client.update_and_parse_checksums!
	rescue CompactIndexClient::Updater::MisMatchedChecksumError => e
	Bundler.ui.debug(e.message)
	nil
	end

So the way bundler behaved before your PR (and after this one if we merge it) is that it falls back to other fetching methods. In the case of sidekiq-pro, to the full index, which works just fine.

A theory of why we fixed this in the first place is that some gem servers would provide no Etag and implement no other fetching methods other than the compact index API. If this is the case, yeah, my PR will break them again 😬. So it sounds like I need to go all the way and keep accepting the response without an ETag in case we're requesting a full (not partial) update.

[deivid-rodriguez]

Alright, I implemented only discarding the no etag response when doing a partial update, but accepting it for full updates. This should be finally ready now assuming CI passes.

[indirect]

Yep, this looks like exactly what I was thinking. Thanks!