[flagsmithsmith]

Contributes to Flagsmith/sdk-spec#13
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: d0d9f70f-8dd6-4e93-b62c-1f3db3f5a9bb

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 (be7f8b8) with main (65ae046)}

[flagsmithsmith]

Spec v0.9.0's event delivery and close scenarios, and the local evaluation scenarios with events, failed because the SDK lacked the behaviour they describe. This push implements that behaviour in EventProcessor and wires the spec steps to it.

SDK changes (sdk/events.ts, sdk/index.ts, sdk/types.ts)

• Retries: a failed batch gets up to 3 attempts per flush. The backoff starts at 1s and doubles each time, with full jitter. Before, it got 2 attempts with a fixed backoff.
• Keep or drop after the last attempt: if the last attempt finds the events API unavailable (a network error, a timeout, or a 408, 429, 502, 503 or 504), the batch is kept and sent with the next flush, ahead of newer events. A batch whose last attempt gets any other server error is dropped. A 4xx drops the batch at once, with no retry.
• 401/403: the events API refused the environment key, so the processor drops what it has buffered and stops tracking events.
• One batch in flight: flushes now run one after the other.
• Bounded buffer: once maxBuffer events are buffered (1000 by default), each new event drops the oldest. The buffer no longer flushes early when it fills up: the "full buffer drops its oldest event" scenario fails if it does.
• Request timeout: events API requests time out after 3s by default, instead of using the client's 10s timeout. eventProcessorConfig.requestTimeoutMs still overrides it.
• close(timeoutMs?): takes an optional timeout. When it runs out, close aborts the request and any backoff in flight, then returns. Without a timeout it waits for the flush, as before.

Steps (sdk-spec/steps.mts)
Adds the steps Create the SDK client with local evaluation and events enabled, tracks events "event_{first}" to "event_{last}", flushes events and closes the client with a timeout of {timeout} seconds. Each one calls the public SDK API.

Tests
The unit tests in tests/sdk/events.test.ts now cover the backoff bounds and jitter, the keep/drop rules for each kind of failure, the 401/403 stop, the bounded buffer and how a kept batch is trimmed to fit it, flushes running one at a time, the default request timeout, and stop with a timeout. Tests for the old early flush on a full buffer and the old 2-attempt retry are replaced.

Locally, all 24 spec scenarios for core events local-evaluation pass against the pinned Flagsmith stack, and npm test passes (490 tests).

[Zaimwa9]

Requesting changes. Spec passes, but close sends a request after returning, interval flushes pile up during an outage, and removing the full-buffer send loses events above ~100/s. Please also retitle: the tracking methods are already on main.

[flagsmithsmith]

Thanks for the review. All of it is addressed in ae69b84. Replies are on each inline comment; here are the review-level points:

Spec passes, but close sends a request after returning,

Fixed. Each attempt now checks the abort signal before it sends, so a close timeout that hits during the backoff stops the retries.

interval flushes pile up during an outage,

Fixed. A timer tick is skipped while a flush is running or queued.

and removing the full-buffer send loses events above ~100/s.

Fixed. Tracking an event into a full buffer flushes it again. The new event drops the oldest one only when a batch is in flight or kept from a failed flush. Because the flush triggers when an event arrives at a full buffer, not when the buffer reaches maxBuffer, the spec's A full buffer drops its oldest event scenario still passes. All 24 spec scenarios pass locally.

Please also retitle: the tracking methods are already on main.

Retitled to "feat: Deliver events with retries, a bounded buffer and a close timeout".

close() is now terminal as well, and retries use the explicit status set 408, 429, 500, 502, 503 and 504. npm test passes (499 tests).

[Zaimwa9]

@themis-blindfold review

[themis-blindfold]

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

[flagsmithsmith]

@themis-blindfold review

The review bot didn't produce a review: it replied "Review failed after 2 attempt(s) (OutputError)", so there are no findings to address, and I haven't changed any code. The branch is still at ae69b84, which addresses your earlier review, and every check on it passes. Whether to run the bot again is up to you.

[Zaimwa9]

@themis-blindfold review

[themis-blindfold]

⚖️ Themis review: 🟠 Fix before merge

The event processor adds bounded buffering, retries and a shutdown timeout, but its handling of HTTP 500 conflicts with the delivery requirement, and its exposure deduplication state can outlive events evicted from the buffer. The recorded CI checks passed, including the SDK specification and Node build-and-test jobs.

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

🟠 Majors

• sdk/events.ts:30 — HTTP 500 responses are retried contrary to the stated delivery requirement.
• sdk/events.ts:339 — Evicted exposures remain in the deduplication set.

📝 Walkthrough

• Event delivery — serialises flushes, retries selected failures and retains unavailable batches.
• Buffer and shutdown — caps buffered events and adds a terminal close with an optional timeout.
• Specification — upgrades the shared SDK scenarios and adds event step implementations.

🧪 How to verify

• Return HTTP 500 from the events service and check that a flush makes only one request.
• Fill a retained buffer, evict an exposure, then track that exposure again and inspect the next batch.
• Exercise close while a request or retry backoff is in progress and check its timeout.
• Automate: add regression tests for HTTP 500 and exposure eviction during an outage.

Product take: More resilient event delivery matters for experiment measurements, especially during transient outages; accurate buffering and retry decisions are central to that benefit.

🧭 Assumptions & unverified claims

No unverified assumptions or claims.

May the next flush be uneventful · reviewed at ae69b84

[flagsmithsmith]

@themis-blindfold review

The bot's review (Fix before merge) had two findings. I've replied to each inline:

• Drop HTTP 500 without retrying: not changed. The released v0.9.0 scenarios require retrying a 500: A batch whose last attempt fails otherwise is kept, after server errors expects the same batch to be answered 500, 500, 503, then 202. Your earlier review also asked for this set. sdk-spec#23 says otherwise, so the issue and the scenarios disagree. I'll leave it to you whether to raise that in sdk-spec.
• Evicted exposures stay in the deduplication set: fixed in be7f8b8, with regression tests. The keys now always match the exposures in the buffer.

npm test passes (502 tests), and all 24 spec scenarios pass locally.