Fix flaky CpuAndWallTimeWorker stats spec on Ruby < 3.3 - #6251
Conversation
The assertion `signal_handler_enqueued_sample >= sample_count` was flaky on Ruby < 3.3. `signal_handler_enqueued_sample` counts only samples enqueued by the SIGPROF handler, but signal delivery is asynchronous and can be lost, so the counter can undercount the actual samples and is not a reliable lower bound for `sample_count`. Relating the two was the same comparison removed as flaky in 4805c9d for the same reason. The test's intent -- samples were taken via thread interruption, not idle sampling -- is still verified by `interrupt_thread_attempts > 0` together with `skip_idle_samples_for_testing: true` disabling the idle path. Observed failing on Ruby 3.0 in https://github.com/DataDog/dd-trace-rb/actions/runs/32440039677/job/96648837950 (29/30 shards passed, 1 failed: `expected: >= 1, got: 0`).
p-datadog
added
the
AI Generated
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: ee9c77e | Docs | View more details | Give us feedback! |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee9c77e338
ℹ️ 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".
| # `signal_handler_enqueued_sample` counts only samples enqueued by the SIGPROF handler. Signal delivery is | ||
| # asynchronous and can be lost (delivered to a thread that released the GVL, or skipped during GC compaction on | ||
| # the alternate stack), so this counter can undercount the actual samples and is not a reliable lower bound for | ||
| # `sample_count`. Relating the two was flaky on Ruby < 3.3 (non-atomic `rb_postponed_job_register`); the same |
There was a problem hiding this comment.
Lost signal delivery cannot make this counter lower than samples produced by the current delivery: handle_sampling_signal increments signal_handler_enqueued_sample before it schedules sample_from_postponed_job. Therefore, the observed sample_count == 1/counter == 0 state means the sample came from another path, such as a postponed job left by an earlier worker on Ruby < 3.3. After removing the assertion, that exact state satisfies both remaining comparisons, so this spec can pass without the current worker ever successfully handling a signal and no longer verifies the statistic named in its description. Instead, make the polling condition wait for both a recorded sample and a positive accepted-signal count.
Useful? React with 👍 / 👎.
2 participants
What does this PR do?
Removes the flaky assertion
expect(stats.fetch(:signal_handler_enqueued_sample)).to be >= sample_countfrom theCpuAndWallTimeWorker"keeps statistics on how many samples were triggered by the background thread" spec.Motivation:
Flaky test observed on Ruby 3.0 in this CI job (PR #6043, an unrelated DI PR that does not touch profiling):
29 of 30 shards passed on the same commit; 1 failed. Pass/fail divergence on one commit = flaky. The failure was on Ruby 3.0 only; all other Ruby versions passed.
Failure:
signal_handler_enqueued_samplewas 0 whilesample_count(main-threadcpu-samples) was >= 1.Root cause:
signal_handler_enqueued_samplecounts only samples enqueued by the SIGPROF handler (handle_sampling_signalinext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c). Signal delivery is asynchronous and can be lost (delivered to a thread that released the GVL, or skipped during GC compaction on the alternate signal stack), so this counter can undercount the actual samples and is not a reliable lower bound forsample_count. The flake is Ruby < 3.3 specific: those versions use the non-atomicrb_postponed_job_register, whereas Ruby 3.3+ uses the atomicrb_postponed_job_trigger(no flake observed on 3.3+).Relating
signal_handler_enqueued_sampleto the actual sample count is the same comparison that was removed as flaky in 4805c9de91 for the same reason: "signal delivery is asynchronous, can be lost, duplicate signals are ignored ... we can't really rely ontrigger_sample_attemptsbeing related to actually how many times we sampled." That commit removed the sibling assertions; the line-266 assertion was later reintroduced in 6efd27d to tighten coverage, re-importing the flakiness.Fix: Remove the unreliable
signal_handler_enqueued_sample >= sample_countcomparison. The test's intent — samples were taken via thread interruption, not idle sampling — is still verified byinterrupt_thread_attempts > 0together withskip_idle_samples_for_testing: true(which disables the idle sampling path, so samples can only come via the signal/thread-interruption path). The remaining valid assertions (sample_count > 0,trigger_sample_attempts >= signal_handler_enqueued_sample,interrupt_thread_attempts > 0) are unchanged.Related PRs / commits:
signal_handler_enqueued_samplecomparisons as flaky for the same reason.Reproduction note: The flake (~3% per CI run) did not reproduce locally after extensive attempts on Ruby 3.0.7 (the failing CI Ruby) with the exact CI seed: 30 runs of the full profiling suite and 1000 iterations of an isolated reproducer were all clean. This matches the experience recorded in 4805c9d and c340b77, where the maintainers also could not reproduce locally. CI on this branch is the validation.
Change log entry
None.
How to test the change?
bundle exec rspec spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb:266passes on Ruby 3.0.7 (the failing CI Ruby) and Ruby 3.2.3. The fullspec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rbsuite remains green. The change is test-only; no production code is modified.