fix: Honor x-ld-fd-fallback header in fdv2 initializer phase#381
Merged
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}
| @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.
jsonbailey
approved these changes
May 1, 2026
Cursor Bugbot flagged the closure-over fdv1_fallback_pending in
StreamingDataSource#sync as "stale signal across reconnects." After
review, the latched-and-never-reset behavior is intentional and
matches the Go and Python reference SDKs: the FDv1 Fallback Directive
is one-way and terminal, so a mid-sync reconnect whose response no
longer carries the directive must NOT cancel a directive seen
earlier. The previous comment overclaimed ("cannot leak across
reconnects"), which was misleading -- it was true across sync
invocations but not within one. Rewrite the comment to spell out the
latch semantics explicitly.
Also add three process_message unit tests that codify the
directive-stamping contract: TRANSFER_NONE and PAYLOAD_TRANSFERRED
Updates carry fallback_to_fdv1 when the kwarg is pending, and
intermediate events that return nil do not. These give a future
maintainer a clear test-failure signal if someone tries to reset the
latch on each connect.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes 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 c1f08f1. Configure here.
The poll method computes the fallback flag from response headers but the rescue clause hardcoded fallback_to_fdv1: false on its FetchResult. If an exception fired after the headers were read (for example, a malformed ChangeSet whose selector was nil triggering a NoMethodError on Basis construction), the server's FDv1 Fallback Directive was silently dropped and the data system would treat it as a generic transient error instead of engaging FDv1. Initialize fallback to false before the requester call so the rescue also has a defined value when the requester itself raises, then thread the computed fallback through the rescue's FetchResult so the directive survives the exception.
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
2 participants
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
Changes FDv2 initialization and sync control flow to react to a server-directed protocol fallback header; mistakes could cause premature OFF state or incorrect protocol switching in production SDK clients.
Overview
Honors the server-directed FDv1 fallback directive during FDv2 initialization. Initializers now return a new
FetchResultthat carries both the basisResultand afallback_to_fdv1signal, andFDv2#run_initializersapplies any successful payload before switching terminally to the configured FDv1 fallback synchronizer (or transitioning toOFFif none exists).Adds shared, case-insensitive header parsing helpers for
X-LD-FD-Fallback/X-LD-EnvIDand updates polling/streaming to use them (including latching fallback across streaming reconnects). RenamesUpdate.revert_to_fdv1toUpdate.fallback_to_fdv1, adjustsResult.successto optionally carry headers, and expands specs/contract-test harness wiring (newfdv1-fallbackcapability, dedicateddataSystem.fdv1Fallbackconfig, and contract-test pin bump tov3.0.0-alpha.6).Reviewed by Cursor Bugbot for commit 0ece86a. Bugbot is set up for automated code reviews on this repo. Configure here.