test(profiler): Remove wall-clock race in continuous profiler auto-start tests - #7483
Draft
LouisDeconinck wants to merge 1 commit into
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Refs #6957.
Source of the flake
In
test_continuous_profiler_auto_start_and_stop_sampled{,_span_streaming}, the block between the first and second transaction/segment relied ontime.sleep(0.03)followed byassert get_profiler_id() is not Noneand, later,len(profiler_ids) == 1/max_chunks=1. Those assertions only hold if the test thread wakes before the sampler's soft shutdown commits.Soft shutdown needs at least two sampler cycles after the last profile stops — the first observes and removes the inactive profile, the next sees empty queues and returns
True, andrunningis set toFalseat the end of that cycle's interval. At the mocked 21 Hz frequency that is >= ~47.6 ms afterprofile.stop(), so the 30 ms sleep had only ~17 ms of margin: if the test thread oversleeps or is preempted past the window (routine on a loaded CI runner), the profiler legitimately stops, and eitherprofiler should be runningfails or transaction 2 spins up a new profiler session, breaking the single-profiler-id/single-chunk assertions. This is the same class of race as the originally reportedassert get_profiler_id() is Nonefailure (fixed in #7364), just in the other direction. Reproduced deterministically by lengthening the gap sleep past the shutdown window.Fix
Added
suspend_profiler_sampling(), a small helper that wrapsscheduler.samplerwith a gate (threading.Event). While suspended, the sampler thread parks insideself.sampler()and cannot observe stopped profiles or complete soft shutdown, soget_profiler_id()keeps reporting the running profiler regardless of test-thread scheduling. Transaction/segment 2's profile is already queued byauto_start()before the gate is released inside its body, so the same buffer/profiler session is reused deterministically — no wall-clock assumptions remain in the critical section. The original sampler is restored in afinally, so the parked thread always resumes and self-terminates normally even on failure.This does not mask genuine profiler failures: an early/unexpected shutdown still fails
profiler should be running, and a profiler that never stops still failswait_for_profiler_to_stop()'s bounded poll. That poll's ceiling was raised to 5 s — it waits for an event that must occur once the queues are empty, so the bound only guards against scheduler starvation, not timing.Test coverage / verification
TESTPATH=tests/profiler/test_continuous_profiler.py uv run tox -e py3.12-gevent— 92 passed (coversthreadandgeventparams)pytest tests/profiler/— 105 passednprocbusy loops) — all greenuv run ruff check/ruff format— clean;uv run --group typing mypy sentry_sdk— no issuesReminders
uv run ruff.feat:,fix:,ref:,meta:)