Skip to content

feat: Keep retrying a rejected auto-configuration key - #866

Draft
keelerm84 wants to merge 2 commits into
mk/SDK-3063/big-segment-retryfrom
mk/SDK-3061/autoconfig-retry
Draft

feat: Keep retrying a rejected auto-configuration key#866
keelerm84 wants to merge 2 commits into
mk/SDK-3063/big-segment-retryfrom
mk/SDK-3061/autoconfig-retry

Conversation

@keelerm84

Copy link
Copy Markdown
Member

Summary

Stacked on #857. The base is mk/SDK-3063/big-segment-retry, not v8, so the diff shows only this change rather than the internal/retry package underneath it. GitHub will retarget this to v8 automatically when #857 merges.

A 401 or 403 on the auto-configuration stream stopped the stream and exited the process. Every Relay Proxy instance in a fleet did that at once, and recovery required an operator to restart each one, even when a persistent cache held a configuration those instances could have served from.

The stream now classifies each failure and keeps retrying:

  • A rejected key (401, 403, other 4xx, TLS or certificate validation) activates a second eventsource retry profile: 5 minutes growing to a 1 hour ceiling, so the stream recovers on its own once the key is valid again without adding load to a service that is rejecting every request.
  • Ordinary failures (400, 408, 429, any 5xx, network errors) keep the delays they already had, so an outage behaves exactly as before. A 500 is byte-for-byte unchanged: same warn log, same CloseNow: false, no profile activation.

This required moving off the single-regime UseBackoff/UseJitter options onto retry profiles. The normal profile keeps the existing constants, which already matched the RETRY spec's illustrative values. The profile API is available because #856 brought eventsource to v1.14.0.

What happens to SDK requests

Relay cannot serve a request until it knows its environments, so the answer depends on whether it has a configuration:

  • Cached configuration available: Relay serves those environments and keeps trying the key indefinitely. applyCachedContent already reaches ReceivedAllEnvironments, so this path was fully wired; nothing here changes it.
  • No configuration from any source: Relay reports the failure and lets the process exit, surfacing a bad key to whatever supervises it.

Relay decides that as soon as it knows both facts, rather than after a fixed delay. The cache goroutine closes its channel when the read completes, so a closed channel is the "nothing cached" answer and no waiting is needed to learn it. initTimeout bounds the wait only for the case where the read never completes, which matters because neither cache store sets its own deadline. Setting ignoreConnectionErrors keeps Relay running and retrying instead, which is what that option's documentation already promises.

No new configuration surface: initTimeout and ignoreConnectionErrors are existing documented options, and neither was previously plumbed into the auto-config path.

Worth weighing during review

With no cache configured, Relay now learns immediately that it cannot serve, so it exits before any retry happens. That is effectively the previous behavior, which means the retry-and-recover improvement lands only for deployments with a persistent cache, or with ignoreConnectionErrors set. An earlier revision of this branch used an invented one-minute grace period to give an uncached Relay a recovery window; that was removed because the first extended retry is 2.5-5 minutes away, so the grace period expired before Relay ever retried and bought exactly zero additional attempts.

I believe exiting promptly is right — an uncached Relay has nothing to serve, so letting the supervisor restart it is more useful than holding a process that answers 503, and each restart retries anyway. But it does mean the RETRY spec's motivating scenario, a transient 401 during credential propagation, still costs an uncached deployment a restart cycle.

On the tests

Mutation testing found that removing the profile activation left every auto-configuration test green: they proved the stream keeps retrying, but nothing proved it retries slowly, which is the point of the change. TestUnauthorizedEngagesTheExtendedDelays closes that. eventsource applies the returned profile before computing the delay and then logs it, so the first log line after a rejection already reflects the extended delays and the test never waits the delay out.

TestNoReconnectAfterUnrecoverableHTTPError described the behavior being removed and is replaced by four tests: the stream recovers once the key works, Relay gives up with nothing cached, Relay stays up when the cache supplies a configuration, and ignoreConnectionErrors keeps it running with neither. I also confirmed that removing the failure notification, the option guard, or the cache result each fails a different test.

subscribe exceeded the cyclomatic limit once the classification was added, so the error handler moved to newStreamErrorHandler and classifyAndLogStreamError. That also let the SubscriptionError check become errors.As, so a wrapped error now classifies correctly.

Not addressed

Event forwarding still latches off permanently on a 401 (SDK-3067), and Relay's SDK-facing auth gate still keys on the SDK client's construction error rather than on data availability. Both are tracked under the same epic.

A 401 or 403 on the auto-configuration stream stopped the stream and
exited the process. Every Relay Proxy instance in a fleet did this at
once, and recovery needed an operator to restart each one, even when a
persistent cache held a configuration those instances could have served
from.

The stream now classifies each failure and keeps retrying. A rejected
key moves it to delays that grow from five minutes to one hour, so it
recovers on its own once the key becomes valid again without adding load
to a service that is rejecting every request. Ordinary failures keep the
delays they already had, so an outage behaves as it did before.

What happens to SDK requests depends on whether Relay has a
configuration. With cached configuration it serves those environments
and keeps trying the key indefinitely. With no configuration it can
serve nothing, so it reports the failure and lets the process exit,
which surfaces a bad key to whatever supervises Relay.

Relay decides that as soon as it knows both facts: the key was rejected,
and the cache read finished without data. The cache goroutine closes its
channel when it completes, so no waiting is needed to learn there is
nothing cached. Neither cache store sets its own deadline, so initTimeout
bounds the wait in case the read does not complete. Setting
ignoreConnectionErrors keeps Relay running and retrying instead of
reporting the failure, which is what that option already promises.

Move the stream off the single-regime eventsource backoff options onto
retry profiles, which is what allows the second set of delays. The normal
profile keeps the previous values, which already matched the retry
specification.

TestNoReconnectAfterUnrecoverableHTTPError described the old behavior and
is replaced by four tests: the stream recovers once the key works, Relay
gives up when nothing is cached, Relay stays up when the cache supplies a
configuration, and ignoreConnectionErrors keeps it running with neither.
Applying the big segment review's method to this change found the same
kind of gap. Removing the profile activation from the error handler left
every auto-configuration test green: they proved the stream keeps
retrying, but nothing proved it retries slowly, which is the reason for
the change.

eventsource computes the delay after applying the profile the error
handler returns, and logs it, so the first log line after a rejected key
already reflects the extended delays. The test reads that line and
returns, so it never waits the delay out.

Verified that the test fails when the activation is removed, and that
removing the notification the error handler sends, the option guard, and
the cache result each fail a different test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant