DEBUG-5089 Rescue all exceptions in DI and SymDB#5945
Conversation
Typing analysisNote: Ignored files are excluded from the next sections.
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: e07c0c0 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-06-29 19:28:39 Comparing candidate commit e07c0c0 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.
|
|
@codex review |
There was a problem hiding this comment.
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_fataland updates DI/symdb catch-all rescues to use them. - Updates DI serializer fatal-exception handling to use a dedicated
SERIALIZER_FATAL_EXCEPTION_CLASSES(excludingNoMemoryErrorby design). - Adds/extends specs and RBS signatures to cover non-
StandardErrorand 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.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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
left a comment
There was a problem hiding this comment.
I think few small bits here and there could be helpful. The rest is solid 👏🏼
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.
1c6f0bd to
1d5d504
Compare
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).
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.
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 newDatadog::DI.reraise_if_fatalhelper backed byDatadog::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