fix: flush pending assignment and exposure events on LocalEvaluationClient.stop() - #79
fix: flush pending assignment and exposure events on LocalEvaluationClient.stop()#79cmyui wants to merge 1 commit into
Conversation
LocalEvaluationClient.stop() stopped the flag config poller and closed the connection pool, but never flushed or shut down the Amplitude analytics instances backing the assignment and exposure services. Any events still in their buffers (up to flush_queue_size per instance, accumulating for up to flush_interval_millis) were silently dropped unless the interpreter happened to exit cleanly enough for the analytics SDK's atexit hook to fire - which it often doesn't in forked/reaped server workers. stop() now flushes both instances, waits up to a configurable timeout (new parameter, default 10s, None = wait indefinitely) for the pending batches to send, then shuts the instances down. Instances shut down after the flush so late-tracked events are dropped deliberately rather than accumulating in a stopped client. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.
Reviewed by Cursor Bugbot for commit 41c2df7. Configure here.
| 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) |
There was a problem hiding this comment.
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.
Reviewed by Cursor Bugbot for commit 41c2df7. Configure here.
There was a problem hiding this comment.
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.


Problem
LocalEvaluationClient.stop()stops the flag config poller and closes the connection pool, but never touches theAmplitudeanalytics instances created for the assignment and exposure services. Events still sitting in their buffers — up toflush_queue_sizeper instance, accumulating for up toflush_interval_millis(defaults: 200 events / 10s) — are silently dropped.The analytics SDK does register an atexit shutdown hook, but that only fires on a clean interpreter exit of the main thread. In the environments where server-side local evaluation typically runs (gunicorn/uwsgi workers being recycled, forked job runners, containers receiving SIGKILL after a grace period), that hook frequently never runs — so the tail of assignment/
$exposureevents for every worker lifecycle is lost.stop()/ the context-manager__exit__is the documented lifecycle point, and it should deliver what was tracked.Fix
stop()now, after stopping the poller and closing the connection pool:flush()on the assignment and exposureAmplitudeinstances (public API) and collects the returned futures.timeoutparameter, default 10 seconds, consistent with the SDK's other network timeout defaults.Nonewaits indefinitely. On timeout, a warning is logged with the number of unsent batches.shutdown()on both instances — after the flush, so events tracked post-stop()are dropped deliberately rather than accumulating in a stopped client.Clients with no assignment/exposure config are unaffected.
Behavior change
stop()previously returned immediately; it can now block up totimeoutseconds performing network sends. That is the point of the fix, but it is a change — flagging it for review.stop(timeout=0)effectively restores fire-and-forget (flush is still triggered; the wait is skipped).Tests
tests/local/stop_flush_test.py: flush-then-shutdown ordering on both services, timeout bounding with a never-completing future, no-op with no event services, and context-manager exit. The 4 pre-existing errors intests/util/user_test.pyoccur identically on unmodifiedmainin my environment.🤖 Generated with Claude Code
Note
Medium Risk
Changes documented client lifecycle semantics and can block up to 10s on shutdown; behavior is intentional but may affect worker recycle timing in production.
Overview
LocalEvaluationClient.stop()now drains buffered assignment and exposure analytics before tearing down, instead of only stopping the flag poller and closing the HTTP pool.After the existing shutdown steps, it **
flush()**es each configured assignment/exposureAmplitudeinstance, **wait()**s on returned futures up to a newtimeoutargument (10s default;None= wait forever), logs a warning if batches remain, then callsshutdown()on those instances.__exit__still callsstop(), so context-manager use gets the same behavior. Clients without assignment/exposure config are unchanged.stop()may now block on network I/O (up totimeout);stop(timeout=0)still triggers flush but skips waiting.Adds
tests/local/stop_flush_test.pyfor flush/shutdown ordering, timeout bounding, no-op without event services, and context-manager exit.Reviewed by Cursor Bugbot for commit 41c2df7. Bugbot is set up for automated code reviews on this repo. Configure here.