feat: Retry the event push to LaunchDarkly once before giving up - #69
Merged
Conversation
The push in eventLoop got one attempt. Any failure -- a dropped connection, a rate limit, one node answering 503 -- cost the batch and a whole poll interval. It now gets the same budget the other LaunchDarkly server SDKs give it: one attempt, a one second delay, then a final attempt. The structure follows the event sender in go-sdk-events. Each attempt builds its own request, because client.Do reads the body to the end and a reused request would post nothing. Statuses are classified by isHTTPErrorRecoverable, a copy of the function of the same name in that package, so among the 4xx statuses only 400, 408 and 429 earn the retry and everything outside 4xx does. Two things are specific to the bridge. The hard stop on 401 or 403 is unchanged and spends no attempt, since a rejected SDK key does not improve on a retry. The delay waits on the bridge context as well as the clock, so a shutdown during it stops the daemon at once and abandons the retry. This is not delivery durability. Salesforce deletes the events as it hands them over, so a batch that fails both attempts is lost, and the loop now logs that plainly. The retry converts transient failures into successes inside the cycle, which is what it is for.
newTestBridge left the new field at its zero value, so a test in main_test.go that reached a recoverable push failure would retry with no delay and race the shutdown select the same way a zero poll interval does. The retry tests set their own value, so nothing today depends on this.
Both failure log lines reported the same text on a first failure and a final one, so the log did not distinguish a transient blip from lost events. The other LaunchDarkly SDKs annotate their event push failures with the outcome; this does the same. pushDisposition reports one of three things: a retry follows, the status is not retryable, or the attempts are spent. Both give-up cases name the batch as lost, because nothing sends it again -- Salesforce deleted the events as it handed them over. That annotation replaces the separate "gave up" line the loop logged after the fact. Every failing path now reports its own outcome, so the trailing line was saying a second time what the failure line already said, and the flag that drove it is gone with it. The attempt budget is now named, since "is a retry left" would otherwise be a second literal 2 sitting apart from the loop bound.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1e1b241. Configure here.
The retry sent the same batch again with no X-LaunchDarkly-Payload-ID, so LaunchDarkly had no way to recognize it. That makes the retry unsafe in the one case it cannot see: an attempt the service accepted whose response never reached the bridge. The retry cannot tell that from an attempt nobody received, so it sends again either way, and without the header those events are ingested twice. One id per batch, generated before the first attempt and repeated by the retry. A fresh id per attempt would look like a second batch and double-count exactly the same way, so generating it outside the loop is the fix rather than an incidental detail. This matches the reference Go SDK's event sender, which builds the header once per payload and reuses it across both attempts. Both halves of the property are pinned: the retry repeats its batch's id, and two batches get different ids -- a second batch reusing the first's id would be discarded as a duplicate. Verified against no header at all and against a fresh id per attempt. Reported by Cursor Bugbot on the pull request.
The retry loop kept the fixed string this site used before, which discards what http.NewRequest said about the URI. #71 replaces that same string on main, and this branch rewrote the surrounding block, so a merge would have put the two in conflict with the fixed string as one of the candidate resolutions. Carrying the change here instead makes the merge order stop mattering: both sides now agree, so nothing is left for a resolution to drop.
| return "not retryable, this batch is lost" | ||
| } | ||
|
|
||
| if attempt < EVENT_PUSH_ATTEMPTS-1 { |
There was a problem hiding this comment.
Consider making attempt counting start a 1 so this becomes attempt < MAX_EVENT_PUSH_ATTEMPTS.
Member
Author
There was a problem hiding this comment.
Done in d35e50d, and it pairs with the rename nicely.
Attempts now count from 1, so the loop is attempt <= MAX_EVENT_PUSH_ATTEMPTS and pushDisposition's retry test is attempt < MAX_EVENT_PUSH_ATTEMPTS. Both read directly against the maximum. Previously the retry test had to be attempt < EVENT_PUSH_ATTEMPTS-1, so the two sites disagreed about what the constant meant and one carried a -1 correction -- which is what made this worth doing rather than a style preference.
Behavior is unchanged: two attempts, delay before the second only. The retry tests assert exact attempt counts, so a budget altered by the reindexing would have failed them. Green on Go 1.15 (CI) and 1.26, and under -race.
| // EVENT_PUSH_ATTEMPTS is how many times eventLoop sends one batch of events before | ||
| // it abandons them. Two means the original attempt and one retry, matching the | ||
| // other LaunchDarkly server SDKs. | ||
| EVENT_PUSH_ATTEMPTS = 2 |
Member
Author
There was a problem hiding this comment.
Renamed to MAX_EVENT_PUSH_ATTEMPTS in d35e50d.
tanderson-ld
self-requested a review
August 31, 2026 18:09
tanderson-ld
approved these changes
Aug 31, 2026
Review feedback. EVENT_PUSH_ATTEMPTS becomes MAX_EVENT_PUSH_ATTEMPTS, and attempts count from 1 rather than 0. The rename and the reindexing go together. With 0-based attempts, "is a retry left" had to be written attempt < EVENT_PUSH_ATTEMPTS-1, so the loop bound and the retry test disagreed about what the constant meant and one of them carried a correction. Counting from 1 makes the loop attempt <= MAX_EVENT_PUSH_ATTEMPTS and the retry test attempt < MAX_EVENT_PUSH_ATTEMPTS, both read directly against the maximum. Behavior is unchanged: two attempts, delay before the second only. The tests assert exact attempt counts, so a budget changed by the reindexing would fail them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The event push to LaunchDarkly got one attempt. On failure the loop logged and waited a
whole poll interval, so a dropped connection or one unhealthy node answering 503 cost a
full cycle. It now gets the same retry the other LaunchDarkly server SDKs give it: two
attempts one second apart, then give up. Mirrors
go-sdk-events/event_sender.go.Details that carry the behavior:
bytes.NewReader(pollBytes).client.Doreads the body to the end, so reusing one request would send the events once and an
empty body after that.
selecton the shutdown context, not atime.Sleep, so Ctrl-C doesnot hang for up to a second. Shutdown abandons the retry rather than making one more
attempt.
isHTTPErrorRecoverableis a copy of the same-named function ingo-sdk-events:among 4xx only 400, 408 and 429 are recoverable, and everything outside 4xx is, so an
unexpected status costs a retry rather than a batch.
so a rejected SDK key never spends an attempt.
drainAndClose,including the error path.
Bridgefield seeded fromEVENT_PUSH_RETRY_DELAY, so tests canshorten it instead of each burning a real second.
Both failure log lines report what becomes of the batch --
will retry,not retryable, this batch is lost, orout of attempts, this batch is lost-- so afirst failure reads differently from a final one. That replaces a separate trailing
"gave up" line, which said a second time what the failure line already said.
This does not make event delivery durable.
EventREST.prepareEventsdeletes theEventData__crows before the bridge has confirmed the push, so a batch that fails bothattempts is gone and no later cycle resends it. The retry converts many transient
failures into successes inside the cycle, which is the point, but closing the durability
gap needs a two-phase drain on the Apex side and is tracked separately.
Two deliberate divergences from the reference. A non-recoverable status does not shut the
daemon down as
MustShutDowndoes there -- only 401/403 stop this process, so a 404 losesthe batch and the loop carries on. And success stays
200 || 202rather than the whole2xx range, which is pre-existing behavior left alone; worth noting that a 201 or 204 now
costs a retry before being dropped.
Each behavior was mutation-tested: a single attempt, a body hoisted out of the loop, a
time.Sleepin place of the select, classification removed, and the 401/403 stop removedeach fail only the test that covers them. Verified on Go 1.26 and on the CI-pinned Go
1.15.
Note
Overview
Event push to LaunchDarkly now gets two attempts (original plus one retry after
EVENT_PUSH_RETRY_DELAY, default 1s), aligned with other LaunchDarkly server SDKs, instead of failing once and waiting a full poll interval.Each batch gets a per-batch
X-LaunchDarkly-Payload-IDreused on the retry so LaunchDarkly can dedupe when the first attempt succeeded but the response never arrived. Each attempt builds a new POST withbytes.NewReader(pollBytes)so retries do not send an empty body afterclient.Doconsumes the first request’s body.Retries use
isHTTPErrorRecoverable(same rules asgo-sdk-events): transient/network-style failures and selected 4xx (400, 408, 429) retry; other 4xx stop after one attempt. 401/403 still stop the daemon with no retry. The inter-attempt wait is a context-awareselect, so shutdown during the delay exits without hanging or making another attempt.Failure logs now include
pushDispositiontext (will retry,not retryable, this batch is lost,out of attempts, this batch is lost). Delivery is still not durable—Salesforce already deleted the batch when the bridge pushes—so two failed attempts still mean lost events.Adds
bridge/event_push_retry_test.go(integration-style tests througheventLoop) and sets a shorteventPushRetryDelayinnewTestBridgefor existing tests.Reviewed by Cursor Bugbot for commit d35e50d. Bugbot is set up for automated code reviews on this repo. Configure here.