Validate fix for flaky: SymDB scheduler logger.debug#5944
Conversation
Hypothesis: when raw_logger.debug is stubbed to raise (as in PR #5871's "does not propagate exceptions when logger.debug itself raises" test), the scheduler thread's @logger.debug calls in extract_and_upload, scheduler_loop's rescue, and start_upload's rescue have no inner rescue and terminate the thread. Component#shutdown!'s @scheduler_thread.join then re-raises the exception in the test thread (standard Thread#join semantics, all Ruby versions). Forces the race deterministically by holding the scheduler in extract_all on a Queue, activating the stub, then releasing. Reproduces on Ruby 2.6.10, 3.0.7, 3.1.7, 3.3.10, 3.4.8 locally.
Most @logger.debug / @logger.warn calls in SymbolDatabase::Component run
on the scheduler thread:
- extract_and_upload success-path log (component.rb:500)
- extract_and_upload's rescue handler (line 511)
- scheduler_loop's rescue handler (line 458)
- start_upload's rescue handler (line 202, called from main but
schedules the scheduler thread)
A misbehaving customer logger (custom Logger subclass, IO error,
frozen state, rspec-mocks stub) that raises from any of these sites
escapes scheduler_loop, terminating the scheduler thread. Component
#shutdown! invokes @scheduler_thread&.join(5), and Thread#join re-
raises the unhandled exception in the caller — standard Ruby semantics
across every supported version. The customer sees a SymDB exception at
shutdown!.
Fix at the wrapper level: SymbolDatabase::Logger#debug and #warn
now swallow exceptions from the wrapped target. #trace routes through
#debug so it inherits the protection. The same defense pattern that
install_hot_load_hook applies inline at component.rb:566-571 now
applies at every SymDB log site automatically.
Implementation: replaces Forwardable.def_delegators with explicit
methods. Forwardable's generated forwarders cannot rescue; the explicit
version uses bare rescue (StandardError) — IOError / Errno from a disk-
full or broken pipe logger are caught, SignalException / SystemExit
are not.
New spec covers debug / warn / trace under both success and target-
raises conditions.
The companion reproducer at spec/datadog/symbol_database/
logger_boom_reproducer_spec.rb is on the reproduce-symdb-scheduler-
logger-boom branch and demonstrates this fix neutralizing the bug.
p-datadog
added
the
AI Generated
dd-octo-sts
Bot
added
the
debugger
Typing analysisNote: Ignored files are excluded from the next sections. Untyped methodsThis PR introduces 4 partially typed methods, and clears 4 partially typed methods. Partially typed methods (+4-4)❌ Introduced:Untyped other declarationsThis PR introduces 2 untyped other declarations, and clears 2 untyped other declarations. Untyped other declarations (+2-2)❌ Introduced:If you believe a method or an attribute is rightfully untyped or partially typed, you can add |
|
BenchmarksBenchmark execution time: 2026-06-23 18:16:45 Comparing candidate commit c719299 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.
|
There was a problem hiding this comment.
Pull request overview
This PR validates that the SymDB flake fix (swallowing exceptions raised by a wrapped logger) neutralizes the deterministic reproducer that previously forced the scheduler-thread logger.debug race in CI.
Changes:
- Add explicit
Datadog::SymbolDatabase::Logger#debug/#warnimplementations that rescue exceptions from the wrapped target logger. - Add unit coverage for the Logger wrapper’s forwarding and exception-swallowing behavior.
- Add a deterministic reproducer spec that exercises the scheduler-thread path and asserts
Component#shutdown!does not re-raise logger exceptions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/datadog/symbol_database/logger.rb | Switches away from Forwardable delegation for debug/warn and adds exception swallowing in the wrapper. |
| sig/datadog/symbol_database/logger.rbs | Updates RBS to reflect the logger wrapper’s behavior/documentation changes. |
| spec/datadog/symbol_database/logger_spec.rb | New unit specs for forwarding semantics and swallowing exceptions for debug/warn/trace. |
| spec/datadog/symbol_database/logger_boom_reproducer_spec.rb | New deterministic scheduler-thread reproducer ensuring shutdown doesn’t propagate logger failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def debug(*args, &block) | ||
| @target.debug(*args, &block) | ||
| rescue | ||
| nil | ||
| end |
| def warn(*args, &block) | ||
| @target.warn(*args, &block) | ||
| rescue | ||
| nil | ||
| end |
| component = Datadog::SymbolDatabase::Component.build(settings, agent_settings, logger) | ||
|
|
||
| # Wait for the scheduler to enter extract_all. | ||
| Timeout.timeout(5) { entered_extract.pop } | ||
|
|
||
| # Stub raw_logger.debug to raise. The scheduler is blocked inside | ||
| # extract_all and has not yet reached its post-extraction debug log. | ||
| allow(raw_logger).to receive(:debug).and_raise(RuntimeError.new('logger boom')) | ||
|
|
||
| # Release extract_all. The scheduler proceeds to the success-path | ||
| # `@logger.debug { "symdb: initial extracted ..." }` call, which now | ||
| # raises through the wrapped Forwardable chain. | ||
| resume_extract.push(:go) | ||
|
|
||
| # `shutdown!` invokes `@scheduler_thread&.join(5)`. Without the fix, | ||
| # the scheduler thread has terminated with the unhandled `logger boom` | ||
| # exception, and `Thread#join` re-raises it here. | ||
| expect { component.shutdown! }.not_to raise_error | ||
| end |
| # Release extract_all. The scheduler proceeds to the success-path | ||
| # `@logger.debug { "symdb: initial extracted ..." }` call, which now | ||
| # raises through the wrapped Forwardable chain. | ||
| resume_extract.push(:go) |
What does this PR do?
Validates that the fix in #5943 neutralizes the forced failure in #5942. This branch merges:
logger.debugrace deterministically)SymbolDatabase::Logger#debug/#warnswallow target exceptions)If CI passes, the fix addresses the exact failure mode the reproducer forces.
If CI fails, the fix is insufficient.
Do not merge — this PR exists for validation only.
Change log entry
None.
How to test the change?
CI passes = fix works against the forced failure. CI fails = fix is insufficient.
Companion PRs: