Skip to content

DEBUG-5089 Rescue all exceptions in DI and SymDB#5945

Merged
p-datadog merged 11 commits into
masterfrom
di-symdb-rescue-exception
Jun 29, 2026
Merged

DEBUG-5089 Rescue all exceptions in DI and SymDB#5945
p-datadog merged 11 commits into
masterfrom
di-symdb-rescue-exception

Conversation

@p-datadog

@p-datadog p-datadog commented Jun 23, 2026

Copy link
Copy Markdown
Member

What does this PR do?

Replaces rescues of StandardError with rescues of Exception (with SystemExit, SignalException, NoMemoryError) reraised. The re-raise is centralized in a new Datadog::DI.reraise_if_fatal helper backed by Datadog::DI::FATAL_EXCEPTION_CLASSES.

Motivation:

Testing SymDB extraction on an actual application (gitlab source) revealed that SymDB runs customer code and, in case of gitlab, that code raises NotImplemented which does not derive from SymDB. Result is dead SymDB worker thread and no symbols (but customer application otherwise is unaffected).

#5946 makes it less likely that SymDB will invoke customer code. This PR expands the exception rescues to the ones below StandardError.

Change log entry

Yes. DI: the symbol database and DI background threads no longer stop when application code raises a non-StandardError (e.g. NotImplementedError from an overridden #name) during introspection; previously this could silently disable symbol uploads for the rest of the process.

How to test the change?

Unit tests

@p-datadog p-datadog added the AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos label Jun 23, 2026
@dd-octo-sts dd-octo-sts Bot added the debugger Live Debugger (+Dynamic Instrumentation, +Symbol Database) label Jun 23, 2026
@dd-octo-sts

dd-octo-sts Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Typing analysis

Note: Ignored files are excluded from the next sections.

steep:ignore comments

This PR introduces 30 steep:ignore comments, and clears 30 steep:ignore comments.

steep:ignore comments (+30-30)Introduced:
lib/datadog/di/instrumenter.rb:122
lib/datadog/di/instrumenter.rb:155
lib/datadog/di/instrumenter.rb:380
lib/datadog/di/instrumenter.rb:382
lib/datadog/di/instrumenter.rb:708
lib/datadog/di/probe_notification_builder.rb:139
lib/datadog/di/probe_notification_builder.rb:375
lib/datadog/di/remote.rb:47
lib/datadog/di/remote.rb:51
lib/datadog/di/remote.rb:54
lib/datadog/di/serializer.rb:248
lib/datadog/di/serializer.rb:377
lib/datadog/di/serializer.rb:492
lib/datadog/symbol_database/component.rb:579
lib/datadog/symbol_database/extractor.rb:359
lib/datadog/symbol_database/extractor.rb:371
lib/datadog/symbol_database/extractor.rb:386
lib/datadog/symbol_database/extractor.rb:650
lib/datadog/symbol_database/extractor.rb:680
lib/datadog/symbol_database/extractor.rb:804
lib/datadog/symbol_database/extractor.rb:903
lib/datadog/symbol_database/extractor.rb:904
lib/datadog/symbol_database/extractor.rb:995
lib/datadog/symbol_database/extractor.rb:996
lib/datadog/symbol_database/extractor.rb:1044
lib/datadog/symbol_database/remote.rb:112
lib/datadog/symbol_database/remote.rb:120
lib/datadog/symbol_database/scope_batcher.rb:260
lib/datadog/symbol_database/uploader.rb:195
lib/datadog/symbol_database/uploader.rb:209
Cleared:
lib/datadog/di/instrumenter.rb:121
lib/datadog/di/instrumenter.rb:154
lib/datadog/di/instrumenter.rb:377
lib/datadog/di/instrumenter.rb:379
lib/datadog/di/instrumenter.rb:702
lib/datadog/di/probe_notification_builder.rb:137
lib/datadog/di/probe_notification_builder.rb:372
lib/datadog/di/remote.rb:45
lib/datadog/di/remote.rb:49
lib/datadog/di/remote.rb:50
lib/datadog/di/serializer.rb:243
lib/datadog/di/serializer.rb:372
lib/datadog/di/serializer.rb:485
lib/datadog/symbol_database/component.rb:573
lib/datadog/symbol_database/extractor.rb:355
lib/datadog/symbol_database/extractor.rb:367
lib/datadog/symbol_database/extractor.rb:382
lib/datadog/symbol_database/extractor.rb:639
lib/datadog/symbol_database/extractor.rb:669
lib/datadog/symbol_database/extractor.rb:790
lib/datadog/symbol_database/extractor.rb:867
lib/datadog/symbol_database/extractor.rb:886
lib/datadog/symbol_database/extractor.rb:976
lib/datadog/symbol_database/extractor.rb:977
lib/datadog/symbol_database/extractor.rb:1025
lib/datadog/symbol_database/remote.rb:108
lib/datadog/symbol_database/remote.rb:115
lib/datadog/symbol_database/scope_batcher.rb:258
lib/datadog/symbol_database/uploader.rb:193
lib/datadog/symbol_database/uploader.rb:207

@datadog-prod-us1-5

datadog-prod-us1-5 Bot commented Jun 23, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 22.34%
Overall Coverage: 89.91% (-0.11%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: e07c0c0 | Docs | Datadog PR Page | Give us feedback!

@pr-commenter

pr-commenter Bot commented Jun 23, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-06-29 19:28:39

Comparing candidate commit e07c0c0 in PR branch di-symdb-rescue-exception with baseline commit 4d54496 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 ----------------------------------'

@p-datadog

Copy link
Copy Markdown
Member Author

@codex review

@p-datadog
p-datadog requested a review from a team as a code owner June 24, 2026 23:14
@p-datadog
p-datadog requested a review from Copilot June 24, 2026 23:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR strengthens error boundaries in the Dynamic Instrumentation (DI) and Symbol Database subsystems by changing catch-all handlers to rescue Exception while explicitly re-raising process-fatal exceptions. It introduces a shared helper (Datadog::DI.reraise_if_fatal) and adds specs to ensure non-StandardError failures don’t silently stop background work.

Changes:

  • Introduces Datadog::DI::FATAL_EXCEPTION_CLASSES + Datadog::DI.reraise_if_fatal and updates DI/symdb catch-all rescues to use them.
  • Updates DI serializer fatal-exception handling to use a dedicated SERIALIZER_FATAL_EXCEPTION_CLASSES (excluding NoMemoryError by design).
  • Adds/extends specs and RBS signatures to cover non-StandardError and fatal-exception propagation.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
spec/datadog/symbol_database/extractor_spec.rb Adds regression tests for non-StandardError handling and fatal exception propagation in extraction.
spec/datadog/di/serializer_spec.rb Adds tests ensuring custom serializer errors don’t break serialization and fatal exceptions propagate.
spec/datadog/di/fatal_exceptions_spec.rb New spec validating DI fatal exception list and helper behavior.
sig/datadog/di/serializer.rbs Updates signature to reflect renamed serializer fatal exception constant.
sig/datadog/di/fatal_exceptions.rbs Adds RBS for FATAL_EXCEPTION_CLASSES and reraise_if_fatal.
lib/datadog/symbol_database/uploader.rb Switches upload rescue to Exception, re-raising fatal exceptions via DI helper.
lib/datadog/symbol_database/scope_batcher.rb Switches batcher rescues to Exception and re-raises fatal exceptions.
lib/datadog/symbol_database/remote.rb Switches RC integration rescues to Exception and re-raises fatal exceptions.
lib/datadog/symbol_database/file_hash.rb Switches hashing rescue to Exception and re-raises fatal exceptions.
lib/datadog/symbol_database/extractor.rb Switches multiple introspection rescues to Exception and re-raises fatal exceptions.
lib/datadog/symbol_database/component.rb Switches scheduler/hot-load rescues to Exception and re-raises fatal exceptions.
lib/datadog/di/transport/input.rb Switches transport rescues to Exception and re-raises fatal exceptions.
lib/datadog/di/serializer.rb Renames serializer fatal exception constant and broadens rescues to Exception while preserving “safe stub” behavior.
lib/datadog/di/remote.rb Switches RC receiver rescues to Exception and re-raises fatal exceptions.
lib/datadog/di/probe_notifier_worker.rb Switches worker rescues to Exception and re-raises fatal exceptions.
lib/datadog/di/probe_notification_builder.rb Switches template evaluation rescue to Exception and re-raises fatal exceptions.
lib/datadog/di/probe_manager.rb Switches probe manager rescues to Exception and re-raises fatal exceptions.
lib/datadog/di/probe_file_loader.rb Switches file loader rescues to Exception and re-raises fatal exceptions.
lib/datadog/di/instrumenter.rb Switches instrumenter rescues to Exception and re-raises fatal exceptions.
lib/datadog/di/fatal_exceptions.rb New helper and fatal exception list used by DI/symdb catch-all rescues.
lib/datadog/di/component.rb Switches component rescues to Exception and re-raises fatal exceptions.
lib/datadog/di/code_tracker.rb Switches code-tracker rescues to Exception and re-raises fatal exceptions.
lib/datadog/di/base.rb Ensures preload/base path loads fatal-exception helper and uses it in catch-all rescue.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/datadog/di/fatal_exceptions.rb
Comment thread lib/datadog/di/remote.rb
Comment thread lib/datadog/di/instrumenter.rb
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: 1c6f0bd345

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@Strech Strech left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think few small bits here and there could be helpful. The rest is solid 👏🏼

Comment thread lib/datadog/di/serializer.rb Outdated
Comment thread spec/datadog/di/fatal_exceptions_spec.rb Outdated
Comment thread spec/datadog/di/fatal_exceptions_spec.rb Outdated
Comment thread spec/datadog/di/fatal_exceptions_spec.rb
p-ddsign added 7 commits June 28, 2026 21:54
Shared by DI and symbol database catch-all rescue handlers to re-raise
process-fatal exceptions (SystemExit, SignalException, NoMemoryError)
that must never be swallowed.
The DI preload path (datadog/di/preload -> base) activates code tracking
before Datadog::Core is loaded, so the catch-all rescue handlers on that
path cannot reference Core. Define FATAL_EXCEPTION_CLASSES and
reraise_if_fatal under Datadog::DI instead, in a small dependency-free
file, and drop them from Core.
Convert every catch-all rescue (rescue => e / bare rescue) in the DI
subtree to rescue Exception and re-raise process-fatal exceptions
(SystemExit, SignalException, NoMemoryError) via DI.reraise_if_fatal,
catching everything else. Previously these handlers caught only
StandardError, so a non-StandardError such as NotImplementedError would
escape and could kill a background thread.

Typed rescues are left narrow. The serializer keeps its own
SERIALIZER_FATAL_EXCEPTION_CLASSES (without NoMemoryError) because it must
return a safe stub rather than tear the process down when serializing a
large or deeply recursive object.
Convert every catch-all rescue in the symbol database subtree to
rescue Exception and re-raise process-fatal exceptions via
DI.reraise_if_fatal, catching everything else. This stops a
non-StandardError (e.g. NotImplementedError from a customer-overridden
.name) from aborting the whole extraction pass and silently killing the
scheduler thread; such a class is now logged and skipped.

Each affected file requires datadog/di/fatal_exceptions directly.
Adds the missing signature file for lib/datadog/di/fatal_exceptions.rb,
which was failing `rake rbs:missing` (and therefore steep/typecheck and
all-jobs-are-green): every new file under lib/ needs a matching sig/ file.

Also renames the serializer constant in its RBS to match the source: the
code renamed FATAL_EXCEPTION_CLASSES to SERIALIZER_FATAL_EXCEPTION_CLASSES,
but the signature still declared the old name, which would have failed
`steep check` once rbs:missing passed.
Covers the behavior change that had no tests:

- Datadog::DI.reraise_if_fatal: re-raises each fatal class (SystemExit,
  SignalException, Interrupt, NoMemoryError) and returns for non-fatal
  ones (StandardError, NotImplementedError, LoadError, SystemStackError);
  also pins FATAL_EXCEPTION_CLASSES contents and frozen-ness.

- Extractor#extract_all: continues (returns [], logs) when introspection
  raises a non-StandardError such as NotImplementedError -- the scenario
  that previously killed the scheduler thread -- and still propagates a
  fatal exception.

- Serializer custom-serializer condition path: skips the serializer when
  the condition raises a non-StandardError, and propagates a fatal one.

- Serializer main path: propagates a fatal exception instead of returning
  a safe stub (complements the existing NoMemoryError-is-caught test).
steep/typecheck failed:
  lib/datadog/di/transport/input.rb:116:116: [error]
  Type `(::Array[::String] | nil)` does not have method `first`
  (Ruby::NoMethod)

Root cause: this PR broadened the catch-all rescues from `rescue => exc`
(implicit StandardError) to `rescue Exception => exc`. With the implicit
form Steep types `exc` as untyped and does not check the receiver of
`exc.backtrace.first`; with the explicit class Steep types `exc` as
`Exception`, whose `#backtrace` is `Array[String]?` in the stdlib RBS, so
`.first` on a possibly-nil array is flagged. The backtrace-logging line
itself is unchanged — the stricter rescue type is what surfaced the latent
nil dereference.

`Exception#backtrace` genuinely can be nil (no backtrace before raise,
set_backtrace with strings, or an overridden #backtrace — see
probe_notification_builder.rb), and `rescue Exception` now catches the
broader set of exception classes where that is more likely. So this is a
real nil-safety fix, not a Steep workaround: guard with `&.first`. When
backtrace is present the behavior is identical; when nil the debug log
interpolates an empty string instead of raising NoMethodError.

The same `exc.backtrace.first` pattern appears in two more sites that this
PR converted to `rescue Exception` — probe_notifier_worker.rb:99 and :325.
Steep did not flag them only because probe_notifier_worker.rb is in the
Steepfile ignore list, but the latent nil dereference is identical. Fixed
all three for consistency.

Verified: ruby -c clean on both files. `&.first` on `Array[String]?` is the
standard Steep-accepted nil-safe idiom and resolves the Ruby::NoMethod
diagnostic. Steep was not run locally -- bundle install fails here with
Bundler::PermissionError (gem dir not writable); the steep/typecheck CI job
validates.
@p-datadog
p-datadog force-pushed the di-symdb-rescue-exception branch from 1c6f0bd to 1d5d504 Compare June 29, 2026 01:57
p-ddsign added 4 commits June 29, 2026 09:02
These files call Datadog::DI.reraise_if_fatal but did not require the file
that defines it. reraise_if_fatal lives in datadog/di/fatal_exceptions, which
is loaded only via datadog/di/base (the boot path, MRI >= 2.6). di/remote is
loaded unconditionally via datadog/di and via core remote config
(capabilities.rb), and several DI files are required directly by specs, so the
catch-all rescue path could raise NoMethodError while handling another
exception.

Add require_relative 'fatal_exceptions' to every DI file that calls it,
matching what the symbol_database files in this PR already do: code_tracker,
component, instrumenter, probe_file_loader, probe_manager,
probe_notification_builder, probe_notifier_worker, remote, transport/input.

Add spec/datadog/di/fatal_exceptions_loading_spec.rb, a subprocess test
asserting reraise_if_fatal is defined after a standalone require of di/remote
and di/instrumenter.

Addresses review comments on remote.rb and instrumenter.rb.
Remove the FATAL_EXCEPTION_CLASSES contents assertion (it mirrors the constant
and breaks on any edit) and the is-frozen test; the fatal/non-fatal behavior is
already exercised by the .reraise_if_fatal cases. Add a test asserting
reraise_if_fatal preserves the original backtrace when re-raising.

Addresses review comments on fatal_exceptions_spec.rb.
Rename SERIALIZER_FATAL_EXCEPTION_CLASSES across the definition, the three
serializer rescue sites, and the RBS signature. No behavior change.

Addresses review comment on serializer.rb.
Add @param/@return YARD tags to Datadog::DI.reraise_if_fatal so the
public helper's contract is explicit (type of exc, nil-on-non-fatal
return).
@p-datadog p-datadog changed the title Re-raise fatal exceptions from DI and symbol database catch-all rescues DEBUG-5089 Re-raise fatal exceptions from DI and symbol database catch-all rescues Jun 29, 2026
@p-datadog p-datadog changed the title DEBUG-5089 Re-raise fatal exceptions from DI and symbol database catch-all rescues DEBUG-5089 Rescue all exceptions in DI and SymDB Jun 29, 2026
@p-datadog
p-datadog merged commit e3ffae1 into master Jun 29, 2026
590 checks passed
@p-datadog
p-datadog deleted the di-symdb-rescue-exception branch June 29, 2026 20:17
@dd-octo-sts dd-octo-sts Bot added this to the 2.37.0 milestone Jun 29, 2026
p-datadog pushed a commit that referenced this pull request Jul 2, 2026
PR #5945 (Rescue all exceptions in DI and SymDB) converted DI/SymDB
catch-all rescues to `rescue Exception` + Datadog::DI.reraise_if_fatal,
so non-StandardError exceptions do not escape into customer code while
fatal exceptions (SystemExit, SignalException, NoMemoryError) still
propagate. That merge covered the code present at the time; this PR later
added two new catch-all rescues that still used `rescue => e`
(StandardError only):

- lib/datadog/di/capture_expression_evaluator.rb: per-expression rescue
  (runs customer expression code) — now rescue Exception + reraise_if_fatal,
  with require_relative "fatal_exceptions"
- lib/datadog/di/instrumenter.rb: entry-time capture-expression rescue —
  now rescue Exception + reraise_if_fatal, matching the sibling
  condition-evaluation rescue above it

Added evaluator coverage mirroring #5945's tests: a non-StandardError
(NotImplementedError) is caught and recorded as an evaluation error; a
fatal exception (SystemExit) is re-raised out of #evaluate.

Verified: rspec (evaluator + instrumenter + integration + duration = 174,
0 failures), StandardRB clean, Steep clean.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos debugger Live Debugger (+Dynamic Instrumentation, +Symbol Database)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants