[Content Addressable] Handle server-widened content address names correctly client-side

[OughtPuts]

TLDR

Support unique server-widened content-addressable gems in rubygems and Bundler.

Summary

A gem is content-addressable when an 8 character SHA derived from the file contents makes up part of its filename. In a scenario where an 8 character SHA value matches with an existing gem name on RubyGems.org, the SHA is widened by an additional character until the content-address becomes unique again.

At present, the resolver prefers an installed candidate to avoid downloading a gem again, which introduces a bug as a mismatching already installed gem could be chosen ahead of a widened SHA from a remote source. I.e.,

# we want
desired_remote_gem: abcdef123

# we have already installed
a_different_gem: abcdef12

So the resolver is satisfied when it should not be. It is only looking at the first 8 chars so it believes we already have the correct gem.

This PR introduces support for widened content addresses, preferring them over an installed default-length address with the same 8 char (default length) prefix.

Testing

• test/rubygems/test_gem_resolver.rb
• test/rubygems/test_gem_installer.rb
• spec/install/gemfile/content_addressable_spec.rb

Top Hat

1. Build a platform-specific, content-addressable gem and serve it from a local Compact Index repository using its default 8-character address.
2. Install it into an isolated gem directory and confirm the short-address gem directory, gemspec, and cached .gem exist.
3. Rename the same server-side .gem using a 12-character prefix of its SHA-256 checksum, then update the Compact Index metadata.
4. Run bundle update <gem-name>.
5. Confirm the lockfile and installed files use the widened address.

✅ Top hatted successfully locally.

[Edouard-chin]

I haven't follow closely the content addressable work and don't know exactly how gem candidates are now selected.

From the PR description:

# we want
desired_remote_gem: abcdef123
# we have already installed
a_different_gem: abcdef12

Just to confirm, when we say "a_different_gem", are we talking about a different gem name or a different version ?

E.g. is the issue about

nokogiri-sha12345.gem # Version 1.0
nokogiri-sha123456.gem # Version 2.0

OR

nokogiri-sha12345.gem
sqlite3-sha123456.gem

I suspect it's the former but want to make sure I understand.

---

Made a few comments for code readability. Also have a question wrt to removing short version gems.

[Edouard-chin]

I'm a bit concerned on the performance impact this has on gem installation, there are few things that will make this codepath quite hot (finding all *gemspec files and loading/parsing them). On a system with many gems installed, this can slow down a bundle install noticeably.

After a successful install, it also removes the short-name copy only when both gem files have identical SHA-256 checksums.

It's not super clear to me how can we end up with two copies (one widened, one not).

[OughtPuts]

You're right - seems like I went down a bit of a rabbit hole trying to be defensive on this one just in case, when actually the likelihood of us having two identical gems with different names is very slim. Particularly in relation to the performance cost. I've removed all the cleanup code.

[OughtPuts]

To clarify - it wouldn't happen in the current flow but it would only be guarding against a change in behaviour / unusual edge case or something like that.

[OughtPuts]

Ahhh ok, I've just been tracing this back to understand where this requirement originally came from and why I implemented it originally.

The genesis of this entire PR was hsbt's review comment - third paragraph. Within the paragraph about suffix widening, he mentions

nothing removes a locally built default-length copy by comparing checksums

I think by 'locally built' he actually means a duplicate copy that a maintainer may have BEFORE it gets pushed to the server and widened. I.e.

1. Ruby 3.3 artifact for this gem name and version already exists on RubyGems.org

SHA: a1b2c3d4AAAAAA

2. A maintainer builds the Ruby 3.4 artifact locally. gem build does not know about server collisions, so it creates:

mygem-1.0-a1b2c3d4.gem
SHA: a1b2c3d4BBBBB

which will eventually clash.

3. They install that local 3.4 build to test it.

4. They push it. RubyGems.org detects the collision and advertises that (widened) 3.4 artifact as

mygem-1.0-a1c2d3d4B.gem
SHA: a1b2c3d4BBBBB

5. Later if the maintainer installs / updates from RubyGems.org the local short and remote widened candidates are the same Ruby 3.4 file with identical checksums.

This means that there's a bit of extra work being done here for the one or two cases of people who have a pre-push gem version installed. For instance, after this change the widened gem would now be fetched (unnecessarily for them as they already have an installed copy) as bundler now sees the widened version and the default length version as different, and then it doesn't remove the duplicate copy.

So the options for moving forward are:

Option 1: No cleanup code, a few people who installed a pre-push version of the gem end up with non-breaking duplicates. If most maintainers are using github workflows to build and push via rake-compiler/cibuildgem, that would also reduce the numbers of people it's a problem for to being a very small amount.

Option 2: Reintroduce the cleanup code, but find a less expensive implementation of it in terms of performance. Off the top of my head, we could do something like construct the exact path with the default length SHA, return if it does not exist, and then load that one spec and compare the two checksums.

Option 3: Reintroduce the cleanup code in its previous expensive form, so that we can get the PR moving forward more quickly. (not really a good option).

My preference is to go with Option 1 and just leave this PR without the cleanup functionality. It would affect such a small subset of people and it wouldn't be a breaking change anyway, and I think it's more important to move this PR along because the cibuildgem / PackageTask / rake-compiler changes are more deserving of attention, and we don't have that much longer to be able to dedicate to the project. We could keep an eye on it though, and I could put a small issue on the backlog just in case any maintainers notice it later down the line?

What do you think @Edouard-chin ? Happy to look into a more performant option 2 if we want to cover all the basis for completeness.

[Edouard-chin]

I would extract this to a private method with a descriptive name for easier readability. I think doing the partition based on widened? is not necessary (which would allow to remove the backward compatible widened? monkey patch from Bundler's rubygem_ext)

compatible = addressable.select(&:matches_current_metadata?)
compatible.reject! { |spec| widened_available?(spec, compatible) }

private

def widened_available?(spec, compatible_specs)
  compatible_specs.any? do |compatible_spec|
    next(false) if compatible_spec == spec

    compatible_spec.platform == spec.platform &&
      compatible_spec.content_address.start_with?(spec.content_address)
  end
end

[Edouard-chin]

Similar ish to my previous comment, I'd extract this into a more readable method.

[Edouard-chin]

It could be useful to include a test that assert only the widened spec matches (when the platform match and content_address of the widen matches the shorter content_address)