Repository navigation
feat: Retry event delivery and close the client with a timeout - #253
flagsmithsmith[bot] wants to merge 5 commits into
Conversation
Its entry in Flagsmith/sdk-spec's compatibility matrix at v0.9.0: `core events local-evaluation`.
Spec v0.7.1 runs the `@events` scenarios for this SDK: getting an experiment flag, tracking an event and tracking an exposure event, all with events enabled. The SDK already implements these (#226), so the steps file wires each step to FlagsmithClient.getExperimentFlag, trackEvent and trackExposureEvent, and creates the events-enabled client through FlagsmithConfig, next to the local evaluation client. (cherry picked from commit 0c48af8) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 29cbe7b) (cherry picked from commit 723a712)
Spec v0.8.0 specifies how SDKs deliver events. The event processor now: - Times out each events request after 3 s, and gives a batch 3 attempts, backing off from 1 s, doubling, with full jitter. Network errors, timeouts, 408, 429, 500, 502, 503 and 504 are retried. - Keeps a batch whose last attempt failed transiently in the buffer, ahead of newer events, for the next flush, unless that attempt got a 500. Any other error response drops the batch. - Stops event tracking, dropping the buffer, on a 401 or 403. - Sends one batch at a time, and caps the buffer at 1000 events by dropping the oldest, kept events included. - Never fails `flushEvents()`, which waits for the send, retries included. `FlagsmithClient.close(timeout, unit)` sends the buffered events for at most the timeout, then abandons the request in flight. `close()` waits for one batch's worst case, 12 s. The spec steps file wires the new steps: flushing events, closing with a timeout, tracking a range of events, and a client with both local evaluation and events enabled. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 88fa258)
flagsmithsmith
Bot
added
the
sdk-spec
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Requesting changes. Spec passes and the structure is good, but events behind a kept batch are never sent on flush or close, and a full buffer after a kept batch retries on every track. Please also retitle: the tracking methods are already on main.
| waiting = nextBatch; | ||
| nextBatch = null; | ||
| // A kept batch means the events API is failing: the buffer waits for the next flush. | ||
| if (waiting != null && outcome != Outcome.KEPT && !buffer.isEmpty()) { |
There was a problem hiding this comment.
A flush waiting behind a KEPT batch completes without sending newer events; on close they are dropped unattempted. Always send the buffer, kept batch first.
There was a problem hiding this comment.
Fixed in f3c57d7. When a flush (from the application, a timer tick or close()) is waiting behind the batch in flight and that batch is kept, settle now sends the whole buffer with the kept batch first. Before, it completed the flush without sending anything. close() therefore tries those events within its timeout instead of dropping them. New tests: flush_sendsTheEventsBehindAKeptBatchWithItWhenRequestedWhileItWasInFlight and close_sendsTheEventsBehindAKeptBatchWithIt. Both check that the sent events are add_to_cart ×4, then checkout.
| droppedToReport = recordDrop(1); | ||
| } | ||
| droppedToReport = dropOverflow(); | ||
| isFull = buffer.size() >= maxBufferItems; |
There was a problem hiding this comment.
After a kept batch every trackEvent starts a new retry cycle. Hold size-triggered flushes until the next tick or explicit flush.
There was a problem hiding this comment.
Fixed in f3c57d7. A full buffer now calls flush(false). After a batch is kept, a size-triggered flush neither starts a send nor, if it was registered while that batch was in flight, sends the buffer when the batch settles. The buffer waits for the next timer tick or explicit flush (flush() / close()). A batch that gets through, or is dropped, lifts the hold. Oldest-first dropping still caps the buffer at 1000 while it waits. New tests: trackEvent_holdsAFullBufferAfterAKeptBatchUntilTheNextFlush (it also checks that size-triggered sends resume after a success) and trackEvent_holdsABufferThatFilledBehindABatchThatIsThenKept.
…hile A flush requested while a batch was in flight completed without sending the events tracked since, when that batch was kept, and close() then dropped them without trying. Such a flush now sends the whole buffer, the kept batch first, and so does close(). After a kept batch, a full buffer no longer starts a new retry cycle on every tracked event: it waits for the next timer tick or explicit flush. A batch that gets through lifts the hold. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
flagsmithsmith
Bot
changed the title
getExperimentFlag, trackEvent and trackExposure methods
|
Thanks @Zaimwa9. Addressed in f3c57d7.
Fixed. A flush waiting behind a kept batch now sends the whole buffer, kept batch first.
Fixed. After a kept batch, a full buffer waits for the next timer tick or explicit flush. The next batch that gets through lifts the hold. Details in the inline reply.
Retitled to "feat: Retry event delivery and close the client with a timeout". Locally, |
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Requesting changes. Both earlier points are fixed, but with the flush timer disabled (withEventsFlushIntervalMillis(0)) one kept batch stops size-triggered delivery for the rest of the process.
| nextBatchRequested |= requested && nextBatch != null; | ||
| return nextBatch == null ? inFlight : nextBatch; | ||
| } | ||
| if (buffer.isEmpty() || (!requested && holdFullBuffer)) { |
There was a problem hiding this comment.
With the timer disabled nothing ticks, so after one kept batch only an explicit flushEvents() or close() lifts this hold; on main it recovers. Let the hold expire after a cool-down when the interval is 0.
Contributes to Flagsmith/sdk-spec#13
Contributes to Flagsmith/sdk-spec#23