Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions src/amplitude_experiment/local/client.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from concurrent.futures import wait
from threading import Lock
from typing import Any, List, Dict, Set
from typing import Any, List, Dict, Set, Optional

from amplitude import Amplitude

Expand Down Expand Up @@ -158,12 +159,35 @@ def __setup_connection_pool(self):
self._connection_pool = HTTPConnectionPool(host, max_size=1, idle_timeout=30,
read_timeout=timeout, scheme=scheme)

def stop(self) -> None:
def stop(self, timeout: Optional[float] = 10.0) -> None:
"""
Stop polling for flag configurations. Close resource like connection pool with client
Stop polling for flag configurations, flush pending assignment and exposure events, and close resources
like the connection pool.

Parameters:
timeout (float | None): Maximum time, in seconds, to wait for pending assignment and exposure
events to finish sending before returning. Defaults to 10 seconds. Pass None to wait
indefinitely.
"""
self.deployment_runner.stop()
self._connection_pool.close()
self.__shutdown_event_services(timeout)

def __shutdown_event_services(self, timeout: Optional[float]) -> None:
instances = [service.amplitude for service in (self.assignment_service, self.exposure_service)
if service is not None]
if not instances:
return
futures = []
for instance in instances:
futures.extend(f for f in (instance.flush() or []) if f is not None)
if futures:
_, not_done = wait(futures, timeout=timeout)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flush futures nested, wait breaks

High Severity

Amplitude.flush() returns a list whose elements are themselves lists of batch futures (from each destination plugin), or None when a destination had nothing to send. This code treats that top-level list as a flat sequence of futures and passes it to wait(), which expects Future instances. When assignment or exposure events are actually pending, stop() raises instead of waiting, so the new flush path fails in the case it is meant to fix and shutdown() is never reached.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 41c2df7. Configure here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified against every released amplitude-analytics version in the supported range (floor >=1.1.1 through current 1.2.3): Workers.flush() never returns a list — it returns None, a single Future (1.1.1–1.2.0, and the single-batch case since 1.2.1), or a combined Future created via threads_pool.submit(wait_for_all) (multi-batch case since 1.2.1). Timeline.flush() therefore returns a flat list of Future | None, and the Nones are filtered before wait(). Also confirmed empirically: with real pending events queued in both the assignment and exposure instances, stop() flushes and returns without raising.

if not_done:
self.logger.warning(f"[Experiment] Stop timed out after {timeout}s waiting for "
f"{len(not_done)} pending event batch(es) to flush")
for instance in instances:
instance.shutdown()

def __enter__(self) -> 'LocalEvaluationClient':
return self
Expand Down
78 changes: 78 additions & 0 deletions tests/local/stop_flush_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import time
import unittest
from concurrent.futures import Future
from unittest.mock import MagicMock

from src.amplitude_experiment import LocalEvaluationClient, LocalEvaluationConfig
from src.amplitude_experiment.assignment import AssignmentConfig
from src.amplitude_experiment.exposure.exposure_config import ExposureConfig

API_KEY = 'server-api-key'


def completed_future() -> Future:
future = Future()
future.set_result(None)
return future


class LocalEvaluationClientStopTestCase(unittest.TestCase):

def _client_with_event_services(self) -> LocalEvaluationClient:
config = LocalEvaluationConfig(
assignment_config=AssignmentConfig(api_key='analytics-api-key'),
exposure_config=ExposureConfig(api_key='analytics-api-key'),
)
return LocalEvaluationClient(API_KEY, config)

def test_stop_flushes_then_shuts_down_assignment_and_exposure(self):
client = self._client_with_event_services()
assignment_amplitude = MagicMock()
assignment_amplitude.flush.return_value = [completed_future()]
exposure_amplitude = MagicMock()
exposure_amplitude.flush.return_value = [None]
client.assignment_service.amplitude = assignment_amplitude
client.exposure_service.amplitude = exposure_amplitude

client.stop()

assignment_amplitude.flush.assert_called_once()
exposure_amplitude.flush.assert_called_once()
assignment_amplitude.shutdown.assert_called_once()
exposure_amplitude.shutdown.assert_called_once()

def test_stop_timeout_bounds_wait_on_pending_events(self):
client = self._client_with_event_services()
never_completes = Future()
assignment_amplitude = MagicMock()
assignment_amplitude.flush.return_value = [never_completes]
client.assignment_service.amplitude = assignment_amplitude
client.exposure_service.amplitude = MagicMock(flush=MagicMock(return_value=[]))

start = time.monotonic()
client.stop(timeout=0.2)
elapsed = time.monotonic() - start

self.assertLess(elapsed, 2)
assignment_amplitude.shutdown.assert_called_once()

def test_stop_without_event_services(self):
client = LocalEvaluationClient(API_KEY, LocalEvaluationConfig())
client.stop()

def test_context_manager_exit_flushes(self):
client = self._client_with_event_services()
exposure_amplitude = MagicMock()
exposure_amplitude.flush.return_value = [completed_future()]
client.assignment_service.amplitude = MagicMock(flush=MagicMock(return_value=[]))
client.exposure_service.amplitude = exposure_amplitude

with client:
pass

exposure_amplitude.flush.assert_called_once()
exposure_amplitude.shutdown.assert_called_once()


if __name__ == '__main__':
unittest.main()