Skip to content

SymDB: hot-load follow-ups — error boundary, race guard, deterministic tests, RBS type#5871

Merged
p-datadog merged 5 commits into
masterfrom
symdb-hot-load-followups
Jun 9, 2026
Merged

SymDB: hot-load follow-ups — error boundary, race guard, deterministic tests, RBS type#5871
p-datadog merged 5 commits into
masterfrom
symdb-hot-load-followups

Conversation

@p-datadog

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

Copy link
Copy Markdown
Member

What does this PR do?

Five follow-ups to PR #5697 (TracePoint :class hot-load hook for incremental class extraction):

  1. Error boundary around the hot-load TracePoint callback. The :class TracePoint fires inside the customer's class Foo; ... end body. An exception escaping the callback propagates into the class definition itself — verified empirically: a raise inside a :class TracePoint backtraces through <class:CustomerClass> and surfaces at the customer's class keyword, breaking their class load. The earlier MODULE_SINGLETON_CLASS_PRED dispatch closed one specific raise source (user-overridden singleton_class?); this commit closes the general case with a rescue around the callback body that matches the sibling pattern in start_upload/scheduler_loop/extract_and_upload: log at debug, report via telemetry, allow the customer's class load to proceed.

  2. enqueue_hot_load race guard. TracePoint#disable does not wait for in-flight callbacks. A :class event firing concurrently with stop_upload could reach enqueue_hot_load after the hook had been torn down and re-arm the scheduler — contradicting stop_upload's contract. Now bails out when @hot_load_tracepoint is nil. The buffer push above the guard is harmless because the next start_upload runs extract_all (@initial_extraction_done was reset), which clears the buffer first.

  3. Components#after_fork spec. Added a dedicated #after_fork describe block to components_spec.rb covering the symbol_database&.after_fork! wiring — both the dispatch path and the nil-safety case.

  4. Three sleeps replaced with deterministic waits in component_spec.rb:

    • "clears scheduler thread reference and pending schedule" — @scheduler_thread is assigned synchronously inside start_upload's mutex via ensure_scheduler_thread, so the sleep was never needed.
    • "leaves a child Component able to start_upload normally after fork-state reset" — replaced with wait_for_idle.
    • "stop_upload prevents a post-stop class definition from triggering extraction" — removed entirely; the @scheduled_at nil check is the structural proof (only enqueue_hot_load sets it after stop_upload, and enqueue_hot_load only runs if the TracePoint fired).
  5. RBS type fix. EXTRACT_DEBOUNCE_INTERVAL is declared Integer but participates in arithmetic with Float clock values and is stubbed to 0.05 in tests. Changed to ::Numeric.

Motivation:

Surfaced during review of #5697 after merge. Items 1 and 2 are behavioral; the rest are test/type hygiene.

Item 1 is the highest-impact: without the rescue, any unexpected raise inside the callback (mutex edge case, user-set Time.now_provider raising, future code change in enqueue_hot_load) propagates into customer code at class Foo; ... end. Item 2 closes a documented race window in which symdb can re-arm the scheduler after the customer disables it via remote config.

Change log entry

None.

Additional Notes:

  • Item 1 verification (Ruby 3.2.3): raising CustomerError inside a TracePoint :class callback produced a backtrace of block in <main><class:CustomerClass><main>. The exception was caught by a rescue placed around the surrounding class CustomerClass; end block — confirming the callback's raise propagates through the class definition body and into customer scope.

How to test the change?

  • bundle exec rspec spec/datadog/symbol_database/component_spec.rb — 51 examples, 0 failures. New specs:
    • rescues exceptions raised inside the :class hook so customer class loads do not break (item 1)
    • enqueue_hot_load called after stop_upload does not re-arm the scheduler (item 2)
  • bundle exec rspec spec/datadog/core/configuration/components_spec.rb — passes including the two new #after_fork examples (item 3).
  • bundle exec steep check lib/datadog/symbol_database/component.rb — clean.

Follow-ups to PR #5697 (TracePoint :class hot-load hook):

- enqueue_hot_load now bails out early when @hot_load_tracepoint has been
  cleared. TracePoint#disable does not wait for in-flight callbacks, so a
  :class event firing concurrently with stop_upload could reach
  enqueue_hot_load after the hook had been torn down and re-arm the
  scheduler — contradicting stop_upload's contract. The buffer push is
  harmless because the next start_upload runs extract_all (since
  @initial_extraction_done was reset), which clears the buffer first. A new
  spec exercises this race directly by calling enqueue_hot_load after
  stop_upload.

- Components#after_fork now has a dedicated spec covering the
  symbol_database&.after_fork! wiring added in #5697 — both the dispatch
  path and the nil-safety case.

- Three sleeps in component_spec.rb replaced with deterministic waits:
  - "clears scheduler thread reference and pending schedule": @scheduler_thread
    is assigned synchronously inside start_upload's mutex via
    ensure_scheduler_thread, so the sleep was never needed.
  - "leaves a child Component able to start_upload normally after fork-state
    reset": replaced sleep with wait_for_idle.
  - "stop_upload prevents a post-stop class definition from triggering
    extraction": removed sleep — the @scheduled_at nil check is the
    structural proof (only enqueue_hot_load sets it after stop_upload, and
    enqueue_hot_load only runs if the TracePoint fired).

- EXTRACT_DEBOUNCE_INTERVAL RBS type changed from Integer to ::Numeric.
  Production value is the Integer 5, but the constant participates in
  arithmetic with Float clock values and is stubbed to 0.05 in tests.

Verified with:
- bundle exec rspec spec/datadog/symbol_database/component_spec.rb
  (50 examples, 0 failures)
- bundle exec rspec spec/datadog/core/configuration/components_spec.rb
  (passes, including the two new after_fork examples)
- bundle exec steep check lib/datadog/symbol_database/component.rb (clean)

Co-Authored-By: Claude <[email protected]>
@p-datadog
p-datadog requested a review from a team as a code owner June 8, 2026 18:58
@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 8, 2026
@dd-octo-sts dd-octo-sts Bot added the debugger Live Debugger (+Dynamic Instrumentation, +Symbol Database) label Jun 8, 2026
The :class TracePoint fires inside the customer's `class Foo; ... end`
body. An exception escaping the callback propagates into the class
definition itself — verified empirically: a `raise` inside a :class
TracePoint backtraces through `<class:CustomerClass>` and surfaces at
the customer's `class` keyword, breaking their class load.

The earlier MODULE_SINGLETON_CLASS_PRED dispatch closed one specific
raise source (user-overridden `singleton_class?` with an incompatible
signature). This commit closes the general case with a rescue around
the callback body, matching the sibling pattern in `start_upload`,
`scheduler_loop`, and `extract_and_upload`: log at debug, report via
telemetry, allow the customer's class load to proceed.

New spec rigs `enqueue_hot_load` to raise, then defines a class —
asserts the class body does NOT raise, and that the debug log fires
with the failure details.

Co-Authored-By: Claude <[email protected]>
@p-datadog p-datadog changed the title SymDB: hot-load follow-ups — race guard, deterministic tests, RBS type SymDB: hot-load follow-ups — error boundary, race guard, deterministic tests, RBS type Jun 8, 2026
@dd-octo-sts

dd-octo-sts Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Typing analysis

Note: Ignored files are excluded from the next sections.

steep:ignore comments

This PR introduces 1 steep:ignore comment, and clears 1 steep:ignore comment.

steep:ignore comments (+1-1)Introduced:
lib/datadog/symbol_database/component.rb:570
Cleared:
lib/datadog/symbol_database/component.rb:548

@datadog-official

datadog-official Bot commented Jun 8, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 0.00%
Overall Coverage: 90.06% (-0.01%)

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

@p-datadog
p-datadog requested a review from Copilot June 8, 2026 19:10
@p-datadog

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

ℹ️ 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".

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

Follow-up hardening for SymDB’s TracePoint :class hot-load hook introduced in #5697, focusing on preventing customer-visible errors, closing a stop/start race, and improving test determinism and typing.

Changes:

  • Add an error boundary around the TracePoint :class callback and report failures via debug logs + telemetry.
  • Guard enqueue_hot_load to prevent re-arming the scheduler after stop_upload (TracePoint disable race).
  • Improve specs: add Components#after_fork coverage, replace sleeps with deterministic waits, and adjust RBS constant typing.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
lib/datadog/symbol_database/component.rb Adds TracePoint callback rescue and a post-stop_upload race guard in enqueue_hot_load.
spec/datadog/symbol_database/component_spec.rb Replaces sleeps with deterministic waits; adds specs for TracePoint rescue and the stop/race guard.
spec/datadog/core/configuration/components_spec.rb Adds #after_fork specs to cover symbol_database&.after_fork! wiring + nil-safety.
sig/datadog/symbol_database/component.rbs Changes EXTRACT_DEBOUNCE_INTERVAL type to ::Numeric to match float usage in tests/arithmetic.

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

Comment thread lib/datadog/symbol_database/component.rb Outdated
@pr-commenter

pr-commenter Bot commented Jun 8, 2026

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-06-08 20:45:23

Comparing candidate commit 2a83dcb in PR branch symdb-hot-load-followups with baseline commit 3ff4f70 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 45 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-ddsign and others added 3 commits June 8, 2026 15:53
The hot-load end-to-end test in remote_config_integration_spec.rb failed
on Ruby 2.6 batch 0 with seed 16392 — two test failures cascading:

  1) hot-load end-to-end / uploads a class defined after the initial
     upload completes:
     `expect(initial_form_count).to be >= 1` got 0
  2) extract_all module with methods AND nested class in same file
     places child class under parent module in the same FILE scope:
     `expect(file_scope).not_to be_nil`

Root cause of (1): the test called `wait_for_idle(timeout: 5)` without
checking the return value. On Ruby 2.6 with heavy monkey-patching from
AppSec specs running in seed-16392 order before this test, extract_all
exceeded 5s — the extractor.rb comment at collect_extractable_modules
specifically calls this out:

  > on Ruby 2.6 specifically, Module#name on unnamed singleton classes
  > with long ancestor chains (e.g. through monkey-patches prepended
  > into Kernel, common in dd-trace-rb test processes) is O(ancestors)
  > — measured ~20ms per call

wait_for_idle silently returned false on timeout, captured_forms was
still empty, the assertion failed at line 235. The test body's
`component.shutdown!` at the end was never reached → scheduler thread
leaked → continued running extract_all → eventually crashed with
ExpiredTestDoubleError on the inherited InstanceDouble logger, and
concurrently raced ObjectSpace iteration with extractor_spec's
extract_all in the next spec, causing failure (2).

Fix:
- Bump both wait_for_idle calls to 30s, matching the other e2e test in
  the same file (line 84).
- Add `expect(...).to be true` to the first wait_for_idle, matching the
  pattern already used at the second wait_for_idle in the same test.
- Wrap the test body in begin/ensure so component.shutdown! always runs
  — a mid-test failure must not leak the scheduler thread and pollute
  subsequent specs in the same rspec process.

Verified on Ruby 2.6.10 locally:
  bundle exec rspec spec/datadog/symbol_database/component_spec.rb \
    spec/datadog/symbol_database/remote_config_integration_spec.rb \
    spec/datadog/symbol_database/extractor_spec.rb
  → 181 examples, 0 failures

Also verified on Ruby 3.2.3: 223 examples, 0 failures across the
related spec files.

Note: this is a fix to a test added in merged PR #5697. It's pulled
into this PR (#5871) because the bug surfaces from this PR's batch
composition (4 new tests shift the random seed). Independent of
#5871's other follow-up fixes.

Co-Authored-By: Claude <[email protected]>
`do...end` blocks support a direct `rescue` clause since Ruby 2.5 — the
explicit `begin`/`end` inside the TracePoint :class block is redundant.
Auto-fixed by `bundle exec rake standard:fix`.

Behavior unchanged:
- `next if MODULE_SINGLETON_CLASS_PRED.bind(mod).call` still exits the
  block early without triggering the rescue (rescue only fires on raise).
- The rescue still catches any exception from the body, preventing
  propagation into the customer's `class Foo; ... end`.
- The rescue test (`rescues exceptions raised inside the :class hook so
  customer class loads do not break`) still passes.

Verified locally:
  bundle exec rake standard          → clean
  bundle exec rspec ...rescue test   → 1 example, 0 failures
  bundle exec steep check lib/...rb  → No type error detected

Co-Authored-By: Claude <[email protected]>
Address review comment: the TracePoint :class callback's outer rescue calls
logger.debug and telemetry.report — if either raises (custom logger
implementation, telemetry worker in an unexpected state), the exception
escapes the rescue and propagates to the customer's class body, defeating
the error-boundary goal documented above the callback.

- Wrap the logger/telemetry calls in an inner rescue in
  lib/datadog/symbol_database/component.rb
- Add behavioral test in spec/datadog/symbol_database/component_spec.rb
  that stubs raw_logger.debug to raise and verifies the class definition
  does not propagate the exception

Verified: full hot-load coverage spec group passes (4 examples, 0 failures);
steep check on component.rb reports no errors.

Co-Authored-By: Claude <[email protected]>
@p-datadog
p-datadog merged commit 34ba16b into master Jun 9, 2026
592 checks passed
@p-datadog
p-datadog deleted the symdb-hot-load-followups branch June 9, 2026 13:21
@dd-octo-sts dd-octo-sts Bot added this to the 2.36.0 milestone Jun 9, 2026
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