Skip to content

feat: Retry event delivery and flush on close - #253

Closed
flagsmithsmith[bot] wants to merge 3 commits into
mainfrom
openhands/sdk-spec-v0.8.0
Closed

flagsmithsmith[bot] wants to merge 3 commits into
mainfrom
openhands/sdk-spec-v0.8.0

Conversation

@flagsmithsmith

Copy link
Copy Markdown
Contributor

Contributes to Flagsmith/sdk-spec#23

renovate Bot and others added 2 commits November 11, 2026 13:05
Its entry in Flagsmith/sdk-spec's compatibility matrix at v0.8.0: `core or events or local-evaluation`.
@flagsmithsmith flagsmithsmith Bot added the sdk-spec Bumps of the Flagsmith SDK behavioural spec label Nov 11, 2026
@coderabbitai

coderabbitai Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

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: 4d6b5ddf-9ad7-473a-a57b-e29dbcef1c94

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

@codspeed

codspeed Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 15 untouched benchmarks


Comparing openhands/sdk-spec-v0.8.0 (0ee482d) with main (0a40fc7)

Open in CodSpeed

Implements the SDK spec v0.8.0's event delivery and client shutdown:

- Retry a 500 like other transient failures, and drop a batch whose last
  attempt got one instead of keeping it.
- Time out requests to the events API after 3 seconds,
  WithEventsRequestTimeout, rather than the client's request timeout.
- Add Client.Close, which sends buffered events, retries included, within
  its context's deadline, and stops the client's background work.
- FlushEvents only returns an error when its context ends first: failed
  sends are handled, logged and counted, never failing the application.
- Wire the spec's flush, close and numbered-events steps.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@flagsmithsmith

Copy link
Copy Markdown
Contributor Author

Implemented spec v0.8.0 in 0ee482d. CI failed because 11 @events scenarios used steps the Go SDK didn't have (flushing, closing, tracking numbered events). The SDK also didn't follow the new delivery rules.

Changes

  • A 500 from the events API is retried like the other transient failures. A batch whose last attempt got a 500 is dropped, not kept for the next flush.
  • Requests to the events API time out after 3 seconds (DefaultEventsRequestTimeout, configurable with WithEventsRequestTimeout), no longer the client's 10 second request timeout.
  • New Client.Close(ctx). It sends buffered events, retries included, and returns by the context's deadline, dropping and counting anything left. It also stops polling, realtime updates and analytics.
  • FlushEvents only returns an error when its context (or shutdown) ends it first. The spec requires that delivery never fails the application. A kept, rejected or refused batch is logged and counted in DroppedEvents, not returned as an error. This changes behaviour for callers that checked FlushEvents' error for delivery failures.
  • sdk-spec/steps.go wires the three new steps to FlushEvents, Close and TrackEvent.
  • Unit tests cover the 500 rule, the timeout and its option, Close (retries, deadline, tracking after close, polling stopped), and the new FlushEvents contract. README updated.

Locally: go test -race ./... passes, golangci-lint v2.11.4 reports 0 issues, and the spec passes all 24 scenarios with core or events or local-evaluation. The exception is TestEngine, which I couldn't run: its test-data submodule needs SSH access, and this change doesn't touch it.

Review effort: 3/5

@flagsmithsmith
flagsmithsmith Bot marked this pull request as ready for review November 11, 2026 14:37
@flagsmithsmith
flagsmithsmith Bot requested a review from a team as a code owner November 11, 2026 14:37
@flagsmithsmith
flagsmithsmith Bot requested review from khvn26 and removed request for a team November 11, 2026 14:37
@flagsmithsmith

Copy link
Copy Markdown
Contributor Author

Superseded by #254, which carries this pull request's work over to the SDK spec's latest version.

@flagsmithsmith flagsmithsmith Bot closed this Nov 11, 2026
@flagsmithsmith
flagsmithsmith Bot deleted the openhands/sdk-spec-v0.8.0 branch November 11, 2026 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sdk-spec Bumps of the Flagsmith SDK behavioural spec

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants