We previously did not registered a finalizer for the values so this introduces new behaviour with potentially extra overhead and bugs. Would you agree to test it without this line? If that's not an option, we're fine with merging this fix as-is

This is necessary, because the same object_id can be reused after the GC run. Up until ruby 2.6, the object_id was the memory address. So if our object would be garbage collected, then another object could take its object_id, and we'd think it's the same object.

Perhaps it'd be good to repeat a benchmark similar to what was done when introducing the CachingResolver in #3630.

Ahhhh excellent point -- I was actually the one that suggested to @vpellan that running without the finalizer might be a better and less error prone idea but you're very right that we might get bad results on legacy Rubies...

But that does make me think -- as we've only observed the crash on Ruby 3.3+ (maybe 3.2 as well? I've a few reports there that might be this one as well), we could scope the workaround further, and take advantage of not needing to cover for object_id on legacy Rubies.

E.g. could we bother you to try this version?

          if RUBY_VERSION < '3.2'
            def resolve(value)
              @cache.fetch(value) do
                if @cache.size >= @cache_limit
                  @cache.shift # Remove the oldest entry if cache is full
                end

                @cache[value] = super
              end
            end
          else
            # Workaround for VM crash reported in https://github.com/DataDog/dd-trace-rb/issues/5718
            # We don't quite understand yet _why_ the crash happens, but using the `object_id` instead of the object
            # itself seems to work around whatever's going wrong inside the VM.
            def resolve(value)
              cache_key = value.object_id
              @cache.fetch(cache_key) do
                if @cache.size >= @cache_limit
                  @cache.shift # Remove the oldest entry if cache is full
                end

                @cache[cache_key] = super
              end
            end
          end

(I also noticed the spec for this file needs a # frozen_string_literal: true otherwise the string objects are different and thus there's one test that fails)

That's an interesting idea. Is it a safe assumption that object_ids won't be reused in ruby versions >= 3.2?
Would you like to take over the PR (or create a new one with the proposed change)?
I might be able to test this change in our app the next week if the time allows.

That's an interesting idea. Is it a safe assumption that object_ids won't be reused in ruby versions >= 3.2?

It is; when they changed it in 2.7 for compaction, it became an ever-increasing counter (this is how it looked in 2.7, this is how it looks nowadays in 4). In particular the counter never gets reset (that I know of).

Would you like to take over the PR (or create a new one with the proposed change)?
I might be able to test this change in our app the next week if the time allows.

Yeap, we'll take care of it!

Would be great if you could test it out once I do that tweak. In theory the ObjectSpace.define_finalizer is not needed for correctness, but since it's not clear at this point what the issue is, I would not rule it out that adding the finalizer to the object might contribute to avoiding whatever Ruby issue we're running into, so I really want to avoid declaring victory until we're sure :)

I made a comment re object_id at https://github.com/DataDog/dd-trace-rb/pull/5793/changes#r3280477084 (before seeing this).
Another way could be to store [value, super] as the Hash value, then there is no risk the key GC's, same as the cache before which held strongly onto the keys.

Doing all this without a Mutex does feel risky though, and might be the reason we're hitting CRuby and/or YJIT bugs in this area.

Avoid using reusable object_id as cache key

Using value.object_id as the cache key can return incorrect cached resolutions after GC: MRI only guarantees object_id uniqueness for live objects, so once an earlier key object is collected, a different object can later reuse that same id while the old cache entry is still present. In long-running threaded apps that allocate many connection config hashes, this can cause resolve to return a stale configuration for an unrelated input and skip recomputation; this regression is introduced by replacing the object key with its numeric id.

Useful? React with 👍 / 👎.