feat: Back off big segment synchronization on a rejected SDK key

[keelerm84]

Summary

The big segment synchronizer retried every failure on a flat ten-second interval, with no backoff, no jitter, and no attempt counter. A service rejecting every request therefore saw each Relay Proxy instance return roughly six times a minute, indefinitely, and a fleet scaled that load in step. That is the retry storm LaunchDarkly's RETRY specification exists to prevent, and RETRY lists relay-proxy in its applies-to.

Adds internal/retry, which implements the RETRY backoff algorithm for long-running components, and uses it in the synchronizer:

• A transient failure (400, 408, 429, any 5xx, ordinary network errors) keeps short delays, growing 10s -> 20s -> 30s and clamping at 30s.
• A failure unlikely to correct itself soon (401, 403, other 4xx, TLS or certificate validation) moves to delays growing 5m -> 10m -> ... -> 1h and clamping at 1h.
• Both curves subtract jitter of up to half the computed delay, because a server that closes many connections at once would otherwise make every instance retry in the same instant.
• Once the extended ceiling is engaged it stays engaged until a reset, so a service alternating between a 401 and a 503 does not fall back to the fast curve.

The synchronizer still never stops. An operator can make a rejected key valid again without Relay knowing, so it keeps trying and returns to the short delays after 60 seconds of continuously marking the store as synchronized. That signal is deliberately stronger than "the last request returned 200" -- marking the store is what this component exists to do, and it is what readers depend on.

internal/retry is shared on purpose: the auto-config stream (SDK-3061) and event forwarding (SDK-3067) need the same classification, and each will bind its own timings.

Please look closely at the deleted branch

This removes isHTTPErrorRecoverable and the branch that called it. That branch has never executed. poll and connectStream return &httpStatusError{...}, while the supervisor tested for a value-typed httpStatusError:

if statusError, ok := err.(httpStatusError); ok {   // never true for *httpStatusError
    if !isHTTPErrorRecoverable(statusError.statusCode) {
        return
    }
}

I verified the assertion misses at runtime rather than only by reading, and confirmed the mismatch is present in the revision that introduced the code in 7.0.0, so there is no release in which it worked. No test covered it. Deleting it therefore changes no observed behavior -- big segment sync already retried a 401 forever, just at a punishing cadence.

Its intent was sound: the comment said such a status "should not make us permanently stop sending requests," and stopping was one way to avoid hammering an invalid key. The extended curve is a better expression of the same intent, since it bounds the load without needing a process restart to recover. So this is not discarding a feature; it is implementing one for the first time.

The v9 line carries the identical defect and will need the same fix.

Other fixes carried along

syncSupervisor called defer timer.Stop() inside its for loop, so deferred stops accumulated for the life of the goroutine. The permanent-stop path also returned without closing segmentUpdatesChan, which only the shutdown path does; removing the path makes that moot.

Operator-visible change

Big segment data will report as potentiallyStale during an extended backoff, and a deployment running bigSegmentsStaleAsDegraded will see Relay report degraded during an auth outage. That reporting already exists, is driven by LastSynchronizedOn against bigSegmentsStaleThreshold (default 5 minutes) rather than by the cause, and so keeps a long wait visible. I believe reporting degraded is correct here, but it is a behavior change and belongs in the release notes.

Not addressed

Relay's auth gate keying on the SDK client's construction error, the /status payload omitting the HTTP status code, and the two remaining components with pre-RETRY handling are tracked separately under the same epic.

---

Note

Overview
Adds internal/retry, a shared RETRY-spec backoff helper (exponential delays, jitter, normal vs extended curves, health-based reset), and wires it into the big segment synchronizer instead of a fixed ~10s retry.

Transient failures (5xx, most network issues, 400/408/429) back off on a short curve (10s → 30s cap). Unexpected failures (e.g. 401/403, TLS cert errors) switch to a long curve (5m → 1h cap) while still retrying forever so a fixed SDK key can recover without restart. Successful setSynced marks drive return to normal delays after 60s of health. Logs now include “Will retry in …” with classified failure levels.

Removes isHTTPErrorRecoverable and the supervisor branch that was meant to stop on unrecoverable 4xx but never ran (*httpStatusError vs value-type assert). Fixes defer timer.Stop() accumulating in the retry loop.

New unit and integration tests cover classification, backoff curves, 401 vs 503, reset after recovery, and stream-end doubling.

^{Reviewed by Cursor Bugbot for commit f7fce0b. Bugbot is set up for automated code reviews on this repo. Configure here.}

[kinyoklion]

🤖 Review written by Claude (AI assistant)

Disclaimer: these comments were generated by Claude on behalf of the reviewer. They propose additional test coverage for behavior this PR already implements correctly. None of them claims to have found a defect in the code; the "request changes" status is only so the suggestions get a look before merge.

Method. On a copy of the PR head (41a668f) I deleted individual lines of the new retry wiring one at a time and ran ./internal/retry/... and ./internal/bigsegments/.... Three deletions leave both packages green, and one existing test fails as a package timeout rather than an assertion if the behavior it pins regresses. Each inline suggestion addresses one of those.

Suggestion	File	Covers
fastRetryStrategy(options ...retry.Option) + fastRetryConfig()	sync_test.go	prerequisite: no-jitter / injected clock for exact waits
Drain the recorder while waiting	sync_test.go, TestSyncKeepsRetryingAfterUnauthorized	fails in 1 s instead of hanging on regression
TestSyncReturnsToNormalDelaysAfterHealthyPeriod, TestSyncBacksOffExponentiallyAcrossStreamEnds	sync_test.go	OnHealthy() in setSynced; nil-branch OnFailure(retry.Normal)
TestResetRestoresTheNormalCeiling	retry_test.go	maxDelay lowered on reset

Verified with all four applied to the PR head: gofmt -l and go vet clean; both packages pass under -race; the new and modified tests pass 10× under -race; each new test fails on its corresponding deletion and passes on the PR as written. Apply the first suggestion before the third; the others are independent.

[kinyoklion]

Written by Claude (AI assistant) for additional test coverage; not a claimed defect in the code.

Prerequisite for the tests suggested at the end of this file: let fastRetryStrategy accept retry.Options so a test can remove jitter (to assert an exact Will retry in duration) or inject a clock, and expose the config so a test can read ResetThreshold. Existing call sites are unchanged.

Suggested change

	// fastRetryStrategy scales this component's retry binding down so a test does not wait out
	// real delays. The proportions are kept so the curve still doubles and clamps.
	func fastRetryStrategy() *retry.Strategy {
	return retry.NewStrategy(retry.Config{
	InitialDelay: time.Millisecond,
	NormalCeiling: 3 * time.Millisecond,
	ExtendedInitialDelay: 30 * time.Millisecond,
	ExtendedCeiling: 360 * time.Millisecond,
	ResetThreshold: 6 * time.Millisecond,
	})
	}
	// fastRetryStrategy scales this component's retry binding down so a test does not wait out
	// real delays. The proportions are kept so the curve still doubles and clamps. Options let a
	// test remove jitter or supply a clock when it needs to assert an exact wait.
	func fastRetryStrategy(options ...retry.Option) *retry.Strategy {
	return retry.NewStrategy(fastRetryConfig(), options...)
	}
	// fastRetryConfig is the scaled-down binding behind fastRetryStrategy.
	func fastRetryConfig() retry.Config {
	return retry.Config{
	InitialDelay: time.Millisecond,
	NormalCeiling: 3 * time.Millisecond,
	ExtendedInitialDelay: 30 * time.Millisecond,
	ExtendedCeiling: 360 * time.Millisecond,
	ResetThreshold: 6 * time.Millisecond,
	}
	}

[kinyoklion]

Written by Claude (AI assistant) for additional test coverage; not a claimed defect in the code.

This is about how the test fails, not what it asserts. If the behavior it pins ever regressed (the synchronizer staying on the normal 1–3 ms delays), it would issue several hundred polls during the one-second Eventually. RecordingHandler's channel has 100 slots, so the handler blocks, the request never completes, and WithServer's deferred Close waits on it: the package then fails at the 10-minute timeout instead of here. I reproduced that by temporarily reintroducing the value-typed errors.As target: the test as written timed out; with the loop below it failed in 1.0 s with the intended message. On the PR as written the log line is already present after the first 401, so the loop body does not run.

Suggested change

	// Three attempts show it did not give up after the first rejection.
	for i := 1; i <= 3; i++ {
	helpers.RequireValue(t, requestsCh, time.Second, "expected poll attempt %d", i)
	}
	require.Eventually(t, func() bool {
	return hasLogMessage(mockLog, ldlog.Info, "engaging extended backoff")
	}, time.Second, 10*time.Millisecond, "expected the extended delays to be engaged")
	// Three attempts show it did not give up after the first rejection.
	for i := 1; i <= 3; i++ {
	helpers.RequireValue(t, requestsCh, time.Second, "expected poll attempt %d", i)
	}
	// Keep reading the recorder while waiting for the log line. If a regression left the
	// synchronizer on the normal delays it would poll every few milliseconds; an unread
	// recorder channel (100 slots) then fills, the handler blocks, and the server's Close
	// hangs the package for the full test timeout instead of failing here in one second.
	deadline := time.After(time.Second)
	for !hasLogMessage(mockLog, ldlog.Info, "engaging extended backoff") {
	select {
	case <-requestsCh:
	case <-deadline:
	require.FailNow(t, "expected the extended delays to be engaged")
	}
	}

[kinyoklion]

Written by Claude (AI assistant) for additional test coverage; not a claimed defect in the code.

Two integration tests for the parts of the retry wiring that the current tests observe only through log substrings:

1. the reset path — setSynced → OnHealthy returning the synchronizer to the normal delays after a recovered 401;
2. the nil-return path — a stream end counting as a normal failure, so consecutive drops double the wait and clamp at the normal ceiling.

Neither currently has a test that fails if the corresponding call in sync.go is removed (I checked by deleting each and running both packages: all green). Both tests pass on this PR (10× under -race) and fail on those deletions with Will retry in 60ms instead of 1ms, and 1ms, 1ms, 1ms instead of 1ms, 2ms, 3ms. Requires the fastRetryStrategy(options...) change suggested above; the helpers are included so the block is self-contained (sync and strings are already imported).

Suggested change

	}
	}
	// zeroJitter removes jitter so a test can assert an exact "Will retry in" duration.
	func zeroJitter() retry.Option {
	return retry.WithJitter(func(time.Duration) time.Duration { return 0 })
	}
	// steppingClock advances by a fixed step on every read. The supervisor goroutine reads it
	// through the retry strategy while the test goroutine drives the servers, so consecutive
	// health marks land exactly one step apart regardless of scheduling.
	type steppingClock struct {
	mu sync.Mutex
	t time.Time
	step time.Duration
	}
	func (c *steppingClock) now() time.Time {
	c.mu.Lock()
	defer c.mu.Unlock()
	r := c.t
	c.t = c.t.Add(c.step)
	return r
	}
	// retryLines returns the "Will retry in ..." warnings in the order they were logged.
	func retryLines(mockLog *ldlogtest.MockLog) []string {
	var out []string
	for _, line := range mockLog.GetOutput(ldlog.Warn) {
	if strings.Contains(line, "Will retry in") {
	out = append(out, line)
	}
	}
	return out
	}
	// drainUpdates consumes the updates channel so notifySegmentsUpdated never blocks the
	// supervisor while a test is only interested in the retry timing.
	func drainUpdates(ch <-chan UpdatesSummary) {
	go func() {
	for range ch {
	}
	}()
	}
	func TestSyncReturnsToNormalDelaysAfterHealthyPeriod(t *testing.T) {
	// After a rejected key is made valid again, ResetThreshold of continuous healthy operation
	// (setSynced succeeding) must return the synchronizer to the normal delays. This pins the
	// OnHealthy call in setSynced: without it the extended ceiling would persist for the life
	// of the process.
	mockLog := ldlogtest.NewMockLog()
	mockLog.Loggers.SetMinLevel(ldlog.Debug)
	defer mockLog.DumpIfTestFailed(t)
	patch1 := newPatchBuilder("segment.g1", "1", "").build()
	pollHandler, requestsCh := httphelpers.RecordingHandler(
	httphelpers.SequentialHandler(
	httphelpers.HandlerWithStatus(401), // poll 1: rejected key, extended regime engaged
	httphelpers.HandlerWithJSONResponse([]bigSegmentPatch{}, nil), // poll 2: key valid again
	httphelpers.HandlerWithJSONResponse([]bigSegmentPatch{}, nil), // poll 3: with the stream open; setSynced follows
	),
	)
	sseHandler, sseControl := httphelpers.SSEHandler(nil)
	streamHandler, streamRequestsCh := httphelpers.RecordingHandler(sseHandler)
	// Each read of the clock advances it by one ResetThreshold, so the second health mark is
	// exactly ResetThreshold after the first.
	clock := &steppingClock{t: time.Now(), step: fastRetryConfig().ResetThreshold}
	httphelpers.WithServer(pollHandler, func(pollServer *httptest.Server) {
	httphelpers.WithServer(streamHandler, func(streamServer *httptest.Server) {
	storeMock := newBigSegmentStoreMock()
	defer storeMock.Close()
	segmentSync := newDefaultBigSegmentSynchronizer(sharedtest.MakeBasicHTTPConfig(), storeMock,
	pollServer.URL, streamServer.URL, config.EnvironmentID("env-xyz"), testSDKKey, mockLog.Loggers, "")
	segmentSync.retryStrategy = fastRetryStrategy(zeroJitter(), retry.WithClock(clock.now))
	defer segmentSync.Close()
	segmentSync.Start()
	drainUpdates(segmentSync.SegmentUpdatesCh())
	helpers.RequireValue(t, requestsCh, time.Second, "expected poll attempt 1 (401)")
	helpers.RequireValue(t, requestsCh, time.Second, "expected poll attempt 2")
	helpers.RequireValue(t, requestsCh, time.Second, "expected poll attempt 3")
	helpers.RequireValue(t, streamRequestsCh, time.Second, "expected the stream request")
	helpers.RequireValue(t, storeMock.syncTimeCh, time.Second, "expected the first health mark")
	// A stream event marks the store again, one ResetThreshold later on the stepping clock.
	sseControl.Enqueue(*makePatchEvent(patch1))
	requirePatch(t, storeMock, patch1)
	helpers.RequireValue(t, storeMock.syncTimeCh, time.Second, "expected the second health mark")
	// The upstream now drops the healthy stream: an ordinary failure on the normal curve.
	sseControl.EndAll()
	require.Eventually(t, func() bool { return len(retryLines(mockLog)) >= 2 },
	time.Second, time.Millisecond)
	lines := retryLines(mockLog)
	assert.Equal(t, "BigSegmentSynchronizer: Will retry in 30ms", lines[0],
	"the first wait is the extended base delay")
	assert.Equal(t, "BigSegmentSynchronizer: Will retry in 1ms", lines[1],
	"after ResetThreshold of health the next wait is the normal initial delay")
	})
	})
	}
	func TestSyncBacksOffExponentiallyAcrossStreamEnds(t *testing.T) {
	// A stream that ends is reported to the supervisor as a nil error. It must still count as
	// a normal failure, so consecutive stream ends double the wait and clamp at the normal
	// ceiling rather than retrying on a flat interval.
	mockLog := ldlogtest.NewMockLog()
	mockLog.Loggers.SetMinLevel(ldlog.Debug)
	defer mockLog.DumpIfTestFailed(t)
	pollHandler := httphelpers.HandlerWithJSONResponse([]bigSegmentPatch{}, nil)
	sseHandler, sseControl := httphelpers.SSEHandler(nil)
	streamHandler, streamRequestsCh := httphelpers.RecordingHandler(sseHandler)
	httphelpers.WithServer(pollHandler, func(pollServer *httptest.Server) {
	httphelpers.WithServer(streamHandler, func(streamServer *httptest.Server) {
	storeMock := newBigSegmentStoreMock()
	defer storeMock.Close()
	segmentSync := newDefaultBigSegmentSynchronizer(sharedtest.MakeBasicHTTPConfig(), storeMock,
	pollServer.URL, streamServer.URL, config.EnvironmentID("env-xyz"), testSDKKey, mockLog.Loggers, "")
	// Each cycle produces exactly one health mark before the stream ends, and a failure
	// ends the healthy period, so no reset can interfere with the doubling.
	segmentSync.retryStrategy = fastRetryStrategy(zeroJitter())
	defer segmentSync.Close()
	segmentSync.Start()
	drainUpdates(segmentSync.SegmentUpdatesCh())
	for i := 1; i <= 3; i++ {
	helpers.RequireValue(t, streamRequestsCh, time.Second, "expected stream connection %d", i)
	helpers.RequireValue(t, storeMock.syncTimeCh, time.Second, "expected the store to be marked synchronized")
	sseControl.EndAll()
	}
	require.Eventually(t, func() bool { return len(retryLines(mockLog)) >= 3 },
	time.Second, time.Millisecond)
	assert.Equal(t, []string{
	"BigSegmentSynchronizer: Will retry in 1ms",
	"BigSegmentSynchronizer: Will retry in 2ms",
	"BigSegmentSynchronizer: Will retry in 3ms", // clamped at NormalCeiling
	}, retryLines(mockLog)[:3])
	})
	})
	}

[kinyoklion]

Written by Claude (AI assistant) for additional test coverage; not a claimed defect in the code.

TestHealthyOperationResetsOnlyAfterTheThreshold checks that a reset restores the base delay, but nothing checks that it also lowers maxDelay: removing s.maxDelay = s.cfg.NormalCeiling from OnHealthy leaves both packages green. This pins the post-reset ceiling (fails with 32s instead of 4s on that deletion; passes on the PR as written).

Suggested change

	}
	}
	func TestResetRestoresTheNormalCeiling(t *testing.T) {
	// A reset must lower maxDelay back to the normal ceiling, not only restore the base delay.
	// Otherwise the normal curve after a recovery would keep doubling up to the extended
	// ceiling instead of clamping at NormalCeiling.
	clock := &fakeClock{t: time.Now()}
	s := NewStrategy(testConfig(), noJitter(), WithClock(clock.now))
	require.True(t, s.OnFailure(Unexpected))
	s.OnHealthy()
	clock.advance(30 * time.Second)
	s.OnHealthy()
	require.False(t, s.InExtendedRegime())
	for range 6 {
	s.OnFailure(Normal)
	}
	assert.Equal(t, 4*time.Second, s.NextWait(), "after a reset the normal ceiling clamps the curve again")
	}

[kinyoklion]

Written by Claude (AI assistant); coverage note only, not a claimed defect in the code.

The call on this line is correct. Noting only that no current test fails if it is removed (both packages stay green). TestSyncReturnsToNormalDelaysAfterHealthyPeriod, suggested in sync_test.go, covers it.

[kinyoklion]

Written by Claude (AI assistant); coverage note only, not a claimed defect in the code.

Same coverage note: this branch is right, but deleting it leaves both packages' tests green, because TestSyncRetryIfStreamFails matches Will retry in \S+ and never observes a second wait. TestSyncBacksOffExponentiallyAcrossStreamEnds, suggested in sync_test.go, pins the doubling.

[kinyoklion]

The suggested edits were done as requested changes, but I approve, and leave them if you are interested in additional test coverage.