fix: Honor x-ld-fd-fallback header in fdv2 initializer phase#381
Open
fix: Honor x-ld-fd-fallback header in fdv2 initializer phase#381
Conversation
Prior to this change, the Ruby SDK only inspected the x-ld-fd-fallback response header on FDv2 synchronizer responses. If an initializer received the header, the signal was silently dropped and the SDK would continue attempting subsequent initializers and FDv2 synchronizers rather than reverting to FDv1. The Initializer fetch contract now returns a FetchResult that pairs the existing Result<Basis> with a fallback_to_fdv1 boolean. The FDv2 data system branches on the new flag, applying any accompanying Basis before swapping the synchronizer list for the FDv1 fallback builder, so evaluations can serve the server-provided payload while FDv1 spins up. When no FDv1 fallback is configured, the data system logs and clears the synchronizer list, mirroring the synchronizer-triggered path. Update.revert_to_fdv1 is renamed to Update.fallback_to_fdv1 with an alias retained for backwards compatibility while FDv2 is in early access. A shared LaunchDarkly::Impl::DataSystem.fdv1_fallback_requested? helper replaces the duplicated header-string checks across the polling and streaming data sources.
The contract harness now treats the FDv1 Fallback Synchronizer as a
distinct field on the data system (DataSystem.FDv1Fallback) rather than
deriving it from the FDv2 synchronizer chain, and gates the directive
subtests on a new fdv1-fallback capability.
Wire the test service to match:
- declare the fdv1-fallback capability
- accept the new dataSystem.fdv1Fallback config field
- build the FDv1 fallback synchronizer from that field directly,
instead of inferring it from the last polling synchronizer
Also bump the contract-tests pin from v3.0.0-alpha.4 to v3.0.0-alpha.6
so the harness ships the new directive subtests.
Three contract-test failures surfaced by sdk-test-harness v3.0.0-alpha.6
shared a single underlying defect: the directive was honored in the unit
tests but silently dropped in the integration paths.
- Polling-initializer signal lost. HTTPPollingRequester#fetch downcases
response header keys before handing them to the data system, but the
fallback-detection helper was a case-sensitive Hash lookup against
the canonical mixed-case constant. Replace the lookup with a helper
that handles both plain Hashes and case-insensitive header
containers (e.g. HTTP::Headers from the http gem). Apply the same
helper to the env-id lookups so neither signal disappears against
a downcased map.
- Streaming-success directive dropped the payload. The on_connect
handler yielded OFF and stopped the stream the moment the fallback
header arrived, so the SSE handshake never produced a payload to
apply (Requirement 1.6.2). Record the directive as a pending flag
and let event processing continue; ride the signal on the next
Valid update so the consumer applies the ChangeSet first, then
transition to FDv1 and close the FDv2 stream.
- "No FDv1 fallback configured" branch didn't halt. When the
synchronizer loop received SyncResult::FDV1 and no FDv1 fallback
builder was wired, it incremented current_index past the end of the
builder list, where the existing wrap-around immediately reset it
to zero -- producing an infinite reconnect loop against the same
FDv2 endpoint that just delivered the directive. Transition the
data source status to OFF and break the loop instead, mirroring
Requirement 1.6.3(4).
Adds case-insensitivity unit tests so a future refactor cannot resilently
re-break the polling path, and includes coverage for the
HTTP::Headers-style case-insensitive container shape that the streaming
code paths actually receive.
- Unify response-header handling on Result. Result.success now accepts optional headers, HTTPPollingRequester / HTTPFDv1PollingRequester pass headers via Result.headers on both success and failure, and PollingDataSource consumes them through a single result.headers lookup. The dual "headers ride on Result.headers on failure / on the value tuple on success" shape is gone. - Drop the FetchResult/Result legacy guard in FDv2.run_initializers. All in-tree initializers produce a FetchResult; the data system can just call .result and .fallback_to_fdv1 directly. - Streaming: move @fdv1_fallback_pending out of instance state into a local closed over by on_connect and on_event. The directive arrives in the connect handshake but on_event has no access to those headers, so some state has to bridge the two callbacks; a local scoped to a single #sync invocation expresses that lifecycle correctly without leaking across reconnects. process_message is now the only place that constructs Update -- it accepts an optional fdv1_fallback_pending: kwarg and stamps fallback_to_fdv1 on the Update it produces for events that complete a payload transfer (TRANSFER_NONE or PAYLOAD_TRANSFERRED), so on_event can simply yield the Update and stop the stream when the directive rode along. Flip envid lookups to ||= so we skip the case-insensitive scan once envid is known (it never changes server-side). - Drop the deprecated revert_to_fdv1 alias on Update. FDv2 is in EAP and we don't need to preserve compatibility with the old name. - Comment cleanup: drop remaining spec-section/requirement citations while preserving the behavioral descriptions, and tighten the return fallback ? true : false to return fallback.
keelerm84
force-pushed
the
mk/sdk-2291/fdv1-fallback
branch
from
d91f0b6 to
8a708b3
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ 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 8a708b3. Configure here.
| client.on_event do |event| | ||
| begin | ||
| update = process_message(event, change_set_builder, envid) | ||
| if update |
There was a problem hiding this comment.
Stale fallback signal persists across SSE reconnections
Medium Severity
The fdv1_fallback_pending variable is set to true in on_connect when the fallback header is present, but is never reset to false on a subsequent reconnection that lacks the header. If the SSE connection drops before a payload is delivered and the client reconnects to a server that no longer signals fallback, the stale true value causes the next completed payload transfer to incorrectly trigger the FDv1 fallback. The comment claims it "cannot leak state across reconnects" but this is inaccurate—it persists across reconnections within the same sync invocation.
Reviewed by Cursor Bugbot for commit 8a708b3. Configure here.
| @change_set = change_set | ||
| @error = error | ||
| @revert_to_fdv1 = revert_to_fdv1 | ||
| @fallback_to_fdv1 = fallback_to_fdv1 |
There was a problem hiding this comment.
Missing promised deprecated alias for revert_to_fdv1
Low Severity
The PR description explicitly states that Update.revert_to_fdv1 is renamed to Update.fallback_to_fdv1 "with a deprecated alias kept for back-compat," but no alias_method :revert_to_fdv1, :fallback_to_fdv1 exists in either the public Interfaces::DataSystem::Update or the internal Impl::DataSystem::Update class. Any existing consumer relying on the revert_to_fdv1 accessor will get a NoMethodError at runtime.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8a708b3. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
X-LD-FD-Fallback: truenow apply any accompanying payload first, then the data system swaps the synchronizer list to the FDv1 Fallback Synchronizer (or transitions to OFF when none is configured). Synchronizer-phase behavior is unchanged.FetchResultsoInitializer#fetchcan pair the existingResult<Basis>with afallback_to_fdv1signal, and renamesUpdate.revert_to_fdv1toUpdate.fallback_to_fdv1(with a deprecated alias kept for back-compat). Polling and streaming detect the header on both success and error paths via a shared helper.fdv1-fallbackcapability and accepts a top-leveldataSystem.fdv1Fallbackconfig object, wiring it directly tofdv1_compatible_synchronizerinstead of inferring from the synchronizer chain. Bumps the contract-tests pin fromv3.0.0-alpha.4tov3.0.0-alpha.6.Mirrors the Go reference change in go-server-sdk#365 and contract-test wiring in go-server-sdk#368. Spec rationale lives in sdk-specs#155; the new harness suite is in sdk-test-harness#336.
Test plan
bundle exec rspec spec/: 996 examples, 0 failures.bundle exec rubocop --parallel: 183 files, no offenses.v3.0.0-alpha.6runs the new "FDv1 Fallback Directive" suite cleanly in CI.Note
Medium Risk
Touches core FDv2 initialization/synchronizer control flow and data-source header handling; mistakes could incorrectly halt the data system or miss/over-trigger protocol fallback.
Overview
Ensures the FDv2 data system honors the server-directed
X-LD-FD-Fallbackdirective during the initializer phase: initializer payloads are applied to the store, then the system switches terminally to the configured FDv1 fallback synchronizer (or transitions toOFFif none is configured), rather than continuing normal initializer/synchronizer failover.Introduces
Interfaces::DataSystem::FetchResultsoInitializer#fetchcan return both the existingResult<Basis>and thefallback_to_fdv1signal, renamesUpdate.revert_to_fdv1toUpdate.fallback_to_fdv1, and updates polling/streaming to detect the directive (andX-LD-EnvID) via new case-insensitive header helpers while keeping response headers inResult.Updates contract-test wiring to declare the
fdv1-fallbackcapability, to configure FDv1 fallback via a dedicateddataSystem.fdv1Fallbackblock (no longer inferred from synchronizers), and bumps the contract test harness version tov3.0.0-alpha.6; adds/adjusts specs to cover directive behavior on both success and error paths and during initialization.Reviewed by Cursor Bugbot for commit 8a708b3. Bugbot is set up for automated code reviews on this repo. Configure here.