SymDB: hot-load follow-ups — error boundary, race guard, deterministic tests, RBS type#5871
Conversation
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]>
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]>
Typing analysisNote: Ignored files are excluded from the next sections.
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 2a83dcb | Docs | Datadog PR Page | Give us feedback! |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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". |
There was a problem hiding this comment.
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
:classcallback and report failures via debug logs + telemetry. - Guard
enqueue_hot_loadto prevent re-arming the scheduler afterstop_upload(TracePoint disable race). - Improve specs: add
Components#after_forkcoverage, 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.
BenchmarksBenchmark execution time: 2026-06-08 20:45:23 Comparing candidate commit 2a83dcb in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 45 metrics, 1 unstable metrics.
|
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]>
What does this PR do?
Five follow-ups to PR #5697 (TracePoint :class hot-load hook for incremental class extraction):
Error boundary around the hot-load TracePoint callback. The
:classTracePoint fires inside the customer'sclass Foo; ... endbody. An exception escaping the callback propagates into the class definition itself — verified empirically: araiseinside a:classTracePoint backtraces through<class:CustomerClass>and surfaces at the customer'sclasskeyword, breaking their class load. The earlierMODULE_SINGLETON_CLASS_PREDdispatch closed one specific raise source (user-overriddensingleton_class?); this commit closes the general case with a rescue around the callback body that matches the sibling pattern instart_upload/scheduler_loop/extract_and_upload: log at debug, report via telemetry, allow the customer's class load to proceed.enqueue_hot_loadrace guard.TracePoint#disabledoes not wait for in-flight callbacks. A:classevent firing concurrently withstop_uploadcould reachenqueue_hot_loadafter the hook had been torn down and re-arm the scheduler — contradictingstop_upload's contract. Now bails out when@hot_load_tracepointis nil. The buffer push above the guard is harmless because the nextstart_uploadrunsextract_all(@initial_extraction_donewas reset), which clears the buffer first.Components#after_forkspec. Added a dedicated#after_forkdescribe block tocomponents_spec.rbcovering thesymbol_database&.after_fork!wiring — both the dispatch path and the nil-safety case.Three sleeps replaced with deterministic waits in
component_spec.rb:@scheduler_threadis assigned synchronously insidestart_upload's mutex viaensure_scheduler_thread, so the sleep was never needed.wait_for_idle.@scheduled_atnil check is the structural proof (onlyenqueue_hot_loadsets it afterstop_upload, andenqueue_hot_loadonly runs if the TracePoint fired).RBS type fix.
EXTRACT_DEBOUNCE_INTERVALis declaredIntegerbut participates in arithmetic withFloatclock values and is stubbed to0.05in 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_providerraising, future code change inenqueue_hot_load) propagates into customer code atclass 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:
CustomerErrorinside aTracePoint :classcallback produced a backtrace ofblock in <main>→<class:CustomerClass>→<main>. The exception was caught by arescueplaced around the surroundingclass CustomerClass; endblock — 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_forkexamples (item 3).bundle exec steep check lib/datadog/symbol_database/component.rb— clean.