[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: 5c5b6ea1-b767-4ffe-9c98-ae9dc51fdfd4

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 (ab90211) with main (e968b8c)}

[flagsmithsmith]

Implemented SDK spec v0.9.0 in 4b281a4. CI had failed because the SDK was missing 4 steps: the local evaluation + events client, flushing events, closing the client, and tracking a numbered range of events. Behind those steps were 2 missing SDK features.

Event delivery (Flagsmith::EventProcessor)

• Replaced Faraday's single retry with explicit delivery. A batch is sent up to 3 times, one batch at a time, with a 3 s request timeout. Retries back off from 1 s, doubling, with full jitter.
• 5xx, 408, 429 and connection errors are retried. If the last attempt fails transiently, the batch is kept for the next flush, ahead of newer events. The exception is a 500 on the last attempt: that batch is dropped, so it is not resent forever. Other 4xx responses drop the batch. 401/403 stops event tracking.
• When the buffer is full, the oldest event is dropped. A flush started by a full buffer no longer sends again events that a concurrent flush already attempted.
• stop(timeout_seconds) stops the interval thread without killing a batch in flight. It sends the buffered events, waits at most the timeout, then cancels any pending retry.

Client

• New Client#flush_events: returns once the tracked events are delivered, dropped or kept, retries included.
• New Client#close(timeout_seconds: 5): stops polling and realtime, then the event processor.

Exposures under local evaluation

• The environment document's feature states carry metadata.experiment, and multivariate options carry key. The SDK used to drop both.
• Both are now mapped into the evaluation context. Identity flags from the document get experiment and variant, so get_experiment_flag records exposures, including control.
• Environment flags carry no experiment, matching the API.
• This follows feat: Record exposures under local evaluation flagsmith-python-client#276.

Tests and steps

• Unit tests cover retries, keep/drop rules, 401/403, the full buffer, the close timeout, flush_events/close, and local-evaluation exposures (new experiment_environment.json fixture).
• sdk-spec/steps.rb maps the new steps to these SDK calls.
• Locally, rspec (450 examples) and rubocop pass. My local spec run wasn't reliable (the Flagsmith container kept timing out), so CI is the check.

Unrelated to this change, possibly a bug: under local evaluation, identity overrides get priority Float::INFINITY. With "lowest wins", a segment override with priority 0 beats them. I left this alone.

[Zaimwa9]

Requesting changes. Spec passes and the threading is sound, but once a kept batch fills the buffer, every track restarts a retry cycle. Please also make close terminal.

[flagsmithsmith]

Requesting changes. Spec passes and the threading is sound, but once a kept batch fills the buffer, every track restarts a retry cycle. Please also make close terminal.

Both are fixed in 6aa2ec4; details are in the inline replies.

• While a kept batch is held, a full buffer waits for the next flush.
• After close, tracking and flushing do nothing.

That change also fixes a related bug: a flush with no events tracked since a kept batch failed used to send nothing. It now resends the kept batch.

Locally, rspec (456 examples, 6 consecutive runs) and rubocop pass.

[flagsmithsmith]

A follow-up to 6aa2ec4, in daead64. In CI, the spec run on 6aa2ec4 failed "A full buffer drops its oldest event": event_1 was delivered.

The cause was in 6aa2ec4's new rule for when a flush skips events another flush already sent. It counted batches taken. If the full-buffer flush had already taken the batch when the application called flush, that flush saw no batch taken since its request. So it resent the kept batch, and the resend got through.

It now counts batches settled. A batch in flight when a flush is requested settles after the request, so the flush skips the events it covered. The new spec "does not send again a batch in flight when the flush was requested" fails on 6aa2ec4 and passes now. All checks are green on daead64.

[Zaimwa9]

@themis-blindfold review

[themis-blindfold]

⚖️ Themis review: 🟠 Fix before merge

The retry policy does not match the requested delivery contract, and shutdown can still start a send after its deadline. Retained exposure events can also be duplicated in the buffer. The SDK specification and Ruby CI checks passed, but they do not cover these cases. Ruby was unavailable for local execution.

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

🟠 Majors

• lib/flagsmith/sdk/event_processor.rb:221 — Non-retryable server responses are retried.
• lib/flagsmith/sdk/event_processor.rb:91 — A queued shutdown flush can send after the deadline.
• lib/flagsmith/sdk/event_processor.rb:181 — Retained exposures lose deduplication state.

📝 Walkthrough

• Event delivery — adds retries, bounded buffering, explicit flush, and shutdown.
• Local evaluation — propagates experiment metadata and variant keys into identity flags.
• SDK specification — updates the shared workflow and adds event and shutdown steps.

🧪 How to verify

1. Return 500 and 501 from the events endpoint and check that each batch is dropped after one request.
2. Block an in-flight send, enqueue another event, close with a short timeout, and check that no queued send starts after close returns.
3. Exhaust retries for an exposure, track the same exposure again, and inspect the next batch.
4. Run the event processor and experiment specs across supported Ruby versions.
5. Automate: add regression tests for these response, shutdown, and retained-exposure cases.

Product take: Reliable event delivery and shutdown materially improve experiment data quality, provided failures and deadlines follow the shared SDK contract.

🧭 Assumptions & unverified claims

No unverified assumptions or claims.

May the last batch leave on time · reviewed at daead64

[flagsmithsmith]

@themis-blindfold review

I have gone through the Themis review of daead64 and replied to each finding inline:

• 500 and other server errors are retried. No change: spec v0.9.0 requires 500 to be retried, with the batch dropped only when the last attempt gets a 500. Its scenarios are quoted in the reply. Whether to drop 501/505+ at once, which the spec does not cover, is left to you.
• A queued final send could start after close returns. Fixed in ab90211: no batch is taken once closed.
• A kept batch lost its exposure deduplication. Fixed in ab90211: exposure keys are rebuilt when a batch is kept.

The new specs for the two fixes fail on daead64 and pass now. Locally, rubocop and rspec pass (460 examples). The only failures were in datapooling_manager_spec, which failed the same way locally before any event-processor change and passes in CI.