Fix race in SimpleFixedSizeExemplarReservoir offer/collect (#5431)

opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exemplar/exemplar_reservoir.py

@@ -5,6 +5,7 @@
 from collections import defaultdict
 from collections.abc import Callable, Mapping, Sequence
 from random import randrange
+from threading import Lock
 from typing import (
     Any,
 )
@@ -151,6 +152,14 @@ def __init__(self, size: int, **kwargs) -> None:
         self._reservoir_storage: Mapping[int, ExemplarBucket] = defaultdict(
             ExemplarBucket
         )
+        # Guards `_reservoir_storage` and the subclass-specific reservoir state
+        # (e.g. `SimpleFixedSizeExemplarReservoir._measurements_seen`) against the
+        # offer()↔collect() race documented in #5431. Without it, collect() can
+        # run _reset() and zero `_measurements_seen` between the increment and the
+        # `randrange(0, self._measurements_seen)` call in `_find_bucket_index`,
+        # producing `ValueError: empty range for randrange() (0, 0, 0)`, and the
+        # `defaultdict` can also be read-while-written during collect()'s iteration.
+        self._lock: Lock = Lock()
 
     def collect(self, point_attributes: Attributes) -> list[Exemplar]:
         """Returns accumulated Exemplars and also resets the reservoir for the next
@@ -164,16 +173,17 @@ def collect(self, point_attributes: Attributes) -> list[Exemplar]:
             exemplars contain the attributes that were filtered out by the aggregator,
             but recorded alongside the original measurement.
         """
-        exemplars = [
-            e
-            for e in (
-                bucket.collect(point_attributes)
-                for _, bucket in sorted(self._reservoir_storage.items())
-            )
-            if e is not None
-        ]
-        self._reset()
-        return exemplars
+        with self._lock:
+            exemplars = [
+                e
+                for e in (
+                    bucket.collect(point_attributes)
+                    for _, bucket in sorted(self._reservoir_storage.items())
+                )
+                if e is not None
+            ]
+            self._reset()
+            return exemplars
 
     def offer(
         self,
@@ -190,17 +200,18 @@ def offer(
             attributes: Measurement attributes
             context: Measurement context
         """
-        try:
-            index = self._find_bucket_index(
-                value, time_unix_nano, attributes, context
-            )
-
-            self._reservoir_storage[index].offer(
-                value, time_unix_nano, attributes, context
-            )
-        except BucketIndexError:
-            # Ignore invalid bucket index
-            pass
+        with self._lock:
+            try:
+                index = self._find_bucket_index(
+                    value, time_unix_nano, attributes, context
+                )
+
+                self._reservoir_storage[index].offer(
+                    value, time_unix_nano, attributes, context
+                )
+            except BucketIndexError:
+                # Ignore invalid bucket index
+                pass
 
     @abstractmethod
     def _find_bucket_index(
@@ -287,12 +298,13 @@ def offer(
         context: Context,
     ) -> None:
         """Offers a measurement to be sampled."""
-        index = self._find_bucket_index(
-            value, time_unix_nano, attributes, context
-        )
-        self._reservoir_storage[index].offer(
-            value, time_unix_nano, attributes, context
-        )
+        with self._lock:
+            index = self._find_bucket_index(
+                value, time_unix_nano, attributes, context
+            )
+            self._reservoir_storage[index].offer(
+                value, time_unix_nano, attributes, context
+            )
 
     def _find_bucket_index(
         self,

opentelemetry-sdk/tests/metrics/test_exemplarreservoir.py

@@ -1,7 +1,8 @@
 # Copyright The OpenTelemetry Authors
 # SPDX-License-Identifier: Apache-2.0
 
-from time import time_ns
+import threading
+from time import sleep, time_ns
 from unittest import TestCase
 
 from opentelemetry import trace
@@ -83,6 +84,66 @@ def test_reset_after_collection(self):
         self.assertEqual(new_exemplars[0].value, 4.0)
         self.assertEqual(new_exemplars[1].value, 5.0)
 
+    def test_concurrent_offer_and_collect_does_not_raise(self):
+        """Regression test for #5431.
+
+        ``offer()`` and ``collect()`` must be synchronized so that
+        ``collect()``→``_reset()`` cannot zero ``_measurements_seen`` between
+        the ``_measurements_seen += 1`` increment and the
+        ``randrange(0, self._measurements_seen)`` call in
+        ``_find_bucket_index``. Without the lock, that race produces
+        ``ValueError: empty range for randrange() (0, 0, 0)`` under concurrent
+        offer/collect.
+
+        size=1 maximizes the probability of hitting the randrange branch on
+        every offer after the first one, so the race shows up quickly.
+        """
+        reservoir = SimpleFixedSizeExemplarReservoir(size=1)
+        stop = threading.Event()
+        errors: list[BaseException] = []
+
+        def offer_loop() -> None:
+            i = 0
+            while not stop.is_set():
+                try:
+                    reservoir.offer(
+                        float(i), time_ns(), {"k": str(i)}, Context()
+                    )
+                    i += 1
+                except BaseException as e:  # noqa: BLE001 - surface any error
+                    errors.append(e)
+                    return
+
+        def collect_loop() -> None:
+            while not stop.is_set():
+                try:
+                    reservoir.collect({})
+                except BaseException as e:  # noqa: BLE001 - surface any error
+                    errors.append(e)
+                    return
+
+        threads = [
+            threading.Thread(target=offer_loop),
+            threading.Thread(target=offer_loop),
+            threading.Thread(target=collect_loop),
+        ]
+        for t in threads:
+            t.start()
+
+        # Bounded run. The race is reproducible in well under a second on
+        # Python 3.10+; 1.0s gives headroom on slow CI runners without
+        # noticeably slowing down the test suite.
+        sleep(1.0)
+        stop.set()
+        for t in threads:
+            t.join(timeout=5.0)
+
+        self.assertEqual(
+            errors,
+            [],
+            f"expected no errors under concurrent offer/collect, got: {errors}",
+        )
+
 
 class TestAlignedHistogramBucketExemplarReservoir(TestCase):
     TRACE_ID = int("d4cda95b652f4a1592b449d5929fda1b", 16)