[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: 0cfea441-4eff-457d-a6b6-c85a2db13b2d

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

✅ 12 untouched benchmarks

---

_{Comparing openhands/sdk-spec-v0.9.0 (11df3af) with main (72c03c8)}

[flagsmithsmith]

Implemented SDK spec v0.9.0 in b6b19ed. The spec run was failing because the SDK had no steps (and no SDK behaviour) for event delivery retries, flushing, closing the client, or a client with both local evaluation and events.

Event delivery (lib/flagsmith_client_events_processor.ex)

• A batch is sent up to 3 times. Each attempt gives up after 3 seconds (httpc's timeout, so the connection is closed). Retries back off from 1 s, doubling, with full jitter. One batch is in flight at a time (the GenServer).
• Statuses 408, 429, 5xx and network errors are retried. Other 4xx drop the batch. A batch whose last attempt gets a 500 is dropped. Otherwise it is kept for the next flush, ahead of events tracked since.
• 401/403 stop event tracking for the environment key.
• A full buffer (1000) drops its oldest event instead of growing. Filling the buffer still triggers a send.
• The events client no longer goes through Tesla.Middleware.Retry, since retrying is now done by the processor.

New public API (lib/flagsmith_client.ex)

• Client.flush_events/1 sends the tracked events and returns once each is delivered, dropped, or kept.
• Client.close/2 sends the buffered events within a timeout (5 s by default), then stops the client's events processor and poller. Events not sent by then are dropped. The processor's restart strategy is now :transient so that closing stops it for good.

Local evaluation of experiment flags (needed by the new @local-evaluation @events scenarios)

• The environment document's feature-state metadata.experiment and the multivariate options' key were dropped during parsing. As a result, flags evaluated locally had no experiment or variant name, and no exposure was sent.
• Both are now parsed, and the engine's selected variant is carried onto the flag. As in the reference engine, an enrolled identity that is served no variant gets "control".

Steps (sdk-spec/steps.exs): the four new steps are wired to the SDK calls above, with no spec values hardcoded.

Tests: unit tests cover retries and the 3 s attempt timeout, kept/dropped batches (400, 500 last attempt, 503 last attempt), 401/403, full-buffer dropping, close (sends the buffer, and returns within its timeout), and locally evaluated experiment exposures. Locally I ran mix format --check-formatted, mix test (425 tests, 0 failures) and mix dialyzer (all clean), and the spec at v0.9.0 with core events local-evaluation (24 passed).

[Zaimwa9]

Requesting changes. Spec passes, but a flush already running keeps sending after close/2 times out.

[flagsmithsmith]

Requesting changes. Spec passes, but a flush already running keeps sending after close/2 times out.

Addressed in f81d3f2 (details in the inline reply). When close/2 times out, it now kills the events processor and waits until the processor is gone. The processor is now a :temporary child so it isn't restarted; tracking an event starts a new one, as before. I added a unit test for a close that times out while a flush is retrying.

Checks run locally:

• mix format --check-formatted: clean
• mix test: 426 tests, 0 failures
• mix dialyzer: clean
• Spec v0.9.0 with core events local-evaluation: 24 passed

One more thing: test/flagsmith_poller_test.exs "the refresh works accordingly" fails some of the time here. It does the same on main (9 of 15 runs of that file under Docker). It waits only 15 ms, and I haven't changed it.

[Zaimwa9]

@themis-blindfold review

[themis-blindfold]

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

[flagsmithsmith]

@themis-blindfold review

The review run didn't produce any findings. It failed with "Review failed after 2 attempt(s) (OutputError)", so I have nothing to address and haven't changed anything. Re-running it is up to you.

The branch is unchanged at f81d3f2, and every check passes (spec v0.9.0, build and test, benchmarks). Your earlier point about close/2 timing out mid-flush is addressed in that commit.

[Zaimwa9]

@themis-blindfold review

[themis-blindfold]

⚖️ Themis review: 🟠 Fix before merge

The retry and shutdown APIs cover the main delivery flow, but several edge cases undermine the promised delivery and resource bounds. The build, SDK specification, and benchmark checks passed. Local tests could not be run because Elixir and Mix are unavailable.

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

🟠 Majors

• lib/flagsmith_client_events_processor.ex: Non-retryable server responses are retried or retained.
• lib/flagsmith_client_events_processor.ex: An explicit flush can leave a failed batch unsent.
• lib/flagsmith_client_events_processor.ex: A concurrent close can terminate a flush caller.
• lib/flagsmith_client_events_processor.ex: Events awaiting processing bypass the buffer limit.

📝 Walkthrough

• Event delivery - adds retries, bounded buffering, explicit flush and close operations.
• Local evaluation - carries experiment metadata and variant keys into returned flags.
• SDK specification - updates the reusable workflow and adds lifecycle steps.

🧪 How to verify

1. Return 500 and 501 from the events service and check that each batch is dropped after one request.
2. Exhaust retries, restore the service, and explicitly flush again without tracking another event.
3. Start a flush against a slow service, then close with a short timeout and check the flush caller survives.
4. Flood tracking while a request is stalled and inspect total pending events and memory.
5. Automate: add regression tests for these delivery and concurrency cases.

Product take: Reliable delivery and predictable shutdown matter for experiment exposure data; these gaps can delay or lose events and affect host applications.

🧭 Assumptions & unverified claims

No unverified assumptions or claims.

May the next flush be a clean one · reviewed at f81d3f2

1 finding(s) anchored outside the diff (not posted inline)

• lib/flagsmith_client_events_processor.ex:203 🟠 Major · 🏗️ Heavy lift

Bound events queued while delivery is in progress

Observed: delivery and retry sleeps run inside the GenServer, while tracking sends asynchronous casts; the 1,000-event limit is applied only after those messages are processed. Predicted: sustained tracking during a slow request would accumulate an unbounded mailbox, exceeding the advertised buffer limit and potentially exhausting host memory. Apply admission and eviction to all pending events, including those arriving during a send, and test a stalled delivery under load.

[flagsmithsmith]

Replies to the Themis review of f81d3f2. The three inline findings have their own replies. Here is where each point landed:

Non-retryable server responses are retried or retained.

No change. The spec at v0.9.0 retries 500s: in "A batch whose last attempt fails otherwise is kept, after server errors", the proxy answers 500, 500, 503, then 202 for the same batch. It drops the batch only when the last attempt gets a 500. How to treat other 5xx (such as 501) is left open in the spec; I've offered that choice in the inline reply.

An explicit flush can leave a failed batch unsent.

No change, because the spec's "A full buffer drops its oldest event" requires it. Re-sending the kept full-buffer batch on the explicit flush would deliver event_1, and that scenario expects exactly event_2…event_1001. I've offered two alternatives in the inline reply, if you want different behaviour.

A concurrent close can terminate a flush caller.

Fixed in 19b4272. close/2 now cancels the batch being sent instead of killing the processor. Waiting flush/1 callers get {:error, :closed}, and flush/1 catches exits from its call.

Bound events queued while delivery is in progress (anchored outside the diff)

Fixed in 19b4272. This was real, both in this PR and before it: delivery and retry backoff ran inside the GenServer, so tracking casts piled up in its mailbox while a batch was retried. Batches are now sent by a task. The processor keeps handling tracked events during a send and drops the oldest once its buffer holds 1,000. At most one full buffer plus the batch in flight is held in memory, and a kept batch is merged back to 1,000.

The new test "events tracked while a batch is being sent are buffered, up to the buffer's limit" holds a batch of 1,000 at the events API, then tracks 1,500 more. It checks that the processor's mailbox is empty and it holds 1,000 buffered events. It also checks that the newest 1,000 are sent once the held batch completes.

Checks:

• mix format --check-formatted: clean
• mix test: 429 tests, 0 failures (3 runs)
• Events tests: 6 more runs, all passing
• mix dialyzer: clean
• CI on 19b4272: all checks pass, including the v0.9.0 spec run

My last local spec run got 15 of 24 through; the other 9 failed in the runner's setup. The runner's admin calls to Flagsmith failed, with 403 on POST /api/v1/projects/ and timeouts fetching environment documents. All 15 event delivery and close scenarios passed, and CI ran all 24.

[Zaimwa9]

Requesting changes. Sending batches in a task fixes the close and mailbox problems, but it introduces two regressions: under steady tracking flush_events/1 can wait indefinitely, and exposures dropped from a full buffer are never tracked again.

[flagsmithsmith]

Requesting changes. Sending batches in a task fixes the close and mailbox problems, but it introduces two regressions: under steady tracking flush_events/1 can wait indefinitely, and exposures dropped from a full buffer are never tracked again.

Both are fixed in 11df3af; details in the inline replies.

• Flush wait: a flush that arrives while a batch is in flight, with events tracked since, now waits for one more batch at most, then gets a reply. With nothing tracked since, it only waits for the batch in flight.
• Dropped exposures: exposures dropped by a full buffer, or when a kept batch no longer fits it, now have their keys forgotten, so they're tracked again.

There are 3 new regression tests, and all 3 fail on 19b4272.

Checks:

• mix format --check-formatted: clean
• mix test: 432 tests, 0 failures
• Events tests: 10 runs in a row, all passing
• mix dialyzer: clean
• CI on 11df3af: all checks pass, including the v0.9.0 spec run