[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: fe40e990-634d-4e04-b3d4-b65b2cdad94c

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

[Zaimwa9]

Once a kept batch fills the buffer, every track starts a new retry cycle. Skip the full-buffer flush while a failed batch is held.

[flagsmithsmith]

Fixed in 6aa2ec4. While the buffer holds a batch whose attempts all failed (@holding_failed_batch, set when a batch is kept and cleared when it is taken again), a full buffer no longer schedules a flush. Tracking still drops the oldest event, and the held batch goes out with the next interval or explicit flush. Covered by "waits for the next flush rather than attempting the kept batch again" and "sends a full buffer without waiting again once the kept batch is sent".

Writing that second test found a related bug, also fixed in 6aa2ec4. A flush skipped any event already attempted, even when the attempt happened before the flush was called. So flush on a kept batch with no new events sent nothing. A flush now only skips events sent by a batch taken after it was requested. New test: "sends a kept batch with the next flush, even with no events tracked since".

[Zaimwa9]

After close, track still buffers and flush still sends. Make both no-ops once stopping.

[flagsmithsmith]

Fixed in 6aa2ec4. Once stop (and so Client#close) begins, track_event/track_exposure_event buffer nothing, and flush (and so Client#flush_events) sends nothing; each logs at debug level ("Not tracking … event: the client is closed" / "Not flushing events: the client is closed"). start also does nothing after a stop. stop itself still sends what was buffered before the close, through its own final flush. Covered by the new "when stopped" specs.

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