Skip to content

Fix race in SimpleFixedSizeExemplarReservoir offer/collect (#5431)#5445

Closed
feiiiiii5 wants to merge 1 commit into
open-telemetry:mainfrom
feiiiiii5:fix/exemplar-reservoir-race-5431
Closed

Fix race in SimpleFixedSizeExemplarReservoir offer/collect (#5431)#5445
feiiiiii5 wants to merge 1 commit into
open-telemetry:mainfrom
feiiiiii5:fix/exemplar-reservoir-race-5431

Conversation

@feiiiiii5

Copy link
Copy Markdown

Fixes #5431.

Problem

SimpleFixedSizeExemplarReservoir._find_bucket_index does:

self._measurements_seen += 1                                    # (a)
if self._measurements_seen < self._size:
    return self._measurements_seen - 1
index = randrange(0, self._measurements_seen)                   # (b)

Between (a) and (b), a concurrent collect() can run _reset() and zero
_measurements_seen, producing:

ValueError: empty range for randrange() (0, 0, 0)

FixedSizeExemplarReservoirABC.collect() has a matching race: it iterates
self._reservoir_storage.items() (a defaultdict) while offer() can
insert a new bucket, raising RuntimeError: dictionary changed size during iteration.

Fix

Add a threading.Lock on FixedSizeExemplarReservoirABC.__init__ and
guard offer() and collect() with it.

AlignedHistogramBucketExemplarReservoir overrides offer() (because
it computes its bucket index from the value, not the reservoir state),
so its override must also acquire the lock — otherwise the shared
_reservoir_storage would still be unsafe.

Verification

  • New regression test test_concurrent_offer_and_collect_does_not_raise
    runs 2 offer threads + 1 collect thread for 1.0s on a size=1
    reservoir (maximizes the probability of hitting the randrange
    branch on every offer after the first).
  • Red → green: with the fix stashed, the test fails with the exact
    `ValueError: empty range for randrange() (0, 0, 0)` from the issue.
    With the fix, it passes.
  • Full exemplar test suite passes:
    uv run --python 3.10 --frozen --all-packages python -m unittest \\
      tests.metrics.test_aggregation \\
      tests.metrics.test_exemplarfilter \\
      tests.metrics.test_exemplarreservoir \\
      tests.metrics.integration_test.test_exemplars
    
    → 51 tests pass.
  • `ruff check` clean.

Notes

  • The lock is on FixedSizeExemplarReservoirABC, not on the base
    ExemplarReservoir, because the base has no shared mutable state to
    protect — the race is specific to the _reservoir_storage defaultdict
    and the subclass-specific reservoir state (_measurements_seen).
  • Performance: offer() is now O(1) under the lock (no I/O, no
    allocations beyond the existing defaultdict lookup), so contention
    is bounded. For high-throughput metrics pipelines, the next step would
    be a per-bucket lock or a lock-free reservoir, but neither is in scope
    for this bug fix.

`_find_bucket_index` does `self._measurements_seen += 1` then
`randrange(0, self._measurements_seen)`. Between those two lines,
`collect()` can run `_reset()` and zero `_measurements_seen`, producing
`ValueError: empty range for randrange() (0, 0, 0)`.

The `defaultdict`-backed `_reservoir_storage` has the matching race:
collect() iterates `self._reservoir_storage.items()` while offer() can
insert a new bucket, raising `RuntimeError: dictionary changed size
during iteration`.

Add a `threading.Lock` on `FixedSizeExemplarReservoirABC` and guard
`offer()` / `collect()` with it. `AlignedHistogramBucketExemplarReservoir`
overrides `offer()`, so its override takes the lock too — otherwise the
shared `_reservoir_storage` would still be unsafe.

Regression test `test_concurrent_offer_and_collect_does_not_raise` runs
2 offer threads + 1 collect thread for 1.0s. Without the lock it
reproduces the exact `ValueError` from the issue; with the lock it
passes.
@feiiiiii5
feiiiiii5 requested a review from a team as a code owner July 20, 2026 01:44
@herin049

Copy link
Copy Markdown
Contributor

Closing this given that there is already an open PR #5437

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

ValueError: empty range for randrange() — data race in SimpleFixedSizeExemplarReservoir between offer() and collect()

2 participants