Skip to content

feat: Refuse a malformed credential payload without losing the environment - #897

Open
keelerm84 wants to merge 1 commit into
mk/SDK-3199/anchor-reanchorfrom
mk/SDK-3202/malformed-payloads
Open

keelerm84 wants to merge 1 commit into
mk/SDK-3199/anchor-reanchorfrom
mk/SDK-3202/malformed-payloads

Conversation

@keelerm84

@keelerm84 keelerm84 commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

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-v9 as the stack merges.

A credential payload that parses as JSON but cannot produce a usable accepted set is now refused before MessageReceiver.Upsert records its version.

The ordering is the whole point

Upsert deduplicates 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.

TestReplayOfTheSameVersionIsAppliedAfterARefusal is the test that matters, and it is control-verified: move the validation back after the Upsert and 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.

persistPut substitutes 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/autoconfigcache already 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 streaming now rejects environment payloads that parse as JSON but cannot build a usable credential set, without dropping the environment or poisoning version tracking.

Validation runs before MessageReceiver.Upsert on PUT and PATCH paths via validateCredentialPayload (BuildAcceptedSet). Refused environments keep their prior credentials; the stream restarts so LaunchDarkly can replay a fixed payload at the same version. Other environments in a mixed PUT still apply. persistPut merges refused entries from the previous cache snapshot on write (or skips the write if the prior snapshot cannot be read).

After two consecutive unusable events (bad JSON or bad credentials), reconnects switch to the extended SSE retry profile so short-delay spin stops; the counter resets on any usable event. Removes unused lastKnownEnvs. New stream-manager tests cover refusal, replay-after-refusal, partial PUT, and cache preservation.

Reviewed by Cursor Bugbot for commit a4a5024. Bugbot is set up for automated code reviews on this repo. 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.
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