[flagsmithsmith]

Contributes to Flagsmith/sdk-spec#23

[coderabbitai]

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 2cbb8565-d305-4454-a9ed-f59911484dfd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[codspeed]

Merging this PR will not alter performance

✅ 15 untouched benchmarks

---

_{Comparing openhands/sdk-spec-v0.9.0 (de59e68) with main (b480743)}

[Zaimwa9]

I started a review but will stop here, grab the first comments to understand the issues in the implementation.
Overall spec passes, but the event processor's structure is not good and needs a rework, not patches: threads are created three ways, with a pile of locks and flags. Use one long-lived worker thread, like EnvironmentDataPollingManager. __del__ must not block, flush_events() must never raise, and close() must be terminal.

[flagsmithsmith]

Replying to the review:

threads are created three ways, with a pile of locks and flags. Use one long-lived worker thread, like EnvironmentDataPollingManager.

Reworked rather than patched, in cd3aa8c. EventProcessor starts one long-lived daemon worker in its constructor. Like EnvironmentDataPollingManager, the worker waits on its stop signal between runs. It sends the buffered events when the flush interval elapses, when the buffer fills, when flush() asks, and once more on close. That replaces the timer, the thread per full buffer and the thread per close. The state is now one threading.Condition, three counters (tracked, flush requested, sent), plus closed, refused and an _abandoned event set when close() gives up. I kept the processor as the thread's owner rather than a Thread subclass, so that track_event/flush/close stay the processor's public interface. Happy to switch to a subclass if you prefer.

__del__ must not block

It now calls EventProcessor.stop(), which only marks the processor closed and wakes the worker. It never joins.

flush_events() must never raise

It logs Not flushing events: events are not enabled. and returns, instead of raising. It does nothing once the client is closed. The worker catches unexpected errors around a send, so a flush can't be left waiting on a dead thread.

close() must be terminal.

Once it is called, tracking and flushing are no-ops. It waits at most timeout_seconds for the final send, and no attempt starts after it returns. Calling it again does nothing. A request already in flight at that point can't be cancelled by requests, and ends within the 3 s request timeout (details in the inline reply).

Checks: 398 unit tests pass, with full coverage of the changed lines. mypy --strict . and pre-commit pass. Spec v0.9.0 passes locally with TAGS="core events local-evaluation" (24 scenarios).

[Zaimwa9]

@themis-blindfold review

[themis-blindfold]

Review failed after 2 attempt(s) (OutputError). Check the worker logs.

[flagsmithsmith]

@themis-blindfold review

This asked the themis-blindfold bot for a review, and its run failed before it produced one ("Review failed after 2 attempt(s) (OutputError)"). That failure is in the bot's worker, not in this repository, so there is no review feedback to address and I haven't changed any code. I haven't re-triggered it, so that you decide whether to run it again.

The branch is unchanged at cd3aa8c, and every check is green, including sdk-spec / Spec v0.9.0. The open choices from my previous replies are still yours: whether the atexit hook should wait up to the close timeout or not wait at all, and whether the worker should be a threading.Thread subclass.

[Zaimwa9]

@themis-blindfold review

[themis-blindfold]

Review failed after 2 attempt(s) (OutputError). Check the worker logs.

[flagsmithsmith]

@themis-blindfold review

The re-run failed the same way ("Review failed after 2 attempt(s) (OutputError)"), so there is still no review feedback to address, and the code is unchanged at cd3aa8c. Both failures happened in the bot's worker, and its logs aren't visible from this pull request, so I can't tell why it fails here. All checks are green, including sdk-spec / Spec v0.9.0.

[Zaimwa9]

@themis-blindfold review

[themis-blindfold]

Review failed after 2 attempt(s) (OutputError). Check the worker logs.

[flagsmithsmith]

@themis-blindfold review

The third run also failed before producing a review ("Review failed after 2 attempt(s) (OutputError)"), so there is nothing new to address. The code is unchanged at cd3aa8c, and all checks are green. The bot fails the same way every time on this pull request, so the cause is likely on its side; its worker logs would show why.

[Zaimwa9]

@themis-blindfold review

[themis-blindfold]

🟠 Major · 🏗️ Heavy lift

Flush retained events even when no new events were tracked

Observed: after three 503 responses, the batch is retained but _sent_count already equals _tracked_count; a second flush() returns without another request. This violates the requirement to retry the retained batch on the next flush, so applications cannot explicitly drain it until a new event arrives or the timer fires. Track flush requests independently of the tracked-event count and wait for the worker to process the retained batch.

[flagsmithsmith]

Right, fixed in de59e68. flush() used to wait only for events tracked since the last send, so a flush right after a failed send returned without sending the kept batch. It now asks the worker for a send of the whole buffer, kept events included. The exception is a send already running that covers every tracked event: then it waits for that send instead. That exception keeps the spec's "A full buffer drops its oldest event" passing, where a flush called while the full-buffer send is in flight must not send the batch again.

Tests: test_flush__kept_batch_and_no_new_events__kept_batch_sent (fails before the fix) and test_flush__send_in_flight_covers_tracked_events__waits_without_resending.

[themis-blindfold]

Fixed: a subsequent flush requests another send of the retained batch (flagsmith/analytics.py:278), with a regression test covering three 503 responses (tests/test_event_processor.py:374). Resolving this thread.

[themis-blindfold]

🟠 Major · 🏗️ Heavy lift

Remove deduplication keys when exposures are evicted

Observed: with a one-item buffer, tracking an exposure, then a custom event, then the same exposure leaves only the custom event buffered: eviction removes the exposure but leaves its key in _buffered_exposure_keys. The later exposure is discarded as a duplicate even though no copy remains to send. Store enough information with each buffered event to remove its exposure key on eviction, and keep the keys consistent when retained batches are requeued.

[flagsmithsmith]

Right, fixed in de59e68. Each buffered event now carries its exposure key. Dropping the oldest event forgets its key, and a kept batch puts its keys back when it is re-queued, so the keys always match the exposures in the buffer.

Test: test_track_exposure_event__equal_exposure_dropped_from_full_buffer__sent, which follows your scenario and fails before the fix.

[themis-blindfold]

The eviction case is fixed, but requeuing can still leave keys inconsistent. If an exposure is tracked again while its earlier batch is in flight, a kept batch can contain the same key as the buffer. When the older copy is evicted, flagsmith/analytics.py:371 removes the shared key even though the newer copy remains, so another duplicate is accepted. Account for duplicate keys when requeuing and evicting (flagsmith/analytics.py:353).

[themis-blindfold]

🟠 Major · ⚡ Quick win

Drop a batch immediately on HTTP 500

Observed: 500 is in the retry set, so the worker retries it and can retain the batch if a later attempt fails differently. The linked delivery requirements specify dropping the batch on any 500 without retrying. Predicted: repeated requests could duplicate events if the server processed the first request before returning 500. Remove 500 from the retry set and update the retry tests to require an immediate drop.

[flagsmithsmith]

Not changed: the spec this PR implements requires retrying a 500. The issue's first summary did say "Drop the batch on any other status, including 500", but the spec was later changed on purpose (sdk-spec commit "Retry a 500, then drop the batch instead of keeping it"). features/event_delivery.feature at v0.9.0 now has:

• "A batch whose last attempt gets a server error is dropped": 503, 503, 500, so the batch is dropped after the 500.
• "A batch whose last attempt fails otherwise is kept, after server errors": the SDK must send 500, 500, 503, then 202 for the same batch.

Dropping on the first 500 would fail the second scenario, and the human review here also asked for 500 in the retry set. The duplicate-delivery risk you raise is real, though, so if the events API cannot deduplicate, it would be worth raising in sdk-spec.

[themis-blindfold]

You're right; I relied on the outdated issue summary. I withdraw the finding. The checkout tests both sequences in tests/test_event_processor.py:335 and tests/test_event_processor.py:501.

[themis-blindfold]

⚖️ Themis review: 🟠 Fix before merge

The single-worker delivery path adds bounded shutdown and retries, but retained batches cannot be retried by a subsequent explicit flush, and buffer eviction can suppress an exposure that is no longer buffered. The 500 response policy also differs from the linked delivery requirements. The recorded CI checks passed.

Area	Score
🎯 Correctness	2/5
🧪 Test coverage	3/5
📐 Code quality	3/5
🚀 Product impact	3/5

🟠 Majors

• flagsmith/analytics.py — A second explicit flush does not retry a retained batch.
• flagsmith/analytics.py — Evicted exposures can suppress subsequent tracking.
• flagsmith/analytics.py — HTTP 500 is retried despite the stated delivery requirements.

⚖️ Acknowledged

• Unbounded send threads under load — thread resolved by @Zaimwa9.
• Blocking destructor and shutdown thread creation — thread resolved by @Zaimwa9.
• Non-serialisable metadata breaking delivery — thread resolved by @Zaimwa9.
• Inconsistent retry classification and retention of 501/505 responses — thread resolved by @Zaimwa9.
• Tracking and requests continuing after close — thread resolved by @Zaimwa9.

📝 Walkthrough

• Event delivery — replaces timer-driven sends with a single worker, retry policy and bounded buffer.
• Client lifecycle — adds explicit event flushing and a timeout-based close.
• Specification — updates the shared SDK specification and adds event lifecycle steps.

🧪 How to verify

1. Return three 503 responses, then 202; flush twice without tracking another event and check the second flush sends the retained batch.
2. Fill a small buffer, evict an exposure, then track that exposure again and check it is delivered.
3. Return 500 on the first attempt and check the batch is dropped without another request.
4. Close while a request is in flight and verify the timeout and remaining buffered events.
5. Automate: add regression tests for the retained-batch, eviction and 500 cases.

Product take: Reliable event delivery is a solid improvement for experimentation analytics; missed or repeated delivery can affect the data users rely on.

🧭 Assumptions & unverified claims

No unverified assumptions or claims.

May the next flush actually flush · reviewed at cd3aa8c

[flagsmithsmith]

@themis-blindfold review

This run produced a review (🟠 Fix before merge, at cd3aa8c). Replies to its three majors, also answered inline:

A second explicit flush does not retry a retained batch.

Fixed in de59e68. A flush now always sends the buffer, kept events included, unless a send already running covers every tracked event.

Evicted exposures can suppress subsequent tracking.

Fixed in de59e68. An exposure's key is forgotten when the event is dropped from a full buffer, and restored when a failed batch is kept.

HTTP 500 is retried despite the stated delivery requirements.

Not changed. The spec at v0.9.0 requires retrying a 500: "A batch whose last attempt fails otherwise is kept, after server errors" expects 500, 500, 503, then 202 for the same batch. Only a 500 on the last attempt drops the batch. The text the bot quotes is from the issue's earlier summary, which the spec later changed on purpose. Details are in the inline reply.

Checks: 401 unit tests pass, stable over 40 repeated runs, and the new regression tests fail without the fix. mypy --strict . and pre-commit pass. Spec v0.9.0 passes locally with TAGS="core events local-evaluation" (24 scenarios).

[Zaimwa9]

Requesting changes. During an events API outage the full-buffer wake restarts the retry cycle back to back instead of waiting for the flush interval, and max_buffer_items=0 now drops every event (inline).

[Zaimwa9]

During an outage the buffer stays full of kept events, so any event tracked during a retry cycle wakes the worker for another full cycle as soon as the current one ends, ignoring the flush interval (in a scaled-down 503 outage: 62 requests where timer pacing gives about 22). Wake on a full buffer only once a full buffer of new events has arrived, e.g. self._tracked_count - self._sent_count >= self._max_buffer.

[Zaimwa9]

With max_buffer_items=0 every tracked event is dropped here as soon as it is buffered, whereas on main each event was sent straight away. Clamp values below 1 in __init__.