Skip to content

feat: Honor the service's FDv1 fallback directive in the client - #623

Open
beekld wants to merge 2 commits into
bklimt/SDK-3035/client-fdv2-identifyfrom
bklimt/SDK-3036/client-fdv2-fdv1-fallback
Open

beekld wants to merge 2 commits into
bklimt/SDK-3035/client-fdv2-identifyfrom
bklimt/SDK-3036/client-fdv2-fdv1-fallback

Conversation

@beekld

@beekld beekld commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Honors the service's directive to move the client off FDv2 back to FDv1, and provides an FDv1 source to fall back to.

  • Adds an FDv1 adapter synchronizer that wraps an existing FDv1 client source so it can act as an FDv2 synchronizer.
  • On the fallback directive (a response header or a goodbye), applies any payload that arrived with it, then rotates from the FDv2 sources to the FDv1 fallback.
  • Retries FDv2 after the directive's TTL expires.

Note

Overview
Adds FDv1 fallback when the service tells the client to leave FDv2: a new FDv1AdapterSynchronizer exposes an existing FDv1 IDataSource as an FDv2 synchronizer (Init/Upsert/status → changesets and FDv2SourceResult).

FDv2DataSource now reacts to fdv1_fallback on initializer and synchronizer results: it still applies any accompanying payload, switches the synchronizer tier via SourceManager, starts the FDv1 fallback when configured, and schedules an FDv2 retry after the directive TTL (cancellable on close). If no FDv1 tier is configured, it reports interrupted with a clear message instead of staying on FDv2.

Unit tests cover the adapter lifecycle/conversions and orchestration (apply-before-fallback, FDv1 tier start, disconnect without fallback, TTL retry).

Reviewed by Cursor Bugbot for commit f0596a8. Bugbot is set up for automated code reviews on this repo. Configure here.

@beekld
beekld force-pushed the bklimt/SDK-3036/client-fdv2-fdv1-fallback branch from 4992afc to 9df0441 Compare October 3, 2026 01:21
@beekld
beekld force-pushed the bklimt/SDK-3036/client-fdv2-fdv1-fallback branch from 9df0441 to 468d47a Compare October 5, 2026 18:00
@beekld
beekld added this pull request to stack #627 October 5, 2026 18:00
@beekld
beekld removed this pull request from stack #627 October 5, 2026 18:05
@beekld
beekld added this pull request to stack #628 October 5, 2026 18:10
@beekld
beekld removed this pull request from stack #628 October 5, 2026 19:18
@beekld
beekld added this pull request to stack #629 October 5, 2026 19:24
@beekld
beekld force-pushed the bklimt/SDK-3036/client-fdv2-fdv1-fallback branch from 468d47a to 346c0d3 Compare October 5, 2026 23:25
@beekld
beekld force-pushed the bklimt/SDK-3036/client-fdv2-fdv1-fallback branch from 346c0d3 to f0596a8 Compare October 6, 2026 05:04
@beekld
beekld marked this pull request as ready for review October 6, 2026 16:53
@beekld
beekld requested a review from a team as a code owner October 6, 2026 16:53

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

❌ 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 f0596a8. Configure here.

active_conditions_.reset();
}
StartSynchronizers();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale shutdown aborts FDv2 retry

High Severity

OnFDv2RetryTimer closes the FDv1 adapter while its Next() is still in the orchestrator's WhenAny. That unblocks the old future with Shutdown, which is posted onto the executor after the new FDv2 synchronizer has already been started. OnSynchronizerResult then treats that stale Shutdown as belonging to the replacement and resets active_synchronizer_, so the SDK is left with no live source after the TTL elapses.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f0596a8. Configure here.

DataSourceStatus::DataSourceState::kInterrupted,
DataSourceStatus::ErrorInfo::ErrorKind::kUnknown,
kNoFDv1FallbackConfigured);
return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fallback status skips shutdown fence

Medium Severity

When a fallback directive arrives and no FDv1 tier is configured, the new path writes kInterrupted through status_manager_->SetState instead of PublishState. That skips GuardShutdown, so a late write after Close can overwrite the shared status that outlives this source.

Additional Locations (1)
Fix in Cursor Fix in Web

Triggered by learned rule: Client FDv2DataSource: basis is selector; GuardShutdown fences store

Reviewed by Cursor Bugbot for commit f0596a8. Configure here.

@kinyoklion

Copy link
Copy Markdown
Member

Note

Agent disclosure: this comment and the linked branch were produced by an AI agent (Claude Code) working for @kinyoklion, as part of a multi-agent review of this PR. The failure was reproduced and the fix checked by building and running the client unit tests locally (Debug, ASan/UBSan). Please review it with that in mind.

The FDv2 retry after the fallback TTL tears down the synchronizer it just started

This is the problem Bugbot raised here. It reproduces in a unit test, and it happens every time the TTL expires while the FDv1 tier is active.

What happens (line links are at f0596a8):

  1. While the FDv1 tier is active, its Next() is always pending in the orchestrator's WhenAny (RunSynchronizerNext).
  2. When the TTL fires, OnFDv2RetryTimer calls Close() on it. The adapter resolves the pending Next() with Shutdown straight away (fdv1_adapter_synchronizer.cpp), so the WhenAny continuation is posted.
  3. OnFDv2RetryTimer then calls StartSynchronizers(), which installs a fresh FDv2 synchronizer.
  4. The posted continuation runs OnSynchronizerResult(Shutdown). Nothing ties the result to the synchronizer that produced it, so the got_shutdown branch resets active_synchronizer_, which is now the new FDv2 synchronizer.

After that there is no synchronizer and no retry timer, and the status still reads VALID from the last FDv1 data. Flag updates stop silently until the next identify. #619 gives every built-in mode an FDv1 fallback, so once it lands this would happen on every TTL expiry.

The same root cause shows up in a narrower window too. If an FDv1 result has already been posted when the retry runs, it is applied after FDv2 has restarted, and it calls Next() a second time on the new synchronizer while the first call is still outstanding.

FDv2IsRetriedAfterTheFallbackTtl doesn't catch the main problem because its retried mock returns Shutdown immediately and the test only checks build_count_.

Branch with a test and a fix

rlamb/SDK-3036/fdv2-retry-stale-shutdown is two commits on top of this PR's head, so you can cherry-pick or merge them:

  • 7a0236c test: FDv2KeepsSynchronizingAfterTheFallbackTtl, which uses the existing fakes. On f0596a8 it fails: the retried synchronizer gets 1 of its 3 Next() calls, and its later data (flagC) is never applied.
  • 4617a80 fix: every time active_synchronizer_ is replaced, a generation counter is bumped. The RunSynchronizerNext continuation captures the counter, and OnSynchronizerResult / OnConditionFired drop results from an earlier generation. The same guard covers the in-flight FDv1 result case. The commit also sends the no-FDv1 disconnect status through PublishState instead of calling SetState directly (Bugbot's second comment).

With the fix, the full client unit suite passes (236 tests).

Another way to fix it is to put the TTL future into the same WhenAny as the synchronizer and its conditions, so there is only ever one chain. The generation guard seemed the smaller change, but either works.

A heads-up, not verified: the server SDK's FDv2DataSystem::OnFDv1RetryTimer (libs/server-sdk/src/data_systems/fdv2/fdv2_data_system.cpp) has the same structure (close outside the WhenAny, then reset on got_shutdown), so it may have the same problem.

This branch has not been deployed

No deployments
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.

2 participants