feat: Add the FDv2 data system and expose it through configuration#310
Conversation
kinyoklion
force-pushed
the
rlamb/sdk-2186/fdv2-data-system
branch
3 times, most recently
from
9e8ec33 to
fff23d4
Compare
kinyoklion
force-pushed
the
rlamb/sdk-2186/fdv2-data-system
branch
from
fff23d4 to
6225462
Compare
…ata source Split the identify strategy into FDv1/FDv2 data managers so cache handling lives with each protocol. FDv2 no longer loads the cache at identify; the data system reads it through a real cachedFlagsReader and feeds it into the pipeline. A PayloadEvent basis flag, set by the orchestrator, gates both the valid status (moved out of handlePayload) and wait-for-network resolution, so cached flags apply without reporting a live connection. Offline is now a real pipeline mode (cache initializer, no synchronizer) that loads cache while the manager keeps the offline status. Adds ConnectionModeId.offline and FlagManager.readCached.
…ctory The held selector was discarded inside the data source factory by comparing context instance identity, which only held while the factory was invoked for every context change. Move the reset to the data manager's identify path, keyed on the context canonical key, so a context change clears the basis regardless of the active connection mode (including offline) and independent of factory invocation timing. The factory no longer tracks the context.
The override test asserted only that a DataSource was produced, which holds whether or not the override is honored. Assert that resolvedDefinition picks the override for the overridden mode and the built-in otherwise. Translating a definition's entries into concrete sources stays covered by the entry-factory tests. Adds a visibleForTesting accessor and the meta dependency it needs.
Flutter 3.22.3 pins meta to 1.12.0 through its SDK dependency (via connectivity_plus), so a ^1.16.0 floor broke workspace version solving on that toolchain while passing on the newer one. visibleForTesting has been in meta since well before 1.12.0.
…and selector Replaces the derived PayloadEvent.basis flag with the explicit signals it stood for. A cache load -- a full changeset with no selector -- is applied and resolves a cached identify, but does not mark the source valid or satisfy a wait-for-network identify. Network data carries a server selector and does both. An intent-none, or being offline, resolves a wait-for-network identify since no selector will arrive. This mirrors js-core's FDv2DataManagerBase minimum-data-availability handling. The wire-level basis query parameter is unaffected.
…ntext keys Identify now unconditionally clears the held selector, matching js-core's FDv2DataManagerBase (which resets the selector on every identify). The previous context-key comparison split the "which context is the selector for" invariant between the data manager (holding the key) and the data system (holding the selector), with nothing keeping the two consistent. Mode switches still keep the selector, since they reach the data source manager directly rather than through the data manager.
The note described the method by contrast to the FDv1 verbs and to a prematurely-valid bug that no longer exists, rather than the code as it is.
Drops the contrast with the removed context-instance inference and the stale "reset on a context change" wording; the data manager clears the selector on every identify.
It returns without completing when a wait-for-network identify has not yet seen fresh data, so the name should reflect that it is conditional.
setValid was gated on the change set carrying a selector, but an intent-none (the server confirming no changes) carries none -- so a healthy reconnect that reported no changes left the status stuck at interrupted. Gate status (and the wait-for-network identify) on whether the change set is server data -- a selector-bearing payload or an intent-none -- rather than a cache load (a selector-less full). Matches js-core and android, where any synchronizer response, including no-change, restores valid. Adds a regression test.
Three linked changes that finish the FDv2 data system in common_client: - Status: the manager marks the source valid on any applied change set while online (offline keeps its offline status). No selector inference -- a cache load and a no-change response both report valid, matching js-core. - Initialization: the orchestrator, which knows the source tier, emits a single InitializedEvent (selector-bearing initializer, initializer exhaustion with data or in a cache-only system, or the first synchronizer change set). The manager resolves a wait-for-network identify on it; a cached identify resolves earlier on the first applied payload. Replaces the fragile type+selector inference, and an FDv1-fallback first response now completes a fresh identify. - FDv1 fallback: a new FDv1 fallback synchronizer (an FDv1 poller whose responses translate to full, selector-less change sets tagged fdv1Fallback:false) is appended as the terminal, initially-blocked slot when a mode configures it. The orchestrator ignores a fallback directive once already on FDv1. The x-ld-fd-fallback detection was already present.
A fresh fallback synchronizer instance must not inherit a prior instance's ETag, which would let it receive a 304 for a connection it never made and keep stale data. The if-none-match header is per-requestor instance state.
Replace the requireFreshData bool with minimumDataAvailability, of type
DataAvailability { cached, fresh } -- matching js-core's cached/fresh
data-availability concept and identifier. FDv2DataManager maps
waitForNetworkResults -> fresh, otherwise cached. cached resolves on any
applied data; fresh waits for the orchestrator's InitializedEvent.
![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 using default effort and found 1 potential issue.
❌ 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 78bcd0c. Configure here.
| // without a selector. Otherwise the first synchronizer completes it. | ||
| if (_cacheOnlyDataSystem || dataReceived) { | ||
| _emitInitialized(); | ||
| } |
There was a problem hiding this comment.
Premature init on cache data
High Severity
At the end of _runInitializers, _emitInitialized() runs when dataReceived is true, not only for cache-only pipelines. Built-in polling and background modes have a cache initializer plus synchronizers, so a cache hit sets dataReceived before any synchronizer runs. That emits InitializedEvent early, so waitForNetworkResults / DataAvailability.fresh identify can finish on cached data alone instead of waiting for network.
Reviewed by Cursor Bugbot for commit 78bcd0c. Configure here.
There was a problem hiding this comment.
Checking this against the other implementations. The wording may be too strong as waiting is a best effort. It doesn't promise you will get a network result.
There was a problem hiding this comment.
I lean toward keeping this behavior as it seems like the correct balance of safety.
| // instead. | ||
| if (_minimumDataAvailability == DataAvailability.cached) { | ||
| _maybeCompleteIdentify(); | ||
| } |
There was a problem hiding this comment.
I would expect a comparison here like payload.dataAvailable >= _minimumDataAvailability. Then all paths through which the payload itself completes identify would happen here (and not only cached here, while fresh or exhaustion happens in InitializedEvent).
Having the payload include the data availability could also eliminate the need for the _cacheOnlyDataSystem boolean and associated special logic in the orchestrator.
It would also eliminate these situations of back to back signals.
_emitPayload(result);
_emitInitialized();
I think then the InitializedEvent indicates the orchestrator is in steady state, which is a way identify completes as well.
There was a problem hiding this comment.
We aren't directly driving the behavior 1 to 1 with a payload. But what types of signals we have over time.
Sometimes they are coupled, but other times they are independent. If payloads were always the direct transition, then that would work.
We get a payload, and we are waiting for "fresh" data.
We handle several more data sources and don't get any better.
The orchestrator then decides we are initialized. Independent of the payloads.
It would be directly handled in the orchestrator, but we actually have some situations which are outside its control, which is why it is handled at this level.
There was a problem hiding this comment.
Basically the cases for whenever they aren't paired, the cases where they are paired is technically redundant. Otherwise this state is already represented in the selector, which the payload already has.
2 participants
What this adds
The piece that ties the FDv2 building blocks together and turns them on:
FDv2DataSystem, the manager routing that applies FDv2 payloads, and the configuration that opts in. This completes the FDv2 work incommon_client— everything below the public Flutter API.FDv2DataSystemcomposes the data source factories theDataSourceManagerconsumes. It owns the state that must outlive any single orchestrator: the current selector and the context it belongs to. A fresh orchestrator is built per connection-mode switch and per identify; the selector survives mode switches (initializers are skipped when a selector is already held) but resets when the context changes, since a selector is specific to one context.buildFactories()returns factories for streaming, polling, and background — offline has no data source and is handled by the manager directly. Custom connection modes from config replace the built-in definitions by name.DataSourceManagerpayload routing. ThePayloadEventcase — a no-op placeholder in the orchestrator PR — now applies the change set throughhandlePayloadand completes the pending identify, the same wayDataEventdoes for FDv1.DataSystemConfig(providing it, even empty, opts into FDv2);LDCommonConfig.dataSystem;LDCommonClientconstructs anFDv2DataSystemand installs its factories whendataSystemis configured, otherwise the FDv1 sources run unchanged; and the new public types are exported.When
dataSystemis absent the FDv1 path is byte-for-byte unchanged, so existing behavior is unaffected.Testing
DataSourceManagertest: aPayloadEventis applied throughhandlePayload, marks the source valid, and completes identify (a dropped/no-op payload would leave identify hanging — this distinguishes the real routing from the placeholder).FDv2DataSystemtests:buildFactoriesexposes streaming/polling/background and not offline; each factory builds a fresh data source per call; a custom connection mode replaces the built-in definition.DataSystemConfigdefault (no custom modes).common_clientsuite passes;flutter_client_sdk(the dependent package) analyzes and tests clean against these changes.SDK-2186
Note
High Risk
Touches core flag acquisition, identify completion, connection-mode/offline behavior, and persistence read paths; incorrect timing could leave identifies hanging or serve stale flags, though FDv1 remains unchanged when
dataSystemis unset.Overview
Opts the SDK into FDv2 when
LDCommonConfig.dataSystemis set (even empty), exportsDataSystemConfig/ConnectionModeIdand mode-definition types, and keeps the FDv1 path whendataSystemis absent.FDv2DataSystembuilds per-mode orchestrator factories (streaming, polling, background, offline as a real cache-only pipeline), holds the selector across mode switches, and appliesconnectionModesoverrides.FDv2DataManagerclears the selector on each identify and mapswaitForNetworkResultsto cached vs fresh availability; FDv1 still loads cache at identify viaFDv1DataManager.DataSourceManagerappliesPayloadEventthroughhandlePayload, sets valid on applied online payloads (including no-change restores), does not promote valid in offline mode, and completes identify on first applied data (cached) orInitializedEvent(fresh).InitializedEventand orchestrator init semantics separate cache hits from network-ready state. Cache loads usereadCachedwithout touching the store until the pipeline applies payloads.Adds an FDv1 fallback synchronizer (FDv1 poll → FDv2 change sets) with loop guards when already on fallback.
Reviewed by Cursor Bugbot for commit 194eae7. Bugbot is set up for automated code reviews on this repo. Configure here.