[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: 0e49ce56-8c38-4342-8301-b1bbb56864bd

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 (f81d3f2) 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.

[Zaimwa9]

If close/2 times out while a flush is retrying, that flush keeps sending for up to ~12 s. Kill the processor on timeout, or run sends in a cancellable task.

[flagsmithsmith]

Fixed in f81d3f2. I went with killing the processor on timeout.

• When the {:close, deadline} call times out, close/2 kills the processor and waits for its :DOWN before returning {:error, :timeout}. It has to be :kill because the processor traps exits, so a :shutdown would just sit in its mailbox behind the running flush. The kill also takes down the request task that Tesla.Middleware.Timeout links to it, so no further attempt or backoff runs.
• The processor child is now restart: :temporary (it was :transient), so the supervisor doesn't bring a killed processor back. track/2 already starts a processor when none is registered, so tracking after a close or a crash still works. The only difference is that a crashed processor's flush timer now restarts with the next tracked event. The crashed processor's buffer was lost either way, so nothing that could be sent is left waiting.
• Known side effect: DynamicSupervisor logs a :killed child termination report when this happens, next to our own "Gave up sending events after … while closing" warning. It only happens when a close times out.

New test: "closing the client stops a flush still sending when its timeout expires". It starts a flush that the mocked events API keeps failing slowly, then calls close(config, 50) mid-retry. It asserts {:error, :timeout}, that the processor is dead and unregistered, and that no further attempt is made in the next 500 ms. It passed 10 out of 10 runs.

I left out the cancellable-task alternative. It would keep the processor alive, but flush callers would then need deferred replies, and events tracked during a send would need queueing behind the batch in flight. That's much more code for the same result here, since a closed processor isn't reused.

[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.