Compare ActiveRecord connection cache entries by identity by marcotc · Pull Request #5890 · DataDog/dd-trace-rb

marcotc · 2026-06-11T07:23:03Z

Originally #5719, by @dmilisic (thank you!).

What does this PR do?
Fix #5718 Segmentation fault in CachingResolver

Motivation:
Ocasional segfaults after upgrading from ddtrace (v1) to datadog (v2)

Change log entry

Yes. Compare ActiveRecord connection cache entries by identity

Additional Notes:
When using rails with multiple threads, each thread will have its own connection configuration Hash with a different object_id. So we can expect more cache entries, but we eliminate the overhead of comparing hashes.

An equivalent workaround (#5719) ran successfully in production for ~2 weeks without a segfault. Previously, segfaults occurred on a daily basis.

How to test the change?

Segmentation faults are difficult to reproduce. There should be no change in behaviour.

dd-octo-sts · 2026-06-11T07:23:16Z

👋 Hey @marcotc, please fill "Change log entry" section in the pull request description.

If changes need to be present in CHANGELOG.md you can state it this way

**Change log entry**

Yes. A brief summary to be placed into the CHANGELOG.md

(possible answers Yes/Yep/Yeah)

Or you can opt out like that

**Change log entry**

None.

(possible answers No/Nope/None)

^{Visited at: 2026-06-11 07:23:15 UTC}

datadog-prod-us1-3 · 2026-06-11T07:37:20Z

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 89.88% (-0.02%)

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: f561415 | Docs | Datadog PR Page | Give us feedback!}

pr-commenter · 2026-06-11T07:51:32Z

Benchmarks

Benchmark execution time: 2026-06-11 07:51:30

Comparing candidate commit f561415 in PR branch marcotc/patch-2 with baseline commit c293349 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

🟩 = significantly better candidate vs. baseline
🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

ivoanjo

👍 LGTM

One thing I'd suggest clarifying in the PR description is that it claims

We're running this fix in production for ~2 weeks without a single segfault. Previously, segfaults ocurred on a daily basis.

and this description came from #5719 which is a different change from this one.

I do agree that they are in theory equivalent. The problem is the Ruby VM in theory shouldn't segfault when doing this, yet it does. So this workaround is about what happens in practice so at least to me I think we should be clear that we expect the workaround to work, but have not actually been able to reproduce the original bug and thus to validate it.

Maybe this is a small detail, but I think it's important to clarify it if we need to revisit the issue in the future.

ivoanjo · 2026-06-11T08:12:30Z

+            # as large change around the crash site was done in that version (https://github.com/ruby/ruby/pull/14039/changes#diff-884a5a8a369ef1b4c7597e00aa65974cec8c5f54f25f03ad5d24848f64892869R1743),
+            # where `RClass.cc_table` (the NULL dereferenced pointer) became a GC-managed object,


Minor: This comment looks unfinished? (And I'm not sure it's actually related to our crash?)

It's related to the crash site.
I only typed , instead of ..
I'll fix it.
I'll reread it today to see if it's not clear.

ivoanjo · 2026-06-11T08:32:34Z

As we have an upcoming release, and we've tested and also approved the previous PR, I'm going to go ahead and merge this PR to make sure it's ready and validated for the release.

I'd still advise:

Updating the PR description with my suggestion above
Doing a follow-up to fix the weird comment

eregon · 2026-06-17T09:13:38Z

Nice.
Just one thing for posterity, have we figured out that it's always fine to cache by identity?
i.e. that ActiveRecord always returns the same connection configuration Hash (seems to be spec'd, good) but also that it's the case for all other usages of Datadog::Tracing::Contrib::Configuration::CachingResolver‎?

dmilisic and others added 4 commits May 6, 2026 09:22

Use object_id as cache key

6be549a

Adapt fix for ruby >= 3.2

5beb7fa

Merge branch 'master' into patch-2

6fe7cc9

Use compare_by_identity and add tests

017c329

marcotc requested review from a team as code owners June 11, 2026 07:23

marcotc requested review from vpellan and wantsui June 11, 2026 07:23

Merge branch 'master' into marcotc/patch-2

f561415

dd-octo-sts Bot added integrations Involves tracing integrations tracing labels Jun 11, 2026

marcotc mentioned this pull request Jun 11, 2026

Use object_id as cache key in CachingResolver #5719

Merged

ivoanjo approved these changes Jun 11, 2026

View reviewed changes

ivoanjo merged commit 0184b53 into master Jun 11, 2026
1144 of 1155 checks passed

ivoanjo deleted the marcotc/patch-2 branch June 11, 2026 08:32

dd-octo-sts Bot added this to the 2.36.0 milestone Jun 11, 2026

marcotc mentioned this pull request Jun 12, 2026

Update comment on crash workaround #5895

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Compare ActiveRecord connection cache entries by identity#5890

Compare ActiveRecord connection cache entries by identity#5890
ivoanjo merged 5 commits into
masterfrom
marcotc/patch-2

marcotc commented Jun 11, 2026 •

edited

Loading

Uh oh!

dd-octo-sts Bot commented Jun 11, 2026

Uh oh!

datadog-prod-us1-3 Bot commented Jun 11, 2026 •

edited by datadog-prod-us1-6 Bot

Loading

Uh oh!

pr-commenter Bot commented Jun 11, 2026

Explanation

More details about the CI and significant changes

Uh oh!

ivoanjo left a comment

Uh oh!

ivoanjo Jun 11, 2026

Uh oh!

marcotc Jun 12, 2026

Uh oh!

ivoanjo commented Jun 11, 2026

Uh oh!

Uh oh!

eregon commented Jun 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		# as large change around the crash site was done in that version (https://github.com/ruby/ruby/pull/14039/changes#diff-884a5a8a369ef1b4c7597e00aa65974cec8c5f54f25f03ad5d24848f64892869R1743),
		# where `RClass.cc_table` (the NULL dereferenced pointer) became a GC-managed object,

Uh oh!

Conversation

marcotc commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dd-octo-sts Bot commented Jun 11, 2026

Uh oh!

datadog-prod-us1-3 Bot commented Jun 11, 2026 • edited by datadog-prod-us1-6 Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pr-commenter Bot commented Jun 11, 2026

Benchmarks

Explanation

More details about the CI and significant changes

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

ivoanjo Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

marcotc Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

ivoanjo commented Jun 11, 2026

Uh oh!

Uh oh!

eregon commented Jun 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marcotc commented Jun 11, 2026 •

edited

Loading

datadog-prod-us1-3 Bot commented Jun 11, 2026 •

edited by datadog-prod-us1-6 Bot

Loading