Repository navigation
feat: Refuse a malformed credential payload without losing the environment - #897
Conversation
a4a5024 to
29924b8
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // The ordering is the whole point. Upsert deduplicates by version, so a rejected payload that had | ||
| // already advanced the version would make LaunchDarkly's replay of that same payload a no-op: the | ||
| // environment would keep serving credentials it should have replaced, with no path back short of | ||
| // restarting the process. Validating first leaves the version where it was, so the replay that | ||
| // follows the reconnect is applied. |
There was a problem hiding this comment.
nit: this comment is hard to follow. I think we are simply stating that we want to preserve the order so we know which cred are valided last?
d903d4b to
be4cd5a
Compare
274abb6 to
7fd2caf
Compare
7fd2caf to
d27ad4f
Compare
a967559 to
686e873
Compare
686e873 to
7a25d3d
Compare
7a25d3d to
e32b69e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ 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 e32b69e. Configure here.
…nment Phase 4 of forward-porting concurrent multi-key support from v8 (#817). A credential payload that parses but cannot produce a usable accepted set is now refused before MessageReceiver.Upsert records its version. The ordering is the point: Upsert deduplicates by version, so a refused payload that had already advanced the version would make LaunchDarkly's replay of it a no-op, leaving the environment serving credentials it should have replaced with no way back short of restarting the process. A refused environment keeps the credentials it had. The stream reconnects, because the service cannot learn that relay refused the payload and would otherwise send nothing further. Well-formed environments in the same put are applied as usual. persistPut substitutes each refused environment's last-good cached entry, so a bad payload does not drop those environments from the auto-config cache, or empty it when every environment in the put was refused. If the prior snapshot cannot be read there is no safe cache to assemble, so it is left alone. The stream also now moves to the extended retry delays after a second consecutive unusable event, rather than reconnecting on the short delays indefinitely. One bad event may be a single corruption and is worth asking again at once; a second means the reconnect brought the same payload back. This closes a finding from the review of #817. Also removes StreamManager.lastKnownEnvs, dead since #401 removed its last read.
…ecovers Activating the extended retry profile was one-way. eventsource clears an activated profile only after a stretch of healthy operation, and it measures that stretch from a timestamp stamped as the event was delivered, so the reset can never fire on a restart that an event itself caused. Nothing in relay activated the default profile either, and the stream is subscribed once per process, so the first pair of refused payloads left every later reconnect on delays of five minutes to an hour, even after the stream had been healthy in between. That also defeated the threshold's own intent, because the next isolated refusal inherited the extended curve instead of reconnecting at once. A delivered, usable event is the only proof the stream works, and it already resets the consecutive-refusal counter. Report it and drop back to the short curve at the same point. Also add the tests that would have caught this. Nothing asserted the threshold, the backoff, or the status transition, so lowering malformedBackoffThreshold to one, raising it, or deleting the profile activation entirely all left the package green.
A put that refused one environment restarted the whole stream, and a second consecutive one moved every environment onto the extended delays. Because the counter only resets on an event relay can use, and because the service replays the same put on each new connection while the healthy environments dedupe away, the escalation climbed without bound. One environment's data could stop auto-configuration for every environment in the process for up to an hour. Reconnecting only helps when the event carried nothing usable. A refused environment comes back identical on a new connection, so for a put that applied at least one environment the reconnect buys nothing and costs every healthy environment its updates while the stream is down. Report how many environments applied, and restart only when none did. A partial refusal still records the invalid-data error, so the refusal stays visible, but it leaves the state VALID: the connection is genuinely working.
The generation a handler reads when an event arrives exists so a success that event would report cannot bury a connection failure recorded while it was still being handled. The partial-refusal path wrote its status directly instead, so a put that applied some environments while the connection was dying reported VALID and discarded the real error. The recovery flag had the same gap: it was set whenever an event processed, so the stream dropped back to the short retry curve even where the generation check had correctly refused the success. Every status decision now goes through one generation-checked call, which reports whether it applied, and the recovery flag is that result. Also stop reporting the configuration as complete when a put applied nothing. Relay would declare itself fully configured while serving no environments and answer 401 for every credential, which says the credentials are wrong when the truth is that there is no configuration to serve; not reporting itself configured produces a 503 instead, which is honest and which a load balancer acts on. An empty put is left alone: it refused nothing, so it is a complete configuration of no environments.
The stream manager refuses a credential payload before the message receiver records its version, so the relay never sees a refused patch at all. That is tested where the decision is made, but nothing asserted the consequence an operator cares about: the environment stays in the status document, its existing credentials go on working, and the unusable key the patch carried does not authenticate. Driving it needs the auto-config stream after start-up, which the existing harness keeps private because most tests only need the initial configuration, so this adds a variant that hands the stream to the test.
A reviewer read "the ordering is the whole point" as ordering between credentials, meaning which of them is validated last, rather than the ordering between validation and Upsert. The sentence never said which ordering it meant, and the paragraph asked the reader to hold four things at once before naming the consequence. Split it into what the function does, the rule, and why the rule exists, and name Upsert in the first sentence of the rule.
The replay was enqueued right after the refusal, while the server could still hold the old connection. Enqueue then wrote it to that connection and it was lost, which failed the test on Go 1.27.1 in CI. Serving it as the second connection's initial event removes the race.
The cache already kept the last known good entry for an environment whose payload was refused, so the next process start served it. This process did not: the refused environment is skipped before it reaches the receiver, and a partial refusal no longer reconnects, so the service never resends it. The Relay Proxy held a serviceable configuration for an environment it answered nothing for. persistPut now returns the entries it carried forward, and the put applies them in memory as well. Recovered environments count towards the readiness gate, because they are being served.
52f5578 to
c1a730e
Compare
| // the life of the process, and a later unrelated reconnect waits minutes. | ||
| stream.ActivateProfile(normalProfile) | ||
| } | ||
| if outcome.restart { |
There was a problem hiding this comment.
There being two ifs here and no else seems like a smell. Are you guaranteed either recovered or restart is set?
There was a problem hiding this comment.
Not guaranteed, and that's deliberate: all four combinations occur. A reconnect event sets
restart without being malformed, so it also goes through the success path and earns the short
curve -- it is both proof the connection works and a request to reconnect, and it needs both
branches. An unusable event asks for a restart alone. A put or patch that applied asks for the
curve alone. An unrecognized event arriving while the connection is already failing asks for
neither, because the generation check declines. An else would drop one of the two actions on the
reconnect path.
You're right that the shape made the reader derive all that, though. I replaced the booleans with a
named curve value plus restart, so the two fields are the two decisions the loop makes, and the
state space is written down on the type. 69709dc.
| stream.ActivateProfile(extendedProfile) | ||
| } | ||
| stream.Restart() | ||
| } |
There was a problem hiding this comment.
No case where you can get backoff without restart?
There was a problem hiding this comment.
There wasn't one: backOff was only ever set in the malformed arm, and every path through that arm
also set restart, so it was a modifier on a reconnect that was already happening. 69709dc
replaced it with a single curve value, and 6730864 then removed the extended curve from the data
path entirely, so there is no backOff left to get without a restart.
|
|
||
| // gotMalformedCredentials reports a payload that parsed but whose credentials cannot produce a | ||
| // usable set. The environment keeps the credentials it already had. | ||
| gotMalformedCredentials := func(envID config.EnvironmentID, err error) { |
There was a problem hiding this comment.
I think other SDK impls are treating malformed JSON of payloads as normal errors and not engaging extended RETRY regardless of how many malformed payloads are seen. This was mostly due to precedent in existing code not treating them as unrecoverable.
Perhaps malformed credentials warrants a different handling.
There was a problem hiding this comment.
You're right, and I've removed it. I checked: v8 relay sets shouldRestart and never touches the
profile, and go-server-sdk's streaming data source activates the extended profile only in its HTTP
error handler. This PR was the odd one out.
My reasoning for it was also wrong. I thought repeated restarts never back off, because eventsource
stamps its health timestamp on every delivered event including a bad one. What actually happens is
that the stamp only suppresses the reset: retryCount keeps climbing, so the normal curve grows to
its 30s ceiling on its own and the reconnects were already bounded. What the extended curve added
was a window of up to an hour where the stream is disconnected and therefore cannot learn that the
payload was corrected.
6730864 drops malformedBackoffThreshold and the consecutive counter; the extended delays now
belong to the stream error handler alone.
| s.handlePut(putMessage.Data) | ||
| malformedEnvIDs, applied := s.handlePut(putMessage.Data) | ||
| switch { | ||
| case len(malformedEnvIDs) == 0: |
There was a problem hiding this comment.
If for some reason the put contains no envs, applied will be 0 and malformed will be 0? But isn't this possibly a situation worth restarting over? I am thinking of the category of bug where the server sends no content.
Perhaps this is hard to counter because a credential with no environments is possible? (even if unlikely). Thoughts?
There was a problem hiding this comment.
Yes, that's the behavior: an empty put takes the len(malformedEnvIDs) == 0 arm, so no restart,
Retain deletes everything relay held, and ReceivedAllEnvironments fires because nothing was
refused.
Your instinct about why it's hard to counter is the reason I left it. An auto-config key whose
policy matches no environments is a legal configuration, and its payload is identical to the one a
"server sent no content" bug would produce, so relay has nothing to discriminate on.
Restarting also costs more than it could buy here. The service would resend the same empty put, and
because eventsource stamps its health timestamp on every delivered event, the backoff never resets:
the reconnects grow to the 30s ceiling and stay there, permanently, against a configuration that is
valid. An operator with a deliberately empty policy would see a relay that never stops reconnecting.
One thing worth flagging that your comment points at: an entry whose envId disagrees with its map
key is skipped with a continue that counts as neither applied nor refused, so a put where every
entry is mismatched behaves exactly like the empty one. That shape really is only ever a defect, but
it isn't fixable by reconnecting either, for the same reason as above.
What the empty case was missing is a way to see it, so 96ef1ae logs a warning when a put removes
every environment relay was serving, naming the count and saying it now answers 401 for every
credential.
| // A read failure on the prior cache means there is no safe snapshot to assemble, so the cache is left | ||
| // as it is. A nil result with no error is an empty cache rather than a failure, and there is simply | ||
| // nothing to carry forward. | ||
| func (s *StreamManager) persistPut(content PutContent, malformedEnvIDs map[config.EnvironmentID]bool) { |
There was a problem hiding this comment.
consider a different verb than persist here unless you know for sure the destination is a persistence (maybe it is indeed and my comment is silly). apply or commit are some I can think of. Also update log below to use the same verb, it is currently using write.
There was a problem hiding this comment.
Not silly, but it is a persistence: the cache comes from autoconfigcache.NewStore in
relay/relay.go, so it's the Redis or DynamoDB store, and the reason for writing it is that the
next process start has something to serve before it reaches LaunchDarkly. write in the log line is
accurate for the same reason.
One wrinkle you couldn't have seen: the function now also returns the entries it carried forward so
the put can apply them in memory, so the name covers only half of what it does. The doc comment
spells the return value out, so I've left the name, but say the word if you'd rather see it split.
The event outcome carried restart, backOff and recovered. Two of the eight combinations could not occur: backOff only ever accompanied restart, and backOff and recovered are mutually exclusive, so the type could express a connection that was asked for both retry curves at once. A retryCurve value replaces backOff and recovered. The event loop reads as the two decisions it makes: which curve the connection uses from here, and whether to reconnect now. No behavior changes.
Repeated unusable events moved the stream to the extended delays, which diverges from v8 and from the SDK's streaming data source: both restart on whatever curve is current and reserve the extended delays for connection failures. The divergence also cost more than it bought. eventsource grows its own delay for each restart that follows an event, up to streamMaxRetryDelay, so the reconnects were already bounded. The extended delays added a window of up to an hour where the stream is disconnected and so cannot learn that the payload it refused has been corrected. The extended delays now belong to the stream error handler alone.
A configuration with no environments in it is legal, so relay applies it. It then serves nothing and answers 401 for every credential, which an operator had to infer from an environment count of zero and a run of delete lines.
| // dedupes an older cached copy away. Recording the last-good version here is correct, because it | ||
| // is what Relay is now serving, and a corrected payload carries a higher one. | ||
| recovered := 0 | ||
| if content.Persist { |
There was a problem hiding this comment.
Are we concerned of the possibility of there having been a previously malformed payload that was saved to a persistent store that would then be loaded into this relay?
Relay versions before this one wrote the auto-configuration cache without checking credentials, so a store an older relay filled can hold a payload this one refuses. Carrying such an entry forward created an environment from a payload relay had just declared unusable, and wrote the same problem back for the next process start. An entry that does not validate is now carried to neither destination.
…898) ## Summary Phase 5, the last of forward-porting concurrent multi-key support from v8 ([#817](#817)) to v9. **Stacked on [#897](#897; GitHub retargets it to `feat/concurrent-keys-v9` as the stack merges. `EnvironmentStatusRep` gains `sdkKeys` and `mobileKeys` arrays of `{ key, value, expiry? }`. Both are always present rather than omitted, so a consumer can iterate without a nil check: `sdkKeys` always holds at least the anchor, and `mobileKeys` is empty for a server-side-only environment. ### The scalar fields stay `sdkKey` and `mobileKey` remain, designating which array entry owns the connection to LaunchDarkly and which mobile key is used where only one can be. Asked and decided during review: they are deliberately *not* replaced by an `anchor: true` flag on an entry. `sdkKey` predates concurrent keys and is what consumers read to identify an environment, v8 reports the same pair of fields so one consumer can read both versions, and encoding the designation in two places would let them drift. The rationale is recorded on the field. ### `expiringSdkKey` is removed One field cannot describe an environment that accepts several keys with different expiries. The soonest-expiring-non-anchor rule phase 3 carried was only there to keep that field, and the metrics reading it, deterministic while the rest of the port landed. Per-key expiry now lives in the arrays. ### Metrics re-based The two instruments #876 derived from `ExpiringSDKKey != ""` move onto the arrays: - **per-environment** becomes `launchdarkly.relay.environment.expiring_keys`, a count of that environment's SDK keys carrying an expiry. A bit cannot say how many, and it no longer means "a rotation is in progress" — a key set can hold an expiring key as a steady state. - **relay-wide** keeps `launchdarkly.relay.environment.expiring_key.count`, still counting environments, now those serving at least one key with an expiry. Descriptions reworded in `status_measures.go` and `docs/metrics.md`. The existing metrics tests caught a name collision when the per-environment gauge was first renamed to `.expiring_key.count`, which is why the two now have clearly distinct names. ### The `expect` API was already ready No grammar change was needed. `internal/api/status_query_test.go` carried a test named "array fields the schema does not have yet", whose comment said the path grammar supports array selectors *specifically so they would survive the concurrent-keys change*. That test now describes the past, so it is split: addressing `sdkKeys` at the wrong level still returns 422, and there is new positive coverage for `sdkKeys[key=...]` and `sdkKeys[0]` resolving against real data, with a selector that matches nothing returning 412 rather than 422. ### Documentation `docs/endpoints.md` documented `expiringSdkKey=...` as a 412 example, which would now be a 422, and its three status-document examples showed only the scalar fields. All updated, plus a short paragraph on why an environment reports several keys and what the scalar fields designate. Part of SDK-3203, under epic SDK-3188. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Breaking change to `/status`:** each environment now exposes always-present **`sdkKeys`** and **`mobileKeys`** arrays (`key`, obscured `value`, optional `expiry`) while **`sdkKey`** / **`mobileKey`** still designate the anchor and primary mobile key. The singular **`expiringSdkKey`** field is **removed**; expiring credentials are read from the arrays. > > The top-level document adds **`refusedEnvironments`** (always an array, sorted by `envId`) for auto-config payloads Relay rejected, with **`serving`** distinguishing “never configured” (`401`) from “update refused but still serving prior config.” Auto-config stream handling records and clears refusals on put/patch/delete; **`expect`** probes can target the new arrays and `refusedEnvironments` (including the inverted alert recipe in the docs). > > **Metrics:** per-environment **`launchdarkly.relay.environment.expiring_keys`** replaces the boolean **`expiring_key`** with a count of SDK keys that have an expiry; the relay-wide **`expiring_key.count`** gauge now counts environments with at least one expiring key. Docs, integration tests, and status builders/tests are updated accordingly. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f93060b. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
4 participants
Summary
Phase 4 of forward-porting concurrent multi-key support from v8 (#817) to v9. Stacked on #896; GitHub retargets it to
feat/concurrent-keys-v9as the stack merges.A credential payload that parses as JSON but cannot produce a usable accepted set is now refused before
MessageReceiver.Upsertrecords its version.The ordering is the whole point
Upsertdeduplicates by version. A refused payload that had already advanced the version would make LaunchDarkly's replay of that same payload a no-op, leaving the environment serving credentials it should have replaced with no way back short of restarting the process.TestReplayOfTheSameVersionIsAppliedAfterARefusalis the test that matters, and it is control-verified: move the validation back after theUpsertand it fails.What happens to a refused payload
The environment keeps the credentials it already had. The stream reconnects, because the service cannot learn that relay refused the payload and would otherwise send nothing further. Well-formed environments in the same put are applied as usual.
persistPutsubstitutes each refused environment's last-good cached entry before writing, so a bad payload does not drop those environments from the auto-config cache, or empty it when every environment in the put was refused. If the prior snapshot cannot be read there is no safe cache to assemble, so it is left untouched rather than rewritten from partial data.Backoff on repeated unusable events
The stream now moves to the extended retry delays after a second consecutive unusable event. This closes the second Medium from the review of #817: the malformed-payload reconnect goes through
stream.Restart(), which bypasses the retry strategy, so it reconnected on the short delays indefinitely.Escalating on the first event was the obvious implementation and it is wrong — it broke
TestStreamStatusRecordsInvalidDataError, and rightly so, since one corrupt event would have cost minutes of recovery. One bad event may be a single corruption and is worth asking again at once; a second means the reconnect brought the same payload back. The counter resets on any event relay can use.This applies to JSON-malformed events as well as credential-malformed ones, which is slightly beyond the ticket but the same defect class, and treating only one of them specially would be arbitrary.
Testing note
The last-good cache preservation is covered by a recording cache fake at the stream-manager level, rather than the Redis-backed test v8 added for it. That exercises the substitution logic without a Redis dependency, and
internal/autoconfigcachealready has its own store round-trip tests. Happy to add the integration-level version if it is wanted.Also removes
StreamManager.lastKnownEnvs, dead since #401 removed its last read in June 2024.Part of SDK-3202, under epic SDK-3188.
Note
Overview
Auto-config PUT/PATCH handling now validates environment credentials with
validateCredentialPayloadbeforeMessageReceiver.Upsert, so a refused payload never records a version that would make a replayed fix a no-op.Refused credentials keep the environment’s previous keys. A wholly unusable event still restarts the SSE stream; a partial PUT applies healthy environments, stays connected, and surfaces
INVALID_DATAwhile the stream remains VALID.persistPutwrites last-good cache entries for refused envs (and applies them in-process); cache reads that fail skip the write. Relay only callsReceivedAllEnvironmentswhen something is actually being served (including cache recovery).Stream status moves through
markValidWithError(generation-safe), and usable events canActivateProfilethe normal retry curve after extended backoff. Removes unusedlastKnownEnvs. Adds stream-manager and relay end-to-end tests for these paths.Reviewed by Cursor Bugbot for commit d93fd4a. Bugbot is set up for automated code reviews on this repo. Configure here.