feat: Keep retrying a rejected auto-configuration key

[keelerm84]

Summary

Stacked on #881, which adds the internal/retry package this uses. Review that one first; GitHub will retarget this to v9 when #881 merges.

A 401 or 403 on the auto-configuration stream stopped the stream and reported a fatal error through the ready channel, which makes ld-relay.go call os.Exit(1). An operator can make the key valid again without Relay knowing, so recovery took a process restart -- and a persistent store holding perfectly usable configuration was thrown away along with the process.

The stream now keeps retrying in every case. A failure unlikely to correct itself soon activates a second eventsource retry profile at a 5 minute base with a one hour ceiling, which bounds how much load a fleet of Relay Proxy instances puts on a service that is rejecting all of its requests. The classification comes from internal/retry, so the auto-config stream and the big segment synchronizer sort failures the same way rather than each inventing a policy.

Status reporting follows: a rejected key reports INTERRUPTED, because the stream is still trying. OFF is now reserved for Close. A stream that never connected stays INITIALIZING, matching the SDK data source status.

Close cancels the stream context in addition to closing halt. halt is only observed when the next attempt fails, so on its own it cannot end a backoff wait that is already pending -- without the cancel, Close would block on s.done for the remainder of a delay that now reaches an hour. TestCloseInterruptsAPendingBackoffWait asserts the goroutines are gone, and TestNoRequestIsMadeAfterClose asserts the same thing in terms of observable traffic.

Behavior change worth noticing

Relay no longer exits when LaunchDarkly rejects the auto-configuration key. It serves from the persistent cache if one holds data, and answers 503 otherwise -- which is exactly what it already did for every other failure to reach LaunchDarkly. If anything relies on the process exiting to detect a bad key, it should read the status resource instead. docs/proxy-mode.md documents this, including how to tell a rejected key apart from an unreachable LaunchDarkly service.

Notes for review

Three existing tests pinned the old stop-on-rejection behavior, so they are rewritten rather than renamed: TestNoReconnectAfterUnrecoverableHTTPError is now TestRecoversAfterUnrecoverableHTTPError, and the two ...IsOff... status tests now assert the retrying states. One of them had a SequentialHandler whose second response succeeds; it now rejects every attempt, since recovery has its own test.

streamErrorInfo deliberately omits the Message field that the v8 version sets. api.ConnectionErrorRep has no Message field on v9, so the status endpoint cannot render it and setting it would be dead data. That omission is from #859 and is left alone.

stream_manager_known_limits_test.go pins an eventsource limitation rather than a Relay one: a server-sent retry: hint replaces the active profile's base delay, including the extended profile's, and is never cleared. So after a hint, engaging the extended profile does not slow retries to the intended 5 minutes. This is tolerated, not fixed; the test exists so a future library fix shows up as a failure somebody updates on purpose.

Relationship to v8

Brings v9 to parity with v8 #866. v9 had deleted errors_and_messages.go in favor of structured slog calls, so the message constants are inlined at their call sites rather than reintroduced.

---

Note

Overview
Relay no longer exits when LaunchDarkly rejects the auto-configuration key (401/403). The auto-config stream keeps retrying like other upstream failures; “unlikely to fix soon” errors (including a bad key) switch to an extended eventsource retry profile (5 minute base, up to 1 hour) via internal/retry classification.

While retrying, Relay can serve from persistent auto-config cache if present; otherwise SDK requests still get 503. Status reports INTERRUPTED (or INITIALIZING if never connected) with the HTTP error in lastError; OFF is reserved for Close() only.

Shutdown cancels the stream context so long backoff waits end promptly, closes the ready channel without a fatal error on graceful shutdown, and docs/proxy-mode.md documents the new behavior and how to detect a rejected key vs unreachable LaunchDarkly.

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

[cursor]

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 034cba2. Configure here.}

[kinyoklion]

Automated comment — posted by an AI code-review agent (multi-agent review of this PR), not a human reviewer.

Close() cancels the stream context, so a stream that never connected returns context.Canceled through streamCh. That branch (stream_manager.go:405-413) has no halt check before signalReady, and the consumer goroutine in relay/relay.go:228-238 calls os.Exit(1) when it receives a non-nil error — so a clean shutdown of a never-connected stream (a rejected key) exits 1. Whether the streamCh or halt select arm wins is decided per-run, so it reproduces only sometimes.

This test pins the intended contract: after Close() in the first-connection retry state, the ready channel must close with a zero value, never deliver a fatal error. It fails 3/3 against the current head by design — it requires the companion change in subscribe (a non-blocking halt check before the signalReady in the streamCh error branch, plus a readyOnce-guarded close(readyCh) on the halt path). Then it passes, including under go test -race.

Suggested change

	// The same guarantee stated in terms of observable behavior: nothing reaches LaunchDarkly after
	// Close() returns. A short extended delay is used so a surviving wait would actually fire.
	// Close() while the stream is stuck in its first-connection retry loop must not report the
	// shutdown's own context cancellation as a fatal stream failure. Eventsource returns that
	// cancellation without consulting the error handler, so if it reaches the ready channel here
	// it makes ld-relay.go call os.Exit(1) in the middle of a graceful shutdown. Close() must
	// instead close the ready channel, so whoever waits on it unblocks without an error.
	func TestCloseWhileRetryingDoesNotSignalAFatalStreamError(t *testing.T) {
	sm, mockLog, _, closeServer := newRejectingStreamManager(t, 5*time.Minute)
	defer closeServer()
	readyCh := sm.Start()
	require.Eventually(t, func() bool {
	return mockLog.HasMessage(slog.LevelError, "invalid auto-configuration key")
	}, 2*time.Second, 10*time.Millisecond, "expected the first rejection")
	sm.Close()
	select {
	case err := <-readyCh:
	assert.NoError(t, err, "the shutdown's context cancellation must not be reported as a fatal stream error")
	case <-time.After(2 * time.Second):
	t.Fatal("the ready channel was not closed after Close() returned")
	}
	}
	// The same guarantee stated in terms of observable behavior: nothing reaches LaunchDarkly after
	// Close() returns. A short extended delay is used so a surviving wait would actually fire.

[keelerm84]

Added in 2a6e612, verbatim apart from wrapping the assert.NoError message onto a second line to stay inside the 110-column lint limit. Assertions, timings and both failure messages are unchanged.

It behaves as you predicted: it failed 5/5 against the previous head and passes 5/5 after the fix, including under -race. Every failure was the t.Fatal("the ready channel was not closed after Close() returned") branch rather than a context.Canceled delivery, so the halt arm won every time in this timing -- the streamCh arm needs the cancellation to arrive in the same scheduling window.

The companion change is the one you described, extended to cover that second arm:

• a non-blocking halt check before the signalReady in the streamCh error branch, and
• a readyOnce-guarded close(readyCh) -- a signalShutdown helper sharing the sync.Once with signalReady -- on both that path and the halt arm, which previously returned without signalling anything.

Every return path in subscribe now signals readyCh exactly once: the URL error, the shutdown-during-streamCh case, a genuine stream error, the halt arm, and the success path.

[keelerm84]

@kinyoklion thanks for confirming that one -- you were right, and it was worse than the report suggested.

Fixed in 2a6e612, and I've replied on both threads with the detail. The short version:

There were two arms of that select mishandling the cancellation, not one. Close closes halt and cancels the stream context, so both arms go ready at once and which wins is decided per-run:

• The streamCh arm is the one Bugbot reported: it passed the cancellation to signalReady, and ld-relay.go:236 exits the process on a non-nil value.
• The halt arm returned without signalling readyCh at all, so a caller still waiting for the first connection waited forever.

The second is the one that actually wins in practice. The regression test reproduced it 5 times out of 5, always via "the ready channel was not closed" and never via the exit -- the exit needs the cancellation to land in the same scheduling window, which matches Bugbot's note about the cached-environment path being likeliest.

Both arms now call a signalShutdown helper that closes readyCh under the same sync.Once as signalReady, so exactly one of the two ever touches the channel and neither can send on a closed one. Closing rather than sending nil is the point: a non-nil value on that channel means fatal, and a shutdown is not that. Every return path in subscribe now signals exactly once -- the URL error, the shutdown-during-streamCh case, a genuine stream error, the halt arm, and the success path.

I took the suggested test verbatim, apart from wrapping one assertion message to stay inside the column limit.

Also worth flagging since it changes what CI means here: this PR was previously based on mk/SDK-3157/big-segment-retry, so ci.yml's pull_request: branches: ["v9", "feat/**"] never matched and only the four repo-wide gates ran. Now that #881 has merged I rebased onto v9 (dropping the squash-merged commit with --onto), so it has run the full 15-check matrix for the first time: 15/15 green, unit tests across both Go versions and both store backends, staging integration tests, and Docker Scout.

Re-requesting your review.