Fix flaky: CpuAndWallTimeWorker "always simulates signal delivery"#5980
Fix flaky: CpuAndWallTimeWorker "always simulates signal delivery"#5980p-datadog wants to merge 1 commit into
Conversation
Root cause: `recorder.serialize!` drains the recorder, so the `all_samples` captured inside `try_wait_until` is only the single batch drained by the iteration whose wait condition first became truthy. The condition was `samples.any?`, which becomes truthy on the first surviving (non-GC, non-overhead) sample of ANY thread. When that first batch contains a sample for another thread but no sample for the main thread, `samples_for_thread(all_samples, Thread.current)` is empty, so `[].reduce(:+)` returns nil and `expect(sample_count).to be > 0` fails with "got: nil". This is the same unreliable wait pattern that commit 3431b01 migrated away from in the sibling specs of this file (switching to an observable stat), but this spec was not migrated. Fix: wait until the drained batch actually contains a main-thread sample, so the batch kept as `all_samples` is guaranteed to have one. This is a test synchronization fix; the assertions are unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 56ee2b9 | Docs | Datadog PR Page | Give us feedback! |
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR fixes a flaky profiling spec (CpuAndWallTimeWorker#start when using the no signals workaround always simulates signal delivery) by making the wait condition align with what the test later asserts on: the presence of main-thread samples in the specific batch returned by recorder.serialize! (which drains the recorder).
Changes:
- Update the
try_wait_untilcondition to wait for at least one main-thread sample (samples_for_thread(..., Thread.current).any?) instead of any sample at all, preventing the drained batch from containing only profiler-internal-thread samples.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
|
Ahh my bad I missed this one and basically did the same in #5983 . Sorry for the duplicated work :( We can close off this one now. |
What does this PR do?
Fixes the flaky spec
spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb:678(CpuAndWallTimeWorker#start when using the no signals workaround always simulates signal delivery) by waiting on a reliable condition instead ofsamples.any?.Motivation:
Observed flaking on Ruby 2.7 / build & test (standard) [0] (commit
ab2601db), an unrelated PR. In that jobprofiling:mainran 3 times and this spec failed once and passed twice on the same commit; it passed on all other Ruby versions.Failure:
Root cause:
recorder.serialize!drains the recorder, so theall_samplescaptured insidetry_wait_untilis only the single batch drained by the iteration whose wait condition first became truthy. The conditionsamples.any?becomes truthy on the first surviving (non-GC, non-overhead) sample of any thread.Since the on-serialize flush was added in #5955, every
serialize!emits catch-up samples for the profiler-internal threads (the worker and the idle-sampling helper). Those samples are not GC and notprofiler overhead, so they survive the filter. When aserialize!happens before the worker's first per-tick sample has hit the main thread, the drained batch contains only profiler-internal-thread samples (and/or another application thread) with no main-thread sample.samples_for_thread(all_samples, Thread.current)is then empty,[].reduce(:+)returnsnil, andexpect(sample_count).to be > 0fails withgot: nil.This is the same unreliable wait pattern that #5955 migrated away from in the sibling specs of this file (switching to
stats.fetch(:cpu_sampled) > 0), but this spec was not migrated.Fix:
Wait until the drained batch actually contains a main-thread sample, so the batch kept as
all_samplesis guaranteed to have one. Test-synchronization change only; the assertions are unchanged (no weakening).Change log entry
None.
How to test the change?
Reproduced deterministically and verified locally (Ruby 3.2.3):
serialize!flushes only profiler-internal-thread samples: the unmodified spec fails 20/20 with the exactgot: nil; with the knob off it passes; with this fix applied it passes 30/30.